[Merge] lp:~sharmavarun/postorius/postorius_gsoc2013 into lp:postorius

Stephen Turnbull stephen at xemacs.org
Thu May 16 17:59:26 CEST 2013


1. In an earlier revision of user_delete(), you had the code

    user_id = kwargs["user_id"]
    user = MailmanUser.objects.get(address=user_id)

outside the "if".  I don't understand why you moved it inside, and moving the assignment to user_id inside the "try" seems inappropriate since you have to do it again after the "if" if the request isn't a POST.  This is a minor DRY violation.

2. The redirect to "user_index" is also repeated (once in the try, once in the except).  I think this can go outside the try, no?

3. What is going to throw an HTTPError inside the try?  Surely the assignment to user_id won't throw an HTTPError.  So you should restrict the scope of the try to those statements that can actually raise the error you're catching.  That's only user.delete(), right?  I think the same argument applies to catching MailmanAPIError.  However, Florian seems to be OK with it, so it's really a matter of style.  A possible alternative would be to attack the DRY violation at the root, by writing a decorator that catches those two errors.  This would be useful because the same pattern appears in other commands like user_new().
Your team Mailman Coders is subscribed to branch lp:postorius.

More information about the Mailman-coders mailing list