Check validity of new slugs before doing page moves

RESOLVED FIXED

Status

Mozilla Developer Network
General
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: sheppy, Unassigned)

Tracking

Details

(Whiteboard: [specification][type:change])

(Reporter)

Description

4 years ago
What feature should be changed? Please provide the URL of the feature if possible.
==================================================================================
Currently, it's possible to enter bad slugs in the page move UI and have Kuma attempt the move anyway.

What problems would this solve?
===============================
For example, I gave it "/en-US/docs/Some/URL" and it happily attempted the move, even though the slug was into space that didn't exist. This needs fixing.

Who would use this?
===================
People moving pages around.

What would users see?
=====================
No visible changes.

What would users do? What would happen as a result?
===================================================
Same as now, but they would get an error if the slug isn't valid.

Is there anything else we should know?
======================================
One page that was mismoved:

Original URL: https://developer.mozilla.org/en-US/docs/Advanced_styling_for_HTML_forms

Tentative URL: https://developer.mozilla.org/en-US/docs/HTML/Forms/Advanced_styling_for_HTML_forms

What really happened: https://developer.mozilla.org/en-US/docs_HTML/Forms/Advanced_styling_for_HTML_forms
It sounds like this is different than bug 851199, but I am not exactly sure how. Sheppy, can you update the title of this bug to call out the specific page moving bug that you noticed here. We can open separate bugs for other types of move input that fails.
Flags: needinfo?(eshepherd)
(Reporter)

Comment 3

4 years ago
This is simply a matter of needing to be sure the new slug being typed in is correct. the key points:

- It shouldn't matter if you start with a slash or not; just fix it as needed.

- If the user includes the locale and/or "/docs/", don't blindly create pages like /en-US/docs/en-US/docs/foo -- help protect users from making this mistake (I've done it several times myself).
Flags: needinfo?(eshepherd)
Summary: Improve handling of improper slugs in page move UI → Check validity of new slugs before doing page moves

Comment 4

4 years ago
I have just mismoved a page by adding a '/' at the start of the new slug

The page URL is now:
https://developer.mozilla.org/fr/docs_CSS/CSS_Reference/S%C3%A9lecteurs_d%27enfants_adjacents

It should be:
https://developer.mozilla.org/fr/docs/CSS/CSS_Reference/S%C3%A9lecteurs_d%27enfants_adjacents
Can you look at this James?  From what I understand, we need to add two pre-submission treatments here:

1.  Remove a "{locale}/docs/" pattern from the start of the move slug, if it exists
2.  Check the first character of the new slug, add a "/" if not present.
Flags: needinfo?(jbennett)
(In reply to David Walsh :davidwalsh from comment #5)
> Can you look at this James?  From what I understand, we need to add two
> pre-submission treatments here:
> 
> 1.  Remove a "{locale}/docs/" pattern from the start of the move slug, if it
> exists
> 2.  Check the first character of the new slug, add a "/" if not present.

Reading point 2, I think that is the opposite. Right now, the move feature chokes on leading "/" and is ok if it's not present. Not 100% sure I understood what you meant, but I'd like it to be clear for all of us.
(Reporter)

Comment 7

4 years ago
Fred is right, you currently have to leave out the leading slash or things go awry. We should fix it to work either way.
Yes, sorry, I misunderstood Sheppy's comments in IRC.
I'm investigating a bit, but somewhat confused since -- as y'all are probably aware from seeing slug-collision errors -- we *do* perform checks before moving.
Flags: needinfo?(jbennett)
Assignee: nobody → jbennett
(In reply to James Bennett [:ubernostrum] from comment #9)
> I'm investigating a bit, but somewhat confused since -- as y'all are
> probably aware from seeing slug-collision errors -- we *do* perform checks
> before moving.

To be honest I have no idea if the addition of a leading '/' concludes to a collision or a mangled URL that Kuma doesn't resolve (remember that issue we had some months ago with '_' inserted around the locale code when rewritting '//' or something like that?). But what is sure is that right now, the checks done on the slug don't cover this case.
Duplicate of this bug: 876738
I'm stumped. The behavior being reported contradicts what the unit tests for collision-detection seem to be exercising and passing. Anyone else want to give it a try?
Assignee: jbennett → nobody
In doing local testing, I added a "/" at the beginning of the slug and ended up with:

/en-US/docs//QueMasoMo

I added a JS event to remove leading slashes, but we'll want to fix this on the server side.
There are a bunch of commenters and ideas here so I want to clarify the root issue which needs to be fixed:

- We must remove leading "/" from slugs
- We must remove leading "/{locale}/docs/" from slugs

Is this correct per this ticket?
(Reporter)

Comment 15

4 years ago
(In reply to David Walsh :davidwalsh from comment #14)
> There are a bunch of commenters and ideas here so I want to clarify the root
> issue which needs to be fixed:
> 
> - We must remove leading "/" from slugs
> - We must remove leading "/{locale}/docs/" from slugs
> 
> Is this correct per this ticket?

Yes.

Comment 16

4 years ago
1, Remove leading "https://developer.mozilla.org" from slugs
2, Remove leading "/" from slugs
3, Remove leading "{locale}/" from slugs
4, Remove leading "docs/" from slugs

Comment 17

4 years ago
Commits pushed to master at https://github.com/mozilla/kuma

https://github.com/mozilla/kuma/commit/8550add7f6e170a538e633c0155cf58b522a5859
fix bug 854878 - Remove leading slash and locale/docs from slug move

https://github.com/mozilla/kuma/commit/d5e4580ac4581e9b3431891096864b5bef3751f6
Merge pull request #1181 from darkwing/move-slug-854878

fix bug 854878 - Remove leading slash and locale/docs from slug move

Updated

4 years ago
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED

Comment 18

4 years ago
Great!

Can you please also fix the URLs of the two buggy pages?

https://developer.mozilla.org/fr/docs_CSS/CSS_Reference/S%C3%A9lecteurs_d%27enfants_adjacents
should be:
https://developer.mozilla.org/fr/docs/CSS/CSS_Reference/S%C3%A9lecteurs_d%27enfants_adjacents

and

https://developer.mozilla.org/en-US/docs_HTML/Forms/Advanced_styling_for_HTML_forms
should be
https://developer.mozilla.org/en-US/docs/HTML/Forms/Advanced_styling_for_HTML_forms
Sheppy: Please see comment 18.
Flags: needinfo?(eshepherd)
(Reporter)

Comment 20

4 years ago
I can't find those pages.
Flags: needinfo?(eshepherd)
You need to log in before you can comment on or make changes to this bug.