Ahtisham/add ora reminder notification by AhtishamShahid · Pull Request #2389 · openedx/edx-ora2

AhtishamShahid · 2026-04-09T05:57:48Z

This pull request introduces the ORA Reminder feature, which automatically sends periodic notifications to learners who have submitted an Open Response Assessment (ORA) but have not yet completed required peer or self review steps. The implementation involves a new database model, a Celery sweeper task, and integration with the submission flow to create and manage reminders. Documentation and configuration options are also provided.

Key changes include:

Feature Implementation: ORA Reminders

Added a new ORAReminder model to persist reminder schedules for learners, including fields for user, course, submission, deadlines, and reminder state. This enables efficient tracking and querying of pending reminders without frequent modulestore lookups.
Integrated reminder creation into the submission process by adding _handle_post_submission_notifications in submissions_actions.py, which creates a reminder entry and ensures the sweeper task is running after a qualifying submission.
Registered and imported the sweep_ora_reminders Celery task so it is autodiscovered and can process due reminders on a platform-wide schedule.

Documentation and Configuration

Added comprehensive documentation in docs/ora_reminders.rst, describing how the reminder system works, configuration options, and operational details for durability and error handling.

Supporting Changes

Bumped the package version to 7.1.0 to reflect the addition of this new feature.

Copilot

Pull request overview

Adds an ORA Reminder feature to automatically send periodic notifications to learners who have submitted an ORA but still need to complete peer/self review steps, using a persisted reminder schedule and a self-chaining Celery sweeper.

Changes:

Added ORAReminder model + migration to persist reminder schedules and sweep efficiently without modulestore lookups.
Introduced sweep_ora_reminders Celery task and submission-flow hook to create reminders and ensure the sweeper is running.
Added configuration defaults and documentation for enabling/tuning ORA reminders.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
settings/base.py	Adds ORA reminder feature flag and default tuning knobs (intervals, batch size, etc.).
openassessment/xblock/utils/ora_reminders.py	Implements reminder creation, sweeper task, per-reminder processing, and notification dispatch.
openassessment/xblock/test/test_ora_reminders.py	Adds unit tests covering reminder creation, sweeping, and submission-flow orchestration.
openassessment/xblock/test/test_notifications.py	Extends notification util tests to cover a new “submission created” notification path.
openassessment/xblock/apis/submissions/submissions_actions.py	Hooks reminder creation into the submission flow and starts the sweeper chain.
openassessment/workflow/tasks.py	Imports the sweeper task to ensure Celery autodiscovery registers it.
openassessment/workflow/models.py	Adds the `ORAReminder` model to the workflow app.
openassessment/workflow/migrations/0007_orareminder.py	Creates the `ORAReminder` table and sweep index.
openassessment/init.py	Bumps package version to `7.1.0`.
docs/ora_reminders.rst	Documents the feature, configuration, and durability/operations details.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-04-09T06:03:24Z

+    reminder.reminder_sent_count += 1
+    if reminder.reminder_sent_count >= max_count:
+        reminder.is_active = False
+        reminder.next_reminder_at = None


In the "final reminder" path, next_reminder_at is set to None before saving. The ORAReminder.next_reminder_at field is defined as non-nullable in both the model and migration, so this will raise an IntegrityError on save and prevent deactivation after the last reminder. Either keep a non-null timestamp when deactivating or make the field nullable (and update the migration accordingly).

Suggested change

reminder.next_reminder_at = None

reminder.next_reminder_at = now

codecov · 2026-04-13T08:16:45Z

Codecov Report

❌ Patch coverage is 99.01575% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 95.43%. Comparing base (ed2bb54) to head (9ed46c8).

Files with missing lines	Patch %	Lines
openassessment/workflow/tasks.py	50.00%	4 Missing ⚠️
openassessment/xblock/test/test_ora_reminders.py	99.46%	1 Missing and 3 partials ⚠️
openassessment/xblock/utils/ora_reminders.py	98.85%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2389      +/-   ##
==========================================
+ Coverage   95.26%   95.43%   +0.17%     
==========================================
  Files         196      198       +2     
  Lines       21679    22694    +1015     
  Branches     1509     1546      +37     
==========================================
+ Hits        20653    21659    +1006     
- Misses        776      781       +5     
- Partials      250      254       +4

Flag	Coverage Δ
unittests	`95.43% <99.01%> (+0.17%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

feanil · 2026-04-15T19:24:02Z

+
+The notification type ``ora_reminder`` must also be registered in edx-platform's
+``openedx.core.djangoapps.notifications.base_notification`` (already included
+in edx-platform ≥ the version that ships this feature).


Is there already a corresponding PR for this or will you be making one, once this is released?

Here is PR for edx-platfrom . openedx/openedx-platform#38298

feanil · 2026-04-15T19:26:31Z

+   * - ``ORA_REMINDER_SWEEP_INTERVAL_SECONDS``
+     - ``1800``
+     - How often (in seconds) the sweeper re-schedules itself. Each run
+       processes all rows whose ``next_reminder_at`` has passed.
+   * - ``ORA_REMINDER_SWEEP_BATCH_SIZE``
+     - ``1000``
+     - Maximum rows processed per sweep cycle. If more rows are due they
+       will be picked up on the next sweep.


Does this mean that if there are more than 48,000 reminders that need to be sent, that the ones above 48,000 won't be sent in time?

batch is ordered oldest-first so nothing gets permanently skipped. Also, added a note that ORA_REMINDER_SWEEP_BATCH_SIZE can be increased for high-volume deployments

feanil · 2026-04-15T19:33:01Z

+    """
+    user = models.ForeignKey(settings.AUTH_USER_MODEL, related_name="ora_reminders", on_delete=models.CASCADE)
+    course_id = models.CharField(max_length=255, db_index=True)
+    ora_usage_key = models.CharField(max_length=255)


Use a UsageKey model field instead: Use a usage key model field instead: https://github.com/openedx/opaque-keys/blob/master/opaque_keys/edx/django/models.py#L263

feanil · 2026-04-15T19:35:23Z

+    next_reminder_at = models.DateTimeField(db_index=True, null=True, blank=True)
+    is_active = models.BooleanField(default=True, db_index=True)


When you instantiate this object, it will be null by default, and it will have is_active set to True those seem like two contradictory settings. Does it make sense to set is_active to False by default? or set auto_now_add to True on the reminder date?

changed is_active default to False. create_ora_reminder always sets it explicitly to True so runtime behavior is unchanged, but the schema is consistent now

feanil · 2026-04-15T19:39:15Z

+def on_worker_ready(sender, **kwargs):  # pylint: disable=unused-argument
+    """
+    Start the ORA reminder sweep chain when a Celery worker comes online.
+
+    Clears the sweep lock first because pending countdown tasks from a
+    previous worker process are lost on restart.  In a multi-worker setup
+    ``cache.add`` inside ``ensure_sweep_chain_running`` still prevents
+    duplicate chains — only the first worker to re-acquire the lock wins.
+    """
+    from django.core.cache import cache
+    from openassessment.xblock.utils.ora_reminders import ensure_sweep_chain_running, SWEEP_LOCK_KEY
+    cache.delete(SWEEP_LOCK_KEY)
+    ensure_sweep_chain_running()


Won't this break if more than one worker is started? If Worker A is started up and starts running the sweep. Then later an operator needs more workers and starts up worker B, won't worker B clobber the cache key and start running the sweep?

on worker restart the old chain's countdown tasks are lost but the lock might still be alive. deleting first + using cache.add (atomic) means only one worker wins the lock and starts a single chain. added a comment explaining this in the code

feanil · 2026-04-15T19:42:44Z

+        lms_root_url = getattr(settings, 'LMS_ROOT_URL', '')
+        content_url = f"{lms_root_url}/courses/{course_id_str}/jump_to/{item_id}"


Why use an LMS root URL at all, why not use the relative URL instead?

when user clicks on the notification user is redirected to that item, and the notification system is built in a way that would not be able to construct url dynamically

feanil · 2026-04-15T19:52:19Z

+                reminder.id, reminder.user_id, reminder.ora_usage_key,
+            )
+
+    if processed:


Should the sweep lock be cleared at this point?

feat: add reminder_sent_count field to ORAReminder Adds a PositiveIntegerField with default=0 to track how many notifications have actually been sent per ORAReminder row, as a prerequisite for replacing the time-window guard with a per-send counter in the sweeper logic. fix: replace time-window Guard 1 with reminder_sent_count check Peer-unavailable deferrals (Guard 5) never increment reminder_sent_count, so Guard 1 was firing and deactivating rows before any notification was sent when peer submissions were slow to appear. Replacing the time-window check with a count check ensures the budget is only consumed by real sends. fix: restore bridge reminder_window_end, update process_single_reminder docstring fix: increment reminder_sent_count on send, deactivate when count reaches max Replace time-window-based advance-schedule logic with count-based termination: increment reminder_sent_count after each send and deactivate the row when the count reaches max_count. Remove the temporary bridge variables (window_base, reminder_window_end, initial_delay_hours) that kept the old block working during Task 2. Add reminder_sent_count to update_fields so the DB write is not silently skipped. docs: update termination condition docs to count-based (was time-window) fix: reset reminder_sent_count to 0 on workflow step transition test: assert ensure_sweep_chain_running not called on same-step no-op test: add regression test for peer-availability window exhaustion bug

Copilot AI review requested due to automatic review settings April 9, 2026 05:57

Copilot started reviewing on behalf of AhtishamShahid April 9, 2026 05:58 View session

Copilot AI reviewed Apr 9, 2026

View reviewed changes

AhtishamShahid requested review from awais-ansari, bmtcril and saraburns1 April 13, 2026 08:13

saraburns1 requested a review from feanil April 13, 2026 13:51

AhtishamShahid mentioned this pull request Apr 13, 2026

ORA reminder notifications openedx/frontend-app-ora#419

Open

feanil reviewed Apr 15, 2026

View reviewed changes

AhtishamShahid added 8 commits April 28, 2026 22:54

feat: added ora reminder notifications

e16f652

feat: added ora reminder notifications

b4bcced

fix: updated unit tests

b2e001b

fix: updated logic for sweeper and others

ebd37e9

fix: resolved linter and updated tests

5be0f15

fix: resolved linter errors

8dcffa7

fix: reminder notification for 2nd step

796ed4c

fix: updated comments/docs and defaults

c729e36

AhtishamShahid force-pushed the ahtisham/add_ora_reminder_notification branch from a3fbad0 to 3cdcad2 Compare April 28, 2026 17:56

AhtishamShahid force-pushed the ahtisham/add_ora_reminder_notification branch from 3cdcad2 to 7083a1d Compare April 28, 2026 18:20

Merge branch 'master' into ahtisham/add_ora_reminder_notification

9ed46c8

awais-ansari requested a review from feanil May 4, 2026 06:52

saraburns1 reviewed May 4, 2026

View reviewed changes

Comment thread docs/ora_reminders.rst

AhtishamShahid mentioned this pull request May 11, 2026

feat: add ora reminder notification openedx/openedx-platform#38298

Open

	reminder.next_reminder_at = None
	reminder.next_reminder_at = now

		next_reminder_at = models.DateTimeField(db_index=True, null=True, blank=True)
		is_active = models.BooleanField(default=True, db_index=True)

		lms_root_url = getattr(settings, 'LMS_ROOT_URL', '')
		content_url = f"{lms_root_url}/courses/{course_id_str}/jump_to/{item_id}"

Conversation

AhtishamShahid commented Apr 9, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Apr 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Apr 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov Bot commented Apr 13, 2026 •

edited

Loading