Summary: | Onboarding tool fails due to inserting maxissueqty into IssuingRule | ||
---|---|---|---|
Product: | Koha | Reporter: | Hayley Pelham <hayleypelham> |
Component: | Installation and upgrade (web-based installer) | Assignee: | Hayley Pelham <hayleypelham> |
Status: | CLOSED FIXED | QA Contact: | Testopia <testopia> |
Severity: | blocker | ||
Priority: | P2 | CC: | alexbuckley, gmcharlt, jonathan.druart, josef.moravec, katrin.fischer, martin.renvoize, nick, wizzyrea |
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: | ||
Bug Depends on: | 18925 | ||
Bug Blocks: | |||
Attachments: |
Bug 22489: Fixed issue with onboarding tool failing at step 5
Modified to pass QA tests Bug 22489: Update onboarding tool for bug 18925 Bug 22489: Update onboarding tool for bug 18925 Bug 22489: (follow-up) Correctly set success messages Bug 22489: Update onboarding tool for bug 18925 Bug 22489: (follow-up) Correctly set success messages Bug 22489: Update onboarding tool for bug 18925 |
Description
Hayley Pelham
2019-03-11 05:24:32 UTC
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. |