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

Barry Warsaw (barry) wrote on 2012-10-13:

On Sep 24, 2012, at 06:00 AM, Jimmy Bergman wrote:

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

You're right, sorry about that.

BTW, I'm probably going to change the way IBanManager works, to be a bit more
like IAcceptableAliasSet. This would mean you'd adapt an IMailingList to an
IBanManager, and then we could drop the mailing_list parameters to the various
calls. The one difference is that we'd allow you to adapt None to IBanManager
to get the global bans.

Don't worry about all that though. Once your branch is ready, I'll make those
changes as I merge in your work.

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

Cool.

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

True. I'm also not really happy with DELETE on .../bans because it doesn't
actually delete the resource. Okay, so let's forget DELETE on all. Do we
need GET on all?

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

I would definitely want it to be symmetrical. So if we want to have DELETE
all global bans, we'd want the same for list bans, and if not, then neither
ban list would support DELETE. Same with GET.

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

Agreed, let's keep GET on a specific address. I suppose it would 404 if the
given address had no ban, and probably 204 if there is a ban. It would work
the same for global ban queries.

Now the question is, is it okay to do two queries to see if an address has
either a list-specific ban or a global ban? That certainly would be the
easiest to implement (at the cost of some efficiency, which we already are
leaning toward not caring about for DELETE).

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

Let's support DELETE on a single ban only.

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

You have enough time. Consensus seems to be to delay the release of the core
to allow for Postorius to get closer to complete. I'm okay with that, with a
more liberal view of what's a bug in the core. :)

Go for it!

On Sep 24, 2012, at 06:00 AM, Jimmy Bergman wrote:

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

You're right, sorry about that.

BTW, I'm probably going to change the way IBanManager works, to be a bit more
like IAcceptableAliasSet.  This would mean you'd adapt an IMailingList to an
IBanManager, and then we could drop the mailing_list parameters to the various
calls.  The one difference is that we'd allow you to adapt None to IBanManager
to get the global bans.

Don't worry about all that though.  Once your branch is ready, I'll make those
changes as I merge in your work.

Cool.

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

True.  I'm also not really happy with DELETE on .../bans because it doesn't
actually delete the resource.  Okay, so let's forget DELETE on all.  Do we
need GET on all?

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

I would definitely want it to be symmetrical.  So if we want to have DELETE
all global bans, we'd want the same for list bans, and if not, then neither
ban list would support DELETE.  Same with GET.

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

Agreed, let's keep GET on a specific address.  I suppose it would 404 if the
given address had no ban, and probably 204 if there is a ban.  It would work
the same for global ban queries.

Now the question is, is it okay to do two queries to see if an address has
either a list-specific ban or a global ban?  That certainly would be the
easiest to implement (at the cost of some efficiency, which we already are
leaning toward not caring about for DELETE).

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

Let's support DELETE on a single ban only.

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

You have enough time.  Consensus seems to be to delay the release of the core
to allow for Postorius to get closer to complete.  I'm okay with that, with a
more liberal view of what's a bug in the core. :)

Go for it!

« Back to merge proposal