gitea
/
Koha-clms
1
0
Fork 0
Code Issues Pull Requests Packages Projects Releases Wiki Activity

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 <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
This commit is contained in:
Martin Renvoize 2022-06-28 16:50:29 +01:00 committed by Tomas Cohen Arazi
parent 2c502d320e
commit 743fac1fe0
No known key found for this signature in database
GPG Key ID: 0A272EA1B2F3C15F
7 changed files with 90 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

9
Koha/Patron.pm
View File

@ -41,6 +41,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;
@ -2288,11 +2289,9 @@ Returns 0 if the I<user> 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);

13
Koha/REST/V1/Patrons.pm
View File

@ -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') ) {

20
t/db_dependent/Koha/Patron.t
View File

@ -682,22 +682,26 @@ subtest 'to_api() tests' => sub {
my $patron = $builder->build_object(
{
class => 'Koha::Patrons',
value => {
debarred => undef
}
value => { debarred => undef }
}
);
my $restricted = $patron->to_api->{restricted};
my $consumer = $builder->build_object(
{
class => 'Koha::Patrons',
}
);
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' );

14
t/db_dependent/Koha/REST/Plugin/Objects.t
View File

@ -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;

12
t/db_dependent/api/v1/acquisitions_baskets.t
View File

@ -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;
};

24
t/db_dependent/api/v1/acquisitions_funds.t
View File

@ -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/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, $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(
{ user => $patron_with_permission }
),
$superlibrarian->to_api( { user => $superlibrarian } )
]
);
$schema->storage->txn_rollback;
};

35
t/db_dependent/api/v1/patrons.t
View File

@ -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};