Paths

Table of Contentst

Differential D25968

Mail Notifications: Do not CC disabled user accounts
Needs RevisionPublic
Actions

Authored by aklapper on Apr 21 2025, 19:51.

Details

Reviewers

valerio.bozzolan

Group Reviewers

O1: Blessed Committers

Maniphest Tasks

T16033: "To:" and "CC:" Recipient Hint footers in mail notifications list disabled user accounts

Summary

Do not list disabled user accounts in mail footer (when metamta.recipients.show-hints is enabled) and do not add them to the X-Phabricator-Cc header, as they are not supposed to receive mail anyway per https://we.phorge.it/source/phorge/browse/master/src/applications/metamta/query/PhabricatorMetaMTAActor.php;07723b46274c6dbf062af070e5c19101537e0d91$130-131.

Closes T16033

Test Plan

Likely trigger notifications on a task or changeset with disabled user accounts being subscribed or members/watchers of a project. Not tested though as I do not have a local mail setup.

Diff Detail

Repository

rP Phorge

Branch

T16033disabledRecipients (branched from master)

Lint

Lint Passed

Unit

Test Failures

Build Status

Buildable 1878
Build 1878: arc lint + arc unit

Unit TestsFailed
View All

	Time	Test
	601 ms	PhabricatorCelerityTestCase::testCelerityMaps Assertion failed, expected 'true' (at PhabricatorCelerityTestCase.php:27): When this test fails, it means the Celerity resource map is out of date. Run `bin/celerity map` to rebuild it. ACTUAL VALUE
	772 ms	PhabricatorConduitTestCase::testConduitMethods 1 assertion passed.
	441 ms	PhabricatorInfrastructureTestCase::testApplicationsInstalled 1 assertion passed.
	467 ms	PhabricatorInfrastructureTestCase::testRejectMySQLNonUTF8Queries 1 assertion passed.
	493 ms	PhabricatorLibraryTestCase::testEverythingImplemented 1 assertion passed.
		View Full Test Results (1 Failed · 6 Passed)

Event Timeline

aklapper created this revision.Apr 21 2025, 19:51

Owners added a reviewer: O1: Blessed Committers.Apr 21 2025, 19:51

Herald added subscribers: Cigaryno, Matthew, valerio.bozzolan, tobiaswiese. · View Herald TranscriptApr 21 2025, 19:51

aklapper requested review of this revision.Apr 21 2025, 19:51

Harbormaster completed remote builds in B1878: Diff 2892.Apr 21 2025, 19:51

Clean my local dir from other experiments

Harbormaster completed remote builds in B1879: Diff 2893.Apr 21 2025, 19:55

Don't check twice for account status; already covered by query parameter

Harbormaster completed remote builds in B1880: Diff 2894.Apr 21 2025, 19:57

I guess I should add a big This is untested banner here

valerio.bozzolan added inline comments.Wed, Apr 23, 07:12

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php
3459	Can we really assume that `$email_cc` always has the keys as this? Probably instead of just unset by key, it's necessary to search by value -> unset by its key.

Mark as "some comments"

This revision now requires changes to proceed.Sat, May 3, 16:36

Revision Contents
Changeset List

Path

Size

Coverage (All)

Coverage (Touched)

Packages

src/

applications/

transactions/

editor/

PhabricatorApplicationTransactionEditor.php

14 lines

O1: Blessed Committers

Diff 2892

View Options

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

Mail Notifications: Do not CC disabled user accountsNeeds RevisionPublicActions