Page MenuHomePhabricator

Address a transaction issue with some audit actions not applying correctly
ClosedPublic

Authored by epriestley on Wed, Dec 5, 1:57 PM.

Details

Summary

See https://discourse.phabricator-community.org/t/cannot-accept-commits-in-audit/2166/.

In D19842, I changed PhabricatorEditField->shouldGenerateTransactionsFromComment().

  • Previously, it bailed on getIsConduitOnly().
  • After the patch, it bails on a missing getCommentActionLabel().

The old code was actually wrong, and it was previously possible to apply possibly-invalid actions in some cases (or, at least, sneak them through this layer: they would only actually apply if not validated properly).

In practice, it let a different bug through: we sometimes loaded commits without loading their audit authority, so testing whether the viewer could "Accept" the commit or not (or take some other actions like "Raise Concern") would always fail and throw an exception: "Trying to access data not attached to this object..."

Fixing the insufficiently-strict transaction generation code exposed the "authority not attached" bug, which caused some actions to fail to generate transactions.

This appeared in the UI as either an unhelpful error ("You can't post an empty comment") or an action with no effect. The unhelpful error was because we show that error if you aren't taking any other actions, and we wouldn't generate an "Accept" action because of the interaction of these bugs, so the code thought you were just posting an empty comment.

Test Plan

Without leaving comments, accepted and rejected commits. No more error messages, and actions took effect.

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

epriestley created this revision.Wed, Dec 5, 1:57 PM
epriestley requested review of this revision.Wed, Dec 5, 1:59 PM
epriestley marked 2 inline comments as done.Wed, Dec 5, 2:03 PM

I noted the immediate change which exposed this on D19842, for spelunking purposes.

A second reason this slipped through is that we use throw new Exception and } catch (Exception $ex) { for internal validation, instead of some more narrowly tailored exception subclass. This makes the actual validation logic slightly easier and slightly more user-friendly, but possibly merits reconsideration at some point.

src/applications/diffusion/query/DiffusionCommitQuery.php
241–256

This also fixes a UNION DISTINCT construction in an adjacent query (when searching for commits using "responsible users").

src/applications/repository/storage/PhabricatorRepositoryCommit.php
221–223

Prophetic!

hskiba added a subscriber: hskiba.Wed, Dec 5, 5:15 PM
amckinley accepted this revision.Fri, Dec 7, 10:25 PM
This revision is now accepted and ready to land.Fri, Dec 7, 10:25 PM
This revision was automatically updated to reflect the committed changes.