feat(notifications): implement transactional email notification system#357
Open
thejoeejoee wants to merge 34 commits intodevelopfrom
Open
feat(notifications): implement transactional email notification system#357thejoeejoee wants to merge 34 commits intodevelopfrom
thejoeejoee wants to merge 34 commits intodevelopfrom
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces a comprehensive notifications system enabling scheduled email notifications for buddy/pickup matches and membership updates. Integrates notification hooks across buddy_system, pickup_system, accounts, and sections apps. Adds Docker/build tooling updates and configuration adjustments. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/<br/>Request Handler
participant View as Match View
participant Transaction as DB Transaction
participant Hook as after_match_created<br/>Hook
participant Service as Notification Service
participant Scheduler as Scheduler
participant EmailCmd as send_scheduled<br/>_notifications Cmd
participant Mailer as Email Sender
User->>View: Submit match form
View->>View: Create match record
View->>Transaction: Begin transaction
Transaction->>View: Confirm
View->>Hook: Invoke after_match_created(match)
Hook->>Service: notify_buddy_match/notify_pickup_match
Service->>Scheduler: Check matcher prefs
Service->>Mailer: Send matcher email (immediate)
Mailer-->>Service: Email sent
Service->>Scheduler: enqueue_delayed_notification(issuer)
Scheduler->>Scheduler: Create ScheduledNotification
Scheduler-->>Service: Notification queued
Service-->>Hook: Complete
Hook->>Transaction: Commit
Transaction-->>View: Success
View-->>User: Redirect
EmailCmd->>EmailCmd: Query pending notifications<br/>with send_after <= now
EmailCmd->>EmailCmd: Claim with select_for_update
EmailCmd->>Scheduler: Load ScheduledNotification
EmailCmd->>Service: _resolve_related_object()
Service-->>EmailCmd: Match/Membership object
EmailCmd->>Mailer: send_notification_email()
Mailer->>Mailer: Check recipient opt-out
Mailer->>Mailer: Render HTML+text templates
Mailer->>Mailer: Send via Django email
Mailer-->>EmailCmd: Sent
EmailCmd->>Scheduler: Mark sent_at = now
Scheduler-->>EmailCmd: Updated
EmailCmd-->>User: Email received
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull request overview
Introduces a new apps.notifications Django app intended to support transactional email notifications by persisting per-user notification preferences and scheduled (delayed) notifications.
Changes:
- Register
apps.notificationsinINSTALLED_APPS. - Add models for per-section notification preferences and scheduled notifications (with a generic link to the triggering object).
- Add the
NotificationsConfigAppConfig and model exports.
Reviewed changes
Copilot reviewed 5 out of 6 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
fiesta/fiesta/settings/project.py |
Registers the new notifications app in INSTALLED_APPS. |
fiesta/apps/notifications/apps.py |
Defines NotificationsConfig for the new Django app. |
fiesta/apps/notifications/models/scheduled.py |
Adds ScheduledNotification and NotificationKind for delayed/digest notifications. |
fiesta/apps/notifications/models/preferences.py |
Adds SectionNotificationPreferences for per-user/per-section opt-in flags. |
fiesta/apps/notifications/models/__init__.py |
Exports the new models. |
fiesta/apps/notifications/__init__.py |
Initializes the new Python package (empty). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 20 out of 21 changed files in this pull request and generated 7 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
fiesta/apps/notifications/services/notification_email_sender.py
Outdated
Show resolved
Hide resolved
fiesta/apps/notifications/templates/notifications/pickup_system/matched_issuer_body.txt
Outdated
Show resolved
Hide resolved
fiesta/apps/notifications/management/commands/send_notifications.py
Outdated
Show resolved
Hide resolved
…membership workflows
…email calls + add global opt-out tests
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 66 out of 68 changed files in this pull request and generated 21 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
fiesta/apps/notifications/management/commands/send_scheduled_notifications.py
Show resolved
Hide resolved
fiesta/apps/notifications/templates/notifications/buddy_system/matched_matcher.txt
Outdated
Show resolved
Hide resolved
fiesta/apps/notifications/services/notification_email_sender.py
Outdated
Show resolved
Hide resolved
fiesta/apps/notifications/templates/notifications/pickup_system/matched_matcher.html
Show resolved
Hide resolved
fiesta/apps/notifications/management/commands/send_notifications.py
Outdated
Show resolved
Hide resolved
fiesta/apps/notifications/management/commands/send_scheduled_notifications.py
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 64 out of 66 changed files in this pull request and generated 12 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
fiesta/apps/notifications/management/commands/send_scheduled_notifications.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 65 out of 67 changed files in this pull request and generated 9 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
fiesta/apps/notifications/templates/notifications/buddy_system/matched_issuer.html
Outdated
Show resolved
Hide resolved
fiesta/apps/notifications/templates/notifications/pickup_system/matched_issuer.txt
Show resolved
Hide resolved
fiesta/apps/notifications/templates/notifications/pickup_system/matched_issuer.html
Outdated
Show resolved
Hide resolved
fiesta/apps/notifications/management/commands/send_scheduled_notifications.py
Outdated
Show resolved
Hide resolved
fiesta/apps/notifications/management/commands/send_scheduled_notifications.py
Outdated
Show resolved
Hide resolved
fiesta/apps/notifications/templates/notifications/buddy_system/matched_issuer.txt
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 72 out of 74 changed files in this pull request and generated 15 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
fiesta/apps/notifications/management/commands/send_scheduled_notifications.py
Show resolved
Hide resolved
fiesta/apps/notifications/management/commands/send_scheduled_notifications.py
Show resolved
Hide resolved
fiesta/apps/notifications/templates/notifications/buddy_system/matched_issuer.html
Show resolved
Hide resolved
fiesta/apps/notifications/templates/notifications/buddy_system/matched_issuer.txt
Show resolved
Hide resolved
fiesta/apps/notifications/templates/notifications/pickup_system/matched_issuer.html
Outdated
Show resolved
Hide resolved
fiesta/apps/notifications/templates/notifications/pickup_system/matched_matcher.html
Show resolved
Hide resolved
fiesta/apps/notifications/templates/notifications/buddy_system/matched_issuer.txt
Show resolved
Hide resolved
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 72 out of 74 changed files in this pull request and generated 4 comments.
Comments suppressed due to low confidence (1)
webpack/webpack.dev.config.js:36
allowedHosts: 'all'disables host checking in webpack-dev-server, which can enable DNS rebinding / host-header attacks if the dev server is reachable on the network. Prefer restricting to the expected dev domains (e.g., ROOT_DOMAIN/subdomains) or making this behavior opt-in via an env var.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
fiesta/apps/notifications/templates/notifications/buddy_system/matched_issuer.html
Outdated
Show resolved
Hide resolved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
fiesta/apps/fiestarequests/views/editor.py (1)
61-81:⚠️ Potential issue | 🟠 MajorCancel the old match's scheduled notifications before deleting it.
This rematch path deletes
br.matchdirectly, but the new notification flow only cancels scheduled jobs inBaseUpdateRequestStateView. If a request is reassigned here, delayed emails queued for the previous match can still fire after the row is gone.🧹 Proposed fix
try: - if br.match: - # could be already matched by someone else - br.match.delete() + current_match = br.match except ObjectDoesNotExist: - pass + current_match = None + + if current_match: + from apps.notifications.services.scheduler import cancel_scheduled_notifications_for + + # could be already matched by someone else + cancel_scheduled_notifications_for(current_match) + current_match.delete()
🧹 Nitpick comments (10)
fiesta/apps/notifications/templates/notifications/buddy_system/matched_issuer.html (1)
1-22: Consider adding i18n support for email content.The template has hardcoded English strings for user-facing email content (e.g., "Great news — you've been matched with a buddy!", "Your buddy request has been matched!"). For consistency with the broader application's internationalization strategy, consider wrapping these in
{% trans %}or{% blocktrans %}tags.This is a non-blocking suggestion since email templates may have different localization requirements.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiesta/apps/notifications/templates/notifications/buddy_system/matched_issuer.html` around lines 1 - 22, Wrap all user-facing static strings in the notifications/buddy_system/matched_issuer.html template with Django i18n tags (use {% trans %} for short strings and {% blocktrans %} where variables are interpolated) — specifically wrap the heading "Great news — you've been matched with a buddy! 🎉", the greeting "Hi {{ notification.recipient.user.first_name }},", the paragraph "Your buddy request has been matched! You now have a buddy at <strong>{{ section }}</strong> who will help you settle in.", the fallback line "Your buddy will be in touch soon. Don't hesitate to reach out first!", and any labels like "Your buddy:" and "Email:"; ensure variables such as {{ section }}, {{ notification.recipient.user.first_name }}, and {{ match.matcher.user.get_full_name }} are preserved inside {% blocktrans %} where needed and that the template still handles the {% if match %} and {% if match.matcher.user.email %} conditionals unchanged.fiesta/apps/notifications/models/__init__.py (1)
6-6: Sort__all__alphabetically to satisfy linter.The static analysis tool flags that
__all__should be sorted (RUF022).Proposed fix
-__all__ = ["NotificationKind", "SectionNotificationPreferences", "ScheduledNotification"] +__all__ = ["NotificationKind", "ScheduledNotification", "SectionNotificationPreferences"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiesta/apps/notifications/models/__init__.py` at line 6, The __all__ list is not alphabetized causing linter RUF022; update the exported names so they are sorted alphabetically (NotificationKind, ScheduledNotification, SectionNotificationPreferences) by editing the __all__ assignment that currently lists NotificationKind, SectionNotificationPreferences, ScheduledNotification so the entries appear in alphabetical order.fiesta/apps/buddy_system/migrations/0032_add_email_notification_fields.py (1)
1-4: Add the repo-wide future import.This migration is missing
from __future__ import annotationsat the top.As per coding guidelines,
**/*.py: Setfrom __future__ import annotationsat the top of every Python file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiesta/apps/buddy_system/migrations/0032_add_email_notification_fields.py` around lines 1 - 4, This migration file (0032_add_email_notification_fields.py) is missing the repo-wide future import; add the line "from __future__ import annotations" as the very first statement in the module (before the datetime and Django imports) so that the migration module and symbols like migrations and models use postponed annotations consistently across the codebase.fiesta/apps/notifications/tests/test_scheduler.py (1)
1-6: Add the repo-wide future import.This test module is missing
from __future__ import annotationsat the top.As per coding guidelines,
**/*.py: Setfrom __future__ import annotationsat the top of every Python file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiesta/apps/notifications/tests/test_scheduler.py` around lines 1 - 6, The file is missing the repo-required future import; add "from __future__ import annotations" as the very first import statement in the module (before any other imports) in test_scheduler.py so the module-level annotations behavior is consistent across the codebase—ensure it appears above existing imports like "from datetime import timedelta", "from django.test import TestCase", and "from django.utils import timezone".fiesta/apps/pickup_system/migrations/0010_add_email_notification_fields.py (1)
1-4: Add the repo-wide future import.This migration is missing
from __future__ import annotationsat the top.As per coding guidelines,
**/*.py: Setfrom __future__ import annotationsat the top of every Python file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiesta/apps/pickup_system/migrations/0010_add_email_notification_fields.py` around lines 1 - 4, Add the repo-wide future import by inserting "from __future__ import annotations" as the very first import in the migration file 0010_add_email_notification_fields.py (before any other imports such as datetime, django.db.migrations, or django.db.models) so the file begins with that future import in compliance with the coding guideline.fiesta/apps/notifications/migrations/0001_initial.py (1)
1-8: Add the repo-wide future import.This migration is missing
from __future__ import annotationsat the top.As per coding guidelines,
**/*.py: Setfrom __future__ import annotationsat the top of every Python file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiesta/apps/notifications/migrations/0001_initial.py` around lines 1 - 8, Add the repo-wide future import to the migration file by inserting "from __future__ import annotations" as the very first import statement (before any other imports) in fiesta/apps/notifications/migrations/0001_initial.py; this ensures the module-level annotations behavior is consistent for the migration module that contains the generated imports (e.g., django.conf.settings, django.db.migrations, and django_extensions.db.fields).fiesta/apps/notifications/services/membership.py (1)
46-46: En-dash in email subject may cause encoding issues.The subject uses an en-dash (
–, U+2013) instead of a hyphen-minus (-, U+002D). While typographically correct, some older email clients or systems may have issues rendering or encoding Unicode characters in email subjects. Consider using a regular hyphen for maximum compatibility.♻️ Proposed fix
- subject=f"{section} – Application received", + subject=f"{section} - Application received",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiesta/apps/notifications/services/membership.py` at line 46, Subject string in the membership notification uses an en-dash character (subject=f"{section} – Application received") which can cause encoding issues in some email clients; replace the en-dash (U+2013) with a standard hyphen-minus (U+002D) so the subject reads subject=f"{section} - Application received". Update the string in the membership notification code (where the subject variable/argument is set) and run existing email subject tests to ensure no regressions.fiesta/apps/notifications/tests/test_match_notifications.py (1)
16-63: Consider extracting shared test helpers to a common module.The helper functions
_make_buddy_config,_make_pickup_config,_make_section_with_buddy_config,_make_section_with_pickup_config, and_make_match_and_requestare duplicated across test files (test_match_notifications.pyandtest_global_opt_out.py). Extracting them to a shared test utilities module (e.g.,tests/helpers.py) would improve maintainability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiesta/apps/notifications/tests/test_match_notifications.py` around lines 16 - 63, Duplicate test helper functions (_make_buddy_config, _make_pickup_config, _make_section_with_buddy_config, _make_section_with_pickup_config, _make_match_and_request) exist in multiple test files; extract them into a shared test utilities module (e.g., tests/helpers.py) and import them from there. Create a new helpers module that defines these five functions (keeping signatures and behavior), update test_match_notifications.py and test_global_opt_out.py to remove their local copies and import the helpers (from tests.helpers import _make_buddy_config, _make_pickup_config, _make_section_with_buddy_config, _make_section_with_pickup_config, _make_match_and_request), and run tests to ensure no behavioral changes; ensure MagicMock/spec usage and return values remain identical to avoid breaking tests.fiesta/apps/notifications/admin.py (2)
38-41: Action description may be misleading.The action is named
mark_as_sentand described as "Send now (bypass send_after)", but it actually setssend_after=timezone.now()rather than marking notifications as sent. This schedules them for immediate processing by the command, not instant delivery. Consider renaming toschedule_immediateor updating the description to clarify behavior.♻️ Proposed fix
- `@admin.action`(description=_("Send now (bypass send_after)")) - def mark_as_sent(self, request: HttpRequest, queryset: QuerySet[ScheduledNotification]) -> None: + `@admin.action`(description=_("Schedule for immediate processing")) + def schedule_immediate(self, request: HttpRequest, queryset: QuerySet[ScheduledNotification]) -> None: updated = queryset.filter(sent_at__isnull=True, cancelled_at__isnull=True).update(send_after=timezone.now()) - self.message_user(request, _("Scheduled %s notification(s) to send immediately.") % updated) + self.message_user(request, _("Scheduled %s notification(s) for immediate processing.") % updated)Also update the
actionslist:- actions = ["mark_as_sent", "cancel_notifications"] + actions = ["schedule_immediate", "cancel_notifications"]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiesta/apps/notifications/admin.py` around lines 38 - 41, The admin action mark_as_sent is misleading because it sets send_after=timezone.now() (schedules immediate delivery) rather than marking notifications as sent; rename the method to schedule_immediate (or change the admin.action description) and update the decorator description text to something like "Schedule now (bypass send_after)"; also update the ModelAdmin.actions list (and any references to mark_as_sent) to use the new name schedule_immediate so labels and internal references remain consistent.
43-50: Use bulk update for improved performance with large selections.The
cancel()method only sets thecancelled_atfield with no side effects, so the current loop-and-save pattern can be optimized using Django's.update()to reduce database queries from N+1 to 1. Sincetimezoneis already imported, the optimization is straightforward.♻️ Proposed bulk update
`@admin.action`(description=_("Cancel selected notifications")) def cancel_notifications(self, request: HttpRequest, queryset: QuerySet[ScheduledNotification]) -> None: - count = 0 - for notification in queryset.filter(sent_at__isnull=True, cancelled_at__isnull=True): - notification.cancel() - notification.save(update_fields=["cancelled_at", "modified"]) - count += 1 + count = queryset.filter(sent_at__isnull=True, cancelled_at__isnull=True).update( + cancelled_at=timezone.now(), + modified=timezone.now(), + ) self.message_user(request, _("Cancelled %s notification(s).") % count)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiesta/apps/notifications/admin.py` around lines 43 - 50, The loop in admin action cancel_notifications causes N+1 saves; replace the per-object cancel() + save with a single bulk update on the filtered queryset (ScheduledNotification entries matching sent_at__isnull=True and cancelled_at__isnull=True) setting cancelled_at=timezone.now() and updating modified to timezone.now() (or a single tz value captured once) so only one DB query is executed, then call self.message_user(request, ...) with the updated count from queryset.count() or the number returned by update().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Dockerfile`:
- Around line 113-114: The Dockerfile currently runs "uv pip install --python
/venv/bin/python 'setuptools<72'", which bypasses the locked dependencies;
instead add setuptools<72 to your project's declared dependencies by running "uv
add 'setuptools<72'" so it is recorded in pyproject.toml, regenerate the
lockfile with uv to update uv.lock, and remove the ad-hoc "uv pip install" line
so that "uv sync" or the normal locked install path installs setuptools
reproducibly.
In `@fiesta/apps/notifications/forms.py`:
- Around line 43-47: The save method currently persists self.user_profile
unconditionally which breaks commit=False semantics and also saves the two
models non-atomically; update the SectionNotificationPreferencesForm.save to
respect the commit flag by only calling self.user_profile.save(...) when commit
is True, and when commit is True wrap saving both the section preferences
(super().save(commit=True)) and the user_profile update in a single transaction
(e.g. using transaction.atomic) so both persist atomically; keep the behavior of
returning the instance when commit=False without persisting the related
UserProfile.
In
`@fiesta/apps/notifications/management/commands/send_scheduled_notifications.py`:
- Around line 35-43: The code is using sent_at as the claim marker which
permanently hides rows if the worker crashes; change the claim to a separate
field on the ScheduledNotification model (e.g., claim_id, claimed_at or
lease_expires) and modify the claim/update logic so the transaction.atomic block
and the select_for_update(skip_locked=True) reserve rows by setting that claim
field (not sent_at); then after each notification is successfully delivered set
sent_at and clear/update the claim field as needed; update any references in
this module to use the new claim field (e.g., where you currently build
reserved_ids and call
ScheduledNotification.objects.filter(pk__in=reserved_ids).update(...), update
the claim field there and only set sent_at after send succeeds).
- Around line 62-76: The loop treats any non-exceptional return from
self._send(notification) as success, but _send can return False for unknown
kinds or unresolved generic relations; change the try block to capture the
boolean result (e.g., result = self._send(notification)) and if result is False
do not increment sent — instead either release the claim by resetting
ScheduledNotification.objects.filter(pk=notification_pk).update(sent_at=None)
and increment skipped, or for unrecoverable cases mark the notification
cancelled (update cancelled_at) before continuing; only increment sent when
result is truthy and notification.cancelled_at is still None.
In `@fiesta/apps/notifications/migrations/0001_initial.py`:
- Around line 77-145: The ScheduledNotification model allows duplicate pending
rows because there is no DB-level uniqueness on the pending key; add a partial
UniqueConstraint to enforce uniqueness for pending notifications by including
fields ("kind", "recipient", "section", "content_type", "object_id") with a
condition that sent_at IS NULL AND cancelled_at IS NULL (or alternatively add a
non-null dedupe_key populated on enqueue and create a UniqueConstraint on that
key); implement this by updating the ScheduledNotification Meta options (or a
new migration) to add models.UniqueConstraint(fields=[ "kind", "recipient",
"section", "content_type", "object_id" ], name="unique_pending_notification",
condition=django.db.models.Q(sent_at__isnull=True, cancelled_at__isnull=True))
so concurrent enqueues cannot create duplicate pending rows.
In `@fiesta/apps/notifications/models/scheduled.py`:
- Around line 64-69: Add a DB-level guard and race handling for delayed
notification upserts: add a conditional UniqueConstraint to
ScheduledNotification.Meta (name it e.g.
"notifications_unique_active_notification") over fields
["kind","recipient","content_type","object_id"] with condition
Q(sent_at__isnull=True, cancelled_at__isnull=True); then in
enqueue_delayed_notification(), wrap the
ScheduledNotification.objects.create(...) call in a try/except catching
django.db.IntegrityError, and on IntegrityError fetch the existing row using
ScheduledNotification.objects.select_for_update().get(...) and update send_after
(and modified) via save(update_fields=[...]) to resolve the race and return the
existing record.
In `@fiesta/apps/notifications/services/mailer.py`:
- Line 35: The logger.info call is writing raw recipient_email (PII) to logs;
update all occurrences (the logger.info call that references recipient_email and
the similar occurrences around lines 54-57) to avoid logging full addresses by
either logging an internal user identifier (user_id) or a masked email (e.g.,
show only first char and domain or replace with ****), and ensure the code uses
the same safe field everywhere the function that sends emails or checks opt-out
(reference recipient_email and logger.info) to prevent exposing raw email
addresses in logs.
In `@fiesta/apps/notifications/services/match.py`:
- Around line 78-83: Replace the hardcoded preferences_url construction
(currently using preferences_url =
f"https://{section.space_slug}.{settings.ROOT_DOMAIN}/notifications/preferences/")
with a call to the shared site URL helper (e.g., get_site_url or build_site_url)
that reads the base scheme/host from env-backed settings; pass
section.space_slug (or section) and the path "/notifications/preferences/" into
that helper so the URL is assembled from configured site data rather than
hardcoded ROOT_DOMAIN; apply the same replacement for the duplicate logic around
the preferences_url usage at the other location (lines 126-131) so all
preference link generation uses the shared helper.
In `@fiesta/apps/notifications/services/membership.py`:
- Around line 25-29: Replace the bare "except Exception:" around the access to
section.sections_plugin_configuration with targeted exception handling for the
missing-relation cases (e.g., catch django.core.exceptions.ObjectDoesNotExist
and AttributeError) so only expected "no configuration" errors are swallowed;
import ObjectDoesNotExist if needed and keep the logger.debug and return
behavior when those specific exceptions are raised.
In `@fiesta/apps/notifications/templates/notifications/base_email.html`:
- Around line 38-41: Footer copy assumes variable "section" exists and will
render "a member of ." when it's missing; update the template
(notifications/base_email.html) to avoid that by either wrapping the entire
sentence in a conditional that checks {% if section %} before rendering "You are
receiving this email because you are a member of {{ section }}." or by rendering
a safe default using the template filter (e.g., using {{ section|default:"this
community" }}) so the footer text is always grammatically correct; ensure the
existing preferences_url logic (preferences_url) remains unchanged.
In `@fiesta/apps/notifications/templates/notifications/preferences.html`:
- Around line 15-18: The form in the template renders only "{{ form }}" and has
no submit control, so add an HTML submit button inside the same <form> element
(after the "{{ form }}" rendering) — e.g., a <button type="submit">Save
preferences</button> (optionally with your CSS classes such as "btn
btn-primary") so users can submit the NotificationPreferences form; ensure the
button stays within the existing {% csrf_token %} form block.
In `@fiesta/apps/notifications/tests/test_membership_notifications.py`:
- Around line 53-54: The test currently patches enqueue_delayed_notification at
apps.notifications.services.scheduler which won't intercept the import used by
the membership module; update the patch target to
apps.notifications.services.membership.enqueue_delayed_notification so the mock
replaces the function where membership.py imported it (keep the existing `@patch`
for send_notification_email as-is and ensure the patch decorator order matches
the test function parameters).
In `@fiesta/apps/sections/migrations/0024_initial.py`:
- Around line 1-4: Add the future annotations import at the top of the migration
module by inserting "from __future__ import annotations" as the first import
(immediately after the generated file header comment) so that it precedes the
existing imports (datetime and django.db imports) in 0024_initial.py; this
satisfies the coding guideline and avoids changing any migration semantics
(leave dependencies/operations lists as-is).
In `@fiesta/apps/sections/models/configuration.py`:
- Around line 92-95: The DurationField email_digest_interval currently allows
zero or negative timedeltas; add a lower-bound validator to reject non-positive
values by adding a validator to the field (e.g., a MinValueValidator with
datetime.timedelta(seconds=1) or a small custom validator that raises
ValidationError when value <= datetime.timedelta(0)) on the
email_digest_interval DurationField declaration so the admin cannot save
0/negative intervals; update any model migration if required.
In `@webpack/webpack.dev.config.js`:
- Line 29: Update the dev webpack configuration to restore host header
validation by replacing the allowedHosts setting currently set to 'all' with
'auto' in the webpack dev server config (look for the allowedHosts property in
webpack.dev.config.js / devServer config); this will re-enable DNS-rebinding
protections while still allowlisting localhost, the configured host, and the
client websocket hostname (so webpack.<ROOT_DOMAIN> continues to work).
---
Nitpick comments:
In `@fiesta/apps/buddy_system/migrations/0032_add_email_notification_fields.py`:
- Around line 1-4: This migration file (0032_add_email_notification_fields.py)
is missing the repo-wide future import; add the line "from __future__ import
annotations" as the very first statement in the module (before the datetime and
Django imports) so that the migration module and symbols like migrations and
models use postponed annotations consistently across the codebase.
In `@fiesta/apps/notifications/admin.py`:
- Around line 38-41: The admin action mark_as_sent is misleading because it sets
send_after=timezone.now() (schedules immediate delivery) rather than marking
notifications as sent; rename the method to schedule_immediate (or change the
admin.action description) and update the decorator description text to something
like "Schedule now (bypass send_after)"; also update the ModelAdmin.actions list
(and any references to mark_as_sent) to use the new name schedule_immediate so
labels and internal references remain consistent.
- Around line 43-50: The loop in admin action cancel_notifications causes N+1
saves; replace the per-object cancel() + save with a single bulk update on the
filtered queryset (ScheduledNotification entries matching sent_at__isnull=True
and cancelled_at__isnull=True) setting cancelled_at=timezone.now() and updating
modified to timezone.now() (or a single tz value captured once) so only one DB
query is executed, then call self.message_user(request, ...) with the updated
count from queryset.count() or the number returned by update().
In `@fiesta/apps/notifications/migrations/0001_initial.py`:
- Around line 1-8: Add the repo-wide future import to the migration file by
inserting "from __future__ import annotations" as the very first import
statement (before any other imports) in
fiesta/apps/notifications/migrations/0001_initial.py; this ensures the
module-level annotations behavior is consistent for the migration module that
contains the generated imports (e.g., django.conf.settings,
django.db.migrations, and django_extensions.db.fields).
In `@fiesta/apps/notifications/models/__init__.py`:
- Line 6: The __all__ list is not alphabetized causing linter RUF022; update the
exported names so they are sorted alphabetically (NotificationKind,
ScheduledNotification, SectionNotificationPreferences) by editing the __all__
assignment that currently lists NotificationKind,
SectionNotificationPreferences, ScheduledNotification so the entries appear in
alphabetical order.
In `@fiesta/apps/notifications/services/membership.py`:
- Line 46: Subject string in the membership notification uses an en-dash
character (subject=f"{section} – Application received") which can cause encoding
issues in some email clients; replace the en-dash (U+2013) with a standard
hyphen-minus (U+002D) so the subject reads subject=f"{section} - Application
received". Update the string in the membership notification code (where the
subject variable/argument is set) and run existing email subject tests to ensure
no regressions.
In
`@fiesta/apps/notifications/templates/notifications/buddy_system/matched_issuer.html`:
- Around line 1-22: Wrap all user-facing static strings in the
notifications/buddy_system/matched_issuer.html template with Django i18n tags
(use {% trans %} for short strings and {% blocktrans %} where variables are
interpolated) — specifically wrap the heading "Great news — you've been matched
with a buddy! 🎉", the greeting "Hi {{ notification.recipient.user.first_name
}},", the paragraph "Your buddy request has been matched! You now have a buddy
at <strong>{{ section }}</strong> who will help you settle in.", the fallback
line "Your buddy will be in touch soon. Don't hesitate to reach out first!", and
any labels like "Your buddy:" and "Email:"; ensure variables such as {{ section
}}, {{ notification.recipient.user.first_name }}, and {{
match.matcher.user.get_full_name }} are preserved inside {% blocktrans %} where
needed and that the template still handles the {% if match %} and {% if
match.matcher.user.email %} conditionals unchanged.
In `@fiesta/apps/notifications/tests/test_match_notifications.py`:
- Around line 16-63: Duplicate test helper functions (_make_buddy_config,
_make_pickup_config, _make_section_with_buddy_config,
_make_section_with_pickup_config, _make_match_and_request) exist in multiple
test files; extract them into a shared test utilities module (e.g.,
tests/helpers.py) and import them from there. Create a new helpers module that
defines these five functions (keeping signatures and behavior), update
test_match_notifications.py and test_global_opt_out.py to remove their local
copies and import the helpers (from tests.helpers import _make_buddy_config,
_make_pickup_config, _make_section_with_buddy_config,
_make_section_with_pickup_config, _make_match_and_request), and run tests to
ensure no behavioral changes; ensure MagicMock/spec usage and return values
remain identical to avoid breaking tests.
In `@fiesta/apps/notifications/tests/test_scheduler.py`:
- Around line 1-6: The file is missing the repo-required future import; add
"from __future__ import annotations" as the very first import statement in the
module (before any other imports) in test_scheduler.py so the module-level
annotations behavior is consistent across the codebase—ensure it appears above
existing imports like "from datetime import timedelta", "from django.test import
TestCase", and "from django.utils import timezone".
In `@fiesta/apps/pickup_system/migrations/0010_add_email_notification_fields.py`:
- Around line 1-4: Add the repo-wide future import by inserting "from __future__
import annotations" as the very first import in the migration file
0010_add_email_notification_fields.py (before any other imports such as
datetime, django.db.migrations, or django.db.models) so the file begins with
that future import in compliance with the coding guideline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f79b95d-1370-40ed-aea1-a7b461a5729c
📒 Files selected for processing (74)
DockerfileMakefiledocker-compose.override.ymldocker-compose.prod.ymldocker-compose.ymlfiesta/apps/accounts/migrations/0028_add_email_notifications_enabled.pyfiesta/apps/accounts/models/profile.pyfiesta/apps/accounts/templates/accounts/user_profile/base.htmlfiesta/apps/buddy_system/migrations/0032_add_email_notification_fields.pyfiesta/apps/buddy_system/models/configuration.pyfiesta/apps/buddy_system/views/editor.pyfiesta/apps/buddy_system/views/matching.pyfiesta/apps/dashboard/models/configuration.pyfiesta/apps/fiestarequests/views/editor.pyfiesta/apps/fiestarequests/views/matching.pyfiesta/apps/notifications/__init__.pyfiesta/apps/notifications/admin.pyfiesta/apps/notifications/apps.pyfiesta/apps/notifications/forms.pyfiesta/apps/notifications/management/__init__.pyfiesta/apps/notifications/management/commands/__init__.pyfiesta/apps/notifications/management/commands/send_scheduled_notifications.pyfiesta/apps/notifications/migrations/0001_initial.pyfiesta/apps/notifications/migrations/0002_alter_sectionnotificationpreferences_unique_together_and_more.pyfiesta/apps/notifications/migrations/__init__.pyfiesta/apps/notifications/models/__init__.pyfiesta/apps/notifications/models/preferences.pyfiesta/apps/notifications/models/scheduled.pyfiesta/apps/notifications/services/__init__.pyfiesta/apps/notifications/services/mailer.pyfiesta/apps/notifications/services/match.pyfiesta/apps/notifications/services/membership.pyfiesta/apps/notifications/services/scheduler.pyfiesta/apps/notifications/templates/notifications/base_email.htmlfiesta/apps/notifications/templates/notifications/base_email.txtfiesta/apps/notifications/templates/notifications/buddy_system/matched_issuer.htmlfiesta/apps/notifications/templates/notifications/buddy_system/matched_issuer.txtfiesta/apps/notifications/templates/notifications/buddy_system/matched_matcher.htmlfiesta/apps/notifications/templates/notifications/buddy_system/matched_matcher.txtfiesta/apps/notifications/templates/notifications/pickup_system/matched_issuer.htmlfiesta/apps/notifications/templates/notifications/pickup_system/matched_issuer.txtfiesta/apps/notifications/templates/notifications/pickup_system/matched_matcher.htmlfiesta/apps/notifications/templates/notifications/pickup_system/matched_matcher.txtfiesta/apps/notifications/templates/notifications/preferences.htmlfiesta/apps/notifications/templates/notifications/sections/membership_pending.htmlfiesta/apps/notifications/templates/notifications/sections/membership_pending.txtfiesta/apps/notifications/templates/notifications/sections/membership_received.htmlfiesta/apps/notifications/templates/notifications/sections/membership_received.txtfiesta/apps/notifications/tests/__init__.pyfiesta/apps/notifications/tests/factories.pyfiesta/apps/notifications/tests/test_global_opt_out.pyfiesta/apps/notifications/tests/test_match_notifications.pyfiesta/apps/notifications/tests/test_membership_notifications.pyfiesta/apps/notifications/tests/test_scheduled_command.pyfiesta/apps/notifications/tests/test_scheduler.pyfiesta/apps/notifications/urls.pyfiesta/apps/notifications/views.pyfiesta/apps/pages/models/configuration.pyfiesta/apps/pickup_system/migrations/0010_add_email_notification_fields.pyfiesta/apps/pickup_system/models/configuration.pyfiesta/apps/pickup_system/views/editor.pyfiesta/apps/pickup_system/views/matching.pyfiesta/apps/plugins/views/mixins.pyfiesta/apps/sections/middleware/section_space.pyfiesta/apps/sections/migrations/0024_initial.pyfiesta/apps/sections/models/configuration.pyfiesta/apps/sections/models/membership.pyfiesta/apps/utils/templatetags/messages.pyfiesta/fiesta/checks/models.pyfiesta/fiesta/settings/project.pyfiesta/fiesta/urls.pyfiesta/manage.pypyproject.tomlwebpack/webpack.dev.config.js
💤 Files with no reviewable changes (3)
- fiesta/apps/pages/models/configuration.py
- fiesta/apps/dashboard/models/configuration.py
- docker-compose.prod.yml
fiesta/apps/notifications/management/commands/send_scheduled_notifications.py
Show resolved
Hide resolved
fiesta/apps/notifications/management/commands/send_scheduled_notifications.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tests.yml:
- Around line 30-37: The CI uses DJANGO_CONFIGURATION=Development but
Development (via DatabaseConfigMixin) hardcodes HOST="db" and ignores
DATABASE_URL, so tests try to connect to host "db" instead of localhost; fix by
adding a Testing config class (e.g., class Testing(Development)) in
settings/__init__.py that reads DATABASE_URL and overrides the DATABASES
property to call dj_database_url.parse(self.DATABASE_URL,
conn_max_age=self.DATABASE_CONN_MAX_AGE,
conn_health_checks=self.DATABASE_CONN_HEALTH_CHECKS) and return the parsed
default DB, then update the workflow to set DJANGO_CONFIGURATION=Testing;
alternatively, modify DatabaseConfigMixin to accept an optional DATABASE_URL
override (parse with dj_database_url.parse in the DATABASES property when
DATABASE_URL is present) so Development will honor the env var.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 42e87c8c-d2f2-4a24-acd7-2fe2576fcb76
📒 Files selected for processing (1)
.github/workflows/tests.yml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
fiesta/apps/notifications/management/commands/send_scheduled_notifications.py (1)
62-72:⚠️ Potential issue | 🟠 Major
_send()return value is still ignored —Falsecounts as success.The return value from
self._send(notification)is not captured. When_send()returnsFalse(unknown kind, section isNone, or related object missing after resolution), the code falls through tosent += 1(line 72) without rolling back the claim. These notifications remain permanently "sent" despite no email being delivered.,
🐛 Proposed fix to handle the return value
try: - self._send(notification) + success = self._send(notification) if notification.cancelled_at is not None: # Notification was soft-cancelled during _send (e.g. related object deleted). # Roll back the claim so sent_at reflects that no email was actually sent. ScheduledNotification.objects.filter(pk=notification_pk).update(sent_at=None) skipped += 1 continue + if not success: + # Unknown kind or missing required data — roll back claim for retry or manual review. + ScheduledNotification.objects.filter(pk=notification_pk).update(sent_at=None) + skipped += 1 + continue + sent += 1🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiesta/apps/notifications/management/commands/send_scheduled_notifications.py` around lines 62 - 72, Capture the boolean return from self._send(notification) and treat False as a failed send: assign e.g. sent_ok = self._send(notification), and if sent_ok is False then roll back the claim by calling ScheduledNotification.objects.filter(pk=notification_pk).update(sent_at=None), increment skipped, and continue (similar to the existing cancelled_at branch) so that False does not increment sent; keep the existing cancelled_at check as-is after handling the return value..github/workflows/tests.yml (1)
30-40:⚠️ Potential issue | 🔴 Critical
Developmentis still the wrong Django configuration for CI.This job still exports
DJANGO_CONFIGURATION: Development, so the workflow is not using a dedicated test configuration for the Postgres service. In this repo, that meansDATABASE_URLis still not the reliable source of truth for test DB settings, and the job can keep targeting the wrong host/database despite the extra env vars.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 30 - 40, The CI workflow is still exporting DJANGO_CONFIGURATION: Development which prevents tests from using the dedicated test settings and ignores DATABASE_URL; update the env block in the tests job to set DJANGO_CONFIGURATION to the repository's test/CI configuration name (e.g., "Test" or "CI") instead of "Development" so the Django settings loader uses the test configuration that respects DATABASE_URL and the provided DJANGO_DATABASE_* vars; ensure the workflow's env replaces DJANGO_CONFIGURATION "Development" with the correct test configuration name and re-run to confirm the Postgres service is targeted by the test config.
🧹 Nitpick comments (6)
fiesta/apps/events/migrations/0005_alter_pricevariant_amount_currency.py (1)
1-4: Add the required future import.This new Python file is missing
from __future__ import annotationsat the top.As per coding guidelines,
**/*.py: Setfrom __future__ import annotationsat the top of every Python file.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiesta/apps/events/migrations/0005_alter_pricevariant_amount_currency.py` around lines 1 - 4, Add the required future import at the top of the migration module: insert "from __future__ import annotations" as the very first import line in 0005_alter_pricevariant_amount_currency.py (before "from django.db import migrations" and "import djmoney.models.fields") so the file complies with the project's coding guideline to include future annotations in every .py file.fiesta/apps/notifications/management/commands/send_scheduled_notifications.py (3)
91-95: Redundant conditional on line 94.After returning on line 92 when
section is None, the ternaryif section else ""on line 94 will always evaluate to the truthy branch, making the condition unnecessary.♻️ Proposed simplification
section = notification.section if section is None: return False - preferences_url = ( - f"https://{section.space_slug}.{settings.ROOT_DOMAIN}/notifications/preferences/" if section else "" - ) + preferences_url = f"https://{section.space_slug}.{settings.ROOT_DOMAIN}/notifications/preferences/"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiesta/apps/notifications/management/commands/send_scheduled_notifications.py` around lines 91 - 95, The ternary on the preferences_url assignment is redundant because the function already returns when section is None; update the preferences_url computation in send_scheduled_notifications (the preferences_url assignment that uses section.space_slug) to unconditionally build the URL using section.space_slug and settings.ROOT_DOMAIN (remove the `if section else ""` fallback) so the code is simpler and clearer while preserving existing behavior.
173-173: Subject line contains EN DASH (–) instead of HYPHEN-MINUS (-).Ruff flagged ambiguous Unicode character. While this renders correctly for most users, standard ASCII hyphen is safer for email subjects across different mail clients and encodings.
♻️ Proposed fix
send_notification_email( - subject=f"{section} – You've been matched with a buddy!", + subject=f"{section} - You've been matched with a buddy!", recipient_email=notification.recipient.email,Same applies to lines 193 and 218.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiesta/apps/notifications/management/commands/send_scheduled_notifications.py` at line 173, Replace the EN DASH character (–) used inside the subject f-strings with a standard ASCII hyphen-minus (-) for all subject definitions in this module: update the subject=f"{section} – You've been matched with a buddy!" and the two other subject f-strings (the ones analogous at lines near 193 and 218) so they read subject=f"{section} - You've been matched with a buddy!" (and the corresponding replacements for the other two subjects); search for subject= f-strings in send_scheduled_notifications.py and make the same character substitution where the EN DASH appears.
200-213:related_objectparameter is unused.The
related_object(typed asSectionMembership) is passed but never used. Thewaiting_countis computed from a fresh query instead. This is wasteful since the related object is resolved and passed but then ignored.Consider either:
- Using
related_objectif its data is relevant (e.g., as a trigger membership).- Documenting why this parameter exists but is intentionally unused.
♻️ Option: Add comment explaining the intent
def _send_member_joined_editors( self, *, notification: ScheduledNotification, - related_object: SectionMembership, + related_object: SectionMembership, # noqa: ARG002 - triggering membership; count is computed fresh context: dict[str, Any], section: Section, ) -> None:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiesta/apps/notifications/management/commands/send_scheduled_notifications.py` around lines 200 - 213, The related_object parameter of _send_member_joined_editors (typed SectionMembership) is currently ignored; either use it as the trigger membership or document why it’s kept. Fix by referencing related_object inside _send_member_joined_editors (for example treat it as the triggering membership and include its state when computing waiting_count or adjust the filter to exclude/include related_object as intended) so the parameter isn’t wasted, or if the param must remain unused, rename it to _related_object and add a brief comment explaining why it’s intentionally unused; update any logic around waiting_count and the SectionMembership import accordingly.fiesta/apps/notifications/tests/test_global_opt_out.py (1)
71-106: Test usessectionasrelated_objectwhich mismatches expected types.The test passes
sectionas therelated_objectfor aBUDDY_MATCHED_ISSUERnotification, but the management command expects aBuddyRequestMatch. This works for testing the opt-out gate (which fires before type resolution), but creates aScheduledNotificationthat would fail if actually processed.Consider using a proper mock or factory for the expected related object type for more realistic test coverage, or add a comment clarifying this is intentionally testing only the opt-out gate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiesta/apps/notifications/tests/test_global_opt_out.py` around lines 71 - 106, The test test_enqueue_delayed_notification_skips_opted_out_and_creates_for_opted_in passes a KnownSectionFactory instance as related_object for NotificationKind.BUDDY_MATCHED_ISSUER which mismatches the expected BuddyRequestMatch type and could break processing; update the test to supply a proper BuddyRequestMatch instance (use an existing BuddyRequestMatchFactory or create a minimal BuddyRequestMatch object) when calling enqueue_delayed_notification, ensuring the created ScheduledNotification would be valid for downstream processing, or alternatively add a clarifying comment in the test near the enqueue_delayed_notification calls stating that section is intentionally used only to exercise the opt-out gate and not full processing..github/workflows/tests.yml (1)
60-62: Migrate the test suite topytest+pytest-djangoto align with project standards.Currently, all tests in the codebase use Django's unittest style (
TestCase). Per project learnings, the testing framework should usepytestwithpytest-djangoand setDJANGO_CONFIGURATIONenv var (defaults toDevelopment). The workflow should invokepytestinstead ofmanage.py test, and the project dependencies should includepytestandpytest-django.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/tests.yml around lines 60 - 62, Update the "Run tests" workflow step to invoke pytest instead of Django's manage.py test: replace the current run command (../.venv/bin/python manage.py test --verbosity 1) with a pytest invocation, set the DJANGO_CONFIGURATION environment variable (default "Development") for the step, and ensure project dependencies include pytest and pytest-django so pytest-django can load Django settings; locate the workflow step named "Run tests" in the tests.yml and update the run command and env entries accordingly and add pytest/pytest-django to the project's test dependencies.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fiesta/apps/events/migrations/0005_alter_pricevariant_amount_currency.py`:
- Around line 9-18: This migration file
(0005_alter_pricevariant_amount_currency.py) is a no-op duplicating an existing
migrations.AlterField for model_name='pricevariant' and name='amount_currency';
delete the 0005 migration file so it doesn't add churn, verify the current model
(PriceVariant.amount_currency) in models/price_variant.py matches the existing
migration state, and only run ./manage.py makemigrations to regenerate a new
migration if there is an actual model delta.
---
Duplicate comments:
In @.github/workflows/tests.yml:
- Around line 30-40: The CI workflow is still exporting DJANGO_CONFIGURATION:
Development which prevents tests from using the dedicated test settings and
ignores DATABASE_URL; update the env block in the tests job to set
DJANGO_CONFIGURATION to the repository's test/CI configuration name (e.g.,
"Test" or "CI") instead of "Development" so the Django settings loader uses the
test configuration that respects DATABASE_URL and the provided DJANGO_DATABASE_*
vars; ensure the workflow's env replaces DJANGO_CONFIGURATION "Development" with
the correct test configuration name and re-run to confirm the Postgres service
is targeted by the test config.
In
`@fiesta/apps/notifications/management/commands/send_scheduled_notifications.py`:
- Around line 62-72: Capture the boolean return from self._send(notification)
and treat False as a failed send: assign e.g. sent_ok =
self._send(notification), and if sent_ok is False then roll back the claim by
calling
ScheduledNotification.objects.filter(pk=notification_pk).update(sent_at=None),
increment skipped, and continue (similar to the existing cancelled_at branch) so
that False does not increment sent; keep the existing cancelled_at check as-is
after handling the return value.
---
Nitpick comments:
In @.github/workflows/tests.yml:
- Around line 60-62: Update the "Run tests" workflow step to invoke pytest
instead of Django's manage.py test: replace the current run command
(../.venv/bin/python manage.py test --verbosity 1) with a pytest invocation, set
the DJANGO_CONFIGURATION environment variable (default "Development") for the
step, and ensure project dependencies include pytest and pytest-django so
pytest-django can load Django settings; locate the workflow step named "Run
tests" in the tests.yml and update the run command and env entries accordingly
and add pytest/pytest-django to the project's test dependencies.
In `@fiesta/apps/events/migrations/0005_alter_pricevariant_amount_currency.py`:
- Around line 1-4: Add the required future import at the top of the migration
module: insert "from __future__ import annotations" as the very first import
line in 0005_alter_pricevariant_amount_currency.py (before "from django.db
import migrations" and "import djmoney.models.fields") so the file complies with
the project's coding guideline to include future annotations in every .py file.
In
`@fiesta/apps/notifications/management/commands/send_scheduled_notifications.py`:
- Around line 91-95: The ternary on the preferences_url assignment is redundant
because the function already returns when section is None; update the
preferences_url computation in send_scheduled_notifications (the preferences_url
assignment that uses section.space_slug) to unconditionally build the URL using
section.space_slug and settings.ROOT_DOMAIN (remove the `if section else ""`
fallback) so the code is simpler and clearer while preserving existing behavior.
- Line 173: Replace the EN DASH character (–) used inside the subject f-strings
with a standard ASCII hyphen-minus (-) for all subject definitions in this
module: update the subject=f"{section} – You've been matched with a buddy!" and
the two other subject f-strings (the ones analogous at lines near 193 and 218)
so they read subject=f"{section} - You've been matched with a buddy!" (and the
corresponding replacements for the other two subjects); search for subject=
f-strings in send_scheduled_notifications.py and make the same character
substitution where the EN DASH appears.
- Around line 200-213: The related_object parameter of
_send_member_joined_editors (typed SectionMembership) is currently ignored;
either use it as the trigger membership or document why it’s kept. Fix by
referencing related_object inside _send_member_joined_editors (for example treat
it as the triggering membership and include its state when computing
waiting_count or adjust the filter to exclude/include related_object as
intended) so the parameter isn’t wasted, or if the param must remain unused,
rename it to _related_object and add a brief comment explaining why it’s
intentionally unused; update any logic around waiting_count and the
SectionMembership import accordingly.
In `@fiesta/apps/notifications/tests/test_global_opt_out.py`:
- Around line 71-106: The test
test_enqueue_delayed_notification_skips_opted_out_and_creates_for_opted_in
passes a KnownSectionFactory instance as related_object for
NotificationKind.BUDDY_MATCHED_ISSUER which mismatches the expected
BuddyRequestMatch type and could break processing; update the test to supply a
proper BuddyRequestMatch instance (use an existing BuddyRequestMatchFactory or
create a minimal BuddyRequestMatch object) when calling
enqueue_delayed_notification, ensuring the created ScheduledNotification would
be valid for downstream processing, or alternatively add a clarifying comment in
the test near the enqueue_delayed_notification calls stating that section is
intentionally used only to exercise the opt-out gate and not full processing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 209109f4-3c4e-4e6a-848e-b261feaab12b
📒 Files selected for processing (5)
.github/workflows/tests.ymlfiesta/apps/events/migrations/0005_alter_pricevariant_amount_currency.pyfiesta/apps/notifications/management/commands/send_scheduled_notifications.pyfiesta/apps/notifications/tests/test_global_opt_out.pyfiesta/fiesta/settings/db.py
fiesta/apps/events/migrations/0005_alter_pricevariant_amount_currency.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
fiesta/apps/notifications/migrations/0001_initial.py (1)
38-56:⚠️ Potential issue | 🟠 MajorPending-notification dedupe still needs a DB guard.
The
ScheduledNotificationtable still lacks a partialUniqueConstraintto prevent duplicate pending rows with the same(kind, recipient, section, content_type, object_id). Without this, concurrent enqueues can create duplicates and send duplicate emails.Add a constraint in the model's Meta (or a follow-up migration):
models.UniqueConstraint( fields=["kind", "recipient", "section", "content_type", "object_id"], name="unique_pending_notification", condition=models.Q(sent_at__isnull=True, cancelled_at__isnull=True), )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiesta/apps/notifications/migrations/0001_initial.py` around lines 38 - 56, The ScheduledNotification model needs a DB-level partial unique constraint to prevent concurrent duplicate pending notifications; add a UniqueConstraint for fields kind, recipient, section, content_type and object_id with name unique_pending_notification and a condition that sent_at IS NULL and cancelled_at IS NULL (either by adding it to the model's Meta.constraints on ScheduledNotification or creating a follow-up migration that installs this constraint) so the database enforces dedupe of pending notifications.fiesta/apps/notifications/services/membership.py (1)
26-30:⚠️ Potential issue | 🟡 MinorAvoid catching bare
Exception.Catching all exceptions masks unexpected errors. The intent is to handle missing configuration, so catch the specific exception types that indicate a missing relation.
🛡️ Proposed fix
+from django.core.exceptions import ObjectDoesNotExist + def notify_new_membership(membership: SectionMembership) -> None: ... try: config = section.sections_plugin_configuration - except Exception: + except (ObjectDoesNotExist, AttributeError): logger.debug("No SectionsConfiguration for section %s, skipping membership notifications", section) return🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiesta/apps/notifications/services/membership.py` around lines 26 - 30, The code currently catches a bare Exception when accessing section.sections_plugin_configuration; change this to catch only the specific errors that indicate a missing relation (e.g., AttributeError and the ORM-specific DoesNotExist for the related object) instead of all exceptions. Replace the broad except Exception with an except tuple like (AttributeError, <RelatedObjectDoesNotExist>) where <RelatedObjectDoesNotExist> is the correct DoesNotExist class for the sections_plugin_configuration relation, keep the existing logger.debug message and return path, and import the ORM-specific exception if needed so only missing-configuration cases are suppressed.
🧹 Nitpick comments (1)
fiesta/apps/notifications/services/membership.py (1)
39-52: Consider centralizing URL construction.The preferences URL is constructed inline using f-string with
section.space_slugandsettings.ROOT_DOMAIN. If this pattern is used elsewhere, consider extracting it to a helper method on the Section model or a utility function to ensure consistency and easier maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@fiesta/apps/notifications/services/membership.py` around lines 39 - 52, The inline construction of preferences_url in _send_member_received_email duplicates URL-building logic; add a Section method (e.g., Section.get_preferences_url or Section.get_public_url) or a shared utility (e.g., build_section_url) that accepts a Section (or its space_slug) and returns the full preferences URL, then replace the f-string in _send_member_received_email to call that new helper and use its return value for preferences_url to centralize URL construction and ensure consistency across code.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@fiesta/apps/notifications/services/membership.py`:
- Line 47: Replace the EN DASH character in the subject string with a standard
hyphen-minus: find the subject assignment using the string f"{section} –
Application received" (the subject variable) and change the dash character to
"-" so it becomes f"{section} - Application received"; ensure no other Unicode
dash characters are used in that subject generation.
---
Duplicate comments:
In `@fiesta/apps/notifications/migrations/0001_initial.py`:
- Around line 38-56: The ScheduledNotification model needs a DB-level partial
unique constraint to prevent concurrent duplicate pending notifications; add a
UniqueConstraint for fields kind, recipient, section, content_type and object_id
with name unique_pending_notification and a condition that sent_at IS NULL and
cancelled_at IS NULL (either by adding it to the model's Meta.constraints on
ScheduledNotification or creating a follow-up migration that installs this
constraint) so the database enforces dedupe of pending notifications.
In `@fiesta/apps/notifications/services/membership.py`:
- Around line 26-30: The code currently catches a bare Exception when accessing
section.sections_plugin_configuration; change this to catch only the specific
errors that indicate a missing relation (e.g., AttributeError and the
ORM-specific DoesNotExist for the related object) instead of all exceptions.
Replace the broad except Exception with an except tuple like (AttributeError,
<RelatedObjectDoesNotExist>) where <RelatedObjectDoesNotExist> is the correct
DoesNotExist class for the sections_plugin_configuration relation, keep the
existing logger.debug message and return path, and import the ORM-specific
exception if needed so only missing-configuration cases are suppressed.
---
Nitpick comments:
In `@fiesta/apps/notifications/services/membership.py`:
- Around line 39-52: The inline construction of preferences_url in
_send_member_received_email duplicates URL-building logic; add a Section method
(e.g., Section.get_preferences_url or Section.get_public_url) or a shared
utility (e.g., build_section_url) that accepts a Section (or its space_slug) and
returns the full preferences URL, then replace the f-string in
_send_member_received_email to call that new helper and use its return value for
preferences_url to centralize URL construction and ensure consistency across
code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 8aaf45a1-8d67-40ba-b1f5-f39be061d340
📒 Files selected for processing (4)
fiesta/apps/notifications/migrations/0001_initial.pyfiesta/apps/notifications/services/membership.pyfiesta/apps/notifications/templates/notifications/buddy_system/matched_issuer.htmlfiesta/apps/notifications/templates/notifications/pickup_system/matched_issuer.html
🚧 Files skipped from review as they are similar to previous changes (2)
- fiesta/apps/notifications/templates/notifications/buddy_system/matched_issuer.html
- fiesta/apps/notifications/templates/notifications/pickup_system/matched_issuer.html
Member
Author
|
@copilot so with enabled notifications, which types are currently sent? |
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
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.
Summary by CodeRabbit
Release Notes