[Merge] lp:~sharmavarun/postorius/postorius_gsoc2013 into lp:postorius
Varun Sharma has proposed merging lp:~sharmavarun/postorius/postorius_gsoc2013 into lp:postorius. Requested reviews: Mailman Coders (mailman-coders) For more details, see: https://code.launchpad.net/~sharmavarun/postorius/postorius_gsoc2013/+merge/... Added delete user functionaly, created user_delete method to postorius/views/user.py and added template user_confirm_delete.html to templates. Right now users can be deleted by superuser only but as discussed on irc, self deletion of user may be added in future. -- https://code.launchpad.net/~sharmavarun/postorius/postorius_gsoc2013/+merge/... Your team Mailman Coders is requested to review the proposed merge of lp:~sharmavarun/postorius/postorius_gsoc2013 into lp:postorius.
Review: Needs Fixing Thanks Varun Sharma for taking on this feature! Deleting users works fine, there are only a couple of minor things: 1) display_name in the success message The display_name is not a mandatory field, so we cannot assume it is set. If it isn't the success message will say: "The user None has been deleted". I suggest to use the email address for the success message as a fallback. 2) Mailman not running (MailmanAPIError) I like that you put the logic inside a try/except block, but there's one important scenario currently missing: It's possible that Postorius running but Mailman is not, in which case the REST API wouldn't respond and no HTTPError would be raised (resulting in an ugly 500 error raised by Django). mailman.client has an Exception class for that (MailmanAPIError) which we use to render a generic "API not available message". See an example in the user_new function. Could you add that as well? 3) Tests The test coverage in Postorius is far from complete, but I'd like to add a test for every new feature. Would you like to take a shot at that? Although I have to acknowledge that testing the Postorius/mailman.client/Mailman combination can be a little tricky, so I can also just do it before I do the merge. 4) This branch It looks like this branch not only contains the user deletion feature, but also changes on the preferences template. Usually we'd like to merge branches feature by feature. So could you add the user deletion changes to a fresh feature branch? Cheers and thank you very much Florian -- https://code.launchpad.net/~sharmavarun/postorius/postorius_gsoc2013/+merge/... Your team Mailman Coders is subscribed to branch lp:postorius.
Thanks Varun Sharma for taking on this feature!
Deleting users works fine, there are only a couple of minor things:
1) display_name in the success message
The display_name is not a mandatory field, so we cannot assume it is set. If it isn't the success message will say: "The user None has been deleted". I suggest to use the email address for the success message as a fallback.
2) Mailman not running (MailmanAPIError)
I like that you put the logic inside a try/except block, but there's one important scenario currently missing: It's possible that Postorius running but Mailman is not, in which case the REST API wouldn't respond and no HTTPError would be raised (resulting in an ugly 500 error raised by Django). mailman.client has an Exception class for that (MailmanAPIError) which we use to render a generic "API not available message". See an example in the user_new function. Could you add that as well?
3) Tests
The test coverage in Postorius is far from complete, but I'd like to add a test for every new feature. Would you like to take a shot at that? Although I have to acknowledge that testing the Postorius/mailman.client/Mailman combination can be a little tricky, so I can also just do it before I do the merge.
4) This branch
It looks like this branch not only contains the user deletion feature, but also changes on the preferences template. Usually we'd like to merge branches feature by feature. So could you add the user deletion changes to a fresh feature branch?
Cheers and thank you very much Florian
Thanks Florian for taking time in reviewing and suggesting the improvements. I have taken all of your suggestions in consideration and done all the suggested changes in my new merge request at https://code.launchpad.net/~varun/postorius/postorius_gsoc2013_modified/+mer... -- https://code.launchpad.net/~varun/postorius/postorius_gsoc2013/+merge/164011 Your team Mailman Coders is subscribed to branch lp:postorius.
The proposal to merge lp:~sharmavarun/postorius/postorius_gsoc2013 into lp:postorius has been updated. Status: Needs review => Work in progress For more details, see: https://code.launchpad.net/~sharmavarun/postorius/postorius_gsoc2013/+merge/... -- https://code.launchpad.net/~sharmavarun/postorius/postorius_gsoc2013/+merge/... Your team Mailman Coders is subscribed to branch lp:postorius.
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(). -- https://code.launchpad.net/~sharmavarun/postorius/postorius_gsoc2013/+merge/... Your team Mailman Coders is subscribed to branch lp:postorius.
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.
participants (3)
-
Florian Fuchs -
Stephen Turnbull -
Varun Sharma