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)

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.

fatal: sha1 information is lacking or useless (koha-tmpl/intranet-tmpl/prog/en/modules/members/memberentrygen.tt).

See comment 2 for the correct way to fix it.

(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.

Sorry, I made the change to [% IF ( debarments && debarments.size < 1 ) %]

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 %]

(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.

(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. :)

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.

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>

Pushed to Master - Should be in the May 2016 release.  Thanks!

Patch pushed to 3.22.x, will be in 3.22.3

This patch has been pushed to 3.20.x, will be in 3.20.9.