From ad93b4f8e10695bae2dbf4fe8cd555f5cb07e6f1 Mon Sep 17 00:00:00 2001 From: Stephan Klinger Date: Sun, 7 Jan 2018 22:58:57 +0100 Subject: [PATCH 01/34] Add redirection control panel --- .../controlpanel/browser/configure.zcml | 15 + .../browser/redirects-controlpanel.pt | 174 +++++++++++ .../controlpanel/browser/redirects-manage.pt | 130 ++++++++ .../controlpanel/browser/redirects.py | 280 ++++++++++++++++++ .../test_controlpanel_browser_redirection.py | 239 +++++++++++++++ Products/CMFPlone/interfaces/controlpanel.py | 19 ++ 6 files changed, 857 insertions(+) create mode 100644 Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt create mode 100644 Products/CMFPlone/controlpanel/browser/redirects-manage.pt create mode 100644 Products/CMFPlone/controlpanel/browser/redirects.py create mode 100644 Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py diff --git a/Products/CMFPlone/controlpanel/browser/configure.zcml b/Products/CMFPlone/controlpanel/browser/configure.zcml index 5115a526a3..3d35108ce1 100644 --- a/Products/CMFPlone/controlpanel/browser/configure.zcml +++ b/Products/CMFPlone/controlpanel/browser/configure.zcml @@ -291,4 +291,19 @@ permission="cmf.ManagePortal" /> + + + + diff --git a/Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt b/Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt new file mode 100644 index 0000000000..162c5b403c --- /dev/null +++ b/Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt @@ -0,0 +1,174 @@ + + + +
+
+
Error
+
+

+ No aliases were added. Please correct these errors in your CSV file and try again: +

+ + + + + + +
+ + Line + + ${error/line_number} + + : + + + ${error/line} + + ${error/message} +
+
+
+ +

+ Manage aliases +

+ +
+
+ + Add a new alias + + + + + + + + +
+ +
+ +
+
+ + +
+
+ + Bulk-upload aliases + + +

+ Add many aliases at once by uploading a CSV file. The first column should be the path to + redirect from; the second, the path to redirect to. Both paths must be Plone-site-relative, + starting with a slash (/). +

+

+ Example:
+ + /old-home-page.asp,/front-page
+ /people/JoeT,/Users/joe-thurston + +

+ + + +
+ +
+ +
+
+ +
+
+ + All existing aliases for this site + + + + +
+ +
+ + +
+ +
+ + +
+
+ +
+ +
+ +
+
+
+ + diff --git a/Products/CMFPlone/controlpanel/browser/redirects-manage.pt b/Products/CMFPlone/controlpanel/browser/redirects-manage.pt new file mode 100644 index 0000000000..558e9c44ee --- /dev/null +++ b/Products/CMFPlone/controlpanel/browser/redirects-manage.pt @@ -0,0 +1,130 @@ + + + +
+

+ Manage aliases +

+ +
+
+ Info +
+
+ You are managing the aliases of a default view in a container. + To manage them for the entire container, + + go here + . +
+
+ +

+ Using this form, you can manage aliases for an item. This + is an easy way to make an item available under two different URLs. +

+ +
+
+ + Existing aliases for this item + + +
+ + + + (Required) + + +
+ Validation error output +
+ +
+ Enter the absolute path where the alias should exist. The path must start with '/'. +
+ + +
+
+ +
+ +
+
+ +
+
+ + Existing aliases for this item + + + +
+ +
+ + +
+ +
+ +
+
+
+ + diff --git a/Products/CMFPlone/controlpanel/browser/redirects.py b/Products/CMFPlone/controlpanel/browser/redirects.py new file mode 100644 index 0000000000..d1a1d6f251 --- /dev/null +++ b/Products/CMFPlone/controlpanel/browser/redirects.py @@ -0,0 +1,280 @@ +# -*- coding: utf-8 -*- +from AccessControl import getSecurityManager +from cStringIO import StringIO +from plone import api +from plone.app.redirector.interfaces import IRedirectionStorage +from plone.batching.browser import PloneBatchView +from plone.memoize.view import memoize +from Products.CMFCore.interfaces import ISiteRoot +from Products.CMFCore.permissions import ManagePortal +from Products.CMFPlone.PloneBatch import Batch +from Products.Five.browser import BrowserView +from Products.Five.browser.pagetemplatefile import ViewPageTemplateFile +from Products.statusmessages.interfaces import IStatusMessage +from zope.component import getUtility +from zope.i18nmessageid import MessageFactory + +import csv + +_ = MessageFactory('plone') + + +def absolutize_path(path, context=None, is_alias=True): + """Check whether object exist to the provided `path`. + Assume relative paths are relative to `context`; + reject relative paths if `context` is None. + Return a 2-tuple: (absolute redirection path, + an error message if something goes wrong and otherwise ''). + """ + + portal = getUtility(ISiteRoot) + storage = getUtility(IRedirectionStorage) + err = None + if path is None or path == '': + err = (is_source and _(u"You have to enter an alias.") + or _(u"You have to enter a target.")) + else: + if path.startswith('/'): + context_path = "/".join(portal.getPhysicalPath()) + path = "{0}{1}".format(context_path, path) + else: + if context is None: + err = (is_alias and _(u"Alias path must start with a slash.") + or _(u"Target path must start with a slash.")) + else: + # What case should this be? + context_path = "/".join(context.getPhysicalPath()[:-1]) + path = "{0}/{1}".format(context_path, path) + if not err and not is_alias: + # Check whether obj exists at source path + result = api.content.find(path={"query": path}) + if len(result) == 0: + err = _(u"The provided target object does not exist.") + if not err and is_alias: + # Check whether already exists in storage + if storage.get(path): + err = _(u"The provided alias already exists!") + + return path, err + +class RedirectsView(BrowserView): + template = ViewPageTemplateFile('redirects-manage.pt') + + def redirects(self): + storage = getUtility(IRedirectionStorage) + portal = getUtility(ISiteRoot) + context_path = "/".join(self.context.getPhysicalPath()) + portal_path = "/".join(portal.getPhysicalPath()) + redirects = storage.redirects(context_path) + for redirect in redirects: + path = redirect[len(portal_path):] + yield { + 'redirect': redirect, + 'path': path, + } + + def __call__(self): + storage = getUtility(IRedirectionStorage) + request = self.request + form = request.form + status = IStatusMessage(self.request) + errors = {} + + if 'form.button.Add' in form: + redirection, err = absolutize_path(form.get('redirection'), is_alias=True) + if err: + errors['redirection'] = err + status.addStatusMessage(err, type='error') + else: + del form['redirection'] + storage.add(redirection, "/".join(self.context.getPhysicalPath())) + status.addStatusMessage(_(u"Alias added."), type='info') + elif 'form.button.Remove' in form: + redirects = form.get('redirects', ()) + for redirect in redirects: + storage.remove(redirect) + if len(redirects) > 1: + status.addStatusMessage(_(u"Aliases removed."), type='info') + else: + status.addStatusMessage(_(u"Alias removed."), type='info') + + return self.template(errors=errors) + + @memoize + def view_url(self): + return self.context.absolute_url() + '/@@manage-aliases' + + +class RedirectionSet(object): + def __init__(self, query=''): + self.storage = getUtility(IRedirectionStorage) + + portal = getUtility(ISiteRoot) + self.portal_path = '/'.join(portal.getPhysicalPath()) + self.portal_path_len = len(self.portal_path) + + # noinspection PyProtectedMember + if query: + min_k = u'{0:s}/{1:s}'.format(self.portal_path, query.strip('/')) + max_k = min_k[:-1] + chr(ord(min_k[-1]) + 1) + self.data = list(self.storage._paths.keys(min=min_k, max=max_k)) + else: + self.data = list(self.storage._paths.keys()) # maybe be costly + + def __len__(self): + return len(self.data) + + def __getitem__(self, item): + redirect = self.data[item] + if redirect.startswith(self.portal_path): + path = redirect[self.portal_path_len:] + else: + path = redirect + redirect_to = self.storage.get(redirect) + if redirect_to.startswith(self.portal_path): + redirect_to = redirect_to[self.portal_path_len:] + return { + 'redirect': redirect, + 'path': path, + 'redirect-to': redirect_to, + } + + +class RedirectsBatchView(PloneBatchView): + def make_link(self, pagenumber=None, omit_params=None): + if omit_params is None: + omit_params = ['ajax_load'] + url = super(RedirectsBatchView, self).make_link(pagenumber, + omit_params) + return u'{0:s}#manage-existing-aliases'.format(url) + + +class RedirectsControlPanel(BrowserView): + + template = ViewPageTemplateFile('redirects-controlpanel.pt') + + def __init__(self, context, request): + super(RedirectsControlPanel, self).__init__(context, request) + self.errors = [] + # list of tuples: (line_number, absolute_redirection_path, err_msg, target) + + def batching(self): + return RedirectsBatchView(self.context, self.request)(self.redirects()) + + @memoize + def redirects(self): + """ Get existing redirects from the redirection storage. + Return dict with the strings redirect, path and redirect-to. + Strip the id of the instance from path and redirect-to if + it is present. (Seems to be always true) + If id of instance is not present in path the var 'path' and + 'redirect' are equal. + """ + return Batch( + RedirectionSet(self.request.form.get('q', '')), + 15, + int(self.request.form.get('b_start', '0')), + orphan=1 + ) + + def __call__(self): + storage = getUtility(IRedirectionStorage) + portal = getUtility(ISiteRoot) + request = self.request + form = request.form + status = IStatusMessage(self.request) + + if 'form.button.Remove' in form: + redirects = form.get('redirects', ()) + for redirect in redirects: + storage.remove(redirect) + if len(redirects) == 0: + status.addStatusMessage(_(u"No aliases selected for removal."), type='info') + elif len(redirects) > 1: + status.addStatusMessage(_(u"Aliases removed."), type='info') + else: + status.addStatusMessage(_(u"Alias removed."), type='info') + elif 'form.button.Add' in form: + self.add(form['redirection'], form['target_path'], portal, storage, status) + elif 'form.button.Upload' in form: + self.upload(form['file'], portal, storage, status) + + return self.template() + + def add(self, redirection, target, portal, storage, status): + """Add the redirections from the form. If anything goes wrong, do nothing.""" + abs_target = '' + target_err = '' + + abs_redirection, err = absolutize_path(redirection, is_alias=True) + if not err: + abs_target, target_err = absolutize_path(target, is_alias=False) + + if err and target_err: + err = "{0} {1}".format(err, target_err) + elif target_err: + err = target_err + else: + if abs_redirection == abs_target: + err = _(u"Aliases that point to themselves will cause" + u"an endless cycle of redirects.") + # TODO: detect indirect recursion + + if err: + status.addStatusMessage(_(err), type='error') + else: + storage.add(abs_redirection, abs_target) + status.addStatusMessage(_(u"Alias {0} → {1} added.").format(abs_redirection, abs_target), + type='info') + + def upload(self, file, portal, storage, status): + """Add the redirections from the CSV file `file`. If anything goes wrong, do nothing.""" + + # No file picked. Theres gotta be a better way to handle this. + if not file.filename: + status.addStatusMessage(_(u"Please pick a file to upload."), type='info') + return + # Turn all kinds of newlines into LF ones. The csv module doesn't do + # its own newline sniffing and requires either \n or \r. + file = StringIO('\n'.join(file.read().splitlines())) + + # Use first two lines as a representative sample for guessing format, + # in case one is a bunch of headers. + dialect = csv.Sniffer().sniff(file.readline() + file.readline()) + file.seek(0) + + successes = [] # list of tuples: (abs_redirection, target) + had_errors = False + for i, fields in enumerate(csv.reader(file, dialect)): + if len(fields) == 2: + redirection, target = fields + abs_redirection, err = absolutize_path(redirection, is_alias=True) + abs_target, target_err = absolutize_path(target, is_alias=False) + if err and target_err: + err = "%s %s" % (err, target_err) # sloppy w.r.t. i18n + elif target_err: + err = target_err + else: + if abs_redirection == abs_target: + # TODO: detect indirect recursion + err = _(u"Aliases that point to themselves will cause" + u"an endless cycle of redirects.") + else: + err = _(u"Each line must have 2 columns.") + + if not err: + if not had_errors: # else don't bother + successes.append((abs_redirection, abs_target)) + else: + had_errors = True + self.errors.append(dict(line_number=i+1, line=dialect.delimiter.join(fields), + message=err)) + + if not had_errors: + for abs_redirection, abs_target in successes: + storage.add(abs_redirection, abs_target) + status.addStatusMessage(_(u"%i aliases added.") % len(successes), type='info') + + @memoize + def view_url(self): + return self.context.absolute_url() + '/@@redirection-controlpanel' diff --git a/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py b/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py new file mode 100644 index 0000000000..1bc765fc11 --- /dev/null +++ b/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py @@ -0,0 +1,239 @@ +# -*- coding: utf-8 -*- +from Products.CMFPlone.PloneBatch import Batch +from Products.CMFPlone.controlpanel.browser.redirects import RedirectionSet +from plone.app.testing import SITE_OWNER_NAME, SITE_OWNER_PASSWORD +from plone.app.redirector.interfaces import IRedirectionStorage +from plone.testing.z2 import Browser + +from zope.component import getMultiAdapter +from zope.component import getUtility + +from Products.CMFPlone.testing import \ + PRODUCTS_CMFPLONE_FUNCTIONAL_TESTING + +import math +import unittest +import transaction + +class RedirectionControlPanelFunctionalTest(unittest.TestCase): + """Test that changes in the mail control panel are actually + stored in the registry. + """ + + layer = PRODUCTS_CMFPLONE_FUNCTIONAL_TESTING + + def setUp(self): + self.app = self.layer['app'] + self.portal = self.layer['portal'] + self.portal_url = self.portal.absolute_url() + self.browser = Browser(self.app) + self.browser.handleErrors = False + self.browser.addHeader( + 'Authorization', + 'Basic %s:%s' % (SITE_OWNER_NAME, SITE_OWNER_PASSWORD,) + ) + + """ + def test_mail_controlpanel_link(self): + self.browser.open( + "%s/@@overview-controlpanel" % self.portal_url) + self.browser.getLink('Mail').click() + + def test_mail_controlpanel_backlink(self): + self.browser.open( + "%s/@@mail-controlpanel" % self.portal_url) + self.assertTrue("General" in self.browser.contents) + + def test_mail_controlpanel_sidebar(self): + self.browser.open( + "%s/@@mail-controlpanel" % self.portal_url) + self.browser.getLink('Site Setup').click() + self.assertTrue( + self.browser.url.endswith('/plone/@@overview-controlpanel') + ) + """ + + def test_redirection_controlpanel_view(self): + view = getMultiAdapter((self.portal, self.portal.REQUEST), + name="redirection-controlpanel") + self.assertTrue(view()) + + def test_redirection_controlpanel_add_redirect(self): + storage = getUtility(IRedirectionStorage) + redirection_path = '/alias-folder' + target_path = '/test-folder' + storage_path = '/plone/alias-folder' + + self.browser.open( + "%s/@@redirection-controlpanel" % self.portal_url) + self.browser.getControl( + name='redirection').value = redirection_path + self.browser.getControl( + name='target_path').value = target_path + self.browser.getControl(name='form.button.Add').click() + + self.assertTrue( + storage.has_path(storage_path), + u'Redirection storage should have path "{0}"'.format(storage_path) + ) + + def test_redirection_controlpanel_set(self): + storage = getUtility(IRedirectionStorage) + portal_path = self.layer['portal'].absolute_url_path() + for i in range(1000): + storage.add('{0:s}/foo/{1:s}'.format(portal_path, str(i)), + '{0:s}/bar/{1:s}'.format(portal_path, str(i))) + redirects = RedirectionSet() + self.assertEqual(len(redirects), 1000) + self.assertDictEqual(redirects[0], { + 'redirect': '{0:s}/foo/0'.format(portal_path), + 'path': '/foo/0', 'redirect-to': '/bar/0' + }) + self.assertDictEqual(redirects[999], { + 'redirect': '{0:s}/foo/999'.format(portal_path), + 'path': '/foo/999', 'redirect-to': '/bar/999' + }) + self.assertEqual(len(list(iter(redirects))), 1000) + self.assertDictEqual(list(iter(redirects))[0], { + 'redirect': '{0:s}/foo/0'.format(portal_path), + 'path': '/foo/0', 'redirect-to': '/bar/0' + }) + + def test_redirection_controlpanel_batching(self): + storage = getUtility(IRedirectionStorage) + portal_path = self.layer['portal'].absolute_url_path() + for i in range(1000): + storage.add('{0:s}/foo/{1:s}'.format(portal_path, str(i)), + '{0:s}/bar/{1:s}'.format(portal_path, str(i))) + view = getMultiAdapter((self.layer['portal'], self.layer['request']), + name='redirection-controlpanel') + # Test that view/redirects returns batch + self.assertIsInstance(view.redirects(), Batch) + + # Test that view/batching returns batching with anchor in urls + batching = view.batching() + self.assertIn('?b_start:int=990#manage-existing-aliases', batching) + + def test_redirection_controlpanel_redirect_alias_exists(self): + path_alias = '/alias' + path_target = '/test-folder' + storage_alias = '/plone{0}'.format(path_alias) + storage_target = '/plone{0}'.format(path_target) + storage = getUtility(IRedirectionStorage) + storage.add(storage_alias, storage_target) + transaction.commit() + + self.browser.open( + "%s/@@redirection-controlpanel" % self.portal_url) + self.browser.getControl( + name='redirection').value = path_alias + self.browser.getControl( + name='target_path').value = path_target + self.browser.getControl(name='form.button.Add').click() + + self.assertTrue( + storage.get(storage_alias) == storage_target, + '{0} not target of alias!'.format(storage_target) + ) + self.assertTrue( + 'The provided alias already exists!' in self.browser.contents, + u'Message "alias already exists" not in page!' + ) + + def test_redirection_controlpanel_filtering(self): + storage = getUtility(IRedirectionStorage) + portal_path = self.layer['portal'].absolute_url_path() + for i in range(1000): + storage.add('{0:s}/foo1/{1:s}'.format(portal_path, str(i)), + '{0:s}/bar/{1:s}'.format(portal_path, str(i))) + for i in range(1000): + storage.add('{0:s}/foo2/{1:s}'.format(portal_path, str(i)), + '{0:s}/bar/{1:s}'.format(portal_path, str(i))) + + redirects = RedirectionSet() + self.assertEqual(len(redirects), 2000) + redirects = RedirectionSet(query='/foo') + self.assertEqual(len(redirects), 2000) + redirects = RedirectionSet(query='/foo1') + self.assertEqual(len(redirects), 1000) + redirects = RedirectionSet(query='/foo2') + self.assertEqual(len(redirects), 1000) + + request = self.layer['request'].clone() + request.form['q'] = '/foo' + view = getMultiAdapter((self.layer['portal'], request), + name='redirection-controlpanel') + self.assertEqual(view.redirects().numpages, math.ceil(2000 / 15.)) + + request = self.layer['request'].clone() + request.form['q'] = '/foo1' + view = getMultiAdapter((self.layer['portal'], request), + name='redirection-controlpanel') + self.assertEqual(view.redirects().numpages, math.ceil(1000 / 15.)) + + request = self.layer['request'].clone() + request.form['q'] = '/foo2' + view = getMultiAdapter((self.layer['portal'], request), + name='redirection-controlpanel') + self.assertEqual(view.redirects().numpages, math.ceil(1000 / 15.)) + + request = self.layer['request'].clone() + view = getMultiAdapter((self.layer['portal'], request), + name='redirection-controlpanel') + self.assertEqual(view.redirects().numpages, math.ceil(2000 / 15.)) + + # Filtering without new request does not have effect because memoize + request.form['q'] = '/foo2' + self.assertEqual(view.redirects().numpages, math.ceil(2000 / 15.)) + + def test_redirection_controlpanel_redirect_no_target(self): + path_alias = '/alias' + path_target = '/not-existing' + + self.browser.open( + "%s/@@redirection-controlpanel" % self.portal_url) + self.browser.getControl( + name='redirection').value = path_alias + self.browser.getControl( + name='target_path').value = path_target + self.browser.getControl(name='form.button.Add').click() + + self.assertTrue( + 'The provided target object does not exist.' in self.browser.contents, + u'Message "target does not exist" not in page!' + ) + + def test_redirection_controlpanel_missing_slash_target(self): + path_alias = '/alias' + path_target = 'Members' + + self.browser.open( + "%s/@@redirection-controlpanel" % self.portal_url) + self.browser.getControl( + name='redirection').value = path_alias + self.browser.getControl( + name='target_path').value = path_target + self.browser.getControl(name='form.button.Add').click() + + self.assertTrue( + 'Target path must start with a slash.' in self.browser.contents, + u'Errormessage for missing slash on target path missing' + ) + + + def test_redirection_controlpanel_missing_slash_alias(self): + path_alias = 'alias' + path_target = '/Members' + + self.browser.open( + "%s/@@redirection-controlpanel" % self.portal_url) + self.browser.getControl( + name='redirection').value = path_alias + self.browser.getControl( + name='target_path').value = path_target + self.browser.getControl(name='form.button.Add').click() + + self.assertTrue( + 'Alias path must start with a slash.' in self.browser.contents, + u'Errormessage for missing slash on alias path missing' + ) diff --git a/Products/CMFPlone/interfaces/controlpanel.py b/Products/CMFPlone/interfaces/controlpanel.py index dd52e532af..24c1dcb85a 100644 --- a/Products/CMFPlone/interfaces/controlpanel.py +++ b/Products/CMFPlone/interfaces/controlpanel.py @@ -1426,6 +1426,25 @@ class ITypesSchema(Interface): value_type=schema.TextLine() ) + redirection_manageable_types = schema.Tuple( + title=_(u"Manage redirects for types"), + description=_(u"Select the types for which the " + "aliases can be managed"), + required=False, + default=( + 'Image', + 'File', + 'Link', + 'News Item', + 'Folder', + 'Document', + 'Event' + ), + missing_value=(), + value_type=schema.Choice( + source='plone.app.vocabularies.ReallyUserFriendlyTypes') + ) + class IMailSchema(Interface): From 56feca0b1dd32100acf885cc0b56f9b13efb51bb Mon Sep 17 00:00:00 2001 From: Stephan Klinger Date: Tue, 9 Jan 2018 13:55:21 +0100 Subject: [PATCH 02/34] Update CHANGES --- news/1486.feature | 4 ++++ 1 file changed, 4 insertions(+) create mode 100644 news/1486.feature diff --git a/news/1486.feature b/news/1486.feature new file mode 100644 index 0000000000..2cf99dbc67 --- /dev/null +++ b/news/1486.feature @@ -0,0 +1,4 @@ +PLIP 1486: Merge Products.RedirectionTool into core. +Allow users to manage redirects on their site and aliases to content. +See https://github.com/plone/Products.CMFPlone/issues/1486 +[staeff, maurits] \ No newline at end of file From 284405a27205ac47d8d8ad91ba003981b6840705 Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Wed, 6 Feb 2019 15:33:35 +0100 Subject: [PATCH 03/34] Fixed Python3 import --- Products/CMFPlone/controlpanel/browser/redirects.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Products/CMFPlone/controlpanel/browser/redirects.py b/Products/CMFPlone/controlpanel/browser/redirects.py index d1a1d6f251..6db462a1da 100644 --- a/Products/CMFPlone/controlpanel/browser/redirects.py +++ b/Products/CMFPlone/controlpanel/browser/redirects.py @@ -1,6 +1,5 @@ # -*- coding: utf-8 -*- from AccessControl import getSecurityManager -from cStringIO import StringIO from plone import api from plone.app.redirector.interfaces import IRedirectionStorage from plone.batching.browser import PloneBatchView @@ -11,6 +10,7 @@ from Products.Five.browser import BrowserView from Products.Five.browser.pagetemplatefile import ViewPageTemplateFile from Products.statusmessages.interfaces import IStatusMessage +from six import StringIO from zope.component import getUtility from zope.i18nmessageid import MessageFactory From 434b99f905ebfaee8cdae00c4d014f0358abe788 Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Wed, 6 Feb 2019 15:36:18 +0100 Subject: [PATCH 04/34] Fixed undefined name 'is_source', and other QA on redirects controlpanel. --- Products/CMFPlone/controlpanel/browser/redirects.py | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-) diff --git a/Products/CMFPlone/controlpanel/browser/redirects.py b/Products/CMFPlone/controlpanel/browser/redirects.py index 6db462a1da..3ed429c683 100644 --- a/Products/CMFPlone/controlpanel/browser/redirects.py +++ b/Products/CMFPlone/controlpanel/browser/redirects.py @@ -1,11 +1,9 @@ # -*- coding: utf-8 -*- -from AccessControl import getSecurityManager from plone import api from plone.app.redirector.interfaces import IRedirectionStorage from plone.batching.browser import PloneBatchView from plone.memoize.view import memoize from Products.CMFCore.interfaces import ISiteRoot -from Products.CMFCore.permissions import ManagePortal from Products.CMFPlone.PloneBatch import Batch from Products.Five.browser import BrowserView from Products.Five.browser.pagetemplatefile import ViewPageTemplateFile @@ -31,7 +29,7 @@ def absolutize_path(path, context=None, is_alias=True): storage = getUtility(IRedirectionStorage) err = None if path is None or path == '': - err = (is_source and _(u"You have to enter an alias.") + err = (is_alias and _(u"You have to enter an alias.") or _(u"You have to enter a target.")) else: if path.startswith('/'): @@ -57,6 +55,7 @@ def absolutize_path(path, context=None, is_alias=True): return path, err + class RedirectsView(BrowserView): template = ViewPageTemplateFile('redirects-manage.pt') @@ -267,7 +266,7 @@ def upload(self, file, portal, storage, status): successes.append((abs_redirection, abs_target)) else: had_errors = True - self.errors.append(dict(line_number=i+1, line=dialect.delimiter.join(fields), + self.errors.append(dict(line_number=i + 1, line=dialect.delimiter.join(fields), message=err)) if not had_errors: From 758b213bd28056206e20e511a9cf9eeeddd4e52f Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Wed, 6 Feb 2019 15:41:25 +0100 Subject: [PATCH 05/34] Redirects: replaced plone.api call. --- Products/CMFPlone/controlpanel/browser/redirects.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/Products/CMFPlone/controlpanel/browser/redirects.py b/Products/CMFPlone/controlpanel/browser/redirects.py index 3ed429c683..27d2fd5f2f 100644 --- a/Products/CMFPlone/controlpanel/browser/redirects.py +++ b/Products/CMFPlone/controlpanel/browser/redirects.py @@ -1,9 +1,9 @@ # -*- coding: utf-8 -*- -from plone import api from plone.app.redirector.interfaces import IRedirectionStorage from plone.batching.browser import PloneBatchView from plone.memoize.view import memoize from Products.CMFCore.interfaces import ISiteRoot +from Products.CMFCore.utils import getToolByName from Products.CMFPlone.PloneBatch import Batch from Products.Five.browser import BrowserView from Products.Five.browser.pagetemplatefile import ViewPageTemplateFile @@ -45,7 +45,8 @@ def absolutize_path(path, context=None, is_alias=True): path = "{0}/{1}".format(context_path, path) if not err and not is_alias: # Check whether obj exists at source path - result = api.content.find(path={"query": path}) + catalog = getToolByName(context, 'portal_catalog') + result = catalog.searchResults(path={"query": path}) if len(result) == 0: err = _(u"The provided target object does not exist.") if not err and is_alias: From 7251fc860dd95cfd7da9f8434adc99b8b1685d6a Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Wed, 6 Feb 2019 17:23:59 +0100 Subject: [PATCH 06/34] Fixed security declaration in redirects for nonexistent method 'errors'. Class Products.Five.browser.metaconfigure.RedirectsView has a security declaration for nonexistent method 'errors' --- Products/CMFPlone/controlpanel/browser/configure.zcml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Products/CMFPlone/controlpanel/browser/configure.zcml b/Products/CMFPlone/controlpanel/browser/configure.zcml index 3d35108ce1..6d1d7d3955 100644 --- a/Products/CMFPlone/controlpanel/browser/configure.zcml +++ b/Products/CMFPlone/controlpanel/browser/configure.zcml @@ -303,7 +303,7 @@ for="Products.CMFCore.interfaces.IContentish" class=".redirects.RedirectsView" permission="cmf.ModifyPortalContent" - allowed_attributes="redirects view_url errors" + allowed_attributes="redirects view_url" /> From 4e96d4bcc43c174231ea85c752a3a4552b2bc697 Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Wed, 6 Feb 2019 17:29:44 +0100 Subject: [PATCH 07/34] Redirects: moved templates to zcml. Makes it easier to override just the template. --- Products/CMFPlone/controlpanel/browser/configure.zcml | 2 ++ Products/CMFPlone/controlpanel/browser/redirects.py | 8 ++------ 2 files changed, 4 insertions(+), 6 deletions(-) diff --git a/Products/CMFPlone/controlpanel/browser/configure.zcml b/Products/CMFPlone/controlpanel/browser/configure.zcml index 6d1d7d3955..2dea057a35 100644 --- a/Products/CMFPlone/controlpanel/browser/configure.zcml +++ b/Products/CMFPlone/controlpanel/browser/configure.zcml @@ -295,6 +295,7 @@ name="redirection-controlpanel" for="Products.CMFPlone.interfaces.IPloneSiteRoot" class=".redirects.RedirectsControlPanel" + template="redirects-controlpanel.pt" permission="cmf.ManagePortal" /> @@ -302,6 +303,7 @@ name="manage-aliases" for="Products.CMFCore.interfaces.IContentish" class=".redirects.RedirectsView" + template="redirects-manage.pt" permission="cmf.ModifyPortalContent" allowed_attributes="redirects view_url" /> diff --git a/Products/CMFPlone/controlpanel/browser/redirects.py b/Products/CMFPlone/controlpanel/browser/redirects.py index 27d2fd5f2f..d6acfb39b5 100644 --- a/Products/CMFPlone/controlpanel/browser/redirects.py +++ b/Products/CMFPlone/controlpanel/browser/redirects.py @@ -6,7 +6,6 @@ from Products.CMFCore.utils import getToolByName from Products.CMFPlone.PloneBatch import Batch from Products.Five.browser import BrowserView -from Products.Five.browser.pagetemplatefile import ViewPageTemplateFile from Products.statusmessages.interfaces import IStatusMessage from six import StringIO from zope.component import getUtility @@ -58,7 +57,6 @@ def absolutize_path(path, context=None, is_alias=True): class RedirectsView(BrowserView): - template = ViewPageTemplateFile('redirects-manage.pt') def redirects(self): storage = getUtility(IRedirectionStorage) @@ -98,7 +96,7 @@ def __call__(self): else: status.addStatusMessage(_(u"Alias removed."), type='info') - return self.template(errors=errors) + return self.index(errors=errors) @memoize def view_url(self): @@ -151,8 +149,6 @@ def make_link(self, pagenumber=None, omit_params=None): class RedirectsControlPanel(BrowserView): - template = ViewPageTemplateFile('redirects-controlpanel.pt') - def __init__(self, context, request): super(RedirectsControlPanel, self).__init__(context, request) self.errors = [] @@ -199,7 +195,7 @@ def __call__(self): elif 'form.button.Upload' in form: self.upload(form['file'], portal, storage, status) - return self.template() + return self.index() def add(self, redirection, target, portal, storage, status): """Add the redirections from the form. If anything goes wrong, do nothing.""" From 200084e32dd61dcdfed1008049452ac3bfcf5fb7 Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Wed, 6 Feb 2019 19:37:38 +0100 Subject: [PATCH 08/34] Redirection: added hint about what min_k and max_k is. --- Products/CMFPlone/controlpanel/browser/redirects.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/Products/CMFPlone/controlpanel/browser/redirects.py b/Products/CMFPlone/controlpanel/browser/redirects.py index d6acfb39b5..76e1b96607 100644 --- a/Products/CMFPlone/controlpanel/browser/redirects.py +++ b/Products/CMFPlone/controlpanel/browser/redirects.py @@ -113,6 +113,10 @@ def __init__(self, query=''): # noinspection PyProtectedMember if query: + # with query path /Plone/news: + # min_k is /Plone/news and + # max_k is /Plone/newt + # Apparently that is the way to minize the keys we ask. min_k = u'{0:s}/{1:s}'.format(self.portal_path, query.strip('/')) max_k = min_k[:-1] + chr(ord(min_k[-1]) + 1) self.data = list(self.storage._paths.keys(min=min_k, max=max_k)) From 24581d2680ab1c67bb595828bbd743795ee0d7cd Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Wed, 6 Feb 2019 21:02:47 +0100 Subject: [PATCH 09/34] Redirection: do not list the path keys. That would take a few seconds when there are a million keys. We still call len(self.data) at some point, but this does not actually list the keys, but uses internal BTree optimizations. So this plays nice with the batched navigation. --- Products/CMFPlone/controlpanel/browser/redirects.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/Products/CMFPlone/controlpanel/browser/redirects.py b/Products/CMFPlone/controlpanel/browser/redirects.py index 76e1b96607..4d1adc3ed2 100644 --- a/Products/CMFPlone/controlpanel/browser/redirects.py +++ b/Products/CMFPlone/controlpanel/browser/redirects.py @@ -119,9 +119,9 @@ def __init__(self, query=''): # Apparently that is the way to minize the keys we ask. min_k = u'{0:s}/{1:s}'.format(self.portal_path, query.strip('/')) max_k = min_k[:-1] + chr(ord(min_k[-1]) + 1) - self.data = list(self.storage._paths.keys(min=min_k, max=max_k)) + self.data = self.storage._paths.keys(min=min_k, max=max_k) else: - self.data = list(self.storage._paths.keys()) # maybe be costly + self.data = self.storage._paths.keys() def __len__(self): return len(self.data) From af8adbae86693da17c8cfe1db0da83da9e821e7b Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Wed, 6 Feb 2019 21:23:24 +0100 Subject: [PATCH 10/34] Redirection: added Collection to default types. And sorted the list. --- Products/CMFPlone/interfaces/controlpanel.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/Products/CMFPlone/interfaces/controlpanel.py b/Products/CMFPlone/interfaces/controlpanel.py index 24c1dcb85a..dcc0aecf0b 100644 --- a/Products/CMFPlone/interfaces/controlpanel.py +++ b/Products/CMFPlone/interfaces/controlpanel.py @@ -1432,13 +1432,14 @@ class ITypesSchema(Interface): "aliases can be managed"), required=False, default=( - 'Image', + 'Collection', + 'Document', + 'Event', 'File', + 'Folder', + 'Image', 'Link', 'News Item', - 'Folder', - 'Document', - 'Event' ), missing_value=(), value_type=schema.Choice( From 75b6285ad010d72cdcc593d1146d5bacd71d8dfc Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Wed, 6 Feb 2019 23:29:17 +0100 Subject: [PATCH 11/34] absolutize_path: renamed is_alias keyword arg to is_source. This name makes the meaning clearer, I hope. A previous commit already started this, but only in one place, where I reverted it. Makes sense to rename it now. --- .../controlpanel/browser/redirects.py | 20 +++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/Products/CMFPlone/controlpanel/browser/redirects.py b/Products/CMFPlone/controlpanel/browser/redirects.py index 4d1adc3ed2..113a22faab 100644 --- a/Products/CMFPlone/controlpanel/browser/redirects.py +++ b/Products/CMFPlone/controlpanel/browser/redirects.py @@ -16,7 +16,7 @@ _ = MessageFactory('plone') -def absolutize_path(path, context=None, is_alias=True): +def absolutize_path(path, context=None, is_source=True): """Check whether object exist to the provided `path`. Assume relative paths are relative to `context`; reject relative paths if `context` is None. @@ -28,7 +28,7 @@ def absolutize_path(path, context=None, is_alias=True): storage = getUtility(IRedirectionStorage) err = None if path is None or path == '': - err = (is_alias and _(u"You have to enter an alias.") + err = (is_source and _(u"You have to enter an alias.") or _(u"You have to enter a target.")) else: if path.startswith('/'): @@ -36,19 +36,19 @@ def absolutize_path(path, context=None, is_alias=True): path = "{0}{1}".format(context_path, path) else: if context is None: - err = (is_alias and _(u"Alias path must start with a slash.") + err = (is_source and _(u"Alias path must start with a slash.") or _(u"Target path must start with a slash.")) else: # What case should this be? context_path = "/".join(context.getPhysicalPath()[:-1]) path = "{0}/{1}".format(context_path, path) - if not err and not is_alias: + if not err and not is_source: # Check whether obj exists at source path catalog = getToolByName(context, 'portal_catalog') result = catalog.searchResults(path={"query": path}) if len(result) == 0: err = _(u"The provided target object does not exist.") - if not err and is_alias: + if not err and is_source: # Check whether already exists in storage if storage.get(path): err = _(u"The provided alias already exists!") @@ -79,7 +79,7 @@ def __call__(self): errors = {} if 'form.button.Add' in form: - redirection, err = absolutize_path(form.get('redirection'), is_alias=True) + redirection, err = absolutize_path(form.get('redirection'), is_source=True) if err: errors['redirection'] = err status.addStatusMessage(err, type='error') @@ -206,9 +206,9 @@ def add(self, redirection, target, portal, storage, status): abs_target = '' target_err = '' - abs_redirection, err = absolutize_path(redirection, is_alias=True) + abs_redirection, err = absolutize_path(redirection, is_source=True) if not err: - abs_target, target_err = absolutize_path(target, is_alias=False) + abs_target, target_err = absolutize_path(target, is_source=False) if err and target_err: err = "{0} {1}".format(err, target_err) @@ -248,8 +248,8 @@ def upload(self, file, portal, storage, status): for i, fields in enumerate(csv.reader(file, dialect)): if len(fields) == 2: redirection, target = fields - abs_redirection, err = absolutize_path(redirection, is_alias=True) - abs_target, target_err = absolutize_path(target, is_alias=False) + abs_redirection, err = absolutize_path(redirection, is_source=True) + abs_target, target_err = absolutize_path(target, is_source=False) if err and target_err: err = "%s %s" % (err, target_err) # sloppy w.r.t. i18n elif target_err: From fb962c82ce908577e03b0c636219585b881c051d Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Wed, 6 Feb 2019 23:50:10 +0100 Subject: [PATCH 12/34] Added test_absolutize_path. I want to change the absolutize_path function from redirection, because it has unused code paths, making it too difficult. --- .../test_controlpanel_browser_redirection.py | 52 +++++++++++++++++++ 1 file changed, 52 insertions(+) diff --git a/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py b/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py index 1bc765fc11..78f620673a 100644 --- a/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py +++ b/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py @@ -237,3 +237,55 @@ def test_redirection_controlpanel_missing_slash_alias(self): 'Alias path must start with a slash.' in self.browser.contents, u'Errormessage for missing slash on alias path missing' ) + + def test_absolutize_path(self): + # absolutize_path is a helper function that returns a tuple + # of absolute path and error message. + from Products.CMFPlone.controlpanel.browser.redirects import ( + absolutize_path as ap) + + # A path is required. + self.assertEqual( + ap(''), + ('', 'You have to enter an alias.')) + self.assertEqual( + ap('', is_source=False), + ('', 'You have to enter a target.')) + + # relative paths are not accepted + self.assertEqual( + ap('foo'), + ('foo', 'Alias path must start with a slash.')) + self.assertEqual( + ap('foo', is_source=True), + ('foo', 'Alias path must start with a slash.')) + self.assertEqual( + ap('foo', is_source=False), + ('foo', 'Target path must start with a slash.')) + + # absolute paths are good + self.assertEqual(ap('/foo'), ('/plone/foo', None)) + self.assertEqual(ap('/foo', is_source=True), ('/plone/foo', None)) + + # for targets, an object must exist on the path + self.assertEqual( + ap('/foo', is_source=False), + ('/plone/foo', 'The provided target object does not exist.')) + self.assertEqual( + ap('/test-folder', is_source=False), + ('/plone/test-folder', None)) + + # sources do not need to exist, but if an object exists, + # we currently have no problems with it. + self.assertEqual( + ap('/test-folder'), + ('/plone/test-folder', None)) + + # A source must not already exist in the redirect list. + storage = getUtility(IRedirectionStorage) + portal_path = self.layer['portal'].absolute_url_path() + storage.add('{0:s}/foo'.format(portal_path), + '{0:s}/test-folder'.format(portal_path)) + self.assertEqual( + ap('/foo', is_source=True), + ('/plone/foo', 'The provided alias already exists!')) From 6a510f0a79d14a867bf2a04e6837f8ca28280904 Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Thu, 7 Feb 2019 00:01:39 +0100 Subject: [PATCH 13/34] Refactored absolutize_path to not accept a context keyword argument. No code path used this. --- .../controlpanel/browser/redirects.py | 53 +++++++++++-------- 1 file changed, 30 insertions(+), 23 deletions(-) diff --git a/Products/CMFPlone/controlpanel/browser/redirects.py b/Products/CMFPlone/controlpanel/browser/redirects.py index 113a22faab..f3f01eb90f 100644 --- a/Products/CMFPlone/controlpanel/browser/redirects.py +++ b/Products/CMFPlone/controlpanel/browser/redirects.py @@ -16,42 +16,49 @@ _ = MessageFactory('plone') -def absolutize_path(path, context=None, is_source=True): - """Check whether object exist to the provided `path`. - Assume relative paths are relative to `context`; - reject relative paths if `context` is None. - Return a 2-tuple: (absolute redirection path, - an error message if something goes wrong and otherwise ''). +def absolutize_path(path, is_source=True): + """Create path including the path of the portal root. + + The path must be absolute, so starting with a slash. + + If is_source is true, this is an alternative url + that will point to a target (unknown here). + + If is_source is true, path is the path of a target. + An object must exist at this path. + + Return a 2-tuple: (absolute redirection path, + an error message if something goes wrong and otherwise ''). """ portal = getUtility(ISiteRoot) - storage = getUtility(IRedirectionStorage) err = None - if path is None or path == '': - err = (is_source and _(u"You have to enter an alias.") - or _(u"You have to enter a target.")) + if not path: + if is_source: + err = _(u"You have to enter an alias.") + else: + err = _(u"You have to enter a target.") else: if path.startswith('/'): context_path = "/".join(portal.getPhysicalPath()) path = "{0}{1}".format(context_path, path) else: - if context is None: - err = (is_source and _(u"Alias path must start with a slash.") - or _(u"Target path must start with a slash.")) + if is_source: + err = _(u"Alias path must start with a slash.") else: - # What case should this be? - context_path = "/".join(context.getPhysicalPath()[:-1]) - path = "{0}/{1}".format(context_path, path) - if not err and not is_source: - # Check whether obj exists at source path - catalog = getToolByName(context, 'portal_catalog') - result = catalog.searchResults(path={"query": path}) - if len(result) == 0: - err = _(u"The provided target object does not exist.") - if not err and is_source: + err = _(u"Target path must start with a slash.") + if not err: + if is_source: # Check whether already exists in storage + storage = getUtility(IRedirectionStorage) if storage.get(path): err = _(u"The provided alias already exists!") + else: + # Check whether obj exists at target path + catalog = getToolByName(portal, 'portal_catalog') + result = catalog.searchResults(path={"query": path}) + if len(result) == 0: + err = _(u"The provided target object does not exist.") return path, err From 1c68fb916c11a47cb7e061cac75ca0fd4cc06403 Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Thu, 7 Feb 2019 00:34:06 +0100 Subject: [PATCH 14/34] Redirection: source must not be an existing object. --- Products/CMFPlone/controlpanel/browser/redirects.py | 8 +++++++- .../tests/test_controlpanel_browser_redirection.py | 5 ++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/Products/CMFPlone/controlpanel/browser/redirects.py b/Products/CMFPlone/controlpanel/browser/redirects.py index f3f01eb90f..9a2ff5a2c7 100644 --- a/Products/CMFPlone/controlpanel/browser/redirects.py +++ b/Products/CMFPlone/controlpanel/browser/redirects.py @@ -48,14 +48,20 @@ def absolutize_path(path, is_source=True): else: err = _(u"Target path must start with a slash.") if not err: + catalog = getToolByName(portal, 'portal_catalog') if is_source: # Check whether already exists in storage storage = getUtility(IRedirectionStorage) if storage.get(path): err = _(u"The provided alias already exists!") + else: + # Check whether obj exists at source path. + # A redirect would be useless then. + result = catalog.searchResults(path={"query": path}) + if len(result) > 0: + err = _(u"Cannot use an existing object as alias.") else: # Check whether obj exists at target path - catalog = getToolByName(portal, 'portal_catalog') result = catalog.searchResults(path={"query": path}) if len(result) == 0: err = _(u"The provided target object does not exist.") diff --git a/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py b/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py index 78f620673a..b03dd3af2f 100644 --- a/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py +++ b/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py @@ -275,11 +275,10 @@ def test_absolutize_path(self): ap('/test-folder', is_source=False), ('/plone/test-folder', None)) - # sources do not need to exist, but if an object exists, - # we currently have no problems with it. + # sources must not exist self.assertEqual( ap('/test-folder'), - ('/plone/test-folder', None)) + ('/plone/test-folder', 'Cannot use an existing object as alias.')) # A source must not already exist in the redirect list. storage = getUtility(IRedirectionStorage) From d7167e6619b7a9be97a128c1440edfc1c02b1993 Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Thu, 7 Feb 2019 00:59:39 +0100 Subject: [PATCH 15/34] Redirection: source must not be traversable. --- Products/CMFPlone/controlpanel/browser/redirects.py | 3 +++ .../tests/test_controlpanel_browser_redirection.py | 6 +++++- 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/Products/CMFPlone/controlpanel/browser/redirects.py b/Products/CMFPlone/controlpanel/browser/redirects.py index 9a2ff5a2c7..1c257a7001 100644 --- a/Products/CMFPlone/controlpanel/browser/redirects.py +++ b/Products/CMFPlone/controlpanel/browser/redirects.py @@ -60,6 +60,9 @@ def absolutize_path(path, is_source=True): result = catalog.searchResults(path={"query": path}) if len(result) > 0: err = _(u"Cannot use an existing object as alias.") + else: + if portal.unrestrictedTraverse(path, None) is not None: + err = _(u"Cannot use a working path as alias.") else: # Check whether obj exists at target path result = catalog.searchResults(path={"query": path}) diff --git a/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py b/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py index b03dd3af2f..323e6fece8 100644 --- a/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py +++ b/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py @@ -275,10 +275,14 @@ def test_absolutize_path(self): ap('/test-folder', is_source=False), ('/plone/test-folder', None)) - # sources must not exist + # A source must not exist. self.assertEqual( ap('/test-folder'), ('/plone/test-folder', 'Cannot use an existing object as alias.')) + # More general: a source must not be traversable already. + self.assertEqual( + ap('/view'), + ('/plone/view', 'Cannot use a working path as alias.')) # A source must not already exist in the redirect list. storage = getUtility(IRedirectionStorage) From 0134c5da4337a364890b884fe62603b59c1b89bd Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Thu, 7 Feb 2019 01:11:23 +0100 Subject: [PATCH 16/34] Redirection: source and target must not be a view. That is bound to lead to problems. --- .../controlpanel/browser/redirects.py | 20 +++++++++++-------- .../test_controlpanel_browser_redirection.py | 6 ++++++ 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/Products/CMFPlone/controlpanel/browser/redirects.py b/Products/CMFPlone/controlpanel/browser/redirects.py index 1c257a7001..465cd09639 100644 --- a/Products/CMFPlone/controlpanel/browser/redirects.py +++ b/Products/CMFPlone/controlpanel/browser/redirects.py @@ -38,15 +38,19 @@ def absolutize_path(path, is_source=True): err = _(u"You have to enter an alias.") else: err = _(u"You have to enter a target.") - else: - if path.startswith('/'): - context_path = "/".join(portal.getPhysicalPath()) - path = "{0}{1}".format(context_path, path) + elif not path.startswith('/'): + if is_source: + err = _(u"Alias path must start with a slash.") else: - if is_source: - err = _(u"Alias path must start with a slash.") - else: - err = _(u"Target path must start with a slash.") + err = _(u"Target path must start with a slash.") + elif '@@' in path: + if is_source: + err = _(u"Alias path must not be a view.") + else: + err = _(u"Target path must not be a view.") + else: + context_path = "/".join(portal.getPhysicalPath()) + path = "{0}{1}".format(context_path, path) if not err: catalog = getToolByName(portal, 'portal_catalog') if is_source: diff --git a/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py b/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py index 323e6fece8..394df203cc 100644 --- a/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py +++ b/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py @@ -274,6 +274,9 @@ def test_absolutize_path(self): self.assertEqual( ap('/test-folder', is_source=False), ('/plone/test-folder', None)) + self.assertEqual( + ap('/test-folder/@@sharing', is_source=False), + ('/test-folder/@@sharing', 'Target path must not be a view.')) # A source must not exist. self.assertEqual( @@ -283,6 +286,9 @@ def test_absolutize_path(self): self.assertEqual( ap('/view'), ('/plone/view', 'Cannot use a working path as alias.')) + self.assertEqual( + ap('/@@overview-controlpanel'), + ('/@@overview-controlpanel', 'Alias path must not be a view.')) # A source must not already exist in the redirect list. storage = getUtility(IRedirectionStorage) From c9850fc061434d5dc7da3fab9a41072ba384af5c Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Thu, 7 Feb 2019 11:08:15 +0100 Subject: [PATCH 17/34] Redirection: renamed alias to alternative url. --- .../browser/redirects-controlpanel.pt | 14 ++++---- .../controlpanel/browser/redirects-manage.pt | 14 ++++---- .../controlpanel/browser/redirects.py | 32 +++++++++---------- .../test_controlpanel_browser_redirection.py | 24 +++++++------- 4 files changed, 42 insertions(+), 42 deletions(-) diff --git a/Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt b/Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt index 162c5b403c..11a4472381 100644 --- a/Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt +++ b/Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt @@ -14,7 +14,7 @@
Error

- No aliases were added. Please correct these errors in your CSV file and try again: + No alternative urls were added. Please correct these errors in your CSV file and try again:

- Manage aliases + Manage alternative urls

- Add a new alias + Add a new alternative url - + @@ -83,11 +83,11 @@ enctype="multipart/form-data">
- Bulk-upload aliases + Bulk-upload alternative urls

- Add many aliases at once by uploading a CSV file. The first column should be the path to + Add many alternative urls at once by uploading a CSV file. The first column should be the path to redirect from; the second, the path to redirect to. Both paths must be Plone-site-relative, starting with a slash (/).

@@ -123,7 +123,7 @@
- All existing aliases for this site + All existing alternative urls for this site diff --git a/Products/CMFPlone/controlpanel/browser/redirects-manage.pt b/Products/CMFPlone/controlpanel/browser/redirects-manage.pt index 558e9c44ee..a1e23cf6df 100644 --- a/Products/CMFPlone/controlpanel/browser/redirects-manage.pt +++ b/Products/CMFPlone/controlpanel/browser/redirects-manage.pt @@ -13,7 +13,7 @@

- Manage aliases + Manage alternative urls

- You are managing the aliases of a default view in a container. + You are managing the alternative urls of a default view in a container. To manage them for the entire container,

- Using this form, you can manage aliases for an item. This + Using this form, you can manage alternative urls for an item. This is an easy way to make an item available under two different URLs.

@@ -45,7 +45,7 @@ method="post">
- Existing aliases for this item + Existing alternative urls for this item
- Alias + Alternative url
- Enter the absolute path where the alias should exist. The path must start with '/'. + Enter the absolute path where the alternative url should exist. The path must start with '/'.
- Existing aliases for this item + Existing alternative urls for this item diff --git a/Products/CMFPlone/controlpanel/browser/redirects.py b/Products/CMFPlone/controlpanel/browser/redirects.py index 465cd09639..37f0ade787 100644 --- a/Products/CMFPlone/controlpanel/browser/redirects.py +++ b/Products/CMFPlone/controlpanel/browser/redirects.py @@ -35,17 +35,17 @@ def absolutize_path(path, is_source=True): err = None if not path: if is_source: - err = _(u"You have to enter an alias.") + err = _(u"You have to enter an alternative url.") else: err = _(u"You have to enter a target.") elif not path.startswith('/'): if is_source: - err = _(u"Alias path must start with a slash.") + err = _(u"Alternative url path must start with a slash.") else: err = _(u"Target path must start with a slash.") elif '@@' in path: if is_source: - err = _(u"Alias path must not be a view.") + err = _(u"Alternative url path must not be a view.") else: err = _(u"Target path must not be a view.") else: @@ -57,16 +57,16 @@ def absolutize_path(path, is_source=True): # Check whether already exists in storage storage = getUtility(IRedirectionStorage) if storage.get(path): - err = _(u"The provided alias already exists!") + err = _(u"The provided alternative url already exists!") else: # Check whether obj exists at source path. # A redirect would be useless then. result = catalog.searchResults(path={"query": path}) if len(result) > 0: - err = _(u"Cannot use an existing object as alias.") + err = _(u"Cannot use an existing object as alternative url.") else: if portal.unrestrictedTraverse(path, None) is not None: - err = _(u"Cannot use a working path as alias.") + err = _(u"Cannot use a working path as alternative url.") else: # Check whether obj exists at target path result = catalog.searchResults(path={"query": path}) @@ -106,15 +106,15 @@ def __call__(self): else: del form['redirection'] storage.add(redirection, "/".join(self.context.getPhysicalPath())) - status.addStatusMessage(_(u"Alias added."), type='info') + status.addStatusMessage(_(u"Alternative url added."), type='info') elif 'form.button.Remove' in form: redirects = form.get('redirects', ()) for redirect in redirects: storage.remove(redirect) if len(redirects) > 1: - status.addStatusMessage(_(u"Aliases removed."), type='info') + status.addStatusMessage(_(u"Alternative urls removed."), type='info') else: - status.addStatusMessage(_(u"Alias removed."), type='info') + status.addStatusMessage(_(u"Alternative url removed."), type='info') return self.index(errors=errors) @@ -209,11 +209,11 @@ def __call__(self): for redirect in redirects: storage.remove(redirect) if len(redirects) == 0: - status.addStatusMessage(_(u"No aliases selected for removal."), type='info') + status.addStatusMessage(_(u"No alternative urls selected for removal."), type='info') elif len(redirects) > 1: - status.addStatusMessage(_(u"Aliases removed."), type='info') + status.addStatusMessage(_(u"Alternative urls removed."), type='info') else: - status.addStatusMessage(_(u"Alias removed."), type='info') + status.addStatusMessage(_(u"Alternative url removed."), type='info') elif 'form.button.Add' in form: self.add(form['redirection'], form['target_path'], portal, storage, status) elif 'form.button.Upload' in form: @@ -236,7 +236,7 @@ def add(self, redirection, target, portal, storage, status): err = target_err else: if abs_redirection == abs_target: - err = _(u"Aliases that point to themselves will cause" + err = _(u"Alternative urls that point to themselves will cause" u"an endless cycle of redirects.") # TODO: detect indirect recursion @@ -244,7 +244,7 @@ def add(self, redirection, target, portal, storage, status): status.addStatusMessage(_(err), type='error') else: storage.add(abs_redirection, abs_target) - status.addStatusMessage(_(u"Alias {0} → {1} added.").format(abs_redirection, abs_target), + status.addStatusMessage(_(u"Alternative url {0} → {1} added.").format(abs_redirection, abs_target), type='info') def upload(self, file, portal, storage, status): @@ -277,7 +277,7 @@ def upload(self, file, portal, storage, status): else: if abs_redirection == abs_target: # TODO: detect indirect recursion - err = _(u"Aliases that point to themselves will cause" + err = _(u"Alternative urls that point to themselves will cause" u"an endless cycle of redirects.") else: err = _(u"Each line must have 2 columns.") @@ -293,7 +293,7 @@ def upload(self, file, portal, storage, status): if not had_errors: for abs_redirection, abs_target in successes: storage.add(abs_redirection, abs_target) - status.addStatusMessage(_(u"%i aliases added.") % len(successes), type='info') + status.addStatusMessage(_(u"%i alternative urls added.") % len(successes), type='info') @memoize def view_url(self): diff --git a/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py b/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py index 394df203cc..154734f98a 100644 --- a/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py +++ b/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py @@ -133,11 +133,11 @@ def test_redirection_controlpanel_redirect_alias_exists(self): self.assertTrue( storage.get(storage_alias) == storage_target, - '{0} not target of alias!'.format(storage_target) + '{0} not target of alternative url!'.format(storage_target) ) self.assertTrue( - 'The provided alias already exists!' in self.browser.contents, - u'Message "alias already exists" not in page!' + 'The provided alternative url already exists!' in self.browser.contents, + u'Message "alternative url already exists" not in page!' ) def test_redirection_controlpanel_filtering(self): @@ -234,8 +234,8 @@ def test_redirection_controlpanel_missing_slash_alias(self): self.browser.getControl(name='form.button.Add').click() self.assertTrue( - 'Alias path must start with a slash.' in self.browser.contents, - u'Errormessage for missing slash on alias path missing' + 'Alternative url path must start with a slash.' in self.browser.contents, + u'Errormessage for missing slash on alternative url missing' ) def test_absolutize_path(self): @@ -247,7 +247,7 @@ def test_absolutize_path(self): # A path is required. self.assertEqual( ap(''), - ('', 'You have to enter an alias.')) + ('', 'You have to enter an alternative url.')) self.assertEqual( ap('', is_source=False), ('', 'You have to enter a target.')) @@ -255,10 +255,10 @@ def test_absolutize_path(self): # relative paths are not accepted self.assertEqual( ap('foo'), - ('foo', 'Alias path must start with a slash.')) + ('foo', 'Alternative url path must start with a slash.')) self.assertEqual( ap('foo', is_source=True), - ('foo', 'Alias path must start with a slash.')) + ('foo', 'Alternative url path must start with a slash.')) self.assertEqual( ap('foo', is_source=False), ('foo', 'Target path must start with a slash.')) @@ -281,14 +281,14 @@ def test_absolutize_path(self): # A source must not exist. self.assertEqual( ap('/test-folder'), - ('/plone/test-folder', 'Cannot use an existing object as alias.')) + ('/plone/test-folder', 'Cannot use an existing object as alternative url.')) # More general: a source must not be traversable already. self.assertEqual( ap('/view'), - ('/plone/view', 'Cannot use a working path as alias.')) + ('/plone/view', 'Cannot use a working path as alternative url.')) self.assertEqual( ap('/@@overview-controlpanel'), - ('/@@overview-controlpanel', 'Alias path must not be a view.')) + ('/@@overview-controlpanel', 'Alternative url path must not be a view.')) # A source must not already exist in the redirect list. storage = getUtility(IRedirectionStorage) @@ -297,4 +297,4 @@ def test_absolutize_path(self): '{0:s}/test-folder'.format(portal_path)) self.assertEqual( ap('/foo', is_source=True), - ('/plone/foo', 'The provided alias already exists!')) + ('/plone/foo', 'The provided alternative url already exists!')) From 81f8c91c23d4d99bb7a0bd1c8ebd690645c357ed Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Thu, 7 Feb 2019 11:12:37 +0100 Subject: [PATCH 18/34] Redirection: ran isort and black over files. --- .../controlpanel/browser/redirects.py | 121 ++++++--- .../test_controlpanel_browser_redirection.py | 240 ++++++++++-------- 2 files changed, 215 insertions(+), 146 deletions(-) diff --git a/Products/CMFPlone/controlpanel/browser/redirects.py b/Products/CMFPlone/controlpanel/browser/redirects.py index 37f0ade787..d264800971 100644 --- a/Products/CMFPlone/controlpanel/browser/redirects.py +++ b/Products/CMFPlone/controlpanel/browser/redirects.py @@ -13,6 +13,7 @@ import csv + _ = MessageFactory('plone') @@ -63,10 +64,14 @@ def absolutize_path(path, is_source=True): # A redirect would be useless then. result = catalog.searchResults(path={"query": path}) if len(result) > 0: - err = _(u"Cannot use an existing object as alternative url.") + err = _( + u"Cannot use an existing object as alternative url." + ) else: if portal.unrestrictedTraverse(path, None) is not None: - err = _(u"Cannot use a working path as alternative url.") + err = _( + u"Cannot use a working path as alternative url." + ) else: # Check whether obj exists at target path result = catalog.searchResults(path={"query": path}) @@ -77,7 +82,6 @@ def absolutize_path(path, is_source=True): class RedirectsView(BrowserView): - def redirects(self): storage = getUtility(IRedirectionStorage) portal = getUtility(ISiteRoot) @@ -85,11 +89,8 @@ def redirects(self): portal_path = "/".join(portal.getPhysicalPath()) redirects = storage.redirects(context_path) for redirect in redirects: - path = redirect[len(portal_path):] - yield { - 'redirect': redirect, - 'path': path, - } + path = redirect[len(portal_path) :] + yield {'redirect': redirect, 'path': path} def __call__(self): storage = getUtility(IRedirectionStorage) @@ -99,22 +100,32 @@ def __call__(self): errors = {} if 'form.button.Add' in form: - redirection, err = absolutize_path(form.get('redirection'), is_source=True) + redirection, err = absolutize_path( + form.get('redirection'), is_source=True + ) if err: errors['redirection'] = err status.addStatusMessage(err, type='error') else: del form['redirection'] - storage.add(redirection, "/".join(self.context.getPhysicalPath())) - status.addStatusMessage(_(u"Alternative url added."), type='info') + storage.add( + redirection, "/".join(self.context.getPhysicalPath()) + ) + status.addStatusMessage( + _(u"Alternative url added."), type='info' + ) elif 'form.button.Remove' in form: redirects = form.get('redirects', ()) for redirect in redirects: storage.remove(redirect) if len(redirects) > 1: - status.addStatusMessage(_(u"Alternative urls removed."), type='info') + status.addStatusMessage( + _(u"Alternative urls removed."), type='info' + ) else: - status.addStatusMessage(_(u"Alternative url removed."), type='info') + status.addStatusMessage( + _(u"Alternative url removed."), type='info' + ) return self.index(errors=errors) @@ -149,30 +160,26 @@ def __len__(self): def __getitem__(self, item): redirect = self.data[item] if redirect.startswith(self.portal_path): - path = redirect[self.portal_path_len:] + path = redirect[self.portal_path_len :] else: path = redirect redirect_to = self.storage.get(redirect) if redirect_to.startswith(self.portal_path): - redirect_to = redirect_to[self.portal_path_len:] - return { - 'redirect': redirect, - 'path': path, - 'redirect-to': redirect_to, - } + redirect_to = redirect_to[self.portal_path_len :] + return {'redirect': redirect, 'path': path, 'redirect-to': redirect_to} class RedirectsBatchView(PloneBatchView): def make_link(self, pagenumber=None, omit_params=None): if omit_params is None: omit_params = ['ajax_load'] - url = super(RedirectsBatchView, self).make_link(pagenumber, - omit_params) + url = super(RedirectsBatchView, self).make_link( + pagenumber, omit_params + ) return u'{0:s}#manage-existing-aliases'.format(url) class RedirectsControlPanel(BrowserView): - def __init__(self, context, request): super(RedirectsControlPanel, self).__init__(context, request) self.errors = [] @@ -194,7 +201,7 @@ def redirects(self): RedirectionSet(self.request.form.get('q', '')), 15, int(self.request.form.get('b_start', '0')), - orphan=1 + orphan=1, ) def __call__(self): @@ -209,13 +216,26 @@ def __call__(self): for redirect in redirects: storage.remove(redirect) if len(redirects) == 0: - status.addStatusMessage(_(u"No alternative urls selected for removal."), type='info') + status.addStatusMessage( + _(u"No alternative urls selected for removal."), + type='info', + ) elif len(redirects) > 1: - status.addStatusMessage(_(u"Alternative urls removed."), type='info') + status.addStatusMessage( + _(u"Alternative urls removed."), type='info' + ) else: - status.addStatusMessage(_(u"Alternative url removed."), type='info') + status.addStatusMessage( + _(u"Alternative url removed."), type='info' + ) elif 'form.button.Add' in form: - self.add(form['redirection'], form['target_path'], portal, storage, status) + self.add( + form['redirection'], + form['target_path'], + portal, + storage, + status, + ) elif 'form.button.Upload' in form: self.upload(form['file'], portal, storage, status) @@ -236,23 +256,31 @@ def add(self, redirection, target, portal, storage, status): err = target_err else: if abs_redirection == abs_target: - err = _(u"Alternative urls that point to themselves will cause" - u"an endless cycle of redirects.") + err = _( + u"Alternative urls that point to themselves will cause" + u"an endless cycle of redirects." + ) # TODO: detect indirect recursion if err: status.addStatusMessage(_(err), type='error') else: storage.add(abs_redirection, abs_target) - status.addStatusMessage(_(u"Alternative url {0} → {1} added.").format(abs_redirection, abs_target), - type='info') + status.addStatusMessage( + _(u"Alternative url {0} → {1} added.").format( + abs_redirection, abs_target + ), + type='info', + ) def upload(self, file, portal, storage, status): """Add the redirections from the CSV file `file`. If anything goes wrong, do nothing.""" # No file picked. Theres gotta be a better way to handle this. if not file.filename: - status.addStatusMessage(_(u"Please pick a file to upload."), type='info') + status.addStatusMessage( + _(u"Please pick a file to upload."), type='info' + ) return # Turn all kinds of newlines into LF ones. The csv module doesn't do # its own newline sniffing and requires either \n or \r. @@ -268,8 +296,12 @@ def upload(self, file, portal, storage, status): for i, fields in enumerate(csv.reader(file, dialect)): if len(fields) == 2: redirection, target = fields - abs_redirection, err = absolutize_path(redirection, is_source=True) - abs_target, target_err = absolutize_path(target, is_source=False) + abs_redirection, err = absolutize_path( + redirection, is_source=True + ) + abs_target, target_err = absolutize_path( + target, is_source=False + ) if err and target_err: err = "%s %s" % (err, target_err) # sloppy w.r.t. i18n elif target_err: @@ -277,8 +309,10 @@ def upload(self, file, portal, storage, status): else: if abs_redirection == abs_target: # TODO: detect indirect recursion - err = _(u"Alternative urls that point to themselves will cause" - u"an endless cycle of redirects.") + err = _( + u"Alternative urls that point to themselves will cause" + u"an endless cycle of redirects." + ) else: err = _(u"Each line must have 2 columns.") @@ -287,13 +321,20 @@ def upload(self, file, portal, storage, status): successes.append((abs_redirection, abs_target)) else: had_errors = True - self.errors.append(dict(line_number=i + 1, line=dialect.delimiter.join(fields), - message=err)) + self.errors.append( + dict( + line_number=i + 1, + line=dialect.delimiter.join(fields), + message=err, + ) + ) if not had_errors: for abs_redirection, abs_target in successes: storage.add(abs_redirection, abs_target) - status.addStatusMessage(_(u"%i alternative urls added.") % len(successes), type='info') + status.addStatusMessage( + _(u"%i alternative urls added.") % len(successes), type='info' + ) @memoize def view_url(self): diff --git a/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py b/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py index 154734f98a..c614d13ef2 100644 --- a/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py +++ b/Products/CMFPlone/controlpanel/tests/test_controlpanel_browser_redirection.py @@ -1,19 +1,18 @@ # -*- coding: utf-8 -*- -from Products.CMFPlone.PloneBatch import Batch -from Products.CMFPlone.controlpanel.browser.redirects import RedirectionSet -from plone.app.testing import SITE_OWNER_NAME, SITE_OWNER_PASSWORD from plone.app.redirector.interfaces import IRedirectionStorage +from plone.app.testing import SITE_OWNER_NAME +from plone.app.testing import SITE_OWNER_PASSWORD from plone.testing.z2 import Browser - +from Products.CMFPlone.controlpanel.browser.redirects import RedirectionSet +from Products.CMFPlone.PloneBatch import Batch +from Products.CMFPlone.testing import PRODUCTS_CMFPLONE_FUNCTIONAL_TESTING from zope.component import getMultiAdapter from zope.component import getUtility -from Products.CMFPlone.testing import \ - PRODUCTS_CMFPLONE_FUNCTIONAL_TESTING - import math -import unittest import transaction +import unittest + class RedirectionControlPanelFunctionalTest(unittest.TestCase): """Test that changes in the mail control panel are actually @@ -30,7 +29,7 @@ def setUp(self): self.browser.handleErrors = False self.browser.addHeader( 'Authorization', - 'Basic %s:%s' % (SITE_OWNER_NAME, SITE_OWNER_PASSWORD,) + 'Basic %s:%s' % (SITE_OWNER_NAME, SITE_OWNER_PASSWORD), ) """ @@ -54,8 +53,9 @@ def test_mail_controlpanel_sidebar(self): """ def test_redirection_controlpanel_view(self): - view = getMultiAdapter((self.portal, self.portal.REQUEST), - name="redirection-controlpanel") + view = getMultiAdapter( + (self.portal, self.portal.REQUEST), name="redirection-controlpanel" + ) self.assertTrue(view()) def test_redirection_controlpanel_add_redirect(self): @@ -64,49 +64,64 @@ def test_redirection_controlpanel_add_redirect(self): target_path = '/test-folder' storage_path = '/plone/alias-folder' - self.browser.open( - "%s/@@redirection-controlpanel" % self.portal_url) - self.browser.getControl( - name='redirection').value = redirection_path - self.browser.getControl( - name='target_path').value = target_path + self.browser.open("%s/@@redirection-controlpanel" % self.portal_url) + self.browser.getControl(name='redirection').value = redirection_path + self.browser.getControl(name='target_path').value = target_path self.browser.getControl(name='form.button.Add').click() self.assertTrue( storage.has_path(storage_path), - u'Redirection storage should have path "{0}"'.format(storage_path) + u'Redirection storage should have path "{0}"'.format(storage_path), ) def test_redirection_controlpanel_set(self): storage = getUtility(IRedirectionStorage) portal_path = self.layer['portal'].absolute_url_path() for i in range(1000): - storage.add('{0:s}/foo/{1:s}'.format(portal_path, str(i)), - '{0:s}/bar/{1:s}'.format(portal_path, str(i))) + storage.add( + '{0:s}/foo/{1:s}'.format(portal_path, str(i)), + '{0:s}/bar/{1:s}'.format(portal_path, str(i)), + ) redirects = RedirectionSet() self.assertEqual(len(redirects), 1000) - self.assertDictEqual(redirects[0], { - 'redirect': '{0:s}/foo/0'.format(portal_path), - 'path': '/foo/0', 'redirect-to': '/bar/0' - }) - self.assertDictEqual(redirects[999], { - 'redirect': '{0:s}/foo/999'.format(portal_path), - 'path': '/foo/999', 'redirect-to': '/bar/999' - }) + self.assertDictEqual( + redirects[0], + { + 'redirect': '{0:s}/foo/0'.format(portal_path), + 'path': '/foo/0', + 'redirect-to': '/bar/0', + }, + ) + self.assertDictEqual( + redirects[999], + { + 'redirect': '{0:s}/foo/999'.format(portal_path), + 'path': '/foo/999', + 'redirect-to': '/bar/999', + }, + ) self.assertEqual(len(list(iter(redirects))), 1000) - self.assertDictEqual(list(iter(redirects))[0], { - 'redirect': '{0:s}/foo/0'.format(portal_path), - 'path': '/foo/0', 'redirect-to': '/bar/0' - }) + self.assertDictEqual( + list(iter(redirects))[0], + { + 'redirect': '{0:s}/foo/0'.format(portal_path), + 'path': '/foo/0', + 'redirect-to': '/bar/0', + }, + ) def test_redirection_controlpanel_batching(self): storage = getUtility(IRedirectionStorage) portal_path = self.layer['portal'].absolute_url_path() for i in range(1000): - storage.add('{0:s}/foo/{1:s}'.format(portal_path, str(i)), - '{0:s}/bar/{1:s}'.format(portal_path, str(i))) - view = getMultiAdapter((self.layer['portal'], self.layer['request']), - name='redirection-controlpanel') + storage.add( + '{0:s}/foo/{1:s}'.format(portal_path, str(i)), + '{0:s}/bar/{1:s}'.format(portal_path, str(i)), + ) + view = getMultiAdapter( + (self.layer['portal'], self.layer['request']), + name='redirection-controlpanel', + ) # Test that view/redirects returns batch self.assertIsInstance(view.redirects(), Batch) @@ -123,32 +138,34 @@ def test_redirection_controlpanel_redirect_alias_exists(self): storage.add(storage_alias, storage_target) transaction.commit() - self.browser.open( - "%s/@@redirection-controlpanel" % self.portal_url) - self.browser.getControl( - name='redirection').value = path_alias - self.browser.getControl( - name='target_path').value = path_target + self.browser.open("%s/@@redirection-controlpanel" % self.portal_url) + self.browser.getControl(name='redirection').value = path_alias + self.browser.getControl(name='target_path').value = path_target self.browser.getControl(name='form.button.Add').click() self.assertTrue( storage.get(storage_alias) == storage_target, - '{0} not target of alternative url!'.format(storage_target) + '{0} not target of alternative url!'.format(storage_target), ) self.assertTrue( - 'The provided alternative url already exists!' in self.browser.contents, - u'Message "alternative url already exists" not in page!' + 'The provided alternative url already exists!' + in self.browser.contents, + u'Message "alternative url already exists" not in page!', ) def test_redirection_controlpanel_filtering(self): storage = getUtility(IRedirectionStorage) portal_path = self.layer['portal'].absolute_url_path() for i in range(1000): - storage.add('{0:s}/foo1/{1:s}'.format(portal_path, str(i)), - '{0:s}/bar/{1:s}'.format(portal_path, str(i))) + storage.add( + '{0:s}/foo1/{1:s}'.format(portal_path, str(i)), + '{0:s}/bar/{1:s}'.format(portal_path, str(i)), + ) for i in range(1000): - storage.add('{0:s}/foo2/{1:s}'.format(portal_path, str(i)), - '{0:s}/bar/{1:s}'.format(portal_path, str(i))) + storage.add( + '{0:s}/foo2/{1:s}'.format(portal_path, str(i)), + '{0:s}/bar/{1:s}'.format(portal_path, str(i)), + ) redirects = RedirectionSet() self.assertEqual(len(redirects), 2000) @@ -161,107 +178,104 @@ def test_redirection_controlpanel_filtering(self): request = self.layer['request'].clone() request.form['q'] = '/foo' - view = getMultiAdapter((self.layer['portal'], request), - name='redirection-controlpanel') - self.assertEqual(view.redirects().numpages, math.ceil(2000 / 15.)) + view = getMultiAdapter( + (self.layer['portal'], request), name='redirection-controlpanel' + ) + self.assertEqual(view.redirects().numpages, math.ceil(2000 / 15.0)) request = self.layer['request'].clone() request.form['q'] = '/foo1' - view = getMultiAdapter((self.layer['portal'], request), - name='redirection-controlpanel') - self.assertEqual(view.redirects().numpages, math.ceil(1000 / 15.)) + view = getMultiAdapter( + (self.layer['portal'], request), name='redirection-controlpanel' + ) + self.assertEqual(view.redirects().numpages, math.ceil(1000 / 15.0)) request = self.layer['request'].clone() request.form['q'] = '/foo2' - view = getMultiAdapter((self.layer['portal'], request), - name='redirection-controlpanel') - self.assertEqual(view.redirects().numpages, math.ceil(1000 / 15.)) + view = getMultiAdapter( + (self.layer['portal'], request), name='redirection-controlpanel' + ) + self.assertEqual(view.redirects().numpages, math.ceil(1000 / 15.0)) request = self.layer['request'].clone() - view = getMultiAdapter((self.layer['portal'], request), - name='redirection-controlpanel') - self.assertEqual(view.redirects().numpages, math.ceil(2000 / 15.)) + view = getMultiAdapter( + (self.layer['portal'], request), name='redirection-controlpanel' + ) + self.assertEqual(view.redirects().numpages, math.ceil(2000 / 15.0)) # Filtering without new request does not have effect because memoize request.form['q'] = '/foo2' - self.assertEqual(view.redirects().numpages, math.ceil(2000 / 15.)) + self.assertEqual(view.redirects().numpages, math.ceil(2000 / 15.0)) def test_redirection_controlpanel_redirect_no_target(self): path_alias = '/alias' path_target = '/not-existing' - self.browser.open( - "%s/@@redirection-controlpanel" % self.portal_url) - self.browser.getControl( - name='redirection').value = path_alias - self.browser.getControl( - name='target_path').value = path_target + self.browser.open("%s/@@redirection-controlpanel" % self.portal_url) + self.browser.getControl(name='redirection').value = path_alias + self.browser.getControl(name='target_path').value = path_target self.browser.getControl(name='form.button.Add').click() self.assertTrue( - 'The provided target object does not exist.' in self.browser.contents, - u'Message "target does not exist" not in page!' + 'The provided target object does not exist.' + in self.browser.contents, + u'Message "target does not exist" not in page!', ) def test_redirection_controlpanel_missing_slash_target(self): path_alias = '/alias' path_target = 'Members' - self.browser.open( - "%s/@@redirection-controlpanel" % self.portal_url) - self.browser.getControl( - name='redirection').value = path_alias - self.browser.getControl( - name='target_path').value = path_target + self.browser.open("%s/@@redirection-controlpanel" % self.portal_url) + self.browser.getControl(name='redirection').value = path_alias + self.browser.getControl(name='target_path').value = path_target self.browser.getControl(name='form.button.Add').click() self.assertTrue( 'Target path must start with a slash.' in self.browser.contents, - u'Errormessage for missing slash on target path missing' + u'Errormessage for missing slash on target path missing', ) - def test_redirection_controlpanel_missing_slash_alias(self): path_alias = 'alias' path_target = '/Members' - self.browser.open( - "%s/@@redirection-controlpanel" % self.portal_url) - self.browser.getControl( - name='redirection').value = path_alias - self.browser.getControl( - name='target_path').value = path_target + self.browser.open("%s/@@redirection-controlpanel" % self.portal_url) + self.browser.getControl(name='redirection').value = path_alias + self.browser.getControl(name='target_path').value = path_target self.browser.getControl(name='form.button.Add').click() self.assertTrue( - 'Alternative url path must start with a slash.' in self.browser.contents, - u'Errormessage for missing slash on alternative url missing' + 'Alternative url path must start with a slash.' + in self.browser.contents, + u'Errormessage for missing slash on alternative url missing', ) def test_absolutize_path(self): # absolutize_path is a helper function that returns a tuple # of absolute path and error message. from Products.CMFPlone.controlpanel.browser.redirects import ( - absolutize_path as ap) + absolutize_path as ap, + ) # A path is required. + self.assertEqual(ap(''), ('', 'You have to enter an alternative url.')) self.assertEqual( - ap(''), - ('', 'You have to enter an alternative url.')) - self.assertEqual( - ap('', is_source=False), - ('', 'You have to enter a target.')) + ap('', is_source=False), ('', 'You have to enter a target.') + ) # relative paths are not accepted self.assertEqual( - ap('foo'), - ('foo', 'Alternative url path must start with a slash.')) + ap('foo'), ('foo', 'Alternative url path must start with a slash.') + ) self.assertEqual( ap('foo', is_source=True), - ('foo', 'Alternative url path must start with a slash.')) + ('foo', 'Alternative url path must start with a slash.'), + ) self.assertEqual( ap('foo', is_source=False), - ('foo', 'Target path must start with a slash.')) + ('foo', 'Target path must start with a slash.'), + ) # absolute paths are good self.assertEqual(ap('/foo'), ('/plone/foo', None)) @@ -270,31 +284,45 @@ def test_absolutize_path(self): # for targets, an object must exist on the path self.assertEqual( ap('/foo', is_source=False), - ('/plone/foo', 'The provided target object does not exist.')) + ('/plone/foo', 'The provided target object does not exist.'), + ) self.assertEqual( - ap('/test-folder', is_source=False), - ('/plone/test-folder', None)) + ap('/test-folder', is_source=False), ('/plone/test-folder', None) + ) self.assertEqual( ap('/test-folder/@@sharing', is_source=False), - ('/test-folder/@@sharing', 'Target path must not be a view.')) + ('/test-folder/@@sharing', 'Target path must not be a view.'), + ) # A source must not exist. self.assertEqual( ap('/test-folder'), - ('/plone/test-folder', 'Cannot use an existing object as alternative url.')) + ( + '/plone/test-folder', + 'Cannot use an existing object as alternative url.', + ), + ) # More general: a source must not be traversable already. self.assertEqual( ap('/view'), - ('/plone/view', 'Cannot use a working path as alternative url.')) + ('/plone/view', 'Cannot use a working path as alternative url.'), + ) self.assertEqual( ap('/@@overview-controlpanel'), - ('/@@overview-controlpanel', 'Alternative url path must not be a view.')) + ( + '/@@overview-controlpanel', + 'Alternative url path must not be a view.', + ), + ) # A source must not already exist in the redirect list. storage = getUtility(IRedirectionStorage) portal_path = self.layer['portal'].absolute_url_path() - storage.add('{0:s}/foo'.format(portal_path), - '{0:s}/test-folder'.format(portal_path)) + storage.add( + '{0:s}/foo'.format(portal_path), + '{0:s}/test-folder'.format(portal_path), + ) self.assertEqual( ap('/foo', is_source=True), - ('/plone/foo', 'The provided alternative url already exists!')) + ('/plone/foo', 'The provided alternative url already exists!'), + ) From 757d93e9571088d8ec6b698dd8848b9719f3552b Mon Sep 17 00:00:00 2001 From: Maurits van Rees Date: Thu, 7 Feb 2019 11:18:12 +0100 Subject: [PATCH 19/34] Redirection: use 'URL Management' as title. --- .../CMFPlone/controlpanel/browser/redirects-controlpanel.pt | 2 +- Products/CMFPlone/controlpanel/browser/redirects-manage.pt | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt b/Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt index 11a4472381..7631a19759 100644 --- a/Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt +++ b/Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt @@ -44,7 +44,7 @@

- Manage alternative urls + URL Management

- Manage alternative urls + URL Management
Date: Thu, 7 Feb 2019 11:35:15 +0100 Subject: [PATCH 20/34] Redirection: let both add alias forms look more like each other. The view on the context had nice help text, which was missing in the controlpanel. Use more same labels. --- .../browser/redirects-controlpanel.pt | 42 +++++++++++++++++-- .../controlpanel/browser/redirects-manage.pt | 8 ++-- .../controlpanel/browser/redirects.py | 12 +++++- 3 files changed, 53 insertions(+), 9 deletions(-) diff --git a/Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt b/Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt index 7631a19759..9156780547 100644 --- a/Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt +++ b/Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt @@ -55,15 +55,51 @@ Add a new alternative url - + + + + (Required) + + +
+ Enter the absolute path where the alternative url should exist. The path must start with '/'. +
+ + name="redirection" + value="#" + size="40" + tal:attributes="value request/redirection | nothing;"/> + + + (Required) + + +
+ Enter the absolute path of the target. The path must start with '/'. +
+ + name="target_path" + value="#" + size="40" + tal:attributes="value request/target_path | nothing;"/>
- - Existing alternative urls for this item + + Add a new alternative url
Date: Thu, 7 Feb 2019 12:09:40 +0100 Subject: [PATCH 21/34] Redirection: improved form error handling. Show the errors in the form fields, instead of only at the top of the form. --- .../browser/redirects-controlpanel.pt | 50 +++++++++++++++++-- .../controlpanel/browser/redirects-manage.pt | 3 +- .../controlpanel/browser/redirects.py | 36 +++++++------ 3 files changed, 65 insertions(+), 24 deletions(-) diff --git a/Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt b/Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt index 9156780547..827b549a68 100644 --- a/Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt +++ b/Products/CMFPlone/controlpanel/browser/redirects-controlpanel.pt @@ -9,7 +9,7 @@
Error
@@ -21,7 +21,7 @@ border="0" cellpadding="2" cellspacing="0"> -
@@ -55,6 +55,10 @@ Add a new alternative url +
+
+ +
@@ -90,6 +103,10 @@ (Required) +
+ Validation error output +
+
Enter the absolute path of the target. The path must start with '/'.
@@ -100,6 +117,7 @@ value="#" size="40" tal:attributes="value request/target_path | nothing;"/> +

- + + + + (Required) + + +
+ Validation error output +
+ +
+
+