Merge lp:~black-perl/mailman/fix-mailman into lp:mailman

Proposed by Ankush Sharma on 2015-03-04
Status: Merged
Approved by: Barry Warsaw on 2015-03-08
Approved revision: 7302
Merged at revision: 7304
Proposed branch: lp:~black-perl/mailman/fix-mailman
Merge into: lp:mailman
Diff against target: 32 lines (+10/-1)
2 files modified
src/mailman/app/ (+1/-1)
src/mailman/rest/tests/ (+9/-0)
To merge this branch: bzr merge lp:~black-perl/mailman/fix-mailman
Reviewer Review Type Date Requested Status
Barry Warsaw 2015-03-04 Needs Fixing on 2015-03-08
Review via email:

Description of the change

Discussed here about the bug and its fix.
Added test to call mailman REST api using case sensitive email address in 'test_try_to_join_a_list_twice' method.

To post a comment you must log in.
Barry Warsaw (barry) wrote :

I think you've done a great job in finding the root cause. I have just a couple of inlined comments, but I am happy to fix those myself when I merge to trunk.

review: Approve
Barry Warsaw (barry) :
Barry Warsaw (barry) wrote :

Ah, the fix isn't right after all. The problem is that the call to mlist.subscribe() after breaking out of the loop can subscribe the original lower case address, but it should always subscribe the case preserved address. I have to think about this.

review: Needs Fixing
Barry Warsaw (barry) wrote :

What this means is that people cannot subscribe two email addresses to a mailing list that differ only in case. In some sense this conflicts with the model, since the case-different email addresses are considered different address objects, but I still think given the stated policy of case-preserving case-insensitive addresses, this will be acceptable.

It will still probably be possible to link two addresses to the user which differ only in case, but since these can't both be subscribed to the same mailing list (with the same role), I suppose we'll have to accept this oddity.

It's also possible to subscribe a user with a preferred address that differs in case with another address object linked to the user. This is consistent with the existing semantics.

Barry Warsaw (barry) wrote :

I'm not sure how much of your original branch survived, but I gave you credit for finding the bug. Thanks! You might take a look at r7304 for details. This was a tricky and fun one.

Ankush Sharma (black-perl) wrote :

Thank you Barry for your time. I am glad you appreciated the bug find. I will surely take a look.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'src/mailman/app/'
2--- src/mailman/app/ 2015-01-05 01:22:39 +0000
3+++ src/mailman/app/ 2015-03-04 09:46:29 +0000
4@@ -98,7 +98,7 @@
5 else:
6 # The user exists and is linked to the address.
7 for address in user.addresses:
8- if == email:
9+ if == email.lower():
10 break
11 else:
12 raise AssertionError(
14=== modified file 'src/mailman/rest/tests/'
15--- src/mailman/rest/tests/ 2015-01-05 01:40:47 +0000
16+++ src/mailman/rest/tests/ 2015-03-04 09:46:29 +0000
17@@ -98,6 +98,15 @@
18 self.assertEqual(cm.exception.code, 409)
19 self.assertEqual(cm.exception.reason, b'Member already subscribed')
21+ # Member subscription using case sensitive email
22+ with self.assertRaises(HTTPError) as cm:
23+ call_api('http://localhost:9001/3.0/members', {
24+ 'list_id': '',
25+ 'subscriber': '',
26+ })
27+ self.assertEqual(cm.exception.code, 409)
28+ self.assertEqual(cm.exception.reason, b'Member already subscribed')
30 def test_join_with_invalid_delivery_mode(self):
31 with self.assertRaises(HTTPError) as cm:
32 call_api('http://localhost:9001/3.0/members', {