You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@allura.apache.org by br...@apache.org on 2019/11/07 15:19:14 UTC
[allura] 02/02: [#8339] allow multiple site notifications to be
active at once
This is an automated email from the ASF dual-hosted git repository.
brondsem pushed a commit to branch db/8339
in repository https://gitbox.apache.org/repos/asf/allura.git
commit f4b22d1cbf5d40e82eb38f49f166a0ba804ef44e
Author: Dave Brondsema <da...@brondsema.net>
AuthorDate: Thu Nov 7 10:18:45 2019 -0500
[#8339] allow multiple site notifications to be active at once
---
Allura/allura/lib/plugin.py | 70 ++++++---
Allura/allura/model/notification.py | 5 +-
Allura/allura/public/nf/js/allura-base.js | 10 +-
.../site_admin_site_notifications_list.html | 4 +
Allura/allura/tests/functional/test_site_admin.py | 3 +
Allura/allura/tests/model/test_notification.py | 4 +-
Allura/allura/tests/test_plugin.py | 166 ++++++++++++++-------
Allura/docs/getting_started/administration.rst | 4 +-
8 files changed, 173 insertions(+), 93 deletions(-)
diff --git a/Allura/allura/lib/plugin.py b/Allura/allura/lib/plugin.py
index 00d6128..c95df68 100644
--- a/Allura/allura/lib/plugin.py
+++ b/Allura/allura/lib/plugin.py
@@ -1480,33 +1480,55 @@ class ThemeProvider(object):
def _get_site_notification(self, url='', user=None, tool_name='', site_notification_cookie_value=''):
from allura.model.notification import SiteNotification
- note = SiteNotification.current()
- if note is None:
- return None
- if note.user_role and (not user or user.is_anonymous()):
- return None
- if note.user_role:
- projects = user.my_projects_by_role_name(note.user_role)
- if len(projects) == 0 or len(projects) == 1 and projects[0].is_user_project:
- return None
+ existing_cookie_info = {}
+ try:
+ for existing_cookie_chunk in site_notification_cookie_value.split('_'):
+ note_id, views, closed = existing_cookie_chunk.split('-')
+ views = asint(views)
+ closed = asbool(closed)
+ existing_cookie_info[note_id] = (views, closed)
+ except ValueError:
+ # ignore any weird cookie data
+ pass
+
+ set_cookie_chunks = []
+ notes = SiteNotification.actives()
+ note_to_show = None
+ for note in notes:
+ if note.user_role and (not user or user.is_anonymous()):
+ continue
+ if note.user_role:
+ projects = user.my_projects_by_role_name(note.user_role)
+ if len(projects) == 0 or len(projects) == 1 and projects[0].is_user_project:
+ continue
- if note.page_regex and re.search(note.page_regex, url) is None:
- return None
- if note.page_tool_type and tool_name.lower() != note.page_tool_type.lower():
- return None
+ if note.page_regex and re.search(note.page_regex, url) is None:
+ continue
+ if note.page_tool_type and tool_name.lower() != note.page_tool_type.lower():
+ continue
- cookie = site_notification_cookie_value.split('-')
- if len(cookie) == 3 and cookie[0] == str(note._id):
- views = asint(cookie[1]) + 1
- closed = asbool(cookie[2])
- else:
- views = 1
- closed = False
- if closed or note.impressions > 0 and views > note.impressions:
- return None
+ views_closed = existing_cookie_info.get(str(note._id))
+ if views_closed:
+ views, closed = views_closed
+ else:
+ views = 0
+ closed = False
+
+ if closed or note.impressions > 0 and views > note.impressions:
+ # preserve info about this in the cookie (so it won't be forgotten, and thus be displayed again)
+ set_cookie_chunks.append('-'.join(map(str, [note._id, views, closed])))
+ # but stop this loop iteration since the notification shouldn't be shown again
+ continue
+
+ # this notification is ok to show, so if there's not one picked already, this is the one.
+ if not note_to_show:
+ views += 1
+ note_to_show = note
+
+ set_cookie_chunks.append('-'.join(map(str, [note._id, views, closed])))
- set_cookie = '-'.join(map(str, [note._id, views, closed]))
- return note, set_cookie
+ if note_to_show:
+ return note_to_show, '_'.join(set_cookie_chunks)
def get_site_notification(self):
from tg import request, response
diff --git a/Allura/allura/model/notification.py b/Allura/allura/model/notification.py
index 080ef05..410a71c 100644
--- a/Allura/allura/model/notification.py
+++ b/Allura/allura/model/notification.py
@@ -735,6 +735,5 @@ class SiteNotification(MappedClass):
)
@classmethod
- def current(cls):
- note = cls.query.find({'active': True}).sort('_id', -1).limit(1).first()
- return note
\ No newline at end of file
+ def actives(cls):
+ return cls.query.find({'active': True}).sort('_id', -1)
diff --git a/Allura/allura/public/nf/js/allura-base.js b/Allura/allura/public/nf/js/allura-base.js
index 21857b4..66600cd 100644
--- a/Allura/allura/public/nf/js/allura-base.js
+++ b/Allura/allura/public/nf/js/allura-base.js
@@ -208,13 +208,15 @@ $(function(){
}
});
- var SN_ID=0, SN_VIEWS=1, SN_CLOSED=2;
$('#site-notification .btn-close').click(function(e) {
var $note = $(this).parent();
$note.hide();
- var status = $.cookie('site-notification').split('-');
- status[SN_CLOSED] = 'true';
- $.cookie('site-notification', status.join('-'), {
+ var note_id = $note.attr('data-notification-id');
+ var cookie = $.cookie('site-notification');
+ // change e.g. "5dc2f69f07ae3175c7c21972-5-False" to "5dc2f69f07ae3175c7c21972-5-True" to mark as closed
+ // cookie may have multiple id-num-bool sets in it
+ cookie = cookie.replace(new RegExp(note_id + '-([0-9]+)-False'), note_id + '-$1-True');
+ $.cookie('site-notification', cookie, {
expires: 365,
path: '/'
});
diff --git a/Allura/allura/templates/site_admin_site_notifications_list.html b/Allura/allura/templates/site_admin_site_notifications_list.html
index e795c94..b289c35 100644
--- a/Allura/allura/templates/site_admin_site_notifications_list.html
+++ b/Allura/allura/templates/site_admin_site_notifications_list.html
@@ -23,6 +23,10 @@
{% block content %}
<div class="grid-23"><a href="/nf/admin">Back to Site Admin Home</a></div>
<div class="grid-23"><a href="new">Create a new notification</a></div>
+ <div class="grid-23">
+ If there are multiple active notifications whose criteria match for a visitor, the most recent one will be used. After that notification has been closed explicitly or automatically reached its limit, the next one can appear.
+ <br><br>
+ </div>
{{c.page_size.display(limit=limit, page=page_url, count=count)}}
<table id="site_notifications">
<thead>
diff --git a/Allura/allura/tests/functional/test_site_admin.py b/Allura/allura/tests/functional/test_site_admin.py
index 4d691f3..d7eebc6 100644
--- a/Allura/allura/tests/functional/test_site_admin.py
+++ b/Allura/allura/tests/functional/test_site_admin.py
@@ -170,6 +170,9 @@ class TestSiteAdmin(TestController):
task_name='allura.tests.functional.test_site_admin.test_task'))
assert json.loads(r.body)['doc'] == 'test_task doc string'
+
+class TestSiteAdminNotifications(TestController):
+
def test_site_notifications_access(self):
self.app.get('/nf/admin/site_notifications', extra_environ=dict(
username='test_user'), status=403)
diff --git a/Allura/allura/tests/model/test_notification.py b/Allura/allura/tests/model/test_notification.py
index 57eefd2..d154bd7 100644
--- a/Allura/allura/tests/model/test_notification.py
+++ b/Allura/allura/tests/model/test_notification.py
@@ -507,8 +507,8 @@ class TestSiteNotification(unittest.TestCase):
def _clear_subscriptions():
- M.Mailbox.query.remove({})
+ M.Mailbox.query.remove({})
def _clear_notifications():
- M.Notification.query.remove({})
+ M.Notification.query.remove({})
diff --git a/Allura/allura/tests/test_plugin.py b/Allura/allura/tests/test_plugin.py
index 6c582b1..7d889d0 100644
--- a/Allura/allura/tests/test_plugin.py
+++ b/Allura/allura/tests/test_plugin.py
@@ -251,12 +251,44 @@ class TestProjectRegistrationProviderPhoneVerification(object):
class TestThemeProvider(object):
+ @patch('allura.app.g')
+ @patch('allura.lib.plugin.g')
+ def test_app_icon_str(self, plugin_g, app_g):
+ class TestApp(Application):
+ icons = {
+ 24: 'images/testapp_24.png',
+ }
+ plugin_g.entry_points = {'tool': {'testapp': TestApp}}
+ assert_equals(ThemeProvider().app_icon_url('testapp', 24),
+ app_g.theme_href.return_value)
+ app_g.theme_href.assert_called_with('images/testapp_24.png')
+
+ @patch('allura.lib.plugin.g')
+ def test_app_icon_str_invalid(self, g):
+ g.entry_points = {'tool': {'testapp': Mock()}}
+ assert_equals(ThemeProvider().app_icon_url('invalid', 24),
+ None)
+
+ @patch('allura.app.g')
+ def test_app_icon_app(self, g):
+ class TestApp(Application):
+ icons = {
+ 24: 'images/testapp_24.png',
+ }
+ app = TestApp(None, None)
+ assert_equals(ThemeProvider().app_icon_url(app, 24),
+ g.theme_href.return_value)
+ g.theme_href.assert_called_with('images/testapp_24.png')
+
+
+class TestThemeProvider_notifications(object):
+
@patch('allura.lib.plugin.c', MagicMock())
@patch('allura.model.notification.SiteNotification')
@patch('tg.response')
@patch('tg.request')
def test_get_site_notification_no_note(self, request, response, SiteNotification):
- SiteNotification.current.return_value = None
+ SiteNotification.actives.return_value = []
assert_is_none(ThemeProvider().get_site_notification())
assert not response.set_cookie.called
@@ -265,10 +297,12 @@ class TestThemeProvider(object):
@patch('tg.response')
@patch('tg.request')
def test_get_site_notification_closed(self, request, response, SiteNotification):
- SiteNotification.current.return_value._id = 'deadbeef'
- SiteNotification.current.return_value.user_role = None
- SiteNotification.current.return_value.page_regex = None
- SiteNotification.current.return_value.page_tool_type = None
+ note = MagicMock()
+ note._id = 'deadbeef'
+ note.user_role = None
+ note.page_regex = None
+ note.page_tool_type = None
+ SiteNotification.actives.return_value = [note]
request.cookies = {'site-notification': 'deadbeef-1-true'}
assert_is_none(ThemeProvider().get_site_notification())
assert not response.set_cookie.called
@@ -278,12 +312,13 @@ class TestThemeProvider(object):
@patch('tg.response')
@patch('tg.request')
def test_get_site_notification_impressions_over(self, request, response, SiteNotification):
- note = SiteNotification.current.return_value
+ note = MagicMock()
note._id = 'deadbeef'
note.impressions = 2
note.user_role = None
note.page_regex = None
note.page_tool_type = None
+ SiteNotification.actives.return_value = [note]
request.cookies = {'site-notification': 'deadbeef-3-false'}
assert_is_none(ThemeProvider().get_site_notification())
assert not response.set_cookie.called
@@ -293,12 +328,13 @@ class TestThemeProvider(object):
@patch('tg.response')
@patch('tg.request')
def test_get_site_notification_impressions_under(self, request, response, SiteNotification):
- note = SiteNotification.current.return_value
+ note = MagicMock()
note._id = 'deadbeef'
note.impressions = 2
note.user_role = None
note.page_regex = None
note.page_tool_type = None
+ SiteNotification.actives.return_value = [note]
request.cookies = {'site-notification': 'deadbeef-1-false'}
assert_is(ThemeProvider().get_site_notification(), note)
response.set_cookie.assert_called_once_with(
@@ -309,12 +345,13 @@ class TestThemeProvider(object):
@patch('tg.response')
@patch('tg.request')
def test_get_site_notification_impressions_persistent(self, request, response, SiteNotification):
- note = SiteNotification.current.return_value
+ note = MagicMock()
note._id = 'deadbeef'
note.impressions = 0
note.user_role = None
note.page_regex = None
note.page_tool_type = None
+ SiteNotification.actives.return_value = [note]
request.cookies = {'site-notification': 'deadbeef-1000-false'}
assert_is(ThemeProvider().get_site_notification(), note)
@@ -323,12 +360,13 @@ class TestThemeProvider(object):
@patch('tg.response')
@patch('tg.request')
def test_get_site_notification_new_notification(self, request, response, SiteNotification):
- note = SiteNotification.current.return_value
+ note = MagicMock()
note._id = 'deadbeef'
note.impressions = 1
note.user_role = None
note.page_regex = None
note.page_tool_type = None
+ SiteNotification.actives.return_value = [note]
request.cookies = {'site-notification': '0ddba11-1000-true'}
assert_is(ThemeProvider().get_site_notification(), note)
response.set_cookie.assert_called_once_with(
@@ -339,12 +377,13 @@ class TestThemeProvider(object):
@patch('tg.response')
@patch('tg.request')
def test_get_site_notification_no_cookie(self, request, response, SiteNotification):
- note = SiteNotification.current.return_value
+ note = MagicMock()
note._id = 'deadbeef'
note.impressions = 0
note.user_role = None
note.page_regex = None
note.page_tool_type = None
+ SiteNotification.actives.return_value = [note]
request.cookies = {}
assert_is(ThemeProvider().get_site_notification(), note)
response.set_cookie.assert_called_once_with(
@@ -355,55 +394,28 @@ class TestThemeProvider(object):
@patch('tg.response')
@patch('tg.request')
def test_get_site_notification_bad_cookie(self, request, response, SiteNotification):
- note = SiteNotification.current.return_value
+ note = MagicMock()
note._id = 'deadbeef'
note.impressions = 0
note.user_role = None
note.page_regex = None
note.page_tool_type = None
+ SiteNotification.actives.return_value = [note]
request.cookies = {'site-notification': 'deadbeef-1000-true-bad'}
assert_is(ThemeProvider().get_site_notification(), note)
response.set_cookie.assert_called_once_with(
'site-notification', 'deadbeef-1-False', max_age=dt.timedelta(days=365))
- @patch('allura.app.g')
- @patch('allura.lib.plugin.g')
- def test_app_icon_str(self, plugin_g, app_g):
- class TestApp(Application):
- icons = {
- 24: 'images/testapp_24.png',
- }
- plugin_g.entry_points = {'tool': {'testapp': TestApp}}
- assert_equals(ThemeProvider().app_icon_url('testapp', 24),
- app_g.theme_href.return_value)
- app_g.theme_href.assert_called_with('images/testapp_24.png')
-
- @patch('allura.lib.plugin.g')
- def test_app_icon_str_invalid(self, g):
- g.entry_points = {'tool': {'testapp': Mock()}}
- assert_equals(ThemeProvider().app_icon_url('invalid', 24),
- None)
-
- @patch('allura.app.g')
- def test_app_icon_app(self, g):
- class TestApp(Application):
- icons = {
- 24: 'images/testapp_24.png',
- }
- app = TestApp(None, None)
- assert_equals(ThemeProvider().app_icon_url(app, 24),
- g.theme_href.return_value)
- g.theme_href.assert_called_with('images/testapp_24.png')
-
@patch('allura.lib.plugin.c')
@patch('allura.model.notification.SiteNotification')
@patch('tg.response', MagicMock())
@patch('tg.request', MagicMock())
def test_get_site_notification_with_role(self, SiteNotification, c):
- note = SiteNotification.current.return_value
+ note = MagicMock()
note.user_role = 'Test'
note.page_regex = None
note.page_tool_type = None
+ SiteNotification.actives.return_value = [note]
projects = c.user.my_projects_by_role_name
c.user.is_anonymous.return_value = True
@@ -428,10 +440,11 @@ class TestThemeProvider(object):
@patch('tg.response', MagicMock())
@patch('tg.request', MagicMock())
def test_get_site_notification_without_role(self, SiteNotification):
- note = SiteNotification.current.return_value
+ note = MagicMock()
note.user_role = None
note.page_regex = None
note.page_tool_type = None
+ SiteNotification.actives.return_value = [note]
assert_is(ThemeProvider().get_site_notification(), note)
@patch('allura.lib.plugin.c', MagicMock())
@@ -440,10 +453,11 @@ class TestThemeProvider(object):
@patch('tg.response', MagicMock())
@patch('tg.request', MagicMock())
def test_get_site_notification_with_page_regex(self, SiteNotification, search):
- note = SiteNotification.current.return_value
+ note = MagicMock()
note.user_role = None
note.page_regex = 'test'
note.page_tool_type = None
+ SiteNotification.actives.return_value = [note]
search.return_value = True
assert_is(ThemeProvider().get_site_notification(), note)
@@ -456,12 +470,12 @@ class TestThemeProvider(object):
@patch('tg.response', MagicMock())
@patch('tg.request', MagicMock())
def test_get_site_notification_with_page_tool_type(self, SiteNotification, c):
- note = SiteNotification.current.return_value
+ note = MagicMock()
note.user_role = None
note.page_regex = None
- c.app = Mock()
note.page_tool_type.lower.return_value = 'test1'
-
+ SiteNotification.actives.return_value = [note]
+ c.app = Mock()
c.app.config.tool_name.lower.return_value = 'test1'
assert_is(ThemeProvider().get_site_notification(), note)
@@ -476,11 +490,12 @@ class TestThemeProvider(object):
@patch('allura.model.notification.SiteNotification')
@patch('tg.response', MagicMock())
def test_get_site_notification_with_page_tool_type_page_regex(self, SiteNotification, request, c):
- note = SiteNotification.current.return_value
+ note = MagicMock()
note.user_role = None
note.page_regex = 'test'
- c.app = Mock()
note.page_tool_type.lower.return_value = 'test1'
+ SiteNotification.actives.return_value = [note]
+ c.app = Mock()
request.path_qs = 'ttt'
c.app.config.tool_name.lower.return_value = 'test2'
@@ -504,33 +519,68 @@ class TestThemeProvider(object):
@patch('allura.model.notification.SiteNotification')
def test_get__site_notification(self, SiteNotification):
- note = SiteNotification.current.return_value
- note._id = 'test_id'
+ note = MagicMock()
+ note._id = 'testid'
note.user_role = None
note.page_regex = None
note.page_tool_type = None
+ SiteNotification.actives.return_value = [note]
get_note = ThemeProvider()._get_site_notification()
assert isinstance(get_note, tuple)
assert len(get_note) == 2
assert get_note[0] is note
- assert get_note[1] == 'test_id-1-False'
+ assert get_note[1] == 'testid-1-False'
@patch('allura.model.notification.SiteNotification')
- @patch('tg.request')
- def test_get_site_notifications_with_api_cookie(self, request, SiteNotification):
- note = SiteNotification.current.return_value
- note._id = 'test_id'
+ def test_get__site_notification_multiple(self, SiteNotification):
+ note1 = MagicMock()
+ note1._id = 'test1'
+ note1.user_role = None
+ note1.page_regex = 'this-will-not-match'
+ note1.page_tool_type = None
+ note2 = MagicMock()
+ note2._id = 'test2'
+ note2.user_role = None
+ note2.page_regex = None
+ note2.page_tool_type = None
+ note3 = MagicMock()
+ note3._id = 'test3'
+ note3.user_role = None
+ note3.page_regex = None
+ note3.page_tool_type = None
+ SiteNotification.actives.return_value = [note1, note2, note3]
+ get_note = ThemeProvider()._get_site_notification()
+
+ assert isinstance(get_note, tuple)
+ assert len(get_note) == 2
+ assert get_note[0] is note2
+ assert_equal(get_note[1], 'test2-1-False_test3-0-False')
+
+ # and with a cookie set
+ get_note = ThemeProvider()._get_site_notification(
+ site_notification_cookie_value='test2-3-True_test3-0-False'
+ )
+
+ assert isinstance(get_note, tuple)
+ assert len(get_note) == 2
+ assert get_note[0] is note3
+ assert_equal(get_note[1], 'test2-3-True_test3-1-False')
+
+ @patch('allura.model.notification.SiteNotification')
+ def test_get_site_notifications_with_api_cookie(self, SiteNotification):
+ note = MagicMock()
+ note._id = 'testid'
note.user_role = None
note.page_regex = None
note.page_tool_type = None
- request.cookies = {}
+ SiteNotification.actives.return_value = [note]
get_note = ThemeProvider()._get_site_notification(
- site_notification_cookie_value='test_id-1-False'
+ site_notification_cookie_value='testid-1-False'
)
assert get_note[0] is note
- assert get_note[1] == 'test_id-2-False'
+ assert get_note[1] == 'testid-2-False'
class TestLocalAuthenticationProvider(object):
diff --git a/Allura/docs/getting_started/administration.rst b/Allura/docs/getting_started/administration.rst
index 6c35f6f..76469b7 100644
--- a/Allura/docs/getting_started/administration.rst
+++ b/Allura/docs/getting_started/administration.rst
@@ -333,8 +333,8 @@ which have role 'Developer' or higher in one of their projects. And if url of
the current page is matching regex :code:`(Home|browse_pages)` and app
tool type is :code:`wiki`. An "Impressions" value of 0 will show the
notification indefinitely (until closed). The notification content can contain
-HTML. Only the most recent active notification will be shown.
-"User Role", "Page Regex" and "Page Type" are optional.
+HTML. The most recent notification that is active and matches for the visitor
+will be shown. "User Role", "Page Regex" and "Page Type" are optional.
.. _delete-projects: