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

[feat] Notification Preferences Page #290

Open
wants to merge 76 commits into
base: gsoc24-rebased
Choose a base branch
from

Conversation

Dhanus3133
Copy link
Member

@Dhanus3133 Dhanus3133 commented Jul 15, 2024

Should fix #110, #148 and #255 issues

The Preference Page be like
image

@Dhanus3133 Dhanus3133 changed the base branch from master to gsoc24 July 15, 2024 16:55
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.

Notes from our last discussion in the weekly meeting:

diff --git a/openwisp_notifications/api/urls.py b/openwisp_notifications/api/urls.py
index 597d2a7..82c236f 100644
--- a/openwisp_notifications/api/urls.py
+++ b/openwisp_notifications/api/urls.py
@@ -9,32 +9,47 @@ def get_api_urls(api_views=None):
     if not api_views:
         api_views = views
     return [
-        path('', views.notifications_list, name='notifications_list'),
-        path('read/', views.notifications_read_all, name='notifications_read_all'),
-        path('<uuid:pk>/', views.notification_detail, name='notification_detail'),
+        path('notification/', views.notifications_list, name='notifications_list'),
+        path('notification/read/', views.notifications_read_all, name='notifications_read_all'),
+        path('notification/<uuid:pk>/', views.notification_detail, name='notification_detail'),
         path(
-            '<uuid:pk>/redirect/',
+            'notification/<uuid:pk>/redirect/',
             views.notification_read_redirect,
             name='notification_read_redirect',
         ),
+        # path(
+        #     'notification/user-setting/',
+        #     views.notification_setting_list,
+        #     name='notification_setting_list',
+        # ),
+        # path(
+        #     'notification/user-setting/<uuid:pk>/',
+        #     views.notification_setting,
+        #     name='notification_setting',
+        # ),
         path(
-            'user-setting/',
-            views.notification_setting_list,
-            name='notification_setting_list',
-        ),
-        path(
-            'user-setting/<uuid:pk>/',
-            views.notification_setting,
-            name='notification_setting',
-        ),
-        path(
-            'ignore/',
+            'notification/ignore/',
             views.ignore_object_notification_list,
             name='ignore_object_notification_list',
         ),
         path(
-            'ignore/<str:app_label>/<str:model_name>/<uuid:object_id>/',
+            'notification/ignore/<str:app_label>/<str:model_name>/<uuid:object_id>/',
             views.ignore_object_notification,
             name='ignore_object_notification',
         ),
+        path(
+            'user/<uuid:user_id>/setting/',
+            # views.admin_user_organization_notification_setting,
+            # name='admin_user_organization_notification_setting',
+        ),
+        path(
+            'user/<uuid:user_id>/setting/<uuid:setting_pk>/',
+            # views.admin_user_organization_notification_setting,
+            # name='admin_user_organization_notification_setting',
+        ),
+        path(
+            'user/<uuid:user_id>/organization/<uuid:organization_id>/setting/',
+            views.admin_user_organization_notification_setting,
+            name='admin_user_organization_notification_setting',
+        )
     ]
diff --git a/openwisp_notifications/urls.py b/openwisp_notifications/urls.py
index efc6d2b..3c33306 100644
--- a/openwisp_notifications/urls.py
+++ b/openwisp_notifications/urls.py
@@ -10,7 +10,7 @@ def get_urls(api_views=None, social_views=None):
         api_views(optional): views for Notifications API
     """
     urls = [
-        path('api/v1/notifications/notification/', include(get_api_urls(api_views)))
+        path('api/v1/notifications/', include(get_api_urls(api_views)))
     ]
     return urls

@Dhanus3133 Dhanus3133 marked this pull request as ready for review August 4, 2024 15:51
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.

Good progress @Dhanus3133 👍

Please keep in mind that you need to focus on the point of view of the user (user-centered), making sure the UI provides up to date data and guides the user in a way that allows him/her to understand what's going on.

  • Web notification on the left, email on the right (as in the current notification settings)
  • Keep in mind web notifications are required for email notifications, therefore the web/email checkbox inputs have work as in the current notification settings (if you disable web, email will be also disabled and when both are disabled, if you enable email, web will be also be enabled)
  • Type: show verbose_message in the UI, in the API we call this type_label colors
  • Please reuse CSS classes for colors, titles, form row, etc
  • Open first org in the UI automatically
  • Triangle icon: we should have similar icons to reuse
  • Accordion title of the org: reuse the style of other pages, eg:

Screenshot from 2024-08-06 10-18-14

  • Table: reuse styles from any change list page (table headings, table body, table rows, checkbox, etc)
  • Make sure email and web are tags
  • Preferences saved message: please change as discussed (fixed place, different logic than notification toast)
  • Look for drop-in replacements of default checkbox inputs to look more similar to what the UX designers shared (but no execute yet, just look for possible open source options to use)

Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

Good work @Dhanus3133! In this review, I primarily focused on the UX/UI. I hope these points would help you progress with this PR

  • Add link to the notification preference page in the notification widget
  • Add link to the notification preference page in the UserAdmin
  • When the API request fails, the user operation should rollback. E.g. if the user disabled the global web notifications and the API request fails, then the UI should flag the checkbox as enabled again.
  • Add tooltip to the global and organization settings

Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

In our weekly call, we discussed the following changes to the docs

  • explain the new interaction between default notification settings in the type definition and disabled global notifications in the preference of the user
  • make sure API docs page is up to date

We also evaluated the feasibility for adding "Notification Settings" button on the UserAdmin. I have attached the git diff below for reference

diff --git a/openwisp_notifications/admin.py b/openwisp_notifications/admin.py
index 0e320be..15636a8 100644
--- a/openwisp_notifications/admin.py
+++ b/openwisp_notifications/admin.py
@@ -10,16 +10,4 @@ Notification = load_model('Notification')
 NotificationSetting = load_model('NotificationSetting')
 
 
-class NotificationSettingInline(
-    NotificationSettingAdminMixin, AlwaysHasChangedMixin, admin.TabularInline
-):
-    model = NotificationSetting
-    extra = 0
-
-    def has_change_permission(self, request, obj=None):
-        return request.user.is_superuser or request.user == obj
-
-
-UserAdmin.inlines = [NotificationSettingInline] + UserAdmin.inlines
-
 _add_object_notification_widget()
diff --git a/openwisp_notifications/templates/admin/openwisp_users/user/change_form_object_tools.html b/openwisp_notifications/templates/admin/openwisp_users/user/change_form_object_tools.html
new file mode 100644
index 0000000..cd2761b
--- /dev/null
+++ b/openwisp_notifications/templates/admin/openwisp_users/user/change_form_object_tools.html
@@ -0,0 +1,6 @@
+{% extends "admin/change_form_object_tools.html" %}
+{% load i18n admin_urls %}
+{% block object-tools-items %}
+    <li><a class="button" href="/notifications/settings/">Notification preferences</a></li>
+    {{ block.super }}
+{% endblock %}

openwisp_notifications/api/serializers.py Outdated Show resolved Hide resolved
openwisp_notifications/tasks.py Outdated Show resolved Hide resolved
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.

Notes from today's weekly meeting:

  • Handling of API errors: the UI should not wait for succesful response to update itself, it should just rollback in case of errors
  • Switch Input Chekbox: we can use this solution (we need to make the :focus more evident and ajust the colors): https://www.w3schools.com/howto/howto_css_switch.asp

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.

  • update notification preferences button in user page to link the correct URL
  • notification preferences of another user should show the username in the first heading of the page, please do not change the top right header as discussed
  • in the notification widget, please change "settings" to "preferences"
  • once we are done we will need to update the docs

@Dhanus3133 Dhanus3133 changed the base branch from gsoc24 to gsoc24-rebased August 21, 2024 06:54
@Dhanus3133 Dhanus3133 changed the base branch from gsoc24-rebased to gsoc24 August 21, 2024 07:08
Dhanus3133 and others added 11 commits August 21, 2024 12:41
- Updated the `NotificationSettingSerializer` to also include
`organization_name`.
- New endpoint `/api/user-setting/organization/<uuid:organization_id>/`
to allow changes toggling of email/web notification settings of a
particular org with just a single API call.
openwisp_notifications/api/views.py Outdated Show resolved Hide resolved
openwisp_notifications/api/views.py Outdated Show resolved Hide resolved
openwisp_notifications/api/views.py Outdated Show resolved Hide resolved
Comment on lines 229 to 237
def get(self, request, user_id):
notification_settings, created = NotificationSetting.objects.get_or_create(
user_id=user_id,
organization=None,
type=None,
defaults={'email': True, 'web': True},
)
serializer = self.get_serializer(notification_settings)
return Response(serializer.data, status=status.HTTP_200_OK)
Copy link
Member

Choose a reason for hiding this comment

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

IIRC, we decided that we would make a migration file that would create global notification setting. I don't think, we would need create notification settings on the fly here.

Let me know if i missed on something.

Copy link
Member

Choose a reason for hiding this comment

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

I don't see a migration file in the PR. So, the current implement looks good.

@nemesifier what do you think about this?

Comment on lines 11 to 15

@property
def qs(self):
parent_qs = super().qs
return parent_qs.exclude(organization__isnull=True, type__isnull=True)
Copy link
Member

Choose a reason for hiding this comment

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

Why did you exclude the global notifications from here? This means that these settings will not appear when the user browse the NotificationSettingListView API endpoint from the browser. Thus, it makes impossible to turn off global notifications using the REST API.

Copy link
Member Author

@Dhanus3133 Dhanus3133 Sep 7, 2024

Choose a reason for hiding this comment

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

We already have a seperate endpoint to update the global notification settings here

path(
'user/<uuid:user_id>/preference/',
views.notification_preference,
name='notification_preference',
),

Copy link
Member

Choose a reason for hiding this comment

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

Thinking out loud: What are the benefits of having a separate endpoint for only managing global notifications?

openwisp_notifications/api/views.py Show resolved Hide resolved
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.

Notes from last round of testing during our weekly meeting:

  • Fix exception when deleting user (write a failing test first please)

    Added by @pandafy: Fix string representation of global NotificationSetting

  • In the test "test_post_migration_handler", add assertions to verify that global settings are being created

    Added by @pandafy: Update test_post_migration_handler in test_notification_settings.py.
    Delete the global notifications settings after creating the user.
    Call the notification_type_registered_unregistered_handler function
    and then assert that global notification setting is created.

  • Add validation in the model to ensure there can be only 1 global setting for each user

  • Let's use only 1 endpoint for notification settings

  • Remove the CSS transition from the arrow in the accordion of the UI

- Removed preference api endpoint and just used the `/user-setting`
endpoint.
- Fixed some bugs in preference page for handling few cases like
enabling of global web setting when any one email setting is turned on
(toggles it off).
- Handled validation of only one global setting per user in model level.
Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

Good progress @Dhanus3133 👏🏼 With these last few improvements, we will get this PR ready to merge.

@nemesifier what will be the best way to convey that there was an error loading the notification settings. There's a toast message which notifies that there was an error but the toast is not persistent. Shall we add a paragraph element conveying that there was an error?

Screenshot from 2024-09-16 23-03-27

Comment on lines 215 to 224
for notification_setting in notification_settings:
notification_setting.email = validated_data.get(
'email', notification_setting.email
)
notification_setting.web = validated_data.get(
'web', notification_setting.web
)
NotificationSetting.objects.bulk_update(
notification_settings, ['email', 'web']
)
Copy link
Member

Choose a reason for hiding this comment

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

I apologise, my previous comment was misleading. I meant to suggest update instead of bulk_update. So, the code would become:

Suggested change
for notification_setting in notification_settings:
notification_setting.email = validated_data.get(
'email', notification_setting.email
)
notification_setting.web = validated_data.get(
'web', notification_setting.web
)
NotificationSetting.objects.bulk_update(
notification_settings, ['email', 'web']
)
NotificationSetting.objects.update(
email=validated_data['email'],
web=validated_data['web'],
)

Comment on lines 208 to 229
def post(self, request, user_id, organization_id):
notification_settings = NotificationSetting.objects.filter(
organization_id=organization_id, user_id=user_id
)
serializer = self.get_serializer(data=request.data)
if serializer.is_valid():
validated_data = serializer.validated_data
web = validated_data.get('web')
email = validated_data.get('email')

for notification_setting in notification_settings:
if web and not email:
notification_setting.web = web
else:
notification_setting.web = web
notification_setting.email = email

NotificationSetting.objects.bulk_update(
notification_settings, ['web', 'email']
)
return Response(status=status.HTTP_200_OK)
return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
def post(self, request, user_id, organization_id):
notification_settings = NotificationSetting.objects.filter(
organization_id=organization_id, user_id=user_id
)
serializer = self.get_serializer(data=request.data)
if serializer.is_valid():
validated_data = serializer.validated_data
web = validated_data.get('web')
email = validated_data.get('email')
for notification_setting in notification_settings:
if web and not email:
notification_setting.web = web
else:
notification_setting.web = web
notification_setting.email = email
NotificationSetting.objects.bulk_update(
notification_settings, ['web', 'email']
)
return Response(status=status.HTTP_200_OK)
return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
def post(self, request, user_id, organization_id):
serializer = self.get_serializer(data=request.data)
if serializer.is_valid():
validated_data = serializer.validated_data
web = validated_data['web']
email = validated_data['email']
NotificationSetting.objects.filter(
organization_id=organization_id, user_id=user_id
).update(web=web, email=email)
return Response(status=status.HTTP_200_OK)
return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)

This way, we won't need to iterate over individual objects.

Copy link
Member

Choose a reason for hiding this comment

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

I used validated_data['web'] instead of validated_data.get('web') because NotificationSettingUpdateSerializer requires web and email fields to be present in valid payload.

Copy link
Member

Choose a reason for hiding this comment

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

After another review, I came to conclusion that we don't need to make any changes to the serializer. Please make changes to the test for this endpoint as requested in my previous review. We will see if something breaks because this change.

openwisp_notifications/tests/test_notifications.py Outdated Show resolved Hide resolved
openwisp_notifications/tests/test_admin.py Outdated Show resolved Hide resolved
openwisp_notifications/tests/test_api.py Outdated Show resolved Hide resolved
openwisp_notifications/tests/test_api.py Show resolved Hide resolved
openwisp_notifications/tests/test_api.py Outdated Show resolved Hide resolved
openwisp_notifications/tests/test_api.py Outdated Show resolved Hide resolved
openwisp_notifications/tests/test_selenium.py Outdated Show resolved Hide resolved
openwisp_notifications/tests/test_utils.py Outdated Show resolved Hide resolved
openwisp_notifications/views.py Outdated Show resolved Hide resolved
openwisp_notifications/tests/test_selenium.py Show resolved Hide resolved
Comment on lines 208 to 229
def post(self, request, user_id, organization_id):
notification_settings = NotificationSetting.objects.filter(
organization_id=organization_id, user_id=user_id
)
serializer = self.get_serializer(data=request.data)
if serializer.is_valid():
validated_data = serializer.validated_data
web = validated_data.get('web')
email = validated_data.get('email')

for notification_setting in notification_settings:
if web and not email:
notification_setting.web = web
else:
notification_setting.web = web
notification_setting.email = email

NotificationSetting.objects.bulk_update(
notification_settings, ['web', 'email']
)
return Response(status=status.HTTP_200_OK)
return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
Copy link
Member

Choose a reason for hiding this comment

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

After another review, I came to conclusion that we don't need to make any changes to the serializer. Please make changes to the test for this endpoint as requested in my previous review. We will see if something breaks because this change.

Comment on lines 310 to 322
try:
original = self.__class__.objects.get(pk=self.pk)
if self.web and (self.email == original.email):
self.user.notificationsetting_set.exclude(pk=self.pk).update(
web=self.web
)
else:
self.user.notificationsetting_set.exclude(pk=self.pk).update(
web=self.web, email=self.email
)
except self.__class__.DoesNotExist:
pass
return super().save(*args, **kwargs)
Copy link
Member

Choose a reason for hiding this comment

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

What are the drawbacks for upgrading the NotificationSetting using update()

self.user.notificationsetting_set(web=self.web, email=self.email)

What case would be left uncovered? Can you demonstrate that with a failing test case?

Copy link
Member Author

Choose a reason for hiding this comment

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

I have added a failing testcase for this

with self.subTest(
'Update organization-level web to False while keeping one of email notification setting to true'
):
url = self._get_path(
'organization_notification_setting',
self.admin.pk,
org.pk,
)
# Set the default type notification setting's email to True
NotificationSetting.objects.filter(
user=self.admin, organization_id=org.pk, type='default'
).update(email=True)
response = self.client.post(url, data={'web': True, 'email': False})
self.assertTrue(
NotificationSetting.objects.filter(
user=self.admin, organization_id=org.pk, type='default', email=True
).exists()
)

Also did add for the global setting here

with self.subTest(
'Update global web to False while ensuring at least one email setting is True'
):
# Set the default type notification setting's email to True
NotificationSetting.objects.filter(
user=admin, organization=org, type='default'
).update(email=True)
global_setting.web = True
global_setting.save()
self.assertTrue(
NotificationSetting.objects.filter(
user=admin, organization=org, email=True, type='default'
).exists()
)

Let me know what do you think about this.

Copy link
Member

Choose a reason for hiding this comment

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

I commented out the code in question, but the added test didn't fail. :(

I was expecting the test to fail without the added logic.

openwisp_notifications/base/models.py Outdated Show resolved Hide resolved
Comment on lines 850 to 871
with self.subTest(
'Update organization-level web to False while keeping one of email notification setting to true'
):
url = self._get_path(
'organization_notification_setting',
self.admin.pk,
org.pk,
)

# Set the default type notification setting's email to True
NotificationSetting.objects.filter(
user=self.admin, organization_id=org.pk, type='default'
).update(email=True)

response = self.client.post(url, data={'web': True, 'email': False})

self.assertTrue(
NotificationSetting.objects.filter(
user=self.admin, organization_id=org.pk, type='default', email=True
).exists()
)

Copy link
Member

@pandafy pandafy Sep 18, 2024

Choose a reason for hiding this comment

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

This is the scenario which I think this test emulates:

Starting state:

            NotificationSetting.objects.filter(
                user=self.admin, organization_id=org.pk, type='default'
            ).update(email=True)

Screenshot from 2024-09-18 20-12-10
state:

Turning off email notifications for the organization:

response = self.client.post(url, data={'web': True, 'email': False})

Screenshot from 2024-09-18 20-14-40

Contradictory to the UI

            self.assertTrue(
                NotificationSetting.objects.filter(
                    user=self.admin, organization_id=org.pk, type='default', email=True
                ).exists()
            )

The UI shows that all email notifications for the organization is disabled, which is correct. But, the test expects the one email notification to stay enabled.

Do I understand the test correctly?

Copy link
Member

@pandafy pandafy left a comment

Choose a reason for hiding this comment

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

@Dhanus3133 update the code (including tests) to call the full_clean model method before calling save on an individual object.

Django does not perform any validation if you save the object directly. The validation is performed in full_clean, hence it is required to call the method before save.

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.

[feature] Add a dedicated view for managing notification preferences
3 participants