edit notification fixes: picker handles 24h and deadlines can be set #619
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
now handles 12h mode and 24h mode, setting and interpreting the hours, minutes, and period properly. fixes beeminder#172
The same calculation was being used for computing alertStart and deadline offsets. Deadline however can be - or +, - for times between 07:00 and 23:59 and positive for times between 00:01 and 06:00. This resulted in invalid values being sent from app to the server for many of the deadline possibilities and thus the backend replying with an error code.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
Edit Notifications contained several logical errors around 12h / 24h mode conversions. In 24h mode, the picker could display the wrong values.
Furthermore, deadlines were being calculated incorrectly from the picker. The backend was rejected these, leaving the user with a mere 422 error message and unable to set the deadline to a range of deadlines in the app.
This Merge Request fixes these conversions and fixes the deadline calculation.
For UI changes including screenshots of before and after is great.
before
12h mode
deadline of 11:59pm, tapping it shows 12:00 AM in the picker
24h mode
deadline of 23:59, tapping it shows 00:59 in the picker
setting any 'before midnight' deadline fails
after
12h mode
deadline of 11:59pm, tapping it shows 11:59 PM in the picker
24h mode
deadline of 23:59, tapping it shows 23:59 in the picker
setting also 'before midnight' deadline succeeds
Validation
Ran app in 12h and 24h modes in simulator.
Used the picker for before noon and after noon times and also times between 11pm and 12am.
Also, updated a goal's deadline both in 12h mode and in 24h.
Note
This MR does not address several other issues with these edit notification screens: data shown can quickly become out of sync with UI components on the same screen, can also become out of sync with the values the backend has; Elsewhere the app uses goal.slug but notification uses the goal.title; general lack of feedback about data being sent/saved
fixes #172
fixes #204