Page MenuHomePhorge

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

Authored by valerio.bozzolan on Aug 28 2023, 12:09.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Jun 10, 12:54
Unknown Object (File)
Sun, Jun 9, 14:13
Unknown Object (File)
Sat, Jun 8, 13:23
Unknown Object (File)
Sat, Jun 8, 02:45
Unknown Object (File)
Sat, Jun 8, 01:28
Unknown Object (File)
Thu, Jun 6, 22:27
Unknown Object (File)
Thu, Jun 6, 20:24
Unknown Object (File)
Wed, Jun 5, 22:11

Details

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.

BeforeAfter
Committer Feed before D25421.png (836×1 px, 152 KB)
Committer Feed after D25421.png (836×1 px, 140 KB)

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 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
src/applications/audit/storage/PhabricatorAuditTransaction.php
341–351

✅ This is the supposed non-empty check for a PHID. See line 323.

speck subscribed.

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
src/applications/audit/storage/PhabricatorAuditTransaction.php
341–351

Thanks speck. I tried for some minutes but failing.

Any idea?

src/applications/audit/storage/PhabricatorAuditTransaction.php
341–351

Tried again refactoring, without success 🤔 Uhm. Need some sleep and/or help.

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

(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