Bug 14145 - Noisy warns in admin/preferences.pl
Summary: Noisy warns in admin/preferences.pl
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: System Administration (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: M. Tompsett
QA Contact: Testopia
URL:
Whiteboard:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-05-05 05:02 UTC by Aleisha Amohia
Modified: 2016-12-05 21:23 UTC (History)
4 users (show)

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


Attachments
Bug 14145: Noisy warns in admin/preferences.pl (2.46 KB, patch)
2015-05-05 05:13 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14145: Noisy warns in admin/preferences.pl on Enhanced Content tab (1.06 KB, patch)
2015-05-05 05:49 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14145: Nicer counter-patch (2.11 KB, patch)
2015-05-09 05:39 UTC, M. Tompsett
Details | Diff | Splinter Review
[SIGNED-OFF] Bug 14145: Noisy warns in admin/preferences.pl (2.26 KB, patch)
2015-05-11 18:26 UTC, Bernardo Gonzalez Kriegel
Details | Diff | Splinter Review
Bug 14145: Noisy warns in admin/preferences.pl (2.43 KB, patch)
2015-05-22 09:17 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 Aleisha Amohia 2015-05-05 05:02:02 UTC
'Odd number elements in anonymous hash' warns on lines 116 and 121
Comment 1 Aleisha Amohia 2015-05-05 05:13:02 UTC Comment hidden (obsolete)
Comment 2 Aleisha Amohia 2015-05-05 05:49:09 UTC Comment hidden (obsolete)
Comment 3 M. Tompsett 2015-05-09 05:39:35 UTC Comment hidden (obsolete)
Comment 4 M. Tompsett 2015-05-09 05:44:57 UTC
To sign off this bug either:
1) sign off the first two and obsolete my counter patch
OR
2) sign off my counter patch and obsolete the first two.
Though, I personally am confused by some of the changes in the first two patches, so that is why I wrote the counter patch.
Comment 5 M. Tompsett 2015-05-09 05:54:11 UTC
Comment on attachment 38838 [details] [review]
Bug 14145: Noisy warns in admin/preferences.pl

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

::: admin/preferences.pl
@@ +46,4 @@
>      my $tab_template = C4::Templates::gettemplate( 'admin/preferences/' . $tab . '.pref', 'intranet', $input );
>  
>      my $active_currency = GetCurrency();
> +    my $local_currency;;

Double ;;'s?

@@ +112,5 @@
>              sort { $a->{'text'} cmp $b->{'text'} }
>                map {
>                  my $option_value = $_;
>                  {
> +                    text     => $options{type}->{$option_value},

This works, because the $options{type} doesn't exist, so two undefs reduces the list size by an even number for the failed grep too. There is no reason to change 'multiple' to 'type'.
Comment 6 M. Tompsett 2015-05-09 05:59:15 UTC
Comment on attachment 38839 [details] [review]
Bug 14145: Noisy warns in admin/preferences.pl on Enhanced Content tab

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

::: admin/preferences.pl
@@ +62,4 @@
>  
>      my $name = $options{'pref'};
>      my $chunk = { name => $name, value => $value, type => $options{'type'} || 'input', class => $options{'class'} };;
> +    $value //= '';

If $value is undef, then the question is what else is undef in the list above? $options{'pref'}? $options{'class'}? Because otherwise line 64 would be barfing errors too about uneven lists.

Perhaps there is something here to fix? Neither my counter patch nor this would fix it.
Comment 7 Bernardo Gonzalez Kriegel 2015-05-11 18:26:33 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2015-05-22 09:17:11 UTC
Created attachment 39384 [details] [review]
Bug 14145: Noisy warns in admin/preferences.pl

The problem with the odd number of hashes happens when not everything is selected.
The grep returns undef, because it isn't found in @values.
By turning the grep into a ternary-operator truth value, we can set a value (1 or 0) expressly.

The next problem is when nothing is selected in these multiple lists, $value is
undefined, so you can't split it. By splitting the definition of @values from
the actual splitting, we can split only if $value is defined, thus eliminating the
warning message.

TEST PLAN
---------
1) back up your koha error log file
2) blank your koha error log file
3) log in to the staff client
4) Home -> Koha administration -> Global system preferences
5) Click on every tab, EXCEPT local use.
6) notice the koha error log file has warnings.
7) blank the koha error log file again
8) apply this patch
9) Click on every tab, EXCEPT local use, again.
10) notice the koha error log file has no warnings.
11) koha qa test tools.

Signed-off-by: Bernardo Gonzalez Kriegel <bgkriegel@gmail.com>
I like more this version
Works as described
No koha-qa errors

Signed-off-by: Jonathan Druart <jonathan.druart@koha-community.org>
To reproduce: go on the OPAC tab, OpacAdvSearchMoreOptions  or
OpacAdvSearchOptions should not have all options selected.
Comment 9 Tomás Cohen Arazi 2015-05-22 14:20:52 UTC
Patch pushed to master.

Thanks Mark!