Page MenuHomePhabricator

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

Authored by epriestley on Dec 5 2018, 1:57 PM.
Tags
None
Referenced Files
Unknown Object (File)
Tue, Mar 19, 10:50 PM
Unknown Object (File)
Sun, Mar 10, 10:17 AM
Unknown Object (File)
Feb 4 2024, 7:50 AM
Unknown Object (File)
Jan 22 2024, 3:57 AM
Unknown Object (File)
Dec 28 2023, 9:30 PM
Unknown Object (File)
Dec 27 2023, 8:03 PM
Unknown Object (File)
Dec 24 2023, 6:29 PM
Unknown Object (File)
Dec 23 2023, 3:16 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
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

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!

This revision is now accepted and ready to land.Dec 7 2018, 10:25 PM
This revision was automatically updated to reflect the committed changes.