Attachment 71920 Details for Bug 16330 – Bug 16330: Add routes to add, update and delete patrons

[patch] Bug 16330: Add routes to add, update and delete patrons

Bug-16330-Add-routes-to-add-update-and-delete-patr.patch (text/plain), 60.33 KB, created by Josef Moravec on 2018-02-19 08:43:04 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Josef Moravec

Created: 2018-02-19 08:43:04 UTC

Size: 60.33 KB

patch

obsolete

>From b3547f5586df42e622033f628bc006c9aceca08b Mon Sep 17 00:00:00 2001
>From: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
>Date: Wed, 27 Apr 2016 13:47:04 +0000
>Subject: [PATCH] Bug 16330: Add routes to add, update and delete patrons
>
>This patch adds support for add, edit and delete patrons via REST API.
>
>GET  /api/v1/patrons                   Get patron list from params
>GET  /api/v1/patrons/<borrowernumber>  Get single patron
>POST /api/v1/patrons                   Create a new patron
>PUT  /api/v1/patrons/<borrowernumber>  Update data about patron
>DEL  /api/v1/patrons/<borrowernumber>  Delete a patron
>
>Revised Test plan:
>1) Apply this patch
>2) Run tests perl t/db_dependent/api/v1/patrons.t
>3) Add a user with proper rights to use the REST API
>4) play with your favourite REST client (curl/httpie, etc.):
>   Authenticate with the user created above and get a CGISESSION id.
>   Use the CGISESSION to add, edit and delete patrons via the API.
>5) Use PUT /patrons/<borrowernumber> for a patron without borrowers
>   flag. This should go into pending patron modification status and
>   needs to be accepted by a librarian.
>
>Please note there is no validation of body input in PUT/POST other
>than branchcode,category,userid,cardnumber.
>
>Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
>Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
>---
> Koha/Exceptions.pm                     |   8 +-
> Koha/Exceptions/Category.pm            |  17 +
> Koha/Exceptions/Library.pm             |  17 +
> Koha/Exceptions/Patron.pm              |  17 +
> Koha/Patron.pm                         | 127 ++++++
> Koha/REST/V1/Patron.pm                 | 166 +++++++-
> api/v1/swagger/definitions/patron.json |  14 +-
> api/v1/swagger/paths/patrons.json      | 708 ++++++++++++++++++++++++++++++++-
> t/db_dependent/Koha/Patrons.t          |  94 +++++
> t/db_dependent/api/v1/patrons.t        | 494 ++++++++++++++++++-----
> 10 files changed, 1547 insertions(+), 115 deletions(-)
> create mode 100644 Koha/Exceptions/Category.pm
> create mode 100644 Koha/Exceptions/Library.pm
> create mode 100644 Koha/Exceptions/Patron.pm
>
>diff --git a/Koha/Exceptions.pm b/Koha/Exceptions.pm
>index 9d7f397..f49a289 100644
>--- a/Koha/Exceptions.pm
>+++ b/Koha/Exceptions.pm
>@@ -10,8 +10,8 @@ use Exception::Class (
>     },
>     'Koha::Exceptions::BadParameter' => {
>         isa => 'Koha::Exceptions::Exception',
>-        description => 'Bad parameter was given',
>-        fields => ["parameter"],
>+        description => 'A bad parameter was given',
>+        fields => ['parameter'],
>     },
>     'Koha::Exceptions::DuplicateObject' => {
>         isa => 'Koha::Exceptions::Exception',
>@@ -29,6 +29,10 @@ use Exception::Class (
>         isa => 'Koha::Exceptions::Exception',
>         description => 'A required parameter is missing'
>     },
>+    'Koha::Exceptions::NoChanges' => {
>+        isa => 'Koha::Exceptions::Exception',
>+        description => 'No changes were made',
>+    },
>     'Koha::Exceptions::WrongParameter' => {
>         isa => 'Koha::Exceptions::Exception',
>         description => 'One or more parameters are wrong',
>diff --git a/Koha/Exceptions/Category.pm b/Koha/Exceptions/Category.pm
>new file mode 100644
>index 0000000..3487faa
>--- /dev/null
>+++ b/Koha/Exceptions/Category.pm
>@@ -0,0 +1,17 @@
>+package Koha::Exceptions::Category;
>+
>+use Modern::Perl;
>+
>+use Exception::Class (
>+
>+    'Koha::Exceptions::Category' => {
>+        description => 'Something went wrong!',
>+    },
>+    'Koha::Exceptions::Category::CategorycodeNotFound' => {
>+        isa => 'Koha::Exceptions::Category',
>+        description => "Category does not exist",
>+        fields => ["categorycode"],
>+    },
>+);
>+
>+1;
>diff --git a/Koha/Exceptions/Library.pm b/Koha/Exceptions/Library.pm
>new file mode 100644
>index 0000000..1aea6ad
>--- /dev/null
>+++ b/Koha/Exceptions/Library.pm
>@@ -0,0 +1,17 @@
>+package Koha::Exceptions::Library;
>+
>+use Modern::Perl;
>+
>+use Exception::Class (
>+
>+    'Koha::Exceptions::Library' => {
>+        description => 'Something went wrong!',
>+    },
>+    'Koha::Exceptions::Library::BranchcodeNotFound' => {
>+        isa => 'Koha::Exceptions::Library',
>+        description => "Library does not exist",
>+        fields => ["branchcode"],
>+    },
>+);
>+
>+1;
>diff --git a/Koha/Exceptions/Patron.pm b/Koha/Exceptions/Patron.pm
>new file mode 100644
>index 0000000..c409b4e
>--- /dev/null
>+++ b/Koha/Exceptions/Patron.pm
>@@ -0,0 +1,17 @@
>+package Koha::Exceptions::Patron;
>+
>+use Modern::Perl;
>+
>+use Exception::Class (
>+
>+    'Koha::Exceptions::Patron' => {
>+        description => 'Something went wrong!',
>+    },
>+    'Koha::Exceptions::Patron::DuplicateObject' => {
>+        isa => 'Koha::Exceptions::Patron',
>+        description => "Patron cardnumber and userid must be unique",
>+        fields => ["conflict"],
>+    },
>+);
>+
>+1;
>diff --git a/Koha/Patron.pm b/Koha/Patron.pm
>index d0be998..faf4efa 100644
>--- a/Koha/Patron.pm
>+++ b/Koha/Patron.pm
>@@ -2,6 +2,7 @@ package Koha::Patron;
> 
> # Copyright ByWater Solutions 2014
> # Copyright PTFS Europe 2016
>+# Copyright Koha-Suomi Oy 2017
> #
> # This file is part of Koha.
> #
>@@ -30,6 +31,11 @@ use Koha::Database;
> use Koha::DateUtils;
> use Koha::Holds;
> use Koha::Old::Checkouts;
>+use Koha::Exceptions;
>+use Koha::Exceptions::Category;
>+use Koha::Exceptions::Library;
>+use Koha::Exceptions::Patron;
>+use Koha::Libraries;
> use Koha::Patron::Categories;
> use Koha::Patron::HouseboundProfile;
> use Koha::Patron::HouseboundRole;
>@@ -831,6 +837,18 @@ sub is_child {
>     return $self->category->category_type eq 'C' ? 1 : 0;
> }
> 
>+=head3 store
>+
>+=cut
>+
>+sub store {
>+    my ($self) = @_;
>+
>+    # $self->_validate();
>+
>+    return $self->SUPER::store();
>+}
>+
> =head3 type
> 
> =cut
>@@ -839,6 +857,115 @@ sub _type {
>     return 'Borrower';
> }
> 
>+=head2 Internal methods
>+
>+=head3 _check_branchcode
>+
>+Checks the existence of patron's branchcode and throws
>+Koha::Exceptions::Library::BranchcodeNotFound if branchcode is not found.
>+
>+=cut
>+
>+sub _check_branchcode {
>+    my ($self) = @_;
>+
>+    return unless $self->branchcode;
>+    unless (Koha::Libraries->find($self->branchcode)) {
>+        Koha::Exceptions::Library::BranchcodeNotFound->throw(
>+            error => "Library does not exist",
>+            branchcode => $self->branchcode,
>+        );
>+    }
>+    return 1;
>+}
>+
>+=head3 _check_categorycode
>+
>+Checks the existence of patron's categorycode and throws
>+Koha::Exceptions::Category::CategorycodeNotFound if categorycode is not found.
>+
>+=cut
>+
>+sub _check_categorycode {
>+    my ($self) = @_;
>+
>+    return unless $self->categorycode;
>+    unless (Koha::Patron::Categories->find($self->categorycode)) {
>+        Koha::Exceptions::Category::CategorycodeNotFound->throw(
>+            error => "Patron category does not exist",
>+            categorycode => $self->categorycode,
>+        );
>+    }
>+    return 1;
>+}
>+
>+=head3 _check_uniqueness
>+
>+Checks patron's cardnumber and userid for uniqueness and throws
>+Koha::Exceptions::Patron::DuplicateObject if conflicting with another patron.
>+
>+=cut
>+
>+sub _check_uniqueness {
>+    my ($self) = @_;
>+
>+    my $select = {};
>+    $select->{cardnumber} = $self->cardnumber if $self->cardnumber;
>+    $select->{userid} = $self->userid if $self->userid;
>+
>+    return unless keys %$select;
>+
>+    # Find conflicting patrons
>+    my $patrons = Koha::Patrons->search({
>+        '-or' => $select
>+    });
>+
>+    if ($patrons->count) {
>+        my $conflict = {};
>+        foreach my $patron ($patrons->as_list) {
>+            # New patron $self: a conflicting patron $patron found.
>+            # Updating patron $self: first make sure conflicting patron $patron is
>+            #                        not this patron $self.
>+            if (!$self->in_storage || $self->in_storage &&
>+            $self->borrowernumber != $patron->borrowernumber) {
>+                # Populate conflict information to exception
>+                if ($patron->cardnumber && $self->cardnumber &&
>+                    $patron->cardnumber eq $self->cardnumber)
>+                {
>+                    $conflict->{cardnumber} = $self->cardnumber;
>+                }
>+                if ($patron->userid && $self->userid &&
>+                    $patron->userid eq $self->userid)
>+                {
>+                    $conflict->{userid} = $self->userid;
>+                }
>+            }
>+        }
>+
>+        Koha::Exceptions::Patron::DuplicateObject->throw(
>+            error => "Patron data conflicts with another patron",
>+            conflict => $conflict
>+        ) if keys %$conflict;
>+    }
>+    return 1;
>+}
>+
>+=head3 _validate
>+
>+Performs a set of validations on this object and throws Koha::Exceptions if errors
>+are found.
>+
>+=cut
>+
>+sub _validate {
>+    my ($self) = @_;
>+
>+    $self->_check_branchcode;
>+    $self->_check_categorycode;
>+    $self->_check_uniqueness;
>+    return $self;
>+}
>+
> =head1 AUTHOR
> 
> Kyle M Hall <kyle@bywatersolutions.com>
>diff --git a/Koha/REST/V1/Patron.pm b/Koha/REST/V1/Patron.pm
>index 8fbe458..763b325 100644
>--- a/Koha/REST/V1/Patron.pm
>+++ b/Koha/REST/V1/Patron.pm
>@@ -19,15 +19,38 @@ 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;
> 
>-    # FIXME The limited does not work here, the userenv is not set
>-    my $patrons = Koha::Patrons->search_limited;
> 
>-    return $c->render(status => 200, openapi => $patrons);
>+    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_limited($filter);
>+        return $c->render(status => 200, openapi => $patrons);
>+    }
>+    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." } );
>+        }
>+    };
> }
> 
> sub get {
>@@ -35,6 +58,7 @@ sub get {
> 
>     my $borrowernumber = $c->validation->param('borrowernumber');
>     my $patron = Koha::Patrons->find($borrowernumber);
>+
>     unless ($patron) {
>         return $c->render(status => 404, openapi => { error => "Patron not found." });
>     }
>@@ -42,4 +66,140 @@ sub get {
>     return $c->render(status => 200, openapi => $patron);
> }
> 
>+sub add {
>+    my ($c, $args, $cb) = @_;
>+
>+    return try {
>+        my $body = $c->req->json;
>+
>+        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);
>+
>+        return $c->$cb($patron, 201);
>+    }
>+    catch {
>+        unless (blessed $_ && $_->can('rethrow')) {
>+            return $c->$cb({ error =>
>+                "Something went wrong, check Koha logs for details."}, 500);
>+        }
>+        if ($_->isa('Koha::Exceptions::Patron::DuplicateObject')) {
>+            return $c->$cb({ error => $_->error, conflict => $_->conflict }, 409);
>+        }
>+        elsif ($_->isa('Koha::Exceptions::Library::BranchcodeNotFound')) {
>+            return $c->$cb({ error => "Given branchcode does not exist" }, 400);
>+        }
>+        elsif ($_->isa('Koha::Exceptions::Category::CategorycodeNotFound')) {
>+            return $c->$cb({ error => "Given categorycode does not exist"}, 400);
>+        }
>+        else {
>+            return $c->$cb({ error =>
>+                "Something went wrong, check Koha logs for details."}, 500);
>+        }
>+    };
>+}
>+
>+sub edit {
>+    my ($c, $args, $cb) = @_;
>+
>+    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);
>+            }
>+        }
>+        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);
>+        }
>+    }
>+    catch {
>+        unless ($patron) {
>+            return $c->$cb({error => "Patron not found"}, 404);
>+        }
>+        unless (blessed $_ && $_->can('rethrow')) {
>+            return $c->$cb({ error =>
>+                "Something went wrong, check Koha logs for details."}, 500);
>+        }
>+        if ($_->isa('Koha::Exceptions::Patron::DuplicateObject')) {
>+            return $c->$cb({ error => $_->error, conflict => $_->conflict }, 409);
>+        }
>+        elsif ($_->isa('Koha::Exceptions::Library::BranchcodeNotFound')) {
>+            return $c->$cb({ error => "Given branchcode does not exist" }, 400);
>+        }
>+        elsif ($_->isa('Koha::Exceptions::Category::CategorycodeNotFound')) {
>+            return $c->$cb({ error => "Given categorycode does not exist"}, 400);
>+        }
>+        elsif ($_->isa('Koha::Exceptions::MissingParameter')) {
>+            return $c->$cb({error => "Missing mandatory parameter(s)",
>+                            parameters => $_->parameter }, 400);
>+        }
>+        elsif ($_->isa('Koha::Exceptions::BadParameter')) {
>+            return $c->$cb({error => "Invalid parameter(s)",
>+                            parameters => $_->parameter }, 400);
>+        }
>+        elsif ($_->isa('Koha::Exceptions::NoChanges')) {
>+            return $c->$cb({error => "No changes have been made"}, 204);
>+        }
>+        else {
>+            return $c->$cb({ error =>
>+                "Something went wrong, check Koha logs for details."}, 500);
>+        }
>+    };
>+}
>+
>+sub delete {
>+    my ($c, $args, $cb) = @_;
>+
>+    my $patron;
>+
>+    return try {
>+        $patron = Koha::Patrons->find($args->{borrowernumber});
>+        # check if loans, reservations, debarrment, etc. before deletion!
>+        my $res = $patron->delete;
>+
>+        return $c->$cb({}, 200);
>+    }
>+    catch {
>+        unless ($patron) {
>+            return $c->$cb({error => "Patron not found"}, 404);
>+        }
>+        else {
>+            return $c->$cb({ error =>
>+                "Something went wrong, check Koha logs for details."}, 500);
>+        }
>+    };
>+}
>+
>+sub _delete_unmodifiable_parameters {
>+    my ($body) = @_;
>+
>+    my %columns = map { $_ => 1 } Koha::Patron::Modifications->columns;
>+    foreach my $param (keys %$body) {
>+        unless (exists $columns{$param}) {
>+            delete $body->{$param};
>+        }
>+    }
>+    return $body;
>+}
>+
> 1;
>diff --git a/api/v1/swagger/definitions/patron.json b/api/v1/swagger/definitions/patron.json
>index 2c6f9a6..948b956 100644
>--- a/api/v1/swagger/definitions/patron.json
>+++ b/api/v1/swagger/definitions/patron.json
>@@ -121,6 +121,7 @@
>     },
>     "dateofbirth": {
>       "type": ["string", "null"],
>+      "format": "date",
>       "description": "patron's date of birth"
>     },
>     "branchcode": {
>@@ -133,10 +134,12 @@
>     },
>     "dateenrolled": {
>       "type": ["string", "null"],
>+      "format": "date",
>       "description": "date the patron was added to Koha"
>     },
>     "dateexpiry": {
>       "type": ["string", "null"],
>+      "format": "date",
>       "description": "date the patron's card is set to expire"
>     },
>     "date_renewed": {
>@@ -153,6 +156,7 @@
>     },
>     "debarred": {
>       "type": ["string", "null"],
>+      "format": "date",
>       "description": "until this date the patron can only check-in"
>     },
>     "debarredcomment": {
>@@ -272,15 +276,17 @@
>       "description": "produce a warning for this patron if this item has previously been checked out to this patron if 'yes', not if 'no', defer to category setting if 'inherit'"
>     },
>     "updated_on": {
>-      "type": ["string", "null"],
>+      "type": "string",
>+      "format": "date-time",
>       "description": "time of last change could be useful for synchronization with external systems (among others)"
>     },
>     "lastseen": {
>       "type": ["string", "null"],
>+      "format": "date-time",
>       "description": "last time a patron has been seen (connected at the OPAC or staff interface)"
>     },
>     "lang": {
>-      "type": ["string", "null"],
>+      "type": "string",
>       "description": "lang to use to send notices to this patron"
>     },
>     "login_attempts": {
>@@ -291,5 +297,7 @@
>       "type": ["string", "null"],
>       "description": "persist OverDrive auth token"
>     }
>-  }
>+  },
>+  "additionalProperties": false,
>+  "required": ["surname", "address", "city", "branchcode", "categorycode"]
> }
>diff --git a/api/v1/swagger/paths/patrons.json b/api/v1/swagger/paths/patrons.json
>index 565d20c..56218a4 100644
>--- a/api/v1/swagger/paths/patrons.json
>+++ b/api/v1/swagger/paths/patrons.json
>@@ -7,6 +7,517 @@
>       "produces": [
>           "application/json"
>       ],
>+      "parameters": [{
>+        "name": "borrowernumber",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on borrowernumber",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "cardnumber",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on cardnumber",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "surname",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on surname",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "firstname",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on firstname",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "title",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on title",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "othernames",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on othernames",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "initials",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on initials",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "streetnumber",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on streetnumber",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "streettype",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on streettype",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "address",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on address",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "address2",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on address2",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "city",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on city",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "state",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on state",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "zipcode",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on zipcode",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "country",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on country",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "email",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on email",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "phone",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on phone",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "mobile",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on mobile",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "fax",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on fax",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "emailpro",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on emailpro",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "phonepro",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on phonepro",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "B_streetnumber",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on B_streetnumber",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "B_streettype",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on B_streettype",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "B_address",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on B_address",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "B_address2",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on B_address2",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "B_city",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on B_city",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "B_state",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on B_state",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "B_zipcode",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on B_zipcode",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "B_country",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on B_country",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "B_email",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on B_email",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "B_phone",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on B_phone",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "dateofbirth",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on dateofbirth",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "branchcode",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on branchcode",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "categorycode",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on categorycode",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "dateenrolled",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on dateenrolled",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "dateexpiry",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on dateexpiry",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "gonenoaddress",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on gonenoaddress",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "lost",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on lost",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "debarred",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on debarred",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "debarredcomment",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on debarredcomment",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "contactname",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on contactname",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "contactfirstname",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on contactfirstname",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "contacttitle",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on contacttitle",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "guarantorid",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on guarantorid",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "borrowernotes",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on borrowernotes",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "relationship",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on relationship",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "sex",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on sex",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "password",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on password",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "flags",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on flags",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "userid",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on userid",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "opacnote",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on opacnote",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "contactnote",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on contactnote",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "sort1",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on sort1",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "sort2",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on sort2",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "altcontactfirstname",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on altcontactfirstname",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "altcontactsurname",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on altcontactsurname",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "altcontactaddress1",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on altcontactaddress1",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "altcontactaddress2",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on altcontactaddress2",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "altcontactaddress3",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on altcontactaddress3",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "altcontactstate",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on altcontactstate",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "altcontactzipcode",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on altcontactzipcode",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "altcontactcountry",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on altcontactcountry",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "altcontactphone",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on altcontactphone",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "smsalertnumber",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on smsalertnumber",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "sms_provider_id",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on sms_provider_id",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "privacy",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on privacy",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "privacy_guarantor_checkouts",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on privacy_guarantor_checkouts",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "checkprevcheckout",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on checkprevcheckout",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "updated_on",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on updated_on",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "lastseen",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on lastseen",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "lang",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on lang",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "login_attempts",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on login_attempts",
>+        "required": false,
>+        "type": "string"
>+      },
>+      {
>+        "name": "overdrive_auth_token",
>+        "in": "query",
>+        "description": "Case insensetive 'starts_with' search on overdrive_auth_token",
>+        "required": false,
>+        "type": "string"
>+      }],
>       "responses": {
>         "200": {
>           "description": "A list of patrons",
>@@ -34,12 +545,78 @@
>           "schema": {
>             "$ref": "../definitions.json#/error"
>           }
>+        }
>+      },
>+      "x-koha-authorization": {
>+        "permissions": {
>+          "borrowers": "1"
>+        }
>+      }
>+    },
>+    "post": {
>+      "operationId": "addPatron",
>+      "tags": ["patrons"],
>+      "parameters": [{
>+        "name": "body",
>+        "in": "body",
>+        "description": "A JSON object containing information about the new patron",
>+        "required": true,
>+        "schema": {
>+          "$ref": "../definitions.json#/patron"
>+        }
>+      }],
>+      "consumes": ["application/json"],
>+      "produces": ["application/json"],
>+      "responses": {
>+        "201": {
>+          "description": "A successfully created patron",
>+          "schema": {
>+            "items": {
>+              "$ref": "../definitions.json#/patron"
>+            }
>+          }
>+        },
>+        "400": {
>+          "description": "Bad parameter",
>+          "schema": {
>+            "$ref": "../definitions.json#/error"
>+          }
>+        },
>+        "401": {
>+          "description": "Authentication required",
>+          "schema": {
>+            "$ref": "../definitions.json#/error"
>+          }
>+        },
>+        "403": {
>+          "description": "Access forbidden",
>+          "schema": {
>+            "$ref": "../definitions.json#/error"
>+          }
>+        },
>+        "404": {
>+          "description": "Resource not found",
>+          "schema": {
>+            "$ref": "../definitions.json#/error"
>+          }
>+        },
>+        "409": {
>+          "description": "Conflict in creating resource",
>+          "schema": {
>+            "$ref": "../definitions.json#/error"
>+          }
>         },
>         "503": {
>           "description": "Under maintenance",
>           "schema": {
>             "$ref": "../definitions.json#/error"
>           }
>+        },
>+        "500": {
>+          "description": "Internal server error",
>+          "schema": {
>+            "$ref": "../definitions.json#/error"
>+          }
>         }
>       },
>       "x-koha-authorization": {
>@@ -55,11 +632,10 @@
>       "operationId": "getPatron",
>       "tags": ["patrons"],
>       "parameters": [{
>-          "$ref": "../parameters.json#/borrowernumberPathParam"
>-        }
>-      ],
>+        "$ref": "../parameters.json#/borrowernumberPathParam"
>+      }],
>       "produces": [
>-          "application/json"
>+        "application/json"
>       ],
>       "responses": {
>         "200": {
>@@ -106,6 +682,130 @@
>           "borrowers": "edit_borrowers"
>         }
>       }
>+    },
>+    "put": {
>+      "operationId": "editPatron",
>+      "tags": ["patrons"],
>+      "parameters": [
>+        {
>+          "$ref": "../parameters.json#/borrowernumberPathParam"
>+        },
>+        {
>+          "name": "body",
>+          "in": "body",
>+          "description": "A JSON object containing new information about existing patron",
>+          "required": true,
>+          "schema": {
>+            "$ref": "../definitions.json#/patron"
>+          }
>+        }
>+      ],
>+      "consumes": ["application/json"],
>+      "produces": ["application/json"],
>+      "responses": {
>+        "200": {
>+          "description": "A successfully updated patron",
>+          "schema": {
>+            "items": {
>+              "$ref": "../definitions.json#/patron"
>+            }
>+          }
>+        },
>+        "202": {
>+          "description": "Accepted and waiting for librarian verification",
>+          "schema": {
>+            "type": "object"
>+          }
>+        },
>+        "204": {
>+          "description": "No Content",
>+          "schema": {
>+            "type": "object"
>+          }
>+        },
>+        "400": {
>+          "description": "Bad parameter",
>+          "schema": {
>+            "$ref": "../definitions.json#/error"
>+          }
>+        },
>+        "403": {
>+          "description": "Access forbidden",
>+          "schema": {
>+            "$ref": "../definitions.json#/error"
>+          }
>+        },
>+        "404": {
>+          "description": "Resource not found",
>+          "schema": {
>+            "$ref": "../definitions.json#/error"
>+          }
>+        },
>+        "409": {
>+          "description": "Conflict in updating resource",
>+          "schema": {
>+            "$ref": "../definitions.json#/error"
>+          }
>+        },
>+        "500": {
>+          "description": "Internal server error",
>+          "schema": {
>+            "$ref": "../definitions.json#/error"
>+          }
>+        }
>+      },
>+      "x-koha-authorization": {
>+        "allow-owner": true,
>+        "allow-guarantor": true,
>+        "permissions": {
>+          "borrowers": "1"
>+        }
>+      }
>+    },
>+    "delete": {
>+      "operationId": "deletePatron",
>+      "tags": ["patrons"],
>+      "parameters": [{
>+        "$ref": "../parameters.json#/borrowernumberPathParam"
>+      }],
>+      "produces": ["application/json"],
>+      "responses": {
>+        "200": {
>+          "description": "Patron deleted successfully",
>+          "schema": {
>+            "type": "object"
>+          }
>+        },
>+        "400": {
>+          "description": "Patron deletion failed",
>+          "schema": {
>+            "$ref": "../definitions.json#/error"
>+          }
>+        },
>+        "401": {
>+          "description": "Authentication required",
>+          "schema": {
>+            "$ref": "../definitions.json#/error"
>+          }
>+        },
>+        "403": {
>+          "description": "Access forbidden",
>+          "schema": {
>+            "$ref": "../definitions.json#/error"
>+          }
>+        },
>+        "404": {
>+          "description": "Patron not found",
>+          "schema": {
>+            "$ref": "../definitions.json#/error"
>+          }
>+        }
>+      },
>+      "x-koha-authorization": {
>+        "permissions": {
>+          "borrowers": "1"
>+        }
>+      }
>     }
>   }
> }
>diff --git a/t/db_dependent/Koha/Patrons.t b/t/db_dependent/Koha/Patrons.t
>index b5a36a3..0b51a33 100644
>--- a/t/db_dependent/Koha/Patrons.t
>+++ b/t/db_dependent/Koha/Patrons.t
>@@ -1134,3 +1134,97 @@ sub set_logged_in_user {
>         '',                      ''
>     );
> }
>+
>+subtest '_validate() tests' => sub {
>+    plan tests => 4;
>+
>+    $schema->storage->txn_begin;
>+
>+    Koha::Patrons->delete;
>+
>+    my $categorycode = $builder->build({ source => 'Category' })->{categorycode};
>+    my $branchcode = $builder->build({ source => 'Branch' })->{branchcode};
>+    my $patron = $builder->build({
>+        source => 'Borrower',
>+        value => {
>+            branchcode   => $branchcode,
>+            cardnumber   => 'conflict',
>+            categorycode => $categorycode,
>+        }
>+    });
>+
>+    ok(Koha::Patron->new({
>+        surname      => 'Store test',
>+        branchcode   => $branchcode,
>+        categorycode => $categorycode
>+    })->_validate->store, 'Stored a patron');
>+
>+    subtest '_check_categorycode' => sub {
>+        plan tests => 2;
>+
>+        my $conflicting = $builder->build({
>+            source => 'Borrower',
>+            value => {
>+                branchcode   => $branchcode,
>+                categorycode => 'nonexistent',
>+            }
>+        });
>+        delete $conflicting->{borrowernumber};
>+
>+        eval { Koha::Patron->new($conflicting)->_validate };
>+
>+        isa_ok($@, "Koha::Exceptions::Category::CategorycodeNotFound");
>+        is($@->{categorycode}, $conflicting->{categorycode},
>+           'Exception describes non-existent categorycode');
>+    };
>+
>+    subtest '_check_categorycode' => sub {
>+        plan tests => 2;
>+
>+        my $conflicting = $builder->build({
>+            source => 'Borrower',
>+            value => {
>+                branchcode   => 'nonexistent',
>+                categorycode => $categorycode,
>+            }
>+        });
>+        delete $conflicting->{borrowernumber};
>+
>+        eval { Koha::Patron->new($conflicting)->_validate };
>+
>+        isa_ok($@, "Koha::Exceptions::Library::BranchcodeNotFound");
>+        is($@->{branchcode}, $conflicting->{branchcode},
>+           'Exception describes non-existent branchcode');
>+    };
>+
>+    subtest '_check_uniqueness() tests' => sub {
>+        plan tests => 4;
>+
>+        my $conflicting = $builder->build({
>+            source => 'Borrower',
>+            value => {
>+                branchcode   => $branchcode,
>+                categorycode => $categorycode,
>+            }
>+        });
>+        delete $conflicting->{borrowernumber};
>+        $conflicting->{cardnumber} = 'conflict';
>+        $conflicting->{userid} = $patron->{userid};
>+
>+        eval { Koha::Patron->new($conflicting)->_validate };
>+
>+        isa_ok($@, "Koha::Exceptions::Patron::DuplicateObject");
>+        is($@->{conflict}->{cardnumber}, $conflicting->{cardnumber},
>+           'Exception describes conflicting cardnumber');
>+        is($@->{conflict}->{userid}, $conflicting->{userid},
>+           'Exception describes conflicting userid');
>+
>+        $conflicting->{cardnumber} = 'notconflicting';
>+        $conflicting->{userid}     = 'notconflicting';
>+
>+        ok(Koha::Patron->new($conflicting)->_validate->store, 'After modifying'
>+           .' cardnumber and userid to not conflict with others, no exception.');
>+    };
>+
>+    $schema->storage->txn_rollback;
>+};
>diff --git a/t/db_dependent/api/v1/patrons.t b/t/db_dependent/api/v1/patrons.t
>index 2166547..6389b5c 100644
>--- a/t/db_dependent/api/v1/patrons.t
>+++ b/t/db_dependent/api/v1/patrons.t
>@@ -17,122 +17,410 @@
> 
> use Modern::Perl;
> 
>-use Test::More tests => 21;
>+use Test::More tests => 5;
> use Test::Mojo;
>+use Test::Warn;
>+
> use t::lib::TestBuilder;
> use t::lib::Mocks;
> 
> use C4::Auth;
>-use C4::Context;
>-
>+use Koha::Cities;
> use Koha::Database;
>-use Koha::Patron;
> 
> my $schema  = Koha::Database->new->schema;
>-my $builder = t::lib::TestBuilder->new();
>-
>-$schema->storage->txn_begin;
>+my $builder = t::lib::TestBuilder->new;
> 
> # FIXME: sessionStorage defaults to mysql, but it seems to break transaction handling
> # this affects the other REST api tests
> t::lib::Mocks::mock_preference( 'SessionStorage', 'tmp' );
> 
>-$ENV{REMOTE_ADDR} = '127.0.0.1';
>-my $t = Test::Mojo->new('Koha::REST::V1');
>+my $remote_address = '127.0.0.1';
>+my $t              = Test::Mojo->new('Koha::REST::V1');
> 
>-my $categorycode = $builder->build({ source => 'Category' })->{ categorycode };
>-my $branchcode = $builder->build({ source => 'Branch' })->{ branchcode };
>-my $guarantor = $builder->build({
>-    source => 'Borrower',
>-    value => {
>-        branchcode   => $branchcode,
>-        categorycode => $categorycode,
>-        flags        => 0,
>-    }
>-});
>-my $borrower = $builder->build({
>-    source => 'Borrower',
>-    value => {
>-        branchcode   => $branchcode,
>-        categorycode => $categorycode,
>-        flags        => 0,
>-        lost         => 1,
>-        guarantorid  => $guarantor->{borrowernumber},
>-    }
>-});
>-
>-$t->get_ok('/api/v1/patrons')
>-  ->status_is(401);
>-
>-$t->get_ok("/api/v1/patrons/" . $borrower->{ borrowernumber })
>-  ->status_is(401);
>-
>-my $session = C4::Auth::get_session('');
>-$session->param('number', $borrower->{ borrowernumber });
>-$session->param('id', $borrower->{ userid });
>-$session->param('ip', '127.0.0.1');
>-$session->param('lasttime', time());
>-$session->flush;
>-
>-my $session2 = C4::Auth::get_session('');
>-$session2->param('number', $guarantor->{ borrowernumber });
>-$session2->param('id', $guarantor->{ userid });
>-$session2->param('ip', '127.0.0.1');
>-$session2->param('lasttime', time());
>-$session2->flush;
>-
>-my $tx = $t->ua->build_tx(GET => '/api/v1/patrons');
>-$tx->req->cookies({name => 'CGISESSID', value => $session->id});
>-$t->request_ok($tx)
>-  ->status_is(403);
>-
>-$tx = $t->ua->build_tx(GET => "/api/v1/patrons/" . ($borrower->{ borrowernumber }-1));
>-$tx->req->cookies({name => 'CGISESSID', value => $session->id});
>-$t->request_ok($tx)
>-  ->status_is(403)
>-  ->json_is('/required_permissions', {"borrowers" => "edit_borrowers"});
>-
>-# User without permissions, but is the owner of the object
>-$tx = $t->ua->build_tx(GET => "/api/v1/patrons/" . $borrower->{borrowernumber});
>-$tx->req->cookies({name => 'CGISESSID', value => $session->id});
>-$t->request_ok($tx)
>-  ->status_is(200);
>-
>-# User without permissions, but is the guarantor of the owner of the object
>-$tx = $t->ua->build_tx(GET => "/api/v1/patrons/" . $borrower->{borrowernumber});
>-$tx->req->cookies({name => 'CGISESSID', value => $session2->id});
>-$t->request_ok($tx)
>-  ->status_is(200)
>-  ->json_is('/guarantorid', $guarantor->{borrowernumber});
>-
>-my $loggedinuser = $builder->build({
>-    source => 'Borrower',
>-    value => {
>+subtest 'list() tests' => sub {
>+    plan tests => 2;
>+
>+    $schema->storage->txn_begin;
>+
>+    unauthorized_access_tests('GET', undef, undef);
>+
>+    subtest 'librarian access tests' => sub {
>+        plan tests => 8;
>+
>+        my ($borrowernumber, $sessionid) = create_user_and_session({
>+            authorized => 1 });
>+        my $patron = Koha::Patrons->find($borrowernumber);
>+        Koha::Patrons->search({
>+            borrowernumber => { '!=' => $borrowernumber},
>+            cardnumber => { LIKE => $patron->cardnumber . "%" }
>+        })->delete;
>+        Koha::Patrons->search({
>+            borrowernumber => { '!=' => $borrowernumber},
>+            address2 => { LIKE => $patron->address2 . "%" }
>+        })->delete;
>+
>+        my $tx = $t->ua->build_tx(GET => '/api/v1/patrons');
>+        $tx->req->cookies({name => 'CGISESSID', value => $sessionid});
>+        $tx->req->env({REMOTE_ADDR => '127.0.0.1'});
>+        $t->request_ok($tx)
>+          ->status_is(200);
>+
>+        $tx = $t->ua->build_tx(GET => '/api/v1/patrons?cardnumber='.
>+                                  $patron->cardnumber);
>+        $tx->req->cookies({name => 'CGISESSID', value => $sessionid});
>+        $tx->req->env({REMOTE_ADDR => '127.0.0.1'});
>+        $t->request_ok($tx)
>+          ->status_is(200)
>+          ->json_is('/0/cardnumber' => $patron->cardnumber);
>+
>+        $tx = $t->ua->build_tx(GET => '/api/v1/patrons?address2='.
>+                                  $patron->address2);
>+        $tx->req->cookies({name => 'CGISESSID', value => $sessionid});
>+        $tx->req->env({REMOTE_ADDR => '127.0.0.1'});
>+        $t->request_ok($tx)
>+          ->status_is(200)
>+          ->json_is('/0/address2' => $patron->address2);
>+    };
>+
>+    $schema->storage->txn_rollback;
>+};
>+
>+subtest 'get() tests' => sub {
>+    plan tests => 3;
>+
>+    $schema->storage->txn_begin;
>+
>+    unauthorized_access_tests('GET', -1, undef);
>+
>+    subtest 'access own object tests' => sub {
>+        plan tests => 4;
>+
>+        my ($patronid, $patronsessionid) = create_user_and_session({
>+            authorized => 0 });
>+
>+        # Access patron's own data even though they have no borrowers flag
>+        my $tx = $t->ua->build_tx(GET => "/api/v1/patrons/$patronid");
>+        $tx->req->cookies({name => 'CGISESSID', value => $patronsessionid});
>+        $tx->req->env({REMOTE_ADDR => '127.0.0.1'});
>+        $t->request_ok($tx)
>+          ->status_is(200);
>+
>+        my $guarantee = $builder->build({
>+            source => 'Borrower',
>+            value  => {
>+                guarantorid => $patronid,
>+            }
>+        });
>+
>+        # Access guarantee's data even though guarantor has no borrowers flag
>+        my $guaranteenumber = $guarantee->{borrowernumber};
>+        $tx = $t->ua->build_tx(GET => "/api/v1/patrons/$guaranteenumber");
>+        $tx->req->cookies({name => 'CGISESSID', value => $patronsessionid});
>+        $tx->req->env({REMOTE_ADDR => '127.0.0.1'});
>+        $t->request_ok($tx)
>+          ->status_is(200);
>+    };
>+
>+    subtest 'librarian access tests' => sub {
>+        plan tests => 5;
>+
>+        my ($patron_id) = create_user_and_session({
>+            authorized => 0 });
>+        my $patron = Koha::Patrons->find($patron_id);
>+        my ($borrowernumber, $sessionid) = create_user_and_session({
>+            authorized => 1 });
>+        my $tx = $t->ua->build_tx(GET => "/api/v1/patrons/$patron_id");
>+        $tx->req->cookies({name => 'CGISESSID', value => $sessionid});
>+        $t->request_ok($tx)
>+          ->status_is(200)
>+          ->json_is('/borrowernumber' => $patron_id)
>+          ->json_is('/surname' => $patron->surname)
>+          ->json_is('/lost' => Mojo::JSON->false );
>+    };
>+
>+    $schema->storage->txn_rollback;
>+};
>+
>+subtest 'add() tests' => sub {
>+    plan tests => 2;
>+
>+    $schema->storage->txn_begin;
>+
>+    my $categorycode = $builder->build({ source => 'Category' })->{categorycode};
>+    my $branchcode = $builder->build({ source => 'Branch' })->{branchcode};
>+    my $newpatron = {
>+        address      => 'Street',
>         branchcode   => $branchcode,
>+        cardnumber   => $branchcode.$categorycode,
>         categorycode => $categorycode,
>-        flags        => 16 # borrowers flag
>-    }
>-});
>-
>-$session = C4::Auth::get_session('');
>-$session->param('number', $loggedinuser->{ borrowernumber });
>-$session->param('id', $loggedinuser->{ userid });
>-$session->param('ip', '127.0.0.1');
>-$session->param('lasttime', time());
>-$session->flush;
>-
>-$tx = $t->ua->build_tx(GET => '/api/v1/patrons');
>-$tx->req->cookies({name => 'CGISESSID', value => $session->id});
>-$tx->req->env({REMOTE_ADDR => '127.0.0.1'});
>-$t->request_ok($tx)
>-  ->status_is(200);
>-
>-$tx = $t->ua->build_tx(GET => "/api/v1/patrons/" . $borrower->{ borrowernumber });
>-$tx->req->cookies({name => 'CGISESSID', value => $session->id});
>-$t->request_ok($tx)
>-  ->status_is(200)
>-  ->json_is('/borrowernumber' => $borrower->{ borrowernumber })
>-  ->json_is('/surname' => $borrower->{ surname })
>-  ->json_is('/lost' => Mojo::JSON->true );
>-
>-$schema->storage->txn_rollback;
>+        city         => 'Joenzoo',
>+        surname      => "TestUser",
>+        userid       => $branchcode.$categorycode,
>+    };
>+
>+    unauthorized_access_tests('POST', undef, $newpatron);
>+
>+    subtest 'librarian access tests' => sub {
>+        plan tests => 18;
>+
>+        my ($borrowernumber, $sessionid) = create_user_and_session({
>+            authorized => 1 });
>+
>+        $newpatron->{branchcode} = "nonexistent"; # Test invalid branchcode
>+        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)
>+          ->json_is('/error' => "Given branchcode does not exist");
>+        $newpatron->{branchcode} = $branchcode;
>+
>+        $newpatron->{categorycode} = "nonexistent"; # Test invalid patron category
>+        $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)
>+          ->json_is('/error' => "Given categorycode does not exist");
>+        $newpatron->{categorycode} = $categorycode;
>+
>+        $newpatron->{falseproperty} = "Non existent property";
>+        $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);
>+        delete $newpatron->{falseproperty};
>+
>+        $tx = $t->ua->build_tx(POST => "/api/v1/patrons" => json => $newpatron);
>+        $tx->req->cookies({name => 'CGISESSID', value => $sessionid});
>+        $t->request_ok($tx)
>+          ->status_is(201, 'Patron created successfully')
>+          ->json_has('/borrowernumber', 'got a borrowernumber')
>+          ->json_is('/cardnumber', $newpatron->{ cardnumber })
>+          ->json_is('/surname' => $newpatron->{ surname })
>+          ->json_is('/firstname' => $newpatron->{ firstname });
>+        $newpatron->{borrowernumber} = $tx->res->json->{borrowernumber};
>+
>+        $tx = $t->ua->build_tx(POST => "/api/v1/patrons" => json => $newpatron);
>+        $tx->req->cookies({name => 'CGISESSID', value => $sessionid});
>+        $t->request_ok($tx)
>+          ->status_is(409)
>+          ->json_has('/error', 'Fails when trying to POST duplicate'.
>+                     ' cardnumber or userid')
>+          ->json_has('/conflict', {
>+                        userid => $newpatron->{ userid },
>+                        cardnumber => $newpatron->{ cardnumber }
>+                    }
>+            );
>+    };
>+
>+    $schema->storage->txn_rollback;
>+};
>+
>+subtest 'edit() tests' => sub {
>+    plan tests => 3;
>+
>+    $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 $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->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->{branchcode} = 'nonexistent';
>+        $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" .
>+                    $newpatron->{borrowernumber} => 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->{falseproperty} = "Non existent property";
>+        $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" .
>+                    $newpatron->{borrowernumber} => json => $newpatron);
>+        $tx->req->cookies({name => 'CGISESSID', value => $sessionid});
>+        $t->request_ok($tx)
>+          ->status_is(400)
>+          ->json_is('/errors/0/message' =>
>+                    '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 }
>+                }
>+            );
>+
>+        $newpatron->{ cardnumber } = $borrowernumber.$borrowernumber2;
>+        $newpatron->{ userid } = "user".$borrowernumber.$borrowernumber2;
>+        $newpatron->{ surname } = "user".$borrowernumber.$borrowernumber2;
>+
>+        $tx = $t->ua->build_tx(PUT => "/api/v1/patrons/" .
>+                    $newpatron->{borrowernumber} => json => $newpatron);
>+        $tx->req->cookies({name => 'CGISESSID', value => $sessionid});
>+        $t->request_ok($tx)
>+          ->status_is(200, 'Patron updated successfully')
>+          ->json_has($newpatron);
>+        is(Koha::Patrons->find($newpatron->{borrowernumber})->cardnumber,
>+           $newpatron->{ cardnumber }, 'Patron is really updated!');
>+    };
>+
>+    $schema->storage->txn_rollback;
>+};
>+
>+subtest 'delete() tests' => sub {
>+    plan tests => 2;
>+
>+    $schema->storage->txn_begin;
>+
>+    unauthorized_access_tests('DELETE', 123, undef);
>+
>+    subtest 'librarian access test' => sub {
>+        plan tests => 4;
>+
>+        my ($borrowernumber, $sessionid) = create_user_and_session({
>+            authorized => 1 });
>+        my ($borrowernumber2, $sessionid2) = create_user_and_session({
>+            authorized => 0 });
>+
>+        my $tx = $t->ua->build_tx(DELETE => "/api/v1/patrons/-1");
>+        $tx->req->cookies({name => 'CGISESSID', value => $sessionid});
>+        $t->request_ok($tx)
>+          ->status_is(404, 'Patron not found');
>+
>+        $tx = $t->ua->build_tx(DELETE => "/api/v1/patrons/$borrowernumber2");
>+        $tx->req->cookies({name => 'CGISESSID', value => $sessionid});
>+        $t->request_ok($tx)
>+          ->status_is(200, 'Patron deleted successfully');
>+    };
>+
>+    $schema->storage->txn_rollback;
>+};
>+
>+# Centralized tests for 401s and 403s assuming the endpoint requires
>+# borrowers flag for access
>+sub unauthorized_access_tests {
>+    my ($verb, $patronid, $json) = @_;
>+
>+    my $endpoint = '/api/v1/patrons';
>+    $endpoint .= ($patronid) ? "/$patronid" : '';
>+
>+    subtest 'unauthorized access tests' => sub {
>+        plan tests => 5;
>+
>+        my $tx = $t->ua->build_tx($verb => $endpoint => json => $json);
>+        $t->request_ok($tx)
>+          ->status_is(401);
>+
>+        my ($borrowernumber, $sessionid) = create_user_and_session({
>+            authorized => 0 });
>+
>+        $tx = $t->ua->build_tx($verb => $endpoint => json => $json);
>+        $tx->req->cookies({name => 'CGISESSID', value => $sessionid});
>+        $t->request_ok($tx)
>+          ->status_is(403)
>+          ->json_is('/required_permissions', {"borrowers" => "1"});
>+    };
>+}
>+
>+sub create_user_and_session {
>+
>+    my $args  = shift;
>+    my $flags = ( $args->{authorized} ) ? 16 : 0;
>+    my $dbh   = C4::Context->dbh;
>+
>+    my $user = $builder->build(
>+        {
>+            source => 'Borrower',
>+            value  => {
>+                flags => $flags,
>+                gonenoaddress => 0,
>+                lost => 0,
>+                email => 'nobody@example.com',
>+                emailpro => 'nobody@example.com',
>+                B_email => 'nobody@example.com',
>+            }
>+        }
>+    );
>+
>+    # Create a session for the authorized user
>+    my $session = C4::Auth::get_session('');
>+    $session->param( 'number',   $user->{borrowernumber} );
>+    $session->param( 'id',       $user->{userid} );
>+    $session->param( 'ip',       '127.0.0.1' );
>+    $session->param( 'lasttime', time() );
>+    $session->flush;
>+
>+    return ( $user->{borrowernumber}, $session->id );
>+}
>-- 
>2.1.4

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