|
Description
Nick Clemens (kidclamp)
2019-12-17 13:24:27 UTC
Created attachment 96373 [details] [review] Bug 24254: Add get_visible_items method Hi Nick, should this be NSO? It needs tests. 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;
Created attachment 114159 [details] [review] Bug 24254: Unit tests 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 Created attachment 114206 [details] [review] Bug 24254: Unit tests Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> 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> 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> 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> Works as expected, tests pass and qa scripts pass.. Passing QA 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. Why does the method take rules in parameter? Why don't we simply build the rules from the pref in the method? (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? (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}). (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. (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? (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) (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. (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. 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> 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> 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. 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> 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> 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> 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> 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 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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> 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. (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") (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? 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 Created attachment 115132 [details] [review] Bug 24254: Remove items fetch We fetch them already too many times. 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> 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> 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> 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> 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> 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> 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> Pushed to master for 21.05, thanks to everybody involved! Enhancement not pushed to 20.11.x (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. Backported in order to backport Bug 15448 Pushed to 20.11.x for 20.11.02 Doesn't apply cleanly to 20.05. Please rebase if needed. 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> 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> 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> 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> 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> 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> (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! Thanks! Pushed to 20.05.x for 20.05.09 in the interest of correcting bug 15448 Backported: Pushed to 19.11.x branch for 19.11.15 in the interest of correcting bug 15448 *** Bug 22157 has been marked as a duplicate of this bug. *** |