Bug 14480 - Warns when modifying patron
Summary: Warns when modifying patron
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Patrons (show other bugs)
Version: Main
Hardware: All All
: P5 - low minor (vote)
Assignee: Aleisha Amohia
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2015-07-01 05:02 UTC by Aleisha Amohia
Modified: 2017-06-14 22:12 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 14480: Silences warns triggered when modifying patron (2.14 KB, patch)
2015-07-01 05:15 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14480: Silencing warns from editing patron (1.42 KB, patch)
2015-12-14 19:49 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14480: Silences warn from editing patron (1.50 KB, patch)
2015-12-18 00:25 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14480: Silences warn from editing patron (1.43 KB, patch)
2015-12-29 19:40 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14480: Switching the IF/ELSE (1.93 KB, patch)
2016-01-06 00:38 UTC, Aleisha Amohia
Details | Diff | Splinter Review
Bug 14480: Silences warn from editing patron (2.07 KB, patch)
2016-01-06 01:53 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14480: Silences warn from editing patron (2.02 KB, patch)
2016-01-06 01:56 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 14480: Silences warn from editing patron (2.09 KB, patch)
2016-01-06 08:37 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-07-01 05:02:34 UTC
To recreate warns, go to any patron page (ie a link such as http://localhost:8081/cgi-bin/koha/members/moremember.pl?borrowernumber=1369) and click 'Edit'

memberentry.pl: Argument "" isn't numeric in numeric eq (==) at /home/vagrant/kohaclone/members/memberentry.pl line 566
memberentry.pl: Argument "" isn't numeric in numeric lt (<) at /home/vagrant/kohaclone/koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt line 949
Comment 1 Aleisha Amohia 2015-07-01 05:15:42 UTC Comment hidden (obsolete)
Comment 2 Jonathan Druart 2015-07-02 14:47:20 UTC
Comment on attachment 40744 [details] [review]
Bug 14480: Silences warns triggered when modifying patron

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

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
@@ +916,4 @@
>                <fieldset class="rows">
>                  <legend>Patron restrictions</legend>
>  
> +                [% IF ( debarments.size  && debarments.size < 1 ) %]

It should be
  IF debarments and debarments.size
That makes more sense.

::: members/memberentry.pl
@@ +563,4 @@
>  
>      if ($default_city) { # flag the current or default val
>          for my $city ( @{$city_arrayref} ) {
> +            if ($default_city eq $city->{cityid}) {

I don't get this warning but cityid is an integer, so == makes sense.
$default_city is tested before so cityid is empty? Don't understand how it's possible (comes from the DB, stored as int)
Comment 3 Aleisha Amohia 2015-12-14 19:49:03 UTC Comment hidden (obsolete)
Comment 4 Owen Leonard 2015-12-16 13:10:15 UTC
This doesn't work correctly for me. Before the patch, if the patron had no restrictions the restrictions table wouldn't display. Instead there would be a message "Patron is currently unrestricted."

After the patch, there is no message and a table with only a header row appears.
Comment 5 Aleisha Amohia 2015-12-18 00:25:37 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2015-12-28 16:21:25 UTC
fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt).
Comment 7 Jonathan Druart 2015-12-28 16:22:15 UTC
See comment 2 for the correct way to fix it.
Comment 8 Aleisha Amohia 2015-12-28 20:00:08 UTC
(In reply to Jonathan Druart from comment #7)
> See comment 2 for the correct way to fix it.

Hi Jonathan, I made your change to 
[% IF ( debarments.size  && debarments.size < 1 ) %]
which does get rid of the warn, however it gives the same problem in Comment 4 where we see an empty table instead of the message.
Comment 9 Aleisha Amohia 2015-12-28 20:01:08 UTC
Sorry, I made the change to [% IF ( debarments && debarments.size < 1 ) %]
Comment 10 Aleisha Amohia 2015-12-29 19:40:23 UTC Comment hidden (obsolete)
Comment 11 Jonathan Druart 2015-12-30 12:50:04 UTC
Comment on attachment 46025 [details] [review]
Bug 14480: Silences warn from editing patron

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

::: koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt
@@ +941,4 @@
>                <fieldset class="rows">
>                  <legend>Patron restrictions</legend>
>  
> +                [% IF ( debarments.size = 'NULL' ) %]

I cannot explain why this works, but it's not the way to do :)
size is a virtual method provided by Template::Toolkit, you cannot affect it the "NULL" string.
Moreover you are using = instead of ==
Anyway, what I have proposed previously was the wrong test indeed, it should be
  [% UNLESS debarments %]
Comment 12 Mark Tompsett 2016-01-02 03:29:52 UTC
(In reply to Jonathan Druart from comment #11)
> Anyway, what I have proposed previously was the wrong test indeed, it should
> be
>   [% UNLESS debarments %]

Or clearer, swap the IF/ELSE.
You know you are generating the table [% IF (debarments) %], [% ELSE %] you are putting out that other message.

The advantage of Jonathan's suggestion is smaller patch size. The disadvantage is that the novice programmer does not usually think in negation logic.
Comment 13 Mark Tompsett 2016-01-02 05:18:01 UTC
(In reply to Aleisha Amohia from comment #9)
> Sorry, I made the change to [% IF ( debarments && debarments.size < 1 ) %]

Actually, if you wanted to use an UGLY condition.
[% IF ((! debarments) || (debarments && debarments.size<1)) %]

But let's not go there. :)
Comment 14 Aleisha Amohia 2016-01-06 00:38:19 UTC
I tried the UNLESS, and it was successful in clearing the warn but it gave us the empty table again, so decided to go with the IF/ELSE swap.
Comment 15 Aleisha Amohia 2016-01-06 00:38:40 UTC Comment hidden (obsolete)
Comment 16 Mark Tompsett 2016-01-06 01:53:25 UTC Comment hidden (obsolete)
Comment 17 Mark Tompsett 2016-01-06 01:56:42 UTC Comment hidden (obsolete)
Comment 18 Jonathan Druart 2016-01-06 08:37:22 UTC
Created attachment 46305 [details] [review]
Bug 14480: Silences warn from editing patron

Argument "" isn't numeric in numeric lt (<)

TEST PLAN
---------
1) Go to any patron in staff client
2) Click 'Edit'
3) Check error logs
   -- warnings should exist.
4) Apply patch
5) Reload page
   -- warnings should not be triggered.
6) Add restriction and save
7) Go back to edit screen
   -- restriction should be shown
8) Remove restriction and save
9) Go back to edit screen
   -- Should say "Patron is currently unrestricted"
10) Run koha qa test tools.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 19 Brendan Gallagher 2016-01-27 04:40:49 UTC
Pushed to Master - Should be in the May 2016 release.  Thanks!
Comment 20 Julian Maurice 2016-01-29 14:49:51 UTC
Patch pushed to 3.22.x, will be in 3.22.3
Comment 21 Frédéric Demians 2016-02-12 06:42:28 UTC
This patch has been pushed to 3.20.x, will be in 3.20.9.