Attachment 69548 Details for Bug 16330 – Bug 16330: Move patches to OpenAPI

[patch] Bug 16330: Move patches to OpenAPI

Bug-16330-Move-patches-to-OpenAPI.patch (text/plain), 43.16 KB, created by Tomás Cohen Arazi (tcohen) on 2017-12-05 19:45:26 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Tomás Cohen Arazi (tcohen)

Created: 2017-12-05 19:45:26 UTC

Size: 43.16 KB

patch

obsolete

>From c2877622e35d82f455d75117bcf8f78bc0a93c8f Mon Sep 17 00:00:00 2001
>From: Tomas Cohen Arazi <tomascohen@theke.io>
>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 <tomascohen@theke.io>
>---
> Koha/REST/V1/Patron.pm            | 270 +++++++++++++++++---------
> api/v1/swagger/paths/patrons.json | 389 ++++++++++++++++----------------------
> t/db_dependent/api/v1/patrons.t   | 105 +++-------
> 3 files changed, 372 insertions(+), 392 deletions(-)
>
>diff --git a/Koha/REST/V1/Patron.pm b/Koha/REST/V1/Patron.pm
>index 0eccaae997..52ffab3bbb 100644
>--- a/Koha/REST/V1/Patron.pm
>+++ b/Koha/REST/V1/Patron.pm
>@@ -20,38 +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;
> 
>+=head1 NAME
>+
>+Koha::REST::V1::Patron
>+
>+=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;
> 
>-    my $args   = $c->req->params->to_hash;
>-    my $filter = {};
>-    for my $filter_param ( keys %$args ) {
>-        $filter->{$filter_param} = { LIKE => $args->{$filter_param} . "%" };
>-    }
>-
>     return try {
>-        my $patrons = Koha::Patrons->search($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;
> 
>@@ -65,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 b2403f3b88..f34dd17b68 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..e850259e96 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;
>@@ -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.14.1

Actions: View | Diff | Splinter Review

Attachments on bug 16330: 50803 | 50889 | 51066 | 51131 | 51132 | 53216 | 53217 | 53820 | 54942 | 55147 | 56666 | 56667 | 56668 | 63646 | 63647 | 63649 | 63650 | 69546 | 69547 | 69548 | 69549 | 69550 | 69551 | 69752 | 69762 | 69763 | 69764 | 69765 | 69766 | 69767 | 69768 | 69787 | 70034 | 70035 | 70036 | 70037 | 70038 | 71667 | 71668 | 71669 | 71670 | 71671 | 71920 | 71921 | 71922 | 71923 | 71924 | 71925 | 71933 | 71934 | 71935 | 71936 | 71937 | 71938 | 73434