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

Varun Sharma varunsharmalive at gmail.com
Fri May 17 20:26:22 CEST 2013


> 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.


More information about the Mailman-coders mailing list