From 80f8d1ce4b2e0bff57a7db28a29c9dd373a84a0a Mon Sep 17 00:00:00 2001 From: Tomas Cohen Arazi Date: Fri, 28 Oct 2022 19:25:46 -0300 Subject: [PATCH] Bug 26635: Refined data structure and methods This patch makes the returned data structure be simpler: _str => { attribute_1 => { category => 'some_category_name', str => 'description', type => 'av' }, ... } The description is sensible to context, so if public => 1 is passed, then lib_opac is passed, and lib is returned otherwise. Whenever we add language to the combo, we will add it to the implementation. Tests are adjusted accordingly, also to reflect the public => 1 use case. To test: 1. Apply this patch 2. Run: $ kshell k$ prove t/db_dependent/Koha/Object.t \ t/db_dependent/Koha/REST/Plugin/Objects.t => SUCCESS: Tests pass! 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi Signed-off-by: Martin Renvoize --- Koha/Object.pm | 6 +- t/db_dependent/Koha/Object.t | 134 +++++++++++++++------- t/db_dependent/Koha/REST/Plugin/Objects.t | 70 +++++++---- 3 files changed, 144 insertions(+), 66 deletions(-) diff --git a/Koha/Object.pm b/Koha/Object.pm index a574bf8fc7..3b2b80407b 100644 --- a/Koha/Object.pm +++ b/Koha/Object.pm @@ -555,8 +555,8 @@ sub to_api { # coded values handling my $avs = {}; - if ( $params->{av_expand} and $self->can('_fetch_authorised_values') ) { - $avs = $self->_fetch_authorised_values; + if ( $params->{av_expand} and $self->can('api_av_mapping') ) { + $avs = $self->api_av_mapping($params); } # Remove forbidden attributes if required (including their coded values) @@ -597,7 +597,7 @@ sub to_api { } } - $json_object->{_authorised_values} = $avs + $json_object->{_str} = $avs if $params->{av_expand}; # Make sure we duplicate the $params variable to avoid diff --git a/t/db_dependent/Koha/Object.t b/t/db_dependent/Koha/Object.t index d1495d3a33..6e7d8a1a9f 100755 --- a/t/db_dependent/Koha/Object.t +++ b/t/db_dependent/Koha/Object.t @@ -17,7 +17,7 @@ use Modern::Perl; -use Test::More tests => 22; +use Test::More tests => 21; use Test::Exception; use Test::Warn; use DateTime; @@ -226,7 +226,7 @@ subtest 'TO_JSON tests' => sub { subtest "to_api() tests" => sub { - plan tests => 29; + plan tests => 30; $schema->storage->txn_begin; @@ -423,6 +423,97 @@ subtest "to_api() tests" => sub { } }; + subtest 'Authorised values expansion' => sub { + + plan tests => 4; + + $schema->storage->txn_begin; + + # new category + my $category = $builder->build_object({ class => 'Koha::AuthorisedValueCategories' }); + # add two countries + my $argentina = $builder->build_object( + { class => 'Koha::AuthorisedValues', + value => { + category => $category->category_name, + lib => 'AR (Argentina)', + lib_opac => 'Argentina', + } + } + ); + my $france = $builder->build_object( + { class => 'Koha::AuthorisedValues', + value => { + category => $category->category_name, + lib => 'FR (France)', + lib_opac => 'France', + } + } + ); + + my $city_mock = Test::MockModule->new('Koha::City'); + $city_mock->mock( + 'api_av_mapping', + sub { + my ( $self, $params ) = @_; + + my $av = Koha::AuthorisedValues->find( + { + authorised_value => $self->city_country, + category => $category->category_name, + } + ); + + return { + city_country => { + category => $av->category, + str => ( $params->{public} ) ? $av->lib_opac : $av->lib, + type => 'av', + } + }; + } + ); + $city_mock->mock( 'public_read_list', sub { return [ 'city_id', 'city_country', 'city_name', 'city_state' ] } ); + + my $cordoba = $builder->build_object( + { class => 'Koha::Cities', + value => { city_country => $argentina->authorised_value, city_name => 'Cordoba' } + } + ); + my $marseille = $builder->build_object( + { class => 'Koha::Cities', + value => { city_country => $france->authorised_value, city_name => 'Marseille' } + } + ); + + my $mobj = $marseille->to_api( { av_expand => 1, public => 1 } ); + my $cobj = $cordoba->to_api( { av_expand => 1, public => 0 } ); + + ok( exists $mobj->{_str}, '_str exists for Marseille' ); + ok( exists $cobj->{_str}, '_str exists for Córdoba' ); + + is_deeply( + $mobj->{_str}->{country}, + { + category => $category->category_name, + str => $france->lib_opac, + type => 'av', + }, + 'Authorised value for country expanded' + ); + is_deeply( + $cobj->{_str}->{country}, + { + category => $category->category_name, + str => $argentina->lib, + type => 'av' + }, + 'Authorised value for country expanded' + ); + + $schema->storage->txn_rollback; + }; + $schema->storage->txn_rollback; }; @@ -997,44 +1088,5 @@ subtest 'messages() and add_message() tests' => sub { isnt( $patron->object_messages, undef, '->messages initializes the array if required' ); is( scalar @{ $patron->object_messages }, 0, '->messages returns an empty arrayref' ); - $schema->storage->txn_rollback; -}; - -subtest 'Authorised values expansion' => sub { - plan tests => 4; - - $schema->storage->txn_begin; - - Koha::AuthorisedValues->search({category => 'Countries'})->delete; - Koha::AuthorisedValueCategories->search({category_name =>'Countries'})->delete; - - my $cat = $builder->build_object({ class => 'Koha::AuthorisedValueCategories', value => {category_name =>'Countries'} }); - my $fr = $builder->build_object({ class => 'Koha::AuthorisedValues', value => {authorised_value => 'FR', lib=>'France', category=>$cat->category_name} }); - my $us = $builder->build_object({ class => 'Koha::AuthorisedValues', value => {authorised_value => 'US', lib=>'United States of America', category=>$cat->category_name} }); - my $ar = $builder->build_object({ class => 'Koha::AuthorisedValues', value => {authorised_value => 'AR', lib=>'Argentina', category=>$cat->category_name} }); - - my $city_class = Test::MockModule->new('Koha::City'); - $city_class->mock( '_fetch_authorised_values', - sub { - my ($self) = @_; - use Koha::AuthorisedValues; - my $av = Koha::AuthorisedValues->find({authorised_value => $self->city_country, category => 'Countries'}); - return {country => $av->unblessed}; - } - ); - - my $marseille = $builder->build_object({ class => 'Koha::Cities', value => {city_country => 'FR', city_name => 'Marseille'} }); - my $cordoba = $builder->build_object({ class => 'Koha::Cities', value => {city_country => 'AR', city_name => 'Córdoba'} }); - - my $mobj = $marseille->to_api({av_expand => 1}); - my $cobj = $cordoba->to_api({av_expand => 1}); - - isnt($mobj->{_authorised_values}, undef, '_authorised_values exists for Marseille'); - isnt($cobj->{_authorised_values}, undef, '_authorised_values exists for Córdoba'); - - is($mobj->{_authorised_values}->{country}->{lib}, $fr->lib, 'Authorised value for country expanded'); - is($cobj->{_authorised_values}->{country}->{lib}, $ar->lib, 'Authorised value for country expanded'); - - $schema->storage->txn_rollback; }; diff --git a/t/db_dependent/Koha/REST/Plugin/Objects.t b/t/db_dependent/Koha/REST/Plugin/Objects.t index 509d98975b..21f89f0639 100755 --- a/t/db_dependent/Koha/REST/Plugin/Objects.t +++ b/t/db_dependent/Koha/REST/Plugin/Objects.t @@ -621,7 +621,8 @@ subtest 'objects.search helper, search_limited() tests' => sub { }; subtest 'objects.find helper with expanded authorised values' => sub { - plan tests => 14; + + plan tests => 18; $schema->storage->txn_begin; @@ -670,17 +671,25 @@ subtest 'objects.find helper with expanded authorised values' => sub { my $city_class = Test::MockModule->new('Koha::City'); $city_class->mock( - '_fetch_authorised_values', + 'api_av_mapping', sub { - my ($self) = @_; + my ($self, $params) = @_; use Koha::AuthorisedValues; + my $av = Koha::AuthorisedValues->find( { authorised_value => $self->city_country, category => 'Countries' } ); - return { country => $av->unblessed }; + + return { + city_country => { + category => $av->category, + str => ( $params->{public} ) ? $av->lib_opac : $av->lib, + type => 'av', + } + }; } ); @@ -703,26 +712,30 @@ subtest 'objects.find helper with expanded authorised values' => sub { } ); - $t->get_ok( '/cities/' . $manuel->cityid => { 'x-koha-av-expand' => 1 } ) + $t->get_ok( '/cities/' . $manuel->id => { 'x-koha-av-expand' => 1 } ) ->status_is(200)->json_is( '/name' => 'Manuel' ) - ->json_has('/_authorised_values') - ->json_is( '/_authorised_values/country/lib' => $ar->lib ); + ->json_has('/_str') + ->json_is( '/_str/country/type' => 'av' ) + ->json_is( '/_str/country/category' => $cat->category_name ) + ->json_is( '/_str/country/str' => $ar->lib ); - $t->get_ok( '/cities/' . $manuel->cityid => { 'x-koha-av-expand' => 0 } ) + $t->get_ok( '/cities/' . $manuel->id => { 'x-koha-av-expand' => 0 } ) ->status_is(200)->json_is( '/name' => 'Manuel' ) - ->json_hasnt('/_authorised_values'); + ->json_hasnt('/_str'); - $t->get_ok( '/cities/' . $manuela->cityid => { 'x-koha-av-expand' => 1 } ) + $t->get_ok( '/cities/' . $manuela->id => { 'x-koha-av-expand' => 1 } ) ->status_is(200)->json_is( '/name' => 'Manuela' ) - ->json_has('/_authorised_values') - ->json_is( '/_authorised_values/country/lib' => $us->lib ); + ->json_has('/_str') + ->json_is( '/_str/country/type' => 'av' ) + ->json_is( '/_str/country/category' => $cat->category_name ) + ->json_is( '/_str/country/str' => $us->lib ); $schema->storage->txn_rollback; }; subtest 'objects.search helper with expanded authorised values' => sub { - plan tests => 20; + plan tests => 24; my $t = Test::Mojo->new; @@ -771,20 +784,29 @@ subtest 'objects.search helper with expanded authorised values' => sub { my $city_class = Test::MockModule->new('Koha::City'); $city_class->mock( - '_fetch_authorised_values', + 'api_av_mapping', sub { - my ($self) = @_; + my ($self, $params) = @_; use Koha::AuthorisedValues; + my $av = Koha::AuthorisedValues->find( { authorised_value => $self->city_country, category => 'Countries' } ); - return { country => $av->unblessed }; + + return { + city_country => { + category => $av->category, + str => ( $params->{public} ) ? $av->lib_opac : $av->lib, + type => 'av', + } + }; } ); + $builder->build_object( { class => 'Koha::Cities', @@ -808,17 +830,21 @@ subtest 'objects.search helper with expanded authorised values' => sub { { 'x-koha-av-expand' => 1 } )->status_is(200) ->json_has('/0')->json_has('/1')->json_hasnt('/2') ->json_is( '/0/name' => 'Manuel' ) - ->json_has('/0/_authorised_values') - ->json_is( '/0/_authorised_values/country/lib' => $ar->lib ) + ->json_has('/0/_str') + ->json_is( '/0/_str/country/str' => $ar->lib ) + ->json_is( '/0/_str/country/type' => 'av' ) + ->json_is( '/0/_str/country/category' => $cat->category_name ) ->json_is( '/1/name' => 'Manuela' ) - ->json_has('/1/_authorised_values') - ->json_is( '/1/_authorised_values/country/lib' => $us->lib ); + ->json_has('/1/_str') + ->json_is( '/1/_str/country/str' => $us->lib ) + ->json_is( '/1/_str/country/type' => 'av' ) + ->json_is( '/1/_str/country/category' => $cat->category_name ); $t->get_ok( '/cities?name=manuel&_per_page=4&_page=1&_match=starts_with' => { 'x-koha-av-expand' => 0 } )->status_is(200) ->json_has('/0')->json_has('/1')->json_hasnt('/2') - ->json_is( '/0/name' => 'Manuel' )->json_hasnt('/0/_authorised_values') - ->json_is( '/1/name' => 'Manuela' )->json_hasnt('/1/_authorised_values'); + ->json_is( '/0/name' => 'Manuel' )->json_hasnt('/0/_str') + ->json_is( '/1/name' => 'Manuela' )->json_hasnt('/1/_str'); $schema->storage->txn_rollback; -- 2.20.1