[Bug 22489] New: Onboarding tool fails due to inserting maxissueqty into IssuingRule
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Bug ID: 22489 Summary: Onboarding tool fails due to inserting maxissueqty into IssuingRule Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: major Priority: P5 - low Component: Installation and upgrade (web-based installer) Assignee: koha-bugs@lists.koha-community.org Reporter: hayleymapley@catalyst.net.nz QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com Target Milestone: --- When going through the onboarding process, the tool fails on submit of Step 5 - creating an issuing rule. This is due to the maxissueqty column having been removed from the IssuingRules table and added to the CirculationRules table but redundant code still exists in onboarding.pl. As you can see below the maxissueqty variable is being added to the params hash which is used to make the new issuing rule. if ( $step == 5 ) { if ( $op eq 'add_validate_circ_rule' ) { #If no libraries exist then set the $branch value to * my $branch = $input->param('branch') || '*'; my $type = $input->param('type'); my $branchcode = $input->param('branch'); my $categorycode = $input->param('categorycode'); my $itemtype = $input->param('itemtype'); my $maxissueqty = $input->param('maxissueqty'); my $issuelength = $input->param('issuelength'); my $lengthunit = $input->param('lengthunit'); my $renewalsallowed = $input->param('renewalsallowed'); my $renewalperiod = $input->param('renewalperiod'); my $onshelfholds = $input->param('onshelfholds') || 0; $maxissueqty =~ s/\s//g; $maxissueqty = undef if $maxissueqty !~ /^\d+/; $issuelength = $issuelength eq q{} ? undef : $issuelength; my $params = { branchcode => $branchcode, categorycode => $categorycode, itemtype => $itemtype, maxissueqty => $maxissueqty, renewalsallowed => $renewalsallowed, renewalperiod => $renewalperiod, issuelength => $issuelength, lengthunit => $lengthunit, onshelfholds => $onshelfholds, }; my $issuingrule = Koha::IssuingRule->new($params); eval { $issuingrule->store; }; -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Hayley Mapley <hayleymapley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |ASSIGNED Assignee|koha-bugs@lists.koha-commun |hayleymapley@catalyst.net.n |ity.org |z -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Depends on| |18925 CC| |katrin.fischer@bsz-bw.de Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18925 [Bug 18925] Move maxissueqty and maxonsiteissueqty to circulation_rules -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Josef Moravec <josef.moravec@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |josef.moravec@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Alex Buckley <alexbuckley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |alexbuckley@catalyst.net.nz -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Liz Rea <wizzyrea@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Priority|P5 - low |P2 CC| |wizzyrea@gmail.com Severity|major |blocker --- Comment #1 from Liz Rea <wizzyrea@gmail.com> --- We can't have a release with this broken. Thanks for filing the bug, I spotted this too. Cheers, Liz -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 --- Comment #2 from Hayley Mapley <hayleymapley@catalyst.net.nz> --- Created attachment 86496 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86496&action=edit Bug 22489: Fixed issue with onboarding tool failing at step 5 When going through the onboarding process, the tool fails on submit of step 5, due to changes made in the database structure and rule definitions. This patch fixes this, adding code that creates a circulation rule where an issuing rule is also created in onboarding.pl. It removes maxissueqty from being sent with params to make an issuing rule, and adds it instead to the params to make a circulation rule, to reflect the changes that have been made in the database. Test plan: 1) Drop and recreate your database 2) Reload the staff client and follow the installer and onboarding process 3) Confirm that the process finishes without errors -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 --- Comment #3 from Hayley Mapley <hayleymapley@catalyst.net.nz> --- Created attachment 86497 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86497&action=edit Modified to pass QA tests -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Hayley Mapley <hayleymapley@catalyst.net.nz> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|ASSIGNED |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Bug 22489 depends on bug 18925, which changed state. Bug 18925 Summary: Move maxissueqty and maxonsiteissueqty to circulation_rules https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18925 What |Removed |Added ---------------------------------------------------------------------------- Status|Pushed to Master |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Liz Rea <wizzyrea@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Failed QA --- Comment #4 from Liz Rea <wizzyrea@gmail.com> --- Hi Hayley, Thank you for the quick turnaround, however there is a QA fail: installer/onboarding.pl: Found use of Switch. This module is deprecated by the Perl 5 Porters at line 35, column 1. Find an alternative module. (Severity: 5) I suspect you will want to find a different solution for this. Thanks for keeping after it! Cheers, Liz -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 --- Comment #5 from Liz Rea <wizzyrea@gmail.com> --- Oh also you left in some warns - it would be good to tidy those up. :) Cheers, Liz -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 --- Comment #6 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Comment on attachment 86496 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86496 Bug 22489: Fixed issue with onboarding tool failing at step 5 Review of attachment 86496: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=22489&attachment=86496) ----------------------------------------------------------------- Sorry.. I think there's a bit more work to do here I'm afraid. ::: admin/smart-rules.pl @@ +223,5 @@
}
+ warn $bor; + warn $itemtype; + warn $br;
Cleaning up the warnings again would be great ;) @@ +302,5 @@
$sth_insert->execute($branch, $holdallowed, $hold_fulfillment_policy, $returnbranch); }
+ warn $branch; + warn $categorycode;
More warnings ;) ::: installer/onboarding.pl @@ +31,5 @@
use Koha::Patron::Categories; use Koha::ItemTypes; use Koha::IssuingRules; +use Koha::CirculationRules; +use Switch;
Switch is recommended against... and doesn't appear to be used anywhere either. @@ +258,5 @@
issuelength => $issuelength, lengthunit => $lengthunit, onshelfholds => $onshelfholds, }; + $branchcode = $branchcode if $branchcode ne '*';
Erm.. this doesn't make sense "Set branchcode to branchcode if branchcode not equal to '*'".. What were you intending here? @@ +260,5 @@
onshelfholds => $onshelfholds, }; + $branchcode = $branchcode if $branchcode ne '*'; + $branchcode = undef if $branchcode eq '*'; + $categorycode = $categorycode if $categorycode ne '*';
As above.. I'm confused as to what the intention is here? @@ +262,5 @@
+ $branchcode = $branchcode if $branchcode ne '*'; + $branchcode = undef if $branchcode eq '*'; + $categorycode = $categorycode if $categorycode ne '*'; + $categorycode = undef if $categorycode eq '*'; + $itemtype = $itemtype if $itemtype ne '*';
and again.. @@ +278,4 @@
eval { $issuingrule->store; };
+ my $circulationrule = Koha::CirculationRule->new($circulationruleparams); + eval { $circulationrule->store; };
New eval, but we're no longer catching errors for the first insert.. we'd need to catch errors above this line too.. @@ +287,4 @@
push @messages, { code => 'error_on_insert_circ_rule' }; } } + warn Koha::CirculationRules->count;
Another warn ;) ::: koha-tmpl/intranet-tmpl/prog/en/modules/onboarding/onboardingstep5.tt @@ +98,5 @@
+ <label for="maxissueqty" class="required">Current checkouts allowed: </label> + <input type="number" min="0" name="maxissueqty" id="maxissueqty" size="10" value="50" class="required" required="required" /> + <span class="required">Required</span> + </li> + <li>
This looks to me as though it's just been moved.. not sure why yet as I've not actually inspected the new UI layout.. perhaps it makes good sense to move it.. perhaps not. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com --- Comment #7 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I'd take a look around line 225 of admin/smart-rules.pl to see how the new maxissue stuff works.. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 --- Comment #8 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 86547 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86547&action=edit Bug 22489: Update onboarding tool for bug 18925 When going through the onboarding process, the tool fails on submit of step 5, due to changes made in the database structure and rule definitions by bug 18925. This patch fixes this, adding code that creates a circulation rule where an issuing rule is also created in onboarding.pl. It removes maxissueqty from being sent with params to make an issuing rule, and adds it instead to the params to make a circulation rule, to reflect the changes that have been made in the database. Test plan: 1) Drop and recreate your database 2) Reload the staff client and follow the installer and onboarding process 3) Confirm that the process finishes without errors Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #86496|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #86497|0 |1 is obsolete| | -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Failed QA |Needs Signoff --- Comment #9 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- I felt mean asking Hayley to change so much.. so I've done a cleaned up patch myself.. I'd really appreciate your testing though Heyley, thanks for reporting it and having a first crack, you've saved me considerable time digging to work out the cause and fix. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 --- Comment #10 from Hayley Mapley <hayleymapley@catalyst.net.nz> --- (In reply to Martin Renvoize from comment #9)
I felt mean asking Hayley to change so much.. so I've done a cleaned up patch myself.. I'd really appreciate your testing though Heyley, thanks for reporting it and having a first crack, you've saved me considerable time digging to work out the cause and fix.
Sorry for all the mistakes there Martin and Katrin, I was working on this with someone else and I forgot that they had added all those warns. I appreciate having someone more experienced than me to make a follow up, so thank you for that. I will test your patch now :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 --- Comment #11 from Hayley Mapley <hayleymapley@catalyst.net.nz> --- All looks good Martin, the only issue (and it's a small one) is that the alert on the final page of the onboarding 'Circulation rule created' appears twice which may be confusing to users. I haven't found the cause of this yet, but it doesn't appear to be an issue with the database as there are no duplicates of the rules I created during onboarding. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #86547|0 |1 is obsolete| | --- Comment #12 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 86561 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86561&action=edit Bug 22489: Update onboarding tool for bug 18925 When going through the onboarding process, the tool fails on submit of step 5, due to changes made in the database structure and rule definitions by bug 18925. This patch fixes this, adding code that creates a circulation rule where an issuing rule is also created in onboarding.pl. It removes maxissueqty from being sent with params to make an issuing rule, and adds it instead to the params to make a circulation rule, to reflect the changes that have been made in the database. Test plan: 1) Drop and recreate your database 2) Reload the staff client and follow the installer and onboarding process 3) Confirm that the process finishes without errors Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 --- Comment #13 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Created attachment 86562 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86562&action=edit Bug 22489: (follow-up) Correctly set success messages The original patch was duplicating the success message upon successful circ rule insertion. This corrects that. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 --- Comment #14 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- (In reply to Hayley Mapley from comment #11)
All looks good Martin, the only issue (and it's a small one) is that the alert on the final page of the onboarding 'Circulation rule created' appears twice which may be confusing to users. I haven't found the cause of this yet, but it doesn't appear to be an issue with the database as there are no duplicates of the rules I created during onboarding.
Thanks for testing Hayley :). I hope I didn't come across too scary with my comments, it's great to have you onboard. I've now added a followup which should clean up the double messages. Many thanks, Martin -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Patch complexity|--- |Trivial patch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Liz Rea <wizzyrea@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Liz Rea <wizzyrea@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #86561|0 |1 is obsolete| | Attachment #86562|0 |1 is obsolete| | --- Comment #15 from Liz Rea <wizzyrea@gmail.com> --- Created attachment 86577 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86577&action=edit Bug 22489: Update onboarding tool for bug 18925 When going through the onboarding process, the tool fails on submit of step 5, due to changes made in the database structure and rule definitions by bug 18925. This patch fixes this, adding code that creates a circulation rule where an issuing rule is also created in onboarding.pl. It removes maxissueqty from being sent with params to make an issuing rule, and adds it instead to the params to make a circulation rule, to reflect the changes that have been made in the database. Test plan: 1) Drop and recreate your database 2) Reload the staff client and follow the installer and onboarding process 3) Confirm that the process finishes without errors Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 --- Comment #16 from Liz Rea <wizzyrea@gmail.com> --- Created attachment 86578 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86578&action=edit Bug 22489: (follow-up) Correctly set success messages The original patch was duplicating the success message upon successful circ rule insertion. This corrects that. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #86577|0 |1 is obsolete| | Attachment #86578|0 |1 is obsolete| | --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 86632 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=86632&action=edit Bug 22489: Update onboarding tool for bug 18925 When going through the onboarding process, the tool fails on submit of step 5, due to changes made in the database structure and rule definitions by bug 18925. This patch fixes this, adding code that creates a circulation rule where an issuing rule is also created in onboarding.pl. It removes maxissueqty from being sent with params to make an issuing rule, and adds it instead to the params to make a circulation rule, to reflect the changes that have been made in the database. Test plan: 1) Drop and recreate your database 2) Reload the staff client and follow the installer and onboarding process 3) Confirm that the process finishes without errors Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Bug 22489: (follow-up) Correctly set success messages The original patch was duplicating the success message upon successful circ rule insertion. This corrects that. Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Liz Rea <wizzyrea@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Patches squashed for readability -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #18 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- I should add that this regression would have been caught with bug 19185 ;) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |nick@bywatersolutions.com Status|Passed QA |Pushed to Master --- Comment #19 from Nick Clemens <nick@bywatersolutions.com> --- Awesome work all! Pushed to master for 19.05 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Resolution|--- |FIXED Status|Pushed to Master |RESOLVED --- Comment #20 from Martin Renvoize <martin.renvoize@ptfs-europe.com> --- Bug with enhancement not backported to 18.11.x series. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Bug 22489 depends on bug 18925, which changed state. Bug 18925 Summary: Move maxissueqty and maxonsiteissueqty to circulation_rules https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18925 What |Removed |Added ---------------------------------------------------------------------------- Status|RESOLVED |UNCONFIRMED Resolution|FIXED |--- -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22489 Bug 22489 depends on bug 18925, which changed state. Bug 18925 Summary: Move maxissueqty and maxonsiteissueqty to circulation_rules https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=18925 What |Removed |Added ---------------------------------------------------------------------------- Status|UNCONFIRMED |RESOLVED Resolution|--- |FIXED -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org