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; };
We can't have a release with this broken. Thanks for filing the bug, I spotted this too. Cheers, Liz
Created attachment 86496 [details] [review] 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
Created attachment 86497 [details] [review] Modified to pass QA tests
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
Oh also you left in some warns - it would be good to tidy those up. :) Cheers, Liz
Comment on attachment 86496 [details] [review] Bug 22489: Fixed issue with onboarding tool failing at step 5 Review of attachment 86496 [details] [review]: ----------------------------------------------------------------- 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.
I'd take a look around line 225 of admin/smart-rules.pl to see how the new maxissue stuff works..
Created attachment 86547 [details] [review] 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>
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.
(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 :)
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.
Created attachment 86561 [details] [review] 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>
Created attachment 86562 [details] [review] 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>
(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
Created attachment 86577 [details] [review] 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>
Created attachment 86578 [details] [review] 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>
Created attachment 86632 [details] [review] 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
I should add that this regression would have been caught with bug 19185 ;)
Awesome work all! Pushed to master for 19.05
Bug with enhancement not backported to 18.11.x series.