Differential - change "closed by commit" comment to real transaction
ClosedPublic
Actions

Authored by btrahan on Sep 11 2014, 10:25 PM.

Details

Reviewers

Maniphest Tasks

T5536: Automatic diff should be more clearly called out in the UI
Restricted Maniphest Task
T5875: "Close Revision" and "Merge Task" interactions should be real transactions, not comments

Commits

Restricted Diffusion Commit
rPe8985fc9e723: Differential - change "closed by commit" comment to real transaction

Summary

Implements a new transaction - still TYPE_ACTION - but using a new DifferentialAction::ACTION_COMMIT_CLOSE. Augment rendering as necessary to display this new transaction. Saves enough information so T3686 is possible but stops short of implementing a popup to display this information. Fixes T5875. Ref T3686.

One small display oddity - this new transaction now renders at the top of the transaction group whereas when it was a comment it was on the bottom. I think this is basically okay but if not how fix? (Playing with the "strength" of these actions will mess up the email too?)

Test Plan

made a diff X that fixed task Y. committed. checked diff X, task Y, and the commit pages for proper transactions and all looked good.

Diff Detail

Repository

rP Phabricator

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

btrahan updated this revision to Diff 25205.Sep 11 2014, 10:25 PM

btrahan added tasks: T5875: "Close Revision" and "Merge Task" interactions should be real transactions, not comments, Restricted Maniphest Task.

btrahan retitled this revision from to Differential - change "closed by commit" comment to real transaction.

btrahan updated this object.

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

btrahan added a reviewer: epriestley.

Herald added subscribers: Korvin, epriestley. · View Herald TranscriptSep 11 2014, 10:25 PM

Harbormaster completed remote builds in B2523: Diff 25205.Sep 11 2014, 10:26 PM

We could maybe do this without adding a new action -- just add some metadata value like isAutoclose, and check for that to figure out whether to render the fancy version? Not sure if that'd be simpler. This seems quite reasonable, in any case.

I don't think there's a way to get something to the top without also making it strong right now, which, yeah, will mess up email. Leaving "close" on top seems reasonable to me. We could adjust the update transaction in T5536 to have a flag like "this is an auto-update", and then render it as "This revision was automatically updated to reflect the committed changes." or something (instead of "epriestley updated the revision."), to help make T5536 more clear and resolve the ordering issue by making the new order more natural. But I don't think this is a big deal in either case.

This revision is now accepted and ready to land.Sep 11 2014, 10:33 PM

changed from new action to 'isCommitClose' metadata value
- saves us a bit of code here and there
added 'isCommitUpdate' to the pertinent update transaction
- now renders as "This revision was automatically updated to reflect the committed changes."
re-tested