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

[feature] Improve UX of the Notifications Module #344

Merged
merged 3 commits into from
Apr 7, 2025
Merged

Conversation

pandafy
Copy link
Member

@pandafy pandafy commented Mar 24, 2025

Closes #132
Closes #306
Closes #110
Closes #148
Closes #255

@pandafy pandafy moved this from To do (general) to Needs review in OpenWISP Contributor's Board Mar 24, 2025
@pandafy pandafy requested a review from Dhanus3133 March 24, 2025 12:35
@pandafy
Copy link
Member Author

pandafy commented Mar 24, 2025

@Dhanus3133 can you ensure that the coverage does not decrease for these files:

  • api/serializers.py
  • base/models.py
  • tasks.py

@Dhanus3133
Copy link
Member

@pandafy For the coverage missing on base/models.py

def email_notification(self):
if self.email is not None:
return self.email

Seems to be due to the defaults being set here

def create_notification_settings(user, organizations, notification_types):
global_setting, _ = NotificationSetting.objects.get_or_create(
user=user, organization=None, type=None, defaults={'email': True, 'web': True}
)
for type in notification_types:
notification_config = types.get_notification_configuration(type)
for org in organizations:
NotificationSetting.objects.update_or_create(
defaults={
'deleted': False,
'email': global_setting.email
and notification_config.get('email_notification'),
'web': global_setting.web
and notification_config.get('web_notification'),
},
user=user,
type=type,
organization=org,
)

But if that's the case

def web_notification(self):
if self.web is not None:
return self.web
this should also cause a coverage issue but it doesn't, not sure why.

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.

Please resolve conflicts with master and squash commits so we have 1 commit for each feature/change.

I hope we can merge after this.

@github-project-automation github-project-automation bot moved this from Needs review to In progress in OpenWISP Contributor's Board Apr 4, 2025
Closes #132
Closes #306

---------

Co-authored-by: Federico Capoano <[email protected]>
Co-authored-by: Gagan Deep <[email protected]>
@pandafy pandafy force-pushed the gsoc24-rebased branch 2 times, most recently from b20b0be to 54c6b8d Compare April 6, 2025 02:38
Dhanus3133 and others added 2 commits April 6, 2025 08:16
Closes #110
Closes #148
Closes #255

---------

Co-authored-by: Federico Capoano <[email protected]>
Co-authored-by: Gagan Deep <[email protected]>
Implements and closes #256

---------

Co-authored-by: Federico Capoano <[email protected]>
Co-authored-by: Gagan Deep <[email protected]>
@nemesifier nemesifier added the enhancement New feature or request label Apr 7, 2025
@nemesifier nemesifier merged commit f45f574 into master Apr 7, 2025
20 of 35 checks passed
@nemesifier nemesifier deleted the gsoc24-rebased branch April 7, 2025 19:24
@github-project-automation github-project-automation bot moved this from In progress to Done in OpenWISP Contributor's Board Apr 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment