Page MenuHomePhabricator

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

Authored by btrahan on Sep 11 2014, 10:25 PM.
Referenced Files
F12182303: D10485.id25208.diff
Wed, Sep 13, 8:09 AM
F12164124: D10485.diff
Wed, Sep 6, 10:29 PM
F12159879: D10485.diff
Wed, Sep 6, 4:02 AM
F12150788: D10485.id25205.diff
Sat, Sep 2, 10:16 PM
Thu, Aug 31, 4:21 AM
Aug 23 2023, 2:37 PM
F12129106: D10485.diff
Aug 21 2023, 9:15 AM
Unknown Object (File)
Jul 22 2023, 11:25 PM



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

rP Phabricator
Lint Skipped
Tests Skipped

Event Timeline

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.
epriestley edited edge metadata.

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
btrahan edited edge metadata.
  • 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
btrahan updated this revision to Diff 25209.

Closed by commit rPe8985fc9e723 (authored by @btrahan).