To allow the work in bug 24608 to proceed, we need an API route to allow the setting of a checkout's due date. I will add a GET route also. The proposed endpoints are: Return a checkout's due date: Method: GET URL: /checkouts/{checkout_id}/date_due Set a checkout's due date: Method: POST URL: /checkouts/{checkout_id}/date_due Body: { date_due: '2020-02-07 10:49:08' }
Change of plan on this one. It's unnecessary to create a new endpoint for this. Following a discussion with Martin, I've realised we can just add a PUT endpoint for the /checkouts/{checkout_id} route, with whole checkout objects passed as the body.
Created attachment 98924 [details] [review] Bug 24609: Create PUT /checkouts/{checkout_id} This patch creates a PUT /checkouts/{checkout_id} API route enabling checkouts to be updated
Created attachment 98925 [details] [review] Bug 24609: Add API unit tests
Test plan: - Apply patch - prove t/db_dependent/api/v1/checkouts.t
Created attachment 99021 [details] [review] Bug 24609: Create PUT /checkouts/{checkout_id} This patch creates a PUT /checkouts/{checkout_id} API route enabling checkouts to be updated Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Created attachment 99022 [details] [review] Bug 24609: Add API unit tests Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
There is something that annoy me with adding such routes. We have lot of switches in Koha (syspref, permissions) that are not taken into account with them. Do we really want to allow anybody with circulate_remaining_permissions to modify a checkout, and so bypassing sysprefs and circulation rules? That sounds a bit wrong to me.
Thanks for this Jonathan. I guess that then raises the question of whether that logic should go in the route controller? Or whether there should be a centralised location (probably the route spec), which would enable the creator to specify all the conditions that must be met before the route can be accessed. I guess extending what x-koha-authorization does. Having it in the controller feels haphazard and prone to omission, having it a required part of the route spec would be better. But how you would go about defining a way for any arbitrary logic to be specified in a json file would need some thought. Anyway, it feels like this is a question that's bigger than just the addition of this route, do you agree?
(In reply to Andrew Isherwood from comment #8) > Anyway, it feels like this is a question that's bigger than just the > addition of this route, do you agree? It's definitely bigger than this route, do we have already such routes that could be problematic? I have asked Tomas to let a comment with his thoughts.
Hi I'm sorry for the delay to comment on this one. As a general comment, our (current) controller/permissions logic doesn't allow to specify complex conditions. We only check for permissions/ownership, before we reach the controller. [1] So the general rule is that any syspref check or combination of permissions+sysprefs (that can be found in many places in our codebase/templates) should be done either in a high level method (ModIssue?) or the controller itself (e.g. that's why we stash the current user, accesible through $c->stash('koha.user') so we can ask further questions about it. Any authentication method stashes the user for that purpose. That said, changing the date due on a checkout might be something worth having a method in Koha::Checkout->update_due_date so any logic is encapsulated there and, more importantly, the method can be used in other places. Speaking of the current endpoint, we've been observant of the current implementations on the UI to catch all the conditions and prevent misuses of endpoints. This doesn't mean there were no mistakes, but speaking for my contributions, I usually look at the .pl, the .tt and try to put all that logic on the enpdoint. [1] And I'm not sure we should add such a thing to our project as the API hasn't been a target for the refactorings specifically, and so moving existing logic into a (cool) new high level config layer on the spec would mean maintaining two codebases right now.
The code for update() is missing a ->discard_changes() call before returning $checkout->to_api, so the timestamp does not look updated on the response, Regarding the logic in it, I'm worried about the simplicity and how this might need some round actions to trigger things like account fixes, etc.I'm not saying it is wrong, but it looks suspiciously simple without having dug too much into the area recently (i.e. I would accept an 'is safe' from you, if you say so).
(In reply to Tomás Cohen Arazi from comment #11) > The code for update() is missing a ->discard_changes() call before returning > $checkout->to_api, so the timestamp does not look updated on the response, > > Regarding the logic in it, I'm worried about the simplicity and how this > might need some round actions to trigger things like account fixes, etc.I'm > not saying it is wrong, but it looks suspiciously simple without having dug > too much into the area recently (i.e. I would accept an 'is safe' from you, > if you say so). By 'round' actions, do you mean things like triggering an updateFines call ?
OK, I now see what you mean a bit more.. we don't, for example, handle updating the 'items.onloan' field to match the new due date either. I think some of the logic for these 'side effects' ought to be moved into Koha::Checkout::store.. but it'll need a little digging.
Created attachment 115879 [details] [review] Bug 24609: Create PUT /checkouts/{checkout_id} This patch creates a PUT /checkouts/{checkout_id} API route enabling checkouts to be updated Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Created attachment 115880 [details] [review] Bug 24609: Add API unit tests Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Created attachment 115881 [details] [review] Bug 24609: Add test for timezone handling This patch adds an additional set of tests for test for how timezones are handled if we are passed an rfc3339 string with a timezone offset.
I've added a test to highlight the issues faced downstream in bug 24608. We must be able to handle RFC3339 input dates that include a timezone offset... the test will fail prior to applying bug 24850 so I've made it a dependancy.
Created attachment 126191 [details] [review] Bug 24609: Create PUT /checkouts/{checkout_id} This patch creates a PUT /checkouts/{checkout_id} API route enabling checkouts to be updated Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Created attachment 126192 [details] [review] Bug 24609: Add API unit tests Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Created attachment 126193 [details] [review] Bug 24609: Add test for timezone handling This patch adds an additional set of tests for test for how timezones are handled if we are passed an rfc3339 string with a timezone offset.
Created attachment 126194 [details] [review] Bug 24609: Add trigger to set items.onloan on due_date update This patch adds an overridden store method to Koha::Checkout to catch due_date changes and trigger an update of the items.onloan field.
Created attachment 126316 [details] [review] Bug 24609: Create PUT /checkouts/{checkout_id} This patch creates a PUT /checkouts/{checkout_id} API route enabling checkouts to be updated Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 126317 [details] [review] Bug 24609: Add API unit tests Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 126318 [details] [review] Bug 24609: Add test for timezone handling This patch adds an additional set of tests for test for how timezones are handled if we are passed an rfc3339 string with a timezone offset. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 126319 [details] [review] Bug 24609: Add trigger to set items.onloan on due_date update This patch adds an overridden store method to Koha::Checkout to catch due_date changes and trigger an update of the items.onloan field. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 126320 [details] [review] Bug 24609: (QA follow-up) Cleaner handling of unhandled exceptions This patch updates how we handle unhandled exceptions nowadays in Koha controllers. It also adds a missing call to ->discard_changes for the returned object. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 126321 [details] [review] Bug 24609: (QA follow-up) Remove diag from tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Please provide more tests.
Created attachment 126917 [details] [review] Bug 24609: Unit tests for Koha::Checkout->store This patch adds unit tests for the Koha::Item onloan trigger inside Koha::Checkout->store(); Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126921 [details] [review] Bug 24609: Create PUT /checkouts/{checkout_id} This patch creates a PUT /checkouts/{checkout_id} API route enabling checkouts to be updated Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 126922 [details] [review] Bug 24609: Add API unit tests Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 126923 [details] [review] Bug 24609: Add test for timezone handling This patch adds an additional set of tests for test for how timezones are handled if we are passed an rfc3339 string with a timezone offset. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 126924 [details] [review] Bug 24609: Add trigger to set items.onloan on due_date update This patch adds an overridden store method to Koha::Checkout to catch due_date changes and trigger an update of the items.onloan field. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 126925 [details] [review] Bug 24609: (QA follow-up) Cleaner handling of unhandled exceptions This patch updates how we handle unhandled exceptions nowadays in Koha controllers. It also adds a missing call to ->discard_changes for the returned object. Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 126926 [details] [review] Bug 24609: (QA follow-up) Remove diag from tests Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 126927 [details] [review] Bug 24609: Unit tests for Koha::Checkout->store This patch adds unit tests for the Koha::Item onloan trigger inside Koha::Checkout->store(); Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 126928 [details] [review] Bug 24609: WIP on api tests
Rebased and added tests for the trigger introduced in Koha::Checkout->store. I've started adding some further tests to the API endpoint but came somewhat unstuck when trying to think about how readOnly and partial update requirements interact.. posted up my final work in progress patch but this is where I need a bit of input from Tomas regards expected behaviour for these cases.
Hi Martin, do you have plans to continue here? Afraid it doesn't apply again :(
It's still on my list.. but I have other priorities at the moment..