Feature/snapshot subscription model#5021
Open
HarshitVerma109 wants to merge 3 commits into
Open
Conversation
Contributor
|
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:
WalkthroughAdds the ChangesSnapshotSubscription Model and Admin
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## feature/community-snapshots #5021 +/- ##
============================================================
Coverage 98.82% 98.82%
============================================================
Files 541 543 +2
Lines 17289 17333 +44
Branches 2496 2496
============================================================
+ Hits 17086 17130 +44
Misses 88 88
Partials 115 115
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: 4
🤖 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/models/snapshot_subscription.py`:
- Around line 18-21: The indexes list in the Meta class of the
SnapshotSubscription model contains a redundant index on the unsubscribe_token
field. Since this field already has unique=True defined on it, which
automatically creates an index, the explicit Index entry with
fields=["unsubscribe_token"] and name="owasp_sub_token_idx" should be removed
from the indexes list to eliminate unnecessary write and index maintenance
overhead. Keep only the index on the is_active field in the indexes list.
In `@backend/tests/unit/apps/owasp/api/internal/nodes/snapshot_test.py`:
- Around line 74-79: The test currently verifies that order_by is called with
"-created_at" but does not explicitly assert the slice bounds passed to
__getitem__. Add an assert_called_once_with assertion on the
mock_snapshot.issues.order_by.return_value.__getitem__ mock to verify it is
being called with the correct slice that uses RECENT_ISSUES_LIMIT as the upper
bound. This will ensure the resolver is applying the expected limit to the
queryset results.
In `@backend/tests/unit/apps/owasp/models/snapshot_subscription_test.py`:
- Around line 33-55: The test_content_preferences_all_defaults test uses a
MagicMock with manually configured attributes instead of testing actual model
defaults, so field default regressions would not be caught. Replace the
MagicMock creation with an actual instance of SnapshotSubscription created
without manually setting attributes, allowing the test to verify that
content_preferences returns the expected dictionary based on the real default
values defined in the model.
In `@docker-compose/local/compose.yaml`:
- Around line 129-135: The volume names in the docker-compose file have been
renamed to include the `-community-snapshots` suffix
(backend-venv-community-snapshots, cache-data-community-snapshots,
frontend-next-community-snapshots, frontend-node-modules-community-snapshots),
but the cleanup targets in both backend/Makefile (lines 30-39) and
frontend/Makefile (lines 23-32) still reference the old volume names
(nest-local_backend-venv, nest-local_cache-data, nest-local_frontend-next,
nest-local_frontend-node-modules). Update both Makefile cleanup targets to use
the new volume names with the `-community-snapshots` suffix so that local reset
commands properly remove all volumes and prevent stale environments.
🪄 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: 9a3a6a14-259d-4d80-bb8d-ca68606f0c90
⛔ Files ignored due to path filters (22)
frontend/src/types/__generated__/EntityActions.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/aboutQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/apiKeyQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/authQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/boardQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/chapterQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/committeeQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/graphql.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/homeQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/issueQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/menteeQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/mentorshipQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/moduleMutations.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/moduleQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/organizationQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/programsMutations.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/programsQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/projectQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/projectsHealthDashboardQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/repositoryQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/snapshotQueries.generated.tsis excluded by!**/__generated__/**frontend/src/types/__generated__/userQueries.generated.tsis excluded by!**/__generated__/**
📒 Files selected for processing (24)
backend/apps/api/rest/v0/snapshot.pybackend/apps/owasp/admin/__init__.pybackend/apps/owasp/admin/snapshot.pybackend/apps/owasp/admin/snapshot_subscription.pybackend/apps/owasp/api/internal/nodes/snapshot.pybackend/apps/owasp/management/commands/owasp_process_snapshots.pybackend/apps/owasp/migrations/0073_add_snapshot_frequency_events_posts_pull_requests.pybackend/apps/owasp/migrations/0074_rename_snapshot_m2m_fields.pybackend/apps/owasp/migrations/0075_snapshotsubscription.pybackend/apps/owasp/models/__init__.pybackend/apps/owasp/models/snapshot.pybackend/apps/owasp/models/snapshot_subscription.pybackend/tests/unit/apps/api/rest/v0/snapshot_test.pybackend/tests/unit/apps/owasp/admin/snapshot_subscription_test.pybackend/tests/unit/apps/owasp/api/internal/nodes/snapshot_test.pybackend/tests/unit/apps/owasp/management/commands/process_snapshots_test.pybackend/tests/unit/apps/owasp/models/snapshot_subscription_test.pybackend/tests/unit/apps/owasp/models/snapshot_test.pydocker-compose/local/compose.yamlfrontend/__tests__/mockData/mockSnapshotData.tsfrontend/__tests__/unit/pages/SnapshotDetails.test.tsxfrontend/src/app/community/snapshots/[id]/page.tsxfrontend/src/server/queries/snapshotQueries.tsfrontend/src/types/snapshot.ts
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
8 issues found across 46 files
Confidence score: 2/5
- In
backend/apps/owasp/migrations/0073_add_snapshot_frequency_events_posts_pull_requests.py, reusingrelated_name="snapshots"for additional relations on the same target models will collide with existing reverse accessors, which can break model loading/migrations at runtime; this is the highest merge risk — give the new relations uniquerelated_namevalues before merging. backend/apps/api/rest/v0/snapshot.pyrenames publicSnapshotDetailresponse fields, so existing clients expectingnew_*_countkeys can fail or silently mis-handle data after deploy — keep legacy keys as aliases and deprecate them over a transition window.backend/apps/owasp/admin/snapshot_subscription.pyuses aOneToOneFielduser selector that renders as a full user dropdown in admin, which can cause severe slow page loads on larger datasets — switch to an autocomplete/raw-id style widget before merge.docker-compose/local/compose.yamlrenames volumes without matching updates inbackend/Makefileandfrontend/Makefile, so cleanup commands can miss old volumes and leave stale local state that causes confusing dev/test behavior — update the clean targets to handle the new (and optionally old) volume names before merging.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
HarshitVerma109
changed the base branch from
main
to
feature/community-snapshots
HarshitVerma109
added
the
gsoc2026:harshitverma109
arkid15r
requested changes
Jun 26, 2026
HarshitVerma109
dismissed stale reviews from cubic-dev-ai[bot] and coderabbitai[bot]
via
434144c
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/unit/apps/owasp/admin/snapshot_subscription_test.py (1)
12-44: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winTest the actual admin registration path.
This only proves the class config on a manually constructed
AdminSite. Ifadmin.site.register(...)or the import wiring breaks, this test still passes. Add an assertion against the real Django admin registry as well.🤖 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/admin/snapshot_subscription_test.py` around lines 12 - 44, The current test only checks a manually instantiated SnapshotSubscriptionAdmin, so it can miss broken admin registration wiring. Update test_admin_configuration in snapshot_subscription_test.py to also assert the real Django admin registry contains SnapshotSubscriptionAdmin for SnapshotSubscription via admin.site, while keeping the existing configuration checks on the SnapshotSubscriptionAdmin class.
♻️ Duplicate comments (1)
backend/apps/owasp/models/snapshot_subscription.py (1)
28-32: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
Statusas the single status vocabulary.
Statusis added here, but__str__still hardcodes"active"/"inactive", so the model now has two sources of truth for the same values. Either derive the string fromSnapshotSubscription.Statusor drop the enum until a field/property actually uses it.Also applies to: 64-67
🤖 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/models/snapshot_subscription.py` around lines 28 - 32, The snapshot subscription model currently has two status sources of truth: the new Status TextChoices enum and the hardcoded strings in __str__. Update SnapshotSubscription.__str__ to derive from SnapshotSubscription.Status instead of literal values, or remove the Status enum until a field/property actually uses it, so the model consistently uses one status vocabulary.
🤖 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/tests/unit/apps/owasp/admin/snapshot_subscription_test.py`:
- Around line 36-44: The preference field checks in the snapshot test are too
loose because they only verify membership and can miss unexpected extra fields.
Update the assertion in the snapshot test around preferences_fields to compare
against the exact expected tuple of fields, using the existing
preferences_fieldset structure so the contract is checked precisely.
---
Outside diff comments:
In `@backend/tests/unit/apps/owasp/admin/snapshot_subscription_test.py`:
- Around line 12-44: The current test only checks a manually instantiated
SnapshotSubscriptionAdmin, so it can miss broken admin registration wiring.
Update test_admin_configuration in snapshot_subscription_test.py to also assert
the real Django admin registry contains SnapshotSubscriptionAdmin for
SnapshotSubscription via admin.site, while keeping the existing configuration
checks on the SnapshotSubscriptionAdmin class.
---
Duplicate comments:
In `@backend/apps/owasp/models/snapshot_subscription.py`:
- Around line 28-32: The snapshot subscription model currently has two status
sources of truth: the new Status TextChoices enum and the hardcoded strings in
__str__. Update SnapshotSubscription.__str__ to derive from
SnapshotSubscription.Status instead of literal values, or remove the Status enum
until a field/property actually uses it, so the model consistently uses one
status vocabulary.
🪄 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: c167b994-9454-4028-8736-8a06e9266a65
📒 Files selected for processing (2)
backend/apps/owasp/models/snapshot_subscription.pybackend/tests/unit/apps/owasp/admin/snapshot_subscription_test.py
Contributor
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
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/models/snapshot_subscription.py">
<violation number="1" location="backend/apps/owasp/models/snapshot_subscription.py:28">
P2: `Status` TextChoices enum is defined but never used as a field choice or referenced anywhere in the model. This is dead code. Either wire it up by replacing `is_active` BooleanField with a `status` CharField using `Status.choices`, or remove it.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Signed-off-by: Harsh <harshit1092004@gmail.com>
HarshitVerma109
force-pushed
the
feature/snapshot-subscription-model
branch
from
434144c to
ecaed0c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
backend/tests/unit/apps/owasp/admin/snapshot_subscription_test.py (1)
12-45: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAssert registration against the real admin site.
This only verifies a manually constructed
SnapshotSubscriptionAdmin. It would still pass ifapps.owasp.admin.__init__stopped importingsnapshot_subscription, and the model disappeared from Django admin.Proposed test addition
+from django.contrib import admin + +import apps.owasp.admin # ensure registration side effects run + ... def test_admin_configuration(self): """Test admin configuration matches expected setup.""" site = AdminSite() admin = SnapshotSubscriptionAdmin(SnapshotSubscription, site) @@ assert admin.readonly_fields == ("unsubscribe_token", "created_at", "updated_at") + + def test_model_is_registered(self): + assert SnapshotSubscription in admin.site._registry + assert isinstance( + admin.site._registry[SnapshotSubscription], SnapshotSubscriptionAdmin + )🤖 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/admin/snapshot_subscription_test.py` around lines 12 - 45, The current test only instantiates SnapshotSubscriptionAdmin directly, so it does not verify Django admin registration. Update test_admin_configuration to assert the real admin site registration for SnapshotSubscription via the admin registry (or site._registry) in addition to checking SnapshotSubscriptionAdmin attributes, so the test fails if apps.owasp.admin.__init__ stops importing snapshot_subscription or the model is no longer registered.
♻️ Duplicate comments (1)
backend/tests/unit/apps/owasp/admin/snapshot_subscription_test.py (1)
36-44: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCompare the preferences fieldset to the exact tuple.
These membership checks still pass if extra fields are added or the contract drifts.
Proposed tightening
preferences_fields = preferences_fieldset[1]["fields"] - assert "include_chapters" in preferences_fields - assert "include_events" in preferences_fields - assert "include_issues" in preferences_fields - assert "include_posts" in preferences_fields - assert "include_projects" in preferences_fields - assert "include_pull_requests" in preferences_fields - assert "include_releases" in preferences_fields - assert "include_users" in preferences_fields + assert preferences_fields == ( + "include_chapters", + "include_events", + "include_issues", + "include_posts", + "include_projects", + "include_pull_requests", + "include_releases", + "include_users", + )🤖 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/admin/snapshot_subscription_test.py` around lines 36 - 44, The preferences fieldset assertion in snapshot_subscription_test is too loose because membership checks allow extra fields and won’t catch contract drift. Tighten the test by asserting the exact contents and order of preferences_fields from preferences_fieldset[1]["fields"], using the existing snapshot_subscription_test and preferences_fieldset symbols to locate the comparison. Replace the individual membership assertions with a full equality check against the expected tuple so the test fails whenever fields are added, removed, or reordered.
🤖 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/tests/unit/apps/owasp/models/snapshot_subscription_test.py`:
- Around line 33-75: The current SnapshotSubscription tests cover
content_preferences and frequency choices, but they do not verify the new
unsubscribe_token contract. Add a real test on SnapshotSubscription that
instantiates actual model objects and asserts unsubscribe_token is populated
with a UUID-valued default, not None or a fixed string. Also assert that two
separate SnapshotSubscription instances receive different unsubscribe_token
values to catch regressions in default generation.
---
Outside diff comments:
In `@backend/tests/unit/apps/owasp/admin/snapshot_subscription_test.py`:
- Around line 12-45: The current test only instantiates
SnapshotSubscriptionAdmin directly, so it does not verify Django admin
registration. Update test_admin_configuration to assert the real admin site
registration for SnapshotSubscription via the admin registry (or site._registry)
in addition to checking SnapshotSubscriptionAdmin attributes, so the test fails
if apps.owasp.admin.__init__ stops importing snapshot_subscription or the model
is no longer registered.
---
Duplicate comments:
In `@backend/tests/unit/apps/owasp/admin/snapshot_subscription_test.py`:
- Around line 36-44: The preferences fieldset assertion in
snapshot_subscription_test is too loose because membership checks allow extra
fields and won’t catch contract drift. Tighten the test by asserting the exact
contents and order of preferences_fields from preferences_fieldset[1]["fields"],
using the existing snapshot_subscription_test and preferences_fieldset symbols
to locate the comparison. Replace the individual membership assertions with a
full equality check against the expected tuple so the test fails whenever fields
are added, removed, or reordered.
🪄 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: caf0980a-2cfc-4289-b75d-a7eaa5c03174
📒 Files selected for processing (7)
backend/apps/owasp/admin/__init__.pybackend/apps/owasp/admin/snapshot_subscription.pybackend/apps/owasp/migrations/0075_snapshotsubscription.pybackend/apps/owasp/models/__init__.pybackend/apps/owasp/models/snapshot_subscription.pybackend/tests/unit/apps/owasp/admin/snapshot_subscription_test.pybackend/tests/unit/apps/owasp/models/snapshot_subscription_test.py
Signed-off-by: Harsh <harshit1092004@gmail.com>
|
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Proposed change
Resolves #4945
This PR lays the database foundation for the upcoming digest email system by introducing the
SnapshotSubscriptionmodel. It allows users to fully customize what snapshot content they receive and how often, while automatically generating a secure token for future one-click unsubscribing.Checklist
make check-testlocally: all warnings addressed, tests passed