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/164011 Your team Mailman Coders is subscribed to branch lp:postorius. _______________________________________________ Mailman-coders mailing list [email protected] http://mail.python.org/mailman/listinfo/mailman-coders
