[change] Honor notification data["url"] for the web target link#490
[change] Honor notification data["url"] for the web target link#490Eeshu-Yadav wants to merge 1 commit into
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthrough
Estimated code review effort: 2 (Simple) | ~10 minutes Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 2 warnings)
✅ Passed checks (4 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 |
Code Review SummaryStatus: No Issues Found | Recommendation: Merge OverviewThe incremental change since the last review hardens the web No new critical bugs or security vulnerabilities were introduced by the incremental change. Files Reviewed (2 files)
Previous Review Summaries (3 snapshots, latest commit 3edff65)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 3edff65)Status: No Issues Found | Recommendation: Merge The incremental change since the last review is a trivial, correct refactor: the Files Reviewed (1 file)
Previous review (commit 9c542f4)Status: No Issues Found | Recommendation: Merge The incremental changes since the last review cleanly address the earlier feedback. The serializer now overrides The Files Reviewed (2 files)
Previous review (commit e7ef750)Status: No Issues Found | Recommendation: Merge This is a clean, focused change. The serializer now honors Files Reviewed (2 files)
Reviewed by claude-opus-4.8 · Input: 26 · Output: 3.9K · Cached: 328.6K |
There was a problem hiding this comment.
Pull request overview
This PR updates the notifications API serialization so the web notification link can honor a per-notification data["url"] override (matching existing email behavior), while keeping the model’s target_url property unchanged to preserve {target_link} template behavior.
Changes:
- Add a serializer-level
target_urlfield that returnsdata["url"]when present, otherwise falls back to the model’starget_url. - Add a regression test intended to verify the override vs fallback behavior.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| openwisp_notifications/api/serializers.py | Exposes target_url via SerializerMethodField so API clients can use data["url"] overrides for web links. |
| openwisp_notifications/tests/test_notifications.py | Adds a test for the data["url"] override and fallback behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/api/serializers.py`:
- Line 45: Remove "target_url" from the extra_fields declaration, while keeping
the SerializerMethodField declaration unchanged so
super().get_field_names(declared_fields, info) supplies it automatically.
- Around line 47-50: Update get_target_url to validate obj.data["url"] before
returning it: retain the override only when its URL scheme is http or https, and
otherwise fall back to obj.target_url. Preserve the existing fallback when data
or url is absent.
🪄 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: 2727e63c-3fcd-4db0-99fa-e652053e81dd
📒 Files selected for processing (2)
openwisp_notifications/api/serializers.pyopenwisp_notifications/tests/test_notifications.py
📜 Review details
⏰ Context from checks skipped due to timeout. (18)
- GitHub Check: CodeRabbit
- GitHub Check: copilot-pull-request-reviewer
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.10 | django~=4.2.0
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.2.0
- GitHub Check: Python==3.13 | django~=5.1.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.10 | django~=5.2.0
- 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.10 | django~=5.0.0
- GitHub Check: Python==3.11 | django~=5.0.0
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (3)
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/tests/test_notifications.pyopenwisp_notifications/api/serializers.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/tests/test_notifications.pyopenwisp_notifications/api/serializers.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/tests/test_notifications.pyopenwisp_notifications/api/serializers.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)
774-794: LGTM!
| class Meta: | ||
| model = Notification | ||
| exclude = ["description", "deleted", "public"] | ||
| extra_fields = ["message", "email_subject", "target_url"] |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Remove redundant target_url from extra_fields.
Now that target_url is declared as a SerializerMethodField, super().get_field_names(declared_fields, info) already includes it. Keeping it in extra_fields causes a duplicate entry in the field names list — harmless but unnecessary.
♻️ Proposed cleanup
- extra_fields = ["message", "email_subject", "target_url"]
+ extra_fields = ["message", "email_subject"]📝 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.
| extra_fields = ["message", "email_subject", "target_url"] | |
| extra_fields = ["message", "email_subject"] |
🤖 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 `@openwisp_notifications/api/serializers.py` at line 45, Remove "target_url"
from the extra_fields declaration, while keeping the SerializerMethodField
declaration unchanged so super().get_field_names(declared_fields, info) supplies
it automatically.
e7ef750 to
9c542f4
Compare
9c542f4 to
3edff65
Compare
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/api/serializers.py`:
- Around line 50-54: Update to_representation to validate instance.data["url"]
before assigning it to target_url: only accept URLs with http or https schemes,
and preserve the serialized base target_url for all other schemes, including
javascript and malformed values.
🪄 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: 55868b75-5863-49c2-9c1b-4516ecad7959
📒 Files selected for processing (2)
openwisp_notifications/api/serializers.pyopenwisp_notifications/tests/test_notifications.py
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
- GitHub Check: CodeRabbit
- GitHub Check: Kilo Code Review
🧰 Additional context used
📓 Path-based instructions (3)
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/tests/test_notifications.pyopenwisp_notifications/api/serializers.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/tests/test_notifications.pyopenwisp_notifications/api/serializers.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/tests/test_notifications.pyopenwisp_notifications/api/serializers.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/tests/test_notifications.py (1)
774-793: LGTM!openwisp_notifications/api/serializers.py (1)
50-54: 🎯 Functional CorrectnessNotificationListSerializer already inherits the override
NotificationListSerializer(NotificationSerializer)already picks upto_representation, so list responses also includedata["url"]intarget_url.> Likely an incorrect or invalid review comment.
| def to_representation(self, instance): | ||
| data = super().to_representation(instance) | ||
| if instance.data and instance.data.get("url"): | ||
| data["target_url"] = instance.data["url"] | ||
| return data |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Reject non-http(s) url overrides in to_representation.
instance.data["url"] is returned as target_url without validating the URL scheme. A crafted javascript: URL in notification data would be passed to the frontend and could execute on click. Only allow http/https schemes and fall back to the base target_url otherwise.
🛡️ Proposed fix: validate URL scheme before overriding
def to_representation(self, instance):
data = super().to_representation(instance)
- if instance.data and instance.data.get("url"):
- data["target_url"] = instance.data["url"]
+ if instance.data:
+ url = instance.data.get("url")
+ if url and url.startswith(("http://", "https://")):
+ data["target_url"] = url
return data🤖 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 `@openwisp_notifications/api/serializers.py` around lines 50 - 54, Update
to_representation to validate instance.data["url"] before assigning it to
target_url: only accept URLs with http or https schemes, and preserve the
serialized base target_url for all other schemes, including javascript and
malformed values.
…tegration The persistent mass-upgrade reminder relies on openwisp-notifications honoring the per-notification "url" for the web notification link (so the "Open" button lands on the batch filtered to its pending devices). That support lands in openwisp/openwisp-notifications#490; pin the dependency to that commit so the integration is reproducible until it is released.
…tegration The persistent mass-upgrade reminder relies on openwisp-notifications honoring the per-notification "url" for the web notification link, so the "Open" button lands on the batch filtered to its pending devices. That support lands in openwisp/openwisp-notifications#490; pin the dependency to that commit so the integration is reproducible until it is released.
…tegration The persistent mass-upgrade reminder relies on openwisp-notifications honoring the per-notification "url" for the web notification link, so the "Open" button lands on the batch filtered to its pending devices. That support lands in openwisp/openwisp-notifications#490; pin the dependency to that commit so the integration is reproducible until it is released.
The web notification widget links to the serialized target_url. Return
data["url"] when it is set so a per-notification link (for example a
generic_message sent with url=...) points the notification at a specific
page, consistent with the email link which already uses data["url"]. The
model target_url is left unchanged so message templates that render
{target_link} keep pointing at the target object.
The override is restricted to relative paths and http(s) URLs; the widget
assigns target_url to window.location, so a javascript: scheme would
otherwise execute on click.
3edff65 to
d6562a2
Compare
nemesifier
moved this to In progress
in [GSoC26] Persistent & Scheduled Firmware Upgrades
Eeshu-Yadav
moved this from In progress
to In review
in [GSoC26] Persistent & Scheduled Firmware Upgrades
Checklist
Reference to Existing Issue
Related to the persistent mass upgrades work in
openwisp/openwisp-firmware-upgrader#436, which needs a
generic_messagenotification to link to a filtered admin page without registering a dedicated notification type.Description of Changes
The web notification widget links to the serialized
target_url, which forgeneric_messageis always the target object's admin URL. A per-notificationurlpassed tonotify.send()was only used for the email link, so a notification could not point its web link at a specific page without registering a dedicated notification type with atarget_link.This returns
data["url"]from the serializedtarget_urlwhen it is set, falling back to the target object's URL otherwise, which is consistent with the email link that already usesdata["url"]. The modeltarget_urlproperty is left unchanged, so message templates that render{target_link}continue to point at the target object.