Feat/email service abstraction - #5192
Draft
HarshitVerma109 wants to merge 16 commits into
Draft
Conversation
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
Signed-off-by: Harsh <harshit1092004@gmail.com>
Contributor
Summary by CodeRabbit
WalkthroughChangesSnapshot subscription management
Email service abstraction
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: Stream initialization permanently failed: 8 RESOURCE_EXHAUSTED: Received message larger than max (219570039 vs 209715200) 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 |
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## feature/community-snapshots #5192 +/- ##
===============================================================
- Coverage 98.82% 98.75% -0.07%
===============================================================
Files 541 552 +11
Lines 17289 17624 +335
Branches 2496 2528 +32
===============================================================
+ Hits 17086 17405 +319
- Misses 88 94 +6
- Partials 115 125 +10
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report in Codecov by Harness.
🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@backend/apps/owasp/api/internal/mutations/snapshot_subscription.py`:
- Around line 275-299: The unsubscribe_by_token method exposes token state
through different messages for invalid and inactive subscriptions. Update both
failure paths to return the same generic unsubscribe error message, while
preserving the existing success response and deactivation behavior.
In `@backend/apps/owasp/services/email/django_email.py`:
- Around line 42-63: Update send_bulk to acquire one shared Django email
connection via get_connection, verify that API is supported by the project’s
declared Django version, and pass that connection through each message send
instead of invoking self.send() with a separately opened connection. Preserve
the existing sent/failed counting and per-message exception handling.
In
`@backend/tests/unit/apps/owasp/api/internal/mutations/snapshot_subscription_test.py`:
- Around line 59-61: Consolidate the repeated mutations fixture and
SnapshotSubscription.objects patching used by the test classes into module-level
pytest fixtures, such as mutations and mock_subscription_objects. Update
affected tests to consume these fixtures directly, removing duplicated patch
target strings and with-patch blocks while preserving existing mock behavior.
In `@backend/tests/unit/apps/owasp/api/internal/queries/chapter_test.py`:
- Around line 154-158: Strengthen test_search_chapters_whitespace_query by
mocking or spying on Chapter.active_chapters.filter and asserting it is not
called. Keep the whitespace-padded " ab " input and empty-result assertion so
the test proves stripping reduces the query below the minimum length and rejects
it before database access.
In `@backend/tests/unit/apps/owasp/services/email/django_email_test.py`:
- Around line 19-35: Update test_send_success to import django.conf.settings and
assert mock_email_class was called with from_email=settings.DEFAULT_FROM_EMAIL,
while preserving the existing success and message-behavior assertions.
In `@frontend/__tests__/unit/pages/SettingsPage.test.tsx`:
- Around line 90-94: Strengthen the loading-state tests in SettingsPage.test.tsx
by asserting that the rendered output contains the actual loading spinner or
indicator used by SettingsPage. Update both tests covering the initial and
subscription-loading states, while retaining the existing assertions that
loaded-state content is absent.
- Around line 183-193: Strengthen the frequency toggle test by asserting the
selected state changes after clicking “Monthly,” rather than only checking that
the option labels remain visible. Use the toggle’s exposed state, such as
aria-checked, checked, or its active class, and verify the expected pre-click
and post-click selections.
- Around line 44-84: Expand the SettingsPage test setup and cases to cover the
subscription query error state and the project/chapter search-and-select flow.
Update setupMocks to allow overriding the default error and mocked
useApolloClient().query results, then add assertions for error rendering and for
searching, selecting, and adding new project/chapter preferences through the UI.
In `@frontend/src/app/settings/page.tsx`:
- Around line 719-731: Update the Save and Unsubscribe controls in the settings
page so each is disabled while either operation is in flight: Save must account
for cancelling, and Unsubscribe must account for isSaving (including creating or
updating). Preserve their existing individual loading-state checks while
preventing either mutation from being triggered during the other’s pending
state.
- Around line 217-223: Update the dropdown focus/blur logic in the settings page
to retain the timer returned by the onBlur setTimeout, cancel it when the input
refocuses, and clear it during component unmount. Ensure refocusing preserves
the open dropdown and no delayed callback updates state after unmount.
- Around line 606-608: Update handleAddChapter to check whether the chapter is
already present in selectedChapters before appending it, matching the
deduplication behavior of handleAddProject. Return the existing selection
unchanged when the chapter is already selected, while preserving the current
append behavior for new chapters.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6468f2ef-0420-45cb-b07e-e2e74fe584b2
⛔ Files ignored due to path filters (3)
frontend/src/types/__generated__/chapterQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/subscriptionQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (43)
backend/apps/owasp/Makefilebackend/apps/owasp/admin/snapshot_subscription.pybackend/apps/owasp/api/internal/mutations/__init__.pybackend/apps/owasp/api/internal/mutations/snapshot_subscription.pybackend/apps/owasp/api/internal/nodes/chapter.pybackend/apps/owasp/api/internal/nodes/project_subscription_preference.pybackend/apps/owasp/api/internal/nodes/snapshot_subscription.pybackend/apps/owasp/api/internal/queries/__init__.pybackend/apps/owasp/api/internal/queries/chapter.pybackend/apps/owasp/api/internal/queries/snapshot_subscription.pybackend/apps/owasp/migrations/0076_add_project_subscription_preference.pybackend/apps/owasp/models/__init__.pybackend/apps/owasp/models/project_subscription_preference.pybackend/apps/owasp/models/snapshot_subscription.pybackend/apps/owasp/services/email/__init__.pybackend/apps/owasp/services/email/base.pybackend/apps/owasp/services/email/django_email.pybackend/apps/owasp/services/email/factory.pybackend/data/nest.dumpbackend/settings/base.pybackend/settings/graphql.pybackend/tests/unit/apps/owasp/admin/snapshot_subscription_test.pybackend/tests/unit/apps/owasp/api/internal/mutations/__init__.pybackend/tests/unit/apps/owasp/api/internal/mutations/snapshot_subscription_test.pybackend/tests/unit/apps/owasp/api/internal/nodes/project_subscription_preference_test.pybackend/tests/unit/apps/owasp/api/internal/nodes/snapshot_subscription_test.pybackend/tests/unit/apps/owasp/api/internal/queries/chapter_test.pybackend/tests/unit/apps/owasp/api/internal/queries/snapshot_subscription_test.pybackend/tests/unit/apps/owasp/models/project_subscription_preference_test.pybackend/tests/unit/apps/owasp/models/snapshot_subscription_test.pybackend/tests/unit/apps/owasp/services/__init__.pybackend/tests/unit/apps/owasp/services/email/__init__.pybackend/tests/unit/apps/owasp/services/email/base_test.pybackend/tests/unit/apps/owasp/services/email/django_email_test.pybackend/tests/unit/apps/owasp/services/email/factory_test.pyfrontend/__tests__/a11y/pages/SettingsPage.a11y.test.tsxfrontend/__tests__/mockData/mockSubscriptionData.tsfrontend/__tests__/unit/pages/SettingsPage.test.tsxfrontend/src/app/settings/page.tsxfrontend/src/components/ActionButton.tsxfrontend/src/components/UserMenu.tsxfrontend/src/server/queries/chapterQueries.tsfrontend/src/server/queries/subscriptionQueries.ts
Comment on lines
+275
to
+299
| @strawberry.mutation | ||
| def unsubscribe_by_token(self, token: str) -> SnapshotSubscriptionResult: | ||
| """Unsubscribe using a token from an email link. No auth required.""" | ||
| try: | ||
| subscription = SnapshotSubscription.objects.get(unsubscribe_token=token) | ||
| except (SnapshotSubscription.DoesNotExist, ValidationError): | ||
| return SnapshotSubscriptionResult( | ||
| ok=False, | ||
| message="Invalid unsubscribe token.", | ||
| ) | ||
|
|
||
| if not subscription.is_active: | ||
| return SnapshotSubscriptionResult( | ||
| ok=False, | ||
| message="Subscription is already inactive.", | ||
| ) | ||
|
|
||
| subscription.is_active = False | ||
| subscription.save() | ||
|
|
||
| return SnapshotSubscriptionResult( | ||
| ok=True, | ||
| message="Successfully unsubscribed.", | ||
| subscription=subscription, | ||
| ) |
Contributor
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the unsubscribe_token field type and generation mechanism.
rg -n 'unsubscribe_token' backend/apps/owasp/models/snapshot_subscription.py -C5Repository: OWASP/Nest
Length of output: 1983
Use a single generic unsubscribe error message The current responses reveal whether a token is valid but already used; return the same message for both paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/apps/owasp/api/internal/mutations/snapshot_subscription.py` around
lines 275 - 299, The unsubscribe_by_token method exposes token state through
different messages for invalid and inactive subscriptions. Update both failure
paths to return the same generic unsubscribe error message, while preserving the
existing success response and deactivation behavior.
Comment on lines
+42
to
+63
| def send_bulk(self, messages: list[dict]) -> dict: | ||
| """Send multiple emails, tracking success/failure counts.""" | ||
| results = {"sent": 0, "failed": 0} | ||
|
|
||
| for message in messages: | ||
| try: | ||
| success = self.send( | ||
| to=message["to"], | ||
| subject=message["subject"], | ||
| html_body=message["html_body"], | ||
| plain_body=message["plain_body"], | ||
| headers=message.get("headers"), | ||
| ) | ||
| if success: | ||
| results["sent"] += 1 | ||
| else: | ||
| results["failed"] += 1 | ||
| except Exception: | ||
| logger.exception("Failed to send email to %s", message.get("to")) | ||
| results["failed"] += 1 | ||
|
|
||
| return results |
Contributor
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Reuse a single SMTP connection in send_bulk.
Each self.send() call creates a new EmailMultiAlternatives which opens its own SMTP connection. For a batch of N messages this means N TCP connections, N SMTP handshakes, and N authentications. Django supports passing a shared connection to EmailMultiAlternatives, reducing this to a single connection for the entire batch.
♻️ Proposed refactor with shared connection
from django.conf import settings
from django.core.mail import EmailMultiAlternatives
+from django.core.mail.backends.smtp import EmailBackend
+from django.core.mail import get_connection
from apps.owasp.services.email.base import EmailService
def send_bulk(self, messages: list[dict]) -> dict:
"""Send multiple emails, tracking success/failure counts."""
results = {"sent": 0, "failed": 0}
+ connection = get_connection()
+ try:
+ connection.open()
+ except Exception:
+ logger.exception("Failed to open email connection")
+ return {"sent": 0, "failed": len(messages)}
- for message in messages:
- try:
- success = self.send(
- to=message["to"],
- subject=message["subject"],
- html_body=message["html_body"],
- plain_body=message["plain_body"],
- headers=message.get("headers"),
- )
- if success:
- results["sent"] += 1
- else:
- results["failed"] += 1
- except Exception:
- logger.exception("Failed to send email to %s", message.get("to"))
- results["failed"] += 1
+ try:
+ for message in messages:
+ try:
+ msg = EmailMultiAlternatives(
+ subject=message["subject"],
+ body=message["plain_body"],
+ from_email=settings.DEFAULT_FROM_EMAIL,
+ to=[message["to"]],
+ headers=message.get("headers"),
+ connection=connection,
+ )
+ msg.attach_alternative(message["html_body"], "text/html")
+ if msg.send() > 0:
+ results["sent"] += 1
+ else:
+ results["failed"] += 1
+ except Exception:
+ logger.exception("Failed to send email to %s", message.get("to"))
+ results["failed"] += 1
+ finally:
+ connection.close()
return resultsVerify that get_connection is available in the Django version declared in the project's requirements (pyproject.toml or requirements.txt).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def send_bulk(self, messages: list[dict]) -> dict: | |
| """Send multiple emails, tracking success/failure counts.""" | |
| results = {"sent": 0, "failed": 0} | |
| for message in messages: | |
| try: | |
| success = self.send( | |
| to=message["to"], | |
| subject=message["subject"], | |
| html_body=message["html_body"], | |
| plain_body=message["plain_body"], | |
| headers=message.get("headers"), | |
| ) | |
| if success: | |
| results["sent"] += 1 | |
| else: | |
| results["failed"] += 1 | |
| except Exception: | |
| logger.exception("Failed to send email to %s", message.get("to")) | |
| results["failed"] += 1 | |
| return results | |
| def send_bulk(self, messages: list[dict]) -> dict: | |
| """Send multiple emails, tracking success/failure counts.""" | |
| results = {"sent": 0, "failed": 0} | |
| connection = get_connection() | |
| try: | |
| connection.open() | |
| except Exception: | |
| logger.exception("Failed to open email connection") | |
| return {"sent": 0, "failed": len(messages)} | |
| try: | |
| for message in messages: | |
| try: | |
| msg = EmailMultiAlternatives( | |
| subject=message["subject"], | |
| body=message["plain_body"], | |
| from_email=settings.DEFAULT_FROM_EMAIL, | |
| to=[message["to"]], | |
| headers=message.get("headers"), | |
| connection=connection, | |
| ) | |
| msg.attach_alternative(message["html_body"], "text/html") | |
| if msg.send() > 0: | |
| results["sent"] += 1 | |
| else: | |
| results["failed"] += 1 | |
| except Exception: | |
| logger.exception("Failed to send email to %s", message.get("to")) | |
| results["failed"] += 1 | |
| finally: | |
| connection.close() | |
| return results |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/apps/owasp/services/email/django_email.py` around lines 42 - 63,
Update send_bulk to acquire one shared Django email connection via
get_connection, verify that API is supported by the project’s declared Django
version, and pass that connection through each message send instead of invoking
self.send() with a separately opened connection. Preserve the existing
sent/failed counting and per-message exception handling.
Comment on lines
+59
to
+61
| @pytest.fixture | ||
| def mutations(self): | ||
| return SnapshotSubscriptionMutations() |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Duplicated mutations fixture and patch target across all four test classes.
The mutations fixture and the "...snapshot_subscription.SnapshotSubscription.objects" patch target string repeat ~15+ times. Consider consolidating into a module-level fixture (e.g., in a shared base class or conftest.py) to reduce duplication and keep patch targets in sync if the module path changes.
♻️ Example consolidation
`@pytest.fixture`
def mutations():
return SnapshotSubscriptionMutations()
`@pytest.fixture`
def mock_subscription_objects():
with patch(
"apps.owasp.api.internal.mutations.snapshot_subscription.SnapshotSubscription.objects"
) as mock_objects:
yield mock_objectsThen each test can accept mock_subscription_objects directly instead of repeating the with patch(...) block.
Also applies to: 264-266, 402-404, 435-437
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@backend/tests/unit/apps/owasp/api/internal/mutations/snapshot_subscription_test.py`
around lines 59 - 61, Consolidate the repeated mutations fixture and
SnapshotSubscription.objects patching used by the test classes into module-level
pytest fixtures, such as mutations and mock_subscription_objects. Update
affected tests to consume these fixtures directly, removing duplicated patch
target strings and with-patch blocks while preserving existing mock behavior.
Comment on lines
+154
to
+158
| def test_search_chapters_whitespace_query(self): | ||
| """Test search_chapters strips whitespace before checking length.""" | ||
| query = ChapterQuery() | ||
| result = query.search_chapters(query=" ab ") | ||
| assert result == [] |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Whitespace test doesn't verify that stripping actually occurs.
The test only asserts result == [], which could pass even without stripping if the database returns empty results. Since Chapter.active_chapters is not mocked, a non-stripped query (" ab " has 6 chars, passes length check) would hit the real database and might return [] for unrelated reasons.
💚 Proposed fix: verify filter is not called
def test_search_chapters_whitespace_query(self):
"""Test search_chapters strips whitespace before checking length."""
query = ChapterQuery()
- result = query.search_chapters(query=" ab ")
- assert result == []
+ with patch.object(Chapter, "active_chapters") as mock_active:
+ result = query.search_chapters(query=" ab ")
+ mock_active.filter.assert_not_called()
+ assert result == []This verifies the query is rejected after stripping (stripped "ab" is 2 chars, below min length), proving active_chapters.filter is never reached.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_search_chapters_whitespace_query(self): | |
| """Test search_chapters strips whitespace before checking length.""" | |
| query = ChapterQuery() | |
| result = query.search_chapters(query=" ab ") | |
| assert result == [] | |
| def test_search_chapters_whitespace_query(self): | |
| """Test search_chapters strips whitespace before checking length.""" | |
| query = ChapterQuery() | |
| with patch.object(Chapter, "active_chapters") as mock_active: | |
| result = query.search_chapters(query=" ab ") | |
| mock_active.filter.assert_not_called() | |
| assert result == [] |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/unit/apps/owasp/api/internal/queries/chapter_test.py` around
lines 154 - 158, Strengthen test_search_chapters_whitespace_query by mocking or
spying on Chapter.active_chapters.filter and asserting it is not called. Keep
the whitespace-padded " ab " input and empty-result assertion so the test
proves stripping reduces the query below the minimum length and rejects it
before database access.
Comment on lines
+19
to
+35
| def test_send_success(self, mock_email_class, service): | ||
| """Test successful email sending.""" | ||
| mock_msg = MagicMock() | ||
| mock_msg.send.return_value = 1 | ||
| mock_email_class.return_value = mock_msg | ||
|
|
||
| result = service.send( | ||
| to="test@example.com", | ||
| subject="Test Subject", | ||
| html_body="<h1>Hello</h1>", | ||
| plain_body="Hello", | ||
| ) | ||
|
|
||
| assert result is True | ||
| mock_email_class.assert_called_once() | ||
| mock_msg.attach_alternative.assert_called_once_with("<h1>Hello</h1>", "text/html") | ||
| mock_msg.send.assert_called_once() |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Assert from_email in test_send_success.
The test verifies attach_alternative and send calls but never checks that from_email is set to settings.DEFAULT_FROM_EMAIL. A misconfigured sender address could cause deliverability failures or send from the wrong identity — worth catching at test time.
✅ Proposed addition
assert result is True
mock_email_class.assert_called_once()
+ call_kwargs = mock_email_class.call_args[1]
+ assert call_kwargs["from_email"] == settings.DEFAULT_FROM_EMAIL
+ assert call_kwargs["to"] == ["test@example.com"]
mock_msg.attach_alternative.assert_called_once_with("<h1>Hello</h1>", "text/html")
mock_msg.send.assert_called_once()You'll also need to add from django.conf import settings to the test file imports.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@backend/tests/unit/apps/owasp/services/email/django_email_test.py` around
lines 19 - 35, Update test_send_success to import django.conf.settings and
assert mock_email_class was called with from_email=settings.DEFAULT_FROM_EMAIL,
while preserving the existing success and message-behavior assertions.
Comment on lines
+90
to
+94
| test('shows loading spinner when session is loading', () => { | ||
| setupMocks({}, { status: 'loading' }) | ||
| render(<SettingsPage />) | ||
| expect(screen.queryByText('Settings')).not.toBeInTheDocument() | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Loading-state tests don't assert a spinner is actually shown.
Both tests are named "shows loading spinner..." but only assert the absence of unrelated text ('Settings', 'Not Subscribed', 'Subscription Active'). They would still pass if the component rendered a blank screen, an error, or nothing at all — they don't prove a loading indicator is present.
Suggested strengthening
test('shows loading spinner when session is loading', () => {
setupMocks({}, { status: 'loading' })
render(<SettingsPage />)
expect(screen.queryByText('Settings')).not.toBeInTheDocument()
+ expect(screen.getByRole('status')).toBeInTheDocument() // or getByTestId('loading-spinner')
}) test('shows loading spinner when query is loading', () => {
setupMocks({ loading: true })
render(<SettingsPage />)
expect(screen.queryByText('Not Subscribed')).not.toBeInTheDocument()
expect(screen.queryByText('Subscription Active')).not.toBeInTheDocument()
+ expect(screen.getByRole('status')).toBeInTheDocument() // or getByTestId('loading-spinner')
})Also applies to: 285-292
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/__tests__/unit/pages/SettingsPage.test.tsx` around lines 90 - 94,
Strengthen the loading-state tests in SettingsPage.test.tsx by asserting that
the rendered output contains the actual loading spinner or indicator used by
SettingsPage. Update both tests covering the initial and subscription-loading
states, while retaining the existing assertions that loaded-state content is
absent.
Comment on lines
+183
to
+193
| describe('Frequency Selection', () => { | ||
| test('can switch between Weekly and Monthly', () => { | ||
| setupMocks({ data: mockNoSubscription }) | ||
| render(<SettingsPage />) | ||
|
|
||
| fireEvent.click(screen.getByText('Monthly')) | ||
|
|
||
| expect(screen.getByText('Monthly')).toBeInTheDocument() | ||
| expect(screen.getByText('Weekly')).toBeInTheDocument() | ||
| }) | ||
| }) |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Test doesn't verify the frequency toggle actually changed selection state.
This only checks that both 'Weekly' and 'Monthly' text remain present after clicking — which is already true before any click is fired (per the "renders frequency options" test at Lines 141-146), so this test would pass even if clicking Monthly had no effect. The real behavioral verification lives in the "Mutation Payload" test ("Subscribe sends selected frequency"); this test adds little on its own. Consider asserting on the selected/active state directly (e.g. aria-checked, a checked radio, or an active class) instead of label presence.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/__tests__/unit/pages/SettingsPage.test.tsx` around lines 183 - 193,
Strengthen the frequency toggle test by asserting the selected state changes
after clicking “Monthly,” rather than only checking that the option labels
remain visible. Use the toggle’s exposed state, such as aria-checked, checked,
or its active class, and verify the expected pre-click and post-click
selections.
Comment on lines
+217
to
+223
| onChange={(e) => { | ||
| setInputValue(e.target.value) | ||
| setShowDropdown(true) | ||
| }} | ||
| onFocus={() => setShowDropdown(true)} | ||
| onBlur={() => setTimeout(() => setShowDropdown(false), 200)} | ||
| placeholder={selectedItems.length > 0 ? '' : `Search ${label.toLowerCase()}...`} |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Blur-close timer isn't cancelled, so dropdown can close unexpectedly.
onBlur schedules setShowDropdown(false) after 200ms with a bare setTimeout. If the user refocuses the input (or the component unmounts) within that window, the pending timer still fires and force-closes the dropdown (or updates state after unmount), causing a flicker/incorrect close even though the input is focused again.
🔧 Proposed fix
+ const blurTimeoutRef = useRef<ReturnType<typeof setTimeout>>()
+
+ useEffect(() => () => clearTimeout(blurTimeoutRef.current), [])
+
...
- onFocus={() => setShowDropdown(true)}
- onBlur={() => setTimeout(() => setShowDropdown(false), 200)}
+ onFocus={() => {
+ clearTimeout(blurTimeoutRef.current)
+ setShowDropdown(true)
+ }}
+ onBlur={() => {
+ blurTimeoutRef.current = setTimeout(() => setShowDropdown(false), 200)
+ }}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/app/settings/page.tsx` around lines 217 - 223, Update the
dropdown focus/blur logic in the settings page to retain the timer returned by
the onBlur setTimeout, cancel it when the input refocuses, and clear it during
component unmount. Ensure refocusing preserves the open dropdown and no delayed
callback updates state after unmount.
Comment on lines
+606
to
+608
| const handleAddChapter = useCallback((item: EntityItem) => { | ||
| setSelectedChapters((prev) => [...prev, item]) | ||
| }, []) |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
handleAddChapter doesn't dedupe, unlike handleAddProject.
handleAddProject guards against re-adding an already-selected item (if (prev.some(...)) return prev), but handleAddChapter pushes unconditionally. This inconsistency allows the same chapter to be appended twice into selectedChapters, which then gets sent as duplicate subscribedChapterIds.
🔧 Proposed fix
const handleAddChapter = useCallback((item: EntityItem) => {
- setSelectedChapters((prev) => [...prev, item])
+ setSelectedChapters((prev) => {
+ if (prev.some((c) => c.id === item.id)) return prev
+ return [...prev, item]
+ })
}, [])📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| const handleAddChapter = useCallback((item: EntityItem) => { | |
| setSelectedChapters((prev) => [...prev, item]) | |
| }, []) | |
| const handleAddChapter = useCallback((item: EntityItem) => { | |
| setSelectedChapters((prev) => { | |
| if (prev.some((c) => c.id === item.id)) return prev | |
| return [...prev, item] | |
| }) | |
| }, []) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/app/settings/page.tsx` around lines 606 - 608, Update
handleAddChapter to check whether the chapter is already present in
selectedChapters before appending it, matching the deduplication behavior of
handleAddProject. Return the existing selection unchanged when the chapter is
already selected, while preserving the current append behavior for new chapters.
Comment on lines
+719
to
+731
| <div className="flex justify-end gap-3"> | ||
| {hasActiveSubscription && ( | ||
| <Button | ||
| variant="bordered" | ||
| onPress={handleCancel} | ||
| isDisabled={cancelling} | ||
| className={destructiveButtonStyles} | ||
| > | ||
| <FaBellSlash /> | ||
| {cancelling ? 'Cancelling...' : 'Unsubscribe'} | ||
| </Button> | ||
| )} | ||
| <ActionButton onClick={handleSave} isDisabled={isSaving}> |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Save and Unsubscribe actions don't account for each other's in-flight state.
The Unsubscribe button is only disabled by cancelling, and the Save button only by isSaving (creating || updating). A user can trigger both nearly simultaneously (e.g., click Unsubscribe, then immediately click Save before the cancel mutation resolves), letting two mutations race and leaving the subscription in an unpredictable final state.
🔧 Proposed fix
<Button
variant="bordered"
onPress={handleCancel}
- isDisabled={cancelling}
+ isDisabled={cancelling || isSaving}
className={destructiveButtonStyles}
>- <ActionButton onClick={handleSave} isDisabled={isSaving}>
+ <ActionButton onClick={handleSave} isDisabled={isSaving || cancelling}>🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@frontend/src/app/settings/page.tsx` around lines 719 - 731, Update the Save
and Unsubscribe controls in the settings page so each is disabled while either
operation is in flight: Save must account for cancelling, and Unsubscribe must
account for isSaving (including creating or updating). Preserve their existing
individual loading-state checks while preventing either mutation from being
triggered during the other’s pending state.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
17 issues found across 46 files
Confidence score: 2/5
- The highest-risk item is the migration in
backend/apps/owasp/migrations/0076_add_project_subscription_preference.py: it can drop users’ existing project selections and issue/PR/release preferences during upgrade, which is direct user-facing data loss. BackfillProjectSubscriptionPreferencefrom the old M2M/boolean fields before removing legacy fields, and verify with a migration test before merging. backend/apps/owasp/api/internal/mutations/snapshot_subscription.pyhas concrete mutation-path failures: invalid chapter IDs can produce a 500 inupdate_snapshot_subscriptionand a misleading “already exists” message increate_snapshot_subscription, so clients will see incorrect or unstable error behavior. Validate chapter IDs up front (or narrowIntegrityErrorhandling) and return explicit GraphQL errors for invalid input.- The same mutation module has a stale-write race where
update_snapshot_subscriptioncan overwrite a concurrent cancellation and flipis_activeback to true, effectively undoing an unsubscribe. Re-fetch/lock the subscription row in the update path and save only intended fields to prevent lost updates. - On
frontend/src/app/settings/page.tsx, UI-state races (uncancelledsetTimeoutblur handling, duplicate chapter adds, and overlapping Save/Unsubscribe actions) can send inconsistent subscription payloads and cause confusing dropdown behavior. De-risk by clearing pending timers on refocus/unmount, deduplicatingselectedChapters, and disabling conflicting actions while either mutation is in flight.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="backend/apps/owasp/services/email/django_email.py">
<violation number="1" location="backend/apps/owasp/services/email/django_email.py:46">
P2: Large bulk sends open and close one SMTP/provider connection per recipient, adding latency and risking provider connection limits. Build the messages with one backend connection and send them together (or open one shared connection around this loop).</violation>
</file>
<file name="backend/apps/owasp/api/internal/mutations/snapshot_subscription.py">
<violation number="1" location="backend/apps/owasp/api/internal/mutations/snapshot_subscription.py:116">
P2: A concurrent cancellation can be undone by this update: after `cancel_snapshot_subscription` saves `is_active=False`, this stale instance saves all fields with its earlier `is_active=True`. Fetch and lock the subscription inside the transaction (or use targeted updates) so cancellation/unsubscribe cannot be lost.</violation>
<violation number="2" location="backend/apps/owasp/api/internal/mutations/snapshot_subscription.py:119">
P1: Passing a non-existent chapter ID to `update_snapshot_subscription` results in a 500 error instead of a user-friendly GraphQL error response. The `chapters.set(invalid_id)` call raises an `IntegrityError` at the database level (FK violation on the M2M through table), but the mutation's exception handler only catches `ValueError`. The `IntegrityError` propagates unhandled. Add an `except IntegrityError` clause that returns a clear error message, or validate chapter IDs before calling `set()`.</violation>
<violation number="3" location="backend/apps/owasp/api/internal/mutations/snapshot_subscription.py:139">
P1: Providing an invalid chapter ID in `create_snapshot_subscription` returns "Subscription already exists." — a completely misleading error. The `except IntegrityError` on line 163 catches ALL IntegrityErrors from the `transaction.atomic()` block, but the message only describes one possible cause (the `get_or_create` race). Adding `chapters.set()` with invalid chapter IDs or unexpected constraint violations (e.g., the `UniqueConstraint` on `subscription` + `project` in `ProjectSubscriptionPreference`) also hit this handler and produce the wrong message. Either narrow the handler to only catch the expected `IntegrityError` sources with distinct messages, or validate chapter/project IDs before making DB calls.</violation>
</file>
<file name="backend/tests/unit/apps/owasp/api/internal/queries/snapshot_subscription_test.py">
<violation number="1" location="backend/tests/unit/apps/owasp/api/internal/queries/snapshot_subscription_test.py:51">
P2: The tests for `my_subscription` don't verify that `objects.get()` is called with the correct `user` argument. Without this assertion, the tests would pass even if the resolver changed to `get(email=...)` or `get(id=...)`. Add `mock_objects.get.assert_called_once_with(user=info.context.request.user)` to both `test_my_subscription_not_found` and `test_my_subscription_found`.</violation>
</file>
<file name="backend/apps/owasp/migrations/0076_add_project_subscription_preference.py">
<violation number="1" location="backend/apps/owasp/migrations/0076_add_project_subscription_preference.py:31">
P1: Existing digest subscriptions lose all project selections and issue/PR/release settings during upgrade. Create and backfill `ProjectSubscriptionPreference` rows from the old M2M and booleans before removing the old fields.</violation>
</file>
<file name="backend/tests/unit/apps/owasp/api/internal/nodes/project_subscription_preference_test.py">
<violation number="1" location="backend/tests/unit/apps/owasp/api/internal/nodes/project_subscription_preference_test.py:26">
P3: The `project` field's custom resolver (with `select_related` optimization) is untested — the test only checks field name presence. Adding a simple integration or snapshot test that builds a real node instance and resolves the `project` field would surface future regressions in the resolver logic.</violation>
</file>
<file name="frontend/src/app/settings/page.tsx">
<violation number="1" location="frontend/src/app/settings/page.tsx:222">
P2: The `setTimeout` in `onBlur` is not cancelled when the user refocuses the input or the component unmounts. This means the pending timer can fire and force-close the dropdown even while the input is focused (causing a flicker), or update state on an unmounted component.
Consider storing the timeout ID in a `useRef`, clearing it in `onFocus`, and clearing it on unmount via a cleanup `useEffect`.</violation>
<violation number="2" location="frontend/src/app/settings/page.tsx:275">
P2: Project content toggles do not announce whether Issues, Pull Requests, or Releases is enabled. Add the existing `isOn` value as button state.</violation>
<violation number="3" location="frontend/src/app/settings/page.tsx:321">
P2: Frequency selection state is not announced to assistive technology. Expose the selected state (or model the group as radios) so Weekly versus Monthly is discernible without the visual indicator.</violation>
<violation number="4" location="frontend/src/app/settings/page.tsx:361">
P2: General subscription toggles do not announce whether each content type is enabled. Add the current pressed/checked state to the button.</violation>
<violation number="5" location="frontend/src/app/settings/page.tsx:606">
P2: `handleAddChapter` pushes items unconditionally, unlike `handleAddProject` which guards against re-adding the same item. This allows duplicate chapters in `selectedChapters`, which would send duplicate `subscribedChapterIds` to the mutation.</violation>
<violation number="6" location="frontend/src/app/settings/page.tsx:724">
P2: The Unsubscribe button is only disabled when `cancelling` is true and the Save button only when `isSaving` is true. A user could click one then quickly the other before the first mutation resolves, causing a race between cancel and save/create mutations. Consider disabling both buttons with `cancelling || isSaving`.</violation>
</file>
<file name="backend/apps/owasp/api/internal/queries/chapter.py">
<violation number="1" location="backend/apps/owasp/api/internal/queries/chapter.py:11">
P3: `MIN_SEARCH_QUERY_LENGTH = 3` and `MAX_SEARCH_QUERY_LENGTH = 100` duplicate constants already defined in `project.py` with the same values. If query-length bounds ever need to change, these will drift silently. Consider extracting to a shared location or re-exporting from a common queries module to keep validation consistent across search resolvers.</violation>
</file>
<file name="backend/tests/unit/apps/owasp/admin/snapshot_subscription_test.py">
<violation number="1" location="backend/tests/unit/apps/owasp/admin/snapshot_subscription_test.py:24">
P3: The `test_admin_configuration` test doesn't assert the `inlines` attribute on the admin instance. Since `ProjectSubscriptionPreferenceInline` was newly added as part of these changes, verifying its presence in the test would confirm it's correctly wired.</violation>
</file>
<file name="backend/apps/owasp/services/email/base.py">
<violation number="1" location="backend/apps/owasp/services/email/base.py:37">
P2: `send_bulk` calls `self.send()` in a loop, creating a new SMTP connection for each message. For N messages this means N TCP connections, N TLS handshakes, and N authentications. Django's `get_connection()` can be shared across multiple `EmailMultiAlternatives` instances via the `connection` parameter, reducing overhead to a single connection for the entire batch.</violation>
</file>
<file name="frontend/__tests__/unit/pages/SettingsPage.test.tsx">
<violation number="1" location="frontend/__tests__/unit/pages/SettingsPage.test.tsx:286">
P3: This test is named 'shows loading spinner when query is loading' but only asserts the *absence* of certain text (`'Not Subscribed'`, `'Subscription Active'`). It would still pass if the component rendered nothing at all. Consider adding a positive assertion for the loading indicator (e.g., `getByRole('status')` or a test-id).</violation>
</file>
Tip: cubic can generate docs of your entire codebase and keep them up to date. Try it here.
Re-trigger cubic
| ), | ||
| migrations.RemoveField( | ||
| model_name="snapshotsubscription", | ||
| name="subscribed_projects", |
Contributor
There was a problem hiding this comment.
P1: Existing digest subscriptions lose all project selections and issue/PR/release settings during upgrade. Create and backfill ProjectSubscriptionPreference rows from the old M2M and booleans before removing the old fields.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/apps/owasp/migrations/0076_add_project_subscription_preference.py, line 31:
<comment>Existing digest subscriptions lose all project selections and issue/PR/release settings during upgrade. Create and backfill `ProjectSubscriptionPreference` rows from the old M2M and booleans before removing the old fields.</comment>
<file context>
@@ -0,0 +1,79 @@
+ ),
+ migrations.RemoveField(
+ model_name="snapshotsubscription",
+ name="subscribed_projects",
+ ),
+ migrations.RenameField(
</file context>
| user = info.context.request.user | ||
|
|
||
| if input_data.frequency not in dict(SnapshotSubscription.Frequency.choices): | ||
| return SnapshotSubscriptionResult( |
Contributor
There was a problem hiding this comment.
P1: Providing an invalid chapter ID in create_snapshot_subscription returns "Subscription already exists." — a completely misleading error. The except IntegrityError on line 163 catches ALL IntegrityErrors from the transaction.atomic() block, but the message only describes one possible cause (the get_or_create race). Adding chapters.set() with invalid chapter IDs or unexpected constraint violations (e.g., the UniqueConstraint on subscription + project in ProjectSubscriptionPreference) also hit this handler and produce the wrong message. Either narrow the handler to only catch the expected IntegrityError sources with distinct messages, or validate chapter/project IDs before making DB calls.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/apps/owasp/api/internal/mutations/snapshot_subscription.py, line 139:
<comment>Providing an invalid chapter ID in `create_snapshot_subscription` returns "Subscription already exists." — a completely misleading error. The `except IntegrityError` on line 163 catches ALL IntegrityErrors from the `transaction.atomic()` block, but the message only describes one possible cause (the `get_or_create` race). Adding `chapters.set()` with invalid chapter IDs or unexpected constraint violations (e.g., the `UniqueConstraint` on `subscription` + `project` in `ProjectSubscriptionPreference`) also hit this handler and produce the wrong message. Either narrow the handler to only catch the expected `IntegrityError` sources with distinct messages, or validate chapter/project IDs before making DB calls.</comment>
<file context>
@@ -0,0 +1,299 @@
+ user = info.context.request.user
+
+ if input_data.frequency not in dict(SnapshotSubscription.Frequency.choices):
+ return SnapshotSubscriptionResult(
+ ok=False,
+ message="Invalid frequency. Must be 'weekly' or 'monthly'.",
</file context>
| subscription.save() | ||
|
|
||
| if input_data.subscribed_chapter_ids is not None: | ||
| subscription.chapters.set(input_data.subscribed_chapter_ids) |
Contributor
There was a problem hiding this comment.
P1: Passing a non-existent chapter ID to update_snapshot_subscription results in a 500 error instead of a user-friendly GraphQL error response. The chapters.set(invalid_id) call raises an IntegrityError at the database level (FK violation on the M2M through table), but the mutation's exception handler only catches ValueError. The IntegrityError propagates unhandled. Add an except IntegrityError clause that returns a clear error message, or validate chapter IDs before calling set().
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/apps/owasp/api/internal/mutations/snapshot_subscription.py, line 119:
<comment>Passing a non-existent chapter ID to `update_snapshot_subscription` results in a 500 error instead of a user-friendly GraphQL error response. The `chapters.set(invalid_id)` call raises an `IntegrityError` at the database level (FK violation on the M2M through table), but the mutation's exception handler only catches `ValueError`. The `IntegrityError` propagates unhandled. Add an `except IntegrityError` clause that returns a clear error message, or validate chapter IDs before calling `set()`.</comment>
<file context>
@@ -0,0 +1,299 @@
+ subscription.save()
+
+ if input_data.subscribed_chapter_ids is not None:
+ subscription.chapters.set(input_data.subscribed_chapter_ids)
+
+ if input_data.project_preferences is not None:
</file context>
| """Send multiple emails, tracking success/failure counts.""" | ||
| results = {"sent": 0, "failed": 0} | ||
|
|
||
| for message in messages: |
Contributor
There was a problem hiding this comment.
P2: Large bulk sends open and close one SMTP/provider connection per recipient, adding latency and risking provider connection limits. Build the messages with one backend connection and send them together (or open one shared connection around this loop).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/apps/owasp/services/email/django_email.py, line 46:
<comment>Large bulk sends open and close one SMTP/provider connection per recipient, adding latency and risking provider connection limits. Build the messages with one backend connection and send them together (or open one shared connection around this loop).</comment>
<file context>
@@ -0,0 +1,63 @@
+ """Send multiple emails, tracking success/failure counts."""
+ results = {"sent": 0, "failed": 0}
+
+ for message in messages:
+ try:
+ success = self.send(
</file context>
| subscription.include_events = input_data.include_events | ||
| subscription.include_posts = input_data.include_posts | ||
| subscription.include_users = input_data.include_users | ||
| subscription.save() |
Contributor
There was a problem hiding this comment.
P2: A concurrent cancellation can be undone by this update: after cancel_snapshot_subscription saves is_active=False, this stale instance saves all fields with its earlier is_active=True. Fetch and lock the subscription inside the transaction (or use targeted updates) so cancellation/unsubscribe cannot be lost.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/apps/owasp/api/internal/mutations/snapshot_subscription.py, line 116:
<comment>A concurrent cancellation can be undone by this update: after `cancel_snapshot_subscription` saves `is_active=False`, this stale instance saves all fields with its earlier `is_active=True`. Fetch and lock the subscription inside the transaction (or use targeted updates) so cancellation/unsubscribe cannot be lost.</comment>
<file context>
@@ -0,0 +1,299 @@
+ subscription.include_events = input_data.include_events
+ subscription.include_posts = input_data.include_posts
+ subscription.include_users = input_data.include_users
+ subscription.save()
+
+ if input_data.subscribed_chapter_ids is not None:
</file context>
Comment on lines
+606
to
+608
| const handleAddChapter = useCallback((item: EntityItem) => { | ||
| setSelectedChapters((prev) => [...prev, item]) | ||
| }, []) |
Contributor
There was a problem hiding this comment.
P2: handleAddChapter pushes items unconditionally, unlike handleAddProject which guards against re-adding the same item. This allows duplicate chapters in selectedChapters, which would send duplicate subscribedChapterIds to the mutation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/src/app/settings/page.tsx, line 606:
<comment>`handleAddChapter` pushes items unconditionally, unlike `handleAddProject` which guards against re-adding the same item. This allows duplicate chapters in `selectedChapters`, which would send duplicate `subscribedChapterIds` to the mutation.</comment>
<file context>
@@ -0,0 +1,829 @@
+ )
+ }, [])
+
+ const handleAddChapter = useCallback((item: EntityItem) => {
+ setSelectedChapters((prev) => [...prev, item])
+ }, [])
</file context>
Suggested change
| const handleAddChapter = useCallback((item: EntityItem) => { | |
| setSelectedChapters((prev) => [...prev, item]) | |
| }, []) | |
| const handleAddChapter = useCallback((item: EntityItem) => { | |
| setSelectedChapters((prev) => { | |
| if (prev.some((c) => c.id === item.id)) return prev | |
| return [...prev, item] | |
| }) | |
| }, []) |
| "include_issues", | ||
| "include_pull_requests", | ||
| "include_releases", | ||
| "project", |
Contributor
There was a problem hiding this comment.
P3: The project field's custom resolver (with select_related optimization) is untested — the test only checks field name presence. Adding a simple integration or snapshot test that builds a real node instance and resolves the project field would surface future regressions in the resolver logic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/unit/apps/owasp/api/internal/nodes/project_subscription_preference_test.py, line 26:
<comment>The `project` field's custom resolver (with `select_related` optimization) is untested — the test only checks field name presence. Adding a simple integration or snapshot test that builds a real node instance and resolves the `project` field would surface future regressions in the resolver logic.</comment>
<file context>
@@ -0,0 +1,28 @@
+ "include_issues",
+ "include_pull_requests",
+ "include_releases",
+ "project",
+ }
+ assert expected_field_names.issubset(field_names)
</file context>
| from apps.owasp.models.chapter import Chapter | ||
|
|
||
| MAX_LIMIT = 1000 | ||
| MAX_SEARCH_QUERY_LENGTH = 100 |
Contributor
There was a problem hiding this comment.
P3: MIN_SEARCH_QUERY_LENGTH = 3 and MAX_SEARCH_QUERY_LENGTH = 100 duplicate constants already defined in project.py with the same values. If query-length bounds ever need to change, these will drift silently. Consider extracting to a shared location or re-exporting from a common queries module to keep validation consistent across search resolvers.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/apps/owasp/api/internal/queries/chapter.py, line 11:
<comment>`MIN_SEARCH_QUERY_LENGTH = 3` and `MAX_SEARCH_QUERY_LENGTH = 100` duplicate constants already defined in `project.py` with the same values. If query-length bounds ever need to change, these will drift silently. Consider extracting to a shared location or re-exporting from a common queries module to keep validation consistent across search resolvers.</comment>
<file context>
@@ -8,6 +8,9 @@
from apps.owasp.models.chapter import Chapter
MAX_LIMIT = 1000
+MAX_SEARCH_QUERY_LENGTH = 100
+MIN_SEARCH_QUERY_LENGTH = 3
+SEARCH_CHAPTERS_LIMIT = 3
</file context>
| """Test admin configuration matches expected setup.""" | ||
| site = AdminSite() | ||
| admin = SnapshotSubscriptionAdmin(SnapshotSubscription, site) | ||
| admin_instance = SnapshotSubscriptionAdmin(SnapshotSubscription, site) |
Contributor
There was a problem hiding this comment.
P3: The test_admin_configuration test doesn't assert the inlines attribute on the admin instance. Since ProjectSubscriptionPreferenceInline was newly added as part of these changes, verifying its presence in the test would confirm it's correctly wired.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At backend/tests/unit/apps/owasp/admin/snapshot_subscription_test.py, line 24:
<comment>The `test_admin_configuration` test doesn't assert the `inlines` attribute on the admin instance. Since `ProjectSubscriptionPreferenceInline` was newly added as part of these changes, verifying its presence in the test would confirm it's correctly wired.</comment>
<file context>
@@ -9,46 +10,50 @@
"""Test admin configuration matches expected setup."""
site = AdminSite()
- admin = SnapshotSubscriptionAdmin(SnapshotSubscription, site)
+ admin_instance = SnapshotSubscriptionAdmin(SnapshotSubscription, site)
- assert admin.list_display == (
</file context>
| }) | ||
|
|
||
| describe('Loading State', () => { | ||
| test('shows loading spinner when query is loading', () => { |
Contributor
There was a problem hiding this comment.
P3: This test is named 'shows loading spinner when query is loading' but only asserts the absence of certain text ('Not Subscribed', 'Subscription Active'). It would still pass if the component rendered nothing at all. Consider adding a positive assertion for the loading indicator (e.g., getByRole('status') or a test-id).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At frontend/__tests__/unit/pages/SettingsPage.test.tsx, line 286:
<comment>This test is named 'shows loading spinner when query is loading' but only asserts the *absence* of certain text (`'Not Subscribed'`, `'Subscription Active'`). It would still pass if the component rendered nothing at all. Consider adding a positive assertion for the loading indicator (e.g., `getByRole('status')` or a test-id).</comment>
<file context>
@@ -0,0 +1,315 @@
+ })
+
+ describe('Loading State', () => {
+ test('shows loading spinner when query is loading', () => {
+ setupMocks({ loading: true })
+ render(<SettingsPage />)
</file context>
HarshitVerma109
marked this pull request as draft
July 12, 2026 19:55
HarshitVerma109
added
the
gsoc2026:harshitverma109
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
1 participant
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.
Proposed change
Resolves #5191
This PR adds the email service abstraction layer for the backend. It provides a simple way to send emails and easily swap between different email providers in the future.
Checklist