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

Branch

T15129hideAudit (branched from master)

Lint

Lint Passed

Unit

Tests Passed

Build Status

Buildable 1018
Build 1018: arc lint + arc unit

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

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

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

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();

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).

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!

Simplify some code / fix TODOs as proposed by Valerio

Harbormaster completed remote builds in B1012: Diff 1623.Jan 11 2024, 23:22

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

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

Allow to uninstall (hide) Audit application
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 1631

src/applications/audit/application/PhabricatorAuditApplication.php

src/applications/audit/conduit/AuditConduitAPIMethod.php

src/applications/audit/query/PhabricatorCommitSearchEngine.php

src/applications/diffusion/editor/DiffusionCommitEditEngine.php

src/applications/diffusion/herald/DiffusionAuditorsAddAuditorsHeraldAction.php

src/applications/diffusion/herald/DiffusionAuditorsAddSelfHeraldAction.php

src/applications/diffusion/herald/DiffusionCommitAuditorsHeraldField.php

src/applications/diffusion/herald/DiffusionCommitPackageAuditHeraldField.php

src/applications/diffusion/view/DiffusionCommitGraphView.php

src/applications/diffusion/xaction/DiffusionCommitActionTransaction.php

src/applications/search/engine/PhabricatorApplicationSearchEngine.php

Allow to uninstall (hide) Audit applicationClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 1631

src/applications/audit/application/PhabricatorAuditApplication.php

src/applications/audit/conduit/AuditConduitAPIMethod.php

src/applications/audit/query/PhabricatorCommitSearchEngine.php

src/applications/diffusion/editor/DiffusionCommitEditEngine.php

src/applications/diffusion/herald/DiffusionAuditorsAddAuditorsHeraldAction.php

src/applications/diffusion/herald/DiffusionAuditorsAddSelfHeraldAction.php

src/applications/diffusion/herald/DiffusionCommitAuditorsHeraldField.php

src/applications/diffusion/herald/DiffusionCommitPackageAuditHeraldField.php

src/applications/diffusion/view/DiffusionCommitGraphView.php

src/applications/diffusion/xaction/DiffusionCommitActionTransaction.php

src/applications/search/engine/PhabricatorApplicationSearchEngine.php

Allow to uninstall (hide) Audit application
ClosedPublic
Actions

Revision Contents
Changeset List