From 53da84677015871d5d831490f3bcde7f64e5fd8c Mon Sep 17 00:00:00 2001 From: Chris Cormack Date: Thu, 6 Nov 2014 10:34:18 +1300 Subject: [PATCH] Bug 14385: Extend OpacHiddenItems to allow specifying exempt borrower categories Changes the API for calling GetHiddenItems and all the places in the code that call it. This is to allow borrower categories to be passed in. Adds an OpacHiddenItemsExceptions syspref to allow certain borrower categories to be able to see items, even if they are marked hidden by OpacHiddenItems To test: 1) Make two borrowers, one in a category that should see everything (ie Adult), and another in a category that should only see certain things (ie Adult - exceptions) 2) Add the borrower that can see everything (the Adult) to OpacHiddenItemsExceptions 3) To the OpacHiddenItems syspref, add an item type (ensure that you have some records that fall under this type in your library). 4) Log in as the borrower that should only see certain things (Adult - exception) 5) Do a search, filtered to show records which are the item type that you specified in the OpacHiddenItems syspref. No records should show for this borrower as this item type is hidden to them. 6) Log in as the borrower that should see everything (Adult) 7) Do the same search. There should be results from this search, as this borrower category has been specified as an exception to the hidden items --- C4/Items.pm | 17 +++++++++++++---- C4/Search.pm | 14 +++++++------- catalogue/search.pl | 2 +- cataloguing/addbooks.pl | 5 ++++- .../prog/en/modules/admin/preferences/opac.pref | 3 +++ opac/opac-ISBDdetail.pl | 10 +++++++++- opac/opac-MARCdetail.pl | 9 ++++++++- opac/opac-detail.pl | 15 ++++++++++++--- opac/opac-search.pl | 13 +++++++++++-- t/db_dependent/Items.t | 12 ++++++------ t/db_dependent/Search.t | 12 ++++++------ 11 files changed, 80 insertions(+), 32 deletions(-) diff --git a/C4/Items.pm b/C4/Items.pm index d1a866b..73dfbc4 100644 --- a/C4/Items.pm +++ b/C4/Items.pm @@ -1696,16 +1696,25 @@ sub GetBarcodeFromItemnumber { =head2 GetHiddenItemnumbers - my @itemnumbers_to_hide = GetHiddenItemnumbers(@items); + 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. +those that should be hidden. Optionally takes a borcat parameter for certain borrower types +to be excluded =cut sub GetHiddenItemnumbers { - my (@items) = @_; + 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 dont hide anything for this borrower category + } + } + } my @resultitems; my $yaml = C4::Context->preference('OpacHiddenItems'); @@ -1722,7 +1731,7 @@ sub GetHiddenItemnumbers { my $dbh = C4::Context->dbh; # For each item - foreach my $item (@items) { + foreach my $item (@$items) { # We check each rule foreach my $field (keys %$hidingrules) { diff --git a/C4/Search.pm b/C4/Search.pm index 990ed39..7967077 100644 --- a/C4/Search.pm +++ b/C4/Search.pm @@ -1846,9 +1846,9 @@ sub searchResults { require C4::Items; - $search_context = 'opac' if !$search_context || $search_context ne 'intranet'; + $search_context->{'interface'} = 'opac' if !$search_context->{'interface'} || $search_context->{'interface'} ne 'intranet'; my ($is_opac, $hidelostitems); - if ($search_context eq 'opac') { + if ($search_context->{'interface'} eq 'opac') { $hidelostitems = C4::Context->preference('hidelostitems'); $is_opac = 1; } @@ -1946,7 +1946,7 @@ sub searchResults { # add imageurl to itemtype if there is one $oldbiblio->{imageurl} = getitemtypeimagelocation( $search_context, $itemtypes{ $oldbiblio->{itemtype} }->{imageurl} ); - $oldbiblio->{'authorised_value_images'} = ($search_context eq 'opac' && C4::Context->preference('AuthorisedValueImages')) || ($search_context eq 'intranet' && C4::Context->preference('StaffAuthorisedValueImages')) ? C4::Items::get_authorised_value_images( C4::Biblio::get_biblio_authorised_values( $oldbiblio->{'biblionumber'}, $marcrecord ) ) : []; + $oldbiblio->{'authorised_value_images'} = ($search_context->{'interface'} eq 'opac' && C4::Context->preference('AuthorisedValueImages')) || ($search_context->{'interface'} eq 'intranet' && C4::Context->preference('StaffAuthorisedValueImages')) ? C4::Items::get_authorised_value_images( C4::Biblio::get_biblio_authorised_values( $oldbiblio->{'biblionumber'}, $marcrecord ) ) : []; $oldbiblio->{normalized_upc} = GetNormalizedUPC( $marcrecord,$marcflavour); $oldbiblio->{normalized_ean} = GetNormalizedEAN( $marcrecord,$marcflavour); $oldbiblio->{normalized_oclc} = GetNormalizedOCLCNumber($marcrecord,$marcflavour); @@ -2077,7 +2077,7 @@ sub searchResults { next; } # hidden based on OpacHiddenItems syspref - my @hi = C4::Items::GetHiddenItemnumbers($item); + my @hi = C4::Items::GetHiddenItemnumbers({ items=> [ $item ], borcat => $search_context->{category} }); if (scalar @hi) { push @hiddenitems, @hi; $hideatopac_count++; @@ -2201,7 +2201,7 @@ sub searchResults { $other_items->{$key}->{count}++ if $item->{$hbranch}; $other_items->{$key}->{location} = $shelflocations->{ $item->{location} }; $other_items->{$key}->{description} = $item->{description}; - $other_items->{$key}->{imageurl} = getitemtypeimagelocation( $search_context, $itemtypes{ $item->{itype} }->{imageurl} ); + $other_items->{$key}->{imageurl} = getitemtypeimagelocation( $search_context->{'interface'}, $itemtypes{ $item->{itype} }->{imageurl} ); } # item is available else { @@ -2212,7 +2212,7 @@ sub searchResults { $available_items->{$prefix}->{$_} = $item->{$_}; } $available_items->{$prefix}->{location} = $shelflocations->{ $item->{location} }; - $available_items->{$prefix}->{imageurl} = getitemtypeimagelocation( $search_context, $itemtypes{ $item->{itype} }->{imageurl} ); + $available_items->{$prefix}->{imageurl} = getitemtypeimagelocation( $search_context->{'interface'}, $itemtypes{ $item->{itype} }->{imageurl} ); } } } # notforloan, item level and biblioitem level @@ -2237,7 +2237,7 @@ sub searchResults { } # XSLT processing of some stuff - my $interface = $search_context eq 'opac' ? 'OPAC' : ''; + my $interface = $search_context->{'interface'} eq 'opac' ? 'OPAC' : ''; if (!$scan && C4::Context->preference($interface . "XSLTResultsDisplay")) { $oldbiblio->{XSLTResultsRecord} = XSLTParse4Display($oldbiblio->{biblionumber}, $marcrecord, $interface."XSLTResultsDisplay", 1, \@hiddenitems); # the last parameter tells Koha to clean up the problematic ampersand entities that Zebra outputs diff --git a/catalogue/search.pl b/catalogue/search.pl index 4d165ac..0017e27 100755 --- a/catalogue/search.pl +++ b/catalogue/search.pl @@ -553,7 +553,7 @@ for (my $i=0;$i<@servers;$i++) { if ($server =~/biblioserver/) { # this is the local bibliographic server $hits = $results_hashref->{$server}->{"hits"}; my $page = $cgi->param('page') || 0; - my @newresults = searchResults('intranet', $query_desc, $hits, $results_per_page, $offset, $scan, + my @newresults = searchResults({ 'interface' => 'intranet' }, $query_desc, $hits, $results_per_page, $offset, $scan, $results_hashref->{$server}->{"RECORDS"}); $total = $total + $results_hashref->{$server}->{"hits"}; diff --git a/cataloguing/addbooks.pl b/cataloguing/addbooks.pl index 038f291..9088d01 100755 --- a/cataloguing/addbooks.pl +++ b/cataloguing/addbooks.pl @@ -94,7 +94,10 @@ if ($query) { # format output # SimpleSearch() give the results per page we want, so 0 offet here my $total = @{$marcresults}; - my @newresults = searchResults( 'intranet', $query, $total, $results_per_page, 0, 0, $marcresults ); + my @newresults = searchResults( {'interface' => 'intranet'}, $query, $total, $results_per_page, 0, 0, $marcresults ); + foreach my $line (@newresults) { + if ( not exists $line->{'size'} ) { $line->{'size'} = "" } + } $template->param( total => $total_hits, query => $query, diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref index f87f5d5..a94b42b 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/opac.pref @@ -534,6 +534,9 @@ OPAC: class: code - Allows to define custom rules for hiding specific items at opac. See docs/opac/OpacHiddenItems.txt for more informations - + - pref: OpacHiddenItemsExceptions + - List of borrower categories, separated by |, that can see items otherwise hidden by OpacHiddenItems + - - pref: OpacAllowPublicListCreation default: 1 choices: diff --git a/opac/opac-ISBDdetail.pl b/opac/opac-ISBDdetail.pl index 7fb1073..bce8f71 100755 --- a/opac/opac-ISBDdetail.pl +++ b/opac/opac-ISBDdetail.pl @@ -84,7 +84,15 @@ my $marcflavour = C4::Context->preference("marcflavour"); my @items = GetItemsInfo($biblionumber); if (scalar @items >= 1) { - my @hiddenitems = GetHiddenItemnumbers(@items); + my $borcat; + if ( C4::Context->preference('OpacHiddenItemsExceptions') ) { + + # we need to fetch the borrower info here, so we can pass the category + my $borrower = GetMember( borrowernumber => $borrowernumber ); + $borcat = $borrower->{categorycode}; + } + + my @hiddenitems = GetHiddenItemnumbers( { items => \@items, borcat => $borcat }); if (scalar @hiddenitems == scalar @items ) { print $query->redirect("/cgi-bin/koha/errors/404.pl"); # escape early diff --git a/opac/opac-MARCdetail.pl b/opac/opac-MARCdetail.pl index afac5d7..90fda24 100755 --- a/opac/opac-MARCdetail.pl +++ b/opac/opac-MARCdetail.pl @@ -71,7 +71,14 @@ if ( ! $biblionumber ) { my @all_items = GetItemsInfo($biblionumber); my @items2hide; if (scalar @all_items >= 1) { - push @items2hide, GetHiddenItemnumbers(@all_items); + my $borcat; + if ( C4::Context->preference('OpacHiddenItemsExceptions') ) { + + # we need to fetch the borrower info here, so we can pass the category + my $borrower = GetMember( borrowernumber => $borrowernumber ); + $borcat = $borrower->{categorycode}; + } + push @items2hide, GetHiddenItemnumbers({ items => \@all_items, borcat => $botcat }); if (scalar @items2hide == scalar @all_items ) { print $query->redirect("/cgi-bin/koha/errors/404.pl"); diff --git a/opac/opac-detail.pl b/opac/opac-detail.pl index bc9b323..11c3040 100755 --- a/opac/opac-detail.pl +++ b/opac/opac-detail.pl @@ -75,8 +75,17 @@ $biblionumber = int($biblionumber); my @all_items = GetItemsInfo($biblionumber); my @hiddenitems; -if (scalar @all_items >= 1) { - push @hiddenitems, GetHiddenItemnumbers(@all_items); +if ( scalar @all_items >= 1 ) { + my $borcat; + if ( C4::Context->preference('OpacHiddenItemsExceptions') ) { + + # we need to fetch the borrower info here, so we can pass the category + my $borrower = GetMember( borrowernumber => $borrowernumber ); + $borcat = $borrower->{categorycode}; + } + + push @hiddenitems, + GetHiddenItemnumbers( { items => \@all_items, borcat => $borcat } ); if (scalar @hiddenitems == scalar @all_items ) { print $query->redirect("/cgi-bin/koha/errors/404.pl"); # escape early @@ -206,7 +215,7 @@ if ($session->param('busc')) { for (my $i=0;$i<@servers;$i++) { my $server = $servers[$i]; $hits = $results_hashref->{$server}->{"hits"}; - @newresults = searchResults('opac', '', $hits, $results_per_page, $offset, $arrParamsBusc->{'scan'}, $results_hashref->{$server}->{"RECORDS"}); + @newresults = searchResults({ 'interface' => 'opac' }, '', $hits, $results_per_page, $offset, $arrParamsBusc->{'scan'}, $results_hashref->{$server}->{"RECORDS"}); } return \@newresults; }#searchAgain diff --git a/opac/opac-search.pl b/opac/opac-search.pl index 0ff2507..2c9d002 100755 --- a/opac/opac-search.pl +++ b/opac/opac-search.pl @@ -42,6 +42,7 @@ use C4::Branch; # GetBranches use C4::SocialData; use C4::Ratings; use C4::External::OverDrive; +use C4::Borrowers qw(GetMember); use POSIX qw(ceil floor strftime); use URI::Escape; @@ -593,6 +594,14 @@ if ($@ || $error) { # At this point, each server has given us a result set # now we build that set for template display my @sup_results_array; +my $search_context = {}; +$search_context->{'interface'} = 'opac'; +if (C4::Context->preference('OpacHiddenItemsExceptions')){ + # we need to fetch the borrower info here, so we can pass the category + my $borrower = GetMember( borrowernumber => $borrowernumber ); + $search_context->{'category'} = $borrower->{'categorycode'}; +} + for (my $i=0;$i<@servers;$i++) { my $server = $servers[$i]; if ($server && $server =~/biblioserver/) { # this is the local bibliographic server @@ -604,12 +613,12 @@ for (my $i=0;$i<@servers;$i++) { # because pazGetRecords handles retieving only the records # we want as specified by $offset and $results_per_page, # we need to set the offset parameter of searchResults to 0 - my @group_results = searchResults( 'opac', $query_desc, $group->{'group_count'},$results_per_page, 0, $scan, + my @group_results = searchResults( $search_context, $query_desc, $group->{'group_count'},$results_per_page, 0, $scan, $group->{"RECORDS"}); push @newresults, { group_label => $group->{'group_label'}, GROUP_RESULTS => \@group_results }; } } else { - @newresults = searchResults('opac', $query_desc, $hits, $results_per_page, $offset, $scan, + @newresults = searchResults( $search_context, $query_desc, $hits, $results_per_page, $offset, $scan, $results_hashref->{$server}->{"RECORDS"}); } $hits = 0 unless @newresults; diff --git a/t/db_dependent/Items.t b/t/db_dependent/Items.t index b41b483..d4e9b8e 100755 --- a/t/db_dependent/Items.t +++ b/t/db_dependent/Items.t @@ -113,19 +113,19 @@ subtest 'GetHiddenItemnumbers tests' => sub { # Empty OpacHiddenItems C4::Context->set_preference('OpacHiddenItems',''); - ok( !defined( GetHiddenItemnumbers( @items ) ), + ok( !defined( GetHiddenItemnumbers( { items => \@items } ) ), "Hidden items list undef if OpacHiddenItems empty"); # Blank spaces C4::Context->set_preference('OpacHiddenItems',' '); - ok( scalar GetHiddenItemnumbers( @items ) == 0, + ok( scalar GetHiddenItemnumbers( { items => \@items } ) == 0, "Hidden items list empty if OpacHiddenItems only contains blanks"); # One variable / value $opachiddenitems = " withdrawn: [1]"; C4::Context->set_preference( 'OpacHiddenItems', $opachiddenitems ); - @hidden = GetHiddenItemnumbers( @items ); + @hidden = GetHiddenItemnumbers( { items => \@items } ); ok( scalar @hidden == 1, "Only one hidden item"); is( $hidden[0], $item1_itemnumber, "withdrawn=1 is hidden"); @@ -133,7 +133,7 @@ subtest 'GetHiddenItemnumbers tests' => sub { $opachiddenitems = " withdrawn: [1,0]"; C4::Context->set_preference( 'OpacHiddenItems', $opachiddenitems ); - @hidden = GetHiddenItemnumbers( @items ); + @hidden = GetHiddenItemnumbers( { items => \@items } ); ok( scalar @hidden == 2, "Two items hidden"); is_deeply( \@hidden, \@itemnumbers, "withdrawn=1 and withdrawn=0 hidden"); @@ -143,13 +143,13 @@ subtest 'GetHiddenItemnumbers tests' => sub { homebranch: [$branch2] "; C4::Context->set_preference( 'OpacHiddenItems', $opachiddenitems ); - @hidden = GetHiddenItemnumbers( @items ); + @hidden = GetHiddenItemnumbers( { items => \@items } ); ok( scalar @hidden == 2, "Two items hidden"); is_deeply( \@hidden, \@itemnumbers, "withdrawn=1 and homebranch=MPL hidden"); # Valid OpacHiddenItems, empty list @items = (); - @hidden = GetHiddenItemnumbers( @items ); + @hidden = GetHiddenItemnumbers( { items => \@items } ); ok( scalar @hidden == 0, "Empty items list, no item hidden"); $dbh->rollback; diff --git a/t/db_dependent/Search.t b/t/db_dependent/Search.t index b96cc19..93f215a 100644 --- a/t/db_dependent/Search.t +++ b/t/db_dependent/Search.t @@ -467,7 +467,7 @@ if ( $indexing_mode eq 'dom' ) { ($error, $results_hashref, $facets_loop) = getRecords($query,$simple_query,[ ], [ 'biblioserver' ],20,0,undef,\%branches,\%itemtypes,$query_type,0); is($results_hashref->{biblioserver}->{hits}, 19, "getRecords generated keyword search for 'salud' matched right number of records"); - my @newresults = searchResults('opac', $query_desc, $results_hashref->{'biblioserver'}->{'hits'}, 18, 0, 0, + my @newresults = searchResults({'interface' => 'opac'}, $query_desc, $results_hashref->{'biblioserver'}->{'hits'}, 18, 0, 0, $results_hashref->{'biblioserver'}->{"RECORDS"}); is(scalar @newresults,18, "searchResults returns requested number of hits"); @@ -549,7 +549,7 @@ if ( $indexing_mode eq 'dom' ) { ($error, $results_hashref, $facets_loop) = getRecords($query,$simple_query,[ ], [ 'biblioserver' ],20,0,undef,\%branches,\%itemtypes,$query_type,0); is($results_hashref->{biblioserver}->{hits}, 26, "getRecords generated availability-limited search matched right number of records"); - @newresults = searchResults('opac', $query_desc, $results_hashref->{'biblioserver'}->{'hits'}, 17, 0, 0, + @newresults = searchResults({'interface'=>'opac'}, $query_desc, $results_hashref->{'biblioserver'}->{'hits'}, 17, 0, 0, $results_hashref->{'biblioserver'}->{"RECORDS"}); my $allavailable = 'true'; foreach my $result (@newresults) { @@ -681,7 +681,7 @@ if ( $indexing_mode eq 'dom' ) { # Let's just test a few other bits and bobs, just for fun ($error, $results_hashref, $facets_loop) = getRecords("Godzina pąsowej róży","Godzina pąsowej róży",[ ], [ 'biblioserver' ],20,0,undef,\%branches,\%itemtypes,$query_type,0); - @newresults = searchResults('intranet', $query_desc, $results_hashref->{'biblioserver'}->{'hits'}, 17, 0, 0, + @newresults = searchResults({'interface'=>'intranet'}, $query_desc, $results_hashref->{'biblioserver'}->{'hits'}, 17, 0, 0, $results_hashref->{'biblioserver'}->{"RECORDS"}); is($newresults[0]->{'alternateholdings_count'}, 1, 'Alternate holdings filled in correctly'); @@ -700,7 +700,7 @@ if ( $indexing_mode eq 'dom' ) { }); ($error, $results_hashref, $facets_loop) = getRecords("TEST12121212","TEST12121212",[ ], [ 'biblioserver' ],20,0,undef,\%branches,\%itemtypes,$query_type,0); - @newresults = searchResults('intranet', $query_desc, $results_hashref->{'biblioserver'}->{'hits'}, 17, 0, 0, + @newresults = searchResults({'interface'=>'intranet'}, $query_desc, $results_hashref->{'biblioserver'}->{'hits'}, 17, 0, 0, $results_hashref->{'biblioserver'}->{"RECORDS"}); ok(!exists($newresults[0]->{norequests}), 'presence of a transit does not block hold request action (bug 10741)'); @@ -708,7 +708,7 @@ if ( $indexing_mode eq 'dom' ) { ( undef, $results_hashref, $facets_loop ) = getRecords('ti:punctuation', 'punctuation', [], [ 'biblioserver' ], '19', 0, undef, \%branches, \%itemtypes, 'ccl', undef); is($results_hashref->{biblioserver}->{hits}, 1, "search for ti:punctuation returned expected number of records"); - @newresults = searchResults('intranet', $query_desc, $results_hashref->{'biblioserver'}->{'hits'}, 20, 0, 0, + @newresults = searchResults({'interface' =>'intranet'} , $query_desc, $results_hashref->{'biblioserver'}->{'hits'}, 20, 0, 0, $results_hashref->{'biblioserver'}->{"RECORDS"}); is(scalar(@newresults), 0, 'a record that cannot be parsed by MARC::Record is simply skipped (bug 10684)'); @@ -843,7 +843,7 @@ if ( $indexing_mode eq 'dom' ) { ( undef, $results_hashref, $facets_loop ) = getRecords('ti:marc the large record', '', [], [ 'biblioserver' ], '20', 0, undef, \%branches, \%itemtypes, 'ccl', undef); is($results_hashref->{biblioserver}->{hits}, 1, "Can do a search that retrieves an over-large bib record (bug 11096)"); - @newresults = searchResults('opac', $query_desc, $results_hashref->{'biblioserver'}->{'hits'}, 10, 0, 0, + @newresults = searchResults({'interface' =>'opac'}, $query_desc, $results_hashref->{'biblioserver'}->{'hits'}, 10, 0, 0, $results_hashref->{'biblioserver'}->{"RECORDS"}); is($newresults[0]->{title}, 'Marc the Large Record', 'Able to render the title for over-large bib record (bug 11096)'); is($newresults[0]->{biblionumber}, '300', 'Over-large bib record has the correct biblionumber (bug 11096)'); -- 1.7.10.4