Skip to content

[fix] Dynamically updating notification verb #334 #413

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
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

[fix] Dynamically updating notification verb #334 #413

wants to merge 13 commits into from
+94 −13

Conversation

pranshustuff
Copy link

@pranshustuff pranshustuff commented Oct 1, 2025 •
edited
Loading

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

Pranshu and others added 2 commits October 1, 2025 16:23
[fix] Dynamic updating of notification verb openwisp#334
2a38150 
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
@pranshustuff
[change] Remove verb field from Notifs openwisp#334
84a46bf 
Same as above
Fixes openwisp#334.
@pranshustuff pranshustuff changed the title [fix] Dynamic updating of notification verb #334 [fix] Dynamically updating notification verb #334 Oct 1, 2025
@pranshustuff
Copy link
Author

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
nemesifier requested changes Oct 1, 2025
View reviewed changes
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@nemesifier nemesifier added the bug Something isn't working label Oct 1, 2025
[fix] Dynamic update for notif verb openwisp#334
d2cd283 
Added a test to verify correct behaviour and specified
exceptions in the try-except in the verb property in
models.py

Fixes openwisp#334
@pranshustuff
Copy link
Author

pranshustuff commented Oct 1, 2025 •
edited
Loading

I've made the requested changes.

[fix] Ran ./manage.py makemigrations openwisp#334
5b8f80b 
It should pass CI bulds now I think.

Fixes openwisp#334
nemesifier
nemesifier requested changes Oct 1, 2025
View reviewed changes
Copy link
Member

@nemesifier nemesifier left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@github-project-automation github-project-automation bot moved this from To do (general) to In progress in OpenWISP Contributor's Board Oct 1, 2025
[fix] Removed verb field from DB openwisp#334
09f7333 
Removed verb field from DB, verb now updates from
config from NOTIFICATION_TYPES.

Fixes openwisp#334
@pranshustuff
Copy link
Author

pranshustuff commented Oct 1, 2025 •
edited
Loading

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

nemesifier
nemesifier requested changes Oct 14, 2025
View reviewed changes
@pranshustuff
[fix] Dynamic Updates of Notif Verb openwisp#334
32f35b4 
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.
@pranshustuff
Copy link
Author

pranshustuff commented Oct 14, 2025 •
edited
Loading

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@nemesifier nemesifier Awaiting requested review from nemesifier

Requested changes must be addressed to merge this pull request.

Assignees

@pranshustuff pranshustuff

Labels

bug Something isn't working

Projects

OpenWISP Contributor's Board
Status: In progress
OpenWISP Priorities for next releases
Status: In progress

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[bug] Notification verb is saved in database

2 participants

@pranshustuff @nemesifier