Bug 20846: Filter suppressed records from public lists in OPAC

When OpacSuppression is enabled, records marked as suppressed
(biblio.opac_suppressed = 1) still appear in public lists viewed
through the OPAC. This is confusing for patrons.

This patch adds a filter_by_not_suppressed method to
Koha::Virtualshelfcontents and uses it in opac-shelves.pl to
exclude suppressed records from list display when OpacSuppression
is enabled. OpacSuppressionByIPRange is respected: users within
the allowed IP range still see all records.

Test plan:
1. Apply patch
2. Enable OpacSuppression system preference
3. Create a public list and add some titles to it
4. Suppress one of the titles (set 942$n = 1)
5. View the list in the OPAC
=> SUCCESS: Suppressed record is no longer shown in the list
6. Set OpacSuppressionByIPRange to match your IP (e.g. 127.)
7. Repeat step 5
=> SUCCESS: All records (including suppressed) are shown
8. Run:
   $ ktd --shell
   k$ prove t/db_dependent/Virtualshelves.t
=> SUCCESS: Tests pass!
9. Sign off :-D

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Laura_Escamilla <laura.escamilla@bywatersolutions.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>

Koha/Virtualshelfcontents.pm

@@ -31,9 +31,23 @@ Koha::Virtualshelfcontents - Koha Virtualshelfcontents Object class
 
 =head2 Class Methods
 
+=head3 filter_by_not_suppressed
+
+    my $contents = $shelf->get_contents->filter_by_not_suppressed;
+
+Filters out shelf contents whose linked biblio has opac_suppressed set to true.
+
 =cut
 
-=head3 type
+sub filter_by_not_suppressed {
+    my ($self) = @_;
+
+    return $self->search( { 'biblionumber.opac_suppressed' => 0 }, { join => 'biblionumber' } );
+}
+
+=head2 Internal methods
+
+=head3 _type
 
 =cut
 

koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-shelves.tt

@@ -46,7 +46,7 @@
             data-shelfnumber="[% shelf.shelfnumber | html %]"
             data-shelfname="[% shelf.shelfname | html %]"
             data-shared="[% shelf.is_shared | html %]"
-            data-count="[% contents.count | html %]"
+            data-count="[% contents_count | html %]"
         >
             <i class="fa fa-times" aria-hidden="true"></i>
             [% IF ( context == "list" ) %]
@@ -258,8 +258,7 @@
                         </h1>
 
                         [% IF ( itemsloop ) %]
-                            [% SET contents = shelf.get_contents %]
+                            [% IF ( contents_count ) %]<p>This list contains [% contents_count | html %] titles</p>[% END %]
-                            [% IF ( contents.count ) %]<p>This list contains [% contents.count | html %] titles</p>[% END %]
                             <div id="floating" class="sticky">
                                 <div id="toolbar" class="toolbar clearfix">
                                     <div class="list-actions">
@@ -738,11 +737,18 @@
                                             </tr>
                                         </thead>
                                         <tbody>
-                                            [% FOREACH s IN shelves %]
+                                            [% FOREACH entry IN shelves_list %]
-                                                [% SET contents = s.get_contents %]
+                                                [% SET s = entry.shelf %]
                                                 <tr>
                                                     <td><a href="/cgi-bin/koha/opac-shelves.pl?op=view&amp;shelfnumber=[% s.shelfnumber | uri %]">[% s.shelfname | html %]</a></td>
-                                                    <td>[% IF contents.count %][% contents.count | html %] [% IF contents.count == 1 %]<span>item</span>[% ELSE %]<span>items</span>[% END %][% ELSE %]<span>Empty</span>[% END %]</td>
+                                                    <td
+                                                        >[% IF entry.contents_count %]
+                                                            [% entry.contents_count | html %]
+                                                            [% IF entry.contents_count == 1 %]<span>item</span>[% ELSE %]<span>items</span>[% END %]
+                                                        [% ELSE %]
+                                                            <span>Empty</span>
+                                                        [% END %]</td
+                                                    >
                                                     <td>
                                                         [% IF s.is_private %]
                                                             [% IF s.is_shared %]<span>Shared</span>[% ELSE %]<span>Private</span>[% END %]
@@ -762,7 +768,7 @@
                                                             </form>
                                                         [% END %]
                                                         [% IF s.can_be_deleted( loggedinusernumber ) %]
-                                                            [% PROCESS delete_shelf shelf = s context = "list" %]
+                                                            [% PROCESS delete_shelf shelf = s context = "list" contents_count = entry.contents_count %]
                                                         [% END %]
                                                         [% IF s.is_private AND s.can_be_managed( loggedinusernumber ) AND Koha.Preference('OpacAllowSharingPrivateLists') %]
                                                             <a href="/cgi-bin/koha/opac-shareshelf.pl?op=show&shelfnumber=[% s.shelfnumber | uri %]" class="sharelist btn btn-link"><i class="fa fa-share" aria-hidden="true"></i> Share</a>

opac/opac-shelves.pl

@@ -331,8 +331,20 @@ if ( $op eq 'view' ) {
             } else {
                 $rows = C4::Context->preference('OPACnumSearchResults') || 20;
             }
-            my $order_by = $sortfield eq 'itemcallnumber' ? 'items.cn_sort' : $sortfield;
+            my $order_by       = $sortfield eq 'itemcallnumber' ? 'items.cn_sort' : $sortfield;
-            my $contents = $shelf->get_contents->search(
+            my $shelf_contents = $shelf->get_contents;
+
+            if ( C4::Context->preference('OpacSuppression') ) {
+                my $ip_in_range = 0;
+                if ( my $ip_range = C4::Context->preference('OpacSuppressionByIPRange') ) {
+                    my $ip_address = $ENV{'REMOTE_ADDR'};
+                    $ip_in_range = ( $ip_address =~ /^$ip_range/ );
+                }
+                $shelf_contents = $shelf_contents->filter_by_not_suppressed
+                    unless $ip_in_range;
+            }
+
+            my $contents = $shelf_contents->search(
                 {},
                 {
                     distinct => 'biblionumber',
@@ -463,6 +475,7 @@ if ( $op eq 'view' ) {
                 can_delete_shelf   => $shelf->can_be_deleted($loggedinuser),
                 can_remove_biblios => $shelf->can_biblios_be_removed($loggedinuser),
                 can_add_biblios    => $shelf->can_biblios_be_added($loggedinuser),
+                contents_count     => $shelf_contents->count,
                 itemsloop          => \@items_info,
                 sortfield          => $sortfield,
                 direction          => $direction,
@@ -528,8 +541,27 @@ if ( $op eq 'view' ) {
     }
 
     my $pager = $shelves->pager;
+
+    # Pre-compute contents count per shelf, respecting suppression
+    my $apply_suppression = C4::Context->preference('OpacSuppression');
+    my $ip_in_range       = 0;
+    if ( $apply_suppression && ( my $ip_range = C4::Context->preference('OpacSuppressionByIPRange') ) ) {
+        my $ip_address = $ENV{'REMOTE_ADDR'};
+        $ip_in_range = ( $ip_address =~ /^$ip_range/ );
+    }
+
+    my @shelves_list;
+    while ( my $s = $shelves->next ) {
+        my $shelf_contents = $s->get_contents;
+        $shelf_contents = $shelf_contents->filter_by_not_suppressed
+            if $apply_suppression && !$ip_in_range;
+        push @shelves_list, { shelf => $s, contents_count => $shelf_contents->count };
+    }
+    $shelves->reset;
+
     $template->param(
         shelves        => $shelves,
+        shelves_list   => \@shelves_list,
         pagination_bar => pagination_bar(
             q||,   $pager->last_page - $pager->first_page + 1,
             $page, "page",

t/db_dependent/Virtualshelves.t

@@ -17,7 +17,7 @@
 
 use Modern::Perl;
 use Test::NoWarnings;
-use Test::More tests => 11;
+use Test::More tests => 12;
 use Test::Exception;
 
 use DateTime::Duration;
@@ -1659,6 +1659,44 @@ subtest 'filter_by_readable() tests' => sub {
     $schema->storage->txn_rollback;
 };
 
+subtest 'filter_by_not_suppressed() tests' => sub {
+
+    plan tests => 3;
+
+    $schema->storage->txn_begin;
+
+    my $patron = $builder->build_object( { class => 'Koha::Patrons' } );
+
+    my $biblio_visible    = $builder->build_sample_biblio;
+    my $biblio_suppressed = $builder->build_sample_biblio;
+    $biblio_suppressed->opac_suppressed(1)->store;
+
+    my $shelf = Koha::Virtualshelf->new(
+        {
+            shelfname => 'test_suppression_shelf',
+            owner     => $patron->borrowernumber,
+            public    => 1,
+        }
+    )->store;
+
+    $shelf->add_biblio( $biblio_visible->biblionumber,    $patron->borrowernumber );
+    $shelf->add_biblio( $biblio_suppressed->biblionumber, $patron->borrowernumber );
+
+    my $contents = $shelf->get_contents;
+    is( $contents->count, 2, 'Shelf has 2 entries' );
+
+    my $filtered = $shelf->get_contents->filter_by_not_suppressed;
+    is( $filtered->count, 1, 'filter_by_not_suppressed returns only non-suppressed records' );
+
+    is(
+        $filtered->next->biblionumber,
+        $biblio_visible->biblionumber,
+        'The returned content is the visible biblio'
+    );
+
+    $schema->storage->txn_rollback;
+};
+
 sub teardown {
     $dbh->do(q|DELETE FROM virtualshelfshares|);
     $dbh->do(q|DELETE FROM virtualshelfcontents|);