Skip to content
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.

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

[fix] Store notification verb in configuration #335

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

AviGawande
Copy link

@AviGawande AviGawande commented Feb 15, 2025

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 #334.

Please open a new issue if there isn't an existing issue yet.

Description of Changes

Please describe these changes.

  • Modified notify_handler to not save verb in database
  • Added notification_verb property to get verb from configuration
  • Updated templates to use notification_verb
  • Updated tests to verify behavior

Screenshot

Please include any relevant screenshots.

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.

CI build is failing.

@@ -65,6 +65,7 @@ class AbstractNotification(UUIDModel, BaseNotification):
_actor = BaseNotification.actor
_action_object = BaseNotification.action_object
_target = BaseNotification.target
verb = models.CharField(max_length=128, null=True)
Copy link
Member

Choose a reason for hiding this comment

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

Do we really need to do this?
I don't think this was requested, what do you think @pandafy?

Copy link
Member

Choose a reason for hiding this comment

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

This is not required as the verb field is inherited from the BaseNotification model.

Copy link
Author

Choose a reason for hiding this comment

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

Should i revert this changes @nemesifier.

Copy link
Member

Choose a reason for hiding this comment

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

Yes, revert this.

Copy link
Author

Choose a reason for hiding this comment

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

@pandafy done

@@ -65,6 +65,7 @@ class AbstractNotification(UUIDModel, BaseNotification):
_actor = BaseNotification.actor
_action_object = BaseNotification.action_object
_target = BaseNotification.target
verb = models.CharField(max_length=128, null=True)
Copy link
Member

Choose a reason for hiding this comment

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

This is not required as the verb field is inherited from the BaseNotification model.

@@ -65,6 +65,7 @@ class AbstractNotification(UUIDModel, BaseNotification):
_actor = BaseNotification.actor
_action_object = BaseNotification.action_object
_target = BaseNotification.target
verb = models.CharField(max_length=128, null=True)
Copy link
Member

Choose a reason for hiding this comment

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

Yes, revert this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[bug] Notification verb is saved in database
3 participants