[feature] Added generic message notification type (shown in dialg box) #275

Dhanus3133 · 2024-05-18T18:40:01Z

Closes #254.

nemesifier

@Dhanus3133 as part of UI changes, it's nice to post screenshots during reviews. Can you please add a few here?

I think we need to mention this new feature in the README, at the end of the Notification Types section (PS: we've been using tools like ChatGPT and Google Gemini recently to help us write better docs, please give it a try).

We also need to add a basic test, we could reuse the test file we added recently, test_widget.

openwisp_notifications/tests/test_api.py

nemesifier

One more note about the README, let's also add an example of usage of this notification type, eg: a few lines of code which send a generic notification.

openwisp_notifications/types.py

nemesifier

A few more quick notes. Let's discuss these in our weekly meeting.

Short summary

I try to create a test notification with:

default_org = Organization.objects.first()
notify.send(
    sender=default_org,
    description='Test.',
    message='Something alarming happened!',
    type='general_message',
    target=default_org
)

I see a notification created with the following text:

Message notification with message verb and level info by default.

How can modules change this text to convey a short summary to the user?

Links

Clicking on any link in the notification causes the dialog to appear but the page reloads and the dialog disappears, which is not good for UX.

Let's discuss possible solutions

Not specifying a target causes JS to fail

Try the following.

default_org = Organization.objects.first()
notify.send(
    sender=default_org,
    description='Test.',
    message='Something alarming happened!',
    type='general_message'
)

The JS fails for me with the following error:

notifications.js:215 Uncaught TypeError: Failed to construct 'URL': Invalid URL
    at notificationListItem (notifications.js:215:28)
    at notifications.js:117:26
    at Array.forEach (<anonymous>)
    at pageContainer (notifications.js:116:14)
    at appendPage (notifications.js:124:46)
    at Object.success (notifications.js:162:21)
    at fire (jquery.js:3536:31)
    at Object.fireWith [as resolveWith] (jquery.js:3666:7)
    at done (jquery.js:9878:14)
    at XMLHttpRequest.<anonymous> (jquery.js:10139:9)

If I am not mistaken, the goal here is to allow creating generic messages that may not point to any object if needed.

Overlay issue

The navigation menu is not covered by the overlay, this seems odd to me: it's not consistent with other features of the UI.
This probably depends on the placing of the overlay, which must be placed high enough in the HTML tree to be able to cover the menu.

Markdown not support

ed

I probably forgot to specify this in the issue desc, but our notification messages support markdown to allow us to format the text if needed.

Can you support markdown in the rendering of this overlay?

Make X close button bigger

The X close button seems too small to me right now, can you please increase it slightly?

nemesifier · 2024-05-20T20:57:23Z

openwisp_notifications/static/openwisp-notifications/css/notifications.css

+}
+.ow-dialog-buttons button:hover {
+  background-color: #0056b3;
+}


Please reuse existing CSS classes instead of creating new ones, eg: for buttons.
Adding new CSS classes for each feature creates UX inconsistency and adds too much maintenance overhead.

Please look around, we have a lot of existing classes for overlays and buttons, let's reuse as much as possible what's already there, it ensures consistency and makes maintenance easier when we need to update UI styles.

I have reduced most of the CSS like on the buttons, but the existing CSS was not very helpful on the overlays.

Dhanus3133

Need some suggestions here.

openwisp_notifications/api/serializers.py

Dhanus3133 · 2024-05-23T17:30:47Z

openwisp_notifications/tests/test_widget.py

+        WebDriverWait(self.web_driver, 10).until(
+            EC.visibility_of_element_located((By.ID, f'ow-{notification.id}'))
+        )
+        self.web_driver.find_element(By.ID, f'ow-{notification.id}').click()


The click on the notification element doesn't seem to be clicking properly. I'm not sure why this occurs, but when manually clicked, it has no issues.

I suspect this event handler is interfering with the functionality of opening dialog box.

openwisp-notifications/openwisp_notifications/static/openwisp-notifications/js/notifications.js

Lines 316 to 328 in 6ea7022

// Handler for marking notification as read and opening target url

$('.ow-notification-wrapper').on('click keypress', '.ow-notification-elem', function (e) {

// Open target URL only when "Enter" key is pressed

if ((e.type === 'keypress') && (e.which !== 13)){

return;

}

let elem = $(this);

// If notification is unread then send read request

if (elem.hasClass('unread')) {

markNotificationRead(elem.get(0));

}

window.location = elem.data('location');

});

If I execute self.web_driver.find_element(By.ID, f'ow-{notification.id}').click() twice, then the test case pases without any error.

I think, we should add a special case for the generic notifications. Maybe, we can add a CSS class for only generic notifications and exclude those elements from this event handler. Do we have information on the frontend apart from the notification type?

Yeah running twice fixes the issue. The dialog appears when the description is available for any type of notification.

openwisp-notifications/openwisp_notifications/static/openwisp-notifications/js/notifications.js

Line 329 in a7c792d

if (notification.get_description) {

I think we can use that to add an additional CSS class to the notification element. If that class is present, the the logic for marking notification as read is skipped and it opens the notification dialog directly. We can also use a data attribute for this.

I have tried a different approach to solving it, but is it okay to let it be unread even if the user has read that message?

I have tried a different approach to solving it, but is it okay to let it be unread even if the user has read that message?

No, it is not.

nemesifier

Good progress @Dhanus3133!

This review is not exhaustive, I just want to point out the most obvious issues I see after your changes.

I will do more testing in the coming days but in the meanwhile you can fix the issues below.

Menu still not covered by overlay

This problem still stands:

I recommend adding the overlay programmatically in a different HTML root element to fix this.

Notification shown on screen does not trigger overlay

Execute the following from the django shell while you have the application running in your browser:

from openwisp_notifications.signals import notify
notify.send(
    sender=default_org,
    description='Test.',
    message='Something alarming happened!',
    type='generic_message',
    target=default_org
)

The following appears:

Try to click on it.
I would expect the generic message to appear, but that's not the case.

nemesifier · 2024-05-25T19:04:41Z

openwisp_notifications/static/openwisp-notifications/js/notifications.js

            // Don't hide if focus changes to notification bell icon
            if (e.target != $('#openwisp_notifications').get(0)) {
                $('.ow-notification-dropdown').addClass('ow-hide');
            }
        }
    });

-    $('.ow-notification-dropdown').on('keyup', '*', function(e){
+    $(document).on('keyup', '*', function(e){


This will fire on every keyup event that will happen in OpenWISP. Are you sure this change is intended?
Can't we narrow down the firing a bit to make sure it's not fired unless needed?

This allows the notification dialog or widget to close whenever the ESC is pressed. I would like to know if there is any other way to reduce it.

While this is a useful, yet basic feature. I don't know how I feel about triggering an event handler for every key press. This JS is loaded on all the pages of OpenWISP. so this event handler will get executed when filling out any field in OpenWISP.

If we plan to to keep this, we shall adopt the concept of early return. In this case, return from the function if the key is not Esc.

openwisp_notifications/static/openwisp-notifications/js/notifications.js

openwisp_notifications/base/models.py

openwisp_notifications/api/serializers.py

pandafy

Great work @Dhanus3133! I did an extensive code review where I also tried the code suggestions locally and tried to break the logic. For the most part, you have done a phenomenal job.

I have left some suggestions below to improve the code implementation.

openwisp_notifications/templates/admin/base_site.html

openwisp_notifications/tests/test_widget.py

openwisp_notifications/types.py

openwisp_notifications/static/openwisp-notifications/js/notifications.js

pandafy · 2024-05-27T16:45:09Z

openwisp_notifications/static/openwisp-notifications/js/notifications.js

                        </div>
-                    ${convertMessageWithRelativeURL(elem.message)}
+                    ${elem.description ? elem.message.replace(/<a [^>]*>([^<]*)<\/a>/g, '$1') : convertMessageWithRelativeURL(elem.message)}


It is not clear to me what are we doing here.

Actually, in the call, we discussed on the generic message type we don't want to allow users to click on the target_url directly something like this on the Org 2

So, with this regex, we eliminate this ability. The users view the description and then proceed to the target_url via the dialog. (Will add comments there)

openwisp_notifications/static/openwisp-notifications/js/notifications.js

pandafy · 2024-05-27T17:14:30Z

openwisp_notifications/tests/test_widget.py

+        WebDriverWait(self.web_driver, 10).until(
+            EC.visibility_of_element_located((By.ID, f'ow-{notification.id}'))
+        )
+        self.web_driver.find_element(By.ID, f'ow-{notification.id}').click()


I think we can use that to add an additional CSS class to the notification element. If that class is present, the the logic for marking notification as read is skipped and it opens the notification dialog directly. We can also use a data attribute for this.

pandafy · 2024-05-27T17:36:16Z

openwisp_notifications/tests/test_notifications.py

@@ -60,7 +60,6 @@ def setUp(self):
        self.notification_options = dict(
            sender=self.admin,
            description='Test Notification',
-            level='info',


I went ahead and fixed the reason for failing test. This would allow us to close this PR quickly.

In previous commits, we changed the logic of the notify handler to give preference to the level kwarg passed to the notify signal.

@Dhanus3133 we need to add a test case in test_models.py which verifies that the preference of level is given to the level kwargs over the level defined in notification type.

I believe the file is test_notifications.py. Updated with the changes there.

nemesifier

I made some improvements to the JS for readability and for using the relative path also in the notification toast (which was still using the absolute path), please double check.

From my side this is good to go! 👍 👏 🚀

Dhanus3133 · 2024-05-30T15:32:20Z

LGTM 🚀 ! @nemesifier

openwisp_notifications/tests/test_notifications.py

nemesifier

The README update was missing. I added it, please have a look at it and use it as a reference for future modifications.

While writing the README I tried to come up with a simple example that doesn't specify the target object and I noticed the "Open" button is shown anyway, which is not right.

Please make sure that if the target is not specified, the "Open" button does not appear.

That way we can use the notification module to communicate any message to users, even if there isn't any object related to the message we're sending.

…fication

pandafy · 2024-05-30T20:36:31Z

openwisp_notifications/base/models.py

+    def _set_extra_attributes(self, email_message=False):
+        """
+        This method sets extra attributes for the notification
+        which are required to render the notification correctly.
+        """
+        self.actor_link = self.actor_url
+        self.action_link = self.action_url
+        self.target_link = (
+            self.target_url if not email_message else self.redirect_view_url
+        )
+
+    def _delete_extra_attributes(self):
+        """
+        This method removes the extra_attributes set by
+        _set_extra_attributes
+        """
+        for attr in ('actor_link', 'action_link', 'target_link'):
+            delattr(self, attr)
+
+    def _get_formatted_string(self, string, email_message=False):
+        self._set_extra_attributes(email_message=email_message)
+        data = self.data or {}
+        try:
+            formatted_output = string.format(notification=self, **data)
+        except AttributeError as exception:
+            self._invalid_notification(
+                self.pk,
+                exception,
+                'Error encountered in rendering notification message',
+            )
+        self._delete_extra_attributes()
+        return formatted_output


This is required because we handle the target URL differently when we send the email.

The email includes a ridrect URL, which flags the notification as read and then redirects the user to the target object.

pandafy · 2024-05-30T20:37:33Z

openwisp_notifications/base/models.py

    @property
    def email_message(self):
        return self.get_message(email_message=True)

+    def _set_extra_attributes(self, email_message=False):


I thought of performing these operations in the __init__, but the target_link is dynamic based on whether we are using the message in the email.

pandafy · 2024-05-30T20:38:17Z

openwisp_notifications/tests/test_notifications.py

+            {
+                'message': '[{notification.actor}]({notification.actor_link})',
+                'type': 'generic_message',
+                'description': '[{notification.actor}]({notification.actor_link})',
+            }
+        )


I improved this test to verity markdown rendering of message and description works.

nemesifier

We are working with @pandafy to improve the changes he is making to allow him to accomplish what he needs to do in openwisp/openwisp-controller#865.

@Dhanus3133 please see my last request below, with that I think we won't need your help anymore on this.

nemesifier · 2024-05-31T18:26:05Z

openwisp_notifications/static/openwisp-notifications/js/notifications.js

@@ -387,6 +387,7 @@ function notificationHandler($, elem) {

    if (notification.target_url && notification.target_url !== '#') {
        targetUrl = new URL(notification.target_url).pathname;
+        $('.ow-message-target-redirect').removeClass('ow-hide');


Can you please add one new selenium test method only for this specific detail, which is quite important?

pandafy · 2024-06-06T19:15:22Z

openwisp_notifications/static/openwisp-notifications/js/notifications.js

            // Don't hide if focus changes to notification bell icon
            if (e.target != $('#openwisp_notifications').get(0)) {
                $('.ow-notification-dropdown').addClass('ow-hide');
            }
        }
    });

-    $('.ow-notification-dropdown').on('keyup', '*', function(e){
+    $(document).on('keyup', '*', function(e){
        e.stopPropagation();


We should avoid doing such practices because it would break functionalities added by other modules.

This is the reason for openwisp/openwisp-controller#868 bug.

I apologise that this exist!!!

I think we find a way to avoid to bind to the document like that, it should be possible to specify to which element to bind, or filter the elements instead of using *, probably also removing e.stopPropagation(); may be a good idea, depending if we bind this event to an HTML container shared with the rest of the UI or with some other HTML container which is specific to the notifications module.

@Dhanus3133 please let me know if the suggestion above is clear.

Updated, let me know if that requires some changes.

It works, thanks!

nemesifier

See my comments below @pandafy @Dhanus3133.

nemesifier · 2024-06-07T15:35:16Z

openwisp_notifications/static/openwisp-notifications/js/notifications.js

@@ -69,30 +75,53 @@ function initNotificationDropDown($) {
    $(document).focusin(function(e){


@pandafy do you think we could avoid binding on document here too? That stopPropagation() below scares me. We could incur in the same problem we had for the keyup event in other modules if we need to use the same JS event. The problem will be that to find this will probably take hours because by that time we'll have forgotten.
We can open a new issue for this.

openwisp_notifications/static/openwisp-notifications/js/notifications.js

nemesifier · 2024-06-07T15:40:00Z

openwisp_notifications/static/openwisp-notifications/js/notifications.js

@@ -56,10 +57,15 @@ function initNotificationDropDown($) {

    $(document).click(function (e) {
        e.stopPropagation();


@pandafy I see a similar thing here 😨

nemesifier

Please see my comments below. I am insisting with these code comment details because it's important. If we add complexity without commenting the code properly we make maintenance of the system harder.

openwisp_notifications/static/openwisp-notifications/js/notifications.js

nemesifier

Great team work! @Dhanus3133 @pandafy 👏 👍 🎉

Dhanus3133 added 5 commits May 14, 2024 17:17

[feature] Description of notification shown in a dialog admin site

c207c9d

[qa] Fix

1025bfb

[chore] Add trans

2605a09

Merge branch 'master' into feat/display-description-notification-dialog

bf3ba25

[chore] Increase page_size

a15a284

Dhanus3133 force-pushed the feat/display-description-notification-dialog branch from d404586 to a15a284 Compare May 20, 2024 11:18

nemesifier requested changes May 20, 2024

View reviewed changes

openwisp_notifications/tests/test_api.py Show resolved Hide resolved

nemesifier requested changes May 20, 2024

View reviewed changes

openwisp_notifications/types.py Outdated Show resolved Hide resolved

openwisp_notifications/types.py Outdated Show resolved Hide resolved

nemesifier requested changes May 20, 2024

View reviewed changes

Dhanus3133 added 4 commits May 22, 2024 20:55

[chore] Fix relative URL error

3a0377d

[chore] Improvements

2be7037

[chore] Show description with md support

581cc90

[chore] Rename general to generic message types

a7c792d

Dhanus3133 commented May 23, 2024

View reviewed changes

nemesifier requested changes May 24, 2024

View reviewed changes

Dhanus3133 added 3 commits May 25, 2024 23:46

[chore] Fix menu bar

9e7c1fc

[chore] Rename rendered_description

2608c11

[chore] Fix test

2a922a6

nemesifier requested changes May 25, 2024

View reviewed changes

Dhanus3133 added 4 commits May 26, 2024 16:28

[chore] Just use description

0686c2d

[fix] Rendered description shows error for None description

8106f3e

[fix] Level icon repeat

cc528df

[chore] JQuery way

aca39bc

pandafy requested changes May 27, 2024

View reviewed changes

[fix] Fixed tests

1e6b840

pandafy reviewed May 27, 2024

View reviewed changes

Dhanus3133 added 4 commits May 28, 2024 01:07

[chore] Bump changes

00ec345

[chore] Add tests for notification level

e69600f

[chore] Remove comment

bbff510

Merge branch 'master' into feat/display-description-notification-dialog

8dfcfce

nemesifier approved these changes May 30, 2024

View reviewed changes

nemesifier assigned Dhanus3133 May 30, 2024

nemesifier added the enhancement New feature or request label May 30, 2024

nemesifier added this to In progress in OpenWISP Priorities for next releases via automation May 30, 2024

pandafy reviewed May 30, 2024

View reviewed changes

openwisp_notifications/tests/test_notifications.py Show resolved Hide resolved

[chore] Add generic_message test

9ed5982

nemesifier requested changes May 30, 2024

View reviewed changes

nemesifier and others added 2 commits May 30, 2024 14:59

[docs] Added generic_message

4a9956b

[feature] Support markdown in message and description of generic noti…

4593ef3

…fication

pandafy reviewed May 30, 2024

View reviewed changes

pandafy mentioned this pull request May 30, 2024

[change] Notify users of background subnet division rule errors #837 openwisp/openwisp-controller#865

Merged

1 task

[fix] Open button display only when target_url is available

9f11de6

nemesifier requested changes May 31, 2024

View reviewed changes

pandafy and others added 2 commits June 1, 2024 00:58

[changes] Use context manager to handle temporary attribtues

b09cd6f

[chore] Add Tests for open button visibility

8229268

nemesifier changed the title ~~[feature] Description of notification shown in a dialog box~~ [feature] Added generic message notification type (shown in dialg box) Jun 5, 2024

pandafy reviewed Jun 6, 2024

View reviewed changes

nemesifier mentioned this pull request Jun 7, 2024

[bug:ui] Preview keyboard keys do not work anymore openwisp/openwisp-controller#868

Open

[fix] Keyup event handler

08ab21b

Dhanus3133 force-pushed the feat/display-description-notification-dialog branch from 9f33f4f to 08ab21b Compare June 7, 2024 07:41

Merge branch 'master' into feat/display-description-notification-dialog

69bfb39

nemesifier reviewed Jun 7, 2024

View reviewed changes

[chore] Refactor message conversion to improve readability

0020c21

nemesifier requested changes Jun 7, 2024

View reviewed changes

openwisp_notifications/static/openwisp-notifications/js/notifications.js Outdated Show resolved Hide resolved

openwisp_notifications/static/openwisp-notifications/js/notifications.js Show resolved Hide resolved

[chore] Comment changes

859734e

nemesifier approved these changes Jun 13, 2024

View reviewed changes

nemesifier merged commit 65b59c6 into openwisp:master Jun 13, 2024
10 checks passed

OpenWISP Priorities for next releases automation moved this from In progress to Done Jun 13, 2024

	// Handler for marking notification as read and opening target url
	$('.ow-notification-wrapper').on('click keypress', '.ow-notification-elem', function (e) {
	// Open target URL only when "Enter" key is pressed
	if ((e.type === 'keypress') && (e.which !== 13)){
	return;
	}
	let elem = $(this);
	// If notification is unread then send read request
	if (elem.hasClass('unread')) {
	markNotificationRead(elem.get(0));
	}
	window.location = elem.data('location');
	});

		@@ -69,30 +75,53 @@ function initNotificationDropDown($) {
		$(document).focusin(function(e){

		@@ -56,10 +57,15 @@ function initNotificationDropDown($) {

		$(document).click(function (e) {
		e.stopPropagation();

[feature] Added generic message notification type (shown in dialg box) #275

[feature] Added generic message notification type (shown in dialg box) #275

Conversation

Dhanus3133 commented May 18, 2024 • edited by nemesifier Loading

nemesifier left a comment • edited Loading

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

nemesifier left a comment • edited Loading

Choose a reason for hiding this comment

Short summary

Links

Not specifying a target causes JS to fail

Overlay issue

Markdown not support

Make X close button bigger

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dhanus3133 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Menu still not covered by overlay

Notification shown on screen does not trigger overlay

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pandafy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Dhanus3133 commented May 30, 2024

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

nemesifier left a comment

Choose a reason for hiding this comment

Dhanus3133 commented May 18, 2024 •

edited by nemesifier

Loading

nemesifier left a comment •

edited

Loading

nemesifier left a comment •

edited

Loading