Bug 18494

Summary: Fix Letters.t (follow-up of 15702)
Product: Koha Reporter: Lari Taskula <lari.taskula>
Component: Test SuiteAssignee: Lari Taskula <lari.taskula>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: katrin.fischer, kyle, m.de.rooy, mtj, mtompset
Version: master   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 15702    
Bug Blocks:    
Attachments: Bug 18494: Fix Letters.t (follow-up of 5260)
Bug 18494: Fix Letters.t (follow-up of 15702)
Bug 18494: Fix Letters.t (follow-up of 15702)
Bug 18494: Fix Letters.t (follow-up of 15702)
Bug 18494: Pass a code, not hash & Fix double commas
Bug 18494: Pass a code, not hash & Fix double commas

Description Lari Taskula 2017-04-25 15:23:18 UTC
koha@kohacommunity:~/kohaclone$ perl t/db_dependent/Letters.t
1..81
ok 1 - use C4::Context;
ok 2 - use C4::Members;
ok 3 - use C4::Acquisition;
ok 4 - use C4::Biblio;
ok 5 - use C4::Letters;
Can't call method "get_expiry_date" on an undefined value at /home/koha/kohaclone/C4/Members.pm line 465.
# Looks like your test exited with 7 just after 5.

Problem is at line:
my $patron_category = $builder->build({ source => 'Category' });

Change to:
my $patron_category = $builder->build({ source => 'Category' })->{categorycode};
Comment 1 Lari Taskula 2017-04-25 15:28:59 UTC
Created attachment 62689 [details] [review]
Bug 18494: Fix Letters.t (follow-up of 5260)

koha@kohacommunity:~/kohaclone$ perl t/db_dependent/Letters.t
1..81
ok 1 - use C4::Context;
ok 2 - use C4::Members;
ok 3 - use C4::Acquisition;
ok 4 - use C4::Biblio;
ok 5 - use C4::Letters;
Can't call method "get_expiry_date" on an undefined value at
/home/koha/kohaclone/C4/Members.pm line 465.

Looks like your test exited with 7 just after 5.

Problem is at line:
my $patron_category = $builder->build({ source => 'Category' });

Change to:
my $patron_category = $builder->build({ source => 'Category' })->{categorycode};

To test:
1. Before applying patch, run t/db_dependent/Letters.t
2. Observe above failure
3. Apply patch
4. Run t/db_dependent/Letters.t
5. Watch it pass
Comment 2 Lari Taskula 2017-04-25 15:42:31 UTC
Now that I'm looking at this closer, I don't yet understand why it originally fails. If $patron_category is passed as a hashref to ->find, I'm thinking it should still work... I tested it on another container running with slightly different version and I don't experience this failure.
Comment 3 Lari Taskula 2017-04-25 15:47:47 UTC
Ah it seems it's broken by Bug 15702 and not Bug 5260. Sorry!
Comment 4 Lari Taskula 2017-04-26 08:45:07 UTC
Created attachment 62706 [details] [review]
Bug 18494: Fix Letters.t (follow-up of 15702)

koha@kohacommunity:~/kohaclone$ perl t/db_dependent/Letters.t
1..81
ok 1 - use C4::Context;
ok 2 - use C4::Members;
ok 3 - use C4::Acquisition;
ok 4 - use C4::Biblio;
ok 5 - use C4::Letters;
Can't call method "get_expiry_date" on an undefined value at
/home/koha/kohaclone/C4/Members.pm line 465.
Looks like your test exited with 7 just after 5.

This is caused by the new trimming functionality in AddMember when categorycode
is passed as a HASHref. Before trimming, make sure that the value is not a ref.

To test:
1. perl t/db_dependent/Letters.t
2. Observe above failure
3. Apply patch
4. Watch it pass
Comment 5 Lari Taskula 2017-04-26 08:46:25 UTC
Created attachment 62707 [details] [review]
Bug 18494: Fix Letters.t (follow-up of 15702)

koha@kohacommunity:~/kohaclone$ perl t/db_dependent/Letters.t
1..81
ok 1 - use C4::Context;
ok 2 - use C4::Members;
ok 3 - use C4::Acquisition;
ok 4 - use C4::Biblio;
ok 5 - use C4::Letters;
Can't call method "get_expiry_date" on an undefined value at
/home/koha/kohaclone/C4/Members.pm line 465.
Looks like your test exited with 7 just after 5.

This is caused by the new trimming functionality in AddMember when categorycode
is passed as a HASHref. Before trimming, make sure that the value is not a ref.

To test:
1. perl t/db_dependent/Letters.t
2. Observe above failure
3. Apply patch
4. Watch it pass
5. Also prove t/db_dependent/Members.t
Comment 6 Mark Tompsett 2017-04-27 00:56:26 UTC
Created attachment 62749 [details] [review]
Bug 18494: Fix Letters.t (follow-up of 15702)

koha@kohacommunity:~/kohaclone$ perl t/db_dependent/Letters.t
1..81
ok 1 - use C4::Context;
ok 2 - use C4::Members;
ok 3 - use C4::Acquisition;
ok 4 - use C4::Biblio;
ok 5 - use C4::Letters;
Can't call method "get_expiry_date" on an undefined value at
/home/koha/kohaclone/C4/Members.pm line 465.
Looks like your test exited with 7 just after 5.

This is caused by the new trimming functionality in AddMember when categorycode
is passed as a HASHref. Before trimming, make sure that the value is not a ref.

To test:
1. perl t/db_dependent/Letters.t
2. Observe above failure
3. Apply patch
4. Watch it pass
5. Also prove t/db_dependent/Members.t

NOTE: Oops! Thanks for the fix.
Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 7 Mark Tompsett 2017-04-27 00:58:40 UTC
As this bug is marked as a follow up of 15702, I did not fail it, when line 299 failed because it crossed the second boundary on the biblio timestamp. Re-running succeeded with no second boundary crossed.
Comment 8 Marcel de Rooy 2017-05-01 12:51:24 UTC
(In reply to Lari Taskula from comment #2)
> Now that I'm looking at this closer, I don't yet understand why it
> originally fails. If $patron_category is passed as a hashref to ->find, I'm
> thinking it should still work... I tested it on another container running
> with slightly different version and I don't experience this failure.

Passing categorycode as a hashref seems to be wrong. Why are we fixing this error in the wrong place ?
Please convince me before passing qa on this one.
Comment 9 Mark Tompsett 2017-05-01 16:44:47 UTC
(In reply to Lari Taskula from comment #2)
> Now that I'm looking at this closer, I don't yet understand why it
> originally fails. If $patron_category is passed as a hashref to ->find, I'm
> thinking it should still work... I tested it on another container running
> with slightly different version and I don't experience this failure.

Because the builder returns a hash code, when the patron_category is supposed to be a scalar.

The solution in comment #0 is correct.
Comment 10 Mark Tompsett 2017-05-01 16:46:05 UTC
(In reply to Lari Taskula from comment #2)
> I don't yet understand why it originally fails.

Also, there was a double comma.
Wrong number of commas do weird things!
Comment 11 Mark Tompsett 2017-05-01 16:48:14 UTC
Created attachment 62926 [details] [review]
Bug 18494: Pass a code, not hash & Fix double commas

TEST PLAN
---------
1) prove -v t/db_dependent/Letters.t
   -- fails at test 5 or so.
2) apply patch
3) run step 1
   -- success
4) run koha qa test tools
Comment 12 Mark Tompsett 2017-05-01 16:49:39 UTC
DO NOT APPLY BOTH! The second one is my counter-patch.
If that is acceptable. obsolete the first and sign off the second.
Comment 13 Marcel de Rooy 2017-05-02 12:10:28 UTC
Created attachment 62954 [details] [review]
Bug 18494: Pass a code, not hash & Fix double commas

TEST PLAN
---------
1) prove -v t/db_dependent/Letters.t
   -- fails at test 5 or so.
2) apply patch
3) run step 1
   -- success
4) run koha qa test tools

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 14 Marcel de Rooy 2017-05-02 12:11:29 UTC
(In reply to M. Tompsett from comment #12)
> If that is acceptable. obsolete the first and sign off the second.

Counter patch is a trivial one. Passing QA immediately. First patch obsoleted. If there is a need for it, please open a new report.
Comment 15 Mark Tompsett 2017-05-02 16:06:03 UTC
Just noticed that my patch is identical to Lari's first obsoleted patch. Feel free to change author to Lari instead.
Comment 16 Kyle M Hall 2017-05-12 13:21:00 UTC
Pushed to master for 17.05, thanks Mark!
Comment 17 Katrin Fischer 2017-05-14 09:27:44 UTC
This patch has been pushed to 16.11.x and will be in 16.11.08.
Comment 18 Mason James 2017-05-24 02:29:26 UTC
Pushed to 16.05.x, for 16.05.13 release