From df2ead4947d23d15ee419ed57f866795f9a83f0c 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 `user` parameter be passed to `is_accessible` which in turn makes `user` a required parameter for `to_api` in the `Koha::Patron` case. Signed-off-by: Jonathan Druart --- Koha/Patron.pm | 9 +++-- Koha/REST/V1/Patrons.pm | 13 +++++--- t/db_dependent/Koha/Patron.t | 20 ++++++----- t/db_dependent/Koha/REST/Plugin/Objects.t | 14 ++++++++ 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 ++++++++++++-------- 7 files changed, 91 insertions(+), 38 deletions(-) diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 09c47d5366e..d526bd19381 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -40,6 +40,7 @@ use Koha::CurbsidePickups; use Koha::Database; use Koha::DateUtils qw( dt_from_string ); use Koha::Encryption; +use Koha::Exceptions; use Koha::Exceptions::Password; use Koha::Holds; use Koha::Old::Checkouts; @@ -2299,11 +2300,9 @@ 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 - unless $params->{user}; + unless ( defined( $params->{user} ) ) { + Koha::Exceptions::MissingParameter->throw( error => "The `user` parameter is mandatory" ); + } my $consumer = $params->{user}; return $consumer->can_see_patron_infos($self); diff --git a/Koha/REST/V1/Patrons.pm b/Koha/REST/V1/Patrons.pm index d89d47e1027..903467f8ba7 100644 --- a/Koha/REST/V1/Patrons.pm +++ b/Koha/REST/V1/Patrons.pm @@ -103,7 +103,8 @@ Controller function that handles adding a new Koha::Patron object =cut sub add { - my $c = shift->openapi->valid_input or return; + my $c = shift->openapi->valid_input or return; + my $current_user = $c->stash('koha.user'); return try { @@ -168,7 +169,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 => $current_user } ) ); } ); @@ -260,7 +261,8 @@ Controller function that handles updating a Koha::Patron object =cut sub update { - my $c = shift->openapi->valid_input or return; + my $c = shift->openapi->valid_input or return; + my $current_user = $c->stash('koha.user'); my $patron = Koha::Patrons->find( $c->param('patron_id') ); @@ -307,7 +309,10 @@ sub update { $patron->set_from_api($body)->store; $patron->discard_changes; - return $c->render( status => 200, openapi => $patron->to_api ); + return $c->render( + status => 200, + openapi => $patron->to_api( { user => $current_user } ) + ); } catch { unless ( blessed $_ && $_->can('rethrow') ) { diff --git a/t/db_dependent/Koha/Patron.t b/t/db_dependent/Koha/Patron.t index ce59a5583b8..20d1ad11ce5 100755 --- a/t/db_dependent/Koha/Patron.t +++ b/t/db_dependent/Koha/Patron.t @@ -723,22 +723,26 @@ subtest 'to_api() tests' => sub { my $patron = $builder->build_object( { class => 'Koha::Patrons', - value => { - debarred => undef - } + value => { debarred => undef } + } + ); + + my $consumer = $builder->build_object( + { + class => 'Koha::Patrons', } ); - my $restricted = $patron->to_api->{restricted}; + my $restricted = $patron->to_api( { user => $consumer } )->{restricted}; ok( defined $restricted, 'restricted is defined' ); - ok( !$restricted, 'debarred is undef, restricted evaluates to false' ); + ok( !$restricted, 'debarred is undef, restricted evaluates to false' ); $patron->debarred( dt_from_string->add( days => 1 ) )->store->discard_changes; - $restricted = $patron->to_api->{restricted}; + $restricted = $patron->to_api( { user => $consumer } )->{restricted}; ok( defined $restricted, 'restricted is defined' ); - ok( $restricted, 'debarred is defined, restricted evaluates to true' ); + ok( $restricted, 'debarred is defined, restricted evaluates to true' ); - my $patron_json = $patron->to_api({ embed => { algo => {} } }); + my $patron_json = $patron->to_api( { embed => { algo => {} }, user => $consumer } ); ok( exists $patron_json->{algo} ); is( $patron_json->{algo}, 'algo' ); diff --git a/t/db_dependent/Koha/REST/Plugin/Objects.t b/t/db_dependent/Koha/REST/Plugin/Objects.t index 162b40f514d..b0eebbd492c 100755 --- a/t/db_dependent/Koha/REST/Plugin/Objects.t +++ b/t/db_dependent/Koha/REST/Plugin/Objects.t @@ -422,6 +422,16 @@ subtest 'objects.search helper with query parameter' => sub { my $suggestion3 = $builder->build_object( { class => "Koha::Suggestions", value => { suggestedby => $patron2->borrowernumber, biblionumber => $biblio3->biblionumber} } ); my $t = Test::Mojo->new; + + # Set valid koha.user in stash for to_api + my $superlibrarian = + $builder->build_object( { class => 'Koha::Patrons', value => { flags => 1 } } ); + $t->app->hook(before_dispatch => sub { + my $c = shift; + $c->stash('koha.user' => $superlibrarian); + }); + t::lib::Mocks::mock_userenv({ patron => $superlibrarian }); + $t->get_ok('/biblios' => json => {"suggestions.suggester.patron_id" => $patron1->borrowernumber }) ->json_is('/count' => 1, 'there should be 1 biblio with suggestions of patron 1'); @@ -906,6 +916,10 @@ subtest 'objects.find_rs helper' => sub { $schema->storage->txn_begin; + my $superlibrarian = + $builder->build_object( { class => 'Koha::Patrons', value => { flags => 1 } } ); + t::lib::Mocks::mock_userenv({ patron => $superlibrarian }); + # Remove existing cities to have more control on the search results Koha::Cities->delete; diff --git a/t/db_dependent/api/v1/acquisitions_baskets.t b/t/db_dependent/api/v1/acquisitions_baskets.t index 92261f5d88f..dd1edbb342d 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 d4821586f12..56283fe9f82 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 3eb4e3e5255..04060d46a20 100755 --- a/t/db_dependent/api/v1/patrons.t +++ b/t/db_dependent/api/v1/patrons.t @@ -322,7 +322,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); @@ -344,8 +350,15 @@ subtest 'add() tests' => sub { # Mock early, so existing mandatory attributes don't break all the tests my $mocked_patron = Test::MockModule->new('Koha::Patron'); - 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}; @@ -357,12 +370,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; @@ -396,7 +403,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}; @@ -646,7 +653,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}; @@ -723,9 +730,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.34.1