[fix] Dynamically updating notification verb #334

[pranshustuff]

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

Fixes #334.

Description of Changes

• Made verb field in notification nullable.
• Added a property verb under AbstractNotification in models that uses configured value from NOTIFICATION_TYPES and uses DB value as a fallback if that fails.

Screenshot

simplescreenrecorder-2025-10-01_15.25.40.mp4

[pranshustuff]

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]

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.

[pranshustuff]

I've made the requested changes.

[nemesifier]

Tests and QA checks are failing, the developer docs of OpenWISP RADIUS explain how to run tests and QA checks.

[pranshustuff]

Ok locally, it passed the tests, on sample app too. I added a test that mimics the bug in test_notifications.py.

[pranshustuff]

I made the requested changes.
The property is now called resolved_verb which gets verb from config and falls back to verb if config.get() fails. I changed many instances of .verb to .resolved_verb in the tests, so the tests mimic the logic too. And verb field is not being removed, it is being altered to nullable.

[pandafy]

This will break the existing integration with other OpenWISP apps.

We have a context manager notification_render_attributes , Can we set the verb using notification type in the context manage and then clear the field (set to None) in cleanup process?

openwisp-notifications/openwisp_notifications/base/models.py

Lines 45 to 81 in b356e1d

	@contextmanager
	def notification_render_attributes(obj, **attrs):
	"""
	This context manager sets temporary attributes on
	the notification object to allowing rendering of
	notification.
	It can only be used to set aliases of the existing attributes.
	By default, it will set the following aliases:
	- actor_link -> actor_url
	- action_link -> action_url
	- target_link -> target_url
	"""
	defaults = {
	"actor_link": "actor_url",
	"action_link": "action_url",
	"target_link": "target_url",
	}
	defaults.update(attrs)
	for target_attr, source_attr in defaults.items():
	setattr(obj, target_attr, getattr(obj, source_attr))
	# In Django 5.1+, GenericForeignKey fields defined in parent models can no longer
	# be overridden in child models (https://code.djangoproject.com/ticket/36295).
	# To avoid multiple database queries, we explicitly set these attributes here
	# using our cached _related_object method instead of relying on the default
	# GenericForeignKey accessor which would bypass our caching mechanism.
	setattr(obj, "actor", obj._related_object("actor"))
	setattr(obj, "action_object", obj._related_object("action_object"))
	setattr(obj, "target", obj._related_object("target"))
	yield obj
	for attr in defaults.keys():
	delattr(obj, attr)

It should behave similarly to the resolved_verb property.

db_verb = obj.verb
# we give preference to the verb stored in the database
verb = obj.verb or config.get('verb')

# In cleanup 
obj.verb = db_verb

Do you think a solution like this will solve our problem?

[pranshustuff]

I have not tested it yet. But I think it's a plausible solution.

I am a little busy at the moment. I'll get back to you sometime around next week after investigating this approach.

Thanks again for the review!

[coderabbitai]

Walkthrough

The changes implement a solution to prevent notification verbs from persisting in the database and being disconnected from configuration updates. The verb field is made nullable with null=True and blank=True, the handler is updated to stop passing verb when creating notifications, and a new computed property resolved_verb is added that fetches the verb from notification configuration with fallback to the stored verb field. All message templates and tests are updated to use resolved_verb instead of the raw verb field to ensure dynamic configuration changes reflect in rendered notifications.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title '[fix] Dynamically updating notification verb #334' accurately summarizes the main change: making the verb field nullable and adding dynamic resolution from configuration.
Description check	✅ Passed	The PR description covers the key changes: made verb field nullable, added resolved_verb property with config fallback, included test additions. All checklist items except documentation are marked complete.
Linked Issues check	✅ Passed	The PR fully addresses issue #334 by making verb nullable, removing verb from handler kwargs, adding resolved_verb property for dynamic config-based resolution with DB fallback, and including tests.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to fixing issue #334: verb field alterations, resolved_verb property, handler updates, template adjustments, and migration files are all necessary for the fix.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing touches

• 📝 Generate docstrings

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment

---

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 and usage tips.}

[coderabbitai]

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 using logger.exception for better debuggability.

logging.exception logs the exception and the traceback, while logging.error only 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.exception would automatically include the traceback without needing to pass exc_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

📥 Commits

Reviewing files that changed from the base of the PR and between ec47735 and 63c1ed0.

📒 Files selected for processing (9)

• openwisp_notifications/base/models.py
• openwisp_notifications/base/notifications.py
• openwisp_notifications/handlers.py
• openwisp_notifications/migrations/0012_alter_notification_verb.py
• openwisp_notifications/templates/openwisp_notifications/default_message.md
• openwisp_notifications/tests/test_notifications.py
• openwisp_notifications/types.py
• tests/openwisp2/sample_notifications/apps.py
• tests/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 verb field 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 verb field 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 the resolved_verb property.

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_verb instead of verb, 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 nullable verb.

_{✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.}

[coderabbitai]

⚠️ Potential issue | 🟡 Minor

Test doesn't verify resolved_verb fallback to DB value.

The subtest "Test fallback to database verb" sets the DB verb but only asserts notification.verb == "db verb". It should also verify that resolved_verb correctly returns this fallback value after the notification type is unregistered.

💚 Proposed fix to complete the fallback assertion

         with self.subTest("Test fallback to database verb"):
             unregister_notification_type("default")
             notification.__dict__["verb"] = "db verb"
             self.assertEqual(notification.verb, "db verb")
+            self.assertEqual(notification.resolved_verb, "db verb")

📝 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.

Suggested change

	with self.subTest("Test fallback to database verb"):
	unregister_notification_type("default")
	notification.__dict__["verb"] = "db verb"
	self.assertEqual(notification.verb, "db verb")
	with self.subTest("Test fallback to database verb"):
	unregister_notification_type("default")
	notification.__dict__["verb"] = "db verb"
	self.assertEqual(notification.verb, "db verb")
	self.assertEqual(notification.resolved_verb, "db verb")

🤖 Prompt for AI Agents

In `@openwisp_notifications/tests/test_notifications.py` around lines 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.