Add API routes for issues retrieval and renewal
Created attachment 37146 [details] [review] Bug 13895: Add API routes for issues retrieval and renewal GET /borrowers/{borrowernumber}/issues GET /borrowers/{borrowernumber}/issues/{itemnumber} PUT /borrowers/{borrowernumber}/issues/{itemnumber} + unit tests in t/db_dependent/api/v1/borrowers/issues.t Test plan: 1/ Go to http://api.yourlibrary/v1/doc and test the above routes with different values for each parameter 2/ Try to renew the same issue (with PUT) until the maximum number of renewals is reached (you should have a 403 error then) 3/ Run unit tests in t/db_dependent/api/v1/borrowers/issues.t Depends on bug 13799
Created attachment 40104 [details] [review] Bug 13895: Add API routes for issues retrieval and renewal GET /borrowers/{borrowernumber}/issues GET /borrowers/{borrowernumber}/issues/{itemnumber} PUT /borrowers/{borrowernumber}/issues/{itemnumber} + unit tests in t/db_dependent/api/v1/borrowers/issues.t Test plan: 1/ Go to http://api.yourlibrary/v1/doc and test the above routes with different values for each parameter 2/ Try to renew the same issue (with PUT) until the maximum number of renewals is reached (you should have a 403 error then) 3/ Run unit tests in t/db_dependent/api/v1/borrowers/issues.t Depends on bugg 13799 FOLLOWUP 1: -------------- Rebased t/db_dependent/api/v1/borrowers/issues.t because C4::Circulation::AddIssue() was changed to return a DBIx-object, by Bugg 13315 which should have nothing to do modifying AddIssue()-function.
Created attachment 40143 [details] [review] Bug 13895: Add API routes for issues retrieval and renewal GET /borrowers/{borrowernumber}/issues GET /borrowers/{borrowernumber}/issues/{itemnumber} PUT /borrowers/{borrowernumber}/issues/{itemnumber} + unit tests in t/db_dependent/api/v1/borrowers/issues.t Test plan: 1/ Go to http://api.yourlibrary/v1/doc and test the above routes with different values for each parameter 2/ Try to renew the same issue (with PUT) until the maximum number of renewals is reached (you should have a 403 error then) 3/ Run unit tests in t/db_dependent/api/v1/borrowers/issues.t Depends on bugg 13799 FOLLOWUP 1: -------------- Rebased t/db_dependent/api/v1/borrowers/issues.t because C4::Circulation::AddIssue() was changed to return a DBIx-object, by Bugg 13315 which should have nothing to do modifying AddIssue()-function. Signed-off-by: Olli-Antti Kivilahti <olli-antti.kivilahti@jns.fi> Unit tests pass 100% Tested the new API calls using Swagger2 Triggered error cases: -Too many renewals. -Item not checked out or checked out to the wrong borrower.
Created attachment 40588 [details] [review] Bug 13895: 5. Add API routes for issues retrieval and renewal
Created attachment 41163 [details] [review] Bug 13895: 5. Add API routes for issues retrieval and renewal
Blocked by bug 13799.
Created attachment 41548 [details] [review] Bug 13895: 5. Add API routes for issues retrieval and renewal
Created attachment 45817 [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 bug 15126
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)
Created attachment 50021 [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 bug 15126
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
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.
Created attachment 51303 [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 bug 15126 Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
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
By holds i meant checkouts of course, sorry :)
(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 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.
Just a couple very minor things. If those get fixed this should Pass QA!
How can I specify the desired date_due to the API?
Created attachment 53816 [details] [review] Fixed QA demands
Created attachment 53817 [details] [review] Fixed QA demands https://bugs.koha-community.org/show_bug.cgi?id=13895 Signed-off-by: Jiří Kozlovský <mail@jkozlovsky.cz> Test plan still working
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?
Created attachment 53878 [details] [review] Bug 13895: Fixed QA demands https://bugs.koha-community.org/show_bug.cgi?id=13895 Signed-off-by: Jiří Kozlovský <mail@jkozlovsky.cz> Test plan still working Fixes Can't call method "borrowernumber" on unblessed reference at <base>Koha/REST/V1/Checkout.pm line 81
(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(?).
Created attachment 53889 [details] [review] Bug 13895: Fixed QA demands Signed-off-by: Frédéric Demians <f.demians@tamil.fr> Fix QA, and fix tests failing
Created attachment 53890 [details] [review] [SIGNED-OFF] 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 bug 15126 Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> Signed-off-by: Lari Taskula <larit@student.uef.fi>
Created attachment 53891 [details] [review] [SIGNED-OFF] Bug 13895: Fixed QA demands Signed-off-by: Frédéric Demians <f.demians@tamil.fr> Fix QA, and fix tests failing Signed-off-by: Lari Taskula <larit@student.uef.fi>
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?
Created attachment 53901 [details] [review] Bug 13895: Allow user to access their own checkouts and renew them Let user access their own checkouts and if OpacRenewalAllowed system preference is on, also let user to renew their checkouts. Test plan: 1. Open a browser tab on Koha staff and log in (to create CGISESSID cookie) with a Patron that has no permissions. This Patron will be referred as "your patron" or "your borrowernumber" below. 2. Go to http://yourlibrary/api/v1/checkouts?borrowernumber=XXX (replace XXX with your borrowernumber) and check you receive correct data 3. Go to http://yourlibrary/api/v1/checkouts?borrowernumber=XXX (replace XXX with someone else's borrowernumber) and check you get a permission error. 4. Go to http://yourlibrary/api/v1/checkouts/YYY (replace YYY with an existing checkout id of your Patron) and check you receive correct data 5. Go to http://yourlibrary/api/v1/checkouts/YYY (replace YYY with an existing checkout id of some other Patron) and check you get a permission error. 6. Send PUT request to http://yourlibrary/api/v1/checkouts/YYY, replace YYY with existing checkout id of some other Patron. You should get a permission error. 7. Set system preference OpacRenewalAllowed to 0. 8. Send PUT request to http://yourlibrary/api/v1/checkouts/YYY. YYY should be checkout id of checkout for your patron. You should get a permission error. 9. Set system preference OpacRenewalAllowed to 1. 10. Send PUT requests to http://yourlibrary/api/v1/checkouts/YYY until the maximum number of renewals is reached (you should have a 403 error). YYY should be checkout id of checkout for your patron. 11. Run unit tests in t/db_dependent/api/v1/checkouts.t
Created attachment 53924 [details] [review] [SIGNED-OFF] Bug 13895: Allow user to access their own checkouts and renew them Let user access their own checkouts and if OpacRenewalAllowed system preference is on, also let user to renew their checkouts. Test plan: 1. Open a browser tab on Koha staff and log in (to create CGISESSID cookie) with a Patron that has no permissions. This Patron will be referred as "your patron" or "your borrowernumber" below. 2. Go to http://yourlibrary/api/v1/checkouts?borrowernumber=XXX (replace XXX with your borrowernumber) and check you receive correct data 3. Go to http://yourlibrary/api/v1/checkouts?borrowernumber=XXX (replace XXX with someone else's borrowernumber) and check you get a permission error. 4. Go to http://yourlibrary/api/v1/checkouts/YYY (replace YYY with an existing checkout id of your Patron) and check you receive correct data 5. Go to http://yourlibrary/api/v1/checkouts/YYY (replace YYY with an existing checkout id of some other Patron) and check you get a permission error. 6. Send PUT request to http://yourlibrary/api/v1/checkouts/YYY, replace YYY with existing checkout id of some other Patron. You should get a permission error. 7. Set system preference OpacRenewalAllowed to 0. 8. Send PUT request to http://yourlibrary/api/v1/checkouts/YYY. YYY should be checkout id of checkout for your patron. You should get a permission error. 9. Set system preference OpacRenewalAllowed to 1. 10. Send PUT requests to http://yourlibrary/api/v1/checkouts/YYY until the maximum number of renewals is reached (you should have a 403 error). YYY should be checkout id of checkout for your patron. 11. Run unit tests in t/db_dependent/api/v1/checkouts.t Signed-off-by: Jiri Kozlovsky <mail@jkozlovsky.cz> Wow, works like magic! :) Nice job!
Created attachment 54196 [details] [review] Bug 13895: Reword borrowers to patrons in tags We probably want to use patrons instead of borrowers; otherwise we would have both tags since /patrons adds "patrons" as well.
Created attachment 54685 [details] [review] Bug 13895: Reword borrowers to patrons in tags We probably want to use patrons instead of borrowers; otherwise we would have both tags since /patrons adds "patrons" as well. Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed off the last patch
>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?
(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!
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?
But I certainly like the idea of centralizing the permission checks :)
Conclusion at the dev meeting was: I am too paranoid ;)
Should we make this also dependent on bug 14868? We also need to resolve the question of taking opacuserlogin into account.
(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?
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>
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.
> > 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?
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>
(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.
(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.
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>
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>
(In reply to Lari Taskula from comment #48) > Created attachment 58387 [details] [review] [review] This patch only changes Koha::Issues to Koha::Checkouts.
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)
What does "issues retrieval" mean?
(In reply to Katrin Fischer from comment #36) > functionality. Maybe too paranoid? Yes :-D
(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
Created attachment 69123 [details] [review] Bug 13895: (QA followup) Adjust OpenAPI definitions for checkouts Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
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.
Created attachment 84765 [details] [review] Bug 13895: (follow-up) Adapt checkout endpoint to openapi, update terminology
I added follow-up too ;) I made a migration to openapi and add some basic POD.
I am going to add a follow-up for tests with basic authentication
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>
Created attachment 85459 [details] [review] Bug 13895: (QA follow-up) Adjust OpenAPI definitions for checkouts Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 85460 [details] [review] Bug 13895: (follow-up) Adapt checkout endpoint to openapi, update terminology
Created attachment 85461 [details] [review] Bug 13895: Remove the opac checks in privileged endpoint Test plan: prove t/db_dependent/api/v1/checkouts.t
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
Created attachment 85463 [details] [review] Bug 13895: Refactor test using REST Basic auth Test plan: prove t/db_dependent/api/v1/checkouts.t
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
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
Created attachment 85676 [details] [review] Bug 13895: (follow-up) Fix POST response Test plan: prove t/db_dependent/api/v1/chcekouts.t
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>
Created attachment 86576 [details] [review] Bug 13895: Adapt naming according to voted RFC
Created attachment 86579 [details] [review] Bug 13895: Adapt naming according to voted RFC
Created attachment 86580 [details] [review] Bug 13895: Fix test
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>
Created attachment 87106 [details] [review] Bug 13895: (QA follow-up) Adjust OpenAPI definitions for checkouts Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Created attachment 87107 [details] [review] Bug 13895: (follow-up) Adapt checkout endpoint to openapi, update terminology
Created attachment 87108 [details] [review] Bug 13895: Remove the opac checks in privileged endpoint Test plan: prove t/db_dependent/api/v1/checkouts.t
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
Created attachment 87110 [details] [review] Bug 13895: Refactor test using REST Basic auth Test plan: prove t/db_dependent/api/v1/checkouts.t
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
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>
Created attachment 87113 [details] [review] Bug 13895: Adapt naming according to voted RFC
Created attachment 87114 [details] [review] Bug 13895: Fix test
Created attachment 87115 [details] [review] Bug 13895: (QA follow-up) Make the class plural Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Awesome work all! Pushed to master for 19.05
Created attachment 87200 [details] [review] Bug 13895: (follow-up) Add Modern::Perl
(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
As is my policy of backporting api routes, pushed to 18.11.x for 18.11.05
missing dependencies and wont be backporting to the 18.05.x series