Bug 36137 - update_totalissues.pl should always skip_holds_queue
Summary: update_totalissues.pl should always skip_holds_queue
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Lucas Gass
QA Contact: Emily Lamancusa
URL:
Keywords: release-notes-needed
Depends on:
Blocks:
 
Reported: 2024-02-20 15:40 UTC by Lucas Gass
Modified: 2024-05-03 16:02 UTC (History)
3 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.05.00


Attachments
Bug 36137: Make update_totalissues cron always skip the holds queue (2.01 KB, patch)
2024-02-20 16:25 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 36137: Make update_totalissues cron always skip the holds queue (2.16 KB, patch)
2024-03-25 08:47 UTC, Frédéric Demians
Details | Diff | Splinter Review
Bug 36137: Make update_totalissues cron always skip the holds queue (1.80 KB, patch)
2024-04-10 11:57 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 36137: Make update_totalissues cron always skip the holds queue (1.88 KB, patch)
2024-04-30 11:11 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 36137: Make update_totalissues cron always skip the holds queue (1.94 KB, patch)
2024-05-03 14:39 UTC, Emily Lamancusa
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Lucas Gass 2024-02-20 15:40:14 UTC
update_totalissues.pl calls UpdateTotalIssues but does not pass a parameter in to skip the building of the holds queue. update_totalissues.pl should never update holdability.
Comment 1 Lucas Gass 2024-02-20 16:16:09 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
Comment 2 Lucas Gass 2024-02-20 16:25:12 UTC
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
Comment 3 Frédéric Demians 2024-03-25 08:47:01 UTC
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>
Comment 4 Emily Lamancusa 2024-04-08 20:58:44 UTC
(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.
Comment 5 Lucas Gass 2024-04-10 11:57:18 UTC
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.
Comment 6 Lucas Gass 2024-04-10 11:57:40 UTC
(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.
Comment 7 Matt Blenkinsop 2024-04-30 11:11:19 UTC
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>
Comment 8 Emily Lamancusa 2024-05-03 14:39:29 UTC
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>
Comment 9 Emily Lamancusa 2024-05-03 14:40:32 UTC
Looks good now, thanks Lucas! Passing QA
Comment 10 Katrin Fischer 2024-05-03 16:02:49 UTC
Pushed for 24.05!

Well done everyone, thank you!