See also bug 10869 and bug 18360. Until now we focused on not deleting order lines in the interface entirely. So it is quite strange that we allow it without any checks on the REST API. In harmony with 18360 we should check if the status is cancelled.
Created attachment 162017 [details] [review] Bug 36066: Only allow cancelled order deletion in API Note: I had trouble with listing orders in API without status, although formally not required according specs. Test plan: Run t/db_dependent/api/v1/acquisitions_orders.t
I think the change makes sense, but I worry a bit if this could break plugins using delete - there doesn't seem to be a way to cancel an order via the API yet?
(In reply to Katrin Fischer from comment #2) > I think the change makes sense, but I worry a bit if this could break > plugins using delete - there doesn't seem to be a way to cancel an order via > the API yet? No reason to worry. We should not allow plugins to delete in the first place (since Koha did not allow that yet). But I am reading the code and guess that it is just added in line with regular CRUD patterns. It is possible to cancel an order via API using ->update (PUT) and sending a modified orderstatus. Copying Kyle and Martin here: Do you have such plugins in your plugin repos?
(In reply to Marcel de Rooy from comment #3) > (In reply to Katrin Fischer from comment #2) > No reason to worry. We should not allow plugins to delete in the first place > (since Koha did not allow that yet). But I am reading the code and guess > that it is just added in line with regular CRUD patterns. It is possible to > cancel an order via API using ->update (PUT) and sending a modified > orderstatus. But you should also update the cancellation date! See also bug 10869 comment109. You can delete a whole basket in the interface, but the interface takes care of cancelling etc. Since the API method ->delete does not do that (yet), the proposed check is still fine in its current form imo.
Created attachment 162276 [details] [review] Bug 36066: Only allow cancelled order deletion in API Note: I had trouble with listing orders in API without status, although formally not required according specs. Test plan: Run t/db_dependent/api/v1/acquisitions_orders.t Signed-off-by: David Nind <david@davidnind.com>
(In reply to David Nind from comment #5) > Signed-off-by: David Nind <david@davidnind.com> Thanks!
Created attachment 162735 [details] [review] Bug 36066: Add tests Signed-off-by: David Nind <david@davidnind.com>
Created attachment 162736 [details] [review] Bug 36066: Only allow cancelled order deletion in API Note: I had trouble with listing orders in API without status, although formally not required according specs. Test plan: Run t/db_dependent/api/v1/acquisitions_orders.t Signed-off-by: David Nind <david@davidnind.com>
Hi :) After moving the tests to their own commit, I was able to run them without the implementation. And it turns out the tests pass :o > $t->delete_ok( "//$unauth_userid:$password@/api/v1/acquisitions/orders/" . $order->ordernumber )->status_is(403); It's likely because of $unauth_userid is used instead of $auth_userid.
Another thing that made it slip unnoticed is the return code being the same for not having the right permission and trying to delete something not in the right state for deletion. This is totally out of this ticket: Isn't there any code to differential these two things? I'm just asking to know if I should open a ticket or if there is really nothing to do about this and just move on. https://en.wikipedia.org/wiki/List_of_HTTP_status_codes#4xx_client_errors - maybe moving permission failure response to 401? nope, 401 is super specific so out of our case: «The response must include a WWW-Authenticate header field containing a challenge applicable to the requested resource» - «403 [...] user not having the necessary permissions for a resource or needing an account of some sort, or attempting a prohibited action (e.g. creating a duplicate record where only one is allowed).» Ok it seems that's how 403 was made, grouping lack of permission and prohibited action by business rules :(
(In reply to Victor Grousset/tuxayo from comment #10) > Another thing that made it slip unnoticed is the return code being the same > for not having the right permission and trying to delete something not in > the right state for deletion. Good catch! > This is totally out of this ticket: Isn't there any code to differential > these two things? I'm just asking to know if I should open a ticket or if > there is really nothing to do about this and just move on. We use 409 (Conflict) for such cases: "Indicates that the request could not be processed because of conflict in the current state of the resource, such as an edit conflict between multiple simultaneous updates." Try ``` cd api/v1/swagger/paths git grep 409 ``` Best regards
Created attachment 162738 [details] [review] Bug 36066: Add tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] As Victor discovered, the test with status new in subtest 'delete' needed the authorised user now. Test plan: Run t/db_dependent/api/v1/acquisitions_orders.t Without the follow-up patch this should FAIL.
Created attachment 162739 [details] [review] Bug 36066: Only allow cancelled order deletion in API Note: I had trouble with listing orders in API without status, although formally not required according specs. Test plan: Run t/db_dependent/api/v1/acquisitions_orders.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com>
(In reply to Victor Grousset/tuxayo from comment #9) > It's likely because of $unauth_userid is used instead of $auth_userid. Thanks for catching that. Fixed now.
(In reply to Victor Grousset/tuxayo from comment #10) > Another thing that made it slip unnoticed is the return code being the same > for not having the right permission and trying to delete something not in > the right state for deletion. Yeah we also use 409 in the REST API. Mostly when trying to add a duplicate. In this case I have my doubts if we should go there now. I am just reading it like You are not authorized to delete a non-cancelled order line.
(In reply to Tomás Cohen Arazi from comment #11) > > We use 409 (Conflict) for such cases: > > "Indicates that the request could not be processed because of conflict in > the current state of the resource, such as an edit conflict between multiple > simultaneous updates." > (In reply to Marcel de Rooy from comment #15) > > Yeah we also use 409 in the REST API. Mostly when trying to add a duplicate. > In this case I have my doubts if we should go there now. I am just reading > it like You are not authorized to delete a non-cancelled order line. This is clearly a case for the 409, as the resource status is conflicting with the requested action.
Created attachment 162754 [details] [review] Bug 36066: (follow-up) Move 403 to 409 when status ne cancelled Test plan: Run t/db_dependent/api/v1/acquisitions_orders.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Tomás Cohen Arazi from comment #16) > This is clearly a case for the 409, as the resource status is conflicting > with the requested action. It wasnt so clear to me. But fixed now.
(In reply to Tomás Cohen Arazi from comment #16) > This is clearly a case for the 409, as the resource status is conflicting > with the requested action. The description of 409 was way to abstract for me to make the connection with our case but ok good, 409 then :D
Created attachment 162790 [details] [review] Bug 36066: Add tests Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> [EDIT] As Victor discovered, the test with status new in subtest 'delete' needed the authorised user now. Test plan: Run t/db_dependent/api/v1/acquisitions_orders.t Without the follow-up patch this should FAIL. Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 162791 [details] [review] Bug 36066: Only allow cancelled order deletion in API Note: I had trouble with listing orders in API without status, although formally not required according specs. Test plan: Run t/db_dependent/api/v1/acquisitions_orders.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Created attachment 162792 [details] [review] Bug 36066: (follow-up) Move 403 to 409 when status ne cancelled Test plan: Run t/db_dependent/api/v1/acquisitions_orders.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl> Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Works, makes sense, QA script happy, code looks good, passing QA :)
Thanks for also fixing POD :)
Pushed for 24.05! Well done everyone, thank you!
Pushed to 23.11.x for 23.11.04
Backported to 23.05.x for upcoming 23.05.10