Bug 16330

How about the READ call?

(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)

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

(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

> 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 on attachment 53820 [details] [review]
[SIGNED-OFF] Bug 16330: (followup) fix spelling and number of tests

Changed description to have SIGNED-OFF included

Comment on attachment 53217 [details] [review]
[SIGNED-OFF] Bug 16330: (followup) fix spelling and number of tests

same patch occurring twice

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?

(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.

(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.

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.

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 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 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 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?

(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.

(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?

(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.

(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!

About tests: 1 subtest per route you test would be better for the readability.

(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?

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.

The DELETE permissions are wrong. It should be 'borrowers' only, and probably some more checks need to be done (e.g. IndependentBranches).

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

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.

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

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.

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?

(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.

(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

(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.

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.

What needs to happen for this bug to get moving?

(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?

(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..

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.

(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.

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.

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

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.

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

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...

(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

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>

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

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>

Created attachment 69549 [details] [review]
Bug 16330: (QA followup) fix spelling in comment

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

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>

Created attachment 69551 [details] [review]
Bug 16330: (QA followup) Repect guidelines on plurals Patron > Patrons

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

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.

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.

Just attempted applying each dependency manually in order. Have got 16330 to apply now and will follow test plan instructions.

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.

(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

(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.

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.

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!

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>

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>

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>

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>

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>

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>

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>

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>

Works as expected. Tests pass, API responds appropriately.
Great work Tomas and Lari!
A good step forward on the REST api

Can confirm. Works as expected now in line with test plans.

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>

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?

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.

(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?

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>

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>

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>

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>

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>

(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.

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>

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>

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>

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>

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>

Rebased the patches. Please... Anyone...

Tomas, you are still QA contact for this - remove?

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>

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>

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>

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>

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>

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

> 
> 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)

(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.

(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."

(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!

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>

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>

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>

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>

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>

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>

Benjamin, do you agree with Tomas's changes?

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).

(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?

(In reply to Jonathan Druart from comment #110)
> Benjamin, do you agree with Tomas's changes?

Nothing to add, well done pushing this forward!

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?

(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.

(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.

With bug 20287 we will have to redo it again...

(In reply to Jonathan Druart from comment #117)
> With bug 20287 we will have to redo it again...

Not a problem!

(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?

(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!

(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.

Pushed to master for 18.05, thanks to everybody involved!

(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.

Created attachment 73434 [details] [review]
Bug 16330: Remove unecessary Koha::Patron->store method

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

(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.

New feature, not backported for 17.11.x

*** Bug 13607 has been marked as a duplicate of this bug. ***