-
Notifications
You must be signed in to change notification settings - Fork 49
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
Schedule entry number cleanup #4600
Schedule entry number cleanup #4600
Conversation
We don't need to sort because all schedule entries of the period are fetched when printing is started, and the schedule entries of a period are ordered correctly when fetching from the API. This might become relevant again far in the future if we introduce more offline functionality. For now, the sorting was incorrect anyways, because it assumed that the scheduleEntryNumber was unique per day, which it isn't (there can be multiple numbering schemes leading to multiple of the same scheduleEntryNumbers on the same day).
⛔ Feature branch deployment currently inactive.If the PR is still open, you can add the |
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.
We have 2 more places, where we sort scheduleEntries. Do we want to remove this sorting as well for consistency?
https://github.com/ecamp/ecamp3/blob/devel/pdf/src/campPrint/story/StoryDay.vue#L44
https://github.com/ecamp/ecamp3/blob/devel/frontend/src/components/story/StoryDay.vue#L83
ee33de4
to
fb85a49
Compare
Nice catch! I removed the additional sorts. It's not exactly the same case in the frontend, but as per our convention, the story display is responsible to reload the schedule entries when it is mounted. |
ba3d40e
to
fb85a49
Compare
This snapshot was flawed because of the wrong sorting before. There are some schedule entries with numberingScheme 'A' in the store data, and these were previously incorrectly sorted: dayNumber scheduleEntryNumber number startTime 1 1 1.1 08:00 1 2 1.2 09:00 1 1 1.A 10:00 The last one was previously sorted before the second one, which is incorrect.
ff237b4
to
7402cc1
Compare
7402cc1
to
1f11424
Compare
I noticed some minor errors in the description and usage of the
scheduleEntryNumber
field on schedule entries, while writing up #4597.We don't need to sort because all schedule entries of the period are fetched when printing is started, and the schedule entries of a period are ordered correctly when fetching from the API. This might become relevant again far in the future if we introduce more offline functionality.
For now, the sorting was incorrect anyways, because it assumed that the scheduleEntryNumber was unique per day, which it isn't (there can be multiple numbering schemes leading to multiple of the same scheduleEntryNumbers on the same day).