Bug 16330: Add routes to add, update and delete patrons
authorBenjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Wed, 27 Apr 2016 13:47:04 +0000 (13:47 +0000)
committerJonathan Druart <jonathan.druart@bugs.koha-community.org>
Thu, 29 Mar 2018 14:42:06 +0000 (11:42 -0300)
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>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Koha/Exceptions.pm
Koha/Exceptions/Category.pm [new file with mode: 0644]
Koha/Exceptions/Library.pm [new file with mode: 0644]
Koha/Exceptions/Patron.pm [new file with mode: 0644]
Koha/Patron.pm
Koha/REST/V1/Patron.pm
api/v1/swagger/definitions/patron.json
api/v1/swagger/paths/patrons.json
t/db_dependent/Koha/Patrons.t
t/db_dependent/api/v1/patrons.t

index 9d7f397..f49a289 100644 (file)
@@ -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 (file)
index 0000000..3487faa
--- /dev/null
@@ -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 (file)
index 0000000..1aea6ad
--- /dev/null
@@ -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 (file)
index 0000000..c409b4e
--- /dev/null
@@ -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;
index d0be998..faf4efa 100644 (file)
@@ -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>
index 8fbe458..763b325 100644 (file)
@@ -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;
index 2c6f9a6..948b956 100644 (file)
     },
     "dateofbirth": {
       "type": ["string", "null"],
+      "format": "date",
       "description": "patron's date of birth"
     },
     "branchcode": {
     },
     "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": {
     },
     "debarred": {
       "type": ["string", "null"],
+      "format": "date",
       "description": "until this date the patron can only check-in"
     },
     "debarredcomment": {
       "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": {
       "type": ["string", "null"],
       "description": "persist OverDrive auth token"
     }
-  }
+  },
+  "additionalProperties": false,
+  "required": ["surname", "address", "city", "branchcode", "categorycode"]
 }
index 565d20c..56218a4 100644 (file)
@@ -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",
           "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": {
       "operationId": "getPatron",
       "tags": ["patrons"],
       "parameters": [{
-          "$ref": "../parameters.json#/borrowernumberPathParam"
-        }
-      ],
+        "$ref": "../parameters.json#/borrowernumberPathParam"
+      }],
       "produces": [
-          "application/json"
+        "application/json"
       ],
       "responses": {
         "200": {
           "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"
+        }
+      }
     }
   }
 }
index c494241..01a1bd9 100644 (file)
@@ -1153,3 +1153,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;
+};
index 2166547..6389b5c 100644 (file)
 
 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 );
+}