Bug 23336 - Add an API endpoint for checking an item out to a patron
Summary: Add an API endpoint for checking an item out to a patron
Status: Needs documenting
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement with 3 votes (vote)
Assignee: Martin Renvoize
QA Contact: Kyle M Hall
URL:
Keywords:
: 24400 (view as bug list)
Depends on:
Blocks: 24401 33709 33711 36540 30979 34212
  Show dependency treegraph
 
Reported: 2019-07-18 09:57 UTC by Andrew Isherwood
Modified: 2024-04-26 20:35 UTC (History)
10 users (show)

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


Attachments
Bug 23336: Add POST /checkouts issue endpoint (10.04 KB, patch)
2019-07-19 11:06 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23336: Add POST /checkouts issue endpoint (9.90 KB, patch)
2019-07-19 11:08 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23336: Add POST /checkouts issue endpoint (9.91 KB, patch)
2019-07-19 11:09 UTC, Andrew Isherwood
Details | Diff | Splinter Review
Bug 23336: Unit tests (7.40 KB, patch)
2023-05-10 08:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: Add checkout API's (12.25 KB, patch)
2023-05-10 08:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: Fix inconsistent return of AddIssue (1012 bytes, patch)
2023-05-10 08:56 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: Unit tests (7.40 KB, patch)
2023-05-10 09:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: Add checkout API's (12.24 KB, patch)
2023-05-10 09:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: Fix inconsistent return of AddIssue (1007 bytes, patch)
2023-05-10 09:21 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: Unit tests (7.40 KB, patch)
2023-05-10 10:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: Add checkout API's (12.26 KB, patch)
2023-05-10 10:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: Fix inconsistent return of AddIssue (1007 bytes, patch)
2023-05-10 10:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: (follow-up) Switch to using Koha::Token (2.80 KB, patch)
2023-05-16 09:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: Unit tests (7.46 KB, patch)
2023-05-16 19:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23336: Add checkout API's (12.36 KB, patch)
2023-05-16 19:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23336: Fix inconsistent return of AddIssue (1.04 KB, patch)
2023-05-16 19:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23336: (follow-up) Switch to using Koha::Token (2.85 KB, patch)
2023-05-16 19:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23336: (QA follow-up) Some minor fixes (2.71 KB, patch)
2023-05-16 19:54 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23336: Unit tests (7.45 KB, patch)
2023-05-17 17:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: Add checkout API's (12.35 KB, patch)
2023-05-17 17:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: Fix inconsistent return of AddIssue (1.04 KB, patch)
2023-05-17 17:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: (follow-up) Switch to using Koha::Token (2.85 KB, patch)
2023-05-17 17:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: (QA follow-up) Minor fixes (6.70 KB, patch)
2023-05-17 17:01 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: Unit tests (7.45 KB, patch)
2023-06-27 10:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: Add checkout API's (12.35 KB, patch)
2023-06-27 10:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: Fix inconsistent return of AddIssue (1.04 KB, patch)
2023-06-27 10:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: (follow-up) Switch to using Koha::Token (2.85 KB, patch)
2023-06-27 10:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: (QA follow-up) Minor fixes (6.70 KB, patch)
2023-06-27 10:07 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: (QA follow-up) Minor fixes (1.37 KB, patch)
2023-07-03 13:21 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 23336: (follow-up) Fix failing test (1.48 KB, patch)
2023-07-04 06:10 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: (follow-up) Add checkout_availability schema (2.28 KB, patch)
2023-07-04 06:23 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 23336: Compare strings with eq instead of == (781 bytes, patch)
2023-07-06 13:42 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 23336: Fix tests (1.73 KB, patch)
2023-07-06 13:42 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Isherwood 2019-07-18 09:57:09 UTC
The Checkouts API route RFC (https://wiki.koha-community.org/wiki/Checkouts_endpoint_RFC) proposes a POST on /checkouts, which makes sense.
Comment 1 Andrew Isherwood 2019-07-19 11:06:38 UTC
Created attachment 91632 [details] [review]
Bug 23336: Add POST /checkouts issue endpoint

This patch adds a POST /checkouts issue endpoint for creating checkouts

Test plan:

- Run unit tests in t/db_dependent/api/v1/checkouts.t
- Make POST request to http://yourlibrary/api/v1/checkouts with request body
containing at least 'patron_id' & 'barcode' properties, other possible properties are:

    "due_date": Checkout due date
    "inprocess": Boolean
    "ignore_reserves": Boolean
    "cancel_reserve": Either null, "revert" or "cancel"
    "issue_date": Checkout issue date
    "sipmode": Boolean
    "params": Object containing arbitrary properties that will
              be passed to CanBookBeRenewed and AddIssue
- Observe returned data and HTTP code
  - 201 for checkout created
  - 403 for cannot be checked out (with reason in response body)
  - 404 for either invalid patron or barcode
  - 500 for something else went wrong
Comment 2 Andrew Isherwood 2019-07-19 11:08:02 UTC
Created attachment 91633 [details] [review]
Bug 23336: Add POST /checkouts issue endpoint

This patch adds a POST /checkouts issue endpoint for creating checkouts

Test plan:

- Run unit tests in t/db_dependent/api/v1/checkouts.t
- Make POST request to http://yourlibrary/api/v1/checkouts with request body
containing at least 'patron_id' & 'barcode' properties, other possible properties are:

    "due_date": Checkout due date
    "inprocess": Boolean
    "ignore_reserves": Boolean
    "cancel_reserve": Either null, "revert" or "cancel"
    "issue_date": Checkout issue date
    "sipmode": Boolean
    "params": Object containing arbitrary properties that will
              be passed to CanBookBeRenewed and AddIssue
- Observe returned data and HTTP code
  - 201 for checkout created
  - 403 for cannot be checked out (with reason in response body)
  - 404 for either invalid patron or barcode
  - 500 for something else went wrong
Comment 3 Andrew Isherwood 2019-07-19 11:09:37 UTC
Created attachment 91634 [details] [review]
Bug 23336: Add POST /checkouts issue endpoint

This patch adds a POST /checkouts issue endpoint for creating checkouts

Test plan:

- Run unit tests in t/db_dependent/api/v1/checkouts.t
- Make POST request to http://yourlibrary/api/v1/checkouts with request body
containing at least 'patron_id' & 'barcode' properties, other possible properties are:

    "due_date": Checkout due date
    "inprocess": Boolean
    "ignore_reserves": Boolean
    "cancel_reserve": Either null, "revert" or "cancel"
    "issue_date": Checkout issue date
    "sipmode": Boolean
    "params": Object containing arbitrary properties that will
              be passed to CanBookBeRenewed and AddIssue
- Observe returned data and HTTP code
  - 201 for checkout created
  - 403 for cannot be checked out (with reason in response body)
  - 404 for either invalid patron or barcode
  - 500 for something else went wrong
Comment 4 Ian Walls 2019-08-05 13:48:39 UTC
I'm in the process of reviewing this patch.

The unit tests pass without incident.

Unfortunately, I'm getting a 400 error with the message "Expected object - got null." when POSTing to the endpoint.  Not sure if it's my set up or not.

Once this is conquered, it would be helpful to have more information about what behavior to expect from requests with the other specified parameters.  Can they introduce other response codes in certain circumstances (such as 'cancel_reserve' = 'foo'), etc.
Comment 5 Katrin Fischer 2019-08-06 21:39:38 UTC
Hi Ian, 

I think your first comment warrants a "Failed QA" on this one to get the attention of the developer.

Also I think we need documentation on the wiki to vote on for this new part of the checkouts API. Right away I notice some things to fix: reserves for example should not be used, but hold, issue should be checkout etc. We have agreed to standardize on the English GUI language.
Comment 6 Andrew Isherwood 2019-08-07 09:17:24 UTC
Hi Ian

I've not been able to replicate your 400 when POSTing, it would be interesting to know if others can.

This patch is a *very* thin proxy on top of calls to C4::Circulation::CanBookBeIssued and C4::Circulation::AddIssue - terminology, parameter requirements and response specifications come directly from them.

Unfortunately CanBookBeIssued in particular can potentially return a wide range of responses, which are currently just proxied back to the API consumer in the "error" object. Whether any more preparation of the response prior to return is required is up for debate.
Comment 7 Martin Renvoize 2023-05-10 08:56:48 UTC Comment hidden (obsolete)
Comment 8 Martin Renvoize 2023-05-10 08:56:51 UTC Comment hidden (obsolete)
Comment 9 Martin Renvoize 2023-05-10 08:56:53 UTC Comment hidden (obsolete)
Comment 10 Martin Renvoize 2023-05-10 09:14:58 UTC
This is a complete rework based on the implementation I did for bug 30979
Comment 11 Martin Renvoize 2023-05-10 09:21:40 UTC Comment hidden (obsolete)
Comment 12 Martin Renvoize 2023-05-10 09:21:42 UTC Comment hidden (obsolete)
Comment 13 Martin Renvoize 2023-05-10 09:21:45 UTC Comment hidden (obsolete)
Comment 14 Martin Renvoize 2023-05-10 10:26:31 UTC
Created attachment 150960 [details] [review]
Bug 23336: Unit tests

This patch adds unit tests for the new checkout availability endpoint
we're going to introduce in this patchset.

Signed-off-by: Silvia Meakins <smeakins@eso.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 15 Martin Renvoize 2023-05-10 10:26:34 UTC
Created attachment 150961 [details] [review]
Bug 23336: Add checkout API's

This patch adds API's to allow for a checkout flow using the RESTful
API.

We add an availability endpoint to check an items current availability
status. The endpoint can be found at `/checkouts/availability` and is
a GET request that requires item_id and patron_id passed as parameters.

We return an availability object that includes blockers, confirms,
warnings and a confirmation token to be used for checkout.

We also add a corresponding checkout method to the `/checkouts` endpoint.
The method accepts a POST request with checkout details including item_id
, patron_id and the confirmation token in the body.

Future work: We should properly migrate CanBookBeIssued into Koha::* and
use that here instead of refering to C4::Circulation.

Signed-off-by: Silvia Meakins <smeakins@eso.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 16 Martin Renvoize 2023-05-10 10:26:36 UTC
Created attachment 150962 [details] [review]
Bug 23336: Fix inconsistent return of AddIssue

AddIssue can on occasion create a renewal instead of a fresh issue and
in such a case we currently return undefined. We should be consistent
and return the existing issue object for the renewal.

Signed-off-by: Silvia Meakins <smeakins@eso.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 17 Martin Renvoize 2023-05-11 07:32:23 UTC
*** Bug 24400 has been marked as a duplicate of this bug. ***
Comment 18 Danyon Sewell 2023-05-11 23:32:45 UTC
Going to be working on the check-in API endpoint now just in case there is already work being done on it, I really should have checked for duplicates before working on the checkout one too but oh well.
Comment 19 Katrin Fischer 2023-05-14 11:11:52 UTC
(In reply to Danyon Sewell from comment #18)
> Going to be working on the check-in API endpoint now just in case there is
> already work being done on it, I really should have checked for duplicates
> before working on the checkout one too but oh well.

Duplicates are not always easy to spot especially with different terminology used (issue/checkout etc.). I hope this fits your needs as well? And welcome, Danyon :)
Comment 20 Martin Renvoize 2023-05-15 07:16:45 UTC
I'm not aware of anyone working on a Check-in endpoint.. do you have a specification you're working from? I found checkout to be rather complex given all the cases that need to be accounted for (and not all of them are accounted or here as yet.. hence the tail of follow-up bugs) and I'm envisaging the same for Check-in.
Comment 21 Jonathan Druart 2023-05-16 07:36:25 UTC
Why are not you using Koha::Token here?

Koha/Token.pm:use Mojo::JWT;
Comment 22 Jonathan Druart 2023-05-16 07:44:35 UTC
Shouldn't we also rework/reword the confirmation/errors/warnings we send to the client?
Comment 23 Martin Renvoize 2023-05-16 08:38:38 UTC
(In reply to Jonathan Druart from comment #21)
> Why are not you using Koha::Token here?
> 
> Koha/Token.pm:use Mojo::JWT;

Because the work I split this out from has been in progress for months and I totally forgot the existence of Koha::Token. I can follow-up here to move to that.
Comment 24 Martin Renvoize 2023-05-16 08:40:32 UTC
(In reply to Jonathan Druart from comment #22)
> Shouldn't we also rework/reword the confirmation/errors/warnings we send to
> the client?

Also yes.. but I wanted to continue working through modernising this in follow-up bugs.. I was under pressure to get the OPAC trusted self-checkout bug that this is at the bottom of the tree of in... and I found that doing the job properly was well and truly out of scope for the sponsor.. I intend to continue in my own time to improve things here.. perhaps we could mark the endpoint as experimental, allowing ourselves to continue the cleanup whilst still pushing this to allow the core feature to be present.
Comment 25 Martin Renvoize 2023-05-16 09:03:35 UTC
Having now looked at Koha::Token it really doesn't feel appropriate here.. we get rid of claims entirely which is what I'm relying on here.. I find it strange to drop the fundamental point of a JWT in Koha::Token.. can you explain?
Comment 26 Jonathan Druart 2023-05-16 09:12:12 UTC
(In reply to Martin Renvoize from comment #25)
> Having now looked at Koha::Token it really doesn't feel appropriate here..
> we get rid of claims entirely which is what I'm relying on here.. I find it
> strange to drop the fundamental point of a JWT in Koha::Token.. can you
> explain?

We can extend it if you think it's missing something. But IMO we should not use Mojo::JWT directly if we have our own Koha::Token that is using it.
Comment 27 Martin Renvoize 2023-05-16 09:47:13 UTC
Created attachment 151253 [details] [review]
Bug 23336: (follow-up) Switch to using Koha::Token
Comment 28 Martin Renvoize 2023-05-16 09:49:50 UTC
Follow-up to use Koha::Token now added.
Comment 29 Tomás Cohen Arazi 2023-05-16 19:54:12 UTC
Created attachment 151318 [details] [review]
Bug 23336: Unit tests

This patch adds unit tests for the new checkout availability endpoint
we're going to introduce in this patchset.

Signed-off-by: Silvia Meakins <smeakins@eso.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 30 Tomás Cohen Arazi 2023-05-16 19:54:15 UTC
Created attachment 151319 [details] [review]
Bug 23336: Add checkout API's

This patch adds API's to allow for a checkout flow using the RESTful
API.

We add an availability endpoint to check an items current availability
status. The endpoint can be found at `/checkouts/availability` and is
a GET request that requires item_id and patron_id passed as parameters.

We return an availability object that includes blockers, confirms,
warnings and a confirmation token to be used for checkout.

We also add a corresponding checkout method to the `/checkouts` endpoint.
The method accepts a POST request with checkout details including item_id
, patron_id and the confirmation token in the body.

Future work: We should properly migrate CanBookBeIssued into Koha::* and
use that here instead of refering to C4::Circulation.

Signed-off-by: Silvia Meakins <smeakins@eso.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 31 Tomás Cohen Arazi 2023-05-16 19:54:19 UTC
Created attachment 151320 [details] [review]
Bug 23336: Fix inconsistent return of AddIssue

AddIssue can on occasion create a renewal instead of a fresh issue and
in such a case we currently return undefined. We should be consistent
and return the existing issue object for the renewal.

Signed-off-by: Silvia Meakins <smeakins@eso.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 32 Tomás Cohen Arazi 2023-05-16 19:54:22 UTC
Created attachment 151321 [details] [review]
Bug 23336: (follow-up) Switch to using Koha::Token

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 33 Tomás Cohen Arazi 2023-05-16 19:54:25 UTC
Created attachment 151322 [details] [review]
Bug 23336: (QA follow-up) Some minor fixes

This patch deals with some QA script warnings, and also makes some
changes in line with bug 33556.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 34 Tomás Cohen Arazi 2023-05-16 19:57:21 UTC
I took some time to review this and added my two cents.

Things to note:
- There are a couple failing tests
- I expected to see a use of check_jwt (at least) in the tests 
- Maybe the Koha::Token use on bug 33675 might be of help (I used the session id for the token id, etc)
Comment 35 Martin Renvoize 2023-05-17 17:01:10 UTC
Created attachment 151353 [details] [review]
Bug 23336: Unit tests

This patch adds unit tests for the new checkout availability endpoint
we're going to introduce in this patchset.

Signed-off-by: Silvia Meakins <smeakins@eso.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 36 Martin Renvoize 2023-05-17 17:01:14 UTC
Created attachment 151354 [details] [review]
Bug 23336: Add checkout API's

This patch adds API's to allow for a checkout flow using the RESTful
API.

We add an availability endpoint to check an items current availability
status. The endpoint can be found at `/checkouts/availability` and is
a GET request that requires item_id and patron_id passed as parameters.

We return an availability object that includes blockers, confirms,
warnings and a confirmation token to be used for checkout.

We also add a corresponding checkout method to the `/checkouts` endpoint.
The method accepts a POST request with checkout details including item_id
, patron_id and the confirmation token in the body.

Future work: We should properly migrate CanBookBeIssued into Koha::* and
use that here instead of refering to C4::Circulation.

Signed-off-by: Silvia Meakins <smeakins@eso.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 37 Martin Renvoize 2023-05-17 17:01:17 UTC
Created attachment 151355 [details] [review]
Bug 23336: Fix inconsistent return of AddIssue

AddIssue can on occasion create a renewal instead of a fresh issue and
in such a case we currently return undefined. We should be consistent
and return the existing issue object for the renewal.

Signed-off-by: Silvia Meakins <smeakins@eso.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 38 Martin Renvoize 2023-05-17 17:01:20 UTC
Created attachment 151356 [details] [review]
Bug 23336: (follow-up) Switch to using Koha::Token

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 39 Martin Renvoize 2023-05-17 17:01:24 UTC
Created attachment 151357 [details] [review]
Bug 23336: (QA follow-up) Minor fixes

This patch deals with some QA script warnings, and also makes some
changes in line with bug 33556.

We also adds current user id and checkout item id into the confirmation
token to improve security and fix the failing tests.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 40 Martin Renvoize 2023-05-17 17:03:58 UTC
Tests are passing again, and a little more thorough.. I kept your fixes and squashed a few extra's of my own into that same patch.

Back to PQA to see what you think..  at this point I'm not worried if we hit 23.05, though I'd keep this and the tree that depends on it moving if we don't hit release so it can be pushed early next cycle rather than get stuck for another few months and pushed up against the deadline of 23.11
Comment 41 David Nind 2023-05-22 00:19:12 UTC
Patch no longer applies 8-(.. (from trying to test bug 24401):

git bz apply 23336

Bug 23336 - Add an API endpoint for checking an item out to a patron

151322 - Bug 23336: (QA follow-up) Some minor fixes
151353 - Bug 23336: Unit tests
151354 - Bug 23336: Add checkout API's
151355 - Bug 23336: Fix inconsistent return of AddIssue
151356 - Bug 23336: (follow-up) Switch to using Koha::Token
151357 - Bug 23336: (QA follow-up) Minor fixes

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 23336: (QA follow-up) Some minor fixes
error: sha1 information is lacking or useless (Koha/REST/V1/Checkouts.pm).
error: could not build fake ancestor
Patch failed at 0001 Bug 23336: (QA follow-up) Some minor fixes
Comment 42 Martin Renvoize 2023-06-27 10:07:38 UTC
Created attachment 152712 [details] [review]
Bug 23336: Unit tests

This patch adds unit tests for the new checkout availability endpoint
we're going to introduce in this patchset.

Signed-off-by: Silvia Meakins <smeakins@eso.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 43 Martin Renvoize 2023-06-27 10:07:42 UTC
Created attachment 152713 [details] [review]
Bug 23336: Add checkout API's

This patch adds API's to allow for a checkout flow using the RESTful
API.

We add an availability endpoint to check an items current availability
status. The endpoint can be found at `/checkouts/availability` and is
a GET request that requires item_id and patron_id passed as parameters.

We return an availability object that includes blockers, confirms,
warnings and a confirmation token to be used for checkout.

We also add a corresponding checkout method to the `/checkouts` endpoint.
The method accepts a POST request with checkout details including item_id
, patron_id and the confirmation token in the body.

Future work: We should properly migrate CanBookBeIssued into Koha::* and
use that here instead of refering to C4::Circulation.

Signed-off-by: Silvia Meakins <smeakins@eso.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 44 Martin Renvoize 2023-06-27 10:07:46 UTC
Created attachment 152714 [details] [review]
Bug 23336: Fix inconsistent return of AddIssue

AddIssue can on occasion create a renewal instead of a fresh issue and
in such a case we currently return undefined. We should be consistent
and return the existing issue object for the renewal.

Signed-off-by: Silvia Meakins <smeakins@eso.org>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 45 Martin Renvoize 2023-06-27 10:07:50 UTC
Created attachment 152715 [details] [review]
Bug 23336: (follow-up) Switch to using Koha::Token

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 46 Martin Renvoize 2023-06-27 10:07:53 UTC
Created attachment 152716 [details] [review]
Bug 23336: (QA follow-up) Minor fixes

This patch deals with some QA script warnings, and also makes some
changes in line with bug 33556.

We also adds current user id and checkout item id into the confirmation
token to improve security and fix the failing tests.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 47 Tomás Cohen Arazi 2023-07-03 13:21:26 UTC
Created attachment 152938 [details] [review]
Bug 23336: (QA follow-up) Minor fixes

There are two leftover includes, and a couple typos in a comment and POD
title.

This patch fixes that.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 48 Tomás Cohen Arazi 2023-07-03 13:34:28 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 49 Tomás Cohen Arazi 2023-07-03 17:38:16 UTC
Hi, this needs more work:

1) prove xt/api.t => FAIL. It seems you either need to add an exception ofr the added route or add the `additionalProperties: false` on the object definition.

2) prove t/db_dependent/Circulation.t => FAIL. 

38139718f534f308e5eb846f5e26ef54c01c3376 is the first bad commit
commit 38139718f534f308e5eb846f5e26ef54c01c3376
Author: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Date:   Mon Oct 3 17:07:20 2022 +0100

    Bug 23336: Fix inconsistent return of AddIssue

    AddIssue can on occasion create a renewal instead of a fresh issue and
    in such a case we currently return undefined. We should be consistent
    and return the existing issue object for the renewal.

    Signed-off-by: Silvia Meakins <smeakins@eso.org>
    Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
    Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

 C4/Circulation.pm | 1 +

So probably just a matter of adjusting the tests BUT I'd like you to check the uses of AddIssue, in case there's some stray case in which it is expecting an empty string.
Comment 50 Martin Renvoize 2023-07-04 06:10:16 UTC
Created attachment 152999 [details] [review]
Bug 23336: (follow-up) Fix failing test

Our consistency improvement to the AddIssue return in bug 23336
highlighted a bad test assumption.  The 'Reinsert the original issue'
line was silently failing and as such the subsequent test lines were
actually resulting in a renewal (which previous to this patchset
returned an empty return that happened to match the empty return of an
issue failure)
Comment 51 Martin Renvoize 2023-07-04 06:23:34 UTC
Created attachment 153000 [details] [review]
Bug 23336: (follow-up) Add checkout_availability schema

At some point in the patch series we lost the availability api schema.
This patch restores a basic version, but we should work towards a
clearer enum based schema for each of the available blockers, confirms
and warnings.
Comment 52 Jonathan Druart 2023-07-04 13:04:10 UTC
C4::Circ

1536         # get actual issuing if there is one
1537         my $actualissue = $item_object->checkout;

1549             $issue = Koha::Checkouts->find( { itemnumber => $item_object->itemnumber } );

1549 should call ->checkout as well
Comment 53 Jonathan Druart 2023-07-04 13:51:24 UTC
There are a couple of warnings

t/db_dependent/api/v1/checkouts.t ...... 7/101 Argument "1672:1738:confirm1:confirm2:please" isn't numeric in numeric eq (==) at /kohadevbox/koha/Koha/Token.pm line 298.
Argument "1672:1738:confirm1:confirm2" isn't numeric in numeric eq (==) at /kohadevbox/koha/Koha/Token.pm line 298.
t/db_dependent/api/v1/checkouts.t ...... 100/101 Argument "1674:1739:confirm1:confirm2" isn't numeric in numeric eq (==) at /kohadevbox/koha/Koha/Token.pm line 298.
Argument "1674:1739:confirm1:confirm2:please" isn't numeric in numeric eq (==) at /kohadevbox/koha/Koha/Token.pm line 298.
Comment 54 Tomás Cohen Arazi 2023-07-05 13:01:33 UTC
(In reply to Jonathan Druart from comment #52)
> C4::Circ
> 
> 1536         # get actual issuing if there is one
> 1537         my $actualissue = $item_object->checkout;
> 
> 1549             $issue = Koha::Checkouts->find( { itemnumber =>
> $item_object->itemnumber } );
> 
> 1549 should call ->checkout as well

Do you think this is a blocker for the follow-ups? Or can that be moved to a separate bug for Martin to take care?
Comment 55 Jonathan Druart 2023-07-05 13:31:24 UTC
(In reply to Tomás Cohen Arazi from comment #54)
> (In reply to Jonathan Druart from comment #52)
> > C4::Circ
> > 
> > 1536         # get actual issuing if there is one
> > 1537         my $actualissue = $item_object->checkout;
> > 
> > 1549             $issue = Koha::Checkouts->find( { itemnumber =>
> > $item_object->itemnumber } );
> > 
> > 1549 should call ->checkout as well
> 
> Do you think this is a blocker for the follow-ups? Or can that be moved to a
> separate bug for Martin to take care?

That is not related to the follow-ups. That can be done separately, or kept as it if you consider it's ok.
Comment 56 Tomás Cohen Arazi 2023-07-05 15:20:23 UTC
Follow-ups pushed. Martin will file a separate bug for those standing issues.
Comment 57 Jonathan Druart 2023-07-06 13:39:16 UTC
(In reply to Jonathan Druart from comment #53)
> There are a couple of warnings
> 
> t/db_dependent/api/v1/checkouts.t ...... 7/101 Argument
> "1672:1738:confirm1:confirm2:please" isn't numeric in numeric eq (==) at
> /kohadevbox/koha/Koha/Token.pm line 298.
> Argument "1672:1738:confirm1:confirm2" isn't numeric in numeric eq (==) at
> /kohadevbox/koha/Koha/Token.pm line 298.
> t/db_dependent/api/v1/checkouts.t ...... 100/101 Argument
> "1674:1739:confirm1:confirm2" isn't numeric in numeric eq (==) at
> /kohadevbox/koha/Koha/Token.pm line 298.
> Argument "1674:1739:confirm1:confirm2:please" isn't numeric in numeric eq
> (==) at /kohadevbox/koha/Koha/Token.pm line 298.

There is something very wrong with this warning, it should not be ignored.
Comment 58 Jonathan Druart 2023-07-06 13:42:37 UTC
Created attachment 153095 [details] [review]
Bug 23336: Compare strings with eq instead of ==

And don't ignore warnings next time ;)
Comment 59 Jonathan Druart 2023-07-06 13:42:41 UTC
Created attachment 153096 [details] [review]
Bug 23336: Fix tests

Not sure what we were trying to do, but apparently we only use the keys
to generate the id string. How did "please" end up here?
Comment 60 Jonathan Druart 2023-07-07 09:44:23 UTC
Last 2 patches need a stamp from someone involved originally here.
Comment 61 Martin Renvoize 2023-07-07 11:36:02 UTC
See 34212 for all follow-ups (I move Jonathan's there too as I'd already started working on them there at RM request)
Comment 62 Martin Renvoize 2023-07-17 14:49:10 UTC
Most certainly an enhancement and new feature on top.. we won't be backporting this to 23.05.x :(
Comment 63 Jan Kissig 2023-08-30 11:59:47 UTC
In the first patches for this API route a checkout via barcode and patron_id was used but the param barcode got replaced by item_id.

If I want to use this API, in my case for checking in/out items via RFID (not SIP2/NCIP), I cannot rely on an item_id as only the barcode is saved on RFID tags. Is it still possible to checkout/checkin items via barcode?
Comment 64 Martin Renvoize 2023-09-04 10:40:08 UTC
I'd suggest using two API calls.. one to fetch the item by barcode and then a second that uses the item id returned to action the checkout.
Comment 65 Jan Kissig 2023-09-05 07:50:27 UTC
> I'd suggest using two API calls.. 
> one to fetch the item by barcode and then 
> a second that uses the item id returned to action the checkout.

It sounds a bit expensive to me, to fetch an item by its barcode first only to get the item_id. Also fetching an item seems only possible via item_id as the api docs show: 
GET /api/v1/items/{item_id}

I would suggest to check which param is set, either item_id or barcode, and then retrieve the item by the given param
Comment 66 Katrin Fischer 2023-09-05 07:56:24 UTC
(In reply to Jan Kissig from comment #65)
> > I'd suggest using two API calls.. 
> > one to fetch the item by barcode and then 
> > a second that uses the item id returned to action the checkout.
> 
> It sounds a bit expensive to me, to fetch an item by its barcode first only
> to get the item_id. Also fetching an item seems only possible via item_id as
> the api docs show: 
> GET /api/v1/items/{item_id}
> 
> I would suggest to check which param is set, either item_id or barcode, and
> then retrieve the item by the given param

I think you would use "List items" with a query for the barcode.
Comment 67 Martin Renvoize 2023-09-12 07:10:57 UTC
Of course, we're open to follow-up enhancements atop this functionality.

An example of the API in use in practice can be seen in the trusted self checkout development.. code visible here: https://git.koha-community.org/Koha-community/Koha/src/branch/master/koha-tmpl/opac-tmpl/bootstrap/js/modals/checkout.js

I'd be happy for a barcode based lookup option to be added somewhere into the mix.. just need a clear and consistent way of exposing it.