From 1dfb57164fd6d986b110caa695a3baf9800ff93a Mon Sep 17 00:00:00 2001 From: Josef Moravec Date: Mon, 4 Feb 2019 14:22:54 +0000 Subject: [PATCH] Bug 13895: (follow-up) Adapt checkout endpoint to openapi, update terminology Signed-off-by: Nick Clemens --- Koha/REST/V1/Checkout.pm | 184 +++++++++++++++++++---- api/v1/swagger/definitions/checkout.json | 38 ++--- api/v1/swagger/paths/checkouts.json | 11 +- t/db_dependent/api/v1/checkouts.t | 67 +++++---- 4 files changed, 217 insertions(+), 83 deletions(-) diff --git a/Koha/REST/V1/Checkout.pm b/Koha/REST/V1/Checkout.pm index 8c898ac1d8..14a623b34a 100644 --- a/Koha/REST/V1/Checkout.pm +++ b/Koha/REST/V1/Checkout.pm @@ -15,8 +15,6 @@ package Koha::REST::V1::Checkout; # with Koha; if not, write to the Free Software Foundation, Inc., # 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA. -use Modern::Perl; - use Mojo::Base 'Mojolicious::Controller'; use C4::Auth qw( haspermission ); @@ -24,42 +22,84 @@ use C4::Context; use C4::Circulation; use Koha::Checkouts; -sub list { - my ($c, $args, $cb) = @_; +use Try::Tiny; + +=head1 NAME + +Koha::REST::V1::Checkout + +=head1 API - my $borrowernumber = $c->param('borrowernumber'); - my $checkouts = C4::Circulation::GetIssues({ - borrowernumber => $borrowernumber - }); +=head2 Methods - $c->$cb($checkouts, 200); +=head3 list + +List Koha::Checkout objects + +=cut + +sub list { + my $c = shift->openapi->valid_input or return; + try { + my $checkouts_set = Koha::Checkouts->new; + my $checkouts = $c->objects->search( $checkouts_set, \&_to_model, \&_to_api ); + return $c->render( status => 200, openapi => $checkouts ); + } catch { + if ( $_->isa('DBIx::Class::Exception') ) { + return $c->render( + status => 500, + openapi => { error => $_->{msg} } + ); + } else { + return $c->render( + status => 500, + openapi => { error => "Something went wrong, check the logs." } + ); + } + }; } +=head3 get + +get one checkout + +=cut + sub get { - my ($c, $args, $cb) = @_; + my $c = shift->openapi->valid_input or return; - my $checkout_id = $args->{checkout_id}; - my $checkout = Koha::Checkouts->find($checkout_id); + my $checkout = Koha::Checkouts->find( $c->validation->param('checkout_id') ); - if (!$checkout) { - return $c->$cb({ - error => "Checkout doesn't exist" - }, 404); + unless ($checkout) { + return $c->render( + status => 404, + openapi => { error => "Checkout doesn't exist" } + ); } - return $c->$cb($checkout->unblessed, 200); + return $c->render( + status => 200, + openapi => _to_api($checkout->TO_JSON) + ); } +=head3 renew + +Renew a checkout + +=cut + sub renew { - my ($c, $args, $cb) = @_; + my $c = shift->openapi->valid_input or return; - my $checkout_id = $args->{checkout_id}; - my $checkout = Koha::Checkouts->find($checkout_id); + my $checkout_id = $c->validation->param('checkout_id'); + my $checkout = Koha::Checkouts->find( $checkout_id ); - if (!$checkout) { - return $c->$cb({ - error => "Checkout doesn't exist" - }, 404); + unless ($checkout) { + return $c->render( + status => 404, + openapi => { error => "Checkout doesn't exist" } + ); } my $borrowernumber = $checkout->borrowernumber; @@ -69,7 +109,10 @@ sub renew { unless (C4::Context->preference('OpacRenewalAllowed')) { my $user = $c->stash('koha.user'); unless ($user && haspermission($user->userid, { circulate => "circulate_remaining_permissions" })) { - return $c->$cb({error => "Opac Renewal not allowed"}, 403); + return $c->render( + status => 403, + openapi => { error => "Opac Renewal not allowed"} + ); } } @@ -77,13 +120,100 @@ sub renew { $borrowernumber, $itemnumber); if (!$can_renew) { - return $c->$cb({error => "Renewal not authorized ($error)"}, 403); + return $c->render( + status => 403, + openapi => { error => "Renewal not authorized ($error)" } + ); } AddRenewal($borrowernumber, $itemnumber, $checkout->branchcode); $checkout = Koha::Checkouts->find($checkout_id); - return $c->$cb($checkout->unblessed, 200); + return $c->render( + status => 200, + openapi => _to_api( $checkout->TO_JSON ) + ); } +=head3 _to_api + +Helper function that maps a hashref of Koha::Checkout attributes into REST api +attribute names. + +=cut + +sub _to_api { + my $checkout = shift; + + foreach my $column ( keys %{ $Koha::REST::V1::Checkout::to_api_mapping } ) { + my $mapped_column = $Koha::REST::V1::Checkout::to_api_mapping->{$column}; + if ( exists $checkout->{ $column } && defined $mapped_column ) + { + $checkout->{ $mapped_column } = delete $checkout->{ $column }; + } + elsif ( exists $checkout->{ $column } && !defined $mapped_column ) { + delete $checkout->{ $column }; + } + } + return $checkout; +} + +=head3 _to_model + +Helper function that maps REST api objects into Koha::Checkouts +attribute names. + +=cut + +sub _to_model { + my $checkout = shift; + + foreach my $attribute ( keys %{ $Koha::REST::V1::Checkout::to_model_mapping } ) { + my $mapped_attribute = $Koha::REST::V1::Checkout::to_model_mapping->{$attribute}; + if ( exists $checkout->{ $attribute } && defined $mapped_attribute ) + { + $checkout->{ $mapped_attribute } = delete $checkout->{ $attribute }; + } + elsif ( exists $checkout->{ $attribute } && !defined $mapped_attribute ) + { + delete $checkout->{ $attribute }; + } + } + return $checkout; +} + +=head2 Global variables + +=head3 $to_api_mapping + +=cut + +our $to_api_mapping = { + issue_id => 'checkout_id', + borrowernumber => 'patron_id', + itemnumber => 'item_id', + date_due => 'due_date', + branchcode => 'library_id', + returndate => 'checkin_date', + lastreneweddate => 'last_renewed_date', + issuedate => 'checked_out_date', + notedate => 'note_date', +}; + +=head3 $to_model_mapping + +=cut + +our $to_model_mapping = { + checkout_id => 'issue_id', + patron_id => 'borrowernumber', + item_id => 'itemnumber', + due_date => 'date_due', + library_id => 'branchcode', + checkin_date => 'returndate', + last_renewed_date => 'lastreneweddate', + checked_out_date => 'issuedate', + note_date => 'notedate', +}; + 1; diff --git a/api/v1/swagger/definitions/checkout.json b/api/v1/swagger/definitions/checkout.json index 4267eba24c..e540a55583 100644 --- a/api/v1/swagger/definitions/checkout.json +++ b/api/v1/swagger/definitions/checkout.json @@ -1,34 +1,34 @@ { "type": "object", "properties": { - "issue_id": { - "type": "string", + "checkout_id": { + "type": "integer", "description": "internally assigned checkout identifier" }, - "borrowernumber": { - "$ref": "../x-primitives.json#/borrowernumber" + "patron_id": { + "$ref": "../x-primitives.json#/patron_id" }, - "itemnumber": { - "$ref": "../x-primitives.json#/itemnumber" + "item_id": { + "type": "integer", + "description": "internal identifier of checked out item" }, - "date_due": { + "due_date": { "type": "string", + "format": "date-time", "description": "Due date" }, - "branchcode": { - "type": "string", + "library_id": { + "type": ["string", "null"], "description": "code of the library the item was checked out" }, - "issuingbranch": { - "type": "string", - "description": "Code of the branch where issue was made" - }, - "returndate": { + "checkin_date": { "type": ["string", "null"], + "format": "date", "description": "Date the item was returned" }, - "lastreneweddate": { + "last_renewed_date": { "type": ["string", "null"], + "format": "date-time", "description": "Date the item was last renewed" }, "renewals": { @@ -43,8 +43,9 @@ "type": "string", "description": "Last update time" }, - "issuedate": { - "type": ["string", "null"], + "checked_out_date": { + "type": "string", + "format": "date-time", "description": "Date the item was issued" }, "onsite_checkout": { @@ -55,8 +56,9 @@ "type": ["string", "null"], "description": "Issue note text" }, - "notedate": { + "note_date": { "type": ["string", "null"], + "format": "date", "description": "Datetime of the issue note" } } diff --git a/api/v1/swagger/paths/checkouts.json b/api/v1/swagger/paths/checkouts.json index e49a33ef80..54aadd9fc5 100644 --- a/api/v1/swagger/paths/checkouts.json +++ b/api/v1/swagger/paths/checkouts.json @@ -1,10 +1,11 @@ { "/checkouts": { "get": { + "x-mojo-to": "Checkout#list", "operationId": "listCheckouts", "tags": ["patrons", "checkouts"], "parameters": [{ - "$ref": "../parameters.json#/borrowernumberQueryParam" + "$ref": "../parameters.json#/patron_id_qp" }], "produces": [ "application/json" @@ -26,8 +27,6 @@ } }, "x-koha-authorization": { - "allow-owner": true, - "allow-guarantor": true, "permissions": { "circulate": "circulate_remaining_permissions" } @@ -36,6 +35,7 @@ }, "/checkouts/{checkout_id}": { "get": { + "x-mojo-to": "Checkout#get", "operationId": "getCheckout", "tags": ["patrons", "checkouts"], "parameters": [{ @@ -57,14 +57,13 @@ } }, "x-koha-authorization": { - "allow-owner": true, - "allow-guarantor": true, "permissions": { "circulate": "circulate_remaining_permissions" } } }, "put": { + "x-mojo-to": "Checkout#renew", "operationId": "renewCheckout", "tags": ["patrons", "checkouts"], "parameters": [{ @@ -86,8 +85,6 @@ } }, "x-koha-authorization": { - "allow-owner": true, - "allow-guarantor": true, "permissions": { "circulate": "circulate_remaining_permissions" } diff --git a/t/db_dependent/api/v1/checkouts.t b/t/db_dependent/api/v1/checkouts.t index 5298d7fcd4..141beb4b05 100644 --- a/t/db_dependent/api/v1/checkouts.t +++ b/t/db_dependent/api/v1/checkouts.t @@ -32,16 +32,21 @@ use C4::Circulation; use C4::Items; use Koha::Database; +use Koha::DateUtils; use Koha::Patron; my $schema = Koha::Database->schema; -$schema->storage->txn_begin; -my $dbh = C4::Context->dbh; my $builder = t::lib::TestBuilder->new; -$dbh->{RaiseError} = 1; + +$schema->storage->txn_begin; + +t::lib::Mocks::mock_preference( 'SessionStorage', 'tmp' ); $ENV{REMOTE_ADDR} = '127.0.0.1'; my $t = Test::Mojo->new('Koha::REST::V1'); +my $tx; + +my $dbh = C4::Context->dbh; $dbh->do('DELETE FROM issues'); $dbh->do('DELETE FROM items'); @@ -73,14 +78,14 @@ my $branchcode = $builder->build({ source => 'Branch' })->{ branchcode }; my $module = new Test::MockModule('C4::Context'); $module->mock('userenv', sub { { branch => $branchcode } }); -my $tx = $t->ua->build_tx(GET => "/api/v1/checkouts?borrowernumber=$borrowernumber"); +$tx = $t->ua->build_tx(GET => "/api/v1/checkouts?patron_id=$borrowernumber"); $tx->req->cookies({name => 'CGISESSID', value => $session->id}); $t->request_ok($tx) ->status_is(200) ->json_is([]); my $notexisting_borrowernumber = $borrowernumber + 1; -$tx = $t->ua->build_tx(GET => "/api/v1/checkouts?borrowernumber=$notexisting_borrowernumber"); +$tx = $t->ua->build_tx(GET => "/api/v1/checkouts?patron_id=$notexisting_borrowernumber"); $tx->req->cookies({name => 'CGISESSID', value => $session->id}); $t->request_ok($tx) ->status_is(200) @@ -99,16 +104,16 @@ my $date_due2 = Koha::DateUtils::dt_from_string( $issue2->date_due ); my $issue3 = C4::Circulation::AddIssue($loggedinuser, 'TEST000003', $date_due); my $date_due3 = Koha::DateUtils::dt_from_string( $issue3->date_due ); -$tx = $t->ua->build_tx(GET => "/api/v1/checkouts?borrowernumber=$borrowernumber"); +$tx = $t->ua->build_tx(GET => "/api/v1/checkouts?patron_id=$borrowernumber"); $tx->req->cookies({name => 'CGISESSID', value => $session->id}); $t->request_ok($tx) ->status_is(200) - ->json_is('/0/borrowernumber' => $borrowernumber) - ->json_is('/0/itemnumber' => $itemnumber1) - ->json_is('/0/date_due' => $date_due1->ymd . ' ' . $date_due1->hms) - ->json_is('/1/borrowernumber' => $borrowernumber) - ->json_is('/1/itemnumber' => $itemnumber2) - ->json_is('/1/date_due' => $date_due2->ymd . ' ' . $date_due2->hms) + ->json_is('/0/patron_id' => $borrowernumber) + ->json_is('/0/item_id' => $itemnumber1) + ->json_is('/0/due_date' => output_pref({ dateformat => "rfc3339", dt => $date_due1 }) ) + ->json_is('/1/patron_id' => $borrowernumber) + ->json_is('/1/item_id' => $itemnumber2) + ->json_is('/1/due_date' => output_pref({ dateformat => "rfc3339", dt => $date_due2 }) ) ->json_hasnt('/2'); $tx = $t->ua->build_tx(GET => "/api/v1/checkouts/".$issue3->issue_id); @@ -119,7 +124,7 @@ $t->request_ok($tx) required_permissions => { circulate => "circulate_remaining_permissions" } }); -$tx = $t->ua->build_tx(GET => "/api/v1/checkouts?borrowernumber=".$loggedinuser->{borrowernumber}); +$tx = $t->ua->build_tx(GET => "/api/v1/checkouts?patron_id=".$loggedinuser->{borrowernumber}); $tx->req->cookies({name => 'CGISESSID', value => $patron_session->id}); $t->request_ok($tx) ->status_is(403) @@ -127,38 +132,38 @@ $t->request_ok($tx) required_permissions => { circulate => "circulate_remaining_permissions" } }); -$tx = $t->ua->build_tx(GET => "/api/v1/checkouts?borrowernumber=$borrowernumber"); -$tx->req->cookies({name => 'CGISESSID', value => $patron_session->id}); +$tx = $t->ua->build_tx(GET => "/api/v1/checkouts?patron_id=$borrowernumber"); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); $t->request_ok($tx) ->status_is(200) - ->json_is('/0/borrowernumber' => $borrowernumber) - ->json_is('/0/itemnumber' => $itemnumber1) - ->json_is('/0/date_due' => $date_due1->ymd . ' ' . $date_due1->hms) - ->json_is('/1/borrowernumber' => $borrowernumber) - ->json_is('/1/itemnumber' => $itemnumber2) - ->json_is('/1/date_due' => $date_due2->ymd . ' ' . $date_due2->hms) + ->json_is('/0/patron_id' => $borrowernumber) + ->json_is('/0/item_id' => $itemnumber1) + ->json_is('/0/due_date' => output_pref({ dateformat => "rfc3339", dt => $date_due1 }) ) + ->json_is('/1/patron_id' => $borrowernumber) + ->json_is('/1/item_id' => $itemnumber2) + ->json_is('/1/due_date' => output_pref({ dateformat => "rfc3339", dt => $date_due2 }) ) ->json_hasnt('/2'); $tx = $t->ua->build_tx(GET => "/api/v1/checkouts/" . $issue1->issue_id); -$tx->req->cookies({name => 'CGISESSID', value => $patron_session->id}); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); $t->request_ok($tx) ->status_is(200) - ->json_is('/borrowernumber' => $borrowernumber) - ->json_is('/itemnumber' => $itemnumber1) - ->json_is('/date_due' => $date_due1->ymd . ' ' . $date_due1->hms) + ->json_is('/patron_id' => $borrowernumber) + ->json_is('/item_id' => $itemnumber1) + ->json_is('/due_date' => output_pref({ dateformat => "rfc3339", dt => $date_due1 }) ) ->json_hasnt('/1'); $tx = $t->ua->build_tx(GET => "/api/v1/checkouts/" . $issue1->issue_id); $tx->req->cookies({name => 'CGISESSID', value => $session->id}); $t->request_ok($tx) ->status_is(200) - ->json_is('/date_due' => $date_due1->ymd . ' ' . $date_due1->hms); + ->json_is('/due_date' => output_pref({ dateformat => "rfc3339", dt => $date_due1 }) ); $tx = $t->ua->build_tx(GET => "/api/v1/checkouts/" . $issue2->issue_id); $tx->req->cookies({name => 'CGISESSID', value => $session->id}); $t->request_ok($tx) ->status_is(200) - ->json_is('/date_due' => $date_due2->ymd . ' ' . $date_due2->hms); + ->json_is('/due_date' => output_pref( { dateformat => "rfc3339", dt => $date_due2 }) ); $dbh->do('DELETE FROM issuingrules'); @@ -172,7 +177,7 @@ $tx = $t->ua->build_tx(PUT => "/api/v1/checkouts/" . $issue1->issue_id); $tx->req->cookies({name => 'CGISESSID', value => $session->id}); $t->request_ok($tx) ->status_is(200) - ->json_is('/date_due' => $expected_datedue->ymd . ' ' . $expected_datedue->hms); + ->json_is('/due_date' => output_pref( { dateformat => "rfc3339", dt => $expected_datedue }) ); $tx = $t->ua->build_tx(PUT => "/api/v1/checkouts/" . $issue3->issue_id); $tx->req->cookies({name => 'CGISESSID', value => $patron_session->id}); @@ -187,14 +192,14 @@ $tx = $t->ua->build_tx(PUT => "/api/v1/checkouts/" . $issue2->issue_id); $tx->req->cookies({name => 'CGISESSID', value => $patron_session->id}); $t->request_ok($tx) ->status_is(403) - ->json_is({ error => "Opac Renewal not allowed" }); + ->json_is({ error => "Opac Renewal not allowed" }); t::lib::Mocks::mock_preference( "OpacRenewalAllowed", 1 ); $tx = $t->ua->build_tx(PUT => "/api/v1/checkouts/" . $issue2->issue_id); -$tx->req->cookies({name => 'CGISESSID', value => $patron_session->id}); +$tx->req->cookies({name => 'CGISESSID', value => $session->id}); $t->request_ok($tx) ->status_is(200) - ->json_is('/date_due' => $expected_datedue->ymd . ' ' . $expected_datedue->hms); + ->json_is('/due_date' => output_pref({ dateformat => "rfc3339", dt => $expected_datedue}) ); $tx = $t->ua->build_tx(PUT => "/api/v1/checkouts/" . $issue1->issue_id); $tx->req->cookies({name => 'CGISESSID', value => $session->id}); -- 2.39.5