Repository navigation
Conversation
mudabs
left a comment
There was a problem hiding this comment.
Thank you, Olivia, for adding the medication reminder API, controls, localization, and test coverage.
Please address these items before merge:
- Make the selected reminder time drive the actual due reminder. The current logic only checks prescription.next_dose, ignores reminder_time, and never triggers for prescriptions created without next_dose.
- Define the reminder schedule and timezone behavior, then add regression tests where reminder_time differs from next_dose and where next_dose is null.
- Add the missing Arabic translations for reminderEnabled, doseReminderDue, and dismissReminder.
- Display reminderEnabled when a reminder is active instead of the enableReminder action label.
- Add frontend coverage for enabled, paused, disabled, empty, and scheduled reminder states.
mudabs
left a comment
There was a problem hiding this comment.
Thank you Olivia for improving reminder scheduling, localization, and frontend coverage.
Please address these items before approval:
-
Resolve the SonarCloud quality gate failure. New-code duplication is 4.1%, above the required 3% limit. Refactor the duplicated reminder logic or test setup and confirm SonarCloud passes.
-
Fix the partial enable failure flow. If
createRemindersucceeds butupdateReminderfails, retain or reload the created reminder so retrying updates it instead of attempting another create and receiving a conflict. -
Add a regression test covering create success followed by update failure, then verify that retrying recovers successfully.
All other CI checks pass. Please push the fixes and request another review.
There was a problem hiding this comment.
Thank you Olivia for addressing the reminder scheduling, localization, retry handling, and test coverage feedback.
PR #230 is now merged, but this branch has not been rebased onto the updated main. GitHub reports merge conflicts, and the diff still includes the medication adherence implementation that is already in main.
Please rebase this branch onto current main, resolve the conflicts, remove the duplicate adherence changes, and keep only the reminder functionality and its tests. Then push the updated branch, confirm all workflows pass, and request a fresh review.
b96a861 to
662b76a
Compare
mudabs
left a comment
There was a problem hiding this comment.
Thank you, Olivia, for addressing the earlier scheduling, localization, retry, SonarCloud, and rebasing feedback.
Please address one remaining API validation gap: PATCH /prescriptions/{id}/reminder currently accepts reminder_time: null, which can persist an invalid value and cause response validation errors. Reject explicit null values and add a regression test.
Please also request a fresh review after the fix. The branch is based on current main and all CI checks pass, but the PR remains blocked by the existing changes-requested status. Please confirm in the PR that reminders are currently in-app only and do not send background, push, or email notifications.
|



Summary:
Adds medication dose reminders building on the medication adherence functionality from issue 169. The reminders are in-app only, they do not send background, push, or email notifications.
Changes Made:
Closes #171