test(api): add preferred-name regression and privacy tests - #118
Open
Shaashwat3 wants to merge 538 commits into
Open
Shaashwat3 wants to merge 538 commits into
Shaashwat3 wants to merge 538 commits into
Conversation
…y-integration-prs-20260826 ci: add weekly integration PR automation
…received feat(notifications): email on portfolio submission receipt
…rker feat(notifications): queue notification emails with Sidekiq
…ns-isolation-pr-20260824 test(notifications): cover push settings for signed-in and anonymous callers
#43 landed on the base and moved email off the request path, so NotificationService.deliver_email no longer exists and this branch's new deliver method was calling it. The two changes compose without either giving anything up, because they end up guarding the same thing by different means: - Email is queued by Notification's after_commit hook, on create. A reserve that hits the unique dedupe index returns the existing row, creates nothing, and therefore queues nothing. The index is the dedupe guard for email. - deliver keeps its lock and its delivered_at guard for push, which has no equivalent database constraint. So deliver no longer fans out email and delivered_at now means push delivered. Said so in the comment, because a reader who assumes delivered_at covers both channels would draw the wrong conclusion. Three tests needed updating for the same reason, all of them asserting inline delivery counts: - test_dedupe_key_delivers_only_once now asserts one queued job, drains, and asserts one delivery. - test_failed_channel_delivery_is_retried_at_least_once asserted two emails after a push failure and a retry. One is correct, and better: the retry re-delivers push only, so a failed push does not put a duplicate email in the student's inbox. Asserting the job count makes that explicit. - test_reusing_an_abbreviation_for_a_new_task_still_notifies calls the job directly instead of through run_job, so it now drains the mail queue itself. 52 runs, 333 assertions, 0 failures across notification_service, notification_email_job, notification_new_task, notification_task_submitted, notification_tutorial and notification_group.
…s-group-email-link-pr-20260824 fix(notifications): link group membership emails
#43 landed on the base and moved notification email onto Sidekiq, so every assertion in this branch that read ActionMailer::Base.deliveries immediately after notify was asserting the old inline behaviour. Took the base version of the file and re-added the three new tests against the queue. Two things the rewrite turned up. Draining has to happen before the preference is flipped. NotificationEmailJob re-reads the category when it runs, which #43 added deliberately so a preference switched off after queueing is still honoured. Leaving the job queued across user.update! therefore tested the worker's late re-check rather than the gate in notify, which is what these tests are about. They now drain and assert one delivery first, then flip, then assert nothing new is queued and nothing new is delivered. Both halves of 'in both directions' are real. The extension test named the event extension_decided. Nothing raises that. ExtensionComment#assess raises type: 'extension', event: 'extension_assessed' (app/models/comments/extension_comment.rb:72). The point of the test is that 'extension' has no entry in Notification::PREFERENCE_FOR_TYPE so deliver_to? returns true regardless of the toggles, and that still holds, but using an event name nothing emits meant the test would keep passing if the real event were renamed. It now uses extension_assessed and asserts the queued job carries the notification id. 14 runs, 73 assertions, 0 failures.
My earlier resolution took 11.0.x's codeql.yml wholesale, which reverted this
branch's action pins and broke its own invariant:
test_production_image_workflow_actions_are_immutable
Expected /@[0-9a-f]{40}(?:\s+#.*)?$/ to match
" uses: actions/checkout@v4\n"
Both properties were wanted and they are independent, so combine rather than
pick a side: keep this branch's SHA-pinned steps and apply only the trigger
change from 11.0.x, 'pull_request: {}' so CodeQL reports for a pull request
against any protected branch rather than a fixed list.
Re-ran the assertion over the whole workflow directory: 33 uses: lines, all
SHA-pinned. deployment.yml still carries 2 sbom: true, 2 provenance:
mode=max and exactly 3 push: false, which the same test asserts.
…-booked feat(notifications): propose discussion request email
…sion tests feature/notifications is red at HEAD and this is not caused by the change on this branch. #49 merged at 12:28 and #43 at 12:30. #43 moved notification email onto Sidekiq, and the two test files that #49 and #50 added still assert ActionMailer::Base.deliveries immediately after the action, so they were never run against the queued base: NotificationPortfolioTest 4 failures NotificationDiscussionRequestTest 2 failures Same fix as the other nine notification test files already carry. The portfolio file drains inside submit_portfolio, which every test there goes through, so each deliveries assertion reads the way it did before. The discussion file drains at its two call sites. Both clear the queue in setup alongside ActionMailer::Base.deliveries.clear, so a leftover job cannot leak between tests. Verified across every notification suite: 147 runs, 866 assertions, 0 failures, 0 errors.
….x-all-features-20260824 release(api): OnTrack 11.0.x all-features handover
…l/test-preference-gating
…ability-paths fix(notifications): cover task availability paths
Both sides added NotificationEmailJob.drain at the same point in submit_portfolio, yours in 81016ac on this branch and mine in the repair that landed on feature/notifications via #65, so it came out as a conflict over one blank line and a comment. Kept the base version, which carries the explanation, so the file matches what is already on the shared branch and a later merge has nothing left to reconcile. 36 runs, 210 assertions, 0 failures across notification_service, notification_portfolio, notification_discussion_request and notification_email_job.
…miting fix(auth): add rate limiting to authentication endpoint
…rence-gating test(notifications): cover preference gating
…-mvp-20260827 # Conflicts: # .github/workflows/codeql.yml
… into closure/api-ontrack-mvp-20260827 # Conflicts: # app/api/peer_progress_api.rb # docs/peer-progress-api.md # docs/peer-progress/data-source-map.md # test/api/peer_progress_api_test.rb
…ure/api-ontrack-mvp-20260827 # Conflicts: # app/models/notification.rb # app/services/notification_service.rb # app/sidekiq/new_task_available_notification_job.rb # app/sidekiq/notification_email_job.rb # config/sidekiq.yml # db/schema.rb # test/api/settings_push_test.rb # test/models/notification_discussion_request_test.rb # test/models/notification_new_task_test.rb # test/services/notification_service_test.rb # test/sidekiq/scheduled_job_test.rb # test/sidekiq/send_new_task_available_notifications_job_test.rb
…ack-mvp-20260827 Close OnTrack MVP API integration and shard the test suite
Task#assess ran the resubmission extension check on every call, and Task#grant_extension adds weeks rather than setting them. Assessing the same submission twice therefore added two weeks, and the recursive_fix cascade did the same to every dependent task. An already overdue task was the worst case, because it stays inside the seven day window after being extended, so it could be extended again on every pass. The check now lives in four named methods on Task and runs at most once per round of feedback. The guard is an ExtensionComment recorded against the task with task_status_id set and dated at or after the current submission_date, so it survives restarts and duplicate events. task_comments.task_status_id already exists, so there is no migration. That comment is also the audit trail. It records the weeks granted, the status that triggered it, the assessor, the timestamp and a sentence the student can read. task_status_id is nil on extensions a student asked for, which is what tells the two kinds apart. The window is measured from the assess_date the caller passed rather than the wall clock, so a dependent task fixed by the recursive cascade is judged at the same moment as the task that triggered it, and the seven days are added as a duration rather than a fixed hour count. The policy is deliberately unchanged: same four statuses, same seven day trigger, same extension_weeks_on_resubmit_request. SLR-E01 decides the rule and has not started, so this writes the current rule down in docs/submission-lifecycle/effective-resubmission-deadline.md and leaves each part of it in one place to edit. Nothing is applied retroactively.
TCI-01 - Every api test errors in setup when the test database is mig…
…ifications-2026-09 feat(notifications): land feature/notifications on 11.0.x (rebuilt)
…ification-delete-all feat(notifications): delete all notifications up to a confirmed id
…o-registry feat(demo): add the guarded mobile feedback demo registry
…x-attachments feat(comments): accept DOCX feedback attachments with safe retries
…mission-processing feat(submissions): track submission processing state and allow retries
…itional-notification-email feat(notifications): add a verified additional notification email
authorise? only checks observer status when the object can resolve a unit role, and a feedback chip could not. So an observer-only tutor or convenor on a unit could track usage on that unit's chips, and update or delete them. Resolve the unit through the chip's learning outcome, for unit and task chips. Global chips have no unit and are unchanged.
…iewed-api-20260914 Integration: assemble the reviewed API fixes and mobile support
hub_attributes turned the declared params into a plain hash with string keys and then read them with symbol keys, so the explicit offset and ISO 8601 checks never ran. An offset-less start_at was stored as UTC and a published_at of "not a date" silently saved a draft. Symbolise the keys and test both cases. A DNS failure raises SocketError, which the Graph client did not rescue, so it escaped the per-mapping handler and skipped the remaining mappings. Rescue it with the other transport errors.
…nnouncements-sessions feat: add unit hub and optional Teams announcement sync
A student moving a task to Need Help reached nobody. Their tutor now gets a task_help_requested notification, sent by the same method as task_submitted so it keeps the same guards: student or group member only, no repeat when the status did not change, nothing on a group transition, and never to the person who acted. The group transition guard is also what keeps a group upload with the Need Help trigger to one notification. notify_tutor_of_task_submission is renamed notify_tutor_of_student_request since it now covers both statuses. The push body is fixed lock-screen copy because the in-app message names the student.
A student's extension request created a comment for the tutor, or the main convenor when there is no tutor, but raised no notification, so it sat unseen until someone opened the task. The comment's recipient now gets an extension_requested notification. It only fires for a request still waiting on a person. Staff-created extensions and ones the unit approves automatically are assessed on the spot, so they stay silent. The check is assessed? rather than the approval setting, so an automatic approval that fails still reaches the tutor. The type is task, so the tutor's Task notifications switch governs it. The extension type has no preference and would always send. The student's extension_assessed keeps the extension type. The existing extension tests now clear the request's email so they still count only the assessment.
The old base feature/peer-progress-indicator is already fully on 11.0.x. projects_api.rb keeps both sides: the include_task_definitions preload from 11.0.x and the page limits from this branch.
…fications feat(notifications): tell tutors when a student asks for help or an extension
…ogress-indicator-JE-live feat(api): add pagination for large collection endpoints; optimised communication job memory usage
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
9 participants
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.
MISC-PN03 – Preferred Name Regression and Privacy Tests
Summary
Adds regression and privacy coverage for preferred-name handling.
Changes
Test Evidence
Full users API regression suite:
37 runs, 1231 assertions, 0 failures, 0 errors, 0 skips
Targeted preferred-name tests were also executed successfully.
Privacy / Regression Evidence
Commit
9ea5231
Ticket
MISC-PN03