Last Comment Bug 854878 - Check validity of new slugs before doing page moves
: Check validity of new slugs before doing page moves
Status: RESOLVED FIXED
[specification][type:change]
:
Product: Mozilla Developer Network
Classification: Other
Component: General (show other bugs)
: unspecified
: All Other
: -- normal with 1 vote (vote)
: ---
Assigned To: Nobody; OK to take it and work on it
:
Mentors:
: 876738 (view as bug list)
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2013-03-26 06:41 PDT by Eric Shepherd [:sheppy]
Modified: 2013-11-11 18:33 PST (History)
6 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments

Description Eric Shepherd [:sheppy] 2013-03-26 06:41:46 PDT
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?
======================================
Comment 2 John Karahalis [:openjck] 2013-04-29 08:26:40 PDT
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.
Comment 3 Eric Shepherd [:sheppy] 2013-04-30 07:01:19 PDT
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).
Comment 4 Thierry Régagnon 2013-06-04 05:10:17 PDT
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
Comment 5 David Walsh :davidwalsh 2013-06-04 08:53:48 PDT
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.
Comment 6 Frédéric Bourgeon [:FredB] 2013-06-04 08:56:28 PDT
(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.
Comment 7 Eric Shepherd [:sheppy] 2013-06-04 08:57:27 PDT
Fred is right, you currently have to leave out the leading slash or things go awry. We should fix it to work either way.
Comment 8 David Walsh :davidwalsh 2013-06-04 08:58:08 PDT
Yes, sorry, I misunderstood Sheppy's comments in IRC.
Comment 9 James Bennett [:ubernostrum] 2013-06-12 03:14:25 PDT
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.
Comment 10 Frédéric Bourgeon [:FredB] 2013-06-12 05:30:12 PDT
(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.
Comment 11 James Bennett [:ubernostrum] 2013-06-19 01:48:52 PDT
*** Bug 876738 has been marked as a duplicate of this bug. ***
Comment 12 James Bennett [:ubernostrum] 2013-06-19 01:51:26 PDT
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?
Comment 13 David Walsh :davidwalsh 2013-06-20 15:00:10 PDT
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.
Comment 14 David Walsh :davidwalsh 2013-06-21 07:00:15 PDT
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?
Comment 15 Eric Shepherd [:sheppy] 2013-06-21 07:01:31 PDT
(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 ghost@ethertank.jp 2013-06-21 09:09:09 PDT
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 [github robot] 2013-06-24 14:20:08 PDT
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
Comment 19 John Karahalis [:openjck] 2013-07-09 10:13:41 PDT
Sheppy: Please see comment 18.
Comment 20 Eric Shepherd [:sheppy] 2013-07-11 12:59:50 PDT
I can't find those pages.

Note You need to log in before you can comment on or make changes to this bug.