Bug 17729

Summary: Replace IsItemOnHoldAndFound with $item->holds->filter_by_found->count
Product: Koha Reporter: Jonathan Druart <jonathan.druart>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: RESOLVED FIXED QA Contact: Nick Clemens (kidclamp) <nick>
Severity: enhancement    
Priority: P5 - low CC: lucas, nick, paul.derscheid, tomascohen
Version: unspecified   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00
Circulation function:
Bug Depends on:    
Bug Blocks: 17728    
Attachments: Bug 17729: Move IsItemOnHoldAndFound to Koha::Holds
Bug 17729: Replace IsItemOnHoldAndFound
Bug 17729: Replace IsItemOnHoldAndFound
Bug 17729: Replace IsItemOnHoldAndFound
Bug 17729: Replace IsItemOnHoldAndFound

Description Jonathan Druart 2016-12-06 11:13:15 UTC

    
Comment 1 Jonathan Druart 2016-12-06 11:15:21 UTC
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
Comment 2 Jonathan Druart 2024-09-05 10:53:35 UTC
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
Comment 3 Paul Derscheid 2024-09-05 11:17:00 UTC
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.
Comment 4 Jonathan Druart 2024-09-05 11:27:36 UTC
(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?
Comment 5 Paul Derscheid 2024-09-05 11:34:48 UTC
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>
Comment 6 Jonathan Druart 2024-09-05 11:36:28 UTC
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.
Comment 7 Nick Clemens (kidclamp) 2024-09-05 12:01:51 UTC
(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
Comment 8 Nick Clemens (kidclamp) 2024-09-05 12:26:47 UTC
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>
Comment 9 Tomás Cohen Arazi (tcohen) 2024-09-05 12:55:27 UTC
(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
Comment 10 Tomás Cohen Arazi (tcohen) 2024-09-05 12:59:24 UTC
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>
Comment 11 Jonathan Druart 2024-09-05 13:24:26 UTC
(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
Comment 12 Jonathan Druart 2024-09-05 13:25:12 UTC
Almost 8 years to get back to it and got SO+QA in 2 hours, thanks <3
Comment 13 Tomás Cohen Arazi (tcohen) 2024-09-05 13:26:48 UTC
(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'.
Comment 14 Katrin Fischer 2024-09-16 08:53:51 UTC
Pushed for 24.11!

Well done everyone, thank you!