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

Revision history for this message
Barry Warsaw (barry) wrote :

Nice branch, thanks Jimmy. A few comments.

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.

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

I don't particularly like DELETE on the /bans resource to delete all the sub-resources, since the implication is that this will remove /bans, not everything underneath it. But it's the best I can come up with for now. Obviously DELETE on /bans doesn't delete the /bans resource.

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.

Thanks!

review: Needs Fixing

« Back to merge proposal