From ccf0bfeb1096ce27786e3a5d755c3c6382ff866b Mon Sep 17 00:00:00 2001
From: Johanna Raisa <johanna.raisa@gmail.com>
Date: Tue, 9 Jul 2019 11:53:07 +0300
Subject: [PATCH] Bug 23285: Add patrons PATCH route

Implementation for patrons PATCH route.

Test plan
- Apply this patch
- Run prove t/db_dependent/api/v1/patrons.t
- Create user with borrowers permissions
- Authenticate with the user created above and get a CGISESSION id.
- Use the CGISESSION to patch patrons via the API
- PATCH /patrons/{patron_id} with some parameters in the body

Sponsored-by: Koha-Suomi Oy
---
 Koha/REST/V1/Patrons.pm                      |   2 +-
 api/v1/swagger/definitions.json              |   3 +
 api/v1/swagger/definitions/patron_patch.json | 283 +++++++++++++++++++
 api/v1/swagger/paths/patrons.json            |  78 +++++
 t/db_dependent/api/v1/patrons.t              |  66 ++++-
 5 files changed, 430 insertions(+), 2 deletions(-)
 create mode 100644 api/v1/swagger/definitions/patron_patch.json

diff --git a/Koha/REST/V1/Patrons.pm b/Koha/REST/V1/Patrons.pm
index 5e8e398eac..71b67171a7 100644
--- a/Koha/REST/V1/Patrons.pm
+++ b/Koha/REST/V1/Patrons.pm
@@ -198,7 +198,7 @@ sub update {
 
         $patron->set($body)->store;
         $patron->discard_changes;
-        return $c->render( status => 200, openapi => $patron );
+        return $c->render( status => 200, openapi => _to_api( $patron->TO_JSON ) );
     }
     catch {
         unless ( blessed $_ && $_->can('rethrow') ) {
diff --git a/api/v1/swagger/definitions.json b/api/v1/swagger/definitions.json
index e3001b5e60..4a7b6a8ab8 100644
--- a/api/v1/swagger/definitions.json
+++ b/api/v1/swagger/definitions.json
@@ -32,6 +32,9 @@
   "patron_balance": {
     "$ref": "definitions/patron_balance.json"
   },
+  "patron_patch": {
+    "$ref": "definitions/patron_patch.json"
+  },
   "allows_renewal": {
     "$ref": "definitions/allows_renewal.json"
   },
diff --git a/api/v1/swagger/definitions/patron_patch.json b/api/v1/swagger/definitions/patron_patch.json
new file mode 100644
index 0000000000..2f55493d12
--- /dev/null
+++ b/api/v1/swagger/definitions/patron_patch.json
@@ -0,0 +1,283 @@
+{
+  "type": "object",
+  "properties": {
+    "patron_id": {
+      "$ref": "../x-primitives.json#/patron_id"
+    },
+    "cardnumber": {
+      "$ref": "../x-primitives.json#/cardnumber"
+    },
+    "surname": {
+      "$ref": "../x-primitives.json#/surname"
+    },
+    "firstname": {
+      "$ref": "../x-primitives.json#/firstname"
+    },
+    "title": {
+      "type": ["string", "null"],
+      "description": "patron's title"
+    },
+    "other_name": {
+      "type": ["string", "null"],
+      "description": "any other names associated with the patron"
+    },
+    "initials": {
+      "type": ["string", "null"],
+      "description": "initials of the patron"
+    },
+    "street_number": {
+      "type": ["string", "null"],
+      "description": "street number of patron's primary address"
+    },
+    "street_type": {
+      "type": ["string", "null"],
+      "description": "street type of patron's primary address"
+    },
+    "address": {
+      "type": ["string", "null"],
+      "description": "first address line of patron's primary address"
+    },
+    "address2": {
+      "type": ["string", "null"],
+      "description": "second address line of patron's primary address"
+    },
+    "city": {
+      "type": ["string", "null"],
+      "description": "city or town of patron's primary address"
+    },
+    "state": {
+      "type": ["string", "null"],
+      "description": "state or province of patron's primary address"
+    },
+    "postal_code": {
+      "type": ["string", "null"],
+      "description": "zip or postal code of patron's primary address"
+    },
+    "country": {
+      "type": ["string", "null"],
+      "description": "country of patron's primary address"
+    },
+    "email": {
+      "$ref": "../x-primitives.json#/email"
+    },
+    "phone": {
+      "$ref": "../x-primitives.json#/phone"
+    },
+    "mobile": {
+      "type": ["string", "null"],
+      "description": "the other phone number for patron's primary address"
+    },
+    "fax": {
+      "type": ["string", "null"],
+      "description": "fax number for patron's primary address"
+    },
+    "secondary_email": {
+      "type": ["string", "null"],
+      "description": "secondary email address for patron's primary address"
+    },
+    "secondary_phone": {
+      "type": ["string", "null"],
+      "description": "secondary phone number for patron's primary address"
+    },
+    "altaddress_street_number": {
+      "type": ["string", "null"],
+      "description": "street number of patron's alternate address"
+    },
+    "altaddress_street_type": {
+      "type": ["string", "null"],
+      "description": "street type of patron's alternate address"
+    },
+    "altaddress_address": {
+      "type": ["string", "null"],
+      "description": "first address line of patron's alternate address"
+    },
+    "altaddress_address2": {
+      "type": ["string", "null"],
+      "description": "second address line of patron's alternate address"
+    },
+    "altaddress_city": {
+      "type": ["string", "null"],
+      "description": "city or town of patron's alternate address"
+    },
+    "altaddress_state": {
+      "type": ["string", "null"],
+      "description": "state or province of patron's alternate address"
+    },
+    "altaddress_postal_code": {
+      "type": ["string", "null"],
+      "description": "zip or postal code of patron's alternate address"
+    },
+    "altaddress_country": {
+      "type": ["string", "null"],
+      "description": "country of patron's alternate address"
+    },
+    "altaddress_email": {
+      "type": ["string", "null"],
+      "description": "email address for patron's alternate address"
+    },
+    "altaddress_phone": {
+      "type": ["string", "null"],
+      "description": "phone number for patron's alternate address"
+    },
+    "date_of_birth": {
+      "type": ["string", "null"],
+      "format": "date",
+      "description": "patron's date of birth"
+    },
+    "library_id": {
+      "type": "string",
+      "description": "Internal identifier for the patron's home library"
+    },
+    "category_id": {
+      "type": "string",
+      "description": "Internal identifier for the patron's category"
+    },
+    "date_enrolled": {
+      "type": ["string", "null"],
+      "format": "date",
+      "description": "date the patron was added to Koha"
+    },
+    "expiry_date": {
+      "type": ["string", "null"],
+      "format": "date",
+      "description": "date the patron's card is set to expire"
+    },
+    "date_renewed": {
+      "type": ["string", "null"],
+      "description": "date the patron's card was last renewed"
+    },
+    "incorrect_address": {
+      "type": ["boolean", "null"],
+      "description": "set to 1 if library marked this patron as having an unconfirmed address"
+    },
+    "patron_card_lost": {
+      "type": ["boolean", "null"],
+      "description": "set to 1 if library marked this patron as having lost his card"
+    },
+    "restricted": {
+      "type": "boolean",
+      "readOnly": true,
+      "description": "If any restriction applies to the patron"
+    },
+    "guarantor_id": {
+      "type": ["integer", "null"],
+      "description": "patron_id used for children or professionals to link them to guarantor or organizations"
+    },
+    "staff_notes": {
+      "type": ["string", "null"],
+      "description": "a note on the patron's account"
+    },
+    "relationship_type": {
+      "type": ["string", "null"],
+      "description": "used for children to include the relationship to their guarantor"
+    },
+    "gender": {
+      "type": ["string", "null"],
+      "description": "patron's gender"
+    },
+    "userid": {
+      "type": ["string", "null"],
+      "description": "patron's login"
+    },
+    "opac_notes": {
+      "type": ["string", "null"],
+      "description": "a note on the patron's account visible in OPAC and staff client"
+    },
+    "altaddress_notes": {
+      "type": ["string", "null"],
+      "description": "a note related to patron's alternate address"
+    },
+    "statistics_1": {
+      "type": ["string", "null"],
+      "description": "a field that can be used for any information unique to the library"
+    },
+    "statistics_2": {
+      "type": ["string", "null"],
+      "description": "a field that can be used for any information unique to the library"
+    },
+    "altcontact_firstname": {
+      "type": ["string", "null"],
+      "description": "first name of alternate contact for the patron"
+    },
+    "altcontact_surname": {
+      "type": ["string", "null"],
+      "description": "surname or last name of the alternate contact for the patron"
+    },
+    "altcontact_address": {
+      "type": ["string", "null"],
+      "description": "the first address line for the alternate contact for the patron"
+    },
+    "altcontact_address2": {
+      "type": ["string", "null"],
+      "description": "the second address line for the alternate contact for the patron"
+    },
+    "altcontact_city": {
+      "type": ["string", "null"],
+      "description": "the city for the alternate contact for the patron"
+    },
+    "altcontact_state": {
+      "type": ["string", "null"],
+      "description": "the state for the alternate contact for the patron"
+    },
+    "altcontact_postal_code": {
+      "type": ["string", "null"],
+      "description": "the zipcode for the alternate contact for the patron"
+    },
+    "altcontact_country": {
+      "type": ["string", "null"],
+      "description": "the country for the alternate contact for the patron"
+    },
+    "altcontact_phone": {
+      "type": ["string", "null"],
+      "description": "the phone number for the alternate contact for the patron"
+    },
+    "sms_number": {
+      "type": ["string", "null"],
+      "description": "the mobile phone number where the patron would like to receive notices (if SMS turned on)"
+    },
+    "sms_provider_id": {
+      "type": ["integer", "null"],
+      "description": "the provider of the mobile phone number defined in smsalertnumber"
+    },
+    "privacy": {
+      "type": "integer",
+      "description": "patron's privacy settings related to their reading history"
+    },
+    "privacy_guarantor_checkouts": {
+      "type": "integer",
+      "description": "controls if relatives can see this patron's checkouts"
+    },
+    "check_previous_checkout": {
+      "type": "string",
+      "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",
+      "format": "date-time",
+      "description": "time of last change could be useful for synchronization with external systems (among others)"
+    },
+    "last_seen": {
+      "type": ["string", "null"],
+      "format": "date-time",
+      "description": "last time a patron has been seen (connected at the OPAC or staff interface)"
+    },
+    "lang": {
+      "type": "string",
+      "description": "lang to use to send notices to this patron"
+    },
+    "login_attempts": {
+      "type": ["integer", "null"],
+      "description": "number of failed login attemps"
+    },
+    "overdrive_auth_token": {
+      "type": ["string", "null"],
+      "description": "persist OverDrive auth token"
+    },
+    "anonymized": {
+      "type": "boolean",
+      "readOnly": true,
+      "description": "If the patron has been anonymized"
+    }
+  },
+  "additionalProperties": false
+}
diff --git a/api/v1/swagger/paths/patrons.json b/api/v1/swagger/paths/patrons.json
index 27bd08f69f..ed522eec66 100644
--- a/api/v1/swagger/paths/patrons.json
+++ b/api/v1/swagger/paths/patrons.json
@@ -654,6 +654,84 @@
         }
       }
     },
+    "patch": {
+      "x-mojo-to": "Patrons#update",
+      "operationId": "patchPatron",
+      "tags": ["patrons"],
+      "parameters": [
+        {
+          "$ref": "../parameters.json#/patron_id_pp"
+        },
+        {
+          "name": "body",
+          "in": "body",
+          "description": "A JSON object containing new information about existing patron",
+          "required": true,
+          "schema": {
+            "$ref": "../definitions.json#/patron_patch"
+          }
+        }
+      ],
+      "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": {
+        "permissions": {
+          "borrowers": "1"
+        }
+      }
+    },
     "delete": {
       "x-mojo-to": "Patrons#delete",
       "operationId": "deletePatron",
diff --git a/t/db_dependent/api/v1/patrons.t b/t/db_dependent/api/v1/patrons.t
index 4f96a12430..5e66783bae 100644
--- a/t/db_dependent/api/v1/patrons.t
+++ b/t/db_dependent/api/v1/patrons.t
@@ -17,7 +17,7 @@
 
 use Modern::Perl;
 
-use Test::More tests => 5;
+use Test::More tests => 6;
 use Test::Mojo;
 use Test::Warn;
 
@@ -313,6 +313,70 @@ subtest 'update() tests' => sub {
     };
 };
 
+subtest 'patch() tests' => sub {
+    plan tests => 2;
+
+    $schema->storage->txn_begin;
+    unauthorized_access_tests('PATCH', 123, {email => 'nobody@example.com'});
+    $schema->storage->txn_rollback;
+
+    subtest 'librarian access tests' => sub {
+        plan tests => 15;
+
+        $schema->storage->txn_begin;
+
+        t::lib::Mocks::mock_preference('minPasswordLength', 1);
+        my ( $patron_id_1, $session_id ) = create_user_and_session({ authorized => 1 });
+        my ( $patron_id_2, undef )       = create_user_and_session({ authorized => 0 });
+
+        my $patron_1  = Koha::Patrons->find($patron_id_1);
+        my $patron_2  = Koha::Patrons->find($patron_id_2);
+        my $newpatron = {firstname => "myfirstname"};
+
+        my $tx = $t->ua->build_tx(PATCH => "/api/v1/patrons/-1" => json => $newpatron );
+        $tx->req->cookies({name => 'CGISESSID', value => $session_id});
+        $t->request_ok($tx)
+          ->status_is(404)
+          ->json_has('/error', 'Fails when trying to PATCH nonexistent patron');
+
+        $newpatron->{falseproperty} = "Non existent property";
+        $tx = $t->ua->build_tx(PATCH => "/api/v1/patrons/" . $patron_2->id => json => $newpatron );
+        $tx->req->cookies({name => 'CGISESSID', value => $session_id});
+        $t->request_ok($tx)
+          ->status_is(400)
+          ->json_is('/errors/0/message' =>
+                    'Properties not allowed: falseproperty.');
+        delete $newpatron->{falseproperty};
+
+        # Set both cardnumber and userid to already existing values
+        $newpatron->{cardnumber} = $patron_1->cardnumber;
+        $newpatron->{userid}     = $patron_1->userid;
+
+        $tx = $t->ua->build_tx( PATCH => "/api/v1/patrons/" . $patron_2->id => json => $newpatron );
+        $tx->req->cookies({ name => 'CGISESSID', value => $session_id });
+        warning_like {
+            $t->request_ok($tx)
+              ->status_is(409)
+              ->json_has( '/error' => "Fails when trying to update to an existing cardnumber or userid")
+              ->json_is(  '/conflict', 'cardnumber' ); }
+            qr/^DBD::mysql::st execute failed: Duplicate entry '(.*?)' for key 'cardnumber'/;
+
+        delete $newpatron->{cardnumber};
+        delete $newpatron->{userid};
+
+        #Update only firstname field
+        $tx = $t->ua->build_tx(PATCH => "/api/v1/patrons/" . $patron_2->id => json => $newpatron);
+        $tx->req->cookies({name => 'CGISESSID', value => $session_id});
+        $t->request_ok($tx)
+          ->status_is(200, 'Patron updated successfully')
+          ->json_has($newpatron);
+        is(Koha::Patrons->find( $patron_2->id )->firstname,
+           $newpatron->{ firstname }, 'Patron is really updated!');
+
+        $schema->storage->txn_rollback;
+    };
+};
+
 subtest 'delete() tests' => sub {
     plan tests => 2;
 
-- 
2.17.1