Bug 22489 - Onboarding tool fails due to inserting maxissueqty into IssuingRule
Summary: Onboarding tool fails due to inserting maxissueqty into IssuingRule
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (web-based installer) (show other bugs)
Version: master
Hardware: All All
: P2 blocker (vote)
Assignee: Hayley Mapley
QA Contact: Testopia
URL:
Keywords:
Depends on: 18925
Blocks:
  Show dependency treegraph
 
Reported: 2019-03-11 05:24 UTC by Hayley Mapley
Modified: 2019-03-19 09:26 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 22489: Fixed issue with onboarding tool failing at step 5 (6.85 KB, patch)
2019-03-12 03:09 UTC, Hayley Mapley
Details | Diff | Splinter Review
Modified to pass QA tests (1.00 KB, patch)
2019-03-12 03:10 UTC, Hayley Mapley
Details | Diff | Splinter Review
Bug 22489: Update onboarding tool for bug 18925 (2.34 KB, patch)
2019-03-12 17:33 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22489: Update onboarding tool for bug 18925 (2.34 KB, patch)
2019-03-13 09:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22489: (follow-up) Correctly set success messages (2.31 KB, patch)
2019-03-13 09:39 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 22489: Update onboarding tool for bug 18925 (2.37 KB, patch)
2019-03-13 16:04 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22489: (follow-up) Correctly set success messages (2.34 KB, patch)
2019-03-13 16:04 UTC, Liz Rea
Details | Diff | Splinter Review
Bug 22489: Update onboarding tool for bug 18925 (3.03 KB, patch)
2019-03-14 21:15 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Hayley Mapley 2019-03-11 05:24:32 UTC
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; };
Comment 1 Liz Rea 2019-03-11 19:47:35 UTC
We can't have a release with this broken. Thanks for filing the bug, I spotted this too.

Cheers,
Liz
Comment 2 Hayley Mapley 2019-03-12 03:09:55 UTC
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
Comment 3 Hayley Mapley 2019-03-12 03:10:28 UTC
Created attachment 86497 [details] [review]
Modified to pass QA tests
Comment 4 Liz Rea 2019-03-12 15:45:05 UTC
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
Comment 5 Liz Rea 2019-03-12 15:54:46 UTC
Oh also you left in some warns - it would be good to tidy those up. :)

Cheers,
Liz
Comment 6 Martin Renvoize 2019-03-12 17:07:31 UTC
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.
Comment 7 Martin Renvoize 2019-03-12 17:24:11 UTC
I'd take a look around line 225 of admin/smart-rules.pl to see how the new maxissue stuff works..
Comment 8 Martin Renvoize 2019-03-12 17:33:56 UTC
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>
Comment 9 Martin Renvoize 2019-03-12 17:35:44 UTC
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.
Comment 10 Hayley Mapley 2019-03-12 19:43:37 UTC
(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 :)
Comment 11 Hayley Mapley 2019-03-12 20:29:41 UTC
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.
Comment 12 Martin Renvoize 2019-03-13 09:39:24 UTC
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>
Comment 13 Martin Renvoize 2019-03-13 09:39:28 UTC
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>
Comment 14 Martin Renvoize 2019-03-13 09:41:53 UTC
(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
Comment 15 Liz Rea 2019-03-13 16:04:44 UTC
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>
Comment 16 Liz Rea 2019-03-13 16:04:51 UTC
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>
Comment 17 Jonathan Druart 2019-03-14 21:15:11 UTC
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
Comment 18 Jonathan Druart 2019-03-14 21:58:10 UTC
I should add that this regression would have been caught with bug 19185 ;)
Comment 19 Nick Clemens 2019-03-15 12:49:56 UTC
Awesome work all!

Pushed to master for 19.05
Comment 20 Martin Renvoize 2019-03-19 09:26:40 UTC
Bug with enhancement not backported to 18.11.x series.