From dd9b6c165163ac1d6b52df6f2df17ec5de6b2758 Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Mon, 4 Dec 2017 16:18:47 -0300 Subject: [PATCH] Bug 16330: Move patches to OpenAPI This patch refactors the original work so it implements the controllers and the spec using Mojolicious::Plugin::OpenAPI, and OpenAPI for the specification. It removes the ability for patrons without permissions to edit their own data or their guarantee's. This will be moved to a patron modification requests endpoint for simplicity. It makes use of bugs 19410 and 19686 and their dependencies to deal with parameters handling, query building and pagination. Tests are adapted. To test: - Apply this patches and the dependencies - Run: $ kshell k$ prove t/db_dependent/api/v1/patrons.t => SUCCESS: Tests pass! - Sign off :-D Sponsored-by: ByWater Solutions Signed-off-by: Tomas Cohen Arazi Signed-off-by: Benjamin Rokseth Signed-off-by: Josef Moravec Signed-off-by: Jonathan Druart --- Koha/REST/V1/Patron.pm | 271 ++++++++++++++------- api/v1/swagger/paths/patrons.json | 389 +++++++++++++----------------- t/db_dependent/api/v1/patrons.t | 107 +++----- 3 files changed, 373 insertions(+), 394 deletions(-) diff --git a/Koha/REST/V1/Patron.pm b/Koha/REST/V1/Patron.pm index 763b325179..52ffab3bbb 100644 --- a/Koha/REST/V1/Patron.pm +++ b/Koha/REST/V1/Patron.pm @@ -20,39 +20,53 @@ use Modern::Perl; use Mojo::Base 'Mojolicious::Controller'; use C4::Members qw( AddMember ModMember ); -use Koha::AuthUtils qw(hash_password); use Koha::Patrons; -use Koha::Patron::Categories; -use Koha::Patron::Modifications; -use Koha::Libraries; use Scalar::Util qw(blessed); use Try::Tiny; -sub list { - my $c = shift->openapi->valid_input or return; +=head1 NAME +Koha::REST::V1::Patron - my $args = $c->req->params->to_hash; - my $filter = {}; - for my $filter_param ( keys %$args ) { - $filter->{$filter_param} = { LIKE => $args->{$filter_param} . "%" }; - } +=head1 API + +=head2 Methods + +=head3 list + +Controller function that handles listing Koha::Patron objects + +=cut + +sub list { + my $c = shift->openapi->valid_input or return; return try { - my $patrons = Koha::Patrons->search_limited($filter); - return $c->render(status => 200, openapi => $patrons); + my $patrons_set = Koha::Patrons->new; + my @patrons = $c->objects->search( $patrons_set )->as_list; + return $c->render( status => 200, openapi => \@patrons ); } catch { if ( $_->isa('DBIx::Class::Exception') ) { - return $c->render( status => 500, openapi => { error => $_->{msg} } ); + return $c->render( status => 500, + openapi => { error => $_->{msg} } ); } else { - return $c->render( status => 500, openapi => { error => "Something went wrong, check the logs." } ); + return $c->render( + status => 500, + openapi => { error => "Something went wrong, check the logs." } + ); } }; } +=head3 get + +Controller function that handles retrieving a single Koha::Patron object + +=cut + sub get { my $c = shift->openapi->valid_input or return; @@ -66,140 +80,213 @@ sub get { return $c->render(status => 200, openapi => $patron); } +=head3 add + +Controller function that handles adding a new Koha::Patron object + +=cut + sub add { - my ($c, $args, $cb) = @_; + my $c = shift->openapi->valid_input or return; return try { - my $body = $c->req->json; + my $body = $c->validation->param('body'); Koha::Patron->new($body)->_validate; + # TODO: Use AddMember until it has been moved to Koha-namespace my $borrowernumber = AddMember(%$body); - my $patron = Koha::Patrons->find($borrowernumber); + my $patron = Koha::Patrons->find($borrowernumber); - return $c->$cb($patron, 201); + return $c->render( status => 201, openapi => $patron ); } catch { - unless (blessed $_ && $_->can('rethrow')) { - return $c->$cb({ error => - "Something went wrong, check Koha logs for details."}, 500); + unless ( blessed $_ && $_->can('rethrow') ) { + return $c->render( + status => 500, + openapi => { + error => "Something went wrong, check Koha logs for details." + } + ); } - if ($_->isa('Koha::Exceptions::Patron::DuplicateObject')) { - return $c->$cb({ error => $_->error, conflict => $_->conflict }, 409); + if ( $_->isa('Koha::Exceptions::Patron::DuplicateObject') ) { + return $c->render( + status => 409, + openapi => { error => $_->error, conflict => $_->conflict } + ); } - elsif ($_->isa('Koha::Exceptions::Library::BranchcodeNotFound')) { - return $c->$cb({ error => "Given branchcode does not exist" }, 400); + elsif ( $_->isa('Koha::Exceptions::Library::BranchcodeNotFound') ) { + return $c->render( + status => 400, + openapi => { error => "Given branchcode does not exist" } + ); } - elsif ($_->isa('Koha::Exceptions::Category::CategorycodeNotFound')) { - return $c->$cb({ error => "Given categorycode does not exist"}, 400); + elsif ( $_->isa('Koha::Exceptions::Category::CategorycodeNotFound') ) { + return $c->render( + status => 400, + openapi => { error => "Given categorycode does not exist" } + ); } else { - return $c->$cb({ error => - "Something went wrong, check Koha logs for details."}, 500); + return $c->render( + status => 500, + openapi => { + error => "Something went wrong, check Koha logs for details." + } + ); } }; } -sub edit { - my ($c, $args, $cb) = @_; +=head3 update + +Controller function that handles updating a Koha::Patron object + +=cut + +sub update { + my $c = shift->openapi->valid_input or return; + + my $patron = Koha::Patrons->find( $c->validation->param('borrowernumber') ); - my $patron; return try { - my $user = $c->stash('koha.user'); - $patron = Koha::Patrons->find($args->{borrowernumber}); - my $body = $c->req->json; - - $body->{borrowernumber} = $args->{borrowernumber}; - - if (!C4::Auth::haspermission($user->userid, { borrowers => 1 }) && - $user->borrowernumber == $patron->borrowernumber){ - if (C4::Context->preference('OPACPatronDetails')) { - $body = _delete_unmodifiable_parameters($body); - die unless $patron->set($body)->_validate; - my $m = Koha::Patron::Modification->new($body)->store(); - return $c->$cb({}, 202); - } else { - return $c->$cb({ error => "You need a permission to change" - ." Your personal details"}, 403); - } + my $body = $c->validation->param('body'); + + $patron->set( _to_model($body) )->_validate; + + # TODO: Use ModMember until it has been moved to Koha-namespace + if ( ModMember(%$body) ) { + return $c->render( status => 200, openapi => $patron ); } else { - delete $body->{borrowernumber}; - die unless $patron->set($body)->_validate; - # TODO: Use ModMember until it has been moved to Koha-namespace - $body->{borrowernumber} = $args->{borrowernumber}; - die unless ModMember(%$body); - return $c->$cb($patron, 200); + return $c->render( + status => 500, + openapi => { + error => 'Something went wrong, check Koha logs for details.' + } + ); } } catch { unless ($patron) { - return $c->$cb({error => "Patron not found"}, 404); + return $c->render( + status => 404, + openapi => { error => "Patron not found" } + ); } - unless (blessed $_ && $_->can('rethrow')) { - return $c->$cb({ error => - "Something went wrong, check Koha logs for details."}, 500); + unless ( blessed $_ && $_->can('rethrow') ) { + return $c->render( + status => 500, + openapi => { + error => "Something went wrong, check Koha logs for details." + } + ); } - if ($_->isa('Koha::Exceptions::Patron::DuplicateObject')) { - return $c->$cb({ error => $_->error, conflict => $_->conflict }, 409); + if ( $_->isa('Koha::Exceptions::Patron::DuplicateObject') ) { + return $c->render( + status => 409, + openapi => { error => $_->error, conflict => $_->conflict } + ); } - elsif ($_->isa('Koha::Exceptions::Library::BranchcodeNotFound')) { - return $c->$cb({ error => "Given branchcode does not exist" }, 400); + elsif ( $_->isa('Koha::Exceptions::Library::BranchcodeNotFound') ) { + return $c->render( + status => 400, + openapi => { error => "Given branchcode does not exist" } + ); } - elsif ($_->isa('Koha::Exceptions::Category::CategorycodeNotFound')) { - return $c->$cb({ error => "Given categorycode does not exist"}, 400); + elsif ( $_->isa('Koha::Exceptions::Category::CategorycodeNotFound') ) { + return $c->render( + status => 400, + openapi => { error => "Given categorycode does not exist" } + ); } - elsif ($_->isa('Koha::Exceptions::MissingParameter')) { - return $c->$cb({error => "Missing mandatory parameter(s)", - parameters => $_->parameter }, 400); + elsif ( $_->isa('Koha::Exceptions::MissingParameter') ) { + return $c->render( + status => 400, + openapi => { + error => "Missing mandatory parameter(s)", + parameters => $_->parameter + } + ); } - elsif ($_->isa('Koha::Exceptions::BadParameter')) { - return $c->$cb({error => "Invalid parameter(s)", - parameters => $_->parameter }, 400); + elsif ( $_->isa('Koha::Exceptions::BadParameter') ) { + return $c->render( + status => 400, + openapi => { + error => "Invalid parameter(s)", + parameters => $_->parameter + } + ); } - elsif ($_->isa('Koha::Exceptions::NoChanges')) { - return $c->$cb({error => "No changes have been made"}, 204); + elsif ( $_->isa('Koha::Exceptions::NoChanges') ) { + return $c->render( + status => 204, + openapi => { error => "No changes have been made" } + ); } else { - return $c->$cb({ error => - "Something went wrong, check Koha logs for details."}, 500); + return $c->render( + status => 500, + openapi => { + error => + "Something went wrong, check Koha logs for details." + } + ); } }; } +=head3 delete + +Controller function that handles deleting a Koha::Patron object + +=cut + sub delete { - my ($c, $args, $cb) = @_; + my $c = shift->openapi->valid_input or return; my $patron; return try { - $patron = Koha::Patrons->find($args->{borrowernumber}); + $patron = Koha::Patrons->find( $c->validation->param('borrowernumber') ); + # check if loans, reservations, debarrment, etc. before deletion! my $res = $patron->delete; - - return $c->$cb({}, 200); + return $c->render( status => 200, openapi => {} ); } catch { unless ($patron) { - return $c->$cb({error => "Patron not found"}, 404); + return $c->render( + status => 404, + openapi => { error => "Patron not found" } + ); } else { - return $c->$cb({ error => - "Something went wrong, check Koha logs for details."}, 500); + return $c->render( + status => 500, + openapi => { + error => + "Something went wrong, check Koha logs for details." + } + ); } }; } -sub _delete_unmodifiable_parameters { - my ($body) = @_; +=head3 _to_model - my %columns = map { $_ => 1 } Koha::Patron::Modifications->columns; - foreach my $param (keys %$body) { - unless (exists $columns{$param}) { - delete $body->{$param}; - } - } - return $body; +Helper function that maps REST api objects into Koha::Patron +attribute names. + +=cut + +sub _to_model { + my $params = shift; + + $params->{lost} = ($params->{lost}) ? 1 : 0; + $params->{gonenoaddress} = ($params->{gonenoaddress}) ? 1 : 0; + + return $params; } 1; diff --git a/api/v1/swagger/paths/patrons.json b/api/v1/swagger/paths/patrons.json index 56218a42ea..5e479c8ab2 100644 --- a/api/v1/swagger/paths/patrons.json +++ b/api/v1/swagger/paths/patrons.json @@ -10,513 +10,449 @@ "parameters": [{ "name": "borrowernumber", "in": "query", - "description": "Case insensetive 'starts_with' search on borrowernumber", + "description": "Case insensitive search on borrowernumber", "required": false, "type": "string" - }, - { + }, { "name": "cardnumber", "in": "query", - "description": "Case insensetive 'starts_with' search on cardnumber", + "description": "Case insensitive search on cardnumber", "required": false, "type": "string" - }, - { + }, { "name": "surname", "in": "query", - "description": "Case insensetive 'starts_with' search on surname", + "description": "Case insensitive search on surname", "required": false, "type": "string" - }, - { + }, { "name": "firstname", "in": "query", - "description": "Case insensetive 'starts_with' search on firstname", + "description": "Case insensitive search on firstname", "required": false, "type": "string" - }, - { + }, { "name": "title", "in": "query", - "description": "Case insensetive 'starts_with' search on title", + "description": "Case insensitive search on title", "required": false, "type": "string" - }, - { + }, { "name": "othernames", "in": "query", - "description": "Case insensetive 'starts_with' search on othernames", + "description": "Case insensitive search on othernames", "required": false, "type": "string" - }, - { + }, { "name": "initials", "in": "query", - "description": "Case insensetive 'starts_with' search on initials", + "description": "Case insensitive search on initials", "required": false, "type": "string" - }, - { + }, { "name": "streetnumber", "in": "query", - "description": "Case insensetive 'starts_with' search on streetnumber", + "description": "Case insensitive search on streetnumber", "required": false, "type": "string" - }, - { + }, { "name": "streettype", "in": "query", - "description": "Case insensetive 'starts_with' search on streettype", + "description": "Case insensitive search on streettype", "required": false, "type": "string" - }, - { + }, { "name": "address", "in": "query", - "description": "Case insensetive 'starts_with' search on address", + "description": "Case insensitive search on address", "required": false, "type": "string" - }, - { + }, { "name": "address2", "in": "query", - "description": "Case insensetive 'starts_with' search on address2", + "description": "Case insensitive search on address2", "required": false, "type": "string" - }, - { + }, { "name": "city", "in": "query", - "description": "Case insensetive 'starts_with' search on city", + "description": "Case insensitive search on city", "required": false, "type": "string" - }, - { + }, { "name": "state", "in": "query", - "description": "Case insensetive 'starts_with' search on state", + "description": "Case insensitive search on state", "required": false, "type": "string" - }, - { + }, { "name": "zipcode", "in": "query", - "description": "Case insensetive 'starts_with' search on zipcode", + "description": "Case insensitive search on zipcode", "required": false, "type": "string" - }, - { + }, { "name": "country", "in": "query", - "description": "Case insensetive 'starts_with' search on country", + "description": "Case insensitive search on country", "required": false, "type": "string" - }, - { + }, { "name": "email", "in": "query", - "description": "Case insensetive 'starts_with' search on email", + "description": "Case insensitive search on email", "required": false, "type": "string" - }, - { + }, { "name": "phone", "in": "query", - "description": "Case insensetive 'starts_with' search on phone", + "description": "Case insensitive search on phone", "required": false, "type": "string" - }, - { + }, { "name": "mobile", "in": "query", - "description": "Case insensetive 'starts_with' search on mobile", + "description": "Case insensitive search on mobile", "required": false, "type": "string" - }, - { + }, { "name": "fax", "in": "query", - "description": "Case insensetive 'starts_with' search on fax", + "description": "Case insensitive search on fax", "required": false, "type": "string" - }, - { + }, { "name": "emailpro", "in": "query", - "description": "Case insensetive 'starts_with' search on emailpro", + "description": "Case insensitive search on emailpro", "required": false, "type": "string" - }, - { + }, { "name": "phonepro", "in": "query", - "description": "Case insensetive 'starts_with' search on phonepro", + "description": "Case insensitive search on phonepro", "required": false, "type": "string" - }, - { + }, { "name": "B_streetnumber", "in": "query", - "description": "Case insensetive 'starts_with' search on B_streetnumber", + "description": "Case insensitive search on B_streetnumber", "required": false, "type": "string" - }, - { + }, { "name": "B_streettype", "in": "query", - "description": "Case insensetive 'starts_with' search on B_streettype", + "description": "Case insensitive search on B_streettype", "required": false, "type": "string" - }, - { + }, { "name": "B_address", "in": "query", - "description": "Case insensetive 'starts_with' search on B_address", + "description": "Case insensitive search on B_address", "required": false, "type": "string" - }, - { + }, { "name": "B_address2", "in": "query", - "description": "Case insensetive 'starts_with' search on B_address2", + "description": "Case insensitive search on B_address2", "required": false, "type": "string" - }, - { + }, { "name": "B_city", "in": "query", - "description": "Case insensetive 'starts_with' search on B_city", + "description": "Case insensitive search on B_city", "required": false, "type": "string" - }, - { + }, { "name": "B_state", "in": "query", - "description": "Case insensetive 'starts_with' search on B_state", + "description": "Case insensitive search on B_state", "required": false, "type": "string" - }, - { + }, { "name": "B_zipcode", "in": "query", - "description": "Case insensetive 'starts_with' search on B_zipcode", + "description": "Case insensitive search on B_zipcode", "required": false, "type": "string" - }, - { + }, { "name": "B_country", "in": "query", - "description": "Case insensetive 'starts_with' search on B_country", + "description": "Case insensitive search on B_country", "required": false, "type": "string" - }, - { + }, { "name": "B_email", "in": "query", - "description": "Case insensetive 'starts_with' search on B_email", + "description": "Case insensitive search on B_email", "required": false, "type": "string" - }, - { + }, { "name": "B_phone", "in": "query", - "description": "Case insensetive 'starts_with' search on B_phone", + "description": "Case insensitive search on B_phone", "required": false, "type": "string" - }, - { + }, { "name": "dateofbirth", "in": "query", - "description": "Case insensetive 'starts_with' search on dateofbirth", + "description": "Case insensitive search on dateofbirth", "required": false, "type": "string" - }, - { + }, { "name": "branchcode", "in": "query", - "description": "Case insensetive 'starts_with' search on branchcode", + "description": "Case insensitive search on branchcode", "required": false, "type": "string" - }, - { + }, { "name": "categorycode", "in": "query", - "description": "Case insensetive 'starts_with' search on categorycode", + "description": "Case insensitive search on categorycode", "required": false, "type": "string" - }, - { + }, { "name": "dateenrolled", "in": "query", - "description": "Case insensetive 'starts_with' search on dateenrolled", + "description": "Case insensitive search on dateenrolled", "required": false, "type": "string" - }, - { + }, { "name": "dateexpiry", "in": "query", - "description": "Case insensetive 'starts_with' search on dateexpiry", + "description": "Case insensitive search on dateexpiry", "required": false, "type": "string" - }, - { + }, { "name": "gonenoaddress", "in": "query", - "description": "Case insensetive 'starts_with' search on gonenoaddress", + "description": "Search on gonenoaddress", "required": false, - "type": "string" - }, - { + "type": "boolean" + }, { "name": "lost", "in": "query", - "description": "Case insensetive 'starts_with' search on lost", + "description": "Search on lost", "required": false, - "type": "string" - }, - { + "type": "boolean" + }, { "name": "debarred", "in": "query", - "description": "Case insensetive 'starts_with' search on debarred", + "description": "Case insensitive search on debarred", "required": false, "type": "string" - }, - { + }, { "name": "debarredcomment", "in": "query", - "description": "Case insensetive 'starts_with' search on debarredcomment", + "description": "Case insensitive search on debarredcomment", "required": false, "type": "string" - }, - { + }, { "name": "contactname", "in": "query", - "description": "Case insensetive 'starts_with' search on contactname", + "description": "Case insensitive search on contactname", "required": false, "type": "string" - }, - { + }, { "name": "contactfirstname", "in": "query", - "description": "Case insensetive 'starts_with' search on contactfirstname", + "description": "Case insensitive search on contactfirstname", "required": false, "type": "string" - }, - { + }, { "name": "contacttitle", "in": "query", - "description": "Case insensetive 'starts_with' search on contacttitle", + "description": "Case insensitive search on contacttitle", "required": false, "type": "string" - }, - { + }, { "name": "guarantorid", "in": "query", - "description": "Case insensetive 'starts_with' search on guarantorid", + "description": "Case insensitive search on guarantorid", "required": false, "type": "string" - }, - { + }, { "name": "borrowernotes", "in": "query", - "description": "Case insensetive 'starts_with' search on borrowernotes", + "description": "Case insensitive search on borrowernotes", "required": false, "type": "string" - }, - { + }, { "name": "relationship", "in": "query", - "description": "Case insensetive 'starts_with' search on relationship", + "description": "Case insensitive search on relationship", "required": false, "type": "string" - }, - { + }, { "name": "sex", "in": "query", - "description": "Case insensetive 'starts_with' search on sex", + "description": "Case insensitive search on sex", "required": false, "type": "string" - }, - { + }, { "name": "password", "in": "query", - "description": "Case insensetive 'starts_with' search on password", + "description": "Case insensitive search on password", "required": false, "type": "string" - }, - { + }, { "name": "flags", "in": "query", - "description": "Case insensetive 'starts_with' search on flags", + "description": "Case insensitive search on flags", "required": false, "type": "string" - }, - { + }, { "name": "userid", "in": "query", - "description": "Case insensetive 'starts_with' search on userid", + "description": "Case insensitive search on userid", "required": false, "type": "string" - }, - { + }, { "name": "opacnote", "in": "query", - "description": "Case insensetive 'starts_with' search on opacnote", + "description": "Case insensitive search on opacnote", "required": false, "type": "string" - }, - { + }, { "name": "contactnote", "in": "query", - "description": "Case insensetive 'starts_with' search on contactnote", + "description": "Case insensitive search on contactnote", "required": false, "type": "string" - }, - { + }, { "name": "sort1", "in": "query", - "description": "Case insensetive 'starts_with' search on sort1", + "description": "Case insensitive search on sort1", "required": false, "type": "string" - }, - { + }, { "name": "sort2", "in": "query", - "description": "Case insensetive 'starts_with' search on sort2", + "description": "Case insensitive search on sort2", "required": false, "type": "string" - }, - { + }, { "name": "altcontactfirstname", "in": "query", - "description": "Case insensetive 'starts_with' search on altcontactfirstname", + "description": "Case insensitive search on altcontactfirstname", "required": false, "type": "string" - }, - { + }, { "name": "altcontactsurname", "in": "query", - "description": "Case insensetive 'starts_with' search on altcontactsurname", + "description": "Case insensitive search on altcontactsurname", "required": false, "type": "string" - }, - { + }, { "name": "altcontactaddress1", "in": "query", - "description": "Case insensetive 'starts_with' search on altcontactaddress1", + "description": "Case insensitive search on altcontactaddress1", "required": false, "type": "string" - }, - { + }, { "name": "altcontactaddress2", "in": "query", - "description": "Case insensetive 'starts_with' search on altcontactaddress2", + "description": "Case insensitive search on altcontactaddress2", "required": false, "type": "string" - }, - { + }, { "name": "altcontactaddress3", "in": "query", - "description": "Case insensetive 'starts_with' search on altcontactaddress3", + "description": "Case insensitive search on altcontactaddress3", "required": false, "type": "string" - }, - { + }, { "name": "altcontactstate", "in": "query", - "description": "Case insensetive 'starts_with' search on altcontactstate", + "description": "Case insensitive search on altcontactstate", "required": false, "type": "string" - }, - { + }, { "name": "altcontactzipcode", "in": "query", - "description": "Case insensetive 'starts_with' search on altcontactzipcode", + "description": "Case insensitive search on altcontactzipcode", "required": false, "type": "string" - }, - { + }, { "name": "altcontactcountry", "in": "query", - "description": "Case insensetive 'starts_with' search on altcontactcountry", + "description": "Case insensitive search on altcontactcountry", "required": false, "type": "string" - }, - { + }, { "name": "altcontactphone", "in": "query", - "description": "Case insensetive 'starts_with' search on altcontactphone", + "description": "Case insensitive search on altcontactphone", "required": false, "type": "string" - }, - { + }, { "name": "smsalertnumber", "in": "query", - "description": "Case insensetive 'starts_with' search on smsalertnumber", + "description": "Case insensitive search on smsalertnumber", "required": false, "type": "string" - }, - { + }, { "name": "sms_provider_id", "in": "query", - "description": "Case insensetive 'starts_with' search on sms_provider_id", + "description": "Case insensitive search on sms_provider_id", "required": false, "type": "string" - }, - { + }, { "name": "privacy", "in": "query", - "description": "Case insensetive 'starts_with' search on privacy", + "description": "Case insensitive search on privacy", "required": false, "type": "string" - }, - { + }, { "name": "privacy_guarantor_checkouts", "in": "query", - "description": "Case insensetive 'starts_with' search on privacy_guarantor_checkouts", + "description": "Case insensitive search on privacy_guarantor_checkouts", "required": false, "type": "string" - }, - { + }, { "name": "checkprevcheckout", "in": "query", - "description": "Case insensetive 'starts_with' search on checkprevcheckout", + "description": "Case insensitive search on checkprevcheckout", "required": false, "type": "string" - }, - { + }, { "name": "updated_on", "in": "query", - "description": "Case insensetive 'starts_with' search on updated_on", + "description": "Case insensitive search on updated_on", "required": false, "type": "string" - }, - { + }, { "name": "lastseen", "in": "query", - "description": "Case insensetive 'starts_with' search on lastseen", + "description": "Case insensitive search on lastseen", "required": false, "type": "string" - }, - { + }, { "name": "lang", "in": "query", - "description": "Case insensetive 'starts_with' search on lang", + "description": "Case insensitive search on lang", "required": false, "type": "string" - }, - { + }, { "name": "login_attempts", "in": "query", - "description": "Case insensetive 'starts_with' search on login_attempts", + "description": "Case insensitive search on login_attempts", "required": false, "type": "string" - }, - { + }, { "name": "overdrive_auth_token", "in": "query", - "description": "Case insensetive 'starts_with' search on overdrive_auth_token", + "description": "Case insensitive search on overdrive_auth_token", "required": false, "type": "string" + }, { + "$ref": "../parameters.json#/match" + }, { + "$ref": "../parameters.json#/order_by" + }, { + "$ref": "../parameters.json#/page" + }, { + "$ref": "../parameters.json#/per_page" }], "responses": { "200": { @@ -554,6 +490,7 @@ } }, "post": { + "x-mojo-to": "Patron#add", "operationId": "addPatron", "tags": ["patrons"], "parameters": [{ @@ -606,14 +543,14 @@ "$ref": "../definitions.json#/error" } }, - "503": { - "description": "Under maintenance", + "500": { + "description": "Internal server error", "schema": { "$ref": "../definitions.json#/error" } }, - "500": { - "description": "Internal server error", + "503": { + "description": "Under maintenance", "schema": { "$ref": "../definitions.json#/error" } @@ -684,7 +621,8 @@ } }, "put": { - "operationId": "editPatron", + "x-mojo-to": "Patron#update", + "operationId": "updatePatron", "tags": ["patrons"], "parameters": [ { @@ -755,14 +693,13 @@ } }, "x-koha-authorization": { - "allow-owner": true, - "allow-guarantor": true, "permissions": { "borrowers": "1" } } }, "delete": { + "x-mojo-to": "Patron#delete", "operationId": "deletePatron", "tags": ["patrons"], "parameters": [{ diff --git a/t/db_dependent/api/v1/patrons.t b/t/db_dependent/api/v1/patrons.t index 6389b5c90e..26e241bf8c 100644 --- a/t/db_dependent/api/v1/patrons.t +++ b/t/db_dependent/api/v1/patrons.t @@ -19,13 +19,11 @@ use Modern::Perl; use Test::More tests => 5; use Test::Mojo; -use Test::Warn; use t::lib::TestBuilder; use t::lib::Mocks; use C4::Auth; -use Koha::Cities; use Koha::Database; my $schema = Koha::Database->new->schema; @@ -168,7 +166,7 @@ subtest 'add() tests' => sub { authorized => 1 }); $newpatron->{branchcode} = "nonexistent"; # Test invalid branchcode - my $tx = $t->ua->build_tx(POST => "/api/v1/patrons" =>json => $newpatron); + my $tx = $t->ua->build_tx(POST => "/api/v1/patrons" => json => $newpatron ); $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); $t->request_ok($tx) ->status_is(400) @@ -216,92 +214,48 @@ subtest 'add() tests' => sub { $schema->storage->txn_rollback; }; -subtest 'edit() tests' => sub { - plan tests => 3; +subtest 'update() tests' => sub { + plan tests => 2; $schema->storage->txn_begin; unauthorized_access_tests('PUT', 123, {email => 'nobody@example.com'}); - subtest 'patron modifying own data' => sub { - plan tests => 7; - - my ($borrowernumber, $sessionid) = create_user_and_session({ - authorized => 0 }); - my $patron = Koha::Patrons->find($borrowernumber)->TO_JSON; - - t::lib::Mocks::mock_preference("OPACPatronDetails", 0); - my $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . - $patron->{borrowernumber} => json => $patron); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); - $t->request_ok($tx) - ->status_is(403, 'OPACPatronDetails off - modifications not allowed.'); - - t::lib::Mocks::mock_preference("OPACPatronDetails", 1); - $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . - $patron->{borrowernumber} => json => $patron); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); - $t->request_ok($tx) - ->status_is(204, 'Updating myself with my current data'); - - $patron->{'firstname'} = "noob"; - $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . - $patron->{borrowernumber} => json => $patron); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); - $t->request_ok($tx) - ->status_is(202, 'Updating myself with my current data'); - - # Approve changes - Koha::Patron::Modifications->find({ - borrowernumber => $patron->{borrowernumber}, - firstname => "noob" - })->approve; - is(Koha::Patrons->find({ - borrowernumber => $patron->{borrowernumber}})->firstname, - "noob", "Changes approved"); - }; - subtest 'librarian access tests' => sub { plan tests => 20; t::lib::Mocks::mock_preference('minPasswordLength', 1); - my ($borrowernumber, $sessionid) = create_user_and_session({ - authorized => 1 }); - my ($borrowernumber2, undef) = create_user_and_session({ - authorized => 0 }); - my $patron = Koha::Patrons->find($borrowernumber2); - my $newpatron = Koha::Patrons->find($borrowernumber2)->TO_JSON; + my ($borrowernumber, $sessionid) = create_user_and_session({ authorized => 1 }); + my ($borrowernumber2, undef) = create_user_and_session({ authorized => 0 }); + + my $patron_1 = Koha::Patrons->find($borrowernumber); + my $patron_2 = Koha::Patrons->find($borrowernumber2); + my $newpatron = $patron_2->TO_JSON; - my $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/-1" => - json => $newpatron); + my $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/-1" => json => $newpatron ); $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); $t->request_ok($tx) ->status_is(404) ->json_has('/error', 'Fails when trying to PUT nonexistent patron'); $newpatron->{categorycode} = 'nonexistent'; - $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . - $newpatron->{borrowernumber} => json => $newpatron - ); + $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/$borrowernumber2" => json => $newpatron ); $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); $t->request_ok($tx) ->status_is(400) ->json_is('/error' => "Given categorycode does not exist"); - $newpatron->{categorycode} = $patron->categorycode; + $newpatron->{categorycode} = $patron_2->categorycode; $newpatron->{branchcode} = 'nonexistent'; - $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . - $newpatron->{borrowernumber} => json => $newpatron - ); + $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/$borrowernumber2" => json => $newpatron ); $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); $t->request_ok($tx) ->status_is(400) ->json_is('/error' => "Given branchcode does not exist"); - $newpatron->{branchcode} = $patron->branchcode; + $newpatron->{branchcode} = $patron_2->branchcode; $newpatron->{falseproperty} = "Non existent property"; - $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . - $newpatron->{borrowernumber} => json => $newpatron); + $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/$borrowernumber2" => json => $newpatron ); $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); $t->request_ok($tx) ->status_is(400) @@ -309,18 +263,20 @@ subtest 'edit() tests' => sub { 'Properties not allowed: falseproperty.'); delete $newpatron->{falseproperty}; - $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" . - $borrowernumber => json => $newpatron); - $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); - $t->request_ok($tx) - ->status_is(409) - ->json_has('/error' => "Fails when trying to update to an existing" - ."cardnumber or userid") - ->json_has('/conflict', { - cardnumber => $newpatron->{ cardnumber }, - userid => $newpatron->{ userid } - } - ); + # Set both cardnumber and userid to already existing values + $newpatron->{cardnumber} = $patron_1->cardnumber; + $newpatron->{userid} = $patron_1->userid; + + $tx = $t->ua->build_tx( PUT => "/api/v1/patrons/$borrowernumber2" => json => $newpatron ); + $tx->req->cookies({ name => 'CGISESSID', value => $sessionid }); + $t->request_ok($tx)->status_is(409) + ->json_has( '/error' => "Fails when trying to update to an existing cardnumber or userid") + ->json_is( '/conflict', + { + cardnumber => $newpatron->{cardnumber}, + userid => $newpatron->{userid} + } + ); $newpatron->{ cardnumber } = $borrowernumber.$borrowernumber2; $newpatron->{ userid } = "user".$borrowernumber.$borrowernumber2; @@ -390,7 +346,7 @@ sub unauthorized_access_tests { $tx->req->cookies({name => 'CGISESSID', value => $sessionid}); $t->request_ok($tx) ->status_is(403) - ->json_is('/required_permissions', {"borrowers" => "1"}); + ->json_has('/required_permissions'); }; } @@ -398,7 +354,6 @@ sub create_user_and_session { my $args = shift; my $flags = ( $args->{authorized} ) ? 16 : 0; - my $dbh = C4::Context->dbh; my $user = $builder->build( { @@ -409,7 +364,7 @@ sub create_user_and_session { lost => 0, email => 'nobody@example.com', emailpro => 'nobody@example.com', - B_email => 'nobody@example.com', + B_email => 'nobody@example.com' } } ); -- 2.39.5