I recommend a docblock here describing the test condition and expected results.

It seems like it could be good to make an assertion about the result_id here as well.

Shouldn't the above two lines be all instead of any?

Since this is JSON, why not use the JSON list instead of a comma separated list? That way it would be easier to encode and decode on both ends.

LGTM, though I do highly recommend just using a JSON list since the body is already JSON. It'll be much more natural to the users, and it'll also be easier on the server side too.

Yeah, you are right.

Yeah.

I thought the test is quite self explanatory, but I have no problem adding a docblock.

Thanks Randy, I will update the PR with your feedback.

Rebased to address Randy's feedback.

The other changes look good, but I still recommend making this a JSON list instead of a comma separated string. It'll make this code simpler, and it'll make it easier for your users too.

LGTM, though I still recommend using a JSON list instead of a comma-separated string.

Yeah, he's got a point @mjia. Odds are, people will expect this to be JSON and be confused when that doesn't work.

As below, can we do 'result_id': json.dumps(result_ids)?

Also - can we change result_id to be plural here for natural readability?

i.e. 'result_ids': json.dumps(result_ids)

Same here - can you change it to a JSON list (and change result_id to plural result_ids). Other than that, :+1: from me -- and we can get on to making greenwave use this POST interface for greenwave#83.

Aha, I forgot to change result_ids to JSON list. Okay, I've updated the patch with the feedback. Thanks guys.

I think this should be (assuming the I stands for international):
"A ISO" => "An ISO"

I'd prefer using a separate parameter for the end date such as until. That way you create a range with using both since and until. This also gives you more flexibility in your queries.

What's the thought process of using the plus sign in the route? I've just never seen that before.

Correct.

Yeah. This just copied from ResultsDB to make it consistent(maybe not necessarily needed?). We can change it as needed in another PR since we also need to update GET /waivers/

As I recall, it is derived from Beaker used for indicating that this route is a special API. @dcallagh, am I right?

Okay, I've fixed the doc in Commit a07b6ab.

LGTM. :)

@mprahl the plus sign came from.... somewhere. I'm not sure. :-) But it's definitely a convention I've seen around, I didn't totally make it up I swear!

It's because /waivers/ is a collection, so normally /waivers/ would be an element within the collection. But /waivers/+ is a special case, for a URL that isn't an element in the collection. I guess the idea is that the + character does not normally appear in any kind of identifier.

(We obviously couldn't make this endpoint just POST /waivers/ because that already means: create a new waiver.)