Skip to content

fix: switch to POST for executing jobs #324

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Nov 23, 2021
Merged

fix: switch to POST for executing jobs #324

merged 3 commits into from
Nov 23, 2021

Conversation

mediremi
Copy link
Contributor

@mediremi mediremi requested a review from a user November 18, 2021 16:29
@dhis2-bot
Copy link
Contributor

dhis2-bot commented Nov 18, 2021

🚀 Deployed on https://pr-324--dhis2-scheduler.netlify.app

@ghost
Copy link

ghost commented Nov 18, 2021

I'll assign this to @Mohammer5 as well as he's there tomorrow and I'm not. I'm happy to finish reviewing this next week if there's no rush, otherwise feel free to tackle this beforehand. The broad lines look good to me at a glance.

@ghost ghost requested a review from Mohammer5 November 18, 2021 17:01
Copy link
Contributor

@Mohammer5 Mohammer5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we should either prevent the modal from being closed while loading to ensure that a potential error can be shown to the user or show the error in an alert (which would be my preference as it'd be consistent with other apps)

@ghost
Copy link

ghost commented Nov 22, 2021

I think my recommendation would be this pattern: #325. I know it's verbose, but to me it seems wrong that we recommend from one lib to not set the query during runtime, and then in other areas implement workarounds to set the query during runtime. The engine was meant as an escape hatch so to me it seems like the stable solution in the meantime. Not ideal, but I think the semantics make more sense than us trying to subvert the recommended pattern from one of our other libs.

Couple of things that would probably be good to tackle later (seems out of scope for this PR):

  • Discuss @Mohammer5's suggestion, see whether an alert makes sense for any errors (though I prefer the in context noticebox more)
  • Update tests to use the CustomDataProvider instead of mocking
  • Remove the store abstraction (not at all related to this PR, but I really want to get rid of it)
  • Eventually I do think moving towards testing-lib would be nice. Enzyme is becoming a bit of a chore
  • Add functionality to the app-runtime that allows us to deal with this pattern in a more convenient/terse manner

Copy link
Contributor

@Mohammer5 Mohammer5 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the thing that I found will be deferred, I'll approve this issue

@ghost
Copy link

ghost commented Nov 23, 2021

I'm still wondering about the question I asked wrt. the choice for useState (see review note above). Also, I think we can remove the mocking from tests that don't click the 'run job' button (like in #325). After that's addressed this should be good to go as far as I'm concerned as well.

@mediremi
Copy link
Contributor Author

I think we should either prevent the modal from being closed while loading to ensure that a potential error can be shown to the user

I'll implement that 👍

I'm still wondering about the question I asked wrt. the choice for useState (see review note above)

Since it seems like there's a strong possiblity that resource parameterisation might be implemented in app-runtime this week, I'm happy to keep this PR open until then.

@ghost ghost self-requested a review November 23, 2021 13:43
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since it seems like there's a strong possiblity that resource parameterisation might be implemented in app-runtime this week, I'm happy to keep this PR open until then.

I'm good with this being merged with the useState based fix.

Only thing I think I'd do is remove the mocking from tests that don't need it. And setting the useState directly (i.e. with an object). If you want to be completely robust I can imagine that you'd want the dynamic query to respond to changes in the id prop, though practically speaking I don't think that's necessary as the app doesn't do that atm.

Feel free to merge if you're happy with the PR 👍, looks good to me!

@mediremi mediremi merged commit c062ffa into master Nov 23, 2021
@mediremi mediremi deleted the DHIS2-12156 branch November 23, 2021 13:50
dhis2-bot added a commit that referenced this pull request Nov 23, 2021
## [99.9.10](v99.9.9...v99.9.10) (2021-11-23)

### Bug Fixes

* switch to POST for executing jobs ([#324](#324)) ([c062ffa](c062ffa))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 99.9.10 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants