search

plone / Products.CMFPlone

The core of the Plone content management system
https://plone.org
GNU General Public License v2.0
240 stars 183 forks source link

PLIP: Merge Products.RedirectionTool into core #1486

Closed vangheem closed 5 years ago

vangheem commented 8 years ago

Proposer : vangheem

Seconder : ?

Abstract

Allow users to manage redirects on their site and aliases to content.

Motivation

Managing aliases and redirects on your site is a very important feature that should be handled OOTB.

Assumptions

Proposal & Implementation

djay commented 8 years ago

yes please. I added a risk about "Current use of the word "Alias" in redirection tool is confusing". Most of my users when asking for this talk about creating short urls, extra urls, alternative urls for content.

Also I suspect the UI for this could be combined with the UI for adjusting the shortname. Currently the UI for changing the short-name is buried in rename, which is also not that intuitive. It makes sense if you get the folder metaphor, but people coming at CMS from a public website point of view are thinking of adjusting their urls.

gforcada commented 8 years ago

An extra bonus for me would be to be able to create external redirects from within the control panel, so this way our marketing team could handle that from a central place. Right now, although external URLs are supported, they can not be created TTW.

datakurre commented 8 years ago

:+1: and :+1: for @gforcada's addition

ebrehault commented 8 years ago

I think "Current toplevel toolbar item adds extra noise to the toolbar" should not be listed as a risk. It should be part of the changes we need to implement in order to merge it in core. I update the PLIP accordingly.

thet commented 8 years ago

@gforcada in another project I'm using the Plone Link content type for external redirects, where I used nginx or apache redirects before. If you mean that. However, +1 for this PLIP.

gforcada commented 8 years ago

@thet yes I know, but my use case is more about bulk updates rather than one at a time. So having an overview where to see all redirects and being able to upload a mapping that creates them automatically would be a real time saver for us.

ebrehault commented 8 years ago

The PLIP has been approved. See https://community.plone.org/t/framework-team-meeting-minutes-2016-04-12/1943 Regarding implementation: we have concerns with the current UI. So it need to be polished (we also mentionned it might be simpler to rewrite it).

fredvd commented 8 years ago

There is a usability issue with exposing/managing the redirection storage to end users which I seen a few times in the past, curious if others have the same experience. Or did this already change in Plone 5?

There is no distinction between manually added redirects by users and automatically added redirects when you move/rename content. In a large site you would want to clean up those automatic redirects after a certain period because they clutter te redirect list, sometimes also cause confusion, etc.

But as soon as you open the redirects to end users there are also explicit redirects that you shouldn't delete or at least be able to distinguish from.

For me this is a reason not to install redirectiontool by default, only when users request it and I can explain this caveat.

It would be great as a first step if the stored aliases/redirects would have a datetime atrribute and a label to indicate if the redirect as created automatically by the system or through redirectiontool by the user.

tomgross commented 8 years ago

I think rewriting the product might be the better option:

Testing the product I found a severe usability problem: If a redirect exists and another redirect is created with the same name no feedback is issued. I would expect a message: There is a redirect with this name/URL already. You have the following options: Use new redirect, Keep current redirect

Rudd-O commented 7 years ago

This is going to be so awesome. Thank you folks!

svx commented 7 years ago

@staeff @datakurre

Nice work ! Thanks !

Would you mind to update the docs too ?

Since there is some UI involved a robot test for screen-shots would be super duper excellent uber superlecker awesome !

Thanks a lot !!!!

staeff commented 7 years ago

Hi @svx,

thanks for the heads up! I had it on the list to document it. Question: I was skimming the docs to see where it would fit, but I did not find a place. I was looking for a section where all the other controlpanels are documented. Did I miss to find it? If not, any suggestions where to put it?

svx commented 7 years ago

@staeff good point !

When it get merged into CMFPlone it should go into the /docs dir of the repo.

Currently the /docs repo of CMFPlone is using 'old style' and not following best practices.

IMHO it's time to change that :)

Long answer short put into /docs of CMFPlone, and after that I will create a PR which cleans up /docs and moves files to places where they belong.

Later we will 'fetch' /docs and will place it under the other docs on docs.plone.org, IMHO this should go under 'working with content'

@polyester what do you think ?

datakurre commented 6 years ago

@staeff Would you able to create a checklist of missing parts, and possibly also create a WIP pull request for this? Obviously we missed Plone 5.1, but maybe we could get this merged soon after 5.1 final is out.

datakurre commented 6 years ago

https://github.com/plone/Products.CMFPlone/commits/plip-1486-redirection

svx commented 6 years ago

@staeff @datakurre is there something I can help with ?

I really need/want that thing :)

staeff commented 6 years ago

@svx @dataflake I have to apologize, I am the bottle neck here. I will give an update on the state of the work and make a WIP pr this weekend.

staeff commented 6 years ago

@svx @datakurre I made a WIP pull request now. There are some tiny things that could be improved, as I see it. I'd be glad to get your opinions and questions.

staeff commented 6 years ago

@svx here is a list of things, that are needed or would be nice to have from my perspective.

This is one is needed for der Freitag:

svx commented 6 years ago

@staeff I can take care of the docs, no problem :)

djay commented 6 years ago

Was there any update on changing the word alias? I see no reference to this change.

djay commented 6 years ago

call it "url management".

staeff commented 6 years ago

@djay okay.

djay commented 5 years ago

One other UX issue is around alternate urls that no longer work because a new item has been created at that url. This can be very confusing and hard to work out. Worse is when urls won't work due to acquisition. e.g. /plone. The control panel should probably warn the user redirections are ineffective as no 404 event gets generated. Possibly this is new feature after the merge? possibly there could be a warning when viewing target and conflicting content.

mauritsvanrees commented 5 years ago

I am looking at this during the Alpine City Sprint 2019.

The current Products.RedirectionTool has a list of types for which redirects can be created. Looks like this is true for both automatic and manual redirects. The only place where this is checked, is in plone.app.redirector. In the CMFPlone plip branch, this has been taken over, in the new redirection_manageable_types setting in the Types control panel. (BTW, this would need a change in plone.app.redirector to look at the new setting.)

But do we really need/want this? Does it make sense to only create (automatic) redirects for some types? Danger is now that someone installs an add-on with a content type (or creates a Dexterity CT TTW), and forgets to update this setting, and then no redirects will ever get created for this CT.

I could imagine that we use this setting to determine on which content items we show the manage-aliases action. Or add a boolean setting filter_redirection_types, default False, that determines whether we look at the redirection_manageable_types setting at all.

To get this PLIP moving forward fast, I suggest that I remove the redirection_manageable_types setting. This also means we don't need an upgrade step for this setting.

[I thought I had submitted this comment yesterday, but apparently not.]

fredvd commented 5 years ago

But do we really need/want this? Does it make sense to only create (automatic) redirects for some types? Danger is now that someone installs an add-on with a content type (or creates a Dexterity CT TTW), and forgets to update this setting, and then no redirects will ever get created for this CT.

@mauritsvanrees for collective.restrictportlets this was I think the reason we created the logic to 'hide' portlets in a block list instead of creating an allow. When new portlets are registered it will be an proactive task to also hide them. The same could be done for redirection_types.

fredvd commented 5 years ago

Maybe this is not the right place and it should also be listed as a feature after merge, but there is a real risk for 'big' portal sites in the long run. They have been using the RedirectionTool for years and noticed that the redirection list got so big that the @@aliases-controlpanel whould just crash ( (100.550 aliases, 17Mb data structure). So they created custom import/export tools and disabled the UI.

mauritsvanrees commented 5 years ago

@fredvd The crashing aliases controlpanel has been addressed by @staeff by adding batching. I improved it a bit yesterday. I actually added a million aliases and this takes a while to create, but the controlpanel still renders pretty fast after that.

mauritsvanrees commented 5 years ago

for collective.restrictportlets this was I think the reason we created the logic to 'hide' portlets in a block list instead of creating an allow. When new portlets are registered it will be an proactive task to also hide them. The same could be done for redirection_types.

I have removed the unused redirection_manageable_types setting for now.

mauritsvanrees commented 5 years ago

I have updated the PLIP config and requested PLIP jobs, and they have just been started:

I am still making changes, but the most important stuff has been done.

mauritsvanrees commented 5 years ago

And the PLIP jobs pass! Some more changes coming in, but okay.

mauritsvanrees commented 5 years ago

I'd say this is ready for review. @plone/framework-team go ahead!

I have addressed most of the issues raised above, and a few I saw myself. Let me make a list of those, including the changes by @staeff, and the most important differences with the old RedirectionTool.

TODO:

Some remaining items are wishlist or out of scope for this PLIP, at least if we want this accepted and merged soon:

mauritsvanrees commented 5 years ago

@svx You offered to help with the docs. You mentioned putting it in Products.CMFPlone/docs but there is hardly anything related there anymore. I have some text for documentation:

URL Management

New in Plone 5.2 is the URL Management control panel and content action. This is an adaptation of the Products.RedirectionTool add-on. Since many years, if you move or rename a content item, Plone automatically makes a note of this. When a user visits the old url, Plone redirects to the new url.

What is new in 5.2, is that you can manage these alternative urls. You can view them, remove them, and add new ones.

In the Site Setup you can see the new URL Management control panel where you can do all this, including bulk upload of alternative urls. As Editor, you can see a URL Management link in the actions menu of a content item, and add or remove alternative urls for this specific content item.

Migrating from RedirectionTool

The Products.RedirectionTool add-on is no longer needed in Plone 5.2. You can either uninstall it before upgrading to Plone 5.2, or remove the product from the eggs and let the upgrade code from Plone remove it. Any alternative urls (aliases) that you have added manually, will be kept.

Note: In the add-on you could specify the content types for which redirections could be made, but this is no longer supported.

mauritsvanrees commented 5 years ago

In the list of changed items I forgot one thing. I have edited the comment to add it, but let me list it here for clarity:

jensens commented 5 years ago

Framework team decided it is ready for merge.

mauritsvanrees commented 5 years ago

Thanks! There are changes in two other packages besides CMFPlone. They were in the PLIP jobs, which were passing, but I didn't yet create pull requests for them. I now did. The changes are isolated, so the PRs can be merged separately:

Will you merge them or shall I?

jensens commented 5 years ago

well, sure, I need to add checklist on my own before merge next time.

mauritsvanrees commented 5 years ago

Thanks! All is merged now.

It would still be useful to have feedback from Framework team or others on the items of the wishlist, whether they are indeed wished. Most or all are relatively small, so may be fine for inclusion in the beta stage.

jensens commented 5 years ago

Ad wishlist: I don't see any value in making a distinction between manual/automatic redirects.

The proposed removal options are probably a nice to have.

Export is for sure the most important on the list. Best in a format to be used as bulk upload.

Allow/Deny list on based on Content Type is not how I would do it nowadays if this is needed. I'd rather add a behavior to be enabled/disabled on a type in order to switch the feature on or off.

Confirmation dialogs are imo not needed. But proof me wrong here.

fredvd commented 5 years ago

@jensens the difference between automatic and manual redirects is one of the wishes of a very large university portal where the editors insert manual links to have shortcut-urls which have to kept/preserved, but they would like to prune automatic (rename/move) redirects after a certain amount of time. They have/had the 100k+ redirects stored.

Edit: I’ll ask them, they might want to sponsor this.

jensens commented 5 years ago

@fredvd ok, that's a use-case!

pbauer commented 5 years ago

🎉 Thanks for this! See it here in action: http://demo-latest-py3.plone.org/@@redirection-controlpanel The future improvments suggested in the wishlist sound great too 😄

djay commented 5 years ago

It looks good but I can still see some users getting confused by it. A little more explanation on that page could help.

Specifically due to the way other CMS’s work some people still don’t realise you control the primary url of content by renaming content. They will likely end up in the control panel to try and do it here instead.

Also it should mention that alternative urls result in redirections rather than content being available using both urls. It should also warn that the redirection will only work while the alternative url results in a 404 not found so any future adding or renaming content might result in the alternative urls no longer working.

Perhaps?

URL Management
Add a new alternative url to redirect from. To change contents primary url use Actions > Rename.

Alternative url path (Required)
Enter the absolute path where the alternative url should exist. The path must start with ‘/‘. Only urls that results in a 404 not found page will result in a redirect occurring.
[                                   ]

Target Path (Required)
Enter the absolute path of the target. The path must start with ‘/‘. Target must exist or be an existing alternative url path to the target.
[                                   ]

On 13 Feb 2019, at 12:29 pm, Philip Bauer notifications@github.com wrote:

🎉 Thanks for this! See it here in action: http://demo-latest-py3.plone.org/@@redirection-controlpanel The future improvments suggested in the wishlist sound great too 😄

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.