Page MenuHomePhabricator

Improve Phriction page move dialog
ClosedPublic

Authored by epriestley on Nov 12 2014, 2:22 PM.
Tags
None
Referenced Files
F14415524: D10838.diff
Tue, Dec 24, 6:19 PM
Unknown Object (File)
Thu, Dec 19, 8:14 PM
Unknown Object (File)
Thu, Dec 19, 4:25 PM
Unknown Object (File)
Thu, Dec 19, 4:25 PM
Unknown Object (File)
Thu, Dec 19, 4:25 PM
Unknown Object (File)
Thu, Dec 19, 4:25 PM
Unknown Object (File)
Thu, Dec 19, 4:25 PM
Unknown Object (File)
Mon, Dec 16, 11:56 PM
Subscribers

Details

Summary

Fixes T5492. I figured this would be easier to just fix than write a guide for; it actually took me an hour, but I spent like 75% of that futzing with my editor.

  • The Move controller currently accepts either a slug or an ID. I can't find any callsites which pass a slug, and this doesn't make sense. Pretty sure this was copy/pasted from Edit or something. Only accept IDs.
  • Slightly modernize the Move controller (newDialog(), handleRequest(), $viewer).
  • When the user enters a bad slug, warn them that we're going to fix it for them and let them accept or reject the changes.
  • Don't prefill the edit note (this feels inconsistent/unusual).
  • On the form, label the input "Path" instead of "URI".
  • Show the old path, to help remind the user what the input should look like.
  • When a user tries to do a no-op move, show a more tailored message.
  • When the user tries to do an overwriting move, explain how they can fix it.
  • When normalizing a slug like /question/???/mark/, make it normalize to /question/_/mark.
Test Plan
  • Tried to move a document to itself.
  • Tried to overwrite a document.
  • Did a bad-path move, accepted corrected path.
  • Did a good-path move.
  • Did a path move with a weird component like /???/.
  • Added and executed unit tests.

Diff Detail

Repository
rP Phabricator
Branch
wikimove
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 3025
Build 3029: [Placeholder Plan] Wait for 30 Seconds

Event Timeline

epriestley retitled this revision from to Improve Phriction page move dialog.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.
btrahan edited edge metadata.

Nice, thanks for tackling this. I've got a small thing for the wiki now from polishing it up...

The edit note used to be pre-filled with the previous edit's note before I got in there. e.g.

  • edit 1: change title with note "changing title"
  • edit 2: move the page and see note as "changing title"

anyhoo, I like it best as you have it, but that's the mini history here as I understand it. =D

src/applications/phriction/controller/PhrictionMoveController.php
35

usable -> using

37–39

Should this logic also be in the editor?

This revision is now accepted and ready to land.Nov 12 2014, 2:54 PM

Yeah, I think the original note behavior was contributed and might have been copy-pasted or something (given that the slug part looks like it was copy/pasted).

src/applications/phriction/controller/PhrictionMoveController.php
37–39

Ideally maybe-yes, but I think it's fairly hard to raise a custom prompt from within the editor. I generally feel OK about putting minor quality-of-life things like this outside the editor if it's unlikely that Conduit callers would make the mistake. But if we hit a couple more of these, maybe it's an argument for making it easier to raise ad-hoc prompts from within the editor.

In this case, it would also be reasonable for the Conduit version to not normalize the slug, and then the editor to reject it outright (this may already happen). Then Conduit callers would get an error ("you passed a bad slug, fix it"), which seems like the ideal behavior (trying to do a prompt over Conduit seems iffy).

epriestley edited edge metadata.
  • Fix usable -> using.
This revision was automatically updated to reflect the committed changes.