feat(reminders): opt-in push notifications for due flashcards - #69
Conversation
…w guard) - Backend: wrap flashcard reschedule + review log in transaction.atomic; require authentication on DeckViewSet (study_queue was 404 not 401) - Study screen: per-card SM-2 interval hint previews instead of the static 1d/3d/7d legend; don't advance when review POST fails (null fallback); completion timestamp captured in handler; honor mode param - Deck list: guard malformed last_studied_at timestamps - E2E: fix study_queue/.json typo
- Backend: Device.notify_study_due flag (default off) + migration, Device.notify_study_reminder (digest-only safe), send_study_reminders command (single deck deep-links to its study session, several decks aggregate to the deck list, no-due users skipped) - Frontend: 'Study reminders' toggle in Settings + onboarding parity, study_due push-tap deep link to the due study session - Tests: 8 backend command tests, device API + settings + deep-link tests
|
The opt in needs to be on a screen accessible to teens, as they are likely the ones who need it. Probably should give them settings menu and only pin gate the settings that need it. |
Parent Settings (/parent/settings) is PIN-gated and hidden from teen-delegated sessions, so teens could never reach the Study reminders toggle. Add a teen-safe /settings screen exposing only notify_study_due (read-modify-write preserves parent flags; disabled with an explainer while digest-only suppresses it) and route the drawer Settings item there for teens (/parent/settings unchanged for parents).
|
Addressed the teen-accessibility feedback in 4652904:
@copilot review this pull request |
Reviewed the follow-up and found two blocking issues: teen sessions can still change the parent-only device flags by sending a crafted PUT to |
01-teen-login asserted teens never see a Settings drawer item; with the teen-safe /settings screen they now do (study reminders only, no parent surfaces). Guard test gains an explicit /settings kid-safe case.
Copilot review found two blocking issues with the teen-safe opt-in: - Teen-delegated JWTs authenticate as the parent user, so IsOwner alone let a crafted PUT flip parent surveillance flags (oversight evasion). DeviceViewSet now restricts teen writes to notify_study_due only: any other changed field -> 403, DELETE -> 403, and teen creates force parent-only flags to model defaults. Benign full-object round-trips (identical values, as the teen client sends) still pass. - send_study_reminders grouped by parent user, so a single-deck body naming the deck could push a sibling's deck name to a teen device. Bodies are now counts-only (deep-link routing unchanged; deck APIs stay profile-scoped so sibling deep links 404).
|
Both blocking issues fixed in 3ec335e — thanks for catching them:
Verification: new + neighboring backend suites green (16 + 105 passed), @copilot review this pull request again |
Re-reviewed 3ec335e. The teen-write restriction looks good now, but there’s still one blocking issue: |
A single-deck push can name a deck this profile can't load (sibling deck on a shared device: deck APIs are profile-scoped, so the queue comes back empty). Tag push taps with source=reminder; an empty queue on such a tap replaces to the deck list (due badges show what's due) instead of stranding the user on Nothing-due for a push that promised cards. Plain navigation keeps the celebratory empty state.
|
Follow-up on the remaining concern (account-level delivery) in 65a8a0d:
Verification: jest suites green (22 passed incl. new cases), @copilot review this pull request again |
Re-reviewed 65a8a0d and found one remaining issue: the new reminder fallback also triggers when |
fetchStudyQueue() swallowed every failure to [], so the reminder-tap fallback could not tell 'sibling/inaccessible deck' from 'offline/5xx' and would silently redirect on a failed load. It now resolves null on failure (same nullable pattern reviewFlashcard already uses): the study screen only redirects reminder taps on a confirmed-empty queue and keeps the previous empty state on null. Callers updated (study-all-anyway null-guards); the apiClient test mock gains real error classes so the shared request() failure path works under test.
|
Good catch — fixed in 3866bc7. Verification: 6 related suites green (52 passed), @copilot review this pull request again |
Re-reviewed 3866bc7 and found one remaining issue: reminder taps for sibling/inaccessible decks still fall through to the false “Nothing due” state, because |
Splitting 'nothing loadable' three ways: 404 (deck inaccessible to this profile, e.g. a sibling deck on a shared device) resolves [] so reminder taps redirect to the deck list; offline/5xx resolve null so reminder taps keep the empty state instead of silently redirecting on a failed load. Genuine 200-empty is unchanged.
|
Right again — the null lumped 404 in with real failures. Fixed in c584e8e by splitting three ways in
Tests: API-level cases for all three branches; study-screen branch tests unchanged and green. Suites green, @copilot review this pull request again |
Re-reviewed I do still see one remaining issue in the final PR state: token-based device lookup still returns deleted rows. Both the teen settings flow and onboarding reuse Concretely:
So the new create guard fixes fresh crafted creates, but an existing deleted row with the same token is still unrecoverable through the normal re-registration path. |
Token lookups excluded dead rows, but creates then 400'd on the unique token (validator saw deleted rows) while updates PUT the dead row back unchanged — a re-registering device stayed deleted and reminder-less forever. The serializer drops the table-wide UniqueValidator; creates revive the soft-deleted row in place (same device_id, teen flag rules apply) and only reject live duplicates with a 400.
|
Fixed in f17a077. Token lookups (teen and parent) now exclude soft-deleted rows, and creates revive a soft-deleted row with the same token in place — same device_id, teen flag rules applied on revive — instead of 400ing on the unique validator or PUTing the dead row back unchanged. Live duplicates still 400 as before. Tests: lookup exclusions, parent + teen revive, live-duplicate 400. 19 + 41 green, ruff clean. @copilot review this pull request again |
Re-reviewed I do still see two remaining issues in the overall PR state:
|
Reviewed
|
|
Copilot, please review the latest commit f17a077 (soft-deleted device revive on re-registration) and confirm whether anything is still outstanding on PR 69. |
…moves - get_object scopes to live rows: a cached device_id for a dead row now 404s instead of round-tripping (and preserving) deleted_at while the device stays out of lists and delivery. Recovery is via create-revive. - perform_update replaces the dropped table-wide UniqueValidator: moving a live device onto another live device's token is a clean 400, not a DB-constraint 500.
|
Both fixed in 42a5c1d:
Tests: detail-hides-deleted (parent + teen GET, parent PUT stays deleted) and live-token-move 400. 21 + 51 green, ruff clean. @copilot review this pull request again |
Re-reviewed I do still see one remaining issue in PR 69: |
Removed profiles must not contribute counts, profile_id payloads, or trigger pushes.
Re-reviewed I do still see one remaining issue on PR 69: the teen update guard’s |
current == incoming.lower() in (...) parsed as a chained comparison,
so unchanged string booleans ('true' from form-encoded PUTs) never
matched and wrongly 403'd. Covered by a multipart regression test.
Re-reviewed I don’t see anything else outstanding on PR 69. |
Co-authored-by: tpaulshippy <3137263+tpaulshippy@users.noreply.github.com>
Follow-up to #49: study-due push reminders
Base:
feature/roadmap-07-spaced-repetition(stacked; includes one review follow-up commit from the #49 review: atomic review, truthful hints, null-review guard).What changed
Device.notify_study_dueopt-in flag (default off, migration 0055),Device.notify_study_reminder()(digest-only devices stay digest-only), newsend_study_reminderscron-friendly command — single due deck deep-links to its study session, several decks aggregate to the deck list, users with nothing due are skippedstudy_duepush-tap deep link opens the due study session (or deck list when several decks are due)/settings) with only the Study reminders opt-in — parent surveillance flags stay behind the PIN at/parent/settingsand are preserved untouched on save; the drawer routes teens to/settings, parents to/parent/settingsnotify_study_due(anything else → 403, DELETE → 403, creates force parent-flag defaults), teen device lists are token-lookup-only, and teen detail routes are UUID-only; reminder copy is counts-only (no sibling deck names) and reminder taps fall back to the deck list when the tapped deck has nothing loadable for that profileDemo
python manage.py send_study_reminders(schedule a few times a day):Evidence
pr69-study-reminders.mp4