-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat: incorporate job queue in job list #571
Conversation
🚀 Deployed on https://pr-571--dhis2-scheduler.netlify.app |
2bbc56f
to
52e964d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great 🙌! (FYI: I've done a QA review, so I haven't looked at the code)
I agree with the points in your notes. Particularly:
- I find the deletion (/removal of jobs) slightly confusing and how it adds back the jobs without the original scheduling
- I always got queues created in disabled mode, and that didn't really make sense to me as a user (who hasn't read the documentation for job scheduling 🤡).
One thing I would suggest changing before merging
The language in the cancel editing dialog is a bit confusing:
When I first opened the app, my UI language was in French, and the French was (slightly) mistranslated such that it was suggesting that you were deleting the form (I guess the translation memory is picking up a translation from some other app where this would be the correct meaning).
Either way, I think the main dialog might be clearer if it said something like Do you want to discard these changes?
(i.e. not discard the form). Also it's a bit confusing to me with the changed-my-mind button for this dialog being labeled Cancel
, since you get to the dialog by clicking Cancel
(so maybe you would click Cancel
again in the dialog, thinking you were confirming your choice). Maybe the Cancel
button within the dialog could be Keep editing
or something similar?
Other things I noticed (only the first point is related to this PR)
- I find it slightly confusing that the job type is
Queue
for queues but the specific type for job (like maybe it should beJob: {{type}}
? - some of the translations are missing in. the app. The ones I noticed were already missing (job types (from backend) and the cron labels), so I don't think this a new issue, but because of that I didn't review new translation coverage.
- I noticed the backend wouldn't let me save two jobs of the same type/cron expression even if the details are different (specifically I got this when creating jobs to run different data integrity checks). Not sure if you know if that's a known issue.
- It seemed that the backend stopped me from saving jobs with very long names (but then just returned generic 500 error). I guess it would be good to figure out if there's a limit and then add a validation on the input?
@tomzemp Ah yeah, good point. I've updated the text a little. I'm not too sure about changing the button text as well. I see your point, but I feel that the primary and secondary button color also signal what will happen, as that pattern is quite common (as in, primary color: destructive because it's red, and proceed, secondary color: cancel what the modal is suggesting). Relying on the previous context for their labels gets a little confusing and verbose (I tried out a couple other texts). Maybe the original text is to blame. Maybe the "Cancel" button in the edit form should be called "Discard changes". That way the modal can stay simple and we won't have a repeated "Cancel" label. What do you think? (https://dhis2.atlassian.net/browse/DHIS2-16413)
Yeah good point as well. I'll create a separate issue for it and take a look at it with Joe. (https://dhis2.atlassian.net/browse/DHIS2-16414)
Yup, this happens often. Strings from the backend need to be added to the app manually.
Same type and same cron? Yes I think I've noticed that as well. I'm not sure if it's intentional. I suspect it is, but I'll double check. (https://dhis2.atlassian.net/browse/DHIS2-16415)
Yeah I agree. I'll check and create an issue for it. (https://dhis2.atlassian.net/browse/DHIS2-16416) |
# [101.3.0](v101.2.8...v101.3.0) (2024-01-03) ### Features * incorporate job queue in job list ([#571](#571)) ([204173e](204173e))
🎉 This PR is included in version 101.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
closes #575
closes #576
closes #577
closes #578
https://dhis2.atlassian.net/browse/DHIS2-14749
Todo:
Create issues for:
cursor: pointer
to transfer options (https://dhis2.atlassian.net/browse/DHIS2-16411)Backend:
Deleting a queue does not result in the jobs being available from the scheduler endpoint again, but the names of the jobs are still reserved, so it seems they're still present somehow. (Fixed and available on debug dev)Toggling a queue on or off ejects the first job from the queue (Patch bug, new endpoint available now)