[fix] Fixed web notifications received when disabled globally #495 - #498
[fix] Fixed web notifications received when disabled globally #495#498BHARATH0153 wants to merge 0 commit into
Conversation
|
Warning Review limit reached
Next review available in: 56 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
Suggested reviewers: Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors)
✅ Passed checks (6 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 |
|
Hi @BHARATH0153, Thank you for your interest in contributing to OpenWISP. This pull request has been flagged because external contributors must target an issue validated by maintainers before requesting review. Please link this pull request to a validated issue by adding If there is no validated issue yet, please open one first and wait for maintainer validation before continuing with this pull request. An issue is considered validated when it is open, has an appropriate label other than Please see the OpenWISP Anti AI Spam Policy. Feel free to join the OpenWISP dev chatroom to coordinate with the development team. If this is not resolved within 24 hours, this pull request will be closed automatically. Thank you for your understanding. |
Code Review SummaryStatus: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous Review Summaries (3 snapshots, latest commit 45c422e)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 45c422e)Status: No Issues Found | Recommendation: Merge This is an incremental review of commit
Files Reviewed (1 file)
Previous review (commit 72f3b3e)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit 357a3be)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Reviewed by ling-3.0-flash-free · Input: 265.5K · Output: 30.7K · Cached: 598.4K |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@openwisp_notifications/handlers.py`:
- Around line 95-108: Align the web preference resolution used by the handler’s
notification query with NotificationSetting.web_notification when WEB_ENABLED is
false. Update the model resolver or introduce a shared resolver so a missing
OrganizationNotificationSettings row applies the same fallback and yields the
same effective value as the query, preventing enabled notifications from being
reported but excluded from delivery.
🪄 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 Plus
Run ID: 08391bfe-8b85-4cf9-8dd0-36b139d3857e
📒 Files selected for processing (2)
openwisp_notifications/handlers.pyopenwisp_notifications/tests/test_notifications.py
📜 Review details
⏰ Context from checks skipped due to timeout. (15)
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.13 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=4.2.0
- GitHub Check: Python==3.11 | django~=4.2.0
- GitHub Check: Python==3.10 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.1.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (4)
openwisp_notifications/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Mark user-facing strings as translatable with Django i18n helpers, typically
gettext_lazyimported as_
Files:
openwisp_notifications/handlers.pyopenwisp_notifications/tests/test_notifications.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Avoid unnecessary blank lines inside function and method bodies
Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it
Files:
openwisp_notifications/handlers.pyopenwisp_notifications/tests/test_notifications.py
openwisp_notifications/{serializers,views,handlers,tasks}.py
📄 CodeRabbit inference engine (AGENTS.md)
Preserve validation and permission checks when changing serializers, views, handlers, or tasks that include notification payloads with related objects and URLs
Files:
openwisp_notifications/handlers.py
**/*.{py,html,txt,md}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_notifications/handlers.pyopenwisp_notifications/tests/test_notifications.py
🧠 Learnings (1)
📚 Learning: 2026-05-07T16:17:11.806Z
Learnt from: pandafy
Repo: openwisp/openwisp-notifications PR: 450
File: openwisp_notifications/tests/test_selenium.py:276-285
Timestamp: 2026-05-07T16:17:11.806Z
Learning: In OpenWISP projects’ test code (e.g., openwisp-notifications), Celery is configured project-wide to run tasks eagerly/synchronously during tests (e.g., `CELERY_TASK_ALWAYS_EAGER=True` or an equivalent setting). Therefore, when reviewing these tests, don’t flag “race condition” or “async Celery” concerns for code that assumes task completion—tasks are guaranteed to finish before execution proceeds to the next line of test code.
Applied to files:
openwisp_notifications/tests/test_notifications.py
🔇 Additional comments (1)
openwisp_notifications/tests/test_notifications.py (1)
2049-2054: LGTM!Also applies to: 2056-2062, 2064-2073
before fixScreen.Recording.2026-08-03.at.8.22.10.AM.movafter fixScreen.Recording.2026-08-03.at.8.23.44.AM.mov@nemesifier Yes, I verified the changes line by line and did manual testing. I reproduced the issue following #495: with OPENWISP_NOTIFICATIONS_WEB_ENABLED = False, a recipient with inherited settings (web=None, org setting NULL) still received a web notification on the base branch. With the fix, the same steps create no notification. An explicit org web=True override still works. Video demonstration attached. Added 3 regression tests; QA and full test suite pass |
|
This pull request has been automatically closed because it has been flagged as invalid (not referencing a validated issue) for more than 24 hours. |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
@BHARATH0153 please fix the authorship.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@openwisp_notifications/tests/test_notifications.py`:
- Around line 2056-2062: Add coverage for the globally enabled
missing-organization-settings branch by creating a companion test near
test_web_disabled_globally_org_inherited_missing_row with WEB_ENABLED set to
True, the inherited user web setting, and the organization notification settings
row deleted. Send the notification and assert that both the notification is
created and an email is sent using _assert_notification_created and
_assert_email_sent.
🪄 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 Plus
Run ID: 74f25b6a-276d-4eb8-8915-01d8997e1a94
📒 Files selected for processing (2)
openwisp_notifications/handlers.pyopenwisp_notifications/tests/test_notifications.py
📜 Review details
⏰ Context from checks skipped due to timeout. (9)
- GitHub Check: Python==3.10 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.11 | 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.11 | django~=5.1.0
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (4)
openwisp_notifications/**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
Mark user-facing strings as translatable with Django i18n helpers, typically
gettext_lazyimported as_
Files:
openwisp_notifications/handlers.pyopenwisp_notifications/tests/test_notifications.py
**/*.py
📄 CodeRabbit inference engine (AGENTS.md)
**/*.py: Avoid unnecessary blank lines inside function and method bodies
Write comments and docstrings only when they explain why code is shaped a certain way. Put comments before the relevant code block instead of scattering them inside it
Files:
openwisp_notifications/handlers.pyopenwisp_notifications/tests/test_notifications.py
openwisp_notifications/{serializers,views,handlers,tasks}.py
📄 CodeRabbit inference engine (AGENTS.md)
Preserve validation and permission checks when changing serializers, views, handlers, or tasks that include notification payloads with related objects and URLs
Files:
openwisp_notifications/handlers.py
**/*.{py,html,txt,md}
📄 CodeRabbit inference engine (Custom checks)
For Django pull requests, ensure all user-facing strings are marked as translatable using the Django i18n framework.
Files:
openwisp_notifications/handlers.pyopenwisp_notifications/tests/test_notifications.py
🧠 Learnings (1)
📚 Learning: 2026-05-07T16:17:11.806Z
Learnt from: pandafy
Repo: openwisp/openwisp-notifications PR: 450
File: openwisp_notifications/tests/test_selenium.py:276-285
Timestamp: 2026-05-07T16:17:11.806Z
Learning: In OpenWISP projects’ test code (e.g., openwisp-notifications), Celery is configured project-wide to run tasks eagerly/synchronously during tests (e.g., `CELERY_TASK_ALWAYS_EAGER=True` or an equivalent setting). Therefore, when reviewing these tests, don’t flag “race condition” or “async Celery” concerns for code that assumes task completion—tasks are guaranteed to finish before execution proceeds to the next line of test code.
Applied to files:
openwisp_notifications/tests/test_notifications.py
🔇 Additional comments (2)
openwisp_notifications/handlers.py (1)
95-108: Duplicate: Align the query with the model resolver.When
app_settings.WEB_ENABLEDisFalseand the organization settings row is missing, this query excludes users whosenotificationsetting__webisNone. TheNotificationSetting.web_notificationresolver still returns the notification type default when the reverse relation is absent. Model-level consumers can report web notifications as enabled whilenotify_handleromits delivery. Align the resolver with this fallback or use one shared resolver. This is the same unresolved issue from the previous review. (raw.githubusercontent.com)openwisp_notifications/tests/test_notifications.py (1)
2049-2054: LGTM!Also applies to: 2064-2072
0a784e5 to
cad3205
Compare
2 participants
Description
Fixes #495
Changes
notify_handlernow only sends web notifications to users withweb=Nonewhen the organization explicitly enables them, or when the global setting is enabled.web=Truestill overrides a disabled global setting.global disabled + organization inherited setting
global disabled + missing organization settings row
global disabled + organization explicitly enabled
Checklist
Fixes #123).