[fix] Exclude deleted notification settings #426#445
[fix] Exclude deleted notification settings #426#445Piyushkhobragade wants to merge 4 commits intoopenwisp:masterfrom
Conversation
📝 WalkthroughWalkthroughThe PR excludes soft-deleted NotificationSetting records from API responses and update operations. It adds Sequence Diagram(s)sequenceDiagram
participant Client
participant API as NotificationSettingView
participant DB as NotificationSettingModel
Client->>API: GET /notification-settings/ (list)
API->>DB: query NotificationSetting.exclude(deleted=True, ...)
DB-->>API: return filtered settings
API-->>Client: 200 OK (filtered list)
Client->>API: GET /notification-settings/{id}/ (detail)
API->>DB: get(id).exclude(deleted=True)
DB-->>API: 404 if deleted / return object
API-->>Client: 200 OK or 404
Client->>API: PUT /notification-settings/{id}/
API->>DB: filter(id).exclude(deleted=True).update(...)
DB-->>API: 0 rows if deleted / 1 row updated
API-->>Client: 200 OK or 404
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
openwisp_notifications/api/views.py (1)
221-229:⚠️ Potential issue | 🟠 MajorMissing exclusion of soft-deleted records in bulk update.
The PR description states this view should exclude soft-deleted records, but the bulk update at lines 225-227 still operates on all
NotificationSettingrecords for the user/org, including those withdeleted=True. This could unintentionally modify soft-deleted settings that shouldn't be accessible via the API.🔧 Proposed fix to exclude deleted records
def post(self, request, user_id, organization_id): serializer = self.get_serializer(data=request.data) if serializer.is_valid(): validated_data = serializer.validated_data NotificationSetting.objects.filter( organization_id=organization_id, user_id=user_id - ).update(**validated_data) + ).exclude(deleted=True).update(**validated_data) return Response(status=status.HTTP_200_OK) return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@openwisp_notifications/api/views.py` around lines 221 - 229, The bulk update in the post method of the view is currently applying to all NotificationSetting rows including soft-deleted ones; change the queryset used in NotificationSetting.objects.filter(...) inside post (the filter that uses organization_id and user_id) to exclude soft-deleted records (e.g., add deleted=False or .exclude(deleted=True) to that filter) so the update(**validated_data) only affects non-deleted settings.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@openwisp_notifications/api/views.py`:
- Around line 221-229: The bulk update in the post method of the view is
currently applying to all NotificationSetting rows including soft-deleted ones;
change the queryset used in NotificationSetting.objects.filter(...) inside post
(the filter that uses organization_id and user_id) to exclude soft-deleted
records (e.g., add deleted=False or .exclude(deleted=True) to that filter) so
the update(**validated_data) only affects non-deleted settings.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1f1d6a8c-3b4b-49f1-913b-92e285d83e88
📒 Files selected for processing (2)
openwisp_notifications/api/views.pyopenwisp_notifications/tests/test_api.py
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=4.2.0
🔇 Additional comments (2)
openwisp_notifications/api/views.py (1)
130-134: LGTM - Correctly excludes soft-deleted settings from the API queryset.The added
Q(deleted=True)exclusion ensures soft-deletedNotificationSettingrecords are filtered out from list, detail, and update endpoints.Minor note: The
deletedfield allowsNULL(defined withnull=True). This exclusion includesNULLrecords (only excludes explicitTrue), whereashandlers.pyusesdeleted=False(excludes bothNULLandTrue). This is acceptable given the default isFalse, but worth being aware of for consistency.openwisp_notifications/tests/test_api.py (1)
822-847: Good test coverage for the soft-delete exclusion fix.The test correctly verifies that soft-deleted notification settings are excluded from the list endpoint and return 404 for detail and update operations.
Consider adding a subtest for the
UserOrgNotificationSettingViewbulk update endpoint (user_org_notification_setting) to verify that soft-deleted settings are not affected by bulk updates once the corresponding fix is applied to that view.
nemesifier
changed the title
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Looks good, will test asap, please change the prefix from [api] to [fix].
Thanks so much for the review! I initially wasn't sure if updating the PR title was enough, but I just checked the GitHub Actions logs and saw the commit linter was actively failing the build because of the old [api] prefix. I went ahead and squashed the commits and amended the message to use the [fix] prefix so the CI pipeline can pass. Let me know if you need anything else! |
The populate_notification_preferences command soft-deletes obsolete notification settings by setting deleted=True. However, these settings were not being filtered out by the REST API. This caused the frontend to throw a NotificationRenderException when attempting to render undefined notification types. This PR explicitly excludes these soft-deleted records from the BaseNotificationSettingView queryset and the UserOrgNotificationSettingView bulk updates. A test case has also been added to prevent regressions. Fixes openwisp#426
Piyushkhobragade
force-pushed
the
issues/426-fix-deleted-notification-settings
branch
from
0ca45f4 to
13b1e92
Compare
|
Hi @nemesifier, just a quick bump on this! The API fix and commit prefix are all set. I noticed the CI is currently failing globally due to an upstream django-filter dependency drift. Let me know if you need anything else from my end, or if we are just waiting on the infrastructure fix before you test locally. Thanks! |
The populate_notification_preferences command soft-deletes obsolete notification settings by setting deleted=True. However, these settings were not being filtered out by the REST API. This caused the frontend to throw a NotificationRenderException when attempting to render undefined notification types. This PR explicitly excludes these soft-deleted records from the BaseNotificationSettingView queryset and the UserOrgNotificationSettingView bulk updates. A test case has also been added to prevent regressions. Fixes openwisp#426
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 @.gitignore:
- Line 192: Remove the duplicate "venv/" entry in .gitignore by deleting the
later occurrence (the standalone "venv/" at line shown in the diff) and keep the
existing "venv/" entry under the "### venv ###" section; ensure no other
whitespace-only or duplicate lines remain so the single canonical "venv/"
pattern is preserved.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c6e0bf85-99af-468e-a42d-a2aff9488898
📒 Files selected for processing (1)
.gitignore
📜 Review details
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.1.0
2 participants
The populate_notification_preferences command soft-deletes obsolete
notification settings by setting deleted=True. However, these settings
were not being filtered out by the REST API.
This caused the frontend to throw a NotificationRenderException when
attempting to render undefined notification types. This PR explicitly
excludes these soft-deleted records from the BaseNotificationSettingView
queryset and the UserOrgNotificationSettingView bulk updates. A test case
has also been added to prevent regressions.
Fixes #426