This is an automated email from the ASF dual-hosted git repository. brondsem pushed a commit to branch master in repository https://gitbox.apache.org/repos/asf/allura.git
commit 74a15dc526003515c6611cbdc1a4a551a5838156 Author: Guillermo Cruz <[email protected]> AuthorDate: Wed Jun 29 13:40:34 2022 -0600 [#8444] added canonical tag and rel=next/prev where pagination is used to wiki, discussions and tickets --- Allura/allura/lib/helpers.py | 26 ++++++++++++++++++++++ Allura/allura/templates/jinja_master/lib.html | 26 ++++++++++++++++++++++ Allura/allura/tests/test_helpers.py | 13 +++++++++++ ForgeBlog/forgeblog/templates/blog/index.html | 4 +++- ForgeBlog/forgeblog/templates/blog/search.html | 4 +++- .../templates/discussionforums/search.html | 4 +++- .../templates/discussionforums/thread.html | 10 +++++---- .../forgediscussion/templates/index.html | 4 +++- .../forgetracker/templates/tracker/index.html | 3 +++ .../forgetracker/templates/tracker/milestone.html | 6 ++++- .../forgetracker/templates/tracker/search.html | 3 +++ .../forgetracker/tests/functional/test_root.py | 26 ++++++++++++++++++++++ ForgeWiki/forgewiki/templates/wiki/browse.html | 7 +++++- .../forgewiki/templates/wiki/browse_tags.html | 7 +++++- ForgeWiki/forgewiki/templates/wiki/master.html | 7 ++++++ .../forgewiki/templates/wiki/page_history.html | 4 +++- ForgeWiki/forgewiki/templates/wiki/page_view.html | 7 +++--- ForgeWiki/forgewiki/templates/wiki/search.html | 2 ++ ForgeWiki/forgewiki/tests/functional/test_root.py | 18 ++++++++++++--- 19 files changed, 163 insertions(+), 18 deletions(-) diff --git a/Allura/allura/lib/helpers.py b/Allura/allura/lib/helpers.py index bf7b4d04a..5ecb7862f 100644 --- a/Allura/allura/lib/helpers.py +++ b/Allura/allura/lib/helpers.py @@ -65,6 +65,10 @@ from webob.exc import HTTPUnauthorized from allura.lib import exceptions as exc from allura.lib import utils +import urllib.parse as urlparse +from urllib.parse import urlencode +import math +from webob.multidict import MultiDict # import to make available to templates, don't delete: from .security import has_access, is_allowed_by_role, is_site_admin @@ -152,6 +156,28 @@ def make_safe_path_portion(ustr, relaxed=True): def escape_json(data): return json.dumps(data).replace('<', '\\u003C') +def querystring(request, url_params): + """ + add/update/remove url parameters. When a value is set to None the key will + be removed from the final constructed url. + + :param request: request object + :param url_params: dict with the params that should be updated/added/deleted. + :return: a full url with updated url parameters. + """ + params = urlparse.parse_qs(request.query_string) + params.update(url_params) + for param in list(params.keys()): + if params[param] is None: + del params[param] + # flatten dict values + params = {k: v[0] if isinstance(v, list) else v for k, v in params.items()} + url_parts = urlparse.urlparse(request.url) + url = url_parts._replace(query=urlencode(params)).geturl() + return url + +def ceil(number): + return math.ceil(number) def strip_bad_unicode(s): """ diff --git a/Allura/allura/templates/jinja_master/lib.html b/Allura/allura/templates/jinja_master/lib.html index 0af3f490e..b6177a16a 100644 --- a/Allura/allura/templates/jinja_master/lib.html +++ b/Allura/allura/templates/jinja_master/lib.html @@ -886,3 +886,29 @@ This page is based on some examples from Greg Schueler, <a href="mailto:greg@var {%- do g.register_forge_js('js/create-react-class.min.js', location=location) %} {%- do g.register_forge_js('js/prop-types.min.js', location=location) %} {%- endmacro %} + +{% macro canonical_tag(page=None) %} + {# in case is inherithed from a child template and has no access to a page value #} + {% set page = request.GET['page'] if not page and 'page=' in request.query_string else page %} + {% if page == '0' %} + <link rel="canonical" href="{{ h.querystring(request, dict(page=None,limit=None)) }}"/> + {% else %} + <link rel="canonical" href="{{ h.querystring(request, dict(limit=None)) }}"/> + + {% endif %} +{% endmacro %} + +{% macro pagination_meta_tags(request, current_page=None, results_count=None, limit=None) %} + {%- if current_page > 0 -%} + {% if current_page == 1 %} + <link rel="prev" href="{{ h.querystring(request, dict(page=None,limit=None)) }}"/> + {% else %} + <link rel="prev" href="{{ h.querystring(request, dict(page=current_page - 1,limit=None)) }}"/> + {% endif %} + {% endif %} + {% set current_page = current_page + 1 %} + {% set total_pages = h.ceil(results_count/limit) %} + {% if results_count and current_page < total_pages -%} + <link rel="next" href="{{ h.querystring(request, dict(page=current_page,limit=None)) }}"/> + {% endif %} +{% endmacro %} \ No newline at end of file diff --git a/Allura/allura/tests/test_helpers.py b/Allura/allura/tests/test_helpers.py index 3c1428586..650ba023f 100644 --- a/Allura/allura/tests/test_helpers.py +++ b/Allura/allura/tests/test_helpers.py @@ -472,6 +472,7 @@ class TestUrlOpen(TestCase): def side_effect(url, timeout=None): raise socket.timeout() + urlopen.side_effect = side_effect self.assertRaises(socket.timeout, h.urlopen, 'myurl') self.assertEqual(urlopen.call_count, 4) @@ -483,6 +484,7 @@ class TestUrlOpen(TestCase): def side_effect(url, timeout=None): raise OSError(errno.ECONNRESET, 'Connection reset by peer') + urlopen.side_effect = side_effect self.assertRaises(socket.error, h.urlopen, 'myurl') self.assertEqual(urlopen.call_count, 4) @@ -493,6 +495,7 @@ class TestUrlOpen(TestCase): def side_effect(url, timeout=None): raise HTTPError('url', 408, 'timeout', None, None) + urlopen.side_effect = side_effect self.assertRaises(HTTPError, h.urlopen, 'myurl') self.assertEqual(urlopen.call_count, 4) @@ -503,6 +506,7 @@ class TestUrlOpen(TestCase): def side_effect(url, timeout=None): raise HTTPError('url', 404, 'timeout', None, None) + urlopen.side_effect = side_effect self.assertRaises(HTTPError, h.urlopen, 'myurl') self.assertEqual(urlopen.call_count, 1) @@ -681,3 +685,12 @@ def test_hide_private_info(): def test_emojize(): assert_equals(h.emojize(':smile:'), '😄') + + +def test_querystring(): + req = Request.blank('/p/test/foobar?page=1&limit=10&count=100', remote_addr='127.0.0.1', + base_url='https://mysite.com/p/test/foobar') + assert_equals(h.querystring(req, dict(page=2, limit=5)), + 'https://mysite.com/p/test/foobar/p/test/foobar?page=2&limit=5&count=100') + assert_equals(h.querystring(req, dict(page=5, limit=2, count=None)), + 'https://mysite.com/p/test/foobar/p/test/foobar?page=5&limit=2') diff --git a/ForgeBlog/forgeblog/templates/blog/index.html b/ForgeBlog/forgeblog/templates/blog/index.html index b9e82d71c..c88a3aa19 100644 --- a/ForgeBlog/forgeblog/templates/blog/index.html +++ b/ForgeBlog/forgeblog/templates/blog/index.html @@ -17,13 +17,15 @@ under the License. -#} {% extends g.theme.master %} - +{% import 'allura:templates/jinja_master/lib.html' as lib with context %} {% block title %}{{c.project.name}} / {{c.app.config.options.mount_label}}{% endblock %} {% block head %} {% if count == 0 %} <meta name="robots" content="noindex, follow"/> {% endif %} + {{ lib.canonical_tag(page) }} + {{ lib.pagination_meta_tags(request, page, count, limit) }} {% endblock %} {% block header %}{{c.project.name}} / {{c.app.config.options.mount_label}}: Recent posts{% endblock %} diff --git a/ForgeBlog/forgeblog/templates/blog/search.html b/ForgeBlog/forgeblog/templates/blog/search.html index a81e74bb0..b7b84eb63 100644 --- a/ForgeBlog/forgeblog/templates/blog/search.html +++ b/ForgeBlog/forgeblog/templates/blog/search.html @@ -17,11 +17,13 @@ under the License. -#} {% extends g.theme.master %} - +{% import 'allura:templates/jinja_master/lib.html' as lib with context %} {% block title %}{{c.project.name}} / {{c.app.config.options.mount_label}} / Search{% endblock %} {% block head %} <meta name="robots" content="noindex, follow"> + {{ lib.canonical_tag(page) }} + {{ lib.pagination_meta_tags(request, page, count, limit) }} {% endblock %} {% block header %}Search {{c.app.config.options.mount_point}}: {{q}}{% endblock %} diff --git a/ForgeDiscussion/forgediscussion/templates/discussionforums/search.html b/ForgeDiscussion/forgediscussion/templates/discussionforums/search.html index 85de03713..4ea1e5f37 100644 --- a/ForgeDiscussion/forgediscussion/templates/discussionforums/search.html +++ b/ForgeDiscussion/forgediscussion/templates/discussionforums/search.html @@ -17,11 +17,13 @@ under the License. -#} {% extends g.theme.master %} - +{% import 'allura:templates/jinja_master/lib.html' as lib with context %} {% block title %}{{c.project.name}} / {{c.app.config.options.mount_label}} / Search{% endblock %} {% block head %} <meta name="robots" content="noindex, follow"> + {{ lib.canonical_tag(page) }} + {{ lib.pagination_meta_tags(request, page, count, limit) }} {% endblock %} {% block header %}Search {{c.app.config.options.mount_point}}: {{q}}{% endblock %} diff --git a/ForgeDiscussion/forgediscussion/templates/discussionforums/thread.html b/ForgeDiscussion/forgediscussion/templates/discussionforums/thread.html index cbf05173d..ab8d3f4c9 100644 --- a/ForgeDiscussion/forgediscussion/templates/discussionforums/thread.html +++ b/ForgeDiscussion/forgediscussion/templates/discussionforums/thread.html @@ -17,14 +17,16 @@ under the License. -#} {% extends g.theme.master %} - +{% import 'allura:templates/jinja_master/lib.html' as lib with context %} {% block title %} - {{c.project.name}} / {{c.app.config.options.mount_label}} / + {{c.project.name}} / {{c.app.config.options.mount_label}} / {{thread.subject and '%s: %s' % (thread.discussion.name, (thread.subject or '(no subject)')) or thread.discussion.name}} {% endblock %} - +{% block head %} + {{ lib.canonical_tag(page) }} + {{ lib.pagination_meta_tags(request, page, count, limit) }} +{% endblock %} {% block header %}{{'subject' in thread and thread.subject or '(no subject)'}}{% endblock %} - {% block actions %} {% if show_moderate and h.has_access(thread, 'moderate')() %} {{ g.icons['edit'].render(id='mod_thread_link') }} diff --git a/ForgeDiscussion/forgediscussion/templates/index.html b/ForgeDiscussion/forgediscussion/templates/index.html index 57af42022..b3a74b57b 100644 --- a/ForgeDiscussion/forgediscussion/templates/index.html +++ b/ForgeDiscussion/forgediscussion/templates/index.html @@ -17,12 +17,14 @@ under the License. -#} {% extends 'allura:templates/discussion/index.html' %} - +{% import 'allura:templates/jinja_master/lib.html' as lib with context %} {% block head %} {{ super() }} {% if not threads|length %} <meta name="robots" content="noindex, follow"> {% endif %} + {{ lib.canonical_tag(page) }} + {{ lib.pagination_meta_tags(request, page, count, limit) }} {% endblock %} {% block actions %} diff --git a/ForgeTracker/forgetracker/templates/tracker/index.html b/ForgeTracker/forgetracker/templates/tracker/index.html index c4c3cb189..7e15113ef 100644 --- a/ForgeTracker/forgetracker/templates/tracker/index.html +++ b/ForgeTracker/forgetracker/templates/tracker/index.html @@ -25,8 +25,11 @@ {% block head %} <link rel="alternate" type="application/rss+xml" title="RSS" href="feed.rss"/> <link rel="alternate" type="application/atom+xml" title="Atom" href="feed.atom"/> + {{ lib.canonical_tag(page) }} + {{ lib.pagination_meta_tags(request, page, count, limit) }} {% endblock %} + {% block header %}{{c.app.config.options.mount_label}}{% endblock %} {% block actions %} diff --git a/ForgeTracker/forgetracker/templates/tracker/milestone.html b/ForgeTracker/forgetracker/templates/tracker/milestone.html index bb695f122..0bdca6b61 100644 --- a/ForgeTracker/forgetracker/templates/tracker/milestone.html +++ b/ForgeTracker/forgetracker/templates/tracker/milestone.html @@ -18,13 +18,17 @@ -#} {% extends g.theme.master %} {% import 'allura:templates/jinja_master/lib.html' as lib with context %} - {% do g.register_app_css('css/tracker.css') %} {% block title %}{{c.project.name}} / {{c.app.config.options.mount_label}} / {{field.label}} {{milestone.name}}{% endblock %} {% block header %}{{field.label}} {{milestone.name}}{% endblock %} +{% block head %} + {{ lib.canonical_tag(page) }} + {{ lib.pagination_meta_tags(request, page, count, limit) }} +{% endblock %} + {% block actions %} {{ lib.maximize_content_button() }} {% if allow_edit %} diff --git a/ForgeTracker/forgetracker/templates/tracker/search.html b/ForgeTracker/forgetracker/templates/tracker/search.html index 03c86c914..8d43892e9 100644 --- a/ForgeTracker/forgetracker/templates/tracker/search.html +++ b/ForgeTracker/forgetracker/templates/tracker/search.html @@ -30,6 +30,9 @@ {% endif %} <link rel="alternate" type="application/rss+xml" title="RSS" href="feed.rss"/> <link rel="alternate" type="application/atom+xml" title="Atom" href="feed.atom"/> + {{ lib.canonical_tag(page) }} + {{ lib.pagination_meta_tags(request, page, count, limit) }} + {% endblock %} {% block actions %} diff --git a/ForgeTracker/forgetracker/tests/functional/test_root.py b/ForgeTracker/forgetracker/tests/functional/test_root.py index a5c8b23b3..0d98c0a9b 100644 --- a/ForgeTracker/forgetracker/tests/functional/test_root.py +++ b/ForgeTracker/forgetracker/tests/functional/test_root.py @@ -1380,6 +1380,7 @@ class TestFunctionalController(TrackerTestController): M.MonQTask.run_ready() ThreadLocalORMSession.flush_all() response = self.app.get('/p/test/bugs/search/?q=test&limit=2') + response.mustcontain('canonical') response.mustcontain('results of 3') response.mustcontain('test second ticket') next_page_link = response.html.select('.page_list a')[0] @@ -1390,6 +1391,31 @@ class TestFunctionalController(TrackerTestController): # 'filter' is special kwarg, don't let it cause problems r = self.app.get('/p/test/bugs/search/?q=test&filter=blah') + def test_search_canonical(self): + self.new_ticket(summary='test first ticket') + self.new_ticket(summary='test second ticket') + self.new_ticket(summary='test third ticket') + self.new_ticket(summary='test fourth ticket') + self.new_ticket(summary='test fifth ticket') + self.new_ticket(summary='test sixth ticket') + self.new_ticket(summary='test seventh ticket') + self.new_ticket(summary='test eighth ticket') + ThreadLocalORMSession.flush_all() + M.MonQTask.run_ready() + ThreadLocalORMSession.flush_all() + response = self.app.get('/p/test/bugs/search/?q=test&limit=1') + canonical = response.html.select_one('link[rel=canonical]') + assert 'limit=2' not in canonical['href'] + response = self.app.get('/p/test/bugs/search/?q=test&limit=2&page=2') + next = response.html.select_one('link[rel=next]') + assert 'page=3' in next['href'] + prev = response.html.select_one('link[rel=prev]') + assert 'page=1' in prev['href'] + response = self.app.get('/p/test/bugs/search/?q=test&limit=2&page=0') + canonical = response.html.select_one('link[rel=canonical]') + assert 'page=' not in canonical + + def test_search_with_strange_chars(self): r = self.app.get('/p/test/bugs/search/?' + urlencode({'q': 'tést'})) diff --git a/ForgeWiki/forgewiki/templates/wiki/browse.html b/ForgeWiki/forgewiki/templates/wiki/browse.html index dcc49059f..3a96aa719 100644 --- a/ForgeWiki/forgewiki/templates/wiki/browse.html +++ b/ForgeWiki/forgewiki/templates/wiki/browse.html @@ -18,9 +18,14 @@ -#} {% extends 'forgewiki:templates/wiki/master.html' %} {% from 'allura:templates/jinja_master/lib.html' import abbr_date with context %} - +{% import 'allura:templates/jinja_master/lib.html' as lib with context %} {% block title %}{{c.project.name}} / {{c.app.config.options.mount_label}} / Browse Pages{% endblock %} +{% block head %} + {{ super() }} + {{ lib.pagination_meta_tags(request, page, count, limit) }} +{% endblock %} + {% block header %}Browse Pages{% endblock %} {% block wiki_content %} diff --git a/ForgeWiki/forgewiki/templates/wiki/browse_tags.html b/ForgeWiki/forgewiki/templates/wiki/browse_tags.html index e464a39ba..7225526b6 100644 --- a/ForgeWiki/forgewiki/templates/wiki/browse_tags.html +++ b/ForgeWiki/forgewiki/templates/wiki/browse_tags.html @@ -17,9 +17,14 @@ under the License. -#} {% extends 'forgewiki:templates/wiki/master.html' %} - +{% import 'allura:templates/jinja_master/lib.html' as lib with context %} {% block title %}{{c.project.name}} / {{c.app.config.options.mount_label}} / Browse Labels{% endblock %} +{% block head %} + {{ super() }} + {{ lib.pagination_meta_tags(request, page, count, limit) }} +{% endblock %} + {% block header %}Browse Labels{% endblock %} {% block wiki_content %} diff --git a/ForgeWiki/forgewiki/templates/wiki/master.html b/ForgeWiki/forgewiki/templates/wiki/master.html index e2f561ea8..37d237a59 100644 --- a/ForgeWiki/forgewiki/templates/wiki/master.html +++ b/ForgeWiki/forgewiki/templates/wiki/master.html @@ -17,7 +17,14 @@ under the License. -#} {% extends g.theme.master %} +{% import g.theme.jinja_macros as theme_macros with context %} {% do g.register_app_css('css/wiki.css', compress=False) %} +{% import 'allura:templates/jinja_master/lib.html' as lib with context %} + +{% block head %} + {{ lib.canonical_tag() }} +{% endblock %} + {% block edit_box %} {% if show_meta %}{% block wiki_meta %}{% endblock %}{% endif %} diff --git a/ForgeWiki/forgewiki/templates/wiki/page_history.html b/ForgeWiki/forgewiki/templates/wiki/page_history.html index 4bc43b11a..da255c5dd 100644 --- a/ForgeWiki/forgewiki/templates/wiki/page_history.html +++ b/ForgeWiki/forgewiki/templates/wiki/page_history.html @@ -19,11 +19,13 @@ {% extends 'forgewiki:templates/wiki/master.html' %} {% from 'allura:templates/jinja_master/lib.html' import abbr_date with context %} {% import 'allura:templates/jinja_master/dialog_macros.html' as dialog_macros with context %} - +{% import 'allura:templates/jinja_master/lib.html' as lib with context %} {% block title %}{{c.project.name}} / {{c.app.config.options.mount_label}} / {{title}}{% endblock %} {% block head %} + {{ super() }} <meta name="robots" content="noindex,follow" /> + {{ lib.pagination_meta_tags(request, page, count, limit) }} {% endblock %} {% block header %} diff --git a/ForgeWiki/forgewiki/templates/wiki/page_view.html b/ForgeWiki/forgewiki/templates/wiki/page_view.html index e26d9e149..07b1249b4 100644 --- a/ForgeWiki/forgewiki/templates/wiki/page_view.html +++ b/ForgeWiki/forgewiki/templates/wiki/page_view.html @@ -19,6 +19,7 @@ {% extends 'forgewiki:templates/wiki/master.html' %} {% do g.register_forge_css('css/forge/hilite.css', compress=False) %} {% import 'allura:templates/jinja_master/lib.html' as lib with context %} +{% import g.theme.jinja_macros as theme_macros with context %} {% set base_url = page.url().split('?')[0] %} @@ -34,14 +35,14 @@ {% endblock %} {% block head %} - {% if noindex %} + {{ super() }} + {%- if noindex -%} <meta name="robots" content="noindex, follow"> - {% endif %} + {%- endif -%} <link rel="alternate" type="application/rss+xml" title="Page RSS" href="feed.rss"/> <link rel="alternate" type="application/atom+xml" title="Page Atom" href="feed.atom"/> <link rel="alternate" type="application/rss+xml" title="Wiki RSS" href="../feed.rss"/> <link rel="alternate" type="application/atom+xml" title="Wiki Atom" href="../feed.atom"/> -<link rel="canonical" href="{{ h.absurl(base_url) }}"> {% endblock %} {% block body_css_class %} {{super()}} wiki-{{(page.title).replace(' ','_')}}{% endblock %} diff --git a/ForgeWiki/forgewiki/templates/wiki/search.html b/ForgeWiki/forgewiki/templates/wiki/search.html index c13f1171b..9faff6f2c 100644 --- a/ForgeWiki/forgewiki/templates/wiki/search.html +++ b/ForgeWiki/forgewiki/templates/wiki/search.html @@ -21,7 +21,9 @@ {% block title %}{{c.project.name}} / {{c.app.config.options.mount_label}} / Search{% endblock %} {% block head %} + {{ super() }} <meta name="robots" content="noindex, follow"> + {{ lib.pagination_meta_tags(request, page, count, limit) }} {% endblock %} {% block header %}Search {{c.app.config.options.mount_point}}: {{q}}{% endblock %} diff --git a/ForgeWiki/forgewiki/tests/functional/test_root.py b/ForgeWiki/forgewiki/tests/functional/test_root.py index 6c98b5ffc..67e473d50 100644 --- a/ForgeWiki/forgewiki/tests/functional/test_root.py +++ b/ForgeWiki/forgewiki/tests/functional/test_root.py @@ -32,6 +32,7 @@ from allura.tests import decorators as td from alluratest.controller import TestController from forgewiki import model +from unittest.mock import MagicMock class TestRootController(TestController): @@ -90,6 +91,7 @@ class TestRootController(TestController): for ext in ['', '.rss', '.atom']: self.app.get('/wiki/feed%s' % ext, status=200) + @patch('allura.lib.helpers.ceil', MagicMock(return_value=1)) @patch('allura.lib.search.search') def test_search(self, search): r = self.app.get('/wiki/search/?q=test') @@ -475,9 +477,19 @@ class TestRootController(TestController): assert '(Page 1 of 4)' in r assert '<td>label30</td>' in r assert '<td>label1</td>' in r - r = self.app.get('/wiki/browse_tags/?page=3') - assert '<td>label77</td>' in r - assert '<td>label99</td>' in r + r = self.app.get('/wiki/browse_tags/?page=2') + assert '<td>label69</td>' in r + assert '<td>label70</td>' in r + r.mustcontain('canonical') + canonical = r.html.select_one('link[rel=canonical]') + assert 'browse_tags' in canonical['href'] + next = r.html.select_one('link[rel=next]') + assert('page=3' in next['href']) + prev = r.html.select_one('link[rel=prev]') + assert('page=1' in prev['href']) + r = self.app.get('/wiki/browse_tags/?page=0') + canonical = r.html.select_one('link[rel=canonical]') + assert 'page=' not in canonical def test_new_attachment(self): self.app.post(
