https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #1 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 96373 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=96373&action=edit Bug 24254: Add get_visible_items method -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |24403 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24403 [Bug 24403] [OMNIBUS] OpacHiddenItems should hide items everywhere in the OPAC -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #3 from Tomás Cohen Arazi <tomascohen@gmail.com> --- It needs tests. -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #4 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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; -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |tomascohen@gmail.com |ity.org | Summary|Add Koha::Biblio |Add |get_visible_items method |Koha::Items->filter_by_visi | |ble_in_opac -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #96373|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #6 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 114160 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114160&action=edit 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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #8 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 114207 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114207&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114207|0 |1 is obsolete| | --- Comment #10 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 114279 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114279&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- QA Contact|testopia@bugs.koha-communit |martin.renvoize@ptfs-europe |y.org |.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the| |This patch introduces an release notes| |efficient way of filtering | |Koha::Items result sets, to | |hide items that | |OpacHiddenItems determines | |shouldn't be exposed on | |public interfaces. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #13 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Why does the method take rules in parameter? Why don't we simply build the rules from the pref in the method? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #15 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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}). -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |In Discussion Keywords|release-notes-needed | --- Comment #19 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #21 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 114539 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114539&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff --- Comment #23 from Tomás Cohen Arazi <tomascohen@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114540|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114279|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #25 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 114549 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114549&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #27 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 114551 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114551&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the|This patch introduces an |This patch introduces an release notes|efficient way of filtering |efficient way of filtering |Koha::Items result sets, to |Koha::Items result sets, to |hide items that |hide items that that |OpacHiddenItems determines |shouldn't be exposed on |shouldn't be exposed on |public interfaces. |public interfaces. | | |Filtering is governed by | |the following system | |preferences. Two helper | |methods are added to handle | |each of them: | |- | |OpacHiddenItems: | |Koha::Items->filter_out_opa | |chiddenitems is added to | |handle this. | |- | |hidelostitems: | |Koha::Items->filter_out_los | |t 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_visi | |ble_in_opac CC| |kyle@bywatersolutions.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #31 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 114596 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114596&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #33 from Kyle M Hall <kyle@bywatersolutions.com> --- Created attachment 114598 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114598&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114595|0 |1 is obsolete| | --- Comment #35 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 114664 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114664&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114597|0 |1 is obsolete| | --- Comment #37 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 114666 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114666&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114599|0 |1 is obsolete| | --- Comment #39 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 114668 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=114668&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |27358 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27358 [Bug 27358] Add GET /public/biblios/:biblio_id/items -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #42 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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? -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #44 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 115132 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=115132&action=edit Bug 24254: Remove items fetch We fetch them already too many times. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #115132|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114665|0 |1 is obsolete| | --- Comment #46 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 115148 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=115148&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #114667|0 |1 is obsolete| | --- Comment #48 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 115150 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=115150&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #50 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 115152 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=115152&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Text to go in the|This patch introduces an |This patch introduces an release notes|efficient way of filtering |efficient way of filtering |Koha::Items result sets, to |Koha::Items result sets, to |hide items that that |hide items that that |shouldn't be exposed on |shouldn't be exposed on |public interfaces. |public interfaces. | | |Filtering is governed by |Filtering is governed by |the following system |the following system |preferences. Two helper |preferences. A helper |methods are added to handle |method is added to handle |each of them: |lost items: |- |- |OpacHiddenItems: |hidelostitems: |Koha::Items->filter_out_opa |Koha::Items->filter_out_los |chiddenitems is added to |t is added to handle this. |handle this. | |- |Some patrons have |hidelostitems: |exceptions so |Koha::Items->filter_out_los |OpacHiddenItems is not |t is added to handle this. |enforced on them. That's | |why the new method [1] has |Some patrons have |an optional parameter that |exceptions so |expects the logged in |OpacHiddenItems is not |patron to be passed in the |enforced on them. That's |call. |why the new method [1] has | |an optional parameter that |[1] |expects the logged in |Koha::Items->filter_by_visi |patron to be passed in the |ble_in_opac |call. | | | |[1] | |Koha::Items->filter_by_visi | |ble_in_opac | -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #52 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Pushed to master for 21.05, thanks to everybody involved! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #54 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- (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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|REOPENED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to master |Pushed to stable Version(s)|21.05.00 |21.05.00,20.11.02 released in| | --- Comment #55 from Fridolin Somers <fridolin.somers@biblibre.com> --- Backported in order to backport Bug 15448 Pushed to 20.11.x for 20.11.02 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #57 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 116070 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=116070&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #59 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 116072 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=116072&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #61 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 116074 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=116074&action=edit [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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 --- Comment #63 from Tomás Cohen Arazi <tomascohen@gmail.com> --- (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! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |27580 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=27580 [Bug 27580] NULL values not correctly handled in Koha::Items->filter_by_visible_in_opac -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |caroline.cyr-la-rose@inlibr | |o.com -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Pushed to oldoldstable |RESOLVED -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=24254 Jonathan Druart <jonathan.druart+koha@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |29802 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29802 [Bug 29802] biblionumber in OPACHiddenItems breaks opac lists -- You are receiving this mail because: You are watching all bug changes.