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

Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
c207c9d
[feature] Description of notification shown in a dialog admin site
Dhanus3133 May 14, 2024
1025bfb
[qa] Fix
Dhanus3133 May 14, 2024
2605a09
[chore] Add trans
Dhanus3133 May 14, 2024
bf3ba25
Merge branch 'master' into feat/display-description-notification-dialog
Dhanus3133 May 18, 2024
a15a284
[chore] Increase page_size
Dhanus3133 May 20, 2024
3a0377d
[chore] Fix relative URL error
Dhanus3133 May 22, 2024
2be7037
[chore] Improvements
Dhanus3133 May 23, 2024
581cc90
[chore] Show description with md support
Dhanus3133 May 23, 2024
a7c792d
[chore] Rename general to generic message types
Dhanus3133 May 23, 2024
9e7c1fc
[chore] Fix menu bar
Dhanus3133 May 25, 2024
2608c11
[chore] Rename rendered_description
Dhanus3133 May 25, 2024
2a922a6
[chore] Fix test
Dhanus3133 May 25, 2024
0686c2d
[chore] Just use description
Dhanus3133 May 26, 2024
8106f3e
[fix] Rendered description shows error for None description
Dhanus3133 May 26, 2024
cc528df
[fix] Level icon repeat
Dhanus3133 May 26, 2024
aca39bc
[chore] JQuery way
Dhanus3133 May 26, 2024
1e6b840
[fix] Fixed tests
pandafy May 27, 2024
00ec345
[chore] Bump changes
Dhanus3133 May 27, 2024
e69600f
[chore] Add tests for notification level
Dhanus3133 May 27, 2024
bbff510
[chore] Remove comment
Dhanus3133 May 27, 2024
8dfcfce
Merge branch 'master' into feat/display-description-notification-dialog
Dhanus3133 May 28, 2024
df10915
[chore] Focus fix
Dhanus3133 May 28, 2024
6e5db98
[chores] UI/CSS improvements
nemesifier May 29, 2024
6de368f
[fix] Toast notifications dialog
Dhanus3133 May 30, 2024
b0725b1
[fix] Use relative path also for notification toast
nemesifier May 30, 2024
9ed5982
[chore] Add generic_message test
Dhanus3133 May 30, 2024
4a9956b
[docs] Added generic_message
nemesifier May 30, 2024
4593ef3
[feature] Support markdown in message and description of generic noti…
pandafy May 30, 2024
9f11de6
[fix] Open button display only when target_url is available
Dhanus3133 May 31, 2024
b09cd6f
[changes] Use context manager to handle temporary attribtues
pandafy May 31, 2024
8229268
[chore] Add Tests for open button visibility
Dhanus3133 Jun 1, 2024
08ab21b
[fix] Keyup event handler
Dhanus3133 Jun 7, 2024
69bfb39
Merge branch 'master' into feat/display-description-notification-dialog
nemesifier Jun 7, 2024
0020c21
[chore] Refactor message conversion to improve readability
Dhanus3133 Jun 7, 2024
859734e
[chore] Comment changes
Dhanus3133 Jun 8, 2024
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions openwisp_notifications/api/serializers.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,6 +59,7 @@ class Meta(NotificationSerializer.Meta):
fields = [
'id',
'message',
'get_description',
Dhanus3133 marked this conversation as resolved.
Show resolved Hide resolved
'unread',
'target_url',
'email_subject',
Expand Down
8 changes: 7 additions & 1 deletion openwisp_notifications/base/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,10 @@ def target_url(self):
def message(self):
return self.get_message()

@cached_property
def get_description(self):
return mark_safe(markdown(self.description))

@property
def email_message(self):
return self.get_message(email_message=True)
Expand All @@ -120,7 +124,9 @@ def get_message(self, email_message=False):
try:
config = get_notification_configuration(self.type)
data = self.data or {}
if 'message' in config:
if 'message' in data:
md_text = data['message']
elif 'message' in config:
md_text = config['message'].format(notification=self, **data)
else:
md_text = render_to_string(
Expand Down
4 changes: 2 additions & 2 deletions openwisp_notifications/handlers.py
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,8 @@ def notify_handler(**kwargs):
except NotificationRenderException as error:
logger.error(f'Error encountered while creating notification: {error}')
return
level = notification_template.get(
'level', kwargs.pop('level', Notification.LEVELS.info)
level = kwargs.pop(
'level', notification_template.get('level', Notification.LEVELS.info)
)
verb = notification_template.get('verb', kwargs.pop('verb', None))
user_app_name = User._meta.app_label
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,64 @@
border-bottom-left-radius: 3px;
border-bottom-right-radius: 3px;
}
.ow-dialog-overlay {
position: fixed;
left: 0;
top: 0;
width: 100%;
height: 100%;
background-color: rgba(0, 0, 0, 0.6);
display: flex;
justify-content: center;
align-items: center;
transition: opacity 0.3s;
}
.ow-dialog-notification {
position: relative;
background-color: white;
padding: 20px;
padding-top: 40px;
border-radius: 10px;
box-shadow: 0 5px 15px rgba(0, 0, 0, 0.3);
width: 90%;
max-width: 500px;
text-align: left;
}
.ow-dialog-notification-level-wrapper {
display: flex;
justify-content: space-between;
color: #777;
}
.ow-dialog-notification .icon {
min-height: 15;
min-width: 15px;
}
.ow-dialog-close-x {
color: #333;
cursor: pointer;
font-size: 1.75em;
position: absolute;
display: block;
font-weight: bold;
top: 10px;
right: 10px;
}
.ow-dialog-close-x:hover {
color: #e04343;
}
.ow-message-title {
color: #333;
margin-bottom: 10px;
}
.ow-message-title a {
color: #df5d43 !important;
font-weight: bold;
}
.ow-message-description {
margin-bottom: 20px;
line-height: 1.6;
color: #666;
}

/* Notification bell */
.ow-notifications {
Expand Down Expand Up @@ -257,6 +315,7 @@
.ow-notification-level-text {
padding: 0px 6px;
text-transform: uppercase;
font-weight: bold;
}
.ow-notification-elem .icon,
.ow-notification-toast .icon {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -87,6 +87,21 @@ function initNotificationDropDown($) {
});
}

// Used to convert absolute URLs in notification messages to relative paths
function convertMessageWithRelativeURL(htmlString) {
const parser = new DOMParser(),
doc = parser.parseFromString(htmlString, 'text/html'),
links = doc.querySelectorAll('a');
links.forEach((link) => {
let url = link.getAttribute('href');
if (url) {
url = new URL(url, window.location.href);
link.setAttribute('href', url.pathname);
}
});
return doc.body.innerHTML;
}

function notificationWidget($) {

let nextPageUrl = getAbsoluteUrl('/api/v1/notifications/notification/'),
Expand Down Expand Up @@ -197,7 +212,7 @@ function notificationWidget($) {
function notificationListItem(elem) {
let klass;
const datetime = dateTimeStampToDateTimeLocaleString(new Date(elem.timestamp)),
target_url = new URL(elem.target_url);
target_url = new URL(elem.target_url, window.location.href);
Dhanus3133 marked this conversation as resolved.
Show resolved Hide resolved

if (!notificationReadStatus.has(elem.id)) {
if (elem.unread) {
Expand All @@ -208,32 +223,17 @@ function notificationWidget($) {
}
klass = notificationReadStatus.get(elem.id);

// Used to convert absolute URLs in notification messages to relative paths
function convertMessageWithRelativeURL(htmlString) {
const parser = new DOMParser(),
doc = parser.parseFromString(htmlString, 'text/html'),
links = doc.querySelectorAll('a');
links.forEach((link) => {
let url = link.getAttribute('href');
if (url) {
url = new URL(url);
link.setAttribute('href', url.pathname);
}
});
return doc.body.innerHTML;
}

return `<div class="ow-notification-elem ${klass}" id=ow-${elem.id}
data-location="${target_url.pathname}" role="link" tabindex="0">
<div class="ow-notification-inner">
<div class="ow-notification-meta">
<div class="ow-notification-level-wrapper">
<div class="ow-notify-${elem.level} icon"></div>
<div class="ow-notification-level-text">${elem.level}</div>
<div class="ow-notification-level-wrapper">
<div class="ow-notify-${elem.level} icon"></div>
<div class="ow-notification-level-text">${elem.level}</div>
</div>
<div class="ow-notification-date">${datetime}</div>
</div>
<div class="ow-notification-date">${datetime}</div>
</div>
${convertMessageWithRelativeURL(elem.message)}
${elem.get_description ? elem.message.replace(/<a [^>]*>([^<]*)<\/a>/g, '$1') : convertMessageWithRelativeURL(elem.message)}
</div>
</div>`;
}
Expand Down Expand Up @@ -324,7 +324,41 @@ function notificationWidget($) {
if (elem.hasClass('unread')) {
markNotificationRead(elem.get(0));
}
window.location = elem.data('location');

var notification = fetchedPages.flat().find((notification) => notification.id == elem.get(0).id.replace('ow-', ''));
if (notification.get_description) {
const datetime = dateTimeStampToDateTimeLocaleString(new Date(notification.timestamp));
document.querySelector('.ow-dialog-notification-level-wrapper').innerHTML = `
<div class="ow-notification-level-wrapper">
<div class="ow-notify-${notification.level} icon"></div>
<div class="ow-notification-level-text">${notification.level}</div>
</div>
<div class="ow-notification-date">${datetime}</div>
`;
document.querySelector('.ow-message-title').innerHTML = convertMessageWithRelativeURL(notification.message);
document.querySelector('.ow-message-description').innerHTML = notification.get_description;
$('.ow-dialog-overlay').removeClass('ow-hide');
if (notification.target_url) {
var target_url = new URL(notification.target_url, window.location.href);
$(document).on('click', '.ow-message-target-redirect', function () {
window.location = target_url.pathname;
});
$('.ow-message-target-redirect').removeClass('ow-hide');
}
Dhanus3133 marked this conversation as resolved.
Show resolved Hide resolved
} else {
window.location = elem.data('location');
}
});

// Close dialog on click, keypress or esc
$('.ow-dialog-close').on('click keypress', function (e) {
if (e.type === 'keypress' && e.which !== 13 && e.which !== 27) {
return;
}
$('.ow-dialog-overlay').addClass('ow-hide');
if (!$('.ow-message-target-redirect').hasClass('ow-hide')) {
$('.ow-message-target-redirect').addClass('ow-hide');
}
});

// Handler for marking notification as read on mouseout event
Expand Down
10 changes: 10 additions & 0 deletions openwisp_notifications/templates/admin/base_site.html
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,16 @@
<div class="ow-no-notifications ow-round-bottom-border ow-hide">
<p>{% trans 'No new notification.' %}</p>
</div>
<div class="ow-dialog-overlay ow-overlay-inner ow-hide">
<div class="ow-dialog-notification">
<span class="ow-dialog-close ow-dialog-close-x">&times;</span>
<div class="ow-notification-level-wrapper ow-dialog-notification-level-wrapper"></div>
<h2 class="ow-message-title"></h2>
<div class="ow-message-description"></div>
<button class="ow-message-target-redirect ow-hide button default">{% trans 'Open' %}</button>
<button class="ow-dialog-close button default">{% trans 'Close' %}</button>
</div>
</div>
Dhanus3133 marked this conversation as resolved.
Show resolved Hide resolved
</div>
{% endblock %}

Expand Down
2 changes: 1 addition & 1 deletion openwisp_notifications/tests/test_api.py
Original file line number Diff line number Diff line change
Expand Up @@ -533,7 +533,7 @@ def test_notification_setting_list_api(self):
self.assertEqual(len(response.data['results']), number_of_settings)

with self.subTest('Test "page_size" query'):
page_size = 1
page_size = 2
nemesifier marked this conversation as resolved.
Show resolved Hide resolved
url = f'{url}?page_size={page_size}'
response = self.client.get(url)
self.assertEqual(response.status_code, 200)
Expand Down
59 changes: 41 additions & 18 deletions openwisp_notifications/tests/test_widget.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,11 @@
import time

from django.contrib.staticfiles.testing import StaticLiveServerTestCase
from selenium.webdriver.common.by import By
from selenium.webdriver.support import expected_conditions as EC
from selenium.webdriver.support.ui import WebDriverWait

from openwisp_notifications.signals import notify
from openwisp_notifications.swapper import load_model
from openwisp_notifications.utils import _get_object_link
from openwisp_users.tests.utils import TestOrganizationMixin
Expand All @@ -22,27 +25,23 @@ def setUp(self):
self.admin = self._create_admin(
username=self.admin_username, password=self.admin_password
)

def test_notification_relative_link(self):
self.login()
operator = super()._create_operator()
data = dict(
email_subject='Test Email subject',
url='http://localhost:8000/admin/',
)
notification = Notification.objects.create(
actor=self.admin,
self.operator = super()._get_operator()
self.notification_options = dict(
sender=self.admin,
recipient=self.admin,
description='Test Notification Description',
verb='Test Notification',
action_object=operator,
target=operator,
data=data,
)
self.web_driver.implicitly_wait(10)
WebDriverWait(self.web_driver, 10).until(
EC.visibility_of_element_located((By.ID, 'openwisp_notifications'))
email_subject='Test Email subject',
action_object=self.operator,
target=self.operator,
type='default',
)

def _create_notification(self):
return notify.send(**self.notification_options)

def test_notification_relative_link(self):
self.login()
notification = self._create_notification().pop()[1][0]
self.web_driver.find_element(By.ID, 'openwisp_notifications').click()
WebDriverWait(self.web_driver, 10).until(
EC.visibility_of_element_located((By.CLASS_NAME, 'ow-notification-elem'))
Expand All @@ -54,3 +53,27 @@ def test_notification_relative_link(self):
self.assertEqual(
data_location_value, _get_object_link(notification, 'target', False)
)

def test_notification_dialog(self):
self.login()
self.notification_options.update(
{'message': 'Test Message', 'description': 'Test Description'}
)
notification = self._create_notification().pop()[1][0]
self.web_driver.find_element(By.ID, 'openwisp_notifications').click()
time.sleep(4)
Dhanus3133 marked this conversation as resolved.
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.

WebDriverWait(self.web_driver, 10).until(
EC.visibility_of_element_located((By.CLASS_NAME, 'ow-dialog-notification'))
)
dialog = self.web_driver.find_element(By.CLASS_NAME, 'ow-dialog-notification')
self.assertEqual(
dialog.find_element(By.CLASS_NAME, 'ow-message-title').text, 'Test Message'
)
self.assertEqual(
dialog.find_element(By.CLASS_NAME, 'ow-message-description').text,
'Test Description',
)
19 changes: 18 additions & 1 deletion openwisp_notifications/types.py
Original file line number Diff line number Diff line change
Expand Up @@ -17,9 +17,26 @@
'email_notification': True,
'web_notification': True,
},
'generic_message': {
'level': 'info',
'verb': 'generic verb',
'verbose_name': 'Generic Type',
'email_subject': '[{site.name}] Generic Notification Subject',
'message': (
'Generic notification with {notification.verb} and level {notification.level}'
' by [{notification.actor}]({notification.actor_link})'
),
'description': '{notification.description}',
'message_template': 'openwisp_notifications/default_message.md',
Dhanus3133 marked this conversation as resolved.
Show resolved Hide resolved
'email_notification': False,
'web_notification': True,
},
}

NOTIFICATION_CHOICES = [('default', 'Default Type')]
NOTIFICATION_CHOICES = [
('default', 'Default Type'),
('generic_message', 'Generic Message Type'),
]
NOTIFICATION_ASSOCIATED_MODELS = set()


Expand Down
Loading