Bug 24254 - Add Koha::Items->filter_by_visible_in_opac
Summary: Add Koha::Items->filter_by_visible_in_opac
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Martin Renvoize
URL:
Keywords:
: 22157 (view as bug list)
Depends on:
Blocks: 24403 15448 27358 27580 29802
  Show dependency treegraph
 
Reported: 2019-12-17 13:24 UTC by Nick Clemens
Modified: 2022-12-12 21:24 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This patch introduces an efficient way of filtering Koha::Items result sets, to hide items that shouldn't be exposed on public interfaces. Filtering is governed by the following system preferences. A helper method is added to handle lost items: - hidelostitems: Koha::Items->filter_out_lost is added to handle this. Some patrons have exceptions so OpacHiddenItems is not enforced on them. That's why the new method [1] has an optional parameter that expects the logged in patron to be passed in the call. [1] Koha::Items->filter_by_visible_in_opac
Version(s) released in:
21.05.00,20.11.02,20.05.09,19.11.15


Attachments
Bug 24254: Add get_visible_items method (1.29 KB, patch)
2019-12-17 13:28 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 24254: Unit tests (4.08 KB, patch)
2020-12-03 18:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24254: Add Koha::Items->filter_by_visible_in_opac (2.43 KB, patch)
2020-12-03 18:22 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24254: Unit tests (4.15 KB, patch)
2020-12-04 18:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24254: Add Koha::Items->filter_by_visible_in_opac (2.50 KB, patch)
2020-12-04 18:23 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24254: Unit tests (4.20 KB, patch)
2020-12-09 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24254: Add Koha::Items->filter_by_visible_in_opac (2.55 KB, patch)
2020-12-09 14:08 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24354: Compare itemlost with Perl's false values (3.04 KB, patch)
2020-12-19 13:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24254: Read the OpacHiddenItems preference internally (4.57 KB, patch)
2020-12-19 13:27 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24254: Unit tests (4.20 KB, patch)
2020-12-21 11:15 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24254: Add Koha::Items->filter_by_visible_in_opac (2.55 KB, patch)
2020-12-21 11:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24254: Compare itemlost with 0 (3.12 KB, patch)
2020-12-21 11:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24254: Read the OpacHiddenItems preference internally (4.57 KB, patch)
2020-12-21 11:16 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24254: Implement separate filters and add patron param (9.55 KB, patch)
2020-12-21 18:57 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 24254: Unit tests (4.27 KB, patch)
2020-12-22 11:40 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24254: Unit tests (4.27 KB, patch)
2020-12-22 11:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24254: Add Koha::Items->filter_by_visible_in_opac (2.62 KB, patch)
2020-12-22 11:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24254: Compare itemlost with 0 (3.19 KB, patch)
2020-12-22 11:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24254: Read the OpacHiddenItems preference internally (4.64 KB, patch)
2020-12-22 11:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24254: Implement separate filters and add patron param (9.62 KB, patch)
2020-12-22 11:41 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 24254: Unit tests (4.32 KB, patch)
2020-12-23 11:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24254: Add Koha::Items->filter_by_visible_in_opac (2.67 KB, patch)
2020-12-23 11:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24254: Compare itemlost with 0 (3.23 KB, patch)
2020-12-23 11:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24254: Read the OpacHiddenItems preference internally (4.69 KB, patch)
2020-12-23 11:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24254: Implement separate filters and add patron param (9.67 KB, patch)
2020-12-23 11:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24254: Fix ISE (1.39 KB, patch)
2021-01-14 09:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24254: Remove items fetch (1.60 KB, patch)
2021-01-14 09:45 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 24254: Unit tests (4.32 KB, patch)
2021-01-14 11:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24254: Add Koha::Items->filter_by_visible_in_opac (2.67 KB, patch)
2021-01-14 11:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24254: Compare itemlost with 0 (3.23 KB, patch)
2021-01-14 11:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24254: Read the OpacHiddenItems preference internally (4.69 KB, patch)
2021-01-14 11:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24254: Implement separate filters and add patron param (9.67 KB, patch)
2021-01-14 11:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24254: (QA follow-up) Inlines opachiddenitems handling (7.58 KB, patch)
2021-01-14 11:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 24254: (QA follow-up) Inlines opachiddenitems handling (5.98 KB, patch)
2021-01-14 12:18 UTC, Martin Renvoize
Details | Diff | Splinter Review
[20.05.x] Bug 24254: Unit tests (4.33 KB, patch)
2021-01-29 18:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[20.05.x] Bug 24254: Add Koha::Items->filter_by_visible_in_opac (2.57 KB, patch)
2021-01-29 18:34 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[20.05.x] Bug 24254: Compare itemlost with 0 (3.24 KB, patch)
2021-01-29 18:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[20.05.x] Bug 24254: Read the OpacHiddenItems preference internally (4.71 KB, patch)
2021-01-29 18:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[20.05.x] Bug 24254: Implement separate filters and add patron param (9.69 KB, patch)
2021-01-29 18:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
[20.05.x] Bug 24254: (QA follow-up) Inlines opachiddenitems handling (5.73 KB, patch)
2021-01-29 18:35 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2019-12-17 13:24:27 UTC
It would be nice to be able to fetch items excluding opac hidden items in one DB call rather than having to loop through items and compare to the rules for each.
Comment 1 Nick Clemens 2019-12-17 13:28:36 UTC
Created attachment 96373 [details] [review]
Bug 24254: Add get_visible_items method
Comment 2 Katrin Fischer 2020-01-02 07:13:53 UTC
Hi Nick, should this be NSO?
Comment 3 Tomás Cohen Arazi 2020-03-29 15:05:54 UTC
It needs tests.
Comment 4 Tomás Cohen Arazi 2020-12-02 15:27:53 UTC
Hi, Nick. I like your approach but have two suggestions:

1. Allow to optionally get passed the rules. This way if the caller is (say) the opac-search.pl script, we could read the rules once and reuse them.

2. Add this method to Koha::Items, renamed as ->filter_by_visible_in_opac or similar in the way we've been adding lately.

The method would basically do the same:

sub filter_by_visible_in_opac {
    my ($self, $params) = @_;

    my $rules = (exists $params->{rules})
                ? $params->{rules}
                : get_yaml_pref_hash('OpacHiddenItems');

    my $search_params;
    foreach my $field (keys %$rules){
        $search_params->{$field}->{'not in'} = $rules->{$field};
    }
    return $self->search( $search_params );
}


Then when you use it, you can chain the calls:

    my $visible_items = $biblio->items->filter_by_visible_in_opac;
Comment 5 Tomás Cohen Arazi 2020-12-03 18:22:36 UTC
Created attachment 114159 [details] [review]
Bug 24254: Unit tests
Comment 6 Tomás Cohen Arazi 2020-12-03 18:22:41 UTC
Created attachment 114160 [details] [review]
Bug 24254: Add Koha::Items->filter_by_visible_in_opac

This patch adds a method based on the original idea from Nick, but on
Koha::Items.

The idea is to build a proper filter, based on the current rules for
hiding things, directly on the DBIC query. The caller takes care of
knowing if the filtering should apply (i.e. checking the patron category
exceptions) and then it would do something like:

    my @items;
    if ( <patron_category_does_not_have_exception> ) {
        @items = $biblio->items->filter_by_visible_in_opac(
            {
                rules => $rules
            }
        );
    }
    else {
        # still want to enforce 'hidelostitems'
        @items = $biblio->items->filter_by_visible_in_opac;
    }

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Look at the use cases on the tests, read the code
=> SUCCESS: It all makes sense
4. Compare with Koha::Item->hidden_in_opac
=> SUCCESS: It all makes sense
5. Sign off :-D
Comment 7 Kyle M Hall 2020-12-04 18:23:11 UTC
Created attachment 114206 [details] [review]
Bug 24254: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 8 Kyle M Hall 2020-12-04 18:23:21 UTC
Created attachment 114207 [details] [review]
Bug 24254: Add Koha::Items->filter_by_visible_in_opac

This patch adds a method based on the original idea from Nick, but on
Koha::Items.

The idea is to build a proper filter, based on the current rules for
hiding things, directly on the DBIC query. The caller takes care of
knowing if the filtering should apply (i.e. checking the patron category
exceptions) and then it would do something like:

    my @items;
    if ( <patron_category_does_not_have_exception> ) {
        @items = $biblio->items->filter_by_visible_in_opac(
            {
                rules => $rules
            }
        );
    }
    else {
        # still want to enforce 'hidelostitems'
        @items = $biblio->items->filter_by_visible_in_opac;
    }

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Look at the use cases on the tests, read the code
=> SUCCESS: It all makes sense
4. Compare with Koha::Item->hidden_in_opac
=> SUCCESS: It all makes sense
5. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 9 Martin Renvoize 2020-12-09 14:08:17 UTC
Created attachment 114278 [details] [review]
Bug 24254: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 10 Martin Renvoize 2020-12-09 14:08:20 UTC
Created attachment 114279 [details] [review]
Bug 24254: Add Koha::Items->filter_by_visible_in_opac

This patch adds a method based on the original idea from Nick, but on
Koha::Items.

The idea is to build a proper filter, based on the current rules for
hiding things, directly on the DBIC query. The caller takes care of
knowing if the filtering should apply (i.e. checking the patron category
exceptions) and then it would do something like:

    my @items;
    if ( <patron_category_does_not_have_exception> ) {
        @items = $biblio->items->filter_by_visible_in_opac(
            {
                rules => $rules
            }
        );
    }
    else {
        # still want to enforce 'hidelostitems'
        @items = $biblio->items->filter_by_visible_in_opac;
    }

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Look at the use cases on the tests, read the code
=> SUCCESS: It all makes sense
4. Compare with Koha::Item->hidden_in_opac
=> SUCCESS: It all makes sense
5. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 11 Martin Renvoize 2020-12-09 14:08:53 UTC
Works as expected, tests pass and qa scripts pass..

Passing QA
Comment 12 Jonathan Druart 2020-12-18 11:05:36 UTC
Comment on attachment 114279 [details] [review]
Bug 24254: Add Koha::Items->filter_by_visible_in_opac

Review of attachment 114279 [details] [review]:
-----------------------------------------------------------------

::: Koha/Items.pm
@@ +70,5 @@
> +        $search_params->{$field}->{'-not_in'} = $rules->{$field};
> +    }
> +
> +    $search_params->{itemlost}->{'<='} = 0
> +        if C4::Context->preference('hidelostitems');

Where is that <= 0 coming from?

From C4::Search:
1919                 # hidden because lost
1920                 if ($hidelostitems && $item->{itemlost}) {
1921                     $hideatopac_count++;
1922                     next;
1923                 }

Remember that -1 is evaluated true.
Comment 13 Jonathan Druart 2020-12-18 11:08:29 UTC
Why does the method take rules in parameter? Why don't we simply build the rules from the pref in the method?
Comment 14 Jonathan Druart 2020-12-18 11:08:51 UTC
(In reply to Jonathan Druart from comment #13)
> Why does the method take rules in parameter? Why don't we simply build the
> rules from the pref in the method?

ie. do we have other places where we have other rules?
Comment 15 Tomás Cohen Arazi 2020-12-18 11:20:37 UTC
(In reply to Jonathan Druart from comment #13)
> Why does the method take rules in parameter? Why don't we simply build the
> rules from the pref in the method?

If it was called in a loop, we could reuse the rules. That was the idea. Maybe we should read the rules locally if the parameter was not passed at all (i.e. !exists $params->{rules}).
Comment 16 Tomás Cohen Arazi 2020-12-18 11:23:28 UTC
(In reply to Jonathan Druart from comment #12)
> Comment on attachment 114279 [details] [review] [review]
> Bug 24254: Add Koha::Items->filter_by_visible_in_opac
> 
> Review of attachment 114279 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: Koha/Items.pm
> @@ +70,5 @@
> > +        $search_params->{$field}->{'-not_in'} = $rules->{$field};
> > +    }
> > +
> > +    $search_params->{itemlost}->{'<='} = 0
> > +        if C4::Context->preference('hidelostitems');
> 
> Where is that <= 0 coming from?
> 
> From C4::Search:
> 1919                 # hidden because lost
> 1920                 if ($hidelostitems && $item->{itemlost}) {
> 1921                     $hideatopac_count++;
> 1922                     next;
> 1923                 }
> 
> Remember that -1 is evaluated true.

Good catch! I overlooked the negative values here. All this was very undocumented, so I'd say we need a regression test for that.
Comment 17 Jonathan Druart 2020-12-18 12:19:37 UTC
(In reply to Tomás Cohen Arazi from comment #15)
> (In reply to Jonathan Druart from comment #13)
> > Why does the method take rules in parameter? Why don't we simply build the
> > rules from the pref in the method?
> 
> If it was called in a loop, we could reuse the rules. That was the idea.
> Maybe we should read the rules locally if the parameter was not passed at
> all (i.e. !exists $params->{rules}).

What for? Performance?
If we are looping on biblios then it's not that reading the pref and building the rules that will have an impact on perf. If you are concerned about that I would cache it at package level (->{_item_hide_rules}).

I think it's better to prevent calls that will forget to pass the rules, or having to update all the callers if we decide to add one rules.

What do you think?
Comment 18 Tomás Cohen Arazi 2020-12-18 12:26:44 UTC
(In reply to Jonathan Druart from comment #17)
> (In reply to Tomás Cohen Arazi from comment #15)
> > (In reply to Jonathan Druart from comment #13)
> > > Why does the method take rules in parameter? Why don't we simply build the
> > > rules from the pref in the method?
> > 
> > If it was called in a loop, we could reuse the rules. That was the idea.
> > Maybe we should read the rules locally if the parameter was not passed at
> > all (i.e. !exists $params->{rules}).
> 
> What for? Performance?
> If we are looping on biblios then it's not that reading the pref and
> building the rules that will have an impact on perf. If you are concerned
> about that I would cache it at package level (->{_item_hide_rules}).
> 
> I think it's better to prevent calls that will forget to pass the rules, or
> having to update all the callers if we decide to add one rules.
> 
> What do you think?

I usually prefer explicit vs. implicit. But not a strong position on this particular case. The 'if passed use it, if not, read it' approach seems to me like the best compromise option. This could be a follow-up bug (it requires new tests, probably adapt the callers)
Comment 19 Jonathan Druart 2020-12-18 13:02:16 UTC
(In reply to Tomás Cohen Arazi from comment #18)
> (In reply to Jonathan Druart from comment #17)
> > (In reply to Tomás Cohen Arazi from comment #15)
> > > (In reply to Jonathan Druart from comment #13)
> > > > Why does the method take rules in parameter? Why don't we simply build the
> > > > rules from the pref in the method?
> > > 
> > > If it was called in a loop, we could reuse the rules. That was the idea.
> > > Maybe we should read the rules locally if the parameter was not passed at
> > > all (i.e. !exists $params->{rules}).
> > 
> > What for? Performance?
> > If we are looping on biblios then it's not that reading the pref and
> > building the rules that will have an impact on perf. If you are concerned
> > about that I would cache it at package level (->{_item_hide_rules}).
> > 
> > I think it's better to prevent calls that will forget to pass the rules, or
> > having to update all the callers if we decide to add one rules.
> > 
> > What do you think?
> 
> I usually prefer explicit vs. implicit. But not a strong position on this
> particular case. The 'if passed use it, if not, read it' approach seems to
> me like the best compromise option. This could be a follow-up bug (it
> requires new tests, probably adapt the callers)

I don't think it's explicit vs implicit.
->filter_by_visible_in_opac is explicit already.
If you are passing a set of rules then it would be ->filter_by_rules
As I said I am also concerned about the need to update the callers if rules are added.
If we agree on that it should be done on this bug report, not a follow-up bug.
Comment 20 Tomás Cohen Arazi 2020-12-18 13:48:48 UTC
(In reply to Jonathan Druart from comment #19)
> (In reply to Tomás Cohen Arazi from comment #18)
> > (In reply to Jonathan Druart from comment #17)
> > > (In reply to Tomás Cohen Arazi from comment #15)
> > > > (In reply to Jonathan Druart from comment #13)
> > > > > Why does the method take rules in parameter? Why don't we simply build the
> > > > > rules from the pref in the method?
> > > > 
> > > > If it was called in a loop, we could reuse the rules. That was the idea.
> > > > Maybe we should read the rules locally if the parameter was not passed at
> > > > all (i.e. !exists $params->{rules}).
> > > 
> > > What for? Performance?
> > > If we are looping on biblios then it's not that reading the pref and
> > > building the rules that will have an impact on perf. If you are concerned
> > > about that I would cache it at package level (->{_item_hide_rules}).
> > > 
> > > I think it's better to prevent calls that will forget to pass the rules, or
> > > having to update all the callers if we decide to add one rules.
> > > 
> > > What do you think?
> > 
> > I usually prefer explicit vs. implicit. But not a strong position on this
> > particular case. The 'if passed use it, if not, read it' approach seems to
> > me like the best compromise option. This could be a follow-up bug (it
> > requires new tests, probably adapt the callers)
> 
> I don't think it's explicit vs implicit.
> ->filter_by_visible_in_opac is explicit already.
> If you are passing a set of rules then it would be ->filter_by_rules
> As I said I am also concerned about the need to update the callers if rules
> are added.
> If we agree on that it should be done on this bug report, not a follow-up
> bug.

I understand your point, and agree.
I've also reviewed how syspref caching works, and I belive there's no need for the optimization by design I was thinking about.
Comment 21 Tomás Cohen Arazi 2020-12-19 13:27:43 UTC
Created attachment 114539 [details] [review]
Bug 24354: Compare itemlost with Perl's false values

On C4::Search and C4::Circulation the uses of the items.itemlost field
highlight the fact that the comparisson itemlost <= 0 was wrong, as it
is evaluated as a Perl boolean.

As authorised values can take many values that can be casted to 'false',
we need to compare with them explicitly.

This patch changes the tests to reflect this, and adjust the
Koha::Items->filter_by_visible_in_opac implementation to adapt to this.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 22 Tomás Cohen Arazi 2020-12-19 13:27:48 UTC
Created attachment 114540 [details] [review]
Bug 24254: Read the OpacHiddenItems preference internally

After discussing the 'rules' parameter usefulness we decided it was not
the best idea, and the gains in terms of 'performance' would me
meaningless (in-memory caching of sysprefs). This patch makes a really
minor tweak to the tests so they mock the C4::Context->yaml_preference
method, but keeping the same original rules to highlight no behaviour
change takes place.

Then the rules parameter is removed from the calls, and the tests should
keep passing.

A minor change to make $rules = undef is made to highlight the // {}
behaviour when reading the syspref..

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 23 Tomás Cohen Arazi 2020-12-19 13:30:17 UTC
Hi y'all. I've adapted the implementation to:

1. Fix the itemlost value comparisson (bug).
2. Remove the rules parameter and read it inside the method (design issue).

Setting back to NSO so it gets your eyes on it again.
Comment 24 Tomás Cohen Arazi 2020-12-21 11:15:17 UTC
Created attachment 114548 [details] [review]
Bug 24254: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 25 Tomás Cohen Arazi 2020-12-21 11:16:32 UTC
Created attachment 114549 [details] [review]
Bug 24254: Add Koha::Items->filter_by_visible_in_opac

This patch adds a method based on the original idea from Nick, but on
Koha::Items.

The idea is to build a proper filter, based on the current rules for
hiding things, directly on the DBIC query. The caller takes care of
knowing if the filtering should apply (i.e. checking the patron category
exceptions) and then it would do something like:

    my @items;
    if ( <patron_category_does_not_have_exception> ) {
        @items = $biblio->items->filter_by_visible_in_opac(
            {
                rules => $rules
            }
        );
    }
    else {
        # still want to enforce 'hidelostitems'
        @items = $biblio->items->filter_by_visible_in_opac;
    }

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Look at the use cases on the tests, read the code
=> SUCCESS: It all makes sense
4. Compare with Koha::Item->hidden_in_opac
=> SUCCESS: It all makes sense
5. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Tomás Cohen Arazi 2020-12-21 11:16:35 UTC
Created attachment 114550 [details] [review]
Bug 24254: Compare itemlost with 0

On C4::Search and C4::Circulation the uses of the items.itemlost field
highlight the fact that the comparisson itemlost <= 0 was wrong, as it
is evaluated as a Perl boolean.

The column can only be an int and NOT NULL, so we need to check if it is
0 to ponder if not hidden.

This patch changes the tests to reflect this, and adjust the
Koha::Items->filter_by_visible_in_opac implementation to adapt to this.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 27 Tomás Cohen Arazi 2020-12-21 11:16:39 UTC
Created attachment 114551 [details] [review]
Bug 24254: Read the OpacHiddenItems preference internally

After discussing the 'rules' parameter usefulness we decided it was not
the best idea, and the gains in terms of 'performance' would me
meaningless (in-memory caching of sysprefs). This patch makes a really
minor tweak to the tests so they mock the C4::Context->yaml_preference
method, but keeping the same original rules to highlight no behaviour
change takes place.

Then the rules parameter is removed from the calls, and the tests should
keep passing.

A minor change to make $rules = undef is made to highlight the // {}
behaviour when reading the syspref..

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 28 Tomás Cohen Arazi 2020-12-21 18:57:41 UTC
Created attachment 114574 [details] [review]
Bug 24254: Implement separate filters and add patron param

This patch introduces two new methods for stacking filters on
Koha::Items:

- filter_out_lost
_ filter_out_opachiddenitems

This two filters are what actually happened inside the
filter_by_visible_in_opac. Everything is covered by tests.

In the process I added a Koha::Patron param to the method that is
internally used to decide if the OPACHiddenItems syspref needs to be
honoured or not. This *could* be better done with a fallback to
C4::Context->userenv if no param is passed.

I decided to leave that part for later, if we really find it would help
(e.g. if bug 10589 gets some action and we really need something here to
 handle that).

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Sign off :-D
Comment 29 Kyle M Hall 2020-12-22 11:40:50 UTC
Created attachment 114594 [details] [review]
Bug 24254: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 30 Kyle M Hall 2020-12-22 11:41:21 UTC
Created attachment 114595 [details] [review]
Bug 24254: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 31 Kyle M Hall 2020-12-22 11:41:32 UTC
Created attachment 114596 [details] [review]
Bug 24254: Add Koha::Items->filter_by_visible_in_opac

This patch adds a method based on the original idea from Nick, but on
Koha::Items.

The idea is to build a proper filter, based on the current rules for
hiding things, directly on the DBIC query. The caller takes care of
knowing if the filtering should apply (i.e. checking the patron category
exceptions) and then it would do something like:

    my @items;
    if ( <patron_category_does_not_have_exception> ) {
        @items = $biblio->items->filter_by_visible_in_opac(
            {
                rules => $rules
            }
        );
    }
    else {
        # still want to enforce 'hidelostitems'
        @items = $biblio->items->filter_by_visible_in_opac;
    }

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Look at the use cases on the tests, read the code
=> SUCCESS: It all makes sense
4. Compare with Koha::Item->hidden_in_opac
=> SUCCESS: It all makes sense
5. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 32 Kyle M Hall 2020-12-22 11:41:35 UTC
Created attachment 114597 [details] [review]
Bug 24254: Compare itemlost with 0

On C4::Search and C4::Circulation the uses of the items.itemlost field
highlight the fact that the comparisson itemlost <= 0 was wrong, as it
is evaluated as a Perl boolean.

The column can only be an int and NOT NULL, so we need to check if it is
0 to ponder if not hidden.

This patch changes the tests to reflect this, and adjust the
Koha::Items->filter_by_visible_in_opac implementation to adapt to this.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 33 Kyle M Hall 2020-12-22 11:41:38 UTC
Created attachment 114598 [details] [review]
Bug 24254: Read the OpacHiddenItems preference internally

After discussing the 'rules' parameter usefulness we decided it was not
the best idea, and the gains in terms of 'performance' would me
meaningless (in-memory caching of sysprefs). This patch makes a really
minor tweak to the tests so they mock the C4::Context->yaml_preference
method, but keeping the same original rules to highlight no behaviour
change takes place.

Then the rules parameter is removed from the calls, and the tests should
keep passing.

A minor change to make $rules = undef is made to highlight the // {}
behaviour when reading the syspref..

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 34 Kyle M Hall 2020-12-22 11:41:41 UTC
Created attachment 114599 [details] [review]
Bug 24254: Implement separate filters and add patron param

This patch introduces two new methods for stacking filters on
Koha::Items:

- filter_out_lost
_ filter_out_opachiddenitems

This two filters are what actually happened inside the
filter_by_visible_in_opac. Everything is covered by tests.

In the process I added a Koha::Patron param to the method that is
internally used to decide if the OPACHiddenItems syspref needs to be
honoured or not. This *could* be better done with a fallback to
C4::Context->userenv if no param is passed.

I decided to leave that part for later, if we really find it would help
(e.g. if bug 10589 gets some action and we really need something here to
 handle that).

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 35 Martin Renvoize 2020-12-23 11:25:41 UTC
Created attachment 114664 [details] [review]
Bug 24254: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 36 Martin Renvoize 2020-12-23 11:25:44 UTC
Created attachment 114665 [details] [review]
Bug 24254: Add Koha::Items->filter_by_visible_in_opac

This patch adds a method based on the original idea from Nick, but on
Koha::Items.

The idea is to build a proper filter, based on the current rules for
hiding things, directly on the DBIC query. The caller takes care of
knowing if the filtering should apply (i.e. checking the patron category
exceptions) and then it would do something like:

    my @items;
    if ( <patron_category_does_not_have_exception> ) {
        @items = $biblio->items->filter_by_visible_in_opac(
            {
                rules => $rules
            }
        );
    }
    else {
        # still want to enforce 'hidelostitems'
        @items = $biblio->items->filter_by_visible_in_opac;
    }

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Look at the use cases on the tests, read the code
=> SUCCESS: It all makes sense
4. Compare with Koha::Item->hidden_in_opac
=> SUCCESS: It all makes sense
5. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 37 Martin Renvoize 2020-12-23 11:25:48 UTC
Created attachment 114666 [details] [review]
Bug 24254: Compare itemlost with 0

On C4::Search and C4::Circulation the uses of the items.itemlost field
highlight the fact that the comparisson itemlost <= 0 was wrong, as it
is evaluated as a Perl boolean.

The column can only be an int and NOT NULL, so we need to check if it is
0 to ponder if not hidden.

This patch changes the tests to reflect this, and adjust the
Koha::Items->filter_by_visible_in_opac implementation to adapt to this.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 38 Martin Renvoize 2020-12-23 11:25:51 UTC
Created attachment 114667 [details] [review]
Bug 24254: Read the OpacHiddenItems preference internally

After discussing the 'rules' parameter usefulness we decided it was not
the best idea, and the gains in terms of 'performance' would me
meaningless (in-memory caching of sysprefs). This patch makes a really
minor tweak to the tests so they mock the C4::Context->yaml_preference
method, but keeping the same original rules to highlight no behaviour
change takes place.

Then the rules parameter is removed from the calls, and the tests should
keep passing.

A minor change to make $rules = undef is made to highlight the // {}
behaviour when reading the syspref..

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 39 Martin Renvoize 2020-12-23 11:25:55 UTC
Created attachment 114668 [details] [review]
Bug 24254: Implement separate filters and add patron param

This patch introduces two new methods for stacking filters on
Koha::Items:

- filter_out_lost
_ filter_out_opachiddenitems

This two filters are what actually happened inside the
filter_by_visible_in_opac. Everything is covered by tests.

In the process I added a Koha::Patron param to the method that is
internally used to decide if the OPACHiddenItems syspref needs to be
honoured or not. This *could* be better done with a fallback to
C4::Context->userenv if no param is passed.

I decided to leave that part for later, if we really find it would help
(e.g. if bug 10589 gets some action and we really need something here to
 handle that).

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 40 Martin Renvoize 2020-12-23 11:28:50 UTC
Right.. I think this is pretty solid now.. Passing QA.

However, it does make me ask the question as to why our Koha::Item->store routine and cataloguing/additem.pl both do a >=/ge check on itemlost.  I think you've discussed and proved to me that the check on itemlost is purely boolean.. false (0/undef) vs defined/not zero... but I can't help but have a nagging feeling the negatives used to be meaningful in some dim and distant past.
Comment 41 Jonathan Druart 2021-01-14 09:15:31 UTC
(In reply to Martin Renvoize from comment #40)
> Right.. I think this is pretty solid now.. Passing QA.
> 
> However, it does make me ask the question as to why our Koha::Item->store
> routine and cataloguing/additem.pl both do a >=/ge check on itemlost.  I
> think you've discussed and proved to me that the check on itemlost is purely
> boolean.. false (0/undef) vs defined/not zero... but I can't help but have a
> nagging feeling the negatives used to be meaningful in some dim and distant
> past.

Maybe you mixed up with notforloan? (negative values are for "ordered")
Comment 42 Katrin Fischer 2021-01-14 09:29:46 UTC
(In reply to Jonathan Druart from comment #41)
> (In reply to Martin Renvoize from comment #40)
> > Right.. I think this is pretty solid now.. Passing QA.
> > 
> > However, it does make me ask the question as to why our Koha::Item->store
> > routine and cataloguing/additem.pl both do a >=/ge check on itemlost.  I
> > think you've discussed and proved to me that the check on itemlost is purely
> > boolean.. false (0/undef) vs defined/not zero... but I can't help but have a
> > nagging feeling the negatives used to be meaningful in some dim and distant
> > past.
> 
> Maybe you mixed up with notforloan? (negative values are for "ordered")

I think at the moment only the negatives for notforloan and restricted have some visible effect. We could check how the hidelostitems system preference works now?
Comment 43 Jonathan Druart 2021-01-14 09:45:22 UTC
Created attachment 115131 [details] [review]
Bug 24254: Fix ISE

The method Koha::Items->itemnumber is not covered by tests!

Trace begun at /kohadevbox/koha/Koha/Objects.pm line 592
Koha::Objects::AUTOLOAD('Koha::Items=HASH(0x55981fd94790)') called at /kohadevbox/koha/opac/opac-reserve.pl line 465
Comment 44 Jonathan Druart 2021-01-14 09:45:26 UTC
Created attachment 115132 [details] [review]
Bug 24254: Remove items fetch

We fetch them already too many times.
Comment 45 Martin Renvoize 2021-01-14 11:47:19 UTC
Created attachment 115147 [details] [review]
Bug 24254: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 46 Martin Renvoize 2021-01-14 11:47:23 UTC
Created attachment 115148 [details] [review]
Bug 24254: Add Koha::Items->filter_by_visible_in_opac

This patch adds a method based on the original idea from Nick, but on
Koha::Items.

The idea is to build a proper filter, based on the current rules for
hiding things, directly on the DBIC query. The caller takes care of
knowing if the filtering should apply (i.e. checking the patron category
exceptions) and then it would do something like:

    my @items;
    if ( <patron_category_does_not_have_exception> ) {
        @items = $biblio->items->filter_by_visible_in_opac(
            {
                rules => $rules
            }
        );
    }
    else {
        # still want to enforce 'hidelostitems'
        @items = $biblio->items->filter_by_visible_in_opac;
    }

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Look at the use cases on the tests, read the code
=> SUCCESS: It all makes sense
4. Compare with Koha::Item->hidden_in_opac
=> SUCCESS: It all makes sense
5. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 47 Martin Renvoize 2021-01-14 11:47:27 UTC
Created attachment 115149 [details] [review]
Bug 24254: Compare itemlost with 0

On C4::Search and C4::Circulation the uses of the items.itemlost field
highlight the fact that the comparisson itemlost <= 0 was wrong, as it
is evaluated as a Perl boolean.

The column can only be an int and NOT NULL, so we need to check if it is
0 to ponder if not hidden.

This patch changes the tests to reflect this, and adjust the
Koha::Items->filter_by_visible_in_opac implementation to adapt to this.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 48 Martin Renvoize 2021-01-14 11:47:30 UTC
Created attachment 115150 [details] [review]
Bug 24254: Read the OpacHiddenItems preference internally

After discussing the 'rules' parameter usefulness we decided it was not
the best idea, and the gains in terms of 'performance' would me
meaningless (in-memory caching of sysprefs). This patch makes a really
minor tweak to the tests so they mock the C4::Context->yaml_preference
method, but keeping the same original rules to highlight no behaviour
change takes place.

Then the rules parameter is removed from the calls, and the tests should
keep passing.

A minor change to make $rules = undef is made to highlight the // {}
behaviour when reading the syspref..

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 49 Martin Renvoize 2021-01-14 11:47:34 UTC
Created attachment 115151 [details] [review]
Bug 24254: Implement separate filters and add patron param

This patch introduces two new methods for stacking filters on
Koha::Items:

- filter_out_lost
_ filter_out_opachiddenitems

This two filters are what actually happened inside the
filter_by_visible_in_opac. Everything is covered by tests.

In the process I added a Koha::Patron param to the method that is
internally used to decide if the OPACHiddenItems syspref needs to be
honoured or not. This *could* be better done with a fallback to
C4::Context->userenv if no param is passed.

I decided to leave that part for later, if we really find it would help
(e.g. if bug 10589 gets some action and we really need something here to
 handle that).

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 50 Martin Renvoize 2021-01-14 11:47:38 UTC
Created attachment 115152 [details] [review]
Bug 24254: (QA follow-up) Inlines opachiddenitems handling

We decided to inline the opachiddenitems filter as we don't believe it
will be used exclusively.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 51 Martin Renvoize 2021-01-14 12:18:02 UTC
Created attachment 115153 [details] [review]
Bug 24254: (QA follow-up) Inlines opachiddenitems handling

We decided to inline the opachiddenitems filter as we don't believe it
will be used exclusively.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 52 Jonathan Druart 2021-01-14 13:04:29 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 53 Fridolin Somers 2021-01-18 15:11:40 UTC
Enhancement not pushed to 20.11.x
Comment 54 Katrin Fischer 2021-01-18 22:26:52 UTC
(In reply to Fridolin Somers from comment #53)
> Enhancement not pushed to 20.11.x

I believe this is ground work for fixing bug 15448, which is a major bug. So maybe a grey zone in terms of being an enhancement.
Comment 55 Fridolin Somers 2021-01-19 07:57:52 UTC
Backported in order to backport Bug 15448

Pushed to 20.11.x for 20.11.02
Comment 56 Andrew Fuerste-Henry 2021-01-29 17:48:51 UTC
Doesn't apply cleanly to 20.05. Please rebase if needed.
Comment 57 Tomás Cohen Arazi 2021-01-29 18:34:30 UTC
Created attachment 116070 [details] [review]
[20.05.x] Bug 24254: Unit tests

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 58 Tomás Cohen Arazi 2021-01-29 18:34:45 UTC
Created attachment 116071 [details] [review]
[20.05.x] Bug 24254: Add Koha::Items->filter_by_visible_in_opac

This patch adds a method based on the original idea from Nick, but on
Koha::Items.

The idea is to build a proper filter, based on the current rules for
hiding things, directly on the DBIC query. The caller takes care of
knowing if the filtering should apply (i.e. checking the patron category
exceptions) and then it would do something like:

    my @items;
    if ( <patron_category_does_not_have_exception> ) {
        @items = $biblio->items->filter_by_visible_in_opac(
            {
                rules => $rules
            }
        );
    }
    else {
        # still want to enforce 'hidelostitems'
        @items = $biblio->items->filter_by_visible_in_opac;
    }

To test:
1. Apply this patches
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Look at the use cases on the tests, read the code
=> SUCCESS: It all makes sense
4. Compare with Koha::Item->hidden_in_opac
=> SUCCESS: It all makes sense
5. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 59 Tomás Cohen Arazi 2021-01-29 18:35:04 UTC
Created attachment 116072 [details] [review]
[20.05.x] Bug 24254: Compare itemlost with 0

On C4::Search and C4::Circulation the uses of the items.itemlost field
highlight the fact that the comparisson itemlost <= 0 was wrong, as it
is evaluated as a Perl boolean.

The column can only be an int and NOT NULL, so we need to check if it is
0 to ponder if not hidden.

This patch changes the tests to reflect this, and adjust the
Koha::Items->filter_by_visible_in_opac implementation to adapt to this.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 60 Tomás Cohen Arazi 2021-01-29 18:35:20 UTC
Created attachment 116073 [details] [review]
[20.05.x] Bug 24254: Read the OpacHiddenItems preference internally

After discussing the 'rules' parameter usefulness we decided it was not
the best idea, and the gains in terms of 'performance' would me
meaningless (in-memory caching of sysprefs). This patch makes a really
minor tweak to the tests so they mock the C4::Context->yaml_preference
method, but keeping the same original rules to highlight no behaviour
change takes place.

Then the rules parameter is removed from the calls, and the tests should
keep passing.

A minor change to make $rules = undef is made to highlight the // {}
behaviour when reading the syspref..

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 61 Tomás Cohen Arazi 2021-01-29 18:35:36 UTC
Created attachment 116074 [details] [review]
[20.05.x] Bug 24254: Implement separate filters and add patron param

This patch introduces two new methods for stacking filters on
Koha::Items:

- filter_out_lost
_ filter_out_opachiddenitems

This two filters are what actually happened inside the
filter_by_visible_in_opac. Everything is covered by tests.

In the process I added a Koha::Patron param to the method that is
internally used to decide if the OPACHiddenItems syspref needs to be
honoured or not. This *could* be better done with a fallback to
C4::Context->userenv if no param is passed.

I decided to leave that part for later, if we really find it would help
(e.g. if bug 10589 gets some action and we really need something here to
 handle that).

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Items.t
=> SUCCESS: Tests pass!
3. Sign off :-D

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 62 Tomás Cohen Arazi 2021-01-29 18:35:59 UTC
Created attachment 116075 [details] [review]
[20.05.x] Bug 24254: (QA follow-up) Inlines opachiddenitems handling

We decided to inline the opachiddenitems filter as we don't believe it
will be used exclusively.

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 63 Tomás Cohen Arazi 2021-01-29 18:37:04 UTC
(In reply to Andrew Fuerste-Henry from comment #56)
> Doesn't apply cleanly to 20.05. Please rebase if needed.

You asked for it, you've got it!
Comment 64 Andrew Fuerste-Henry 2021-01-29 18:46:46 UTC
Thanks! Pushed to 20.05.x for 20.05.09 in the interest of correcting bug 15448
Comment 65 Victor Grousset/tuxayo 2021-02-01 21:15:09 UTC
Backported: Pushed to 19.11.x branch for 19.11.15
in the interest of correcting bug 15448
Comment 66 Nick Clemens 2022-01-14 14:58:02 UTC
*** Bug 22157 has been marked as a duplicate of this bug. ***