Bug 18494 - Fix Letters.t (follow-up of 15702)
Summary: Fix Letters.t (follow-up of 15702)
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Test Suite (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Target Milestone: ---
Assignee: Lari Taskula
QA Contact: Marcel de Rooy
URL:
Whiteboard:
Keywords:
Depends on: 15702
Blocks:
  Show dependency treegraph
 
Reported: 2017-04-25 15:23 UTC by Lari Taskula
Modified: 2017-12-07 22:20 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Bot Control: ---
When did the bot last check this:
Who signed the patch off:
Text to go in the release notes:


Attachments
Bug 18494: Fix Letters.t (follow-up of 5260) (1.67 KB, patch)
2017-04-25 15:28 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18494: Fix Letters.t (follow-up of 15702) (1.80 KB, patch)
2017-04-26 08:45 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18494: Fix Letters.t (follow-up of 15702) (1.84 KB, patch)
2017-04-26 08:46 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18494: Fix Letters.t (follow-up of 15702) (1.89 KB, patch)
2017-04-27 00:56 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18494: Pass a code, not hash & Fix double commas (1.16 KB, patch)
2017-05-01 16:48 UTC, M. Tompsett
Details | Diff | Splinter Review
Bug 18494: Pass a code, not hash & Fix double commas (1.26 KB, patch)
2017-05-02 12:10 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 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 M. 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 M. 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 M. 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 M. 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 M. 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 M. 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 M. 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