From 729e0543df7877a099ad323ed0995bd0de851ceb Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Fri, 21 Jan 2022 15:06:54 +0100 Subject: [PATCH] Bug 29697: Remove GetHiddenItemnumbers --- C4/Biblio.pm | 8 ++-- C4/Items.pm | 60 ------------------------ C4/Search.pm | 15 ++---- opac/opac-basket.pl | 31 +++---------- opac/opac-detail.pl | 29 ++++++------ opac/opac-search.pl | 2 +- opac/opac-tags.pl | 19 ++++---- t/db_dependent/Items.t | 101 +---------------------------------------- 8 files changed, 39 insertions(+), 226 deletions(-) diff --git a/C4/Biblio.pm b/C4/Biblio.pm index 8e4b5284666..fbf61ceb469 100644 --- a/C4/Biblio.pm +++ b/C4/Biblio.pm @@ -100,7 +100,7 @@ use C4::Charset qw( ); use C4::Linker; use C4::OAI::Sets; -use C4::Items qw( GetHiddenItemnumbers GetMarcItem ); +use C4::Items qw( GetMarcItem ); use Koha::Logger; use Koha::Caches; @@ -2546,15 +2546,13 @@ sub EmbedItemsInMarcBiblio { } push @items, { itemnumber => $itemnumber, item => $item }; } - my @items2pass = map { $_->{item} } @items; - my @hiddenitems = - $opachiddenitems - ? C4::Items::GetHiddenItemnumbers({ items => \@items2pass, borcat => $borcat }) : (); # Convert to a hash for quick searching my %hiddenitems = map { $_ => 1 } @hiddenitems; + my @itemnumbers = Koha::Items->search( { itemnumber => $itemnumbers } ) + ->filter_by_visible_in_opac({ patron => })->get_column('itemnumber'); foreach my $itemnumber ( map { $_->{itemnumber} } @items ) { next if $hiddenitems{$itemnumber}; my $item_marc = C4::Items::GetMarcItem( $biblionumber, $itemnumber ); diff --git a/C4/Items.pm b/C4/Items.pm index c10ca146f63..a4b7158a5ab 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -38,7 +38,6 @@ BEGIN { GetItemsLocationInfo GetHostItemsInfo get_hostitemnumbers_of - GetHiddenItemnumbers GetMarcItem CartToShelf GetAnalyticsCount @@ -979,65 +978,6 @@ sub get_hostitemnumbers_of { return @returnhostitemnumbers; } -=head2 GetHiddenItemnumbers - - my @itemnumbers_to_hide = GetHiddenItemnumbers({ items => \@items, borcat => $category }); - -Given a list of items it checks which should be hidden from the OPAC given -the current configuration. Returns a list of itemnumbers corresponding to -those that should be hidden. Optionally takes a borcat parameter for certain borrower types -to be excluded - -=cut - -sub GetHiddenItemnumbers { - my $params = shift; - my $items = $params->{items}; - if (my $exceptions = C4::Context->preference('OpacHiddenItemsExceptions') and $params->{'borcat'}){ - foreach my $except (split(/\|/, $exceptions)){ - if ($params->{'borcat'} eq $except){ - return; # we don't hide anything for this borrower category - } - } - } - my @resultitems; - - my $hidingrules = C4::Context->yaml_preference('OpacHiddenItems'); - - return - unless $hidingrules; - - my $dbh = C4::Context->dbh; - - # For each item - foreach my $item (@$items) { - - # We check each rule - foreach my $field (keys %$hidingrules) { - my $val; - if (exists $item->{$field}) { - $val = $item->{$field}; - } - else { - my $query = "SELECT $field from items where itemnumber = ?"; - $val = $dbh->selectrow_array($query, undef, $item->{'itemnumber'}); - } - $val = '' unless defined $val; - - # If the results matches the values in the yaml file - if (any { $val eq $_ } @{$hidingrules->{$field}}) { - - # We add the itemnumber to the list - push @resultitems, $item->{'itemnumber'}; - - # If at least one rule matched for an item, no need to test the others - last; - } - } - } - return @resultitems; -} - =head1 LIMITED USE FUNCTIONS The following functions, while part of the public API, diff --git a/C4/Search.pm b/C4/Search.pm index 484d9471d8e..a3bc5f90420 100644 --- a/C4/Search.pm +++ b/C4/Search.pm @@ -1802,21 +1802,16 @@ sub searchResults { } $item->{description} = $itemtypes{ $item->{itype} }{translated_description} if $item->{itype}; - # OPAC hidden items + # OPAC hidden items if ($is_opac) { - # hidden because lost - if ($hidelostitems && $item->{itemlost}) { + # hidden based on OpacHiddenItems syspref or because lost + my $hi = Koha::Items->search( { itemnumber => $item->{itemnumber} } ) + ->filter_by_visible_in_opac({ patron => $search_context->{patron} }); + unless ( $hi->count ) { push @hiddenitems, $item->{itemnumber}; $hideatopac_count++; next; } - # hidden based on OpacHiddenItems syspref - my @hi = C4::Items::GetHiddenItemnumbers({ items=> [ $item ], borcat => $search_context->{category} }); - if (scalar @hi) { - push @hiddenitems, @hi; - $hideatopac_count++; - next; - } } my $hbranch = C4::Context->preference('StaffSearchResultsDisplayBranch'); diff --git a/opac/opac-basket.pl b/opac/opac-basket.pl index d22106089cc..154c17ee9ad 100755 --- a/opac/opac-basket.pl +++ b/opac/opac-basket.pl @@ -26,7 +26,6 @@ use C4::Biblio qw( GetMarcSubjects GetMarcUrls ); -use C4::Items qw( GetHiddenItemnumbers GetItemsInfo ); use C4::Circulation qw( GetTransfers ); use C4::Auth qw( get_template_and_user ); use C4::Output qw( output_html_with_http_headers ); @@ -63,12 +62,7 @@ if (C4::Context->preference('TagsEnabled')) { } } -my $borcat = q{}; -if ( C4::Context->preference('OpacHiddenItemsExceptions') ) { - # we need to fetch the borrower info here, so we can pass the category - my $patron = Koha::Patrons->find($borrowernumber); - $borcat = $patron ? $patron->categorycode : $borcat; -} +my $logged_in_user = Koha::Patrons->find($borrowernumber); my $record_processor = Koha::RecordProcessor->new({ filters => 'ViewPolicy' }); my $rules = C4::Context->yaml_preference('OpacHiddenItems'); @@ -95,27 +89,16 @@ foreach my $biblionumber ( @bibs ) { my $marcseriesarray = GetMarcSeries ($record,$marcflavour); my $marcurlsarray = GetMarcUrls ($record,$marcflavour); - # grab all the items... - my @all_items = &GetItemsInfo( $biblionumber ); - - # determine which ones should be hidden / visible - my @hidden_items = GetHiddenItemnumbers({ items => \@all_items, borcat => $borcat }); - # If every item is hidden, then the biblio should be hidden too. next if $biblio->hidden_in_opac({ rules => $rules }); - # copy the visible ones into the items array. - my @items; - foreach my $item (@all_items) { - - # next if item is hidden - next if grep { $item->{itemnumber} eq $_ } @hidden_items ; - - my $reserve_status = C4::Reserves::GetReserveStatus($item->{itemnumber}); - if( $reserve_status eq "Waiting"){ $item->{'waiting'} = 1; } - if( $reserve_status eq "Reserved"){ $item->{'onhold'} = 1; } - push @items, $item; + # grab all the items... + my @items = $biblio->items->filter_by_visible_in_opac({ patron => $logged_in_user })->unblessed; + foreach my $item (@items) { + my $reserve_status = C4::Reserves::GetReserveStatus($item->{itemnumber}); + if( $reserve_status eq "Waiting"){ $item->{'waiting'} = 1; } + if( $reserve_status eq "Reserved"){ $item->{'onhold'} = 1; } } my $hasauthors = 0; diff --git a/opac/opac-detail.pl b/opac/opac-detail.pl index 343c9c5f5bf..9ee17f46d8e 100755 --- a/opac/opac-detail.pl +++ b/opac/opac-detail.pl @@ -45,7 +45,7 @@ use C4::Biblio qw( GetMarcSubjects GetMarcUrls ); -use C4::Items qw( GetHiddenItemnumbers GetItemsInfo ); +use C4::Items qw( GetItemsInfo ); use C4::Circulation qw( GetTransfers ); use C4::Tags qw( get_tags ); use C4::XISBN qw( get_xisbns ); @@ -108,7 +108,7 @@ if( $specific_item ) { @all_items = grep { $_->{itemnumber} == $query->param('itemnumber') } @all_items; $template->param( specific_item => 1 ); } -my @hiddenitems; +my @items_to_show; my $patron = Koha::Patrons->find( $borrowernumber ); my $biblio = Koha::Biblios->find( $biblionumber ); @@ -126,8 +126,8 @@ unless ( $patron and $patron->category->override_hidden_items ) { exit; } if ( scalar @all_items >= 1 ) { - push @hiddenitems, - GetHiddenItemnumbers( { items => \@all_items, borcat => $patron ? $patron->categorycode : undef } ); + @items_to_show = Koha::Items->search( { itemnumbers => [ map { $_->{itemnumber} } @all_items ] } ) + ->filter_by_visible_in_opac( { patron => $patron } ); } } @@ -249,8 +249,8 @@ if ($session->param('busc')) { my $hits; my @newresults; my $search_context = { - 'interface' => 'opac', - 'category' => ($patron) ? $patron->categorycode : q{} + interface => 'opac', + patron => $patron, }; for (my $i=0;$i<@servers;$i++) { my $server = $servers[$i]; @@ -513,18 +513,17 @@ if ( C4::Context->preference('EasyAnalyticalRecords') ) { my @items; # Are there items to hide? -my $hideitems; -$hideitems = 1 if C4::Context->preference('hidelostitems') or scalar(@hiddenitems) > 0; - # Hide items -if ($hideitems) { +if ( @items_to_show != @all_items ) { for my $itm (@all_items) { - if ( C4::Context->preference('hidelostitems') ) { - push @items, $itm unless $itm->{itemlost} or any { $itm->{'itemnumber'} eq $_ } @hiddenitems; - } else { - push @items, $itm unless any { $itm->{'itemnumber'} eq $_ } @hiddenitems; + next unless any { $itm->{itemnumber} eq $_ } @items_to_show; + if ( C4::Context->preference('hidelostitems') ) { + push @items, $itm unless $itm->{itemlost}; + } + else { + push @items, $itm; + } } -} } else { # Or not @items = @all_items; diff --git a/opac/opac-search.pl b/opac/opac-search.pl index 15fd6e52d25..e2fc5d2246f 100755 --- a/opac/opac-search.pl +++ b/opac/opac-search.pl @@ -587,7 +587,7 @@ my @sup_results_array; my $search_context = {}; $search_context->{'interface'} = 'opac'; if (C4::Context->preference('OpacHiddenItemsExceptions')){ - $search_context->{'category'} = $patron ? $patron->categorycode : q{}; + $search_context->{patron} = $patron; } my $variables = { anonymous_session => ($borrowernumber) ? 0 : 1 }; diff --git a/opac/opac-tags.pl b/opac/opac-tags.pl index 515f4ccd2e4..4e16f8c72a2 100755 --- a/opac/opac-tags.pl +++ b/opac/opac-tags.pl @@ -34,13 +34,14 @@ use Modern::Perl; use CGI qw ( -utf8 ); use CGI::Cookie; # need to check cookies before having CGI parse the POST request +use Array::Utils qw( array_minus ); use C4::Auth qw( check_cookie_auth get_template_and_user ); use C4::Context; use C4::Output qw( output_with_http_headers is_ajax output_html_with_http_headers ); use C4::Scrubber; use C4::Biblio qw( GetMarcBiblio ); -use C4::Items qw( GetHiddenItemnumbers GetItemsInfo ); +use C4::Items qw( GetItemsInfo ); use C4::Tags qw( add_tag get_approval_rows @@ -226,7 +227,6 @@ if ($is_ajax) { my $results = []; my $my_tags = []; -my $borcat = q{}; if ($loggedinuser) { my $patron = Koha::Patrons->find( { borrowernumber => $loggedinuser } ); @@ -258,15 +258,12 @@ if ($loggedinuser) { opac => 1, borcat => $borcat }); next unless $record; - my $hidden_items = undef; - my @hidden_itemnumbers; - my @all_items; + my @hidden_items; if ($should_hide) { - @all_items = GetItemsInfo( $tag->{biblionumber} ); - @hidden_itemnumbers = GetHiddenItemnumbers({ - items => \@all_items, - borcat => $borcat }); - $hidden_items = \@hidden_itemnumbers; + my $items = $biblio->items; + my @all_itemnumbers = $items->get_column('itemnumber'); + my @items_to_show = $items->filter_by_visible_in_opac({ opac => 1, patron => $patron }); + @hidden_items = array_minus( @all_itemnumbers, @items_to_show ); } next if ( @@ -294,7 +291,7 @@ if ($loggedinuser) { record => $record, xsl_syspref => 'OPACXSLTResultsDisplay', fix_amps => 1, - hidden_items => $hidden_items, + hidden_items => \@hidden_items, xslt_variables => $variables } ); diff --git a/t/db_dependent/Items.t b/t/db_dependent/Items.t index 3b5c622a3d0..20fe122d1cf 100755 --- a/t/db_dependent/Items.t +++ b/t/db_dependent/Items.t @@ -19,7 +19,7 @@ use Modern::Perl; use Data::Dumper; use MARC::Record; -use C4::Items qw( ModItemTransfer GetHiddenItemnumbers GetItemsInfo SearchItems AddItemFromMarc ModItemFromMarc get_hostitemnumbers_of Item2Marc ); +use C4::Items qw( ModItemTransfer GetItemsInfo SearchItems AddItemFromMarc ModItemFromMarc get_hostitemnumbers_of Item2Marc ); use C4::Biblio qw( GetMarcFromKohaField EmbedItemsInMarcBiblio GetMarcBiblio AddBiblio ); use C4::Circulation qw( AddIssue ); use Koha::Items; @@ -190,105 +190,6 @@ subtest 'ModItemTransfer tests' => sub { $schema->storage->txn_rollback; }; -subtest 'GetHiddenItemnumbers tests' => sub { - - plan tests => 11; - - # This sub is controlled by the OpacHiddenItems system preference. - - $schema->storage->txn_begin; - - my $builder = t::lib::TestBuilder->new; - my $library1 = $builder->build({ - source => 'Branch', - }); - - my $library2 = $builder->build({ - source => 'Branch', - }); - my $itemtype = $builder->build({ - source => 'Itemtype', - }); - - # Create a new biblio - t::lib::Mocks::mock_preference('marcflavour', 'MARC21'); - my $biblio = $builder->build_sample_biblio(); - - # Add two items - my $item1_itemnumber = $builder->build_sample_item( - { - biblionumber => $biblio->biblionumber, - library => $library1->{branchcode}, - withdrawn => 1, - itype => $itemtype->{itemtype} - } - )->itemnumber; - my $item2_itemnumber = $builder->build_sample_item( - { - biblionumber => $biblio->biblionumber, - library => $library2->{branchcode}, - withdrawn => 0, - itype => $itemtype->{itemtype} - } - )->itemnumber; - my $opachiddenitems; - my @itemnumbers = ($item1_itemnumber,$item2_itemnumber); - my @hidden; - my @items; - push @items, Koha::Items->find( $item1_itemnumber )->unblessed; - push @items, Koha::Items->find( $item2_itemnumber )->unblessed; - - # Empty OpacHiddenItems - t::lib::Mocks::mock_preference('OpacHiddenItems',''); - ok( !defined( GetHiddenItemnumbers( { items => \@items } ) ), - "Hidden items list undef if OpacHiddenItems empty"); - - # Blank spaces - t::lib::Mocks::mock_preference('OpacHiddenItems',' '); - ok( scalar GetHiddenItemnumbers( { items => \@items } ) == 0, - "Hidden items list empty if OpacHiddenItems only contains blanks"); - - # One variable / value - $opachiddenitems = " - withdrawn: [1]"; - t::lib::Mocks::mock_preference( 'OpacHiddenItems', $opachiddenitems ); - @hidden = GetHiddenItemnumbers( { items => \@items } ); - ok( scalar @hidden == 1, "Only one hidden item"); - is( $hidden[0], $item1_itemnumber, "withdrawn=1 is hidden"); - - # One variable, two values - $opachiddenitems = " - withdrawn: [1,0]"; - t::lib::Mocks::mock_preference( 'OpacHiddenItems', $opachiddenitems ); - @hidden = GetHiddenItemnumbers( { items => \@items } ); - ok( scalar @hidden == 2, "Two items hidden"); - is_deeply( \@hidden, \@itemnumbers, "withdrawn=1 and withdrawn=0 hidden"); - - # Two variables, a value each - $opachiddenitems = " - withdrawn: [1] - homebranch: [$library2->{branchcode}] - "; - t::lib::Mocks::mock_preference( 'OpacHiddenItems', $opachiddenitems ); - @hidden = GetHiddenItemnumbers( { items => \@items } ); - ok( scalar @hidden == 2, "Two items hidden"); - is_deeply( \@hidden, \@itemnumbers, "withdrawn=1 and homebranch library2 hidden"); - - # Override hidden with patron category - t::lib::Mocks::mock_preference( 'OpacHiddenItemsExceptions', 'S' ); - @hidden = GetHiddenItemnumbers( { items => \@items, borcat => 'PT' } ); - ok( scalar @hidden == 2, "Two items still hidden"); - @hidden = GetHiddenItemnumbers( { items => \@items, borcat => 'S' } ); - ok( scalar @hidden == 0, "Two items not hidden"); - - # Valid OpacHiddenItems, empty list - @items = (); - @hidden = GetHiddenItemnumbers( { items => \@items } ); - ok( scalar @hidden == 0, "Empty items list, no item hidden"); - - $schema->storage->txn_rollback; -}; - subtest 'GetItemsInfo tests' => sub { plan tests => 9; -- 2.25.1