GNU Mailman

Code review comment for lp:~jimmy-sigint/mailman/rest_api_ban_management

rest_api_ban_management
Merge into 3.0

Revision history for this message

Jimmy Bergman (jimmy-sigint) wrote on 2012-09-24:

> Note that there is an IBanManager utility which you can use to add and remove
> list-specific or global bans. Probably better to use this utility in the REST
> API rather than adding a new interface to IMailingList.

You must have misread the patch, I actually use the IBanManager. I extended
it to be able to return a list of banned emails for a list, since
the semantics wouldn't be RESTy if i couldn't list.

> Also (perhaps because you didn't know about the IBanManager), your branch
> doesn't provide a way to set or delete global bans.

It is more the fact that I created the first version of the patch before
the IBanManager came to life, because our control panel software mailman 3
integration has this view that displays per list bans. I could
add global bans as well since it makes sense.

> What do you think about a resource tree that looks something like this:
>
> GET http://.../bans -> a list of all global bans
> DELETE http://.../bans -> deletes all global bans

Is this really required? I guess it might be nice of course, but
it feels like an uncommon operation (and one that can be done
using the other DELETE although less efficiently).

> POST <email address hidden> http://.../bans -> adds <email address hidden> to the global
> bans
> DELETE http://.../<email address hidden> -> deletes <email address hidden> from the
> global bans

> Then for list-specific bans, we would use
>
> GET http://.../lists/list.example.com/bans -> a list of all list-specific bans
> DELETE http://.../lists/list.example.com/bans -> delete all global bans
> POST <email address hidden> http://.../lists/list.example.com/bans -> adds
> <email address hidden> to <email address hidden>'s bans
> DELETE http://.../<email address hidden> -> deletes
> <email address hidden> from <email address hidden>'s bans

Sounds great (although with the same question re DELETE of all bans) although
in the current patch I also have
GET http://.../<email address hidden> -> check if <email address hidden> is banned on list.example.com

which I think should stay (also i named it ../banlist, but /bans sounds better).

> Would you like to take a shot at making these changes? If so, please push an
> update and respond to this comment and I'd be happy to take another look. I'd
> like to get this into 3.0b3.

Sure, I can do the above change once we agree on:
1. Should there be a DELETE for removing all bans, or is it enough with DELETE of a single ban + iterating if you need to clear the ban list?

2. Should I keep my extra GET on the banned email sub-resource (obviously a similar one would also be implemented for the global bans)?

And what is the timeframe for 3.0b3 so that I know when I need to push it?

Best regards,
Jimmy

> Note that there is an IBanManager utility which you can use to add and remove
> list-specific or global bans.  Probably better to use this utility in the REST
> API rather than adding a new interface to IMailingList.

You must have misread the patch, I actually use the IBanManager. I extended
it to be able to return a list of banned emails for a list, since
the semantics wouldn't be RESTy if i couldn't list.

> Also (perhaps because you didn't know about the IBanManager), your branch
> doesn't provide a way to set or delete global bans.

> What do you think about a resource tree that looks something like this:
> 
> GET http://.../bans -> a list of all global bans
> DELETE http://.../bans -> deletes all global bans

Is this really required? I guess it might be nice of course, but
it feels like an uncommon operation (and one that can be done
using the other DELETE although less efficiently).

> POST foo@example.com http://.../bans -> adds foo@example.com to the global
> bans
> DELETE http://.../bans/foo@example.com -> deletes foo@example.com from the
> global bans

> Then for list-specific bans, we would use
> 
> GET http://.../lists/list.example.com/bans -> a list of all list-specific bans
> DELETE http://.../lists/list.example.com/bans -> delete all global bans
> POST foo@example.com http://.../lists/list.example.com/bans -> adds
> foo@example.com to list@example.com's bans
> DELETE http://.../lists/list.example.com/bans/foo@example.com -> deletes
> foo@example.com from list@example.com's bans

Sounds great (although with the same question re DELETE of all bans) although
in the current patch I also have
GET http://.../lists/list.example.com/bans/foo@example.com -> check if foo@example.com is banned on list.example.com

which I think should stay (also i named it ../banlist, but /bans sounds better).

> Would you like to take a shot at making these changes?  If so, please push an
> update and respond to this comment and I'd be happy to take another look.  I'd
> like to get this into 3.0b3.

Sure, I can do the above change once we agree on:
1. Should there be a DELETE for removing all bans, or is it enough with DELETE of a single ban + iterating if you need to clear the ban list?

2. Should I keep my extra GET on the banned email sub-resource (obviously a similar one would also be implemented for the global bans)?

And what is the timeframe for 3.0b3 so that I know when I need to push it?

Best regards,
Jimmy

« Back to merge proposal