Skip to content

Conversation

@Jo-Chris
Copy link
Member

@Jo-Chris Jo-Chris commented Feb 7, 2025

Bugfix Changelist:

  • fixed a bug that reminded THREE_DAYS users every 24 hrs
  • fixed a bug that put users who are off for more than 24 hrs (or even 50, 100, or 400 hrs) in the wrong TimeSpan. No matter how long off a user was, if he has never been reminded he always needs to be sorted into the ONE_DAY bucket, otherwise, he does not receive up to 3 notifications.
  • fixed lots of false tests
  • fixed a bug where users would not have received their final notification, as the last bucket was misconfigured

Test Changelist:

  • Added bucket tests for 1 / 3 / 14 days buckets with happy path, edge case and null tests cases
  • Updated some edge cases (this is most likely where we will always push more, but the basic logic seems to work now fine)

@wsdt
Copy link
Member

wsdt commented Feb 7, 2025

sick work @Jo-Chris ! does that mean it is ready now?

);
/** @DEV Edge case: if a user has never been reminded he is always in the 24 hrs bucket */
if (!user.lastDateModeReminderSent) {
return TimeSpan.ONE_DAY;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Do we also have test cases for users that:

  1. Have been reminded e.g. 2 months ago and now are in ghost mode? Meaning they should be in 24h bucket again

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This Edge case above is only if something is in ghost mode for 2 months and has never been reminded (for whatever reason). Checking the other case you mentioned, good call!

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Q: Do we also have test cases for users that:

  1. Have been reminded e.g. 2 months ago and now are in ghost mode? Meaning they should be in 24h bucket again

Well, they shouldn't (currently) - because we do not have a set-back logic. If you've been reminded the last time 2 months ago, that would mean you have received all the 3 notifications back then (from one day to 14 days off) and then the algorithm will not consider you, as you probably removed the app - somewhat (max 3 notifications).

So we'd need to reset the lastDateModeReminderSent to re-start that cycle.

Copy link
Member

@wsdt wsdt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Jo-Chris
Copy link
Member Author

Jo-Chris commented Feb 7, 2025

Commit 9cebdd0 restarts the cycle without having to reset the lastDateModeReminderSent to null.

Will add 1-2 more tests after lunch, but very confident now.

@wsdt
Copy link
Member

wsdt commented Feb 7, 2025

Commit 9cebdd0 restarts the cycle without having to reset the lastDateModeReminderSent to null.

Will add 1-2 more tests after lunch, but very confident now.

nice!! sick work Chris. pls let me know once ready to deploy.

@wsdt wsdt merged commit ab6b9ce into main Feb 8, 2025
1 check passed
@wsdt wsdt deleted the jc/cron-job-tests branch February 8, 2025 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants