Page MenuHomePhabricator

Fix Phriction document move on to existing document placeholder
ClosedPublic

Authored by gd on Oct 17 2016, 1:21 PM.

Details

Summary

Looks like the logic was there already but some minor parts were missing.
Fixes T8082.

Test Plan
  • Create document /w/foo
  • Delete document /w/foo
  • Create document /w/bar
  • Move document /w/bar for /w/foo

No error was displayed and document /w/bar was moved to /w/foo.

Diff Detail

Repository
rP Phabricator
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

gd retitled this revision from to Fixing Phirction document move on to existing document placeholder.
gd updated this object.
gd edited the test plan for this revision. (Show Details)
gd added a reviewer: epriestley.
chad retitled this revision from Fixing Phirction document move on to existing document placeholder to Fix Phriction document move on to existing document placeholder.Oct 17 2016, 2:20 PM
chad edited edge metadata.
epriestley edited edge metadata.

This creates a maybe-slightly-confusing policy error if you try to move over a page you don't have permission to overwrite, right? (That is: the error is correct, but could be tailored so it's clear that the target page, not the source page, is generating it.)

I think that's still a big step forward.

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

(Do we need this rtrim() stuff? I think we might have fixed PhabricatorSlug so it mostly works correctly.)

71

(Does this actually needContent()? Does getStatus() rely on it? I don't remember offhand..)

This revision is now accepted and ready to land.Oct 17 2016, 10:36 PM
gd edited edge metadata.
  • Use $normal_slug as is
gd marked 2 inline comments as done.Oct 18 2016, 11:12 AM
gd added inline comments.
src/applications/phriction/controller/PhrictionMoveController.php
70

You're right, looks like this rtrim() is not needed.

71

needContent() is needed because getContent() is called down the line.

gd marked 2 inline comments as done.
  • Use omnipotent user to produce a correct error message

Using PhabricatorUser::getOmnipotentUser() produces the correct error message even if the document is not visible to the user:

You can not move this document there, because it would overwrite an existing document which is already at that location. Move or delete the existing document first.

This probably now allows you to move documents underneath a document you can't edit, and immediately lose edit access to them -- maybe? Ideally, that would be a different error ("You can not move this document to that location because you don't have permission to edit documents at that location and would no longer be able to edit it." or similar).

The repro steps I'm imagining are:

  • Set a/ to "Can Edit: alice".
  • As any other user, move b/example to a/example.
  • a/example can now only be edited by user @alice?
    • The moving user was able to create a document they could not normally create?
    • The moving user can no longer edit the document?

That might actually work fine already, though.

This probably now allows you to move documents underneath a document you can't edit, and immediately lose edit access to them -- maybe? Ideally, that would be a different error ("You can not move this document to that location because you don't have permission to edit documents at that location and would no longer be able to edit it." or similar).

The repro steps I'm imagining are:

  • Set a/ to "Can Edit: alice".
  • As any other user, move b/example to a/example.
  • a/example can now only be edited by user @alice?
    • The moving user was able to create a document they could not normally create?
    • The moving user can no longer edit the document?

That might actually work fine already, though.

In this scenario I get same slightly-confusing policy error as before but the document is not moved:

You do not have permission to view this object.

Users with the "Can View" capability:

  • Administrators can take this action.
  • To view a wiki document, you must also be able to view all of its parents.

Cool, that's good enough. Thanks for testing!

I think this is good to go unless you want to make any other changes. We have a larger iteration on Phriction planned in the future (T9379) and can smooth out things like not-quite-the-best error messages then (since we'll move to EditEngine + Modular Transactions anyway, and have to do work in this section of the code regardless).

Cool. In that case I'm going to land this as is.

This revision was automatically updated to reflect the committed changes.