Bug 16330 - Add routes to add, update and delete patrons
Summary: Add routes to add, update and delete patrons
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: REST api (show other bugs)
Version: master
Hardware: All All
: P5 - low new feature (vote)
Assignee: Benjamin Rokseth
QA Contact: Josef Moravec
URL:
Whiteboard:
Keywords:
Depends on: 14868 16699 18330 19410 19828 19841
Blocks: 16652 19784
  Show dependency treegraph
 
Reported: 2016-04-25 08:20 UTC by Benjamin Rokseth
Modified: 2018-05-14 13:55 UTC (History)
18 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Medium patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:
REST API route for managing patrons in database. Adds CRUD implementation for creating, reading, updating and deleting patrons, as well as listing with optional sorting. Follows new guidelines from REST API RFC and Koha Object Exceptions regarding validation and error handling. Patrons can modify and delete their own object, or anyone with borrower modification permissions.


Attachments
Bug 16330 - REST API: add routes to add, update and delete patrons (16.90 KB, patch)
2016-04-27 13:59 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16330 - REST API: add routes to add, update and delete patrons (17.91 KB, patch)
2016-04-28 09:56 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16330 - REST API: add routes to add, update and delete patrons (19.51 KB, patch)
2016-04-30 22:26 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16330: REST API: add routes to add, update and delete patrons (19.75 KB, patch)
2016-05-03 11:36 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 16330: (followup) fix spelling and number of tests (1.23 KB, patch)
2016-05-03 11:36 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16330: REST API: add routes to add, update and delete patrons (19.75 KB, patch)
2016-07-08 10:44 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16330: (followup) fix spelling and number of tests (1.31 KB, patch)
2016-07-08 10:44 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 16330: (followup) fix spelling and number of tests (1.30 KB, patch)
2016-07-31 09:58 UTC, Jiri Kozlovsky
Details | Diff | Splinter Review
Bug 16330: REST API: add routes to add, update and delete patrons (21.63 KB, patch)
2016-08-26 22:43 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16330: REST API: add routes to add, update and delete patrons (21.63 KB, patch)
2016-09-02 22:11 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16330: (QA-follow-up) Fix permission for DELETE (1.80 KB, patch)
2016-10-19 14:51 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16330: (follow-up) Add Test::Fatal Perl dependency (889 bytes, patch)
2016-10-19 14:52 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16330: (follow-up) Move validation logic from controller to Koha::Patron (16.21 KB, patch)
2016-10-19 14:52 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16330: REST API: add routes to add, update and delete patrons (61.30 KB, patch)
2017-05-23 12:39 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16330: Spot unchanged modification-request in Koha::Patron::Modification (5.16 KB, patch)
2017-05-23 12:39 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16330: REST API: add routes to add, update and delete patrons (61.21 KB, patch)
2017-05-23 12:45 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16330: Spot unchanged modification-request in Koha::Patron::Modification (5.16 KB, patch)
2017-05-23 12:45 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 16330: REST API: add routes to add, update and delete patrons (60.42 KB, patch)
2017-12-05 19:44 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16330: Spot unchanged modification-request in Koha::Patron::Modification (5.17 KB, patch)
2017-12-05 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16330: Move patches to OpenAPI (43.16 KB, patch)
2017-12-05 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16330: (QA followup) fix spelling in comment (1004 bytes, patch)
2017-12-05 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16330: (QA followup) Add missing POD for exceptions (4.69 KB, patch)
2017-12-05 19:45 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16330: (QA followup) Repect guidelines on plurals Patron > Patrons (2.23 KB, patch)
2017-12-05 19:46 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16330: (QA follow-up) ModMember requires borrowernumber (2.90 KB, patch)
2017-12-12 21:56 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16330: REST API: add routes to add, update and delete patrons (60.43 KB, patch)
2017-12-13 08:30 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16330: Spot unchanged modification-request in Koha::Patron::Modification (5.23 KB, patch)
2017-12-13 08:30 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16330: Move patches to OpenAPI (43.21 KB, patch)
2017-12-13 08:30 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16330: (QA followup) fix spelling in comment (1.04 KB, patch)
2017-12-13 08:30 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16330: (QA followup) Add missing POD for exceptions (4.74 KB, patch)
2017-12-13 08:31 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16330: (QA followup) Repect guidelines on plurals Patron > Patrons (2.28 KB, patch)
2017-12-13 08:31 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16330: (QA follow-up) ModMember requires borrowernumber (2.96 KB, patch)
2017-12-13 08:31 UTC, Benjamin Rokseth
Details | Diff | Splinter Review
Bug 16330: (QA follow-up) ModMember requires borrowernumber (2.81 KB, patch)
2017-12-14 05:12 UTC, Dilan Johnpullé
Details | Diff | Splinter Review
Bug 16330: Add routes to add, update and delete patrons (60.48 KB, patch)
2017-12-21 14:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16330: Move patches to OpenAPI (43.23 KB, patch)
2017-12-21 14:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16330: (QA follow-up) Repect guidelines on plurals Patron > Patrons (2.35 KB, patch)
2017-12-21 14:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16330: (QA follow-up) ModMember requires borrowernumber (2.82 KB, patch)
2017-12-21 14:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16330: Remove validation code from Koha::Patron (20.52 KB, patch)
2017-12-21 14:43 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16330: Add routes to add, update and delete patrons (60.55 KB, patch)
2018-02-15 15:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16330: Move patches to OpenAPI (43.60 KB, patch)
2018-02-15 15:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16330: (QA follow-up) Repect guidelines on plurals Patron > Patrons (2.35 KB, patch)
2018-02-15 15:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16330: (QA follow-up) ModMember requires borrowernumber (2.82 KB, patch)
2018-02-15 15:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16330: Remove validation code from Koha::Patron (20.22 KB, patch)
2018-02-15 15:37 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 16330: Add routes to add, update and delete patrons (60.33 KB, patch)
2018-02-19 08:43 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 16330: Move patches to OpenAPI (43.58 KB, patch)
2018-02-19 08:43 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 16330: (QA follow-up) Repect guidelines on plurals Patron > Patrons (2.34 KB, patch)
2018-02-19 08:43 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 16330: (QA follow-up) ModMember requires borrowernumber (2.81 KB, patch)
2018-02-19 08:43 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 16330: Remove validation code from Koha::Patron (20.03 KB, patch)
2018-02-19 08:43 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 16330: (QA follow-up) Update usage of search plugin because of bug 19686 (1.02 KB, patch)
2018-02-19 08:43 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 16330: Add routes to add, update and delete patrons (60.38 KB, patch)
2018-02-19 11:34 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 16330: Move patches to OpenAPI (43.64 KB, patch)
2018-02-19 11:34 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 16330: (QA follow-up) Repect guidelines on plurals Patron > Patrons (2.39 KB, patch)
2018-02-19 11:34 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 16330: (QA follow-up) ModMember requires borrowernumber (2.86 KB, patch)
2018-02-19 11:34 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 16330: Remove validation code from Koha::Patron (20.08 KB, patch)
2018-02-19 11:34 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 16330: (QA follow-up) Update usage of search plugin because of bug 19686 (1.08 KB, patch)
2018-02-19 11:34 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 16330: Remove unecessary Koha::Patron->store method (796 bytes, patch)
2018-03-29 15:46 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Benjamin Rokseth 2016-04-25 08:20:31 UTC
With the great work done on Koha Objects and the new restful API, adding a simple CRUD service for patron management shouldn't be too cumbersome.

This bug intends to add routes to add, edit and delete patrons:

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

for reference: https://wiki.koha-community.org/wiki/New_REST_API_RFC

it would also deprecate Bug 13607 - Patron management API
https://wiki.koha-community.org/wiki/Talk:Koha_/svc/_HTTP_API
Comment 1 Indranil Das Gupta 2016-04-25 09:57:12 UTC
How about the READ call?
Comment 2 Benjamin Rokseth 2016-04-25 10:07:19 UTC
(In reply to Indranil Das Gupta from comment #1)
> How about the READ call?

Hi,

READ is already implemented in Master. (Koha/REST/V1/Patrons.pm)
Comment 3 Benjamin Rokseth 2016-04-27 13:59:25 UTC Comment hidden (obsolete)
Comment 4 Katrin Fischer 2016-04-27 22:45:43 UTC
Please avoid the use of Switch and use if...elsif instead! 

Switch was first released with perl v5.7.3, deprecated (will be CPAN-only) in v5.11.0 and removed from v5.13.1
Comment 5 Benjamin Rokseth 2016-04-28 09:56:47 UTC Comment hidden (obsolete)
Comment 6 Benjamin Rokseth 2016-04-28 09:58:38 UTC
(In reply to Katrin Fischer from comment #4)
> Please avoid the use of Switch and use if...elsif instead! 
> 
> Switch was first released with perl v5.7.3, deprecated (will be CPAN-only)
> in v5.11.0 and removed from v5.13.1

Replaced Switch with if-elsif.
Also added a simple exception handling on POST/PUT with invalid params
Comment 7 Benjamin Rokseth 2016-04-30 22:26:11 UTC Comment hidden (obsolete)
Comment 8 Benjamin Rokseth 2016-04-30 22:31:33 UTC
> GET  /api/v1/patrons                   Get patron list from params
> GET  /api/v1/patrons/<borrowernumber>  Get single patron

Added possibiblity to search for patrons by any patron properties.

e.g. 
GET /api/v1/patrons?cardnumber=<mycardnumber>
GET /api/v1/patrons?branchcode=<mybranchcode>&categorycode=S

should give an array of matching patrons or empty array if none.
Comment 9 Bernardo Gonzalez Kriegel 2016-05-03 11:36:02 UTC Comment hidden (obsolete)
Comment 10 Bernardo Gonzalez Kriegel 2016-05-03 11:36:35 UTC Comment hidden (obsolete)
Comment 11 Benjamin Rokseth 2016-07-08 10:44:25 UTC Comment hidden (obsolete)
Comment 12 Benjamin Rokseth 2016-07-08 10:44:34 UTC Comment hidden (obsolete)
Comment 13 Jiri Kozlovsky 2016-07-31 09:58:33 UTC Comment hidden (obsolete)
Comment 14 Jiri Kozlovsky 2016-08-01 11:57:03 UTC
Comment on attachment 53820 [details] [review]
[SIGNED-OFF] Bug 16330: (followup) fix spelling and number of tests

Changed description to have SIGNED-OFF included
Comment 15 Benjamin Rokseth 2016-08-04 09:29:00 UTC
Comment on attachment 53217 [details] [review]
[SIGNED-OFF] Bug 16330: (followup) fix spelling and number of tests

same patch occurring twice
Comment 16 Marcel de Rooy 2016-08-15 07:20:51 UTC
my @valid_params = Koha::Patrons->_resultset->result_source->columns;

Normally, we should not touch the private underscored subs like _resultset somewhere else?
We also have _columns in Koha/Object. Should that be made public?

In this specific case I even wonder if you should do this validation here in REST or just pass the params to Objects (Patrons) and validate there if needed.
Would it be unexpected to return nothing if you search for an unexisting column?
Comment 17 Marcel de Rooy 2016-08-15 07:34:46 UTC
(In reply to Jiri Kozlovsky from comment #14)
> Changed description to have SIGNED-OFF included

This is not strictly necessary. A lot of reports go without it.
Comment 18 Lari Taskula 2016-08-15 09:42:21 UTC
(In reply to Marcel de Rooy from comment #16)
> my @valid_params = Koha::Patrons->_resultset->result_source->columns;
> 
> Normally, we should not touch the private underscored subs like _resultset
> somewhere else?
> We also have _columns in Koha/Object. Should that be made public?
Also there is a "columns" sub in Koha::Objets from Bug 16889.
Comment 19 Benjamin Rokseth 2016-08-26 22:43:02 UTC Comment hidden (obsolete)
Comment 20 Benjamin Rokseth 2016-08-26 22:47:47 UTC
Rebased against 16699 (now in master) and 14868 (Swagger permission checking).

Also merged in followup patch fixing spelling and tests, and now using Koha::Patrons->columns to test params against, as it was introduced in Bug 16889.
Comment 21 Jonathan Druart 2016-08-29 08:29:20 UTC
Are you sure we want to raise such specific error like "library does not exist" or "category does not exist"?
I'd go for an eval { $patron->store } and return a 500 if something went wrong.
Comment 22 Jonathan Druart 2016-08-29 09:11:27 UTC
Comment on attachment 54942 [details] [review]
Bug 16330: REST API: add routes to add, update and delete patrons

Review of attachment 54942 [details] [review]:
-----------------------------------------------------------------

::: Koha/REST/V1/Patron.pm
@@ +31,5 @@
> +    my $patrons;
> +    if (keys %$params) {
> +        my @valid_params = Koha::Patrons->columns;
> +        foreach my $key (keys %$params) {
> +            delete $params->{$key} unless grep { $key eq $_ } @valid_params;

Should not we raise an error instead of removing invalid params?
Comment 23 Jonathan Druart 2016-08-29 09:13:09 UTC
Comment on attachment 54942 [details] [review]
Bug 16330: REST API: add routes to add, update and delete patrons

Review of attachment 54942 [details] [review]:
-----------------------------------------------------------------

::: Koha/REST/V1/Patron.pm
@@ +85,5 @@
> +        Koha::Patron->new($body)->store;
> +    };
> +
> +    unless ($patron) {
> +        return $c->$cb({error => "Something went wrong, check Koha logs for details"}, 500);

You surrounded the store with an eval, so nothing will be logged.
Comment 24 Jonathan Druart 2016-08-29 09:28:10 UTC
Comment on attachment 54942 [details] [review]
Bug 16330: REST API: add routes to add, update and delete patrons

Review of attachment 54942 [details] [review]:
-----------------------------------------------------------------

::: Koha/REST/V1/Patron.pm
@@ +172,5 @@
> +    } elsif ($res eq '-1') {
> +        return $c->$cb({}, 404);
> +    } else {
> +        return $c->$cb({}, 400);
> +    }

It may be better to surround the ->delete with an eval, returns 200 if == 1 or log the error and return 500, don't you think?
Comment 25 Jonathan Druart 2016-08-29 09:29:06 UTC
(In reply to Jonathan Druart from comment #24)
> Comment on attachment 54942 [details] [review] [review]
> Bug 16330: REST API: add routes to add, update and delete patrons
> 
> Review of attachment 54942 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/REST/V1/Patron.pm
> @@ +172,5 @@
> > +    } elsif ($res eq '-1') {
> > +        return $c->$cb({}, 404);
> > +    } else {
> > +        return $c->$cb({}, 400);
> > +    }
> 
> It may be better to surround the ->delete with an eval, returns 200 if == 1
> or log the error and return 500, don't you think?

Or 400 instead.
Comment 26 Benjamin Rokseth 2016-08-29 09:31:00 UTC
(In reply to Jonathan Druart from comment #21)
> Are you sure we want to raise such specific error like "library does not
> exist" or "category does not exist"?
> I'd go for an eval { $patron->store } and return a 500 if something went
> wrong.


Well, yes and no. If missing category or branch is not part of error response, it is no good. API user must not be expected to know about koha internal dependencies, so a generic internal server response is not a good option.

That being said, the Patron object is probably the best place to handle exceptions, e.g. Bug 16907. So if patron->store gives the needed feedback, it is no point duplicating code/logic in api.

So yes, I agree, but am a bit unsure about alternatives?
Comment 27 Benjamin Rokseth 2016-08-29 09:34:58 UTC
(In reply to Jonathan Druart from comment #22)
> Comment on attachment 54942 [details] [review] [review]
> Bug 16330: REST API: add routes to add, update and delete patrons
> 
> Review of attachment 54942 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/REST/V1/Patron.pm
> @@ +31,5 @@
> > +    my $patrons;
> > +    if (keys %$params) {
> > +        my @valid_params = Koha::Patrons->columns;
> > +        foreach my $key (keys %$params) {
> > +            delete $params->{$key} unless grep { $key eq $_ } @valid_params;
> 
> Should not we raise an error instead of removing invalid params?

Absolutely right! Actually, it might be better to use formData and let swagger definitions do the validation.
Comment 28 Benjamin Rokseth 2016-08-29 09:36:05 UTC
(In reply to Jonathan Druart from comment #25)
> (In reply to Jonathan Druart from comment #24)
> > Comment on attachment 54942 [details] [review] [review] [review]
> > Bug 16330: REST API: add routes to add, update and delete patrons
> > 
> > Review of attachment 54942 [details] [review] [review] [review]:
> > -----------------------------------------------------------------
> > 
> > ::: Koha/REST/V1/Patron.pm
> > @@ +172,5 @@
> > > +    } elsif ($res eq '-1') {
> > > +        return $c->$cb({}, 404);
> > > +    } else {
> > > +        return $c->$cb({}, 400);
> > > +    }
> > 
> > It may be better to surround the ->delete with an eval, returns 200 if == 1
> > or log the error and return 500, don't you think?
> 
> Or 400 instead.

yes. Thanks for thorough feedback, btw!
Comment 29 Jonathan Druart 2016-08-29 09:41:26 UTC
About tests: 1 subtest per route you test would be better for the readability.
Comment 30 Jonathan Druart 2016-08-29 09:45:34 UTC
(In reply to Benjamin Rokseth from comment #26)
> (In reply to Jonathan Druart from comment #21)
> > Are you sure we want to raise such specific error like "library does not
> > exist" or "category does not exist"?
> > I'd go for an eval { $patron->store } and return a 500 if something went
> > wrong.
> 
> 
> Well, yes and no. If missing category or branch is not part of error
> response, it is no good. API user must not be expected to know about koha
> internal dependencies, so a generic internal server response is not a good
> option.
> 
> That being said, the Patron object is probably the best place to handle
> exceptions, e.g. Bug 16907. So if patron->store gives the needed feedback,
> it is no point duplicating code/logic in api.
> 
> So yes, I agree, but am a bit unsure about alternatives?

Actually if you call $patron->store and a FK is wrong/missing, DBIx::Class will raise the relevant DBI error, something like:

DBD::mysql::st execute failed: Cannot add or update a child row: a foreign key constraint fails (`koha_ut`.`borrowers`, CONSTRAINT `borrowers_ibfk_1` FOREIGN KEY (`categorycode`) REFERENCES `categories` (`categorycode`)).

That's why I think we should not handle it somewhere else.

Of course it would be better to get something more readable for the response :)
Did you mean we have to answer "This branchcode XYZ does not exist" or something like "One of the parameter is wrong/missing" would be enough?
Comment 31 Benjamin Rokseth 2016-09-02 22:11:34 UTC
Created attachment 55147 [details] [review]
Bug 16330: REST API: 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 (on top of dependent bug 14868)
2) Run minifySwagger to create swagger.min.json and restart Plack
3) Run tests perl t/db_dependent/api/v1/patrons.t
4) Add a user with proper rights to use the REST API
5) 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.

Please note there is no validation of body input in PUT/POST other
than branchcode,category,userid,cardnumber.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>

On top of 15126
Tested using postman, first try with the api
GET, POST, PUT and DELETE works, i.e. list, add, update, search and delete.
Small errors fixed in followup.
Comment 32 Tomás Cohen Arazi 2016-09-05 12:40:49 UTC
The DELETE permissions are wrong. It should be 'borrowers' only, and probably some more checks need to be done (e.g. IndependentBranches).
Comment 33 Lari Taskula 2016-10-19 14:51:39 UTC
Created attachment 56666 [details] [review]
Bug 16330: (QA-follow-up) Fix permission for DELETE

DELETE should not be allowed for user himself unless he has borrowers permission.

Covered by a test.

To test:
1. Apply patch
2. Run t/db_dependent/api/v1/patrons.t
Comment 34 Lari Taskula 2016-10-19 14:52:10 UTC
Created attachment 56667 [details] [review]
Bug 16330: (follow-up) Add Test::Fatal Perl dependency

Test::Fatal is a simple module for testing exception-based code.
Comment 35 Lari Taskula 2016-10-19 14:52:26 UTC
Created attachment 56668 [details] [review]
Bug 16330: (follow-up) Move validation logic from controller to Koha::Patron

This patch moves validation logic from controller into Koha::Patron object and
takes advantage of Koha::Exceptions for throwing proper exceptions and catching
them inside controller in order to return correct error messages.

To test:
1. Apply this patch
2. Run tests perl t/db_dependent/api/v1/patrons.t
3. Follow the original test plan and observe it still applying
Comment 36 Tomás Cohen Arazi 2016-10-19 15:12:33 UTC
Lari, I don't think we should have such a 'validate' method. We should look at Jonathan's code to move C4::Members methods into Koha::Patron(s) and have them raise those exceptions, and build the endpoint on top of those.
Please contact him if you are willing to help him and make this happen.
Comment 37 Lari Taskula 2016-10-20 11:04:38 UTC
Perhaps validate method can be useful for that work too, as a centralized location for patron information validation with proper exceptions, depending on how we want to do it.

OPACPatronDetails should also be checked for PUT.

By the way, I have noticed that these "OPAC-preferences" make it annoying to check if user is accessing as an owner-of-the-object without actual permissions, in order to perform some specific operation on that user. To make this checking easy inside controllers, perhaps we could set a flag in Koha/REST/V1.pm for users without permission but being the owner of the object.

before:
if (C4::Context->preference('OPACPatronDetails') && !haspermission($user->userid, { borrowers => 1 }) && $user->borrowernumber == $args->{borrowernumber}) {
// accessing as an owner, set patron modification to be verified by a librarian
}

after:
if (C4::Context->preference('OPACPatronDetails') && $c->stash('access_as_object_owner')) {
// accessing as an owner, set patron modification to be verified by a librarian
}

How does this sound? Is there an alternative, better solution or shall I open a new Bug for this?
Comment 38 Jiri Kozlovsky 2016-12-11 18:54:45 UTC
(In reply to Tomás Cohen Arazi from comment #36)
> Lari, I don't think we should have such a 'validate' method. We should look
> at Jonathan's code to move C4::Members methods into Koha::Patron(s) and have
> them raise those exceptions, and build the endpoint on top of those.
> Please contact him if you are willing to help him and make this happen.

I think, that moving all of the C4::Members methods into Koha::Patron is an overkill for this bug. There should be created bug for methods migration from C4 to Koha Objects generally.

If we focus on the goal of this bug - which is adding REST API routes for adding, updating and deleting patrons, we will find only these methods from C4::Members to be dependent, thus migrated:
ModMember
changepassword

AddMember
AddMember_Opac

Note that "changepassword" has already been migrated as a part of bug 17006.

My opinion is also that there shouldn't be general "validate" method, because the validation process depends on what you want to validate.

For example, you are checking for borrowernumber being non-existent in the DB, but it's contra productive if you want to validate input while updating patron. Because this is the case you want to check it's existent. These tiny differences are making it more difficult to maintain single version of validation.

Btw I think having stashed user being owner would be very nice like Lari suggested. Perhaps we should file a new bug.
Comment 39 Josef Moravec 2016-12-23 12:04:13 UTC
(In reply to Tomás Cohen Arazi from comment #36)
> Lari, I don't think we should have such a 'validate' method. We should look
> at Jonathan's code to move C4::Members methods into Koha::Patron(s) and have
> them raise those exceptions, and build the endpoint on top of those.
> Please contact him if you are willing to help him and make this happen.

Marking this as blocked because bug 16846
Comment 40 Tomás Cohen Arazi 2017-03-22 22:32:41 UTC
(In reply to Josef Moravec from comment #39)
> (In reply to Tomás Cohen Arazi from comment #36)
> > Lari, I don't think we should have such a 'validate' method. We should look
> > at Jonathan's code to move C4::Members methods into Koha::Patron(s) and have
> > them raise those exceptions, and build the endpoint on top of those.
> > Please contact him if you are willing to help him and make this happen.
> 
> Marking this as blocked because bug 16846

I didn't say this bug should depend on the C4::Members removal. I'm just saying that the natural way is to have the relevant methods in Koha::Patrons raise the obvious exceptions and capture them.

I am all for having an API regardless of how our classes implement stuff inside, otherwise we would be stuck. But a lot of work has been done to make the codebase more coherent (in out current standards), and a lot of it is waiting for people's feedback and QA. I think it is worth mentioning, because it would mean less work on this complicated-to-implement endpoints; as Lari mentions, all the preferences that come into play are really annoying, and we need a place to put all that together and properly test. Jonathan's work on the methods mo to Koha:: namespace is a good starting point.

About the validate method: I'd override ->store() and perform the validation there (so no need to call it explicitly). Look at bug 17828 for an example of that.
Comment 41 Lari Taskula 2017-03-24 10:27:05 UTC
I support moving validations into store(). Tests also need rewriting. I have done much of this work already and would like to attach it, but we also discussed Swagger parameter naming convention with Tomás (borrowernumber vs patron_id, branchcode vs library_id and so on) and it should be decided and properly implemented before moving on. Please see my e-mail in koha-devel. Switched this Bug to in discussion.
Comment 42 Magnus Enger 2017-05-03 11:55:38 UTC
What needs to happen for this bug to get moving?
Comment 43 Mirko Tietgen 2017-05-17 10:39:04 UTC
(In reply to Magnus Enger from comment #42)
> What needs to happen for this bug to get moving?

I second Magnus' question. What is the status here?
Comment 44 Marcel de Rooy 2017-05-17 11:01:43 UTC
(In reply to Mirko Tietgen from comment #43)
> (In reply to Magnus Enger from comment #42)
> > What needs to happen for this bug to get moving?
> 
> I second Magnus' question. What is the status here?

The last time, Lari changed this to In Discussion..
Comment 45 Lari Taskula 2017-05-22 08:43:48 UTC
Hi! Sorry for the late response. My view for the todo:

1. "Move validation logic from controller to Koha::Patron" patch; obsolete validate() method - validations should occur in Koha::Patron->store().
2. REST tests for this endpoint at t/db_dependent/api/v1/patrons.t is a big mess and should be rewritten to enhance readability and clearly separate tests of different operations, a good example is in cities.t.
3. Consider dependency to Bug 18137. Code-wise not a big change and required sooner or later as Swagger2 plugin is deprecated.
4. We discussed parameter naming convention in koha-devel (borrowernumber vs patron_id). Not sure if this was also discussed in a meeting, but I was left with the feeling that this is wanted by the community. However, could this be handled in another Bug to get this one moving?

I have worked on 1. and 2. and have a patch for that, however the work was paused by discussion on 4. I will re-test the patch and attach it here.
Comment 46 Lari Taskula 2017-05-23 12:16:32 UTC
(In reply to Lari Taskula from comment #45)
> Hi! Sorry for the late response. My view for the todo:
> 
> 1. "Move validation logic from controller to Koha::Patron" patch; obsolete
> validate() method - validations should occur in Koha::Patron->store().
I'm starting to remember the issues with this and why I implemented a separate validate method. When patron makes a modification request to be verified by librarian, we have to validate the data provided by patron without storing the object and then create a new modification request if provided data is ok. (PUT /patrons/123 where 123 is his own borrowernumber, and they don't have borrowers flag. This functionality is not yet in these patches provided before this comment.)

So what I did was in order to avoid validation logic duplication in Koha::Patron::Modification was:
$patron->set($provided_data)->validate;
# if are able to continue, store new Koha::Patron::Modification request

Of course we can have both, a separate validate method and validations integrated in store().

Also my fear was to throw exceptions in widely used method like Koha::Patron->store which would probably break elsewhere in Koha where these exceptions are not yet caught. So this Bug is a lot more problematic to pass. I took a shortcut and excluded the validations from store(). However, as a long term solution, like Tomas suggested, store() is the right place but I think that is too much for the scope of this Bug. Unfortunately I can't work with Koha::Patron more now.

Anyway, I have rewritten the tests as explained earlier, and added all of the patron parameteres to be queryable by GET /patrons. Also I added patron modification request to the PUT request. This is provided in the following patch.
Comment 47 Lari Taskula 2017-05-23 12:39:05 UTC
Created attachment 63646 [details] [review]
Bug 16330: REST API: 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.
Comment 48 Lari Taskula 2017-05-23 12:39:29 UTC
Created attachment 63647 [details] [review]
Bug 16330: Spot unchanged modification-request in Koha::Patron::Modification

Throw Koha::Exceptions::NoChanges if attempting to create a modification request
without changing anything.

This exception is caught in Patron REST API controller.

Also, validate changes via Koha::Patron->_validate

To test:
1. prove t/db_dependent/Koha/Patron/Modifications.t
Comment 49 Lari Taskula 2017-05-23 12:45:20 UTC
Created attachment 63649 [details] [review]
Bug 16330: REST API: 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.
Comment 50 Lari Taskula 2017-05-23 12:45:33 UTC
Created attachment 63650 [details] [review]
Bug 16330: Spot unchanged modification-request in Koha::Patron::Modification

Throw Koha::Exceptions::NoChanges if attempting to create a modification request
without changing anything.

This exception is caught in Patron REST API controller.

Also, validate changes via Koha::Patron->_validate

To test:
1. prove t/db_dependent/Koha/Patron/Modifications.t
Comment 51 Blou 2017-06-19 16:31:18 UTC
If i understand correctly, this patch is "In Discussion" since March(2017) because of "borrowernumber vs patron_id".

Is it still on?  Has it been settled?  I'd love to test (live) this code, but I don't want to tell our associates to deploy on a naming convention that will change.  

Ready to sign off...
Comment 52 Tomás Cohen Arazi 2017-11-24 17:16:02 UTC
(In reply to Blou from comment #51)
> If i understand correctly, this patch is "In Discussion" since March(2017)
> because of "borrowernumber vs patron_id".
> 
> Is it still on?  Has it been settled?  I'd love to test (live) this code,
> but I don't want to tell our associates to deploy on a naming convention
> that will change.  
> 
> Ready to sign off...

On my opinion this patches went too far (patron modifications are a different entity) and updates should be constrained to authorized (by permission) users.

Cleanup is needed, and migration to OpenAPI plugin too.

Will try to work on this ASAP. Anyone willing to do it or help, please pm me
Comment 53 Tomás Cohen Arazi 2017-12-05 19:44:59 UTC
Created attachment 69546 [details] [review]
Bug 16330: REST API: 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>
Comment 54 Tomás Cohen Arazi 2017-12-05 19:45:14 UTC
Created attachment 69547 [details] [review]
Bug 16330: Spot unchanged modification-request in Koha::Patron::Modification

Throw Koha::Exceptions::NoChanges if attempting to create a modification request
without changing anything.

This exception is caught in Patron REST API controller.

Also, validate changes via Koha::Patron->_validate

To test:
1. prove t/db_dependent/Koha/Patron/Modifications.t
Comment 55 Tomás Cohen Arazi 2017-12-05 19:45:26 UTC
Created attachment 69548 [details] [review]
Bug 16330: Move patches to OpenAPI

This patch refactors the original work so it implements the controllers
and the spec using Mojolicious::Plugin::OpenAPI, and OpenAPI for the specification.

It removes the ability for patrons without permissions to edit their own data or their
guarantee's. This will be moved to a patron modification requests endpoint for simplicity.

It makes use of bugs 19410 and 19686 and their dependencies to deal with parameters handling,
query building and pagination.

Tests are adapted.

To test:
- Apply this patches and the dependencies
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 56 Tomás Cohen Arazi 2017-12-05 19:45:42 UTC
Created attachment 69549 [details] [review]
Bug 16330: (QA followup) fix spelling in comment

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 57 Tomás Cohen Arazi 2017-12-05 19:45:53 UTC
Created attachment 69550 [details] [review]
Bug 16330: (QA followup) Add missing POD for exceptions

This patch adds POD to the exceptions files introduced by this patchset.

Note: the qa scripts give false positives on Exceptions. But the patchset
was missing them anyway.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 58 Tomás Cohen Arazi 2017-12-05 19:46:06 UTC
Created attachment 69551 [details] [review]
Bug 16330: (QA followup) Repect guidelines on plurals Patron > Patrons

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 59 Tomás Cohen Arazi 2017-12-05 19:55:19 UTC
I've rebased the previous work, and moved the code so it uses OpenAPI.

As the commit message ('Move patches to OpenAPI' patch) explains. I removed the portion that dealt with patron modification requests.

I also make use of the helper functions introduced by bug 19410, bug 19686 and their dependencies.

Benjamin, Lari and everyone involved. Please test the changes I made (big changes) and please add your signatures to the patchset. Once you (or others) test it, we can move forward with the QA process.
Comment 60 Dilan Johnpullé 2017-12-07 05:20:25 UTC
Attempted to apply the patch but ran into issues with one of the dependencies. Attempting the test afterwards just results in a lot of ugly error messages. 

This is the output from git when attempting to apply:

vagrant@kohadevbox:kohaclone((af7d618...))$ git bz apply -s 16330

Bug 16330 Depends on bug 19410
Follow? [(y)es, (n)o] y

Bug 19410 Depends on bug 19234
Follow? [(y)es, (n)o] y
Bug 19234 - Add query parameters handling helpers

69360 - Bug 19234: Unit tests for query parameters handling helpers
69361 - Bug 19234: Add query parameters handling helpers
69362 - Bug 19234: (followup) Make Query plugin available to endpoints

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 19234: Unit tests for query parameters handling helpers
Applying: Bug 19234: Add query parameters handling helpers
Applying: Bug 19234: (followup) Make Query plugin available to endpoints

Bug 19410 Depends on bug 19278
Follow? [(y)es, (n)o] y

Bug 19278 Depends on bug 19369
Follow? [(y)es, (n)o] y

Bug 19369 Depends on bug 19234
Follow? [(y)es, (n)o] y
Bug 19234 - Add query parameters handling helpers

69360 - Bug 19234: Unit tests for query parameters handling helpers
69361 - Bug 19234: Add query parameters handling helpers
69362 - Bug 19234: (followup) Make Query plugin available to endpoints

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 19234: Unit tests for query parameters handling helpers
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: Bug 19234: Add query parameters handling helpers
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: Bug 19234: (followup) Make Query plugin available to endpoints
Using index info to reconstruct a base tree...
M	Koha/REST/V1.pm
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Bug 19369 - Add a helper function for translating pagination params into SQL::Abstract

69363 - Bug 19369: Unit tests
69364 - Bug 19369: Add helper function for pagination attributes generation

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 19369: Unit tests
Applying: Bug 19369: Add helper function for pagination attributes generation
Bug 19278 - Add a configurable default page size for endpoints

69365 - Bug 19278: Add RESTdefaultPageSize syspref
69366 - Bug 19278: Unit tests
69367 - Bug 19278: Make the pagination plugin use RESTdefaultPageSize
69368 - Bug 19278: Mention RESTdefaultPageSize in POD

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 19278: Add RESTdefaultPageSize syspref
Applying: Bug 19278: Unit tests
Applying: Bug 19278: Make the pagination plugin use RESTdefaultPageSize
Applying: Bug 19278: Mention RESTdefaultPageSize in POD

Bug 19410 Depends on bug 19369
Follow? [(y)es, (n)o] y

Bug 19369 Depends on bug 19234
Follow? [(y)es, (n)o] y
Bug 19234 - Add query parameters handling helpers

69360 - Bug 19234: Unit tests for query parameters handling helpers
69361 - Bug 19234: Add query parameters handling helpers
69362 - Bug 19234: (followup) Make Query plugin available to endpoints

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 19234: Unit tests for query parameters handling helpers
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: Bug 19234: Add query parameters handling helpers
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Applying: Bug 19234: (followup) Make Query plugin available to endpoints
Using index info to reconstruct a base tree...
M	Koha/REST/V1.pm
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
Bug 19369 - Add a helper function for translating pagination params into SQL::Abstract

69363 - Bug 19369: Unit tests
69364 - Bug 19369: Add helper function for pagination attributes generation

Apply? [(y)es, (n)o, (i)nteractive] y
Applying: Bug 19369: Unit tests
Using index info to reconstruct a base tree...
M	t/Koha/REST/Plugin/Pagination.t
Falling back to patching base and 3-way merge...
Auto-merging t/Koha/REST/Plugin/Pagination.t
CONFLICT (content): Merge conflict in t/Koha/REST/Plugin/Pagination.t
Failed to merge in the changes.
Patch failed at 0001 Bug 19369: Unit tests
The copy of the patch that failed is found in:
   /home/vagrant/kohaclone/.git/rebase-apply/patch
When you have resolved this problem run "git bz apply --continue".
If you would prefer to skip this patch, instead run "git bz apply --skip".
To restore the original branch and stop patching run "git bz apply --abort".
Patch left in /tmp/Bug-19369-Unit-tests-aBwR2M.patch

Cannot apply cleanly patches from bug 19369. Everything will be left dirty.
git bz apply --continue will not continue the process if patches from other bug reports need to be applied.
Comment 61 Dilan Johnpullé 2017-12-07 05:49:43 UTC
Just attempted applying each dependency manually in order. Have got 16330 to apply now and will follow test plan instructions.
Comment 62 Dilan Johnpullé 2017-12-07 06:05:20 UTC
Still getting error messages when running the test:

vagrant@kohadevbox:kohaclone((8c8b618...))$ sudo perl t/db_dependent/api/v1/patrons.t
1..5
unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line 243.
unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line 243.
unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line 243.
unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line 243.
unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line 243.
unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line 243.
Use of uninitialized value in concatenation (.) or string at C4/Templates.pm line 154.
unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line 243.
unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line 243.
Use of uninitialized value in concatenation (.) or string at C4/Auth_with_cas.pm line 42.
unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line 243.
unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line 243.
Can't call method "config" on unblessed reference at Koha/Database.pm line 56.
# Looks like your test exited with 2 before it could output anything.
Comment 63 Magnus Enger 2017-12-07 07:06:58 UTC
(In reply to Dilan Johnpullé from comment #62)
> Still getting error messages when running the test:
...
> unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line

These errors should go away if you do "sudo koha-shell <dev-instance-name>" and then run the tests from within that koha-shell. 

If you are not on kohadevbox or another gitified setup that can use koha-shell, you need to include some environment variables in your call to prove, something like:

$ PERL5LIB=/home/me/kohaclone KOHA_CONF=/home/me/koha-site/koha-conf.xml prove path/to/test.t

HTH
Comment 64 Tomás Cohen Arazi 2017-12-07 10:00:28 UTC
(In reply to Dilan Johnpullé from comment #62)
> Still getting error messages when running the test:
> 
> vagrant@kohadevbox:kohaclone((8c8b618...))$ sudo perl
> t/db_dependent/api/v1/patrons.t
> 1..5
> unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line
> 243.
> unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line
> 243.
> unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line
> 243.
> unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line
> 243.
> unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line
> 243.
> unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line
> 243.
> Use of uninitialized value in concatenation (.) or string at C4/Templates.pm
> line 154.
> unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line
> 243.
> unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line
> 243.
> Use of uninitialized value in concatenation (.) or string at
> C4/Auth_with_cas.pm line 42.
> unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line
> 243.
> unable to locate Koha configuration file koha-conf.xml at C4/Context.pm line
> 243.
> Can't call method "config" on unblessed reference at Koha/Database.pm line
> 56.
> # Looks like your test exited with 2 before it could output anything.

You need to get into the instance's she'll:
$ sudo koha-shell kohadev
$ cd /home/vagrant/kohaclon

and then run the tests

If your devbox was created recently try

$ kshell

Also, one of the dependencies makes it retire an upgrade, so inside kshell you need to perform the upgrade:

$ perl installer/data/mysql/updatedatabase.pl

before running the tests. Otherwise you will get tons of 503 errors instead of the expected results.
Comment 65 Dilan Johnpullé 2017-12-12 04:22:00 UTC
Hi Tomas, I was able to successfully run all the tests and add and delete patrons using RESTer following your helpful advice in IRC. Unfortunately attempting to update patrons (PUT /api/v1/patrons/<borrowernumber>) throws a 500 Internal Server Error. The Koha logs don't show anything in them related to this and my apache logs don't show anything out of the ordinary either. 

Thanks again for the help.
Comment 66 Benjamin Rokseth 2017-12-12 11:07:43 UTC
One thing that needs to be fixed:
PUT endpoint uses C4::ModMember (which I think is plain wrong, but thats another issue and I understand why...) and this requires borrowernumber as param due to some internal lookups, categorycode e.g.)

tests pass since entire patron object is passed in, but that's a false positive, since borrowernumber shouldn't be passed in body, its already a path param. since path param must be authoritative, I suggest just overwriting body param in the helper method already used.

Otherwise great work!
Comment 67 Tomás Cohen Arazi 2017-12-12 21:56:14 UTC
Created attachment 69752 [details] [review]
Bug 16330: (QA follow-up) ModMember requires borrowernumber

This patch fixes the tests so they highlight the fact that:
- PUT only worked on the tests because borrowernumber was passed on them
- ModMember expects borrowernumber on the hash it gets passed.

It also fixes the issue.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 68 Benjamin Rokseth 2017-12-13 08:30:32 UTC
Created attachment 69762 [details] [review]
Bug 16330: REST API: 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>
Comment 69 Benjamin Rokseth 2017-12-13 08:30:39 UTC
Created attachment 69763 [details] [review]
Bug 16330: Spot unchanged modification-request in Koha::Patron::Modification

Throw Koha::Exceptions::NoChanges if attempting to create a modification request
without changing anything.

This exception is caught in Patron REST API controller.

Also, validate changes via Koha::Patron->_validate

To test:
1. prove t/db_dependent/Koha/Patron/Modifications.t

Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 70 Benjamin Rokseth 2017-12-13 08:30:48 UTC
Created attachment 69764 [details] [review]
Bug 16330: Move patches to OpenAPI

This patch refactors the original work so it implements the controllers
and the spec using Mojolicious::Plugin::OpenAPI, and OpenAPI for the specification.

It removes the ability for patrons without permissions to edit their own data or their
guarantee's. This will be moved to a patron modification requests endpoint for simplicity.

It makes use of bugs 19410 and 19686 and their dependencies to deal with parameters handling,
query building and pagination.

Tests are adapted.

To test:
- Apply this patches and the dependencies
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 71 Benjamin Rokseth 2017-12-13 08:30:56 UTC
Created attachment 69765 [details] [review]
Bug 16330: (QA followup) fix spelling in comment

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 72 Benjamin Rokseth 2017-12-13 08:31:04 UTC
Created attachment 69766 [details] [review]
Bug 16330: (QA followup) Add missing POD for exceptions

This patch adds POD to the exceptions files introduced by this patchset.

Note: the qa scripts give false positives on Exceptions. But the patchset
was missing them anyway.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 73 Benjamin Rokseth 2017-12-13 08:31:10 UTC
Created attachment 69767 [details] [review]
Bug 16330: (QA followup) Repect guidelines on plurals Patron > Patrons

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 74 Benjamin Rokseth 2017-12-13 08:31:18 UTC
Created attachment 69768 [details] [review]
Bug 16330: (QA follow-up) ModMember requires borrowernumber

This patch fixes the tests so they highlight the fact that:
- PUT only worked on the tests because borrowernumber was passed on them
- ModMember expects borrowernumber on the hash it gets passed.

It also fixes the issue.

To test:
- Apply this patch
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 75 Benjamin Rokseth 2017-12-13 08:32:38 UTC
Works as expected. Tests pass, API responds appropriately.
Great work Tomas and Lari!
A good step forward on the REST api
Comment 76 Dilan Johnpullé 2017-12-14 04:23:55 UTC
Can confirm. Works as expected now in line with test plans.
Comment 77 Dilan Johnpullé 2017-12-14 05:12:01 UTC
Created attachment 69787 [details] [review]
Bug 16330: (QA follow-up) ModMember requires borrowernumber

Works perfectly. Well done everyone!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>
Comment 78 Jonathan Druart 2017-12-18 14:51:32 UTC
Please double check there is something wrong: Koha::Patron->store does nothing, and ->_validate (private) is called from outside the module.

I guess it should be called, and then the expections caught from (at least) memberentry.

Same for Koha::Patron::Modification->store, I think the exception must be caught in our code.

And, first of all, _check_branchcode and _check_categorycode sound wrong: we have a FK on DB level and DBIx::Class will raise an error already if something is missing. Are we going to create these kind of check method for all the FK we have at DB level?
Comment 79 Jonathan Druart 2017-12-18 14:53:41 UTC
Apparently I already raised that concern on comment 21 and comment 30.
Could you convince me we really want/need they _check_* methods? If they are pushed it will become a standard and I do not think it is the way to go.
Comment 80 Tomás Cohen Arazi 2017-12-18 15:00:56 UTC
(In reply to Jonathan Druart from comment #78)
> Please double check there is something wrong: Koha::Patron->store does
> nothing, and ->_validate (private) is called from outside the module.

I agree, _validate doesn't prove that useful, so maybe we should get rid of it at all.

> I guess it should be called, and then the expections caught from (at least)
> memberentry.
> 
> Same for Koha::Patron::Modification->store, I think the exception must be
> caught in our code.

This looked handy, but I actually removed the code that dealt with modification requests on this endpoint. Would you agree we can just remove this from the commit?

> And, first of all, _check_branchcode and _check_categorycode sound wrong: we
> have a FK on DB level and DBIx::Class will raise an error already if
> something is missing. Are we going to create these kind of check method for
> all the FK we have at DB level?

I agree with this assertion. I think ->store should just catch and parse the DBIC exceptions and raise our own to have better control of the situation.

I don't agree we should catch the exceptions on this bug, as those exceptions already raise (DBIC ones) and are currently uncatched. Would you accept that we filed a new bug for better exception handling in controller scripts using Koha::Patron->store?
Comment 81 Tomás Cohen Arazi 2017-12-21 14:43:19 UTC
Created attachment 70034 [details] [review]
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>
Comment 82 Tomás Cohen Arazi 2017-12-21 14:43:28 UTC
Created attachment 70035 [details] [review]
Bug 16330: Move patches to OpenAPI

This patch refactors the original work so it implements the controllers
and the spec using Mojolicious::Plugin::OpenAPI, and OpenAPI for the specification.

It removes the ability for patrons without permissions to edit their own data or their
guarantee's. This will be moved to a patron modification requests endpoint for simplicity.

It makes use of bugs 19410 and 19686 and their dependencies to deal with parameters handling,
query building and pagination.

Tests are adapted.

To test:
- Apply this patches and the dependencies
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 83 Tomás Cohen Arazi 2017-12-21 14:43:37 UTC
Created attachment 70036 [details] [review]
Bug 16330: (QA follow-up) Repect guidelines on plurals Patron > Patrons

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 84 Tomás Cohen Arazi 2017-12-21 14:43:47 UTC
Created attachment 70037 [details] [review]
Bug 16330: (QA follow-up) ModMember requires borrowernumber

Works perfectly. Well done everyone!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>
Comment 85 Tomás Cohen Arazi 2017-12-21 14:43:54 UTC
Created attachment 70038 [details] [review]
Bug 16330: Remove validation code from Koha::Patron

This patch removes previously added validation code from Koha::Patron
as we will rely on the DB structure and relationships to catch the same
problems. This is implemented on bug 19828.

This patch also adapts the API controller class to expect this behaviour
change from Koha::Patron. The expected exceptions are adjusted, and some
minor changes take place. The API tests are adjusted as well.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Patrons.t
 k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests should still pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 86 Bob Birchall 2018-02-08 03:01:19 UTC
(In reply to Tomás Cohen Arazi from comment #85)
> Created attachment 70038 [details] [review] [review]
> Bug 16330: Remove validation code from Koha::Patron
> 
> This patch removes previously added validation code from Koha::Patron
> as we will rely on the DB structure and relationships to catch the same
> problems. This is implemented on bug 19828.
> 
> This patch also adapts the API controller class to expect this behaviour
> change from Koha::Patron. The expected exceptions are adjusted, and some
> minor changes take place. The API tests are adjusted as well.
> 
> To test:
> - Run:
>   $ kshell
>  k$ prove t/db_dependent/Koha/Patrons.t
>  k$ prove t/db_dependent/api/v1/patrons.t
> => SUCCESS: Tests should still pass
> 
> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Is this waiting for extra sign-offs or is it ready for QA? If we (ie Dilan) can help, please let us know.
Comment 87 Tomás Cohen Arazi 2018-02-15 15:37:04 UTC
Created attachment 71667 [details] [review]
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>
Comment 88 Tomás Cohen Arazi 2018-02-15 15:37:14 UTC
Created attachment 71668 [details] [review]
Bug 16330: Move patches to OpenAPI

This patch refactors the original work so it implements the controllers
and the spec using Mojolicious::Plugin::OpenAPI, and OpenAPI for the specification.

It removes the ability for patrons without permissions to edit their own data or their
guarantee's. This will be moved to a patron modification requests endpoint for simplicity.

It makes use of bugs 19410 and 19686 and their dependencies to deal with parameters handling,
query building and pagination.

Tests are adapted.

To test:
- Apply this patches and the dependencies
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 89 Tomás Cohen Arazi 2018-02-15 15:37:29 UTC
Created attachment 71669 [details] [review]
Bug 16330: (QA follow-up) Repect guidelines on plurals Patron > Patrons

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 90 Tomás Cohen Arazi 2018-02-15 15:37:42 UTC
Created attachment 71670 [details] [review]
Bug 16330: (QA follow-up) ModMember requires borrowernumber

Works perfectly. Well done everyone!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>
Comment 91 Tomás Cohen Arazi 2018-02-15 15:37:56 UTC
Created attachment 71671 [details] [review]
Bug 16330: Remove validation code from Koha::Patron

This patch removes previously added validation code from Koha::Patron
as we will rely on the DB structure and relationships to catch the same
problems. This is implemented on bug 19828.

This patch also adapts the API controller class to expect this behaviour
change from Koha::Patron. The expected exceptions are adjusted, and some
minor changes take place. The API tests are adjusted as well.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Patrons.t
 k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests should still pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 92 Tomás Cohen Arazi 2018-02-15 15:40:39 UTC
Rebased the patches. Please... Anyone...
Comment 93 Katrin Fischer 2018-02-18 11:23:44 UTC
Tomas, you are still QA contact for this - remove?
Comment 94 Josef Moravec 2018-02-19 08:43:04 UTC
Created attachment 71920 [details] [review]
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>
Comment 95 Josef Moravec 2018-02-19 08:43:11 UTC
Created attachment 71921 [details] [review]
Bug 16330: Move patches to OpenAPI

This patch refactors the original work so it implements the controllers
and the spec using Mojolicious::Plugin::OpenAPI, and OpenAPI for the specification.

It removes the ability for patrons without permissions to edit their own data or their
guarantee's. This will be moved to a patron modification requests endpoint for simplicity.

It makes use of bugs 19410 and 19686 and their dependencies to deal with parameters handling,
query building and pagination.

Tests are adapted.

To test:
- Apply this patches and the dependencies
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Comment 96 Josef Moravec 2018-02-19 08:43:16 UTC
Created attachment 71922 [details] [review]
Bug 16330: (QA follow-up) Repect guidelines on plurals Patron > Patrons

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 97 Josef Moravec 2018-02-19 08:43:21 UTC
Created attachment 71923 [details] [review]
Bug 16330: (QA follow-up) ModMember requires borrowernumber

Works perfectly. Well done everyone!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>
Comment 98 Josef Moravec 2018-02-19 08:43:27 UTC
Created attachment 71924 [details] [review]
Bug 16330: Remove validation code from Koha::Patron

This patch removes previously added validation code from Koha::Patron
as we will rely on the DB structure and relationships to catch the same
problems. This is implemented on bug 19828.

This patch also adapts the API controller class to expect this behaviour
change from Koha::Patron. The expected exceptions are adjusted, and some
minor changes take place. The API tests are adjusted as well.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Patrons.t
 k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests should still pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 99 Josef Moravec 2018-02-19 08:43:32 UTC
Created attachment 71925 [details] [review]
Bug 16330: (QA follow-up) Update usage of search plugin because of bug 19686

Test plan:
prove t/db_dependent/api/v1/patrons.t
--> without  patch the list subtest fail
--> with patch test pass
Comment 100 Josef Moravec 2018-02-19 08:47:44 UTC
> 
> 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.
> 
This is not implemented, do we need this? Failed QA means I need an answer as this is in test plan and does not work. Otherwise looks good.

Also, I added small follow-up, and rebased again on current master (small conflict in Koha/Patron.pm)
Comment 101 Tomás Cohen Arazi 2018-02-19 10:07:44 UTC
(In reply to Josef Moravec from comment #100)
> > 
> > 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.
> > 
> This is not implemented, do we need this? Failed QA means I need an answer
> as this is in test plan and does not work. Otherwise looks good.

The patrons endpoint is finished on the follow-up bug. We kept this one to preserve the original attribution. One of the things we did was removing that complexity. We don't need it, we voted an RFC which is implemented on the other bug 

> Also, I added small follow-up, and rebased again on current master (small
> conflict in Koha/Patron.pm)

Excellent.
Comment 102 Tomás Cohen Arazi 2018-02-19 10:17:14 UTC
(In reply to Josef Moravec from comment #100)
> > 
> > 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.
> > 
> This is not implemented, do we need this? Failed QA means I need an answer
> as this is in test plan and does not work. Otherwise looks good.

Look at the comment in 'Move patches to OpenAPI':


"It removes the ability for patrons without permissions to edit their own data or their
guarantee's. This will be moved to a patron modification requests endpoint for simplicity."
Comment 103 Josef Moravec 2018-02-19 11:31:02 UTC
(In reply to Tomás Cohen Arazi from comment #102)
> (In reply to Josef Moravec from comment #100)
> > > 
> > > 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.
> > > 
> > This is not implemented, do we need this? Failed QA means I need an answer
> > as this is in test plan and does not work. Otherwise looks good.
> 
> Look at the comment in 'Move patches to OpenAPI':
> 
> 
> "It removes the ability for patrons without permissions to edit their own
> data or their
> guarantee's. This will be moved to a patron modification requests endpoint
> for simplicity."

I am totally blind, does make sense, thanks!
Comment 104 Josef Moravec 2018-02-19 11:34:04 UTC
Created attachment 71933 [details] [review]
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>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 105 Josef Moravec 2018-02-19 11:34:10 UTC
Created attachment 71934 [details] [review]
Bug 16330: Move patches to OpenAPI

This patch refactors the original work so it implements the controllers
and the spec using Mojolicious::Plugin::OpenAPI, and OpenAPI for the specification.

It removes the ability for patrons without permissions to edit their own data or their
guarantee's. This will be moved to a patron modification requests endpoint for simplicity.

It makes use of bugs 19410 and 19686 and their dependencies to deal with parameters handling,
query building and pagination.

Tests are adapted.

To test:
- Apply this patches and the dependencies
- Run:
  $ kshell
 k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests pass!
- Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 106 Josef Moravec 2018-02-19 11:34:15 UTC
Created attachment 71935 [details] [review]
Bug 16330: (QA follow-up) Repect guidelines on plurals Patron > Patrons

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 107 Josef Moravec 2018-02-19 11:34:21 UTC
Created attachment 71936 [details] [review]
Bug 16330: (QA follow-up) ModMember requires borrowernumber

Works perfectly. Well done everyone!

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no>
Signed-off-by: Dilan Johnpullé <dilan@calyx.net.au>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 108 Josef Moravec 2018-02-19 11:34:27 UTC
Created attachment 71937 [details] [review]
Bug 16330: Remove validation code from Koha::Patron

This patch removes previously added validation code from Koha::Patron
as we will rely on the DB structure and relationships to catch the same
problems. This is implemented on bug 19828.

This patch also adapts the API controller class to expect this behaviour
change from Koha::Patron. The expected exceptions are adjusted, and some
minor changes take place. The API tests are adjusted as well.

To test:
- Run:
  $ kshell
 k$ prove t/db_dependent/Koha/Patrons.t
 k$ prove t/db_dependent/api/v1/patrons.t
=> SUCCESS: Tests should still pass

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 109 Josef Moravec 2018-02-19 11:34:32 UTC
Created attachment 71938 [details] [review]
Bug 16330: (QA follow-up) Update usage of search plugin because of bug 19686

Test plan:
prove t/db_dependent/api/v1/patrons.t
--> without  patch the list subtest fail
--> with patch test pass

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 110 Jonathan Druart 2018-02-19 19:29:40 UTC
Benjamin, do you agree with Tomas's changes?
Comment 111 Jonathan Druart 2018-02-19 19:40:41 UTC
1. I have squashed the patches to review them and see that Koha::Patron->store only calls SUPER->store.

2. Koha::REST::V1::{Patrons,Cities}->list are different, must not be identical?


Just a note for later: I think it should be the only routes added that do not use Koha::Cities pattern (i.e. full Koha::Objects based).
Comment 112 Tomás Cohen Arazi 2018-02-19 19:56:22 UTC
(In reply to Jonathan Druart from comment #111)
> 1. I have squashed the patches to review them and see that
> Koha::Patron->store only calls SUPER->store.
> 
> 2. Koha::REST::V1::{Patrons,Cities}->list are different, must not be
> identical?
> 
> 
> Just a note for later: I think it should be the only routes added that do
> not use Koha::Cities pattern (i.e. full Koha::Objects based).

Just a note: this is rewritten in the follow-up bug, right?
Comment 113 Benjamin Rokseth 2018-02-20 07:22:51 UTC
(In reply to Jonathan Druart from comment #110)
> Benjamin, do you agree with Tomas's changes?

Nothing to add, well done pushing this forward!
Comment 114 Jonathan Druart 2018-02-21 17:45:59 UTC
From AddMember, added by bug 19841:

    my $category = Koha::Patron::Categories->find( $data{categorycode} );
    unless ($category) {
        Koha::Exceptions::BadParameter->throw(
            error => 'Invalid parameter passed',
            parameter => 'categorycode'
        );
    }

Must not we remove that?
Comment 115 Tomás Cohen Arazi 2018-02-21 20:23:03 UTC
(In reply to Jonathan Druart from comment #114)
> From AddMember, added by bug 19841:
> 
>     my $category = Koha::Patron::Categories->find( $data{categorycode} );
>     unless ($category) {
>         Koha::Exceptions::BadParameter->throw(
>             error => 'Invalid parameter passed',
>             parameter => 'categorycode'
>         );
>     }
> 
> Must not we remove that?

This belongs to a separate bug, of course. The thing is: we need the category object to specify a default expiration date and default privacy. We need that information before ->store. And if the category object is undef, then we have nothing to do... so better exit before doing more calculations (set values if defined $category, etc).

I think it is ok as it is.
Comment 116 Jonathan Druart 2018-02-21 21:00:58 UTC
(In reply to Tomás Cohen Arazi from comment #115)
> (In reply to Jonathan Druart from comment #114)
> > From AddMember, added by bug 19841:
> > 
> >     my $category = Koha::Patron::Categories->find( $data{categorycode} );
> >     unless ($category) {
> >         Koha::Exceptions::BadParameter->throw(
> >             error => 'Invalid parameter passed',
> >             parameter => 'categorycode'
> >         );
> >     }
> > 
> > Must not we remove that?
> 
> This belongs to a separate bug, of course. The thing is: we need the
> category object to specify a default expiration date and default privacy. We
> need that information before ->store. And if the category object is undef,
> then we have nothing to do... so better exit before doing more calculations
> (set values if defined $category, etc).
> 
> I think it is ok as it is.

The problem is that the whole stuff is not done in a transaction, in that case we will not need it.
Comment 117 Jonathan Druart 2018-03-28 20:38:53 UTC
With bug 20287 we will have to redo it again...
Comment 118 Tomás Cohen Arazi 2018-03-28 20:46:03 UTC
(In reply to Jonathan Druart from comment #117)
> With bug 20287 we will have to redo it again...

Not a problem!
Comment 119 Jonathan Druart 2018-03-28 21:15:34 UTC
(In reply to Tomás Cohen Arazi from comment #118)
> (In reply to Jonathan Druart from comment #117)
> > With bug 20287 we will have to redo it again...
> 
> Not a problem!

You are volunteering, right?
Comment 120 Tomás Cohen Arazi 2018-03-28 23:05:10 UTC
(In reply to Jonathan Druart from comment #119)
> (In reply to Tomás Cohen Arazi from comment #118)
> > (In reply to Jonathan Druart from comment #117)
> > > With bug 20287 we will have to redo it again...
> > 
> > Not a problem!
> 
> You are volunteering, right?

Count on that, yes!
Comment 121 Tomás Cohen Arazi 2018-03-29 10:53:17 UTC
(In reply to Jonathan Druart from comment #119)
> (In reply to Tomás Cohen Arazi from comment #118)
> > (In reply to Jonathan Druart from comment #117)
> > > With bug 20287 we will have to redo it again...
> > 
> > Not a problem!
> 
> You are volunteering, right?

Given the fact that you have this and bug 19784 on your queue, I'd say we should add a follow-up for 20287 (I can) because this patchsets have become harder to rebase.
Comment 122 Jonathan Druart 2018-03-29 15:02:36 UTC
Pushed to master for 18.05, thanks to everybody involved!
Comment 123 Jonathan Druart 2018-03-29 15:44:50 UTC
(In reply to Tomás Cohen Arazi from comment #112)
> (In reply to Jonathan Druart from comment #111)
> > 1. I have squashed the patches to review them and see that
> > Koha::Patron->store only calls SUPER->store.

> Just a note: this is rewritten in the follow-up bug, right?

Nope, it is not.
Comment 124 Jonathan Druart 2018-03-29 15:46:05 UTC
Created attachment 73434 [details] [review]
Bug 16330: Remove unecessary Koha::Patron->store method

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 125 Jonathan Druart 2018-03-29 15:48:33 UTC
(In reply to Jonathan Druart from comment #124)
> Created attachment 73434 [details] [review] [review]
> Bug 16330: Remove unecessary Koha::Patron->store method
> 
> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Pushed to master.
Comment 126 Nick Clemens 2018-05-14 13:55:27 UTC
New feature, not backported for 17.11.x