Skip to content

Commit 6e85b52

Browse files
author
epriestley
committedJul 9, 2020
Don't raise the "Subscribers Won't Be Notified" draft warning if you aren't adding any non-you subscribers
Summary: Currently, adding subscribers to a draft revision raises a warning that they won't get an email/notification. This warning has some false positives: - it triggers on any subscriber change, including removing subscribers; and - it triggers if you're only adding yourself as a subscriber. Narrow the scope of the warning so it is raised only if you're adding a subscriber other than yourself. Test Plan: - Added a non-self subscriber, got the warning as before. - Added self as a subscriber, no warning (previously: warning). - Removed a subscriber, no warning (previously: warning). Differential Revision: https://secure.phabricator.com/D21402
1 parent b21b73b commit 6e85b52

File tree

1 file changed

+16
-4
lines changed

1 file changed

+16
-4
lines changed
 

‎src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

+16-4
Original file line numberDiff line numberDiff line change
@@ -4968,8 +4968,7 @@ private function queueWebhooks($object, array $xactions) {
49684968

49694969
private function hasWarnings($object, $xaction) {
49704970
// TODO: For the moment, this is a very un-modular hack to support
4971-
// exactly one type of warning (mentioning users on a draft revision)
4972-
// that we want to show. See PHI433.
4971+
// a small number of warnings related to draft revisions. See PHI433.
49734972

49744973
if (!($object instanceof DifferentialRevision)) {
49754974
return false;
@@ -4991,8 +4990,21 @@ private function hasWarnings($object, $xaction) {
49914990
return false;
49924991
}
49934992

4994-
// NOTE: This will currently warn even if you're only removing
4995-
// subscribers.
4993+
// We're only going to raise a warning if the transaction adds subscribers
4994+
// other than the acting user. (This implementation is clumsy because the
4995+
// code runs before a lot of normalization occurs.)
4996+
4997+
$old = $this->getTransactionOldValue($object, $xaction);
4998+
$new = $this->getPHIDTransactionNewValue($xaction, $old);
4999+
$old = array_fuse($old);
5000+
$new = array_fuse($new);
5001+
$add = array_diff_key($new, $old);
5002+
5003+
unset($add[$this->getActingAsPHID()]);
5004+
5005+
if (!$add) {
5006+
return false;
5007+
}
49965008

49975009
return true;
49985010
}

0 commit comments

Comments
 (0)
Failed to load comments.