[Bug 16330] New: REST API: add routes to add, update and delete patrons

classic Classic list List threaded Threaded
171 messages Options
123456
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

Jonathan Druart <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]-c
                   |                            |ommunity.org

--- Comment #21 from Jonathan Druart <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

--- Comment #22 from Jonathan Druart <[hidden email]> ---
Comment on attachment 54942
  --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54942
Bug 16330: REST API: add routes to add, update and delete patrons

Review of attachment 54942:
 --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=16330&attachment=54942)
-----------------------------------------------------------------

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

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

--- Comment #23 from Jonathan Druart <[hidden email]> ---
Comment on attachment 54942
  --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54942
Bug 16330: REST API: add routes to add, update and delete patrons

Review of attachment 54942:
 --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=16330&attachment=54942)
-----------------------------------------------------------------

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

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

--- Comment #24 from Jonathan Druart <[hidden email]> ---
Comment on attachment 54942
  --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=54942
Bug 16330: REST API: add routes to add, update and delete patrons

Review of attachment 54942:
 --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=16330&attachment=54942)
-----------------------------------------------------------------

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

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

--- Comment #25 from Jonathan Druart <[hidden email]> ---
(In reply to Jonathan Druart from comment #24)

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

Or 400 instead.

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

--- Comment #26 from Benjamin Rokseth <[hidden email]> ---
(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?

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

--- Comment #27 from Benjamin Rokseth <[hidden email]> ---
(In reply to Jonathan Druart from comment #22)

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

Absolutely right! Actually, it might be better to use formData and let swagger
definitions do the validation.

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

--- Comment #28 from Benjamin Rokseth <[hidden email]> ---
(In reply to Jonathan Druart from comment #25)

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

yes. Thanks for thorough feedback, btw!

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

--- Comment #29 from Jonathan Druart <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

--- Comment #30 from Jonathan Druart <[hidden email]> ---
(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?

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

Benjamin Rokseth <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
  Attachment #54942|0                           |1
        is obsolete|                            |

--- Comment #31 from Benjamin Rokseth <[hidden email]> ---
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 <[hidden email]>

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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

Tomás Cohen Arazi <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]
             Status|Signed Off                  |Failed QA

--- Comment #32 from Tomás Cohen Arazi <[hidden email]> ---
The DELETE permissions are wrong. It should be 'borrowers' only, and probably
some more checks need to be done (e.g. IndependentBranches).

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

Lari Taskula <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Failed QA                   |Needs Signoff

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

--- Comment #33 from Lari Taskula <[hidden email]> ---
Created attachment 56666
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56666&action=edit
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

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

--- Comment #34 from Lari Taskula <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

--- Comment #35 from Lari Taskula <[hidden email]> ---
Created attachment 56668
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=56668&action=edit
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

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

Tomás Cohen Arazi <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Needs Signoff               |In Discussion

--- Comment #36 from Tomás Cohen Arazi <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

--- Comment #37 from Lari Taskula <[hidden email]> ---
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?

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

--- Comment #38 from Jiri Kozlovsky <[hidden email]> ---
(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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

Josef Moravec <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]
         Depends on|                            |16846
             Status|In Discussion               |BLOCKED

--- Comment #39 from Josef Moravec <[hidden email]> ---
(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


Referenced Bugs:

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16846
[Bug 16846] Move patron related code to Koha::Patron
--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

Tomás Cohen Arazi <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|BLOCKED                     |Signed Off

--- Comment #40 from Tomás Cohen Arazi <[hidden email]> ---
(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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

Lari Taskula <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|Signed Off                  |In Discussion

--- Comment #41 from Lari Taskula <[hidden email]> ---
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.

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

Magnus Enger <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]

--- Comment #42 from Magnus Enger <[hidden email]> ---
What needs to happen for this bug to get moving?

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

Mirko Tietgen <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[hidden email]

--- Comment #43 from Mirko Tietgen <[hidden email]> ---
(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?

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

--- Comment #44 from Marcel de Rooy <[hidden email]> ---
(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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

--- Comment #45 from Lari Taskula <[hidden email]> ---
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.

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

--- Comment #46 from Lari Taskula <[hidden email]> ---
(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.

--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

--- Comment #47 from Lari Taskula <[hidden email]> ---
Created attachment 63646
  -->
https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=63646&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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

--- Comment #48 from Lari Taskula <[hidden email]> ---
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.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
Reply | Threaded
Open this post in threaded view
|

[Bug 16330] REST API: add routes to add, update and delete patrons

bugzilla-daemon
In reply to this post by bugzilla-daemon
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16330

Lari Taskula <[hidden email]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Depends on|                            |18330, 18230


Referenced Bugs:

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18230
[Bug 18230] Generate Koha::Patron::Modification verification_token in ->new
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18330
[Bug 18330] REST API: Date-time handling
--
You are receiving this mail because:
You are watching all bug changes.
_______________________________________________
Koha-bugs mailing list
[hidden email]
http://lists.koha-community.org/cgi-bin/mailman/listinfo/koha-bugs
website : http://www.koha-community.org/
git : http://git.koha-community.org/
bugs : http://bugs.koha-community.org/
123456