Bug 25695 - Missing logging of $@ in onboarding.pl after eval block
Summary: Missing logging of $@ in onboarding.pl after eval block
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Installation and upgrade (web-based installer) (show other bugs)
Version: master
Hardware: All All
: P5 - low trivial (vote)
Assignee: Slava Shishkin
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 25790
  Show dependency treegraph
 
Reported: 2020-06-08 15:18 UTC by Slava Shishkin
Modified: 2020-08-03 07:40 UTC (History)
7 users (show)

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


Attachments
Bug 25695: add a missing warning in onboarding.pl (1005 bytes, patch)
2020-06-08 15:26 UTC, Slava Shishkin
Details | Diff | Splinter Review
Bug 25695: merge of similar code blocks after circulation_rules update (2.22 KB, patch)
2020-06-18 09:11 UTC, Slava Shishkin
Details | Diff | Splinter Review
Bug 25695: merge of similar code blocks after circulation_rules update (2.24 KB, patch)
2020-06-18 10:21 UTC, Slava Shishkin
Details | Diff | Splinter Review
Bug 25695: merge of similar code blocks after circulation_rules update (2.31 KB, patch)
2020-06-29 10:18 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 Andrew 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 Andrew 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.