Created attachment 57998 [details] [review] Bug 17729: Move IsItemOnHoldAndFound to Koha::Holds I don't think this is ready yet, since we are in loops, that can introduce speed improvement regressions
Created attachment 171053 [details] [review] Bug 17729: Replace IsItemOnHoldAndFound This subroutine can easily be replaced with $item->holds->filter_by_found->count \o/ Test plan: Confirm that the old sub and $item->holds->filter_by_found->count produce the same query
It's not 100% identical, the question is whether that matters? The new version only tests for values considered 'found': 'P', 'T' and 'W'. The old version tests for everything that's not undef.
(In reply to Paul Derscheid from comment #3) > It's not 100% identical, the question is whether that matters? > > The new version only tests for values considered 'found': 'P', 'T' and 'W'. > > The old version tests for everything that's not undef. There is 'F' (for Finished). I have no idea, Nick maybe?
Created attachment 171061 [details] [review] Bug 17729: Replace IsItemOnHoldAndFound This subroutine can easily be replaced with $item->holds->filter_by_found->count \o/ Test plan: Confirm that the old sub and $item->holds->filter_by_found->count produce the same query Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de>
It seems that "F" is set right before the hold is moved to "old" Koha/Hold.pm 821 sub fill { 827 $self->set( 828 { 829 found => 'F', 830 priority => 0, 831 $params->{item_id} ? ( itemnumber => $params->{item_id} ) : (), 832 } 833 ); 834 835 my $old_me = $self->_move_to_old; and $item->holds only return current holds.
(In reply to Paul Derscheid from comment #3) > It's not 100% identical, the question is whether that matters? > > The new version only tests for values considered 'found': 'P', 'T' and 'W'. > > The old version tests for everything that's not undef Spot checking a few production DB's I only see T,W,P, and NULL in that column We don't define any other statuses, so I think not including an unexpected value as found is okay - we could alter that subroutine if we think we need to check other values
Created attachment 171065 [details] [review] Bug 17729: Replace IsItemOnHoldAndFound This subroutine can easily be replaced with $item->holds->filter_by_found->count \o/ Test plan: Confirm that the old sub and $item->holds->filter_by_found->count produce the same query Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
(In reply to Nick Clemens (kidclamp) from comment #7) > (In reply to Paul Derscheid from comment #3) > > It's not 100% identical, the question is whether that matters? > > > > The new version only tests for values considered 'found': 'P', 'T' and 'W'. > > > > The old version tests for everything that's not undef > > Spot checking a few production DB's I only see T,W,P, and NULL in that column The 'F' status stands for 'filled', and can only be present in 'oldreserves'. Look at Koha/Hold.pm:848
Created attachment 171069 [details] [review] Bug 17729: Replace IsItemOnHoldAndFound This subroutine can easily be replaced with $item->holds->filter_by_found->count \o/ Test plan: Confirm that the old sub and $item->holds->filter_by_found->count produce the same query Signed-off-by: Paul Derscheid <paul.derscheid@lmscloud.de> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
(In reply to Tomás Cohen Arazi from comment #9) > (In reply to Nick Clemens (kidclamp) from comment #7) > > (In reply to Paul Derscheid from comment #3) > > > It's not 100% identical, the question is whether that matters? > > > > > > The new version only tests for values considered 'found': 'P', 'T' and 'W'. > > > > > > The old version tests for everything that's not undef > > > > Spot checking a few production DB's I only see T,W,P, and NULL in that column > > The 'F' status stands for 'filled', and can only be present in 'oldreserves'. Or Finished, depending on where you look at: C4/Reserves.pm 72 F(inished) : the reserve has been completed, and is done 789 return 'Finished' if $found eq 'F'; Yes, means the same but... :D
Almost 8 years to get back to it and got SO+QA in 2 hours, thanks <3
(In reply to Jonathan Druart from comment #12) > Almost 8 years to get back to it and got SO+QA in 2 hours, thanks <3 I would call it 'love'.
Pushed for 24.11! Well done everyone, thank you!