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 <tomascohen@theke.io>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

diff --git a/Koha/Object.pm b/Koha/Object.pm
index a574bf8..3b2b804 100644 (file)
--- 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 d1495d3..6e7d8a1 100755 (executable)
--- 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;
 };
 
@@ -999,42 +1090,3 @@ subtest 'messages() and add_message() tests' => sub {
 
     $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 509d989..21f89f0 100755 (executable)
--- 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;