Bug 33444 - AddRenewal should take a hash of parameters
Summary: AddRenewal should take a hash of parameters
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens (kidclamp)
QA Contact: Marcel de Rooy
URL:
Keywords: release-notes-needed
Depends on: 32013
Blocks:
  Show dependency treegraph
 
Reported: 2023-04-07 13:43 UTC by Nick Clemens (kidclamp)
Modified: 2023-12-28 20:47 UTC (History)
2 users (show)

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


Attachments
Bug 33444: Update AddRenewal to take a hashref of params (27.22 KB, patch)
2023-04-07 15:00 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 33444: Update AddRenewal to take a hashref of params (27.35 KB, patch)
2023-05-16 14:06 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 33444: Update AddRenewal to take a hashref of params (27.39 KB, patch)
2023-07-18 12:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33444: (QA follow-up) Tidy block in automatic_renewals.pl (1.85 KB, patch)
2023-07-18 12:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33444: (QA follow-up) Add forgotten skipfinecalc in POD (893 bytes, patch)
2023-07-18 12:20 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33444: Update AddRenewal to take a hashref of params (27.48 KB, patch)
2023-07-18 12:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33444: (QA follow-up) Tidy block in automatic_renewals.pl (1.77 KB, patch)
2023-07-18 12:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33444: (QA follow-up) Add forgotten skipfinecalc in POD (893 bytes, patch)
2023-07-18 12:32 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens (kidclamp) 2023-04-07 13:43:24 UTC
The routine (after bug 32013) now takes 9 parameters - this is unsustainable as a list
Comment 1 Nick Clemens (kidclamp) 2023-04-07 15:00:30 UTC
Created attachment 149280 [details] [review]
Bug 33444: Update AddRenewal to take a hashref of params
Comment 2 Jonathan Druart 2023-05-16 14:06:17 UTC
Created attachment 151283 [details] [review]
Bug 33444: Update AddRenewal to take a hashref of params

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 3 Marcel de Rooy 2023-05-17 13:58:00 UTC
Feels like an enh ?
Comment 4 Marcel de Rooy 2023-07-18 11:23:59 UTC
Did you see that you solved a bug here (Koha/REST/V1/Checkouts.pm):

         AddRenewal(
-            $checkout->borrowernumber,
-            $checkout->itemnumber,
-            $checkout->branchcode,
-            undef,
-            undef,
-            $seen
+            {
+                borrowernumber => $checkout->borrowernumber,
+                itemnumber     => $checkout->itemnumber,
+                branch         => $checkout->branchcode,
+                seen           => $seen

Seen was in the sixth position before. And should have been 7th.
Comment 5 Marcel de Rooy 2023-07-18 11:39:35 UTC
misc/cronjobs/automatic_renewals.pl

-            my $date_due = AddRenewal( $auto_renew->borrowernumber, $auto_renew->itemnumber, $auto_renew->branchcode, undef, undef, undef, 0, 1 );
+            my $date_due = AddRenewal({
+                borrowernumber => $auto_renew->borrowernumber,
+                itemnumber => $auto_renew->itemnumber,
+                branch => $auto_renew->branchcode,
+                seen => 0,
+                automatic => 1,
+                skip_record_index => 1

This is not the same. skip_record_index was not there before
Please clarify
Comment 6 Marcel de Rooy 2023-07-18 12:20:44 UTC
Created attachment 153593 [details] [review]
Bug 33444: Update AddRenewal to take a hashref of params

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 7 Marcel de Rooy 2023-07-18 12:20:46 UTC
Created attachment 153594 [details] [review]
Bug 33444: (QA follow-up) Tidy block in automatic_renewals.pl

Resolves:
The file is less tidy than before (bad/messy lines before: 94, now: 101)

Test plan:
Run qa tools.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 8 Marcel de Rooy 2023-07-18 12:20:48 UTC
Created attachment 153595 [details] [review]
Bug 33444: (QA follow-up) Add forgotten skipfinecalc in POD

No test plan needed.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 9 Marcel de Rooy 2023-07-18 12:32:44 UTC
Created attachment 153597 [details] [review]
Bug 33444: Update AddRenewal to take a hashref of params

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Removed skip_record_index => 1 from automatic_renewals.pl. See BZ.
Comment 10 Marcel de Rooy 2023-07-18 12:32:47 UTC
Created attachment 153598 [details] [review]
Bug 33444: (QA follow-up) Tidy block in automatic_renewals.pl

Resolves:
The file is less tidy than before (bad/messy lines before: 94, now: 101)

Test plan:
Run qa tools.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 11 Marcel de Rooy 2023-07-18 12:32:49 UTC
Created attachment 153599 [details] [review]
Bug 33444: (QA follow-up) Add forgotten skipfinecalc in POD

No test plan needed.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 12 Marcel de Rooy 2023-07-18 12:33:16 UTC
(In reply to Marcel de Rooy from comment #5)
> misc/cronjobs/automatic_renewals.pl
> 
> -            my $date_due = AddRenewal( $auto_renew->borrowernumber,
> $auto_renew->itemnumber, $auto_renew->branchcode, undef, undef, undef, 0, 1
> );
> +            my $date_due = AddRenewal({
> +                borrowernumber => $auto_renew->borrowernumber,
> +                itemnumber => $auto_renew->itemnumber,
> +                branch => $auto_renew->branchcode,
> +                seen => 0,
> +                automatic => 1,
> +                skip_record_index => 1
> 
> This is not the same. skip_record_index was not there before
> Please clarify

Adjusted in patch 1 in consultation with Nick on IRC.
Comment 13 Tomás Cohen Arazi 2023-07-19 15:08:37 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 14 Fridolin Somers 2023-08-04 01:40:08 UTC
Not backporting in 23.05.x

Method signature change should have a nice release note