Code review comment for lp:~nkarageuzian/mailman/usermanagement

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(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:

You almost never want to use a bare except. Mailman uses them in just a few
very limited places, and it's almost always to perform some task and then
re-raise the exception. Examples are: log the error and then re-raise, or
abort the transaction and re-raise. In all other cases, you should catch the
appropriate exception explicitly.

> + addresses = store.find(Address, email=email.lower())
> + if addresses.count() == 0:
> + raise Exception("inconsistent results")

Generally, you don't want to raise the base Exception, but instead some more
appropriate specific exception. However, in this particular case, the
IUserManager already implements this logic in its get_address() method, so it
should just call that.

Then you can just add an assertion that the address exists, i.e. is not None.
That will be logically consistent if you were to catch ExistingAddressError
above. E.g.

            address = self.get_address(email)
            assert address is not None

> + if addresses.one().user == None:

As a general style consideration, we never compare against None using
equality; always use identity to compare against singletons like None. While
the above comments would make most of this code go away, the test should be
rewritten like so:

            if address.one().user is None

> + address = addresses.one()
> + else:
> + raise ExistingAddressError(email)
> user.link(address)
> + store.add(user)
> return user
>
> @dbconnection