Closed Bug 854878 Opened 11 years ago Closed 11 years ago

Check validity of new slugs before doing page moves

Categories

(developer.mozilla.org Graveyard :: General, defect)

Product:
developer.mozilla.org Graveyard
Component:
General
Platform:
All
Other
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: sheppy, Unassigned)

References

Details

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

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?
======================================
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)
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
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.
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.
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?
(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.
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
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
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Sheppy: Please see comment 18.
Flags: needinfo?(eshepherd)
I can't find those pages.
Flags: needinfo?(eshepherd)
Product: developer.mozilla.org → developer.mozilla.org Graveyard
You need to log in before you can comment on or make changes to this bug.