Summary: | Replace IsItemOnHoldAndFound with $item->holds->filter_by_found->count | ||
---|---|---|---|
Product: | Koha | Reporter: | Jonathan Druart <jonathan.druart> |
Component: | Architecture, internals, and plumbing | Assignee: | 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
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! |