Skip to content

[bug] Notification verb is saved in database #334

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
pandafy opened this issue Feb 12, 2025 · 1 comment · May be fixed by #335
Open

[bug] Notification verb is saved in database #334

pandafy opened this issue Feb 12, 2025 · 1 comment · May be fixed by #335
Labels
bug Something isn't working good first issue Good for newcomers

Comments

@pandafy
Copy link
Member

pandafy commented Feb 12, 2025

Describe the bug
The notification verb is saved in the database. Thus, changing the verb in the NOTIFICATION_TYPE does not change the message of existing notifications.

Steps To Reproduce
Steps to reproduce the behavior:

  1. Create a default notification using the create_notification management command.
  2. Go to Django admin and verify the notification message.
Default notification with default verb and level info by default
  1. Change the verb for the default notification type

    openwisp-notifications/openwisp_notifications/types.py

    Lines 7 to 19 in 55a3443

    'default': {
    'level': 'info',
    'verb': 'default verb',
    'verbose_name': 'Default Type',
    'email_subject': '[{site.name}] Default Notification Subject',
    'message': (
    'Default notification with {notification.verb} and level {notification.level}'
    ' by [{notification.target}]({notification.target_link})'
    ),
    'message_template': 'openwisp_notifications/default_message.md',
    'email_notification': True,
    'web_notification': True,
    },
  2. Go to Django admin again and verify the notification message

Expected behavior
The notification message should show the updated notification verb

Possible solution

Update the notify handler to not save the verb in the database.

openwisp-notifications/openwisp_notifications/handlers.py

Lines 139 to 148 in 55a3443

notification = Notification(
recipient=recipient,
actor=actor,
verb=str(verb),
public=public,
description=description,
timestamp=timestamp,
level=level,
type=notification_type,
)

Update the AbstractNotification model to use the verb from the configuration if database value is set to None
@pandafy pandafy added the bug Something isn't working label Feb 12, 2025
@pandafy pandafy moved this from To do (general) to To do (Python & Django) in OpenWISP Contributor's Board Feb 12, 2025
@pandafy pandafy added the good first issue Good for newcomers label Feb 12, 2025
@nemesifier nemesifier moved this from Backlog to To do in OpenWISP Priorities for next releases Feb 12, 2025
@shwetd19
Copy link

Hey @pandafy, I think the solution approach is to make the verb field nullable, by updating the handler to make verb optional, and add a get_verb() method that prioritizes NOTIFICATION_TYPES config while falling back to the database value, with checking.. notifications properly reflect updated verbs while maintaining backward compatibility can solve this bug

can I give it a try ?

@AviGawande AviGawande linked a pull request Feb 15, 2025 that will close this issue
Open
4 tasks
stktyagi added a commit to stktyagi/openwisp-notifications that referenced this issue May 17, 2025
@stktyagi
[fix] Fix notification verb not updating due to verb being saved in DB
1d8ce2b 
The notification verb was saved in the database, causing existing notifications to
not reflect updates to the verb. Now the verb is not saved,
so messages show the current verb dynamically.

Fixes openwisp#334
stktyagi added a commit to stktyagi/openwisp-notifications that referenced this issue May 18, 2025
@stktyagi
[notifications] Make notification verb dynamic instead of stored open…
55968c5 
…wisp#334

The notification verb was previously saved in the database, preventing existing notifications from reflecting verb updates. Now the verb is calculated dynamically via a property, ensuring messages always show the current verb.

Fixes openwisp#334
@stktyagi stktyagi mentioned this issue May 18, 2025
Closed
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working good first issue Good for newcomers
Projects
OpenWISP Contributor's Board
Status: To do (Python & Django)
OpenWISP Priorities for next releases
Status: To do
Milestone
No milestone
2 participants
@pandafy @shwetd19