Bug 17558

Summary: Fix t/db_dependent/Koha/Patron/Messages.t
Product: Koha Reporter: Josef Moravec <josef.moravec>
Component: Architecture, internals, and plumbingAssignee: Josef Moravec <josef.moravec>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: major    
Priority: P5 - low CC: bgkriegel, hector.hecaxmmx, jonathan.druart, kyle, m.de.rooy, mtj
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: Small patch Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 17397    
Bug Blocks:    
Attachments: Bug 17558: Fix adding manager id to patron messages in store method
Bug 17558: Add more tests
[SIGNED-OFF]Bug 17558: Fix adding manager id to patron messages in store method
[SIGNED-OFF]Bug 17558: Add more tests
Bug 17558: QA followup - use TestBuilder for creating patron
[SIGNED-OFF]Bug 17558: QA followup - use TestBuilder for creating patron
Bug 17558: Fix adding manager id to patron messages in store method
Bug 17558: Add more tests
Bug 17558: QA followup - use TestBuilder for creating patron

Description Josef Moravec 2016-11-04 18:42:31 UTC

    
Comment 1 Josef Moravec 2016-11-04 18:51:58 UTC
Created attachment 57222 [details] [review]
Bug 17558: Fix adding manager id to patron messages in store method

Test plan:

prove t/db_dependent/Koha/Patron/Messages.t
Comment 2 Josef Moravec 2016-11-04 18:52:04 UTC
Created attachment 57223 [details] [review]
Bug 17558: Add more tests
Comment 3 Bernardo Gonzalez Kriegel 2016-11-09 22:00:44 UTC
Hi, I got an error with koha-qa script

 FAIL   t/db_dependent/Koha/Patron/Messages.t
   OK     critic
   OK     forbidden patterns
   OK     git manipulation
   OK     pod
   OK     spelling
   FAIL   valid
                Use of uninitialized value $_[0] in lc

Don't know the cause.
Comment 4 Josef Moravec 2016-11-09 22:24:37 UTC
Strange, my koha-qa says it's OK...
Comment 5 Jonathan Druart 2016-11-10 08:12:08 UTC
Same, they pass for me, try and update your QA script repo.
Comment 6 Héctor Eduardo Castro Avalos 2016-11-10 16:52:33 UTC
This is a(In reply to Bernardo Gonzalez Kriegel from comment #3)
> Hi, I got an error with koha-qa script
> 
>  FAIL   t/db_dependent/Koha/Patron/Messages.t
>    OK     critic
>    OK     forbidden patterns
>    OK     git manipulation
>    OK     pod
>    OK     spelling
>    FAIL   valid
>                 Use of uninitialized value $_[0] in lc
> 
> Don't know the cause.

This is a problem from old Cache.pm in Debian and maybe in some version of Ubuntu

The old module 2.04
>sub Canonicalize_Expiration_Time {
>    my $timespec = lc($_[0])
>        or return undef;
>
>    my $time;

And the update module 2.11:

>sub Canonicalize_Expiration_Time {
>    my $timespec;
> 
>    my $timespec_param = shift(@_);
>    if (! $timespec_param)
>    {
>        return undef;
>    }
>    $timespec = lc($timespec_param);
> 
>    my $time;

Regards
Comment 7 Héctor Eduardo Castro Avalos 2016-11-10 16:56:25 UTC
Created attachment 57430 [details] [review]
[SIGNED-OFF]Bug 17558: Fix adding manager id to patron messages in store method

Test plan:

prove t/db_dependent/Koha/Patron/Messages.t

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Works as advertised. Pass OK
Comment 8 Héctor Eduardo Castro Avalos 2016-11-10 16:56:33 UTC
Created attachment 57431 [details] [review]
[SIGNED-OFF]Bug 17558: Add more tests

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
All tests pass successfuly
Comment 9 Marcel de Rooy 2016-11-11 07:08:59 UTC
 my $patron         = $builder->build( { source => 'Borrower', values => { branchcode => $library->{branchcode} } } );
+my $patron_2       = Koha::Patrons->search->next;

Wouldn't it be more consistent to use TestBuilder for both patrons. This is confusing and might give unexpected results depending on patrons present?
Comment 10 Josef Moravec 2016-11-11 11:25:08 UTC
Created attachment 57455 [details] [review]
Bug 17558: QA followup - use TestBuilder for creating patron
Comment 11 Josef Moravec 2016-11-11 11:26:30 UTC
(In reply to Marcel de Rooy from comment #9)
>  my $patron         = $builder->build( { source => 'Borrower', values => {
> branchcode => $library->{branchcode} } } );
> +my $patron_2       = Koha::Patrons->search->next;
> 
> Wouldn't it be more consistent to use TestBuilder for both patrons. This is
> confusing and might give unexpected results depending on patrons present?

You are right, I've attached a followup patch
Comment 12 Héctor Eduardo Castro Avalos 2016-11-11 14:55:25 UTC
Created attachment 57456 [details] [review]
[SIGNED-OFF]Bug 17558: QA followup - use TestBuilder for creating patron

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Comment 13 Marcel de Rooy 2016-11-11 15:53:51 UTC
Created attachment 57457 [details] [review]
Bug 17558: Fix adding manager id to patron messages in store method

Test plan:

prove t/db_dependent/Koha/Patron/Messages.t

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
Works as advertised. Pass OK

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2016-11-11 15:53:56 UTC
Created attachment 57458 [details] [review]
Bug 17558: Add more tests

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>
All tests pass successfuly

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2016-11-11 15:54:00 UTC
Created attachment 57459 [details] [review]
Bug 17558: QA followup - use TestBuilder for creating patron

Signed-off-by: Hector Castro <hector.hecaxmmx@gmail.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Kyle M Hall (khall) 2016-11-12 02:33:13 UTC
Pushed to master for 16.11, thanks Josef!
Comment 17 Mason James 2016-11-30 11:07:43 UTC
(In reply to Kyle M Hall from comment #16)
> Pushed to master for 16.11, thanks Josef!

blocked by DB changes, passing for 16.05