Bug 17558 - Fix t/db_dependent/Koha/Patron/Messages.t
Summary: Fix t/db_dependent/Koha/Patron/Messages.t
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Josef Moravec
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 17397
Blocks:
  Show dependency treegraph
 
Reported: 2016-11-04 18:42 UTC by Josef Moravec
Modified: 2019-06-27 09:24 UTC (History)
6 users (show)

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


Attachments
Bug 17558: Fix adding manager id to patron messages in store method (922 bytes, patch)
2016-11-04 18:51 UTC, Josef Moravec
Details | Diff | Splinter Review
Bug 17558: Add more tests (2.41 KB, patch)
2016-11-04 18:52 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 17558: Fix adding manager id to patron messages in store method (1.00 KB, patch)
2016-11-10 16:56 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 17558: Add more tests (2.51 KB, patch)
2016-11-10 16:56 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
Bug 17558: QA followup - use TestBuilder for creating patron (1.03 KB, patch)
2016-11-11 11:25 UTC, Josef Moravec
Details | Diff | Splinter Review
[SIGNED-OFF]Bug 17558: QA followup - use TestBuilder for creating patron (1.11 KB, patch)
2016-11-11 14:55 UTC, Héctor Eduardo Castro Avalos
Details | Diff | Splinter Review
Bug 17558: Fix adding manager id to patron messages in store method (1.08 KB, patch)
2016-11-11 15:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17558: Add more tests (2.59 KB, patch)
2016-11-11 15:53 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 17558: QA followup - use TestBuilder for creating patron (1.18 KB, patch)
2016-11-11 15:54 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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 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