Details

Reviewers

speck

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T15528: Diffusion commit Feed: avoid to show the author twice (Authored by xxx)

Commits

rP775d141fe6bf: Audit Feed: less verbose when the author is the committer

Summary

If the author and the committer are the same person, do not show them twice.

From:

UsernameFoo committed <commit hash>: <commit msg> (authored by UsernameFoo).

To:

UsernameFoo committed <commit hash>: <commit msg>

This only affects the feed.

Before	After

Closes T15528

Test Plan

Do some mixed commits and visit /feed/query/all/. No nuclear implosions.

Diff Detail

Repository

rP Phorge

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

valerio.bozzolan created this revision.Aug 28 2023, 12:09

Owners added a reviewer: O1: Blessed Committers.Aug 28 2023, 12:09

Herald added subscribers: Cigaryno, Matthew, tobiaswiese. · View Herald TranscriptAug 28 2023, 12:09

Harbormaster completed remote builds in B809: Diff 1336.Aug 28 2023, 12:09

valerio.bozzolan requested review of this revision.Aug 28 2023, 12:09

fix corner case of NULL phid

Harbormaster completed remote builds in B810: Diff 1337.Aug 28 2023, 12:12

valerio.bozzolan edited the summary of this revision. (Show Details)Aug 28 2023, 12:28

consider more cases

Harbormaster completed remote builds in B811: Diff 1338.Aug 28 2023, 12:30

valerio.bozzolan retitled this revision from Audit: do not show same author twice in Feed to Audit Feed: less verbose when the author is the committer.Aug 28 2023, 13:37

valerio.bozzolan added inline comments.Sep 6 2023, 11:09

src/applications/audit/storage/PhabricatorAuditTransaction.php
341–351	✅ This is the supposed non-empty check for a PHID. See line 323.

Basically it works

Basically it works

😂

Great QoL change, thank you! I just had one thing to consider but leave it you to you to address or not before landing.

src/applications/audit/storage/PhabricatorAuditTransaction.php
341–351	What do you think of merging this logic above with the existing checks on author and committer?

This revision is now accepted and ready to land.Nov 4 2023, 15:07

valerio.bozzolan added inline comments.Nov 7 2023, 09:08

src/applications/audit/storage/PhabricatorAuditTransaction.php
341–351	Thanks speck. I tried for some minutes but failing. Any idea?

valerio.bozzolan added inline comments.Nov 9 2023, 20:08

src/applications/audit/storage/PhabricatorAuditTransaction.php
341–351	Tried again refactoring, without success 🤔 Uhm. Need some sleep and/or help.

speck added inline comments.Nov 10 2023, 04:04

src/applications/audit/storage/PhabricatorAuditTransaction.php
322–352	This is what I was thinking but after taking further look I think I prefer the version you have.

Small improvement thanks to review tip

Harbormaster completed remote builds in B921: Diff 1497.Nov 13 2023, 10:26

(Hide all inline comments or this is unreadable)

Thanks for the tip. Maybe better now?

(This was already approved but I wait for another "Yeah land" since I changed a thing)

Minor, just suggestion

src/applications/audit/storage/PhabricatorAuditTransaction.php
343	This reads a little strange now. I think this check is fine to remain as “if author”

avoid micro-optimizations that make this unreadable

Harbormaster completed remote builds in B941: Diff 1519.Nov 15 2023, 08:41

speck accepted this revision.Nov 15 2023, 12:49

Closed by commit rP775d141fe6bf: Audit Feed: less verbose when the author is the committer. · Explain WhyNov 15 2023, 12:57

This revision was automatically updated to reflect the committed changes.

valerio.bozzolan added a commit: rP775d141fe6bf: Audit Feed: less verbose when the author is the committer.

valerio.bozzolan mentioned this in Next Up.Nov 15 2023, 12:58

avivey mentioned this in 2023 week 49.Dec 8 2023, 17:36

Audit Feed: less verbose when the author is the committer
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 1520

src/applications/audit/storage/PhabricatorAuditTransaction.php

Audit Feed: less verbose when the author is the committerClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 1520

src/applications/audit/storage/PhabricatorAuditTransaction.php

Audit Feed: less verbose when the author is the committer
ClosedPublic
Actions

Revision Contents
Changeset List