Page MenuHomePhabricator

Fix handling of notifications with project members
ClosedPublic

Authored by epriestley on Mar 24 2015, 1:40 PM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 15, 7:06 AM
Unknown Object (File)
Tue, Apr 2, 11:22 AM
Unknown Object (File)
Sat, Mar 30, 1:23 AM
Unknown Object (File)
Thu, Mar 28, 6:52 PM
Unknown Object (File)
Mar 10 2024, 11:15 AM
Unknown Object (File)
Mar 3 2024, 1:02 PM
Unknown Object (File)
Feb 14 2024, 7:42 PM
Unknown Object (File)
Feb 7 2024, 7:24 PM
Subscribers
Tokens
"Grey Medal" token, awarded by btrahan.

Details

Summary

Fixes T7377. We don't expand projects into members when sending notifications right now. Instead, expand them.

Test Plan
  • Added a project as a reviewer to a revision, made a comment, saw project members receive a read notification + email (with appropriate preferences).
  • There's meaningful test coverage on the core mail stuff.

Diff Detail

Repository
rP Phabricator
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

epriestley retitled this revision from to Fix handling of notifications with project members.
epriestley updated this object.
epriestley edited the test plan for this revision. (Show Details)
epriestley added a reviewer: btrahan.
src/applications/feed/PhabricatorFeedStoryPublisher.php
195

This is the actual change.

src/applications/metamta/query/PhabricatorMetaMTAMemberQuery.php
62–66

This allows us to implement executeExpansion() easily and remove some code.

79–81

This lets us remove a bunch of redundant code.

src/applications/metamta/storage/PhabricatorMetaMTAMail.php
784

This is a behavioral change. Previously, members who were recipients via projects would not be returned, because this method did not expand projects. Now, we expand projects and return those recipients.

The old behavior was incorrect, because these members do receive mail. However, the bug it would have caused is "notifications received by project members are incorrectly delivered unread when they should be delivered read", and that bug was masked by the root bug here (those notifications not being delivered at all).

So I fixed the "no notifications" bug, and hit this bug (incorrectly marked "unread" notifications), and fixed it with this change.

  • Be more defensive in loading the expansion map.
btrahan edited edge metadata.
This revision is now accepted and ready to land.Mar 24 2015, 6:03 PM
This revision was automatically updated to reflect the committed changes.