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] Added generic message notification type (shown in dialg box) #275

Conversation

Dhanus3133
Copy link
Member

@Dhanus3133 Dhanus3133 commented May 18, 2024

Closes #254.

@Dhanus3133 Dhanus3133 force-pushed the feat/display-description-notification-dialog branch from d404586 to a15a284 Compare May 20, 2024 11:18
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.

@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 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.

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 Outdated Show resolved Hide resolved
openwisp_notifications/types.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.

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

Screenshot from 2024-05-20 17-04-03

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

Screenshot from 2024-05-20 17-09-06
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?

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

Choose a reason for hiding this comment

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

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.

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 reduced most of the CSS like on the buttons, but the existing CSS was not very helpful on the overlays.

Copy link
Member Author

@Dhanus3133 Dhanus3133 left a comment

Choose a reason for hiding this comment

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

Need some suggestions here.

openwisp_notifications/api/serializers.py Outdated Show resolved Hide resolved
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()
Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

// 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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

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 tried a different approach to solving it, but is it okay to let it be unread even if the user has read that message?

Copy link
Member

Choose a reason for hiding this comment

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

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.

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!

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:

Screenshot from 2024-05-23 20-01-37

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:

Screenshot from 2024-05-23 20-01-37

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

// 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){
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

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 Outdated Show resolved Hide resolved
openwisp_notifications/tests/test_widget.py Outdated Show resolved Hide resolved
openwisp_notifications/types.py Outdated Show resolved Hide resolved
</div>
${convertMessageWithRelativeURL(elem.message)}
${elem.description ? elem.message.replace(/<a [^>]*>([^<]*)<\/a>/g, '$1') : convertMessageWithRelativeURL(elem.message)}
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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
image
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)

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()
Copy link
Member

Choose a reason for hiding this comment

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

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.

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

Choose a reason for hiding this comment

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

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.

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 believe the file is test_notifications.py. Updated with the changes there.

@Dhanus3133
Copy link
Member Author

LGTM 🚀 ! @nemesifier

@nemesifier nemesifier added the enhancement New feature or request label May 30, 2024
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.

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.

Comment on lines 119 to 150
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
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 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.

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

def _set_extra_attributes(self, email_message=False):
Copy link
Member

Choose a reason for hiding this comment

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

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.

Comment on lines +328 to +333
{
'message': '[{notification.actor}]({notification.actor_link})',
'type': 'generic_message',
'description': '[{notification.actor}]({notification.actor_link})',
}
)
Copy link
Member

Choose a reason for hiding this comment

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

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

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.

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.

@@ -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');
Copy link
Member

Choose a reason for hiding this comment

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

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

@nemesifier 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
// 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();
Copy link
Member

Choose a reason for hiding this comment

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

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!!!

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated, let me know if that requires some changes.

Copy link
Member

Choose a reason for hiding this comment

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

It works, thanks!

@Dhanus3133 Dhanus3133 force-pushed the feat/display-description-notification-dialog branch from 9f33f4f to 08ab21b Compare June 7, 2024 07:41
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.

See my comments below @pandafy @Dhanus3133.

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

Choose a reason for hiding this comment

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

@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.

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

$(document).click(function (e) {
e.stopPropagation();
Copy link
Member

Choose a reason for hiding this comment

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

@pandafy I see a similar thing here 😨

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

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.

Great team work! @Dhanus3133 @pandafy 👏 👍 🎉

@nemesifier nemesifier merged commit 65b59c6 into openwisp:master Jun 13, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Development

Successfully merging this pull request may close these issues.

[feature] Add dedicated notification type for internal errors
3 participants