Bug 24609 - Add routes for updating checkouts
Summary: Add routes for updating checkouts
Status: Passed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Andrew Isherwood
QA Contact: Tomás Cohen Arazi
URL:
Keywords:
Depends on: 24850
Blocks: 24608
  Show dependency treegraph
 
Reported: 2020-02-07 10:50 UTC by Andrew Isherwood
Modified: 2021-10-15 11:51 UTC (History)
4 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 24609: Create PUT /checkouts/{checkout_id} (5.53 KB, patch)
2020-02-14 10:51 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24609: Add API unit tests (1.45 KB, patch)
2020-02-14 10:51 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 24609: Create PUT /checkouts/{checkout_id} (5.58 KB, patch)
2020-02-14 19:33 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 24609: Add API unit tests (1.50 KB, patch)
2020-02-14 19:34 UTC, Lisette Scheer
Details | Diff | Splinter Review
Bug 24609: Create PUT /checkouts/{checkout_id} (5.64 KB, patch)
2021-01-27 12:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24609: Add API unit tests (1.50 KB, patch)
2021-01-27 12:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24609: Add test for timezone handling (2.67 KB, patch)
2021-01-27 12:19 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24609: Create PUT /checkouts/{checkout_id} (5.64 KB, patch)
2021-10-13 13:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24609: Add API unit tests (1.50 KB, patch)
2021-10-13 13:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24609: Add test for timezone handling (2.66 KB, patch)
2021-10-13 13:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24609: Add trigger to set items.onloan on due_date update (1.81 KB, patch)
2021-10-13 13:04 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24609: Create PUT /checkouts/{checkout_id} (5.69 KB, patch)
2021-10-15 11:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24609: Add API unit tests (1.56 KB, patch)
2021-10-15 11:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24609: Add test for timezone handling (2.72 KB, patch)
2021-10-15 11:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24609: Add trigger to set items.onloan on due_date update (1.87 KB, patch)
2021-10-15 11:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24609: (QA follow-up) Cleaner handling of unhandled exceptions (1.42 KB, patch)
2021-10-15 11:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24609: (QA follow-up) Remove diag from tests (1.11 KB, patch)
2021-10-15 11:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Isherwood 2020-02-07 10:50:00 UTC
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' }
Comment 1 Andrew Isherwood 2020-02-07 13:46:45 UTC
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.
Comment 2 Andrew Isherwood 2020-02-14 10:51:35 UTC
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
Comment 3 Andrew Isherwood 2020-02-14 10:51:38 UTC
Created attachment 98925 [details] [review]
Bug 24609: Add API unit tests
Comment 4 Andrew Isherwood 2020-02-14 10:53:06 UTC
Test plan:

- Apply patch
- prove t/db_dependent/api/v1/checkouts.t
Comment 5 Lisette Scheer 2020-02-14 19:33:58 UTC
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>
Comment 6 Lisette Scheer 2020-02-14 19:34:12 UTC
Created attachment 99022 [details] [review]
Bug 24609: Add API unit tests

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 7 Jonathan Druart 2020-03-04 13:15:20 UTC
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.
Comment 8 Andrew Isherwood 2020-03-05 10:37:23 UTC
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?
Comment 9 Jonathan Druart 2020-03-16 14:20:58 UTC
(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.
Comment 10 Tomás Cohen Arazi 2020-03-16 17:33:59 UTC
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.
Comment 11 Tomás Cohen Arazi 2020-03-16 18:44:45 UTC
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).
Comment 12 Martin Renvoize 2021-01-27 09:27:18 UTC
(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 ?
Comment 13 Martin Renvoize 2021-01-27 10:44:21 UTC
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.
Comment 14 Martin Renvoize 2021-01-27 12:19:41 UTC
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>
Comment 15 Martin Renvoize 2021-01-27 12:19:44 UTC
Created attachment 115880 [details] [review]
Bug 24609: Add API unit tests

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 16 Martin Renvoize 2021-01-27 12:19:47 UTC
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.
Comment 17 Martin Renvoize 2021-01-27 12:23:11 UTC
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.
Comment 18 Martin Renvoize 2021-10-13 13:04:32 UTC
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>
Comment 19 Martin Renvoize 2021-10-13 13:04:36 UTC
Created attachment 126192 [details] [review]
Bug 24609: Add API unit tests

Signed-off-by: Lisette Scheer <lisettes@latahlibrary.org>
Comment 20 Martin Renvoize 2021-10-13 13:04:40 UTC
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.
Comment 21 Martin Renvoize 2021-10-13 13:04:44 UTC
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.
Comment 22 Tomás Cohen Arazi 2021-10-15 11:51:04 UTC
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>
Comment 23 Tomás Cohen Arazi 2021-10-15 11:51:10 UTC
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>
Comment 24 Tomás Cohen Arazi 2021-10-15 11:51:16 UTC
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>
Comment 25 Tomás Cohen Arazi 2021-10-15 11:51:24 UTC
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>
Comment 26 Tomás Cohen Arazi 2021-10-15 11:51:31 UTC
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>
Comment 27 Tomás Cohen Arazi 2021-10-15 11:51:38 UTC
Created attachment 126321 [details] [review]
Bug 24609: (QA follow-up) Remove diag from tests

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>