-
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: add edit sequence route #550
Conversation
3f74f67
to
e085a9c
Compare
🚀 Deployed on https://pr-550--dhis2-scheduler.netlify.app |
53d142f
to
2e1a254
Compare
86f61cb
to
f920404
Compare
15aa5b6
to
340ba49
Compare
7affec5
to
c356e7a
Compare
c356e7a
to
76a4321
Compare
b7b9d36
to
0d5df1b
Compare
8280edc
to
30ae67e
Compare
30ae67e
to
3618816
Compare
37d4c63
to
9a1c5c2
Compare
Btw. @tomzemp, for testing I recommend debug/dev. Hella found out that 2.40 has a bug that has been resolved in dev. |
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.
Tested successful on version 2.41
Also @tomzemp, if you want to test the UI:
And then you'll have a new sequence to edit |
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 good! I don't have many line-by-line comments, but a couple of things I noticed / had questions about:
- Clicking edit for a sequence from the overview scheduled jobs list sends you to the edit job page, rather than the edit-sequence page. I assume that's going to be changed later?
- the
useJobScheduleById
hook only seems to be used for the job sequences, so would it make sense to rename touseJobSequenceById
? - the
useJobSchedules
hook is reassigning the id of the first job to the overall schedule (I understand this is a workaround for the fact that the backend isn't creating an id for the overall sequence/schedule), but this has the effect of making schedules with only one job accessible from the edit-sequence page. Maybe ifuseJobScheduleById
becomes more explicitly for sequences, it should have logic to return an error if the sequence only has one job (in which case, it wouldn't be a sequence but a job)?
Thanks!
Definitely. When updating the list view I'll update the edit button for sequences as well.
Yeah the wording is a bit messed up atm. I created this issue for it: https://dhis2.atlassian.net/browse/DHIS2-15671. I could tackle that before merging.
Yeah this is a bit confusing conceptually. So the schedules are a mix of jobs and queues. A schedule with one item in the sequence is a job, and a schedule with multiple items in the sequence is a queue. Conceptually schedules are there for both jobs and queues. The idea being that it simplifies displaying both types in the overview. I've collected this and other api quirks I've encountered so far here: https://dhis2.atlassian.net/browse/DHIS2-15669. Let me know what you think of that. |
Thanks for the review @helladawit & @tomzemp |
# [101.2.0](v101.1.6...v101.2.0) (2023-08-09) ### Features * add edit sequence route ([#550](#550)) ([d5fa50c](d5fa50c))
🎉 This PR is included in version 101.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
https://dhis2.atlassian.net/browse/DHIS2-15668
This adds the scheduler edit sequence route. For testing:
http://localhost:3000/#/edit-sequence/{sequence-id}
.Note that the some of the patterns used are things we might want to refactor later. I've stuck with the existing approach to maintain the consistency of the code until then.
Todo
Sequence: {name}
Backend
Follow up