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?
- User A should create a review request and assing a group that User B is a member of
- User B should update their profile to not send own updates
- 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.
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
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.
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'...
-
+
Re: #3,
If the user has
should_send_own_email = False
, then it will be discarded from bothto_field
andrecipients
and therefore cannot make it into thecc_field
. See lines 379--385.Can you attach a unit test that reproduces this behaviour?
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.
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.
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.
Thanks for the updated information. I've managed to reproduce and will have this fixed up shortly.
-
- New + Confirmed