Bug 13895 - Add routes for checkouts retrieval and renewal
Summary: Add routes for checkouts retrieval and renewal
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST API (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Julian Maurice
QA Contact: Tomás Cohen Arazi
URL: https://wiki.koha-community.org/wiki/...
Keywords:
Depends on: 14868 16699 22132
Blocks: 16652 17003 17005 24543
  Show dependency treegraph
 
Reported: 2015-03-23 16:36 UTC by Julian Maurice
Modified: 2020-11-30 21:44 UTC (History)
18 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This new feature adds APIs for checkouts and renewals.
Version(s) released in:


Attachments
Bug 13895: Add API routes for issues retrieval and renewal (11.53 KB, patch)
2015-03-23 16:41 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13895: Add API routes for issues retrieval and renewal (11.89 KB, patch)
2015-06-12 12:37 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13895: Add API routes for issues retrieval and renewal (12.11 KB, patch)
2015-06-15 14:37 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13895: 5. Add API routes for issues retrieval and renewal (12.12 KB, patch)
2015-06-24 15:11 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13895: 5. Add API routes for issues retrieval and renewal (12.12 KB, patch)
2015-07-24 14:06 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13895: 5. Add API routes for issues retrieval and renewal (12.16 KB, patch)
2015-08-17 12:17 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 13895: Add API routes for checkouts retrieval and renewal (17.39 KB, patch)
2015-12-18 12:40 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13895: Add API routes for checkouts retrieval and renewal (15.20 KB, patch)
2016-04-07 12:53 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 13895: Add API routes for checkouts retrieval and renewal (15.65 KB, patch)
2016-05-06 22:01 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Fixed QA demands (1.04 KB, patch)
2016-07-31 09:19 UTC, Jiri Kozlovsky
Details | Diff | Splinter Review
Fixed QA demands (1.21 KB, patch)
2016-07-31 09:22 UTC, Jiri Kozlovsky
Details | Diff | Splinter Review
Bug 13895: Fixed QA demands (1.57 KB, patch)
2016-08-02 09:52 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 13895: Fixed QA demands (1.52 KB, patch)
2016-08-02 11:12 UTC, Frédéric Demians
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13895: Add API routes for checkouts retrieval and renewal (15.71 KB, patch)
2016-08-02 11:18 UTC, Lari Taskula
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13895: Fixed QA demands (1.58 KB, patch)
2016-08-02 11:19 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 13895: Allow user to access their own checkouts and renew them (9.56 KB, patch)
2016-08-02 13:00 UTC, Lari Taskula
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 13895: Allow user to access their own checkouts and renew them (9.65 KB, patch)
2016-08-02 19:27 UTC, Jiri Kozlovsky
Details | Diff | Splinter Review
Bug 13895: Reword borrowers to patrons in tags (1.33 KB, patch)
2016-08-09 11:39 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 13895: Reword borrowers to patrons in tags (1.39 KB, patch)
2016-08-22 12:43 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 13895: Add API routes for checkouts retrieval and renewal (19.87 KB, patch)
2016-08-28 16:39 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 13895: Add API routes for checkouts retrieval and renewal (19.86 KB, patch)
2016-10-11 10:45 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 13895: Add API routes for checkouts retrieval and renewal (19.85 KB, patch)
2016-10-14 11:01 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 13895: Add API routes for checkouts retrieval and renewal (19.87 KB, patch)
2016-12-22 13:57 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 13895: (QA followup) Adjust OpenAPI definitions for checkouts (2.69 KB, patch)
2017-11-13 14:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13895: (follow-up) Adapt checkout endpoint to openapi, update terminology (17.80 KB, patch)
2019-02-05 12:34 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 13895: Add API routes for checkouts retrieval and renewal (19.96 KB, patch)
2019-02-21 16:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 13895: (QA follow-up) Adjust OpenAPI definitions for checkouts (2.68 KB, patch)
2019-02-21 16:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 13895: (follow-up) Adapt checkout endpoint to openapi, update terminology (17.78 KB, patch)
2019-02-21 16:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 13895: Remove the opac checks in privileged endpoint (2.28 KB, patch)
2019-02-21 16:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 13895: Remove create_item and create_biblio subs from test (3.63 KB, patch)
2019-02-21 16:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 13895: Refactor test using REST Basic auth (10.31 KB, patch)
2019-02-21 16:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 13895: Change renewal endpoint (3.07 KB, patch)
2019-02-21 16:46 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 13895: (follow-up) Fix POST response (3.04 KB, patch)
2019-02-25 22:18 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 13895: (follow-up) Fix POST response (3.10 KB, patch)
2019-03-05 20:37 UTC, Michal Denar
Details | Diff | Splinter Review
Bug 13895: Adapt naming according to voted RFC (2.76 KB, patch)
2019-03-13 16:04 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 13895: Adapt naming according to voted RFC (3.61 KB, patch)
2019-03-13 16:09 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 13895: Fix test (733 bytes, patch)
2019-03-13 16:26 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 13895: Add API routes for checkouts retrieval and renewal (20.02 KB, patch)
2019-03-27 17:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13895: (QA follow-up) Adjust OpenAPI definitions for checkouts (2.70 KB, patch)
2019-03-27 17:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13895: (follow-up) Adapt checkout endpoint to openapi, update terminology (17.79 KB, patch)
2019-03-27 17:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13895: Remove the opac checks in privileged endpoint (2.29 KB, patch)
2019-03-27 17:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13895: Remove create_item and create_biblio subs from test (3.63 KB, patch)
2019-03-27 17:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13895: Refactor test using REST Basic auth (10.31 KB, patch)
2019-03-27 17:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13895: Change renewal endpoint (3.08 KB, patch)
2019-03-27 17:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13895: (follow-up) Fix POST response (3.10 KB, patch)
2019-03-27 17:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13895: Adapt naming according to voted RFC (3.61 KB, patch)
2019-03-27 17:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13895: Fix test (733 bytes, patch)
2019-03-27 17:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13895: (QA follow-up) Make the class plural (2.95 KB, patch)
2019-03-27 17:28 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 13895: (follow-up) Add Modern::Perl (749 bytes, patch)
2019-03-29 13:17 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Julian Maurice 2015-03-23 16:36:51 UTC
Add API routes for issues retrieval and renewal
Comment 1 Julian Maurice 2015-03-23 16:41:41 UTC Comment hidden (obsolete)
Comment 2 Olli-Antti Kivilahti 2015-06-12 12:37:11 UTC Comment hidden (obsolete)
Comment 3 Olli-Antti Kivilahti 2015-06-15 14:37:06 UTC Comment hidden (obsolete)
Comment 4 Olli-Antti Kivilahti 2015-06-24 15:11:45 UTC Comment hidden (obsolete)
Comment 5 Olli-Antti Kivilahti 2015-07-24 14:06:29 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2015-08-05 09:38:20 UTC
Blocked by bug 13799.
Comment 7 Olli-Antti Kivilahti 2015-08-17 12:17:55 UTC Comment hidden (obsolete)
Comment 8 Julian Maurice 2015-12-18 12:40:13 UTC Comment hidden (obsolete)
Comment 9 Julian Maurice 2015-12-18 12:46:06 UTC
Patch rebased on top of bug 15126

Signoff removed since this needed significant changes.

Notable changes that were not required for the rebase:
- "issue" reworded to "checkout"
- Routes have changed (e.g. /checkouts?borrowernumber instead of /patrons/{borrowernumber}/checkouts)
Comment 10 Benjamin Rokseth 2016-04-07 12:53:58 UTC Comment hidden (obsolete)
Comment 11 Benjamin Rokseth 2016-04-07 12:57:30 UTC
Rebased against #15548 Move Koha::Borrower -> Koha::Patron

Works fine in API, but tests fail:

not ok 23 - 200 OK
#   Failed test '200 OK'
#   at t/db_dependent/api/v1/checkouts.t line 119.
#          got: '500'
#     expected: '200'
not ok 24 - exact match for JSON Pointer "/date_due"
#   Failed test 'exact match for JSON Pointer "/date_due"'
#   at t/db_dependent/api/v1/checkouts.t line 119.
#          got: undef
#     expected: '2016-04-14 23:59:00'
ok 25 - PUT /api/v1/checkouts/6
not ok 26 - 403 Forbidden
#   Failed test '403 Forbidden'
#   at t/db_dependent/api/v1/checkouts.t line 125.
#          got: '500'
#     expected: '403'
not ok 27 - exact match for JSON Pointer ""
#   Failed test 'exact match for JSON Pointer ""'
#   at t/db_dependent/api/v1/checkouts.t line 125.
#     Structures begin differing at:
#          $got = undef
#     $expected = HASH(0xa7d1648)
# Looks like you failed 4 tests of 27.

Relevant log entry:

No branchcode argument passed to Koha::Calendar->new at /kohadev/kohaclone/C4/Circulation.pm line 3588
Comment 12 Benjamin Rokseth 2016-05-06 22:00:27 UTC
Rebased against master with Bug 15126 and Bug 13903.

Also fixed tests failing above due to missing branchcode in AddIssue and calculated duedate on renewal.
Comment 13 Benjamin Rokseth 2016-05-06 22:01:58 UTC Comment hidden (obsolete)
Comment 14 Jiri Kozlovsky 2016-06-19 17:57:31 UTC
Nice work!

have you thought of including the "renewability"/"renewable" to the list of returned values within an hold?

We would make use of it while showing user his holds by showing/hiding renew link to prevent misunderstanding when refused from clicking it.

Or should I create a new bug for this? I see the data comes from C4::Circulation::GetIssues
Comment 15 Jiri Kozlovsky 2016-06-19 23:05:33 UTC
By holds i meant checkouts of course, sorry :)
Comment 16 Lari Taskula 2016-06-21 16:04:22 UTC
(In reply to Jiří Kozlovský from comment #14)
> have you thought of including the "renewability"/"renewable" to the list of
> returned values
+1 I think this would be useful information.
Comment 17 Kyle M Hall 2016-06-24 18:24:37 UTC
Comment on attachment 51303 [details] [review]
Bug 13895: Add API routes for checkouts retrieval and renewal

Review of attachment 51303 [details] [review]:
-----------------------------------------------------------------

::: Koha/REST/V1/Checkout.pm
@@ +71,5 @@
> +    my $checkout = Koha::Issues->find($checkout_id);
> +
> +    if (!$checkout) {
> +        return $c->$cb({
> +            error => "Checkout doesn't exists"

Should be "Checkout doesn't exist"

@@ +76,5 @@
> +        }, 404);
> +    }
> +
> +    $checkout = $checkout->unblessed;
> +    my ($borrowernumber, $itemnumber) = @$checkout{qw(borrowernumber itemnumber)};

This seems slower than
my $borrowernumber = $checkout->borrowernumber;
my $itemnumber = $checkout->itemnumber;

am I wrong?

::: api/v1/definitions/checkout.json
@@ +29,5 @@
> +        "lastreneweddate": {
> +            "description": "Date the item was last renewed"
> +        },
> +        "return": {
> +            "description": "?"

Looks to be unused. Maybe we should file a bug report to remove it.
Comment 18 Kyle M Hall 2016-06-24 18:25:07 UTC
Just a couple very minor things. If those get fixed this should Pass QA!
Comment 19 Jiri Kozlovsky 2016-06-29 02:02:24 UTC
How can I specify the desired date_due to the API?
Comment 20 Jiri Kozlovsky 2016-07-31 09:19:57 UTC Comment hidden (obsolete)
Comment 21 Jiri Kozlovsky 2016-07-31 09:22:41 UTC Comment hidden (obsolete)
Comment 22 Frédéric Demians 2016-08-02 09:11:44 UTC
I've just tried this patch in order to test bug 17005 depending patch. For me
4 tests are failing: 23, 24, 26, 27. It seems that each time, I've got this
error:

  Can't call method "borrowernumber" on unblessed reference at
  <base>Koha/REST/V1/Checkout.pm line 81

It may be due to my testing environment. Can someone confirm that tests are
still working on master?
Comment 23 Lari Taskula 2016-08-02 09:52:03 UTC Comment hidden (obsolete)
Comment 24 Lari Taskula 2016-08-02 09:57:56 UTC
(In reply to Frédéric Demians from comment #22)
> I've just tried this patch in order to test bug 17005 depending patch. For me
> 4 tests are failing: 23, 24, 26, 27. It seems that each time, I've got this
> error:
> 
>   Can't call method "borrowernumber" on unblessed reference at
>   <base>Koha/REST/V1/Checkout.pm line 81
> 
> It may be due to my testing environment. Can someone confirm that tests are
> still working on master?
It was a small issue in the code. Calling $checkout->borrowernumber on unblessed reference $checkout, probably meant $checkout->{borrowernumber} instead, or even don't unbless $checkout(?).
Comment 25 Frédéric Demians 2016-08-02 11:12:40 UTC Comment hidden (obsolete)
Comment 26 Lari Taskula 2016-08-02 11:18:32 UTC Comment hidden (obsolete)
Comment 27 Lari Taskula 2016-08-02 11:19:00 UTC Comment hidden (obsolete)
Comment 28 Lari Taskula 2016-08-02 11:43:33 UTC
What about user who does not have 'circulate_remaining_permissions' permission, but wants to access his own checkouts or renew his own checkouts if OpacRenewalAllowed is on?
Comment 29 Lari Taskula 2016-08-02 13:00:20 UTC Comment hidden (obsolete)
Comment 30 Jiri Kozlovsky 2016-08-02 19:27:26 UTC Comment hidden (obsolete)
Comment 31 Lari Taskula 2016-08-09 11:39:29 UTC Comment hidden (obsolete)
Comment 32 Julian Maurice 2016-08-22 12:43:00 UTC Comment hidden (obsolete)
Comment 33 Julian Maurice 2016-08-22 12:43:33 UTC
Signed off the last patch
Comment 34 Katrin Fischer 2016-08-22 18:35:32 UTC
>Let user access their own checkouts and if OpacRenewalAllowed system preference
>is on, also let user to renew their checkouts.

I am concerned about this kind of behaviour. Would it mean that any user (without any permission) can do this using the REST API as long as they can get access to a valid session cookie/log into the OPAC?

This checks for OpacRenewalAllowed, but what about opacuserlogin?

Could we make this behaviour optional?
Comment 35 Lari Taskula 2016-08-23 11:21:35 UTC
(In reply to Katrin Fischer from comment #34)
> >Let user access their own checkouts and if OpacRenewalAllowed system preference
> >is on, also let user to renew their checkouts.
> 
> I am concerned about this kind of behaviour. Would it mean that any user
> (without any permission) can do this using the REST API as long as they can
> get access to a valid session cookie/log into the OPAC?
Yes, only for their own checkouts. Of course appropriate system preferences need to be considered, as you mentioned:

> This checks for OpacRenewalAllowed, but what about opacuserlogin?
Great suggestion, I totally ignored opacuserlogin. I think it is very important to also check opacuserlogin here and in other operations where this type of behaviour would be useful.

There are many operations where it would be useful to let resource owner to access their own data even if they have no special permissions for it (checkouts/history, holds, accountlines, patron info, password change etc.). For this behaviour, I have proposed a patch in Bug 14868 which will centralize that feature so we don't have to check permissions/ownership in each controller over and over again, and also for each operation will add permission documentation into Swagger. Perhaps opacuserlogin could be considered there and restrict access to this behaviour in every operation, if it is disabled.

> Could we make this behaviour optional?
I'm not fully sure I understand your concern enough to see why it should have extra optionality, if this behaviour is already enabled in OPAC. If your main concern was opacuserlogin, then I think it is enough to consider it. Are there some other preferences that should also be considered?

Thanks for the comment Katrin, very much appreciated!
Comment 36 Katrin Fischer 2016-08-23 22:17:09 UTC
I think what scares me off is that 'anyone' will be able to use the API to some extend - I am thinking of it not so much as a thing accessible to end users I guess. There would be no way to control who uses certain functionality. Maybe too paranoid?
Comment 37 Katrin Fischer 2016-08-23 22:17:52 UTC
But I certainly like the idea of centralizing the permission checks :)
Comment 38 Katrin Fischer 2016-08-24 20:04:59 UTC
Conclusion at the dev meeting was: I am too paranoid ;)
Comment 39 Katrin Fischer 2016-08-24 20:26:35 UTC
Should we make this also dependent on bug 14868?
We also need to resolve the question of taking opacuserlogin into account.
Comment 40 Lari Taskula 2016-08-25 11:18:44 UTC
(In reply to Katrin Fischer from comment #39)
> Should we make this also dependent on bug 14868?
I read the dev meeting logs and understood that people like the idea in Bug 14868, but as of now, I find it problematic that if any changes will still be introduced especially in Bug 14868's dependency Bug 16699, rebasing patches here will be quite time consuming and could cause hair-pulling. Perhaps we could wait until Bug 16699's structure gets some type of approval?

> We also need to resolve the question of taking opacuserlogin into account.
Yes I think this should be discussed, for example, could it make sense for somebody to disable opacuserlogin but still wanting to have API access to own objects? Or simply control both of these under the same preference, opacuserlogin?
Comment 41 Benjamin Rokseth 2016-08-28 16:39:21 UTC
Created attachment 54951 [details] [review]
Bug 13895: Add API routes for checkouts retrieval and renewal

GET /checkouts?borrowernumber={borrowernumber}
GET /checkouts/{checkout_id}
PUT /checkouts/{checkout_id}

+ unit tests in t/db_dependent/api/v1/checkouts.t

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You should have permission circulate_remaining_permissions.
2. Go to http://yourlibrary/api/v1/checkouts?borrowernumber=XXX (replace
   XXX with a borrowernumber that has checkouts) and check you receive
   correct data
3. Go to http://yourlibrary/api/v1/checkouts/YYY (replace YYY with an
   existing checkout id) and check you receive correct data
4. Send PUT requests to http://yourlibrary/api/v1/checkouts/YYY until
   the maximum number of renewals is reached (you should have a 403
   error)
5. Run unit tests in t/db_dependent/api/v1/checkouts.t

Depends on bugs 16699 and 14868

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Lari Taskula <larit@student.uef.fi>
Comment 42 Benjamin Rokseth 2016-08-28 16:44:04 UTC
Since Bug 16699 now already is in master, I took the liberty of rebasing this patch against master atop on 14868 which was discussed above.

Since rebasing each patch is too much work, I merged all the previous patches in.

No functionality is changed, and tests are updated to match new permissions from Bug 14868, so I leave it as Signed off, but more testing should probably be done.
Comment 43 Katrin Fischer 2016-08-29 06:11:30 UTC
> > We also need to resolve the question of taking opacuserlogin into account.
> Yes I think this should be discussed, for example, could it make sense for
> somebody to disable opacuserlogin but still wanting to have API access to
> own objects? Or simply control both of these under the same preference,
> opacuserlogin?

I can imagine both happening:
- libraries wanting to shut down any OPAC account functionality, but still be able to renew in staff, so the circulation conditions are set up this way. In this case, there should be a way to lock the API (opacuserlogin might be a way)
- libraries shutting down the OPAC, because they use something else like an external discovery layer. In this case they'd still want to use the API, but might turn off the OPAC as far as possible.

Should we just make a separate switch?
Comment 44 Lari Taskula 2016-10-11 10:45:22 UTC
Created attachment 56167 [details] [review]
Bug 13895: Add API routes for checkouts retrieval and renewal

Fixed tab-characters to spaces in api/v1/swagger/paths/checkouts.json

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Lari Taskula <larit@student.uef.fi>
Comment 45 Lari Taskula 2016-10-11 10:47:48 UTC
(In reply to Benjamin Rokseth from comment #42)
> Since Bug 16699 now already is in master, I took the liberty of rebasing
> this patch against master atop on 14868 which was discussed above.
> 
> Since rebasing each patch is too much work, I merged all the previous
> patches in.
> 
> No functionality is changed, and tests are updated to match new permissions
> from Bug 14868, so I leave it as Signed off, but more testing should
> probably be done.
Thanks Benjamin!

(In reply to Katrin Fischer from comment #43)
> > > We also need to resolve the question of taking opacuserlogin into account.
> > Yes I think this should be discussed, for example, could it make sense for
> > somebody to disable opacuserlogin but still wanting to have API access to
> > own objects? Or simply control both of these under the same preference,
> > opacuserlogin?
> 
> I can imagine both happening:
> - libraries wanting to shut down any OPAC account functionality, but still
> be able to renew in staff, so the circulation conditions are set up this
> way. In this case, there should be a way to lock the API (opacuserlogin
> might be a way)
> - libraries shutting down the OPAC, because they use something else like an
> external discovery layer. In this case they'd still want to use the API, but
> might turn off the OPAC as far as possible.
> 
> Should we just make a separate switch?
Yeah, I think it will be useful. I will open a new bug for it.
Comment 46 Lari Taskula 2016-10-12 11:56:43 UTC
(In reply to Lari Taskula from comment #45)
> (In reply to Katrin Fischer from comment #43)
> > > > We also need to resolve the question of taking opacuserlogin into account.
> > > Yes I think this should be discussed, for example, could it make sense for
> > > somebody to disable opacuserlogin but still wanting to have API access to
> > > own objects? Or simply control both of these under the same preference,
> > > opacuserlogin?
> > 
> > I can imagine both happening:
> > - libraries wanting to shut down any OPAC account functionality, but still
> > be able to renew in staff, so the circulation conditions are set up this
> > way. In this case, there should be a way to lock the API (opacuserlogin
> > might be a way)
> > - libraries shutting down the OPAC, because they use something else like an
> > external discovery layer. In this case they'd still want to use the API, but
> > might turn off the OPAC as far as possible.
> > 
> > Should we just make a separate switch?
> Yeah, I think it will be useful. I will open a new bug for it.
I opened Bug 17424 for discussing this & added an example patch.
Comment 47 Lari Taskula 2016-10-14 11:01:33 UTC
Created attachment 56506 [details] [review]
Bug 13895: Add API routes for checkouts retrieval and renewal

GET /checkouts?borrowernumber={borrowernumber}
GET /checkouts/{checkout_id}
PUT /checkouts/{checkout_id}

+ unit tests in t/db_dependent/api/v1/checkouts.t

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You should have permission circulate_remaining_permissions.
2. Go to http://yourlibrary/api/v1/checkouts?borrowernumber=XXX (replace
   XXX with a borrowernumber that has checkouts) and check you receive
   correct data
3. Go to http://yourlibrary/api/v1/checkouts/YYY (replace YYY with an
   existing checkout id) and check you receive correct data
4. Send PUT requests to http://yourlibrary/api/v1/checkouts/YYY until
   the maximum number of renewals is reached (you should have a 403
   error)
5. Run unit tests in t/db_dependent/api/v1/checkouts.t

Depends on bugs 16699 and 14868

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Lari Taskula <larit@student.uef.fi>
Comment 48 Lari Taskula 2016-12-22 13:57:33 UTC
Created attachment 58387 [details] [review]
Bug 13895: Add API routes for checkouts retrieval and renewal

GET /checkouts?borrowernumber={borrowernumber}
GET /checkouts/{checkout_id}
PUT /checkouts/{checkout_id}

+ unit tests in t/db_dependent/api/v1/checkouts.t

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You should have permission circulate_remaining_permissions.
2. Go to http://yourlibrary/api/v1/checkouts?borrowernumber=XXX (replace
   XXX with a borrowernumber that has checkouts) and check you receive
   correct data
3. Go to http://yourlibrary/api/v1/checkouts/YYY (replace YYY with an
   existing checkout id) and check you receive correct data
4. Send PUT requests to http://yourlibrary/api/v1/checkouts/YYY until
   the maximum number of renewals is reached (you should have a 403
   error)
5. Run unit tests in t/db_dependent/api/v1/checkouts.t

Depends on bugs 16699 and 14868

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Lari Taskula <larit@student.uef.fi>
Comment 49 Lari Taskula 2016-12-22 13:59:07 UTC
(In reply to Lari Taskula from comment #48)
> Created attachment 58387 [details] [review] [review]
This patch only changes Koha::Issues to Koha::Checkouts.
Comment 50 Marcel de Rooy 2017-06-16 09:45:12 UTC
Could not find "../x-primitives.json#/branchcode" (../x-primitives.json#/branchcode). Typo in schema "api/v1/swagger/definitions/checkout.json"? at JSON/Validator.pm line 229.
# Looks like your test exited with 255 before it could output anything.

This branchcode is not in api/v1/swagger/x-primitives.json ??

And a small bonus from qa tools:
 FAIL   t/db_dependent/api/v1/checkouts.t
   FAIL   forbidden patterns
                forbidden pattern: tab char (line 190)
Comment 51 David Cook 2017-10-04 00:11:40 UTC
What does "issues retrieval" mean?
Comment 52 Tomás Cohen Arazi 2017-11-10 18:41:34 UTC
(In reply to Katrin Fischer from comment #36)
> functionality. Maybe too paranoid?

Yes :-D
Comment 53 Tomás Cohen Arazi 2017-11-10 18:48:57 UTC
(In reply to David Cook from comment #51)
> What does "issues retrieval" mean?

That was renamed 'checkouts retrieval'. It is related to checkout informations :-P
Comment 54 Tomás Cohen Arazi 2017-11-13 14:16:06 UTC
Created attachment 69123 [details] [review]
Bug 13895: (QA followup) Adjust OpenAPI definitions for checkouts

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 55 Tomás Cohen Arazi 2017-11-13 14:21:41 UTC
I've added a followup fixing the spec (adding types, etc) and annotating the boolean columns so the returned types are correct.

This still needs to be migrated to the OpenAPI plugin.
POD missing too.

I'm not convinced by the idea of making PUT default to renewal. Please consider the 'checkin' scenario and how it could fit too. Right now the Issues and OldIssues schemas are separated, but conceptually, we should be updating a checkout when returning (setting returndate, etc).

So, it looks like a good starting point, but more work is needed, specially thinking about PUT. I can help next week.
Comment 56 Josef Moravec 2019-02-05 12:34:36 UTC
Created attachment 84765 [details] [review]
Bug 13895: (follow-up) Adapt checkout endpoint to openapi, update terminology
Comment 57 Josef Moravec 2019-02-05 13:56:36 UTC
I added follow-up too ;)

I made a migration to openapi and add some basic POD.
Comment 58 Josef Moravec 2019-02-06 07:39:31 UTC
I am going to add a follow-up for tests with basic authentication
Comment 59 Josef Moravec 2019-02-21 16:46:03 UTC
Created attachment 85458 [details] [review]
Bug 13895: Add API routes for checkouts retrieval and renewal

GET /checkouts?borrowernumber={borrowernumber}
GET /checkouts/{checkout_id}
PUT /checkouts/{checkout_id}

+ unit tests in t/db_dependent/api/v1/checkouts.t

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You should have permission circulate_remaining_permissions.
2. Go to http://yourlibrary/api/v1/checkouts?borrowernumber=XXX (replace
   XXX with a borrowernumber that has checkouts) and check you receive
   correct data
3. Go to http://yourlibrary/api/v1/checkouts/YYY (replace YYY with an
   existing checkout id) and check you receive correct data
4. Send PUT requests to http://yourlibrary/api/v1/checkouts/YYY until
   the maximum number of renewals is reached (you should have a 403
   error)
5. Run unit tests in t/db_dependent/api/v1/checkouts.t

Depends on bugs 16699 and 14868

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Lari Taskula <larit@student.uef.fi>
Comment 60 Josef Moravec 2019-02-21 16:46:13 UTC
Created attachment 85459 [details] [review]
Bug 13895: (QA follow-up) Adjust OpenAPI definitions for checkouts

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 61 Josef Moravec 2019-02-21 16:46:21 UTC
Created attachment 85460 [details] [review]
Bug 13895: (follow-up) Adapt checkout endpoint to openapi, update terminology
Comment 62 Josef Moravec 2019-02-21 16:46:28 UTC
Created attachment 85461 [details] [review]
Bug 13895: Remove the opac checks in privileged endpoint

Test plan:
prove t/db_dependent/api/v1/checkouts.t
Comment 63 Josef Moravec 2019-02-21 16:46:34 UTC
Created attachment 85462 [details] [review]
Bug 13895: Remove create_item and create_biblio subs from test

Test plan:
prove t/db_dependent/api/v1/checkouts.t
Comment 64 Josef Moravec 2019-02-21 16:46:42 UTC
Created attachment 85463 [details] [review]
Bug 13895: Refactor test using REST Basic auth

Test plan:
prove t/db_dependent/api/v1/checkouts.t
Comment 65 Josef Moravec 2019-02-21 16:46:51 UTC
Created attachment 85464 [details] [review]
Bug 13895: Change renewal endpoint

Instead of PUT on /checkout/{checkout_id} endpoint, you should now call
POST on /checkouts/{checkout_id}/renewal

Test plan:
prove t/db_dependent/api/v1/checkouts.t
Comment 66 Josef Moravec 2019-02-21 16:49:39 UTC
I added some more patches, to polish test and made some minor changes, it may require some more changes after this endpoint is voted on development meeting, but that can be done after, so setting need signoff to maybe get some feedback
Comment 67 Josef Moravec 2019-02-25 22:18:05 UTC
Created attachment 85676 [details] [review]
Bug 13895: (follow-up) Fix POST response

Test plan:
prove t/db_dependent/api/v1/chcekouts.t
Comment 68 Michal Denar 2019-03-05 20:37:40 UTC
Created attachment 86120 [details] [review]
Bug 13895: (follow-up) Fix POST response

Test plan:
prove t/db_dependent/api/v1/chcekouts.t

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 69 Josef Moravec 2019-03-13 16:04:22 UTC
Created attachment 86576 [details] [review]
Bug 13895: Adapt naming according to voted RFC
Comment 70 Josef Moravec 2019-03-13 16:09:55 UTC
Created attachment 86579 [details] [review]
Bug 13895: Adapt naming according to voted RFC
Comment 71 Josef Moravec 2019-03-13 16:26:27 UTC
Created attachment 86580 [details] [review]
Bug 13895: Fix test
Comment 72 Tomás Cohen Arazi 2019-03-27 17:27:11 UTC
Created attachment 87105 [details] [review]
Bug 13895: Add API routes for checkouts retrieval and renewal

GET /checkouts?borrowernumber={borrowernumber}
GET /checkouts/{checkout_id}
PUT /checkouts/{checkout_id}

+ unit tests in t/db_dependent/api/v1/checkouts.t

Test plan:
1. Open a browser tab on Koha staff and log in (to create CGISESSID
   cookie). You should have permission circulate_remaining_permissions.
2. Go to http://yourlibrary/api/v1/checkouts?borrowernumber=XXX (replace
   XXX with a borrowernumber that has checkouts) and check you receive
   correct data
3. Go to http://yourlibrary/api/v1/checkouts/YYY (replace YYY with an
   existing checkout id) and check you receive correct data
4. Send PUT requests to http://yourlibrary/api/v1/checkouts/YYY until
   the maximum number of renewals is reached (you should have a 403
   error)
5. Run unit tests in t/db_dependent/api/v1/checkouts.t

Depends on bugs 16699 and 14868

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Lari Taskula <larit@student.uef.fi>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 73 Tomás Cohen Arazi 2019-03-27 17:27:17 UTC
Created attachment 87106 [details] [review]
Bug 13895: (QA follow-up) Adjust OpenAPI definitions for checkouts

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 74 Tomás Cohen Arazi 2019-03-27 17:27:23 UTC
Created attachment 87107 [details] [review]
Bug 13895: (follow-up) Adapt checkout endpoint to openapi, update terminology
Comment 75 Tomás Cohen Arazi 2019-03-27 17:27:28 UTC
Created attachment 87108 [details] [review]
Bug 13895: Remove the opac checks in privileged endpoint

Test plan:
prove t/db_dependent/api/v1/checkouts.t
Comment 76 Tomás Cohen Arazi 2019-03-27 17:27:33 UTC
Created attachment 87109 [details] [review]
Bug 13895: Remove create_item and create_biblio subs from test

Test plan:
prove t/db_dependent/api/v1/checkouts.t
Comment 77 Tomás Cohen Arazi 2019-03-27 17:27:38 UTC
Created attachment 87110 [details] [review]
Bug 13895: Refactor test using REST Basic auth

Test plan:
prove t/db_dependent/api/v1/checkouts.t
Comment 78 Tomás Cohen Arazi 2019-03-27 17:27:43 UTC
Created attachment 87111 [details] [review]
Bug 13895: Change renewal endpoint

Instead of PUT on /checkout/{checkout_id} endpoint, you should now call
POST on /checkouts/{checkout_id}/renewal

Test plan:
prove t/db_dependent/api/v1/checkouts.t
Comment 79 Tomás Cohen Arazi 2019-03-27 17:27:47 UTC
Created attachment 87112 [details] [review]
Bug 13895: (follow-up) Fix POST response

Test plan:
prove t/db_dependent/api/v1/chcekouts.t

Signed-off-by: Michal Denar <black23@gmail.com>
Comment 80 Tomás Cohen Arazi 2019-03-27 17:27:52 UTC
Created attachment 87113 [details] [review]
Bug 13895: Adapt naming according to voted RFC
Comment 81 Tomás Cohen Arazi 2019-03-27 17:27:57 UTC
Created attachment 87114 [details] [review]
Bug 13895: Fix test
Comment 82 Tomás Cohen Arazi 2019-03-27 17:28:02 UTC
Created attachment 87115 [details] [review]
Bug 13895: (QA follow-up) Make the class plural

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 83 Nick Clemens 2019-03-28 19:40:17 UTC
Awesome work all!

Pushed to master for 19.05
Comment 84 Nick Clemens 2019-03-29 13:17:31 UTC
Created attachment 87200 [details] [review]
Bug 13895: (follow-up) Add Modern::Perl
Comment 85 Nick Clemens 2019-03-29 13:53:33 UTC
(In reply to Nick Clemens from comment #84)
> Created attachment 87200 [details] [review] [review]
> Bug 13895: (follow-up) Add Modern::Perl

Followup pushed to master
Comment 86 Martin Renvoize 2019-04-01 14:03:15 UTC
As is my policy of backporting api routes, pushed to 18.11.x for 18.11.05
Comment 87 Lucas Gass 2019-04-01 16:49:37 UTC
missing dependencies and wont be backporting to the 18.05.x series