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)
Mon, Nov 18, 4:49 PM
Unknown Object (File)
Oct 27 2024, 10:17 AM
Unknown Object (File)
Oct 17 2024, 6:21 PM
Unknown Object (File)
Oct 14 2024, 6:54 AM
Unknown Object (File)
Sep 9 2024, 11:35 PM
Unknown Object (File)
Sep 3 2024, 12:05 AM
Unknown Object (File)
Sep 2 2024, 7:43 AM
Unknown Object (File)
Aug 26 2024, 1:59 AM

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
Branch
audit1
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 21245
Build 28895: Run Core Tests
Build 28894: arc lint + arc unit

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.