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)
Sat, Dec 14, 6:05 AM
Unknown Object (File)
Sun, Dec 8, 5:19 AM
Unknown Object (File)
Fri, Dec 6, 4:35 PM
Unknown Object (File)
Fri, Nov 29, 12:42 PM
Unknown Object (File)
Wed, Nov 27, 7:02 AM
Unknown Object (File)
Sat, Nov 23, 4:21 AM
Unknown Object (File)
Fri, Nov 22, 5:38 AM
Unknown Object (File)
Thu, Nov 21, 6:50 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.