Bug 28754 - C4::Reserves::FixPriority creates many warns when holds have lowestPriority set
Summary: C4::Reserves::FixPriority creates many warns when holds have lowestPriority set
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Nick Clemens
QA Contact: Joonas Kylmälä
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2021-07-23 17:56 UTC by Nick Clemens
Modified: 2021-08-16 21:06 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Text to go in the release notes:
Version(s) released in:
21.11.00,21.05.03,20.11.09


Attachments
Bug 28754: Only adjust holds on specific biblio and don't go past end of array (2.39 KB, patch)
2021-07-23 18:02 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 28754: Only adjust holds on specific biblio and don't go past end of array (2.44 KB, patch)
2021-07-24 20:51 UTC, David Nind
Details | Diff | Splinter Review
Bug 28754: Only adjust holds on specific biblio and don't go past end of array (2.59 KB, patch)
2021-07-28 07:55 UTC, Joonas Kylmälä
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-07-23 17:56:09 UTC
To recreate:
1 - Add holds with lowest priorty to 2 records in the catalog
2 - Add a hold on a third record
3 - Note errors in log like:
[2021/07/23 17:47:17] [WARN] splice() offset past end of array at /kohadevbox/koha/C4/Reserves.pm line 1649
Comment 1 Nick Clemens 2021-07-23 18:02:32 UTC
Created attachment 123147 [details] [review]
Bug 28754: Only adjust holds on specific biblio and don't go past end of array

Our query for lowest priority holds only needs to adjust holds on the biblio we are looking at
so I add biblionumber

Additionally we can simply find the end of the array and use that rather than 99998
so I set new_rank to scalar @priority

Lastly, we don't need to fetch the lowest priority holds if we are ignoring lowest priority
so I move it into the conditional

To test:
 1 - Add holds with lowest priorty to 2 records in the catalog
 2 - Add a hold on a third record
 3 - Note errors in log like:
    [2021/07/23 17:47:17] [WARN] splice() offset past end of array at /kohadevbox/koha/C4/Reserves.pm line 1649
 4 - Apply patch and restart all the things
 5 - Add a new hold on third record - no warns
 6 - Make one of the holds on third record have lowestPriority (click rightmost arrow with line at bottom)
 7 - Note no warns
 8 - Adjust other holds on record and note no warns
Comment 2 David Nind 2021-07-24 20:51:06 UTC
Created attachment 123158 [details] [review]
Bug 28754: Only adjust holds on specific biblio and don't go past end of array

Our query for lowest priority holds only needs to adjust holds on the biblio we are looking at
so I add biblionumber

Additionally we can simply find the end of the array and use that rather than 99998
so I set new_rank to scalar @priority

Lastly, we don't need to fetch the lowest priority holds if we are ignoring lowest priority
so I move it into the conditional

To test:
 1 - Add holds with lowest priorty to 2 records in the catalog
 2 - Add a hold on a third record
 3 - Note errors in log like:
    [2021/07/23 17:47:17] [WARN] splice() offset past end of array at /kohadevbox/koha/C4/Reserves.pm line 1649
 4 - Apply patch and restart all the things
 5 - Add a new hold on third record - no warns
 6 - Make one of the holds on third record have lowestPriority (click rightmost arrow with line at bottom)
 7 - Note no warns
 8 - Adjust other holds on record and note no warns

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 Joonas Kylmälä 2021-07-28 07:55:30 UTC
Created attachment 123242 [details] [review]
Bug 28754: Only adjust holds on specific biblio and don't go past end of array

Our query for lowest priority holds only needs to adjust holds on the biblio we are looking at
so I add biblionumber

Additionally we can simply find the end of the array and use that rather than 99998
so I set new_rank to scalar @priority

Lastly, we don't need to fetch the lowest priority holds if we are ignoring lowest priority
so I move it into the conditional

To test:
 1 - Add holds with lowest priorty to 2 records in the catalog
 2 - Add a hold on a third record
 3 - Note errors in log like:
    [2021/07/23 17:47:17] [WARN] splice() offset past end of array at /kohadevbox/koha/C4/Reserves.pm line 1649
 4 - Apply patch and restart all the things
 5 - Add a new hold on third record - no warns
 6 - Make one of the holds on third record have lowestPriority (click rightmost arrow with line at bottom)
 7 - Note no warns
 8 - Adjust other holds on record and note no warns

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Joonas Kylmälä <joonas.kylmala@helsinki.fi>
Comment 4 Jonathan Druart 2021-08-06 10:15:03 UTC
Pushed to master for 21.11, thanks to everybody involved!
Comment 5 Kyle M Hall 2021-08-06 18:09:55 UTC
Pushed to 21.05.x for 21.05.03
Comment 6 Fridolin Somers 2021-08-11 20:02:56 UTC
Pushed to 20.11.x for 20.11.09
Comment 7 Victor Grousset/tuxayo 2021-08-16 21:06:19 UTC
Not backported to oldoldstable (20.05.x). Feel free to ask if it's needed.