Page MenuHomePhorge
Differential D25503

Allow to uninstall (hide) Audit application
ClosedPublic
Actions

Authored by aklapper on Jan 4 2024, 07:34.
Tags
None
Referenced Files
F4117516: D25503.1746994092.diff
Sat, May 10, 20:08
F4067692: D25503.1746799880.diff
Thu, May 8, 14:11
F4033506: D25503.1746715742.diff
Wed, May 7, 14:49
F4011239: D25503.1746625679.diff
Tue, May 6, 13:47
F3983484: D25503.1746564125.diff
Mon, May 5, 20:42
F3982307: D25503.1746555312.diff
Mon, May 5, 18:15
F3749183: D25503.1745791572.diff
Sat, Apr 26, 22:06
F3748894: D25503.1745786998.diff
Sat, Apr 26, 20:49
View All 90 Files
Subscribers
Cigaryno
Matthew
tobiaswiese
valerio.bozzolan
Tokens
"Like" token, awarded by valerio.bozzolan.

Details

Reviewers
valerio.bozzolan
Group Reviewers
O1: Blessed Committers
Maniphest Tasks
T15129: Unmark Audit as a required application
Commits
rP821708414eec: Allow to uninstall (hide) Audit application
Summary

While many Phorge applications can be uninstalled (such as Differential for pre-merge commit review), this has not been the case for Audit (post-merge commit review) since rP11861265fe94fa97e4d0563c5bdb7b8cac27282d.

In installations which do not use Audit in their workflows, exposing Audit related UI elements creates additional clutter and complexity.

Fixes T15129

Test Plan
  • Go to /applications/view/PhabricatorAuditApplication/ and click "Uninstall"
  • View an individual merged commit in Diffusion, click "Edit Commit" in the sidebar to go to /diffusion/commit/edit/1/, see that "Auditors" datasource is not displayed
  • View an individual merged commit in Diffusion, click the "Add Action..." dropdown, see that the "Audit Action" section and its entries "Accept Commit" and "Raise Concern" are not displayed
  • View an individual merged commit in Diffusion, click the "Add Action..." dropdown, see that the "Change Auditors" entry is not displayed
  • Go to /conduit/ and see that the API method "audit.query" and the entire section "audit" is not displayed
  • Go to /diffusion/commit/ and see no "Active Audits" and "Audited" searches in the left side bar
  • Go to /diffusion/commit/query/all/ and see that no "Auditors" entry is shown for each commit
  • Go to /diffusion/commit/query/all/ and see that no audit related entry is shown in the grey column on the right (if Harbormaster is not installed, there will be no grey column at all)
  • Go to /herald/edit/?content_type=commit&rule_type=global and see that no Condition entry "Auditors" and no Condition entry "Affected packages that need audit" is displayed
  • Go to /herald/edit/?content_type=commit&rule_type=personal and see that no "Add me as an auditor" Herald action and no "Added Auditors" Herald condition is displayed

Diff Detail

Repository
rP Phorge
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

aklapper created this revision.Jan 4 2024, 07:34
Owners added a reviewer: O1: Blessed Committers.Jan 4 2024, 07:34
Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptJan 4 2024, 07:34
aklapper requested review of this revision.Jan 4 2024, 07:34
Harbormaster completed remote builds in B999: Diff 1607.Jan 4 2024, 07:34
aklapper added a comment.Jan 4 2024, 07:36

This is not ready for merge yet.

Note that there are two TODO items which welcome input.

Open question: Where does the Audits realm end?
For example, which use cases are covered by offering the comment textarea box on Diffusion commits (like at the bottom of https://we.phorge.it/rP428f9686c4171912ee186ebd919640a7427da768) used?
Should this textarea box and the entire "Add Action" dropdown (and the "Edit Commit" entry in the sidebar) be hidden when Audit is disabled? I'm tempted to say yes because disabling Audit to me implies "no post-merge commenting on commits". YMMV.
If yes, this would require making renderAddCommentPanel() return null in https://we.phorge.it/source/phorge/browse/master/src/applications/diffusion/controller/DiffusionCommitController.php$767 and hiding the "Edit Commit" (and potentially "Edit Related Objects"?) sidebar item in buildCurtain() in https://we.phorge.it/source/phorge/browse/master/src/applications/diffusion/controller/DiffusionCommitController.php$835

aklapper updated this revision to Diff 1608.Jan 4 2024, 07:38

Remove a forgotten debug line

Harbormaster completed remote builds in B1000: Diff 1608.Jan 4 2024, 07:38
valerio.bozzolan awarded a token.Jan 7 2024, 10:03
valerio.bozzolan added a comment.Jan 7 2024, 10:09

You are doing a nice work.

Ideally, comments are a general things and should be kept. Also changing Tags and Subscribers etc. is generic and should be kept.

From the commit edit page:

https://we.phorge.it/diffusion/commit/edit/22566/

We should just remove Auditors, but keep Tags and Subscribers.

From the commit view page:

https://we.phorge.it/rP428f9686c4171912ee186ebd919640a7427da768

We probably can remove from the "Add Action" just these specific to Audit:

  • Accept revision
  • Request changes
  • Resign as reviewers
  • Commander revision
  • Change reviewers

But we should keep these or similar:

  • Change Project Tags
  • Change Subscribers
  • Sign with MFA
valerio.bozzolan added inline comments.Jan 7 2024, 10:19
src/applications/diffusion/editor/DiffusionCommitEditEngine.php
120–121

What about something magic and generic like this instead?

diff --git a/src/applications/diffusion/xaction/DiffusionCommitActionTransaction.php b/src/applications/diffusion/xaction/DiffusionCommitActionTransaction.php
index 1d351ffa5d..d381602766 100644
--- a/src/applications/diffusion/xaction/DiffusionCommitActionTransaction.php
+++ b/src/applications/diffusion/xaction/DiffusionCommitActionTransaction.php
@@ -8,6 +8,9 @@ abstract class DiffusionCommitActionTransaction
   }
 
   public function isActionAvailable($object, PhabricatorUser $viewer) {
+    if (!id(new PhabricatorAuditApplication())->isInstalled()) {
+      return false;
+    }
     try {
       $this->validateAction($object, $viewer);
       return true;
valerio.bozzolan added inline comments.Jan 8 2024, 13:19
src/applications/audit/query/PhabricatorCommitSearchEngine.php
234–238

Maybe this could just be:

$show_auditors =
  id(new PhabricatorAuditApplication())->isInstalled();
aklapper added a comment.EditedJan 11 2024, 21:20

Ideally, comments are a general things and should be kept. Also changing Tags and Subscribers etc. is generic and should be kept.

Being "generic" per se is not a great argument so I'd love to understand use cases better. When people look at a merged commit in Diffusion, I am generally wondering why/when would they subscribe to that or add tags to it? (Maybe commenting on it still makes sense, though in my understanding that only makes sense when that repository is also canonically hosted in Diffusion and not a mirrored/observed one.)

But yes I understand that this aspect will be out of scope for this very upstream patch. I might introduce a downstream patch (because vandalism).

aklapper marked 2 inline comments as done.Jan 11 2024, 23:21

Just for the records and bystanders: All expectations described in D25503#14539 are also what the patch currently does.

src/applications/diffusion/editor/DiffusionCommitEditEngine.php
120–121

That works and is so much cleaner, thanks!

aklapper updated this revision to Diff 1623.Jan 11 2024, 23:22
aklapper marked an inline comment as done.

Simplify some code / fix TODOs as proposed by Valerio

Harbormaster completed remote builds in B1012: Diff 1623.Jan 11 2024, 23:22
valerio.bozzolan accepted this revision.Jan 12 2024, 09:10

I'm testing this. It works and this makes me happy. Please wait Sunday before landing this, so others can give an eye.

Just this cosmetic change maybe:

  • the "Bucket" (group by "Required Action") in /diffusion/commit/query/advanced/ could be omitted since it's supposed to don't do anything useful in this case

Thanks again

This revision is now accepted and ready to land.Jan 12 2024, 09:10
aklapper updated this revision to Diff 1631.Jan 12 2024, 12:25

Also hide "Buckets" search field on diffusion/commit/query/advanced/ when Audit is uninstalled

Harbormaster completed remote builds in B1018: Diff 1631.Jan 12 2024, 12:25
Closed by commit rP821708414eec: Allow to uninstall (hide) Audit application. · Explain WhyFeb 12 2024, 11:10
This revision was automatically updated to reflect the committed changes.
aklapper added a commit: rP821708414eec: Allow to uninstall (hide) Audit application.
valerio.bozzolan mentioned this in Next Up.Feb 12 2024, 11:11
avivey mentioned this in 2024 week 19.May 11 2024, 04:37
valerio.bozzolan mentioned this in D25894: Hide "Audits" section in Email Preferences when uninstalled.Thu, Apr 24, 09:28

Revision Contents
Changeset List

Diff 1694

View Options

src/applications/audit/application/PhabricatorAuditApplication.php

Show First 20 Lines • Show All 172 Lines • ▼ Show 20 Linesfinal class DifferentialDiffViewController extends DifferentialController {
} }
private function loadSelectableRevisions( private function loadSelectableRevisions(
PhabricatorUser $viewer, PhabricatorUser $viewer,
$selected_id) { $selected_id) {
$revisions = id(new DifferentialRevisionQuery()) $revisions = id(new DifferentialRevisionQuery())
->setViewer($viewer) ->setViewer($viewer)
->withAuthors(array($viewer->getPHID())) ->withAuthors(array($viewer->getPHID()))
valerio.bozzolanAuthorUnsubmitted
Done
Inline Actions

This is kept intentionally.

valerio.bozzolan: This is kept intentionally.
->withIsOpen(true) ->withIsOpen(true)
->requireCapabilities( ->requireCapabilities(
array( array(
PhabricatorPolicyCapability::CAN_VIEW, PhabricatorPolicyCapability::CAN_VIEW,
PhabricatorPolicyCapability::CAN_EDIT, PhabricatorPolicyCapability::CAN_EDIT,
)) ))
->execute(); ->execute();
$revisions = mpull($revisions, null, 'getID'); $revisions = mpull($revisions, null, 'getID');
// If a specific revision is selected (for example, because the user is // If a specific revision is selected (for example, because the user is
// following the "Update Diff" workflow), but not present in the dropdown, // following the "Update Diff" workflow), but not present in the dropdown,
// try to add it to the dropdown even if it is closed. This allows the // try to add it to the dropdown even if it is closed. This allows the
// workflow to be used to update abandoned revisions. // workflow to be used to update abandoned revisions.
if ($selected_id) { if ($selected_id) {
if (empty($revisions[$selected_id])) { if (empty($revisions[$selected_id])) {
$selected = id(new DifferentialRevisionQuery()) $selected = id(new DifferentialRevisionQuery())
->setViewer($viewer) ->setViewer($viewer)
->withAuthors(array($viewer->getPHID()))
->withIDs(array($selected_id)) ->withIDs(array($selected_id))
->requireCapabilities( ->requireCapabilities(
array( array(
PhabricatorPolicyCapability::CAN_VIEW, PhabricatorPolicyCapability::CAN_VIEW,
PhabricatorPolicyCapability::CAN_EDIT, PhabricatorPolicyCapability::CAN_EDIT,
)) ))
->execute(); ->execute();
$revisions = mpull($selected, null, 'getID') + $revisions; $revisions = mpull($selected, null, 'getID') + $revisions;
} }
} }
return $revisions; return $revisions;
} }
} }
View Options

src/applications/audit/conduit/AuditConduitAPIMethod.php

Show First 20 Lines • Show All 172 Lines • ▼ Show 20 Linesfinal class DifferentialDiffViewController extends DifferentialController {
} }
private function loadSelectableRevisions( private function loadSelectableRevisions(
PhabricatorUser $viewer, PhabricatorUser $viewer,
$selected_id) { $selected_id) {
$revisions = id(new DifferentialRevisionQuery()) $revisions = id(new DifferentialRevisionQuery())
->setViewer($viewer) ->setViewer($viewer)
->withAuthors(array($viewer->getPHID())) ->withAuthors(array($viewer->getPHID()))
valerio.bozzolanAuthorUnsubmitted
Done
Inline Actions

This is kept intentionally.

valerio.bozzolan: This is kept intentionally.
->withIsOpen(true) ->withIsOpen(true)
->requireCapabilities( ->requireCapabilities(
array( array(
PhabricatorPolicyCapability::CAN_VIEW, PhabricatorPolicyCapability::CAN_VIEW,
PhabricatorPolicyCapability::CAN_EDIT, PhabricatorPolicyCapability::CAN_EDIT,
)) ))
->execute(); ->execute();
$revisions = mpull($revisions, null, 'getID'); $revisions = mpull($revisions, null, 'getID');
// If a specific revision is selected (for example, because the user is // If a specific revision is selected (for example, because the user is
// following the "Update Diff" workflow), but not present in the dropdown, // following the "Update Diff" workflow), but not present in the dropdown,
// try to add it to the dropdown even if it is closed. This allows the // try to add it to the dropdown even if it is closed. This allows the
// workflow to be used to update abandoned revisions. // workflow to be used to update abandoned revisions.
if ($selected_id) { if ($selected_id) {
if (empty($revisions[$selected_id])) { if (empty($revisions[$selected_id])) {
$selected = id(new DifferentialRevisionQuery()) $selected = id(new DifferentialRevisionQuery())
->setViewer($viewer) ->setViewer($viewer)
->withAuthors(array($viewer->getPHID()))
->withIDs(array($selected_id)) ->withIDs(array($selected_id))
->requireCapabilities( ->requireCapabilities(
array( array(
PhabricatorPolicyCapability::CAN_VIEW, PhabricatorPolicyCapability::CAN_VIEW,
PhabricatorPolicyCapability::CAN_EDIT, PhabricatorPolicyCapability::CAN_EDIT,
)) ))
->execute(); ->execute();
$revisions = mpull($selected, null, 'getID') + $revisions; $revisions = mpull($selected, null, 'getID') + $revisions;
} }
} }
return $revisions; return $revisions;
} }
} }
View Options

src/applications/audit/query/PhabricatorCommitSearchEngine.php

Show First 20 Lines • Show All 172 Lines • ▼ Show 20 Linesfinal class DifferentialDiffViewController extends DifferentialController {
} }
private function loadSelectableRevisions( private function loadSelectableRevisions(
PhabricatorUser $viewer, PhabricatorUser $viewer,
$selected_id) { $selected_id) {
$revisions = id(new DifferentialRevisionQuery()) $revisions = id(new DifferentialRevisionQuery())
->setViewer($viewer) ->setViewer($viewer)
->withAuthors(array($viewer->getPHID())) ->withAuthors(array($viewer->getPHID()))
valerio.bozzolanAuthorUnsubmitted
Done
Inline Actions

This is kept intentionally.

valerio.bozzolan: This is kept intentionally.
->withIsOpen(true) ->withIsOpen(true)
->requireCapabilities( ->requireCapabilities(
array( array(
PhabricatorPolicyCapability::CAN_VIEW, PhabricatorPolicyCapability::CAN_VIEW,
PhabricatorPolicyCapability::CAN_EDIT, PhabricatorPolicyCapability::CAN_EDIT,
)) ))
->execute(); ->execute();
$revisions = mpull($revisions, null, 'getID'); $revisions = mpull($revisions, null, 'getID');
// If a specific revision is selected (for example, because the user is // If a specific revision is selected (for example, because the user is
// following the "Update Diff" workflow), but not present in the dropdown, // following the "Update Diff" workflow), but not present in the dropdown,
// try to add it to the dropdown even if it is closed. This allows the // try to add it to the dropdown even if it is closed. This allows the
// workflow to be used to update abandoned revisions. // workflow to be used to update abandoned revisions.
if ($selected_id) { if ($selected_id) {
if (empty($revisions[$selected_id])) { if (empty($revisions[$selected_id])) {
$selected = id(new DifferentialRevisionQuery()) $selected = id(new DifferentialRevisionQuery())
->setViewer($viewer) ->setViewer($viewer)
->withAuthors(array($viewer->getPHID()))
->withIDs(array($selected_id)) ->withIDs(array($selected_id))
->requireCapabilities( ->requireCapabilities(
array( array(
PhabricatorPolicyCapability::CAN_VIEW, PhabricatorPolicyCapability::CAN_VIEW,
PhabricatorPolicyCapability::CAN_EDIT, PhabricatorPolicyCapability::CAN_EDIT,
)) ))
->execute(); ->execute();
$revisions = mpull($selected, null, 'getID') + $revisions; $revisions = mpull($selected, null, 'getID') + $revisions;
} }
} }
return $revisions; return $revisions;
} }
} }
View Options

src/applications/diffusion/editor/DiffusionCommitEditEngine.php

Show First 20 Lines • Show All 172 Lines • ▼ Show 20 Linesfinal class DifferentialDiffViewController extends DifferentialController {
} }
private function loadSelectableRevisions( private function loadSelectableRevisions(
PhabricatorUser $viewer, PhabricatorUser $viewer,
$selected_id) { $selected_id) {
$revisions = id(new DifferentialRevisionQuery()) $revisions = id(new DifferentialRevisionQuery())
->setViewer($viewer) ->setViewer($viewer)
->withAuthors(array($viewer->getPHID())) ->withAuthors(array($viewer->getPHID()))
valerio.bozzolanAuthorUnsubmitted
Done
Inline Actions

This is kept intentionally.

valerio.bozzolan: This is kept intentionally.
->withIsOpen(true) ->withIsOpen(true)
->requireCapabilities( ->requireCapabilities(
array( array(
PhabricatorPolicyCapability::CAN_VIEW, PhabricatorPolicyCapability::CAN_VIEW,
PhabricatorPolicyCapability::CAN_EDIT, PhabricatorPolicyCapability::CAN_EDIT,
)) ))
->execute(); ->execute();
$revisions = mpull($revisions, null, 'getID'); $revisions = mpull($revisions, null, 'getID');
// If a specific revision is selected (for example, because the user is // If a specific revision is selected (for example, because the user is
// following the "Update Diff" workflow), but not present in the dropdown, // following the "Update Diff" workflow), but not present in the dropdown,
// try to add it to the dropdown even if it is closed. This allows the // try to add it to the dropdown even if it is closed. This allows the
// workflow to be used to update abandoned revisions. // workflow to be used to update abandoned revisions.
if ($selected_id) { if ($selected_id) {
if (empty($revisions[$selected_id])) { if (empty($revisions[$selected_id])) {
$selected = id(new DifferentialRevisionQuery()) $selected = id(new DifferentialRevisionQuery())
->setViewer($viewer) ->setViewer($viewer)
->withAuthors(array($viewer->getPHID()))
->withIDs(array($selected_id)) ->withIDs(array($selected_id))
->requireCapabilities( ->requireCapabilities(
array( array(
PhabricatorPolicyCapability::CAN_VIEW, PhabricatorPolicyCapability::CAN_VIEW,
PhabricatorPolicyCapability::CAN_EDIT, PhabricatorPolicyCapability::CAN_EDIT,
)) ))
->execute(); ->execute();
$revisions = mpull($selected, null, 'getID') + $revisions; $revisions = mpull($selected, null, 'getID') + $revisions;
} }
} }
return $revisions; return $revisions;
} }
} }
View Options

src/applications/diffusion/herald/DiffusionAuditorsAddAuditorsHeraldAction.php

Loading...
View Options

src/applications/diffusion/herald/DiffusionAuditorsAddSelfHeraldAction.php

Loading...
View Options

src/applications/diffusion/herald/DiffusionCommitAuditorsHeraldField.php

Loading...
View Options

src/applications/diffusion/herald/DiffusionCommitPackageAuditHeraldField.php

Loading...
View Options

src/applications/diffusion/view/DiffusionCommitGraphView.php

Loading...
View Options

src/applications/diffusion/xaction/DiffusionCommitActionTransaction.php

Loading...
View Options

src/applications/search/engine/PhabricatorApplicationSearchEngine.php

Loading...
Content licensed under Creative Commons Attribution-ShareAlike 4.0 (CC-BY-SA) unless otherwise noted; code licensed under Apache 2.0 or other open source licenses. · CC BY-SA 4.0 · Apache 2.0