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

Varun Sharma varunsharmalive at gmail.com
Fri May 17 21:05:27 CEST 2013


> Varun,
> 
> 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().

Hi Steve,
Thanks for the review. As per your suggestion regarding catching of MailmanAPIError, i have reduced its uncertainty by adding it only to api calls. Also as we discussed on irc, the "if" block is necessary to handle the user deletion request and the code outside the "if" request renders the confirmation template. Both are independent of each other. I moved 
>     user_id = kwargs["user_id"]
>     user = MailmanUser.objects.get(address=user_id)
under the "if" block because the creating of mailman user instance is not required in the case of rendering of template. Also as per your suggestions, i have removed the return redirect statement form try and catch statement which was violating DRY principles and placed a common return statement. My new branch with all the changes is at https://code.launchpad.net/~varun/postorius/postorius_gsoc2013_modified
-- 
https://code.launchpad.net/~varun/postorius/postorius_gsoc2013/+merge/164011
Your team Mailman Coders is subscribed to branch lp:postorius.


More information about the Mailman-coders mailing list