From bbc4214df7ed1a0a185058c9ff734d404366a615 Mon Sep 17 00:00:00 2001 From: Martin Renvoize Date: Tue, 28 Jun 2022 16:50:29 +0100 Subject: [PATCH] Bug 29523: Remove the FIXME This patch works through the unit tests and existing code to allow removal of the FIXME I introduced earlier in the patchset. We now require the current user object to be passed into the to_api and subsequent is_accessible method. This is a change to the method signature for the Koha::Patron case and results in use needing to add the user parameter in some unexpected locations (like update and add methods as they use to_api internally for the response). --- Koha/Patron.pm | 5 +-- Koha/REST/V1/Patrons.pm | 4 +-- t/db_dependent/api/v1/acquisitions_baskets.t | 12 +++++-- t/db_dependent/api/v1/acquisitions_funds.t | 26 ++++++++++++--- t/db_dependent/api/v1/patrons.t | 35 ++++++++++++-------- 5 files changed, 55 insertions(+), 27 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index b1dd0a4c0a..3e6173ac87 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -1881,10 +1881,7 @@ Returns 0 if the I parameter is missing. sub is_accessible { my ( $self, $params ) = @_; - # FIXME? It felt tempting to return 0 instead - # but it would mean needing to explicitly add the 'user' - # param in all tests... - return 1 + return 0 unless $params->{user}; my $consumer = $params->{user}; diff --git a/Koha/REST/V1/Patrons.pm b/Koha/REST/V1/Patrons.pm index 576e4f8f2c..2a624c1e0f 100644 --- a/Koha/REST/V1/Patrons.pm +++ b/Koha/REST/V1/Patrons.pm @@ -131,7 +131,7 @@ sub add { $c->res->headers->location($c->req->url->to_string . '/' . $patron->borrowernumber); return $c->render( status => 201, - openapi => $patron->to_api + openapi => $patron->to_api({ user => $patron }) ); } ); @@ -265,7 +265,7 @@ sub update { $patron->set_from_api($c->validation->param('body'))->store; $patron->discard_changes; - return $c->render( status => 200, openapi => $patron->to_api ); + return $c->render( status => 200, openapi => $patron->to_api({ user => $patron }) ); } catch { unless ( blessed $_ && $_->can('rethrow') ) { diff --git a/t/db_dependent/api/v1/acquisitions_baskets.t b/t/db_dependent/api/v1/acquisitions_baskets.t index 92261f5d88..dd1edbb342 100755 --- a/t/db_dependent/api/v1/acquisitions_baskets.t +++ b/t/db_dependent/api/v1/acquisitions_baskets.t @@ -57,8 +57,16 @@ subtest 'list_managers() tests' => sub { } ); - $t->get_ok("//$userid:$password@/api/v1/acquisitions/baskets/managers?q=$api_filter") - ->status_is(200)->json_is( [ $patron_with_permission->to_api, $superlibrarian->to_api ] ); + $t->get_ok( + "//$userid:$password@/api/v1/acquisitions/baskets/managers?q=$api_filter" + )->status_is(200)->json_is( + [ + $patron_with_permission->to_api( + { user => $patron_with_permission } + ), + $superlibrarian->to_api( { user => $superlibrarian } ) + ] + ); $schema->storage->txn_rollback; }; diff --git a/t/db_dependent/api/v1/acquisitions_funds.t b/t/db_dependent/api/v1/acquisitions_funds.t index d4821586f1..56283fe9f8 100755 --- a/t/db_dependent/api/v1/acquisitions_funds.t +++ b/t/db_dependent/api/v1/acquisitions_funds.t @@ -113,11 +113,27 @@ subtest 'list_owners() and list_users() tests' => sub { } ); - $t->get_ok("//$userid:$password@/api/v1/acquisitions/funds/owners?q=$api_filter") - ->status_is(200)->json_is( [ $patron_with_permission->to_api, $superlibrarian->to_api ] ); - - $t->get_ok("//$userid:$password@/api/v1/acquisitions/funds/users?q=$api_filter") - ->status_is(200)->json_is( [ $patron_with_permission->to_api, $superlibrarian->to_api ] ); + $t->get_ok( + "//$userid:$password@/api/v1/acquisitions/funds/owners?q=$api_filter") + ->status_is(200)->json_is( + [ + $patron_with_permission->to_api( + { user => $patron_with_permission } + ), + $superlibrarian->to_api( { user => $superlibrarian } ) + ] + ); + + $t->get_ok( + "//$userid:$password@/api/v1/acquisitions/funds/users?q=$api_filter") + ->status_is(200)->json_is( + [ + $patron_with_permission->to_api( + { user => $patron_with_permission } + ), + $superlibrarian->to_api( { user => $superlibrarian } ) + ] + ); $schema->storage->txn_rollback; }; diff --git a/t/db_dependent/api/v1/patrons.t b/t/db_dependent/api/v1/patrons.t index 3d23b60372..6067034ec0 100755 --- a/t/db_dependent/api/v1/patrons.t +++ b/t/db_dependent/api/v1/patrons.t @@ -295,7 +295,13 @@ subtest 'add() tests' => sub { $schema->storage->txn_begin; - my $patron = $builder->build_object( { class => 'Koha::Patrons' } )->to_api; + my $librarian = $builder->build_object( + { + class => 'Koha::Patrons', + value => { flags => 2**4 } # borrowers flag = 4 + } + ); + my $patron = $builder->build_object( { class => 'Koha::Patrons' } )->to_api({ user => $librarian }); unauthorized_access_tests('POST', undef, $patron); @@ -336,8 +342,15 @@ subtest 'add() tests' => sub { } ); - my $patron = $builder->build_object({ class => 'Koha::Patrons' }); - my $newpatron = $patron->to_api; + my $librarian = $builder->build_object( + { + class => 'Koha::Patrons', + value => { flags => 2**4 } # borrowers flag = 4 + } + ); + + my $patron = $builder->build_object( { class => 'Koha::Patrons' } ); + my $newpatron = $patron->to_api({ user => $librarian }); # delete RO attributes delete $newpatron->{patron_id}; delete $newpatron->{restricted}; @@ -349,12 +362,6 @@ subtest 'add() tests' => sub { # Delete library $library_to_delete->delete; - my $librarian = $builder->build_object( - { - class => 'Koha::Patrons', - value => { flags => 2**4 } # borrowers flag = 4 - } - ); my $password = 'thePassword123'; $librarian->set_password( { password => $password, skip_validation => 1 } ); my $userid = $librarian->userid; @@ -390,7 +397,7 @@ subtest 'add() tests' => sub { delete $newpatron->{falseproperty}; my $patron_to_delete = $builder->build_object({ class => 'Koha::Patrons' }); - $newpatron = $patron_to_delete->to_api; + $newpatron = $patron_to_delete->to_api({ user => $librarian }); # delete RO attributes delete $newpatron->{patron_id}; delete $newpatron->{restricted}; @@ -615,7 +622,7 @@ subtest 'update() tests' => sub { my $patron_1 = $authorized_patron; my $patron_2 = $unauthorized_patron; - my $newpatron = $unauthorized_patron->to_api; + my $newpatron = $unauthorized_patron->to_api({ user => $authorized_patron }); # delete RO attributes delete $newpatron->{patron_id}; delete $newpatron->{restricted}; @@ -695,9 +702,9 @@ subtest 'update() tests' => sub { ->status_is(200, 'Patron updated successfully'); # Put back the RO attributes - $newpatron->{patron_id} = $unauthorized_patron->to_api->{patron_id}; - $newpatron->{restricted} = $unauthorized_patron->to_api->{restricted}; - $newpatron->{anonymized} = $unauthorized_patron->to_api->{anonymized}; + $newpatron->{patron_id} = $unauthorized_patron->to_api({ user => $authorized_patron })->{patron_id}; + $newpatron->{restricted} = $unauthorized_patron->to_api({ user => $authorized_patron })->{restricted}; + $newpatron->{anonymized} = $unauthorized_patron->to_api({ user => $authorized_patron })->{anonymized}; my $got = $result->tx->res->json; my $updated_on_got = delete $got->{updated_on}; -- 2.20.1