Summary: | update_totalissues.pl should always skip_holds_queue | ||
---|---|---|---|
Product: | Koha | Reporter: | Lucas Gass (lukeg) <lucas> |
Component: | Hold requests | Assignee: | Lucas Gass (lukeg) <lucas> |
Status: | RESOLVED FIXED | QA Contact: | Emily Lamancusa (emlam) <emily.lamancusa> |
Severity: | normal | ||
Priority: | P5 - low | CC: | emily.lamancusa, f.demians, fridolin.somers, gmcharlt |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | Trivial patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: | Version(s) released in: |
24.05.00,23.11.06,23.05.12
|
|
Circulation function: | |||
Attachments: |
Bug 36137: Make update_totalissues cron always skip the holds queue
Bug 36137: Make update_totalissues cron always skip the holds queue Bug 36137: Make update_totalissues cron always skip the holds queue Bug 36137: Make update_totalissues cron always skip the holds queue Bug 36137: Make update_totalissues cron always skip the holds queue |
Description
Lucas Gass (lukeg)
2024-02-20 15:40:14 UTC
Looking closer here I see a few more things: I don't see why an if/else is needed here. We can just pass the value $incremental to UpdateTotalIssues - my $ret; - if ( $incremental && $totalissues > 0 ) { - $ret = UpdateTotalIssues( $biblionumber, $totalissues ); - } - else { - $ret = UpdateTotalIssues( $biblionumber, 0, $totalissues ); - } Instead this should work just as good: + my $ret = UpdateTotalIssues( $biblionumber, $incremental, $totalissues, 1 ); Also the POD for skip_holds_queue is missing from C4/Biblio Created attachment 162301 [details] [review] Bug 36137: Make update_totalissues cron always skip the holds queue To test: 1. Run the update_totalissues cron, the holds queue is updated. 2. APPLY PATCH, restart services 3. Run update_totalissues cron again, this time the holds queue should always be skipped. NOTE: This patch also eliminates what looks to me to be an unessesary if/else case. This patch also adds POD for the skips_hold_queue variable in C4/Biblio Created attachment 163766 [details] [review] Bug 36137: Make update_totalissues cron always skip the holds queue To test: 1. Run the update_totalissues cron, the holds queue is updated. 2. APPLY PATCH, restart services 3. Run update_totalissues cron again, this time the holds queue should always be skipped. NOTE: This patch also eliminates what looks to me to be an unessesary if/else case. This patch also adds POD for the skips_hold_queue variable in C4/Biblio Signed-off-by: Frédéric Demians <f.demians@tamil.fr> (In reply to Lucas Gass from comment #1) > I don't see why an if/else is needed here. We can just pass the value > $incremental to UpdateTotalIssues > > - my $ret; > - if ( $incremental && $totalissues > 0 ) { > - $ret = UpdateTotalIssues( $biblionumber, $totalissues ); > - } > - else { > - $ret = UpdateTotalIssues( $biblionumber, 0, $totalissues ); > - } > > Instead this should work just as good: > > + my $ret = UpdateTotalIssues( $biblionumber, $incremental, > $totalissues, 1 ); The if/else is necessary. In the if branch, $totalissues is being passed as the $increase parameter (i.e. the amount to increment the existing value by). In the else branch, $totalissues is being passed as the $value parameter (i.e. the value to overwrite the existing value). Providing a defined $value parameter causes UpdateTotalIssues to ignore the $increment parameter, so if the --incremental flag is set, the third parameter needs to be undef. Otherwise UpdateTotalIssues will overwrite the existing value instead of incrementing it as desired. That being said...if the --incremental flag is set, but $totalissues is 0 for a particular biblio, the else clause will still trigger, and wipe out the totalissues value for that biblio! So the if/else isn't correct to begin with - those conditions need to be checked separately. Created attachment 164610 [details] [review] Bug 36137: Make update_totalissues cron always skip the holds queue To test: 1. Run the update_totalissues cron, the holds queue is updated. 2. APPLY PATCH, restart services 3. Run update_totalissues cron again, this time the holds queue should always be skipped. (In reply to Emily Lamancusa from comment #4) > (In reply to Lucas Gass from comment #1) > > I don't see why an if/else is needed here. We can just pass the value > > $incremental to UpdateTotalIssues > > > > - my $ret; > > - if ( $incremental && $totalissues > 0 ) { > > - $ret = UpdateTotalIssues( $biblionumber, $totalissues ); > > - } > > - else { > > - $ret = UpdateTotalIssues( $biblionumber, 0, $totalissues ); > > - } > > > > Instead this should work just as good: > > > > + my $ret = UpdateTotalIssues( $biblionumber, $incremental, > > $totalissues, 1 ); > > The if/else is necessary. In the if branch, $totalissues is being passed as > the $increase parameter (i.e. the amount to increment the existing value > by). In the else branch, $totalissues is being passed as the $value > parameter (i.e. the value to overwrite the existing value). > > Providing a defined $value parameter causes UpdateTotalIssues to ignore the > $increment parameter, so if the --incremental flag is set, the third > parameter needs to be undef. Otherwise UpdateTotalIssues will overwrite the > existing value instead of incrementing it as desired. > > That being said...if the --incremental flag is set, but $totalissues is 0 > for a particular biblio, the else clause will still trigger, and wipe out > the totalissues value for that biblio! So the if/else isn't correct to begin > with - those conditions need to be checked separately. Thanks Emily. Created attachment 165842 [details] [review] Bug 36137: Make update_totalissues cron always skip the holds queue To test: 1. Run the update_totalissues cron, the holds queue is updated. 2. APPLY PATCH, restart services 3. Run update_totalissues cron again, this time the holds queue should always be skipped. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Created attachment 166142 [details] [review] Bug 36137: Make update_totalissues cron always skip the holds queue To test: 1. Run the update_totalissues cron, the holds queue is updated. 2. APPLY PATCH, restart services 3. Run update_totalissues cron again, this time the holds queue should always be skipped. Signed-off-by: Matt Blenkinsop <matt.blenkinsop@ptfs-europe.com> Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov> Looks good now, thanks Lucas! Passing QA Pushed for 24.05! Well done everyone, thank you! PS : UpdateTotalIssues() needs a review. Maybe it could be turned into a Koha::RecordProcessor say Koha::Filter::MARC::UpdateTotalIssues Pushed to 23.11.x for 23.11.06 Backported to 23.05.x for upcoming 23.05.12 |