Page MenuHomePhorge

Allow to uninstall (hide) Audit application
ClosedPublic

Authored by aklapper on Jan 4 2024, 07:34.
Tags
None
Referenced Files
F1392455: D25503.id1694.diff
Tue, Feb 20, 17:00
Unknown Object (File)
Mon, Feb 19, 14:26
Unknown Object (File)
Mon, Feb 12, 04:40
Unknown Object (File)
Wed, Jan 31, 16:47
Unknown Object (File)
Tue, Jan 30, 14:48
Unknown Object (File)
Tue, Jan 30, 14:13
Unknown Object (File)
Sat, Jan 27, 12:57
Unknown Object (File)
Fri, Jan 26, 14:43
Tokens
"Like" token, awarded by valerio.bozzolan.

Details

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 requested review of this revision.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

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
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;
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!

aklapper marked an inline comment as done.

Simplify some code / fix TODOs as proposed by Valerio

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