Bug 20305 - Warnings in tools scripts need to be removed
Summary: Warnings in tools scripts need to be removed
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low normal (vote)
Assignee: Jonathan Druart
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on: 20017
Blocks:
  Show dependency treegraph
 
Reported: 2018-02-27 16:36 UTC by Jonathan Druart
Modified: 2018-10-24 09:13 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 20305: Remove warnings from tools scripts (895 bytes, patch)
2018-02-27 16:38 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20305: Remove warnings from tools scripts (942 bytes, patch)
2018-03-30 20:44 UTC, Roch D'Amour
Details | Diff | Splinter Review
Bug 20305: Remove warnings from tools scripts (895 bytes, patch)
2018-04-09 13:51 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 20305: Remove warnings from tools scripts (955 bytes, patch)
2018-04-09 15:02 UTC, Julian Maurice
Details | Diff | Splinter Review
Bug 20305: Remove warnings from tools scripts (1010 bytes, patch)
2018-05-04 19:39 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Jonathan Druart 2018-02-27 16:36:00 UTC
Since bug 20017 we have now warnings in tools scripts.
Comment 1 Jonathan Druart 2018-02-27 16:38:29 UTC
Created attachment 72273 [details] [review]
Bug 20305: Remove warnings from tools scripts
Comment 2 Roch D'Amour 2018-03-02 20:22:54 UTC
Hi Jonathan
Do you have a test plan for this?
Comment 3 Maksim Sen 2018-03-09 21:29:34 UTC
+1 for test plan
Comment 4 Jonathan Druart 2018-03-23 17:51:44 UTC
Test plan:
- do not apply the patch
- use the "Batch item modification"
=> Notice the warnings "Argument "" isn't numeric in numeric eq (==)"
- apply the patch
- use the "Batch item modification"
=> No more warnings
Comment 5 M. Tompsett 2018-03-26 06:01:54 UTC
Comment on attachment 72273 [details] [review]
Bug 20305: Remove warnings from tools scripts

Review of attachment 72273 [details] [review]:
-----------------------------------------------------------------

::: tools/batchMod.pl
@@ +315,4 @@
>  my $subfieldsToAllowForBatchmod = C4::Context->preference('SubfieldsToAllowForRestrictedBatchmod');
>  my $allowAllSubfields = (
>      not defined $subfieldsToAllowForBatchmod
> +      or $subfieldsToAllowForBatchmod ne q||

Wrong equality, I believe.
!~ /\S/ might be better?
Comment 6 Jonathan Druart 2018-03-26 14:11:20 UTC
I do not think we do that for other prefs.
Comment 7 Roch D'Amour 2018-03-30 20:44:08 UTC
Created attachment 73504 [details] [review]
Bug 20305: Remove warnings from tools scripts

Signed-off-by: Roch D'Amour <roch.damour@inlibro.com>
Comment 8 Julian Maurice 2018-04-09 12:22:25 UTC
Jonathan,

your patch changes '==' into 'ne'. It should be 'eq' (unless this behavior change is intended but it's not mentioned in the commit message)
Comment 9 Jonathan Druart 2018-04-09 13:51:12 UTC
Created attachment 73868 [details] [review]
Bug 20305: Remove warnings from tools scripts
Comment 10 Jonathan Druart 2018-04-09 13:51:46 UTC
(In reply to Julian Maurice from comment #8)
> Jonathan,
> 
> your patch changes '==' into 'ne'. It should be 'eq' (unless this behavior
> change is intended but it's not mentioned in the commit message)

Erk! Thanks for catching this!
Comment 11 Julian Maurice 2018-04-09 15:02:15 UTC
Created attachment 73876 [details] [review]
Bug 20305: Remove warnings from tools scripts

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Comment 12 Tomás Cohen Arazi 2018-05-04 19:39:03 UTC
Created attachment 75095 [details] [review]
Bug 20305: Remove warnings from tools scripts

Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 13 Jonathan Druart 2018-05-07 15:26:06 UTC
Pushed to master for 18.05, thanks to everybody involved!