Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix reminders for single digit hours (00:00-09:00) #1663

Merged
merged 5 commits into from Aug 9, 2018
Merged

Fix reminders for single digit hours (00:00-09:00) #1663

merged 5 commits into from Aug 9, 2018

Conversation

ghost
Copy link

@ghost ghost commented Aug 6, 2018

Saving a default reminder time using the web frontend saves that time in 24-hour format with leading zero. (At least the standard Docker deployment does.)
However, this time was compared to the current hour without leading zero. Therefore, all reminders between 00:00 and 09:00 were never sent on my deployment.

This PR changes the current hour formatting to 24-hour with leading zero, and adjusts the associated tests to consistently set the default hour with leading zero, which fixes the aforementioned problem.

Feel free to reject this PR if this problem isn't actually present on platforms other than Docker. Still, making all tests associated with reminders, notifications and such consistent would still be a sensible goal IMO.

Checklist

Before submitting the PR

  • Read the CONTRIBUTING document before submitting your PR.
  • If the PR is related to an issue or fix one, don't forget to indicate it.
  • Make sure that the change you propose is the smallest possible.
  • [n/a] Screenshots are included if the PR changes the UI.
  • [n/a] If you change the UI, make sure the user experience is consistent with the current interface.

Code-related tasks

  • Tests added for this feature/bug.
  • [n/a] Impact on the seeders.
  • [n/a] Impact on the API.

If the code changes the SQL schema

  • [n/a] Impact on account export.
  • [n/a] Impact on importing data with vCard and .csv files.
  • [n/a] Impact on account reset and deletion.

Other tasks

Christopher Zentgraf and others added 5 commits August 7, 2018 00:26
Fix reminders not firing for single-digit hours
918a26c
Prepend leading 0 to all tests using default reminder time
2f59a42 
This fixes the discrepancies present between the different reminder tests.
Change timezone for test_user_should_not_be_reminded_because_timezone…
7ba60ee 
…_is_different

Europe/London is UTC+0, which might not satisfy
the different timezone criteria (at least for me, it doesn't).
Add CHANGELOG entry
2f211c6
@asbiin
Merge branch 'master' into 2018-08-07-fix-reminders-for-single-digit-…
3ddef5a 
…hour
@asbiin
Copy link
Member

asbiin commented Aug 9, 2018

Thanks so much @TheZenti !
Actually, this could fix some not sent reminders ...
I wrote this script to set every hours with leading zero. But I missed the fix on isTheRightTimeToBeReminded function.

Cc @djaiss

@asbiin asbiin merged commit d855d52 into monicahq:master Aug 9, 2018
@djaiss
Copy link
Member

djaiss commented Aug 9, 2018

@TheZenti you might have finally found a bug that I've been trying to find the cause for a few weeks now. Huge 👍 .

@ghost
Copy link
Author

ghost commented Aug 10, 2018

Happy to help!

@ghost ghost deleted the 2018-08-07-fix-reminders-for-single-digit-hour branch August 10, 2018 09:19
@github-actions
Copy link

github-actions bot commented Feb 4, 2021

This pull request has been automatically locked since there
has not been any recent activity after it was closed.
Please open a new issue for related bugs.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Feb 4, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@asbiin @djaiss