3985: should_send_own_email notifications broken (again)

elatt
brennie
brennie

What version are you running?

2.0.20

What's the URL of the page containing the problem?

N/A

What steps will reproduce the problem?

  1. User A should create a review request and assing a group that User B is a member of
  2. User B should update their profile to not send own updates
  3. User B should make a comment on the review.

What is the expected output? What do you see instead?

User B should not recieve an email; however, one is sent.

What operating system are you using? What browser?

N/A

Please provide any additional information below.

This seems to be a reincarnation of https://code.google.com/p/reviewboard/issues/detail?id=3684 that was caused by recent refactoring (https://github.com/reviewboard/reviewboard/commit/0734e874e863ebf2ae2bbe98caaba9b3e8a2f4b0#diff-1d2319bf440bfa57b72a6e1cb7c8969b). I have a very crude fix for the problem but am not happy with it. I would like to discuss the contract for the new method build_recipients() as I like the refactoring as it makes this logic easier to test and therefore we should be able to prevent future regressions. However, I feel the current implementation of the method is a little broken. It's probably easier for me to comment on the original review request. I've also attached my patch for completelness.

--- /usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.20-py2.7.egg/reviewboard/notifications/email.py.orig	2015-09-28 10:55:41.455778481 -0400
+++ /usr/local/lib/python2.7/dist-packages/ReviewBoard-2.0.20-py2.7.egg/reviewboard/notifications/email.py	2015-09-24 17:41:52.342091716 -0400
@@ -433,6 +433,7 @@
         recipients.update(to_field)
         recipients.update(review_request.target_groups.all())
 
+    #XXX why bother doing this here if it doesn't work?
     if not user.should_send_own_updates():
         recipients.discard(user)
         to_field.discard(user)
@@ -443,6 +444,7 @@
         to_field = recipients
         cc_field = set()
 
+    logging.debug("email lists -- TO: %s\tCC: %s", to_field, cc_field)
     return to_field, cc_field
 
 
@@ -550,8 +552,12 @@
     to_field = recipients_to_addresses(to_field)
     cc_field = recipients_to_addresses(cc_field) - to_field
 
+    #XXX why didn't they mess with the CC field here too?
     if not user.should_send_own_updates(
#1 elatt

https://reviews.reviewboard.org/r/7560/ is original review request that I just posted comments to.

brennie
#2 brennie

I responded to your issues on https://reviews.reviewboard.org/r/7560/. I'd prefer if discussion stayed here as its easier to follow one thread of discussion.

I'll take a look at reproducing and then fixing this.

  • +brennie
#3 elatt

That's fine, we can discuss here as I don't get updates when comments are posted on the review request. Regarding your comment:

Barret Rennie Barret Rennie
8 minutes ago
On line 490:

cc_field = recipeints_to_addresses(cc_field) - to_field

This ensures that any address in to_field is not in cc_field, so the user's e-mail address cannot be in cc_field.

If the user has should_send_own_email set to false, build_recipients() will have stripped the user from the to_field so I think your assumption about the code above is false.

brennie
#4 brennie

I'm having issues reproducing this issue. I've attached my unit test and it currently passes.

The unit test is also inline for convenience:

    def test_group_member_not_receive_email(self):
        reviewer = User.objects.get(username='dopey')
        profile = Profile.objects.get_or_create(user=reviewer)[0]
        profile.should_send_own_updates = False
        profile.save()

        group = self.create_review_group()
        group.users.add(reviewer)

        review_request = self.create_review_request(public=True)
        review_request.target_groups.add(group)
        review_request.save()

        review = self.create_review(review_request)
        review.publish()

        self.assertEqual(len(mail.outbox), 1)
        msg = mail.outbox[0]

        self.assertListEqual(
            msg.to,
            [get_email_address_for_user(review_request.submitter)])

        self.assertListEqual(msg.cc, [])

Here is the test running output:

./reviewboard/manage.py test -- reviewboard.notifications.tests:ReviewRequestEmailTests.test_group_member_not_receive_email
Running dependency checks (set DEBUG=False to turn this off)...
Warning: bzrlib not found. Bazaar integration will not work.
Warning: mtn binary not found. Monotone integration will not work.

Please see https://www.reviewboard.org/docs/manual/dev/admin/
for help setting up Review Board.


Creating test database for alias 'default'...
test_group_member_not_receive_email (reviewboard.notifications.tests.ReviewRequestEmailTests) ... ok

----------------------------------------------------------------------
Ran 1 test in 0.421s

OK
Destroying test database for alias 'default'...
  • +
    diff --git a/reviewboard/notifications/tests.py b/reviewboard/notifications/tests.py
    index bf5117a..75d9fb1 100644
    --- a/reviewboard/notifications/tests.py
    +++ b/reviewboard/notifications/tests.py
    @@ -505,6 +505,31 @@ class ReviewRequestEmailTests(EmailTestHelper, SpyAgency, TestCase):
     
             self.assertEqual(len(mail.outbox), 0)
     
    +    def test_group_member_not_receive_email(self):
    +        reviewer = User.objects.get(username='dopey')
    +        profile = Profile.objects.get_or_create(user=reviewer)[0]
    +        profile.should_send_own_updates = False
    +        profile.save()
    +
    +        group = self.create_review_group()
    +        group.users.add(reviewer)
    +
    +        review_request = self.create_review_request(public=True)
    +        review_request.target_groups.add(group)
    +        review_request.save()
    +
    +        review = self.create_review(review_request)
    +        review.publish()
    +
    +        self.assertEqual(len(mail.outbox), 1)
    +        msg = mail.outbox[0]
    +
    +        self.assertList
brennie
#5 brennie

Re: #3,

If the user has should_send_own_email = False, then it will be discarded from both to_field and recipients and therefore cannot make it into the cc_field. See lines 379--385.

Can you attach a unit test that reproduces this behaviour?

brennie
#6 brennie

I also tried to reproduce with the following unit test, but it works correctly:

    def test_group_member_not_receive_email(self):
        reviewer = User.objects.get(username='doc')
        profile = Profile.objects.get_or_create(user=reviewer)[0]
        profile.should_send_own_updates = False
        profile.save()

        group = self.create_review_group()
        group.users.add(reviewer)

        review_request = self.create_review_request(public=True)
        review_request.target_groups.add(group)
        review_request.save()

        review = self.create_review(review_request, user=reviewer)
        review.publish()

        self.assertEqual(len(mail.outbox), 0)

That is, the review request submitted is a member of the group that the review request is assigned to. The submitted makes a comment and no e-mail is sent.

#7 elatt

I think my original repro steps were incorrect. The case where I tested this behavior was where I was the submitter, and I have should_send_own_email set to False, and I replied to a comment on the review. So to be clear:
1. User A sets should_send_own_email to False
2. User A creates review request and adds group that User A is a member of
3. User A comments on review
User A should not recieve emails for any of either step 2. or 3.

#8 elatt

Ok, so I think I found yet another wrinkle, from the build_recipients() method:

    if to_field:
        cc_field = recipients.symmetric_difference(to_field)
    else:
        to_field = recipients
        cc_field = set()

So after tracing through the code of build_recipients more deeply (ouch), I think this bug is an edge case, but at our site we seem to hit it a lot as we are often to call out specific users in a review but then also add a group or two for everyone else to follow along (and there is often overlap between the specific users and groups).

So from what I can gather, your second testcase is close to exercising the bug except you need to add a target user as well. From the code above, if to_field becomes empty, recipients will replace it and there will be no cc_field; however, we want the Group object to get sent to the cc_field which will then exercise the bug.

My appologies for not writing a unit test. I felt it would take too long to get an environment setup but in hindsight, it probably would have been simpiler.

brennie
#9 brennie

Thanks for the updated information. I've managed to reproduce and will have this fixed up shortly.

  • -New
    +Confirmed
brennie
#10 brennie

A review request has gone up at https://reviews.reviewboard.org/r/7663

  • -Confirmed
    +PendingReview
brennie
#11 brennie

The patch for this issue has landed on release-2.0.x as 7755109. This will be included in the next Review Board minor release (2.0.21).

  • -PendingReview
    +Fixed