https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=13607 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #2 from Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> --- (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) -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #3 from Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> --- Created attachment 50803 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=50803&action=edit 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. 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 Test plan: 1) Apply this patch (on top of dependent bug 15126) 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. Please note there is no validation of body input in PUT/POST other than branchcode,category,userid,cardnumber. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA CC| |katrin.fischer@bsz-bw.de --- Comment #4 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #6 from Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> --- (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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #8 from Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> ---

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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #51066|0 |1 is obsolete| | --- Comment #9 from Bernardo Gonzalez Kriegel <bgkriegel@gmail.com> --- Created attachment 51131 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=51131&action=edit [SIGNED-OFF] 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 Test plan: 1) Apply this patch (on top of dependent bug 15126) 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. 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=13738 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #11 from Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> --- Created attachment 53216 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53216&action=edit [SIGNED-OFF] 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 Test plan: 1) Apply this patch (on top of dependent bug 15126) 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. 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. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #51131|0 |1 is obsolete| | -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Lari Taskula <larit@student.uef.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |larit@student.uef.fi -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Jiri Kozlovsky <mail@jkozlovsky.cz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mail@jkozlovsky.cz Attachment #53820|Bug 16330: (followup) fix |[SIGNED-OFF] Bug 16330: description|spelling and number of |(followup) fix spelling and |tests |number of tests --- Comment #14 from Jiri Kozlovsky <mail@jkozlovsky.cz> --- Comment on attachment 53820 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53820 [SIGNED-OFF] Bug 16330: (followup) fix spelling and number of tests Changed description to have SIGNED-OFF included -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #53217|0 |1 is obsolete| | --- Comment #15 from Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> --- Comment on attachment 53217 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=53217 [SIGNED-OFF] Bug 16330: (followup) fix spelling and number of tests same patch occurring twice -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |benjamin.rokseth@kul.oslo.k |ity.org |ommune.no Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #18 from Lari Taskula <larit@student.uef.fi> --- (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.

-- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on|15126 |16699, 14868 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14868 [Bug 14868] REST API: Swagger2-driven permission checking https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15126 [Bug 15126] REST API: Use newer version of Swagger2 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16699 [Bug 16699] Swagger: Split parameters and paths, and specify required permissions for resource -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #21 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #29 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- About tests: 1 subtest per route you test would be better for the readability. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #54942|0 |1 is obsolete| | --- Comment #31 from Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> --- Created attachment 55147 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=55147&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Lari Taskula <lari.taskula@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #34 from Lari Taskula <lari.taskula@jns.fi> --- Created attachment 56667 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56667&action=edit Bug 16330: (follow-up) Add Test::Fatal Perl dependency Test::Fatal is a simple module for testing exception-based code. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |In Discussion --- Comment #36 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #38 from Jiri Kozlovsky <mail@jkozlovsky.cz> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Signed Off --- Comment #40 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Magnus Enger <magnus@libriotech.no> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |magnus@libriotech.no --- Comment #42 from Magnus Enger <magnus@libriotech.no> --- What needs to happen for this bug to get moving? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #44 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- (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.. -- You are receiving this mail because: You are watching all bug changes.

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

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #46 from Lari Taskula <lari.taskula@jns.fi> --- (In reply to Lari Taskula from comment #45) 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #48 from Lari Taskula <lari.taskula@jns.fi> --- Created attachment 63647 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63647&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Lari Taskula <lari.taskula@jns.fi> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #63646|0 |1 is obsolete| | --- Comment #49 from Lari Taskula <lari.taskula@jns.fi> --- Created attachment 63649 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63649&action=edit 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Blou <philippe.blouin@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |philippe.blouin@inlibro.com --- Comment #51 from Blou <philippe.blouin@inlibro.com> --- 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... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Bob Birchall <bob@calyx.net.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |bob@calyx.net.au -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #55147|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #56667|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #63649|0 |1 is obsolete| | --- Comment #53 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 69546 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=69546&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #55 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 69548 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=69548&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #57 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 69550 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=69550&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff Patch complexity|Small patch |Medium patch QA Contact| |tomascohen@gmail.com Depends on|16846 |19410 Severity|enhancement |new feature --- Comment #59 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16846 [Bug 16846] Move patron related code to Koha::Patron https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19410 [Bug 19410] Add a helper function for generating object searches for the API -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Dilan Johnpullé <dilan@calyx.net.au> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |dilan@calyx.net.au --- Comment #60 from Dilan Johnpullé <dilan@calyx.net.au> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #62 from Dilan Johnpullé <dilan@calyx.net.au> --- 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 are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #64 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|19774 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19774 [Bug 19774] Adapt existing endpoints to new naming guidelines OMNIBUS -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #65 from Dilan Johnpullé <dilan@calyx.net.au> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #67 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 69752 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=69752&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #69547|0 |1 is obsolete| | --- Comment #69 from Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> --- Created attachment 69763 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=69763&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #69549|0 |1 is obsolete| | --- Comment #71 from Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> --- Created attachment 69765 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=69765&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #69551|0 |1 is obsolete| | --- Comment #73 from Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> --- Created attachment 69767 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=69767&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off --- Comment #75 from 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Dilan Johnpullé <dilan@calyx.net.au> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #69768|0 |1 is obsolete| | --- Comment #77 from Dilan Johnpullé <dilan@calyx.net.au> --- Created attachment 69787 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=69787&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |In Discussion --- Comment #78 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #80 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Bug 16330 depends on bug 18330, which changed state. Bug 18330 Summary: REST API: Date-time handling https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18330 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #69763|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #69765|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #69767|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #81 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 70034 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=70034&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #83 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 70036 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=70036&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #85 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 70038 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=70038&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Bug 16330 depends on bug 19841, which changed state. Bug 19841 Summary: AddMember should raise an exception if categorycode is invalid https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=19841 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #86 from Bob Birchall <bob@calyx.net.au> --- (In reply to Tomás Cohen Arazi from comment #85)

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>

Is this waiting for extra sign-offs or is it ready for QA? If we (ie Dilan) can help, please let us know. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #70035|0 |1 is obsolete| | --- Comment #88 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 71668 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=71668&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #70037|0 |1 is obsolete| | --- Comment #90 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 71670 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=71670&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #92 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Rebased the patches. Please... Anyone... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|tomascohen@gmail.com | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #71667|0 |1 is obsolete| | Attachment #71668|0 |1 is obsolete| | Attachment #71669|0 |1 is obsolete| | Attachment #71670|0 |1 is obsolete| | Attachment #71671|0 |1 is obsolete| | --- Comment #94 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 71920 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=71920&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #96 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 71922 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=71922&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #98 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 71924 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=71924&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Failed QA --- Comment #100 from Josef Moravec <josef.moravec@gmail.com> ---

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) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #102 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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." -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #105 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 71934 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=71934&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #107 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 71936 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=71936&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #109 from Josef Moravec <josef.moravec@gmail.com> --- Created attachment 71938 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=71938&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #111 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- 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). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #113 from Benjamin Rokseth <benjamin.rokseth@kul.oslo.kommune.no> --- (In reply to Jonathan Druart from comment #110)

Benjamin, do you agree with Tomas's changes?

Nothing to add, well done pushing this forward! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #115 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Claudio <costalc@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |costalc@gmail.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #117 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- With bug 20287 we will have to redo it again... -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #119 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #121 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #123 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 --- Comment #125 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (In reply to Jonathan Druart from comment #124)

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

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

Pushed to master. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the|REST API route for managing |REST API route for managing release notes|patrons in database. Adds |patrons in database. Adds |CRUD implementation for |CRUD implementation for |creating, reading, updating |creating, reading, updating |and deleting patrons, as |and deleting patrons, as |well as listing with |well as listing with |optional sorting. |optional sorting. | | |Follows |Follows |new guidelines from REST |new guidelines from REST |API RFC and Koha Object |API RFC and Koha Object |Exceptions regarding |Exceptions regarding |validation and error |validation and error |handling. Patron can modify |handling. Patrons can |and delete own object, or |modify and delete their own |anyone with borrower |object, or anyone with |modification permissions. |borrower modification | |permissions. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED CC| |nick@bywatersolutions.com --- Comment #126 from Nick Clemens <nick@bywatersolutions.com> --- New feature, not backported for 17.11.x -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |robin@catalyst.net.nz --- Comment #127 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- *** Bug 13607 has been marked as a duplicate of this bug. *** -- You are receiving this mail because: You are watching all bug changes.