[fix] Dynamically updating notification verb #334#413
[fix] Dynamically updating notification verb #334#413pranshustuff wants to merge 18 commits intoopenwisp:masterfrom
Conversation
Made verb field in notification nullable. Created a property verb under AbstractNotification in models that prioritizes verb from NOTIFICATION_TYPES and uses DB as a fallback. Fixes openwisp#334
Same as above Fixes openwisp#334.
pranshustuff
changed the title
|
I think the CI builds are failing because the ./manage.py makemigrations isn't being run in the workflow, the migration I added solved the issue of the NOT NULL constraint on verb when I tested locally. |
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Thanks @pranshustuff.
We need a test which replicates the bug described in #334. The test should fail without your patch and should pass with it.
Please also make sure the CI build passes.
See more comments below.
github-project-automation
bot
moved this to In progress
in OpenWISP Priorities for next releases
github-project-automation
bot
moved this to To do (general)
in OpenWISP Contributor's Board
Added a test to verify correct behaviour and specified exceptions in the try-except in the verb property in models.py Fixes openwisp#334
|
I've made the requested changes. |
It should pass CI bulds now I think. Fixes openwisp#334
nemesifier
left a comment
nemesifier
left a comment
There was a problem hiding this comment.
Tests and QA checks are failing, the developer docs of OpenWISP RADIUS explain how to run tests and QA checks.
github-project-automation
bot
moved this from To do (general)
to In progress
in OpenWISP Contributor's Board
This reverts commit 5b8f80b.
Removed verb field from DB, verb now updates from config from NOTIFICATION_TYPES. Fixes openwisp#334
|
Ok locally, it passed the tests, on sample app too. I added a test that mimics the bug in test_notifications.py. |
Same as above, should pass QA checks. Fixes openwisp#334.
openwisp_notifications/migrations/0012_remove_notification_verb.py
Outdated
Show resolved
Hide resolved
Not deleting DB field verb, so it is backwards compatible. Changed instances of .verb to .resolved_verb. Resolved_verb property allows .verb as fallback. Fixes openwisp#334.
|
I made the requested changes. |
WalkthroughNotification verb storage was made optional by changing the Notification/AbstractNotification Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Handler as Notification Handler
participant DB as Notification DB
participant Config as Notification Config
participant Renderer as Renderer
Client->>Handler: create/trigger notification (no verb)
Handler->>DB: create Notification (verb left NULL)
Handler-->>Client: ack
Client->>Renderer: render notification
Renderer->>DB: fetch Notification (reads verb)
alt verb present in DB
DB-->>Renderer: return verb (use DB verb)
else verb is NULL
Renderer->>Config: get_notification_configuration(type)
Config-->>Renderer: return config.verb
end
Renderer-->>Client: rendered notification (with resolved verb)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 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.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_notifications/tests/test_notifications.py`:
- Around line 1533-1537: The test currently unregisters the notification type
and sets the DB verb via notification.__dict__["verb"] then only asserts
notification.verb; update the subTest so it also asserts that
notification.resolved_verb equals the DB fallback ("db verb"). Locate the
subTest block around unregister_notification_type("default") and add a check for
notification.resolved_verb to ensure the resolved_verb fallback path returns the
stored DB value when the type is unregistered.
🧹 Nitpick comments (1)
openwisp_notifications/base/models.py (1)
104-113: Consider usinglogger.exceptionfor better debuggability.
logging.exceptionlogs the exception and the traceback, whilelogging.erroronly logs the exception. The former is more appropriate when logging an exception, as the traceback is often useful for debugging.Since this code is inside an exception handler, switching to
logger.exceptionwould automatically include the traceback without needing to passexc_info=True.♻️ Proposed fix
`@property` def resolved_verb(self): config = {} try: config = get_notification_configuration(self.type) except NotificationRenderException as e: - logger.error( + logger.exception( "Couldn't get notification config for type %s : %s", self.type, e ) return config.get("verb") or self.verb
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (9)
openwisp_notifications/base/models.pyopenwisp_notifications/base/notifications.pyopenwisp_notifications/handlers.pyopenwisp_notifications/migrations/0012_alter_notification_verb.pyopenwisp_notifications/templates/openwisp_notifications/default_message.mdopenwisp_notifications/tests/test_notifications.pyopenwisp_notifications/types.pytests/openwisp2/sample_notifications/apps.pytests/openwisp2/sample_notifications/migrations/0004_alter_notification_verb.py
💤 Files with no reviewable changes (1)
- openwisp_notifications/handlers.py
🧰 Additional context used
🧬 Code graph analysis (4)
openwisp_notifications/migrations/0012_alter_notification_verb.py (1)
tests/openwisp2/sample_notifications/migrations/0004_alter_notification_verb.py (1)
Migration(6-20)
openwisp_notifications/tests/test_notifications.py (3)
openwisp_notifications/base/models.py (2)
resolved_verb(105-113)message(255-257)openwisp_notifications/tests/test_helpers.py (2)
mock_notification_types(74-89)unregister_notification_type(64-66)openwisp_notifications/types.py (2)
get_notification_configuration(42-50)unregister_notification_type(93-100)
openwisp_notifications/base/models.py (2)
openwisp_notifications/types.py (1)
get_notification_configuration(42-50)openwisp_notifications/exceptions.py (1)
NotificationRenderException(1-6)
tests/openwisp2/sample_notifications/migrations/0004_alter_notification_verb.py (1)
openwisp_notifications/migrations/0012_alter_notification_verb.py (1)
Migration(6-20)
🪛 Ruff (0.14.14)
openwisp_notifications/migrations/0012_alter_notification_verb.py
[warning] 8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 12-20: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_notifications/base/models.py
[warning] 110-112: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
tests/openwisp2/sample_notifications/migrations/0004_alter_notification_verb.py
[warning] 8-10: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 12-20: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
⏰ 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). (8)
- GitHub Check: Python==3.13 | django~=5.2.0
- GitHub Check: Python==3.11 | django~=5.2.0
- GitHub Check: Python==3.12 | django~=5.0.0
- GitHub Check: Python==3.12 | django~=5.1.0
- GitHub Check: Python==3.12 | 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.10 | django~=5.1.0
🔇 Additional comments (7)
openwisp_notifications/migrations/0012_alter_notification_verb.py (1)
1-20: LGTM!The migration correctly alters the
verbfield to be nullable (null=True,blank=True), aligning with the PR objective to allow dynamic verb resolution from configuration. The static analysis warnings (RUF012) about mutable class attributes are false positives—this is standard Django migration syntax.openwisp_notifications/base/notifications.py (1)
52-52: LGTM!Making the
verbfield nullable is the correct approach for backward compatibility. Existing notifications retain their stored verb (used as fallback), while new notifications can rely entirely on configuration-based resolution via theresolved_verbproperty.openwisp_notifications/types.py (1)
12-14: LGTM!The message templates correctly use
{notification.resolved_verb}instead of the raw{notification.verb}. This ensures notifications dynamically reflect verb changes in the configuration, which is the core fix for issue#334.Also applies to: 25-27
openwisp_notifications/templates/openwisp_notifications/default_message.md (1)
1-6: LGTM!The template correctly uses
{{ notification.resolved_verb }}in both the head block and body, ensuring dynamically resolved verbs are displayed throughout the notification message.tests/openwisp2/sample_notifications/apps.py (1)
27-27: LGTM!The sample app correctly demonstrates the updated pattern using
{notification.resolved_verb}in the message template.openwisp_notifications/tests/test_notifications.py (1)
410-410: LGTM!The existing test assertions are correctly updated to use
resolved_verbinstead ofverb, ensuring tests validate the new dynamic resolution behavior.Also applies to: 473-473, 576-577, 586-586
tests/openwisp2/sample_notifications/migrations/0004_alter_notification_verb.py (1)
1-20: LGTM — migration cleanly aligns the sample app schema with nullableverb.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
Removed resolved_verb property. Gave preference to db verb, if it exists. If it is None then config verb is given prefernce. Fixes openwisp#334.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@openwisp_notifications/tests/test_notifications.py`:
- Around line 1534-1573: The assertion in test_dynamic_verb_changed incorrectly
expects notification.verb to equal "initial verb"; change it to assert
notification.verb is None because notify_handler stores passed kwargs in
notification.data (not verb) and the config verb only appears via
notification_render_attributes as a rendering fallback; update the first subTest
in test_dynamic_verb_changed (method name test_dynamic_verb_changed) to assert
None for notification.verb and keep the existing subTests that verify rendering
fallback (using notification_render_attributes) and DB precedence unchanged.
🧹 Nitpick comments (3)
openwisp_notifications/base/models.py (1)
65-75: Verb resolution logic looks correct.The precedence is clear: DB verb (non-null) wins over config verb, with config verb as fallback when DB verb is
None. The exception handling for unregistered types gracefully degrades.One minor consideration from the Ruff hint (TRY400): within the
exceptblock,logger.exceptionwould automatically include the traceback, which could aid debugging.logger.erroronly logs the formatted message.🔧 Optional: use logger.exception for richer error context
except NotificationRenderException as e: - logger.error( + logger.exception( "Couldn't get notification config for type %s : %s", obj.type, e )openwisp_notifications/tests/test_notifications.py (2)
1555-1563: Nestednotification_render_attributescalls: fragile but functional.Accessing
n.messageon line 1563 triggers the@cached_property, which internally opens a secondnotification_render_attributescontext manager (seemodels.pyline 259). The inner CM's cleanup willdelattrthe link attributes set by the outer CM. This works here because nothing in the test uses those link attrs after accessingmessage, but it's worth being aware of the nesting.Consider testing
n.get_message()directly instead ofn.messageto avoid the nested CM and cached-property side effects, or at minimum add a comment noting the nesting.
1565-1573: Cleanup restores config — good, but the third subtest could also assert the rendered message.The "db verb takes precedence" subtest only asserts
n.verb == "db verb"but doesn't verify the rendered message contains"db verb". Adding a message assertion would strengthen the test and confirm end-to-end behavior.💚 Suggested enhancement
with self.subTest("Test db verb takes precedence over config verb"): notification.verb = "db verb" notification.save() notification.refresh_from_db() + del notification.message with notification_render_attributes(notification) as n: self.assertEqual(n.verb, "db verb") + self.assertIn("db verb", n.message)
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
openwisp_notifications/base/models.pyopenwisp_notifications/base/notifications.pyopenwisp_notifications/migrations/0013_merge_20260131_1927.pyopenwisp_notifications/tests/test_notifications.pytests/openwisp2/sample_notifications/migrations/0005_merge_20260131_1929.py
🚧 Files skipped from review as they are similar to previous changes (1)
- openwisp_notifications/base/notifications.py
🧰 Additional context used
🧬 Code graph analysis (4)
tests/openwisp2/sample_notifications/migrations/0005_merge_20260131_1929.py (1)
openwisp_notifications/migrations/0013_merge_20260131_1927.py (1)
Migration(6-13)
openwisp_notifications/migrations/0013_merge_20260131_1927.py (1)
tests/openwisp2/sample_notifications/migrations/0005_merge_20260131_1929.py (1)
Migration(6-13)
openwisp_notifications/base/models.py (2)
openwisp_notifications/types.py (1)
get_notification_configuration(42-50)openwisp_notifications/exceptions.py (1)
NotificationRenderException(1-6)
openwisp_notifications/tests/test_notifications.py (4)
openwisp_notifications/base/models.py (2)
notification_render_attributes(46-94)message(258-260)openwisp_notifications/tests/test_helpers.py (1)
mock_notification_types(74-89)openwisp_notifications/types.py (1)
get_notification_configuration(42-50)openwisp_notifications/tests/test_selenium.py (1)
_create_notification(44-45)
🪛 Ruff (0.14.14)
tests/openwisp2/sample_notifications/migrations/0005_merge_20260131_1929.py
[warning] 8-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 13-13: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_notifications/migrations/0013_merge_20260131_1927.py
[warning] 8-11: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
[warning] 13-13: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
openwisp_notifications/base/models.py
[warning] 72-74: Use logging.exception instead of logging.error
Replace with exception
(TRY400)
🔇 Additional comments (4)
openwisp_notifications/migrations/0013_merge_20260131_1927.py (1)
1-13: Standard merge migration — looks correct.Dependencies properly reference both divergent
0012migrations. The Ruff RUF012 warnings are false positives for Django migration files (the framework expects plain class attributes).tests/openwisp2/sample_notifications/migrations/0005_merge_20260131_1929.py (1)
1-13: Standard merge migration for the sample app — looks correct.Mirrors the main app's merge migration pattern with correct dependencies.
openwisp_notifications/base/models.py (1)
89-94: Good improvement: safer cleanup withhasattrguard.The
hasattrcheck beforedelattrpreventsAttributeErrorif an attribute wasn't set (e.g., if the object didn't have the corresponding source attribute). The verb restore on line 90 correctly puts back the original DB value.openwisp_notifications/tests/test_notifications.py (1)
27-27: LGTM — import added for use in the new test.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
@pandafy I used the context manager to render the notification verb in notification message as well as email subject. Updated tests and the video demonstration too. |
3 participants
Checklist
Reference to Existing Issue
Fixes #334.
Description of Changes
Screenshot
Notif_Issue.mp4
Update: The video has been updated. The first three notifications are newly created to show verbs not stored in DB, the ones below are stored in DB. If there are already verbs in DB, it will not be affected. New notifications' verbs can be changed through config.