API-4: ordering

[wvengen]

Part four of #429 in chunks, continued from #572:

• /api/v1/orders(/:id)
• /api/v1/order_articles(/:id)
• /api/v1/group_order_articles(/:id) with CRUD
• /api/v1/article_categories(/:id)

Updating single GroupOrderArticles is something (I believe) we don't really do elsewhere. I've looked into this years ago in the foodcoop-adam fork, and used something similar here.

• add docs and specs for endpoints (rebase on updated API-3: financial transactions #572)
• see if update logic can be moved to model
• specs for updating endpoints
• add description to swagger fields
• test group_order_article serializer total_price (first check if Apivore might test that)
• rebase

[wvengen]

@paroga I think this is ready now, care to review?
(note that the CI error is because of a disappeared commit after a force-push, note that the other CI succeeds)

[wvengen]

I'm slowly proposing to merge this, if there are no further objections.

[paroga]

maybe i missed something, but expected some additional tests:

• do api calls return the correct content? (at least check the id and/or a relevant property like name) i only see some tests which validate the content for GET /orders
• do api calls perform any work? does a DELETE really removes the element from the database and not just returns the expected status code?

we have the chance to improve our test coverage with the new api endpoints, so we should try to write tests for as many relevant cases as possible

[wvengen]

• do api calls return the correct content? (at least check the id and/or a relevant property like name) i only see some tests which validate the content for GET /orders

The swagger_spec uses Apivore which checks that all responses are conforming to the Swagger/OpenAPI specification in doc/.

• do api calls perform any work? does a DELETE really removes the element from the database and not just returns the expected status code?

That's a good point, which I have stepped over perhaps a bit too lightly.

we have the chance to improve our test coverage with the new api endpoints, so we should try to write tests for as many relevant cases as possible

I agree that is a good opportunity, will write tests for modifying methods.

[paroga]

The swagger_spec uses Apivore which checks that all responses are conforming to the Swagger/OpenAPI specification in doc/.

does it also also check if the correct items are returned or does it only check for the schema?

[wvengen]

does it also also check if the correct items are returned or does it only check for the schema?

It only checks for the schema, which is an approach we've already adopted.
I think it wouldn't be a bad idea to move to a different testing approach for the API. We'd probably implement controller specs for all the API endpoints, checking status and return values. This would kind of double with the Apivore specs, which would better be moved to become part of the controller specs (if possible, or find another solution to test OpenAPI conformance, or skip that).
I think that, while it would be good to look into, this overhaul is outside the scope of this PR.

Updating data is new here, I'd be happy to include specs for that.

[paroga]

this overhaul is outside the scope of this PR.

agree

Updating data is new here, I'd be happy to include specs for that.

thanks

[paroga]

do we want to add description to all properties?

[wvengen]

Good idea. Except, perhaps id and name (which are self-explanatory).

[paroga]

LGTM (beside the missing newline)
many thanks for the additional tests!