Merge lp:~nkarageuzian/mailman/usermanagement into lp:mailman

[nicolask]

solves the use case:
user sends a mail to the list without subscribing nor registering
admin accepts message
user wants to register to mailman through API but
create_user method fails as sender's address is registered.

and add tests for usermanager and the create_user function.

[Barry Warsaw (barry)]

nicolask - thanks very much for the contribution to Mailman! I'm afraid I'm going to have to reject this merge proposal though because I think this issue needs to be solved at a different level. The model tries to be as simple as possible, and because the IUser object is a fundamental object, the IUserManager.create_user() method should not implement these additional semantics. On the mailing list, I outlined an approach that I think would be better.

In the hopes that you'll continue to contribute to Mailman, I'd like to offer some additional style feedback on your diff. Again, thanks for working on Mailman!

=== added file 'src/mailman/model/tests/test_usermanager.py'
--- src/mailman/model/tests/test_usermanager.py 1970-01-01 00:00:00 +0000
+++ src/mailman/model/tests/test_usermanager.py 2013-12-28 10:18:08 +0000
> +class TestUserManager(unittest.TestCase):
> + """Test usermanager."""
> +
> + layer = ConfigLayer
> +
> + def setUp(self):
> + self._mlist = create_list('<email address hidden>')
> + #getUtility(IAddress)('<email address hidden>','No one')

Of course, you can get rid of that commented out line, but also, since your
tests all use the IUserManager utility, it might be better to do something
like this in the setUp():

        self._user_manager = getUtility(IUserManager)

and then use self._user_manager instead of having to get the utility every
time.

> +
> + def test_create_user(self):
> + anne = getUtility(IUserManager).create_user(
> + '<email address hidden>', 'Anne Person')
> +
> + def test_create_user_existing_address_user(self):
> + anne = getUtility(IUserManager).create_user(
> + '<email address hidden>', 'Anne Person')
> + self.assertRaises(ExistingAddressError, getUtility(IUserManager).create_user,*(
> + '<email address hidden>', 'Anne Person'))

You'll want to make sure this line is less than 80 characters wide. Also, you
should need the * there. I think the following would work better:

        self.assertRaises(ExistingAddressError,
                          self._user_manager.create_user,
                          '<email address hidden>', 'Anne Person')

> +
> + def test_create_user_existing_address_no_user(self):
> + anne = getUtility(IUserManager).create_user(
> + '<email address hidden>', 'Anne Person')
> + noone = getUtility(IUserManager).create_user(
> + '<email address hidden>','No one')
> + self.assertTrue(anne.user_id != noone.user_id)

It would be better to use self.assertNotEqual() here, since the error message
is better should the test ever fail. (Also, remember, a single space comes
after commas in an argument list.)

https://docs.python.org/2.7/library/unittest.html?highlight=unittest#assert-methods

=== modified file 'src/mailman/model/usermanager.py'
--- src/mailman/model/usermanager.py 2013-01-01 14:05:42 +0000
+++ src/mailman/model/usermanager.py 2013-12-28 10:18:08 +0000
> @@ -40,13 +40,24 @@
> @implementer(IUserManager)
> class UserManager:
> """See `IUserManager`."""
> -
> - def create_user(self, email=None, display_name=None):
> + @dbconnection
> + def create_user(s...

[nicolask (nkarageuzian)]

barry,

No problems and many thanks for the time you spent for writing style hints.

I've read #1312884 and plan to contribute on it as I agree with the aproach proposed.
I guess I could get one more hint (file ref) to dive quickly in test writings for that.

[Barry Warsaw (barry)]

Writing tests for the REST API is actually pretty easy. In general, the
doctests in docs/ should be used to document how to use the API for the common
cases, and it should only document good-path uses (i.e. not error or corner
conditions). Use unittests for bad-path, error condition, or corner cases.

I think good examples of these are the user tests.

Here's the unittests: http://tinyurl.com/mldysb2

And the doctests: http://tinyurl.com/mqtacm9

Hope that helps and don't worry; I'll do a thorough review and help you get
your contribution landed. Thanks again!

Unmerged revisions

7232. By nicolask on 2013-12-28

improve usermanagement : allow create_user method even if an address is known (mail sent to the list without subscribing).

=== added file 'src/mailman/model/tests/test_usermanager.py'
--- src/mailman/model/tests/test_usermanager.py	1970-01-01 00:00:00 +0000
+++ src/mailman/model/tests/test_usermanager.py	2013-12-28 10:18:08 +0000
@@ -0,0 +1,67 @@
+# Copyright (C) 2011-2013 by the Free Software Foundation, Inc.
+#
+# This file is part of GNU Mailman.
+#
+# GNU Mailman is free software: you can redistribute it and/or modify it under
+# the terms of the GNU General Public License as published by the Free
+# Software Foundation, either version 3 of the License, or (at your option)
+# any later version.
+#
+# GNU Mailman is distributed in the hope that it will be useful, but WITHOUT
+# ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+# FITNESS FOR A PARTICULAR PURPOSE.  See the GNU General Public License for
+# more details.
+#
+# You should have received a copy of the GNU General Public License along with
+# GNU Mailman.  If not, see <http://www.gnu.org/licenses/>.
+
+"""Test usermanager."""
+
+from __future__ import absolute_import, unicode_literals
+
+__metaclass__ = type
+__all__ = [
+    'TestUserManager',
+    ]
+
+
+import unittest
+
+from zope.component import getUtility
+
+from mailman.app.lifecycle import create_list
+from mailman.interfaces.usermanager import IUserManager
+from mailman.interfaces.address import ExistingAddressError
+from mailman.interfaces.address import IAddress
+from mailman.testing.layers import ConfigLayer
+from mailman.utilities.datetime import now
+
+
+

+class TestUserManager(unittest.TestCase):
+    """Test usermanager."""
+
+    layer = ConfigLayer
+
+    def setUp(self):
+        self._mlist = create_list('[e-mail]')
+        #getUtility(IAddress)('[e-mail]','No one')
+    
+    def test_create_user(self):
+        anne = getUtility(IUserManager).create_user(
+            '[e-mail]', 'Anne Person')
+        
+    def test_create_user_existing_address_user(self):
+        anne = getUtility(IUserManager).create_user(
+            '[e-mail]', 'Anne Person')
+        self.assertRaises(ExistingAddressError, getUtility(IUserManager).create_user,*(
+            '[e-mail]', 'Anne Person'))
+            
+    def test_create_user_existing_address_no_user(self):
+        anne = getUtility(IUserManager).create_user(
+            '[e-mail]', 'Anne Person')
+        noone = getUtility(IUserManager).create_user(
+            '[e-mail]','No one')
+        self.assertTrue(anne.user_id != noone.user_id)
+    
+

=== modified file 'src/mailman/model/usermanager.py'
--- src/mailman/model/usermanager.py	2013-01-01 14:05:42 +0000
+++ src/mailman/model/usermanager.py	2013-12-28 10:18:08 +0000
@@ -40,13 +40,24 @@
 @implementer(IUserManager)
 class UserManager:
     """See `IUserManager`."""
-
-    def create_user(self, email=None, display_name=None):
+    @dbconnection
+    def create_user(self, store, email=None, display_name=None):
         """See `IUserManager`."""
         user = User(display_name, Preferences())
         if email:
-            address = self.create_address(email, display_name)
+            address = None
+            try:
+                address = self.create_address(email, display_name)
+            except:
+                addresses = store.find(Address, email=email.lower())
+                if addresses.count() == 0:
+                    raise Exception("inconsistent results")
+                if addresses.one().user == None:
+                    address = addresses.one()
+                else:
+                    raise ExistingAddressError(email)
             user.link(address)
+        store.add(user)
         return user
 
     @dbconnection