[feature] Added target_url_suffix param to notify signal #492

[nemesifier]

Checklist

• I have read the OpenWISP Contributing Guidelines.
• I have manually tested the changes proposed in this pull request.
• I have written new test cases for new code and/or updated existing tests for changes to existing code.
• I have updated the documentation.

Reference to Existing Issue

Closes #492

Description of Changes

This PR adds a way to append a querystring or fragment to a notification target URL, especially for generic_message notifications, without allowing callers to override the full URL. It does so by adding support for a new argument target_url_suffix argument to notify.send.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds an optional target_url_suffix argument to notifications. The handler validates that it is a string beginning with ?, &, or #. AbstractNotification.target_url appends the suffix to the related target URL. Tests cover query-string and fragment suffixes, as well as invalid inputs. Developer documentation describes the parameter and usage.

Estimated code review effort: 2 (Simple) | ~10 minutes

Sequence Diagram(s)

sequenceDiagram
  participant notify.send
  participant notify_handler
  participant AbstractNotification.target_url
  participant target object
  notify.send->>notify_handler: pass target_url_suffix
  notify_handler->>notify_handler: validate suffix
  notify_handler->>AbstractNotification.target_url: store notification data
  AbstractNotification.target_url->>target object: resolve base URL
  target object-->>AbstractNotification.target_url: return base URL
  AbstractNotification.target_url-->>notify.send: return suffixed URL

Loading

Suggested reviewers: pandafy, dhanus3133

---

Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

• Ignore

❌ Failed checks (2 errors, 1 warning)

Check name	Status	Explanation	Resolution
Bug Fixes	❌ Error	The added tests cover happy-path suffixing and validation, but not the falsy/empty target_url edge case; target_url still concatenates the suffix unconditionally.	Guard AbstractNotification.target_url before appending suffix, then add a regression test for falsy target URLs; add a Selenium test if the rendered link is in scope.
General Rules	❌ Error	FAIL: the new validation error isn't translatable, and tests don't cover the & suffix/empty-target edge cases.	Wrap the ValueError message in gettext, and add tests for target_url_suffix='&...' plus cases where target URL is missing/empty so suffixing is safe.
Changes	⚠️ Warning	Docs and tests were updated, but target_url_suffix is appended even when target_url falls back to '#', producing malformed URLs and an untested regression.	Guard against empty/fallback target URLs before appending the suffix, and add a test for the '#' fallback case.

✅ Passed checks (5 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly matches the feature and references the linked issue.
Description check	✅ Passed	The description includes the checklist, issue reference, and change summary required by the template.
Linked Issues check	✅ Passed	The code, tests, and docs implement the target_url_suffix feature and its validation as requested in #492.
Out of Scope Changes check	✅ Passed	The changes stay focused on the suffix feature with matching tests and documentation, with no clear out-of-scope edits.
Features	✅ Passed	Issue #492 exists and is triaged (label + project board); docs mention target_url_suffix, and new tests cover valid/invalid suffixes.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feature/target_url_suffix

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[kilo-code-bot]

Code Review Summary

Status: No Issues Found | Recommendation: Merge

Incremental Review (since bc7d0ed)

The changes between the previous review commit and HEAD resolve all prior findings:

• base/models.py:243 now guards against a falsy or # fallback target URL before appending the suffix, preventing malformed URLs (e.g. None?status=pending).
• handlers.py:164 wraps the validation ValueError in gettext_lazy (_), satisfying i18n conventions.
• tests/test_notifications.py adds coverage for querystring (?), fragment (#), the # fallback case, and invalid suffix rejection (regression test included, fails without the fix).

The implementation is correct and focused; the target_url_suffix validation and URL construction behave as intended.

Files Reviewed (5 files)

• docs/developer/notification-types.rst
• docs/developer/sending-notifications.rst
• openwisp_notifications/base/models.py
• openwisp_notifications/handlers.py
• openwisp_notifications/tests/test_notifications.py

Previous Review Summary (commit bc7d0ed)

Current summary above is authoritative. Previous snapshots are kept for context only.

Previous review (commit bc7d0ed)

Status: No Issues Found | Recommendation: Merge

Files Reviewed (5 files)

• docs/developer/notification-types.rst
• docs/developer/sending-notifications.rst
• openwisp_notifications/base/models.py
• openwisp_notifications/handlers.py
• openwisp_notifications/tests/test_notifications.py

---

_{Reviewed by hy3:free · Input: 43.6K · Output: 2.2K · Cached: 121.2K}

[coderabbitai]

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/base/models.py`:
- Around line 241-243: Update the URL construction in the related-object URL
method around _get_related_object_url so the suffix is appended only when both
url and target_url_suffix are present and non-empty; otherwise return the
original url unchanged, preserving None or empty-string values without
formatting them into invalid URLs.

🪄 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: 3e1469c9-2c1f-4860-b865-6e04a1b7ebf6

📥 Commits

Reviewing files that changed from the base of the PR and between 382f613 and bc7d0ed.

📒 Files selected for processing (5)

• docs/developer/notification-types.rst
• docs/developer/sending-notifications.rst
• openwisp_notifications/base/models.py
• openwisp_notifications/handlers.py
• openwisp_notifications/tests/test_notifications.py

📜 Review details

⏰ Context from checks skipped due to timeout. (17)

• GitHub Check: Python==3.13 | django~=5.1.0
• GitHub Check: Python==3.13 | django~=5.2.0
• GitHub Check: Python==3.12 | django~=5.2.0
• GitHub Check: Python==3.10 | django~=4.2.0
• GitHub Check: Python==3.12 | django~=5.0.0
• GitHub Check: Python==3.12 | django~=4.2.0
• GitHub Check: Python==3.10 | django~=5.2.0
• GitHub Check: Python==3.11 | django~=5.2.0
• GitHub Check: Python==3.10 | django~=5.0.0
• GitHub Check: Python==3.10 | django~=5.1.0
• GitHub Check: Python==3.11 | django~=5.0.0
• GitHub Check: Python==3.11 | django~=5.1.0
• GitHub Check: Python==3.12 | django~=5.1.0
• GitHub Check: Python==3.11 | django~=4.2.0
• GitHub Check: Analyze (python)
• GitHub Check: Analyze (javascript-typescript)
• GitHub Check: Kilo Code Review

🧰 Additional context used

📓 Path-based instructions (5)

**/models.py

📄 CodeRabbit inference engine (AGENTS.md)

Preserve swappable model support and integration with openwisp-users organizations and memberships in Django models

Files:

• openwisp_notifications/base/models.py

openwisp_notifications/**/*.py

📄 CodeRabbit inference engine (AGENTS.md)

Mark user-facing strings as translatable with Django i18n helpers, typically gettext_lazy imported as _

Files:

• openwisp_notifications/base/models.py
• openwisp_notifications/handlers.py
• openwisp_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/base/models.py
• openwisp_notifications/handlers.py
• openwisp_notifications/tests/test_notifications.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/base/models.py
• openwisp_notifications/handlers.py
• openwisp_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

🧠 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 (4)

openwisp_notifications/handlers.py (1)

158-165: LGTM!

docs/developer/notification-types.rst (1)

60-77: LGTM!

docs/developer/sending-notifications.rst (1)

67-125: LGTM!

openwisp_notifications/tests/test_notifications.py (1)

167-208: LGTM!

[nemesifier]

@coderabbitai resolve

[coderabbitai]

✅ Action performed

Comments resolved and changes approved.