> 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/+merge/164480 -- https://code.launchpad.net/~varun/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
