Don't show error operations after a successful land operation
ClosedPublic
Actions

Authored by epriestley on Oct 26 2015, 9:24 PM.

Details

Reviewers

chad
Mnkras

Maniphest Tasks

T182: Commit into repository directly from differential

Commits

Restricted Diffusion Commit
rPcea633f698da: Don't show error operations after a successful land operation

Summary

Ref T182. When viewing a revision, if there are several error operations and then a success operation, we currently show the last error. This is misleading.

Instead, don't show anything if there's a success (this may require tuning eventually if you can land multiple times onto different branches or whatever, but should be reasonable for now).

Also make the table a little nicer, particularly for merge failure output.

Test Plan

Screen Shot 2015-10-26 at 2.21.22 PM.png (297×897 px, 45 KB)

Diff Detail

Repository

rP Phabricator

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

epriestley updated this revision to Diff 34634.Oct 26 2015, 9:24 PM

epriestley retitled this revision from to Don't show error operations after a successful land operation.

epriestley updated this object.

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

epriestley added reviewers: chad, Mnkras.

epriestley added a task: T182: Commit into repository directly from differential.

Herald edited edge metadata. · View Herald TranscriptOct 26 2015, 9:24 PM

Mnkras accepted this revision.Oct 26 2015, 9:25 PM

Mnkras edited edge metadata.

This revision is now accepted and ready to land.Oct 26 2015, 9:25 PM

Closed by commit rPcea633f698da: Don't show error operations after a successful land operation (authored by epriestley, committed by epriestley). · Explain WhyOct 26 2015, 9:27 PM

This revision was automatically updated to reflect the committed changes.

Mnkras added inline comments.Oct 27 2015, 12:02 AM

src/applications/differential/controller/DifferentialRevisionViewController.php
1072	I just realized that its possible for `$operation` to not be set here, this should be moved up into the foreach loop.

epriestley added inline comments.Oct 27 2015, 12:06 AM

src/applications/differential/controller/DifferentialRevisionViewController.php
1072	Operation should always be set here because `$operations` is guaranteed nonempty (per line 1056) so `$operation` will leak out of the loop on line 1064 with some value. Relying on loops leaking variables is arguably bad form, of course.