Bug 25695

Summary: Missing logging of $@ in onboarding.pl after eval block
Product: Koha Reporter: Slava Shishkin <slavashishkin>
Component: Installation and upgrade (web-based installer)Assignee: Slava Shishkin <slavashishkin>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: trivial    
Priority: P5 - low CC: aleisha, gmcharlt, jonathan.druart, lucas, martin.renvoize, nugged, stalkernoid
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
20.11.00, 20.05.03
Bug Depends on:    
Bug Blocks: 25790    
Attachments: Bug 25695: add a missing warning in onboarding.pl
Bug 25695: merge of similar code blocks after circulation_rules update
Bug 25695: merge of similar code blocks after circulation_rules update
Bug 25695: merge of similar code blocks after circulation_rules update

Description Slava Shishkin 2020-06-08 15:18:30 UTC
There is a forgotten warning in the second eval block just after the same eval block which has warning.
This makes the error reason not logged. The solution is to add exactly the same ‘warn $@’ in consecutive eval 
block.
Comment 1 Slava Shishkin 2020-06-08 15:26:08 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2020-06-17 12:31:01 UTC
Why are not we actually setting maxissueqty in the same set_rules call?
Comment 3 Jonathan Druart 2020-06-17 12:33:29 UTC
It's coming from
  commit 3c4232e746cef574de9d1a346be7704dc3cd2e33
  Bug 22489: Update onboarding tool for bug 18925

But now we have all our rules moved to circulation_rules, I think maxissueqty can be moved into the same call as the other rules.
Comment 4 Andrii Nugged 2020-06-17 12:37:48 UTC
Yes, that was the second option but more "distant from the original". I agree, we will. Slava: let's do it.
Comment 5 Slava Shishkin 2020-06-18 09:11:57 UTC Comment hidden (obsolete)
Comment 6 Andrii Nugged 2020-06-18 09:58:14 UTC
have we saved a "success" message? Seems it's lost :).
This one:

    push @messages, { code => 'success_on_insert_circ_rule' };

let's update :).
Comment 7 Slava Shishkin 2020-06-18 10:21:21 UTC
Created attachment 105999 [details] [review]
Bug 25695: merge of similar code blocks after circulation_rules update

In onboarding.pl we have second block with adding just one more
rule parameter (maxissueqty) which can be merged to first one
after all parameters now stored in circulation_rules table.

Also this eliminates the need of adding forgotten "warn $@" after
the second eval block.

Mentored-by: Andrew Nugged <nugged@gmail.com>
Comment 8 Martin Renvoize 2020-06-29 10:18:10 UTC
Created attachment 106387 [details] [review]
Bug 25695: merge of similar code blocks after circulation_rules update

In onboarding.pl we have second block with adding just one more
rule parameter (maxissueqty) which can be merged to first one
after all parameters now stored in circulation_rules table.

Also this eliminates the need of adding forgotten "warn $@" after
the second eval block.

Mentored-by: Andrew Nugged <nugged@gmail.com>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize 2020-06-29 10:18:39 UTC
A nice bit of tidying up.. works as expected, thanks!

Going straight for a QA here.. PQA
Comment 10 Jonathan Druart 2020-07-09 10:05:52 UTC
Pushed to master for 20.11, thanks to everybody involved!
Comment 11 Lucas Gass 2020-07-24 20:16:54 UTC
backported to 20.05.x for 20.05.03
Comment 12 Aleisha Amohia 2020-08-03 07:40:32 UTC
Does not apply cleanly to 19.11.x. Please rebase if needed.