Add spiffyier Assigned To panel in Maniphest
AbandonedPublic
Actions

Authored by chad on Mar 8 2016, 1:11 AM.

Details

Reviewers

Summary

Making a pass here at making the Assigned panel a little smarter. It adds one-click claim and dialog assign. I'm sure I built some of this wrong, too.

Test Plan

Click claim, get a task. Click edit pencil, place it up for grabs. Click assign, assign it to notchad.

Diff Detail

Repository

rP Phabricator

Branch

maniphest-claim (branched from master)

Lint

Lint Passed

Severity	Location	Code	Message
Advice	src/applications/maniphest/controller/ManiphestTaskAssignController.php:26	XHP16	TODO Comment

Unit

Tests Passed

Build Status

Buildable 11059
Build 13680: Run Core Tests
Build 13679: arc lint + arc unit

Event Timeline

chad updated this revision to Diff 37197.Mar 8 2016, 1:11 AM

chad retitled this revision from to Add spiffyier Assigned To panel in Maniphest.

chad updated this object.

chad edited the test plan for this revision. (Show Details)

chad added a reviewer: epriestley.

Herald added a subscriber: Korvin. · View Herald TranscriptMar 8 2016, 1:11 AM

chad added inline comments.Mar 8 2016, 1:13 AM

webroot/rsrc/css/phui/phui-object-item-list-view.css
785–787	I plan to re-use this little list in Phame, Conpherence, etc.

Caught a couple of things:

Logged-out users get a "Claim" link which fatals when clicked.
Logged-in users who do not have permission to edit the task get an error on a separate page when they click "Claim", but should get a dialog on the same page.
Logged-in users who do not have permission to edit the task see enabled links, but should (presumably) see visually disabled links?
Visually, these look like links rather than actions, which seems a little off? (I don't think we have links with this appearance elsewhere in the product that work like this?)
The "Assign" link can't be open-in-new-tab'd. There is no reason for this link to be a form, since it does not submit anything. Making this a normal link will let it be open-in-new-tab'd, and may fix:
The "Assign" form doesn't work, I think? It works OK when you click the pencil, but not when you click the "Assign" text. This might be because the "Assign" text is a form?
The "Edit Assignee" datasource prompts users to type "...or 'none'", but fails with an error if they type "none" (this only happens on the "pencil", not the "Assign" link). You can compare the value to PhabricatorPeopleNoOwnerDatasource::FUNCTION_TOKEN to test if it is "No Owner".
The "Claim" link can't be open-in-new-tab'd, which feels a little odd, maybe because it looks so much like a link. Fixing this is a bit tricky, maybe not worth bothering. In theory, it could present a dialog when open-in-new-tab'd, maybe.
The ManiphestTaskClaimController endpoint is quasi-vulnerable to CSRF, where I send you a funny cat page and it claims a bunch of tasks for you in the background, except that a fallback willWrite() check later catches this. The endpoint should check $request->isFormPost() and return 404 if the request isn't a form post.
Both the Assign and Claim controllers could probably be merged into the same controller with a /(?P<action>assign|claim)/ match in the URI to simplify things a little.
When you click the pencil icon, then don't type anything into the typeahead, then save, this error appears in the log:

[Mon Mar 07 17:41:53.802285 2016] [:error] [pid 60134] [client 127.0.0.1:54265] [2016-03-07 17:41:53] ERROR 8: Undefined offset: 0 at [/Users/epriestley/dev/core/lib/phabricator/src/applications/maniphest/controller/ManiphestTaskAssignController.php:30]
[Mon Mar 07 17:41:53.803289 2016] [:error] [pid 60134] [client 127.0.0.1:54265] arcanist(head=master, ref.master=ccbaee585e1a), corgi(head=87c4e69a9f9654390770045ac0e65271a3b542ab, ref.master=ad26f63a398b), instances(head=master, ref.master=794f8c37b870), ledger(head=master, ref.master=4da4a24b8779), libcore(), phabricator(head=master, ref.master=49724d28cb6b, custom=5), phutil(head=master, ref.master=62f63fc614a1), services(head=stable, ref.master=a2c79ba63fd9, ref.stable=babb5c93cec9)
[Mon Mar 07 17:41:53.803311 2016] [:error] [pid 60134] [client 127.0.0.1:54265]   #0 ManiphestTaskAssignController::handleRequest(AphrontRequest) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:237]
[Mon Mar 07 17:41:53.803317 2016] [:error] [pid 60134] [client 127.0.0.1:54265]   #1 AphrontApplicationConfiguration::processRequest(AphrontRequest, PhutilDeferredLog, AphrontPHPHTTPSink, MultimeterControl) called at [<phabricator>/src/aphront/configuration/AphrontApplicationConfiguration.php:149]
[Mon Mar 07 17:41:53.803322 2016] [:error] [pid 60134] [client 127.0.0.1:54265]   #2 AphrontApplicationConfiguration::runHTTPRequest(AphrontPHPHTTPSink) called at [<phabricator>/webroot/index.php:17]

You can do something like this instead:

$owner_phids = $request->getArr('phid');
if (!$owner_phids) {
  $owner_phid = null;
} else {
  $owner_phid = head($owner_phids);
  if ($owner_phid === PhabricatorPeopleNoOwnerDatasource::FUNCTION_TOKEN) {
    $owner_phid = null;
  }
}

The first case handles the user not typing anything, the second case handles the user typing "none" for the "No Owner" token.

src/applications/maniphest/controller/ManiphestTaskAssignController.php
23	Maybe `$task->getMonogram()`.
26	The tokenizer always submits a list of PHIDs, even with `setLimit(1)`, so this is fine (although maybe prefer `head()`). See also note in main comment.
43–46	It's impossible for this to be false here since we required `CAN_EDIT` in the query up top. (If it were `false`, we'd fail to define `$form` and fatal on line 63 when using `$form`, and render a dialog with no form in it which would be silly.)
src/applications/maniphest/controller/ManiphestTaskDetailController.php
274–290	This should just be a link.

This revision now requires changes to proceed.Mar 8 2016, 1:44 AM

This also allows users to bypass UI restrictions imposed by EditEngine. For example, on this install, users who are not members of Community can not assign or claim tasks without going to great lengths (e.g., using the API).

After this lands, they'll be able to.

I suppose we can load the user's default edit form, check if the "Assigned To" field is editable in it, and use that to make a decision about whether they can use this action, but the error message if they can't is going to be messy/complicated. We could also just hide these elements completely.

Or we could let users bypass these advisory policies, but I think that's bad news for open source installs like us, WMF, etc.

Caught a couple of things

But other than the 20 bugs, it seems ok?

I guess I'm not sure if we need all the complexity it entails? I think it's going to get like 2x-3x more complex by the time all the bugs and policy interactions are fixed, and there's already a way to do this on the same page, a few inches away, which has the right behaviors in all these edge cases.

It also sets users up to ask us to do this for every other field, like making subscribers not only support subscribe/unsubscribe but also support "edit subscribers", and letting projects be edited inline, etc., and then maybe wanting the stuff to Ajax in.

And these interactions will generate low-quality single-transaction emails, which opens up another whole can of worms.

Putting actions for subscribe and tokens in the panels makes a lot of sense to me since we can remove the "Action List" actions and put them in context, but these claim/assign actions aren't a one-for-one trade: they're just adding new actions and complexity without removing anything or simplifying anything.

If we want to build Asana with inline edit, we can do that too, but I think it opens up a lot of rough edges and I'm not sure it's really useful. I think I spend a ton of time in Maniphest, but the amount of time I'm losing to not having inline edit feels totally insignificant.

Personally, I'd probably never use "Assign", at least, since I think I always want to add an explanatory comment when assigning (e.g., "@whoever, can you take a look at this? I think Dxxx broke it...").

True, I would use claim/up for grabs, but probably never assign. It just seemed strange to have claim there but not also assign.

My slippery slope scenario where we're forced to rewrite the whole product in Javascript is obviously a bit extreme/ridiculous, and we can just say "no" to "make every other field inline-editable".

I could imagine using "Claim" occasionally and it does seem weird to have it without "Assign", and the EditEngine policy stuff isn't really too bad today and I can give you some reasonable $engine->isTransactionAvailableToUser($viewer, Whatever::TRANSACTION_OWNER) without too much of an issue.

So I don't think this is bad or anything, it just seems like kind of a lot of work for a pretty minor feature.

(I'll try to get you a diff for the engine thing tonight but have a couple things I have to take care of and might not get to it.)