Bug 16942

Summary: Confirm hold results in ugly error
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Hold requestsAssignee: Kyle M Hall <kyle>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: major    
Priority: P5 - low CC: abl, fridolin.somers, gmcharlt, jonathan.druart+koha, mirko, mtj, veron
Version: master   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17101
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=17246
Change sponsored?: --- Patch complexity: Small patch
Text to go in the release notes:
Version(s) released in:
Bug Depends on: 14757    
Bug Blocks: 11360    
Attachments: Bug 16942 - Confirm hold results in ugly error
Bug 16942 - Confirm hold results in ugly error
Bug 16942 [QA Followup] - Add unit test
[PASSED QA] Bug 16942 - Confirm hold results in ugly error
[PASSED QA] Bug 16942 [QA Followup] - Add unit test
Bug 16942 - Confirm hold results in ugly error
Bug 16942 [QA Followup] - Add unit test

Description Kyle M Hall 2016-07-19 16:55:27 UTC
Confirming a hold to set it to waiting will result in an DBIC error in master.
Comment 1 Kyle M Hall 2016-07-19 16:58:28 UTC Comment hidden (obsolete)
Comment 2 Kyle M Hall 2016-07-19 17:16:17 UTC
*** Bug 16904 has been marked as a duplicate of this bug. ***
Comment 3 Mirko Tietgen 2016-07-19 17:54:39 UTC Comment hidden (obsolete)
Comment 4 Jonathan Druart 2016-07-20 15:28:13 UTC
Kyle, the following test should fail without this patch:
  is( $prepared_letter->{content}, $hold->id(), 'Hold object used correctly' );        

Could you please adapt it?
Comment 5 Kyle M Hall 2016-07-26 15:03:06 UTC Comment hidden (obsolete)
Comment 6 Katrin Fischer 2016-07-26 19:54:36 UTC Comment hidden (obsolete)
Comment 7 Katrin Fischer 2016-07-26 19:55:25 UTC Comment hidden (obsolete)
Comment 8 Jonathan Druart 2016-08-01 10:51:00 UTC
(In reply to Jonathan Druart from comment #4)
> Kyle, the following test should fail without this patch:
>   is( $prepared_letter->{content}, $hold->id(), 'Hold object used correctly'
> );        
> 
> Could you please adapt it?

The test still passes.
Comment 9 Jonathan Druart 2016-08-12 10:29:14 UTC
*** Bug 17115 has been marked as a duplicate of this bug. ***
Comment 10 Kyle M Hall 2016-08-17 17:27:26 UTC
(In reply to Jonathan Druart from comment #8)
> (In reply to Jonathan Druart from comment #4)
> > Kyle, the following test should fail without this patch:
> >   is( $prepared_letter->{content}, $hold->id(), 'Hold object used correctly'
> > );        
> > 
> > Could you please adapt it?
> 
> The test still passes.

They unit tests fail for me without the initial patch applied.

prove t/db_dependent/Letters/TemplateToolkit.t
t/db_dependent/Letters/TemplateToolkit.t .. 1/14 DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'BIBLIONUMBER '557' AND `ARRAY(0x8818f68)` BORROWERNUMBER '140' ) AND `ARRAY(0x88' at line 1 [for Statement "SELECT `me`.`reserve_id`, `me`.`borrowernumber`, `me`.`reservedate`, `me`.`biblionumber`, `me`.`branchcode`, `me`.`notificationdate`, `me`.`reminderdate`, `me`.`cancellationdate`, `me`.`reservenotes`, `me`.`priority`, `me`.`found`, `me`.`timestamp`, `me`.`itemnumber`, `me`.`waitingdate`, `me`.`expirationdate`, `me`.`lowestPriority`, `me`.`suspend`, `me`.`suspend_until`, `me`.`itemtype` FROM `reserves` `me` WHERE ( ( ( `ARRAY(0x8818f68)` BIBLIONUMBER ? AND `ARRAY(0x8818f68)` BORROWERNUMBER ? ) AND `ARRAY(0x8818f68)` RESERVE_ID ? ) )" with ParamValues: 0='557', 1='140', 2='114'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832.
DBD::mysql::st execute failed: You have an error in your SQL syntax; check the manual that corresponds to your MySQL server version for the right syntax to use near 'BIBLIONUMBER '557' AND `ARRAY(0x8818f68)` BORROWERNUMBER '140' ) AND `ARRAY(0x88' at line 1 [for Statement "SELECT `me`.`reserve_id`, `me`.`borrowernumber`, `me`.`reservedate`, `me`.`biblionumber`, `me`.`branchcode`, `me`.`notificationdate`, `me`.`reminderdate`, `me`.`cancellationdate`, `me`.`reservenotes`, `me`.`priority`, `me`.`found`, `me`.`timestamp`, `me`.`itemnumber`, `me`.`waitingdate`, `me`.`expirationdate`, `me`.`lowestPriority`, `me`.`suspend`, `me`.`suspend_until`, `me`.`itemtype` FROM `reserves` `me` WHERE ( ( ( `ARRAY(0x8818f68)` BIBLIONUMBER ? AND `ARRAY(0x8818f68)` BORROWERNUMBER ? ) AND `ARRAY(0x8818f68)` RESERVE_ID ? ) )" with ParamValues: 0='557', 1='140', 2='114'] at /usr/share/perl5/DBIx/Class/Storage/DBI.pm line 1832.
# Looks like your test exited with 255 just after 8.
t/db_dependent/Letters/TemplateToolkit.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 6/14 subtests

Test Summary Report
-------------------
t/db_dependent/Letters/TemplateToolkit.t (Wstat: 65280 Tests: 8 Failed: 0)
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 14 tests but ran 8.
Files=1, Tests=8,  3 wallclock secs ( 0.02 usr  0.01 sys +  1.74 cusr  0.18 csys =  1.95 CPU)
Result: FAIL
Comment 11 Jonathan Druart 2016-08-18 08:33:35 UTC
(In reply to Kyle M Hall from comment #10)
> (In reply to Jonathan Druart from comment #8)
> > (In reply to Jonathan Druart from comment #4)
> > > Kyle, the following test should fail without this patch:
> > >   is( $prepared_letter->{content}, $hold->id(), 'Hold object used correctly'
> > > );        
> > > 
> > > Could you please adapt it?
> > 
> > The test still passes.
> 
> They unit tests fail for me without the initial patch applied.

% git fetch origin
% git reset --hard origin/master
% git clean -f -d
%  prove t/db_dependent/Letters/TemplateToolkit.t
t/db_dependent/Letters/TemplateToolkit.t .. ok     
All tests successful.
Files=1, Tests=13,  4 wallclock secs ( 0.06 usr  0.00 sys +  2.78 cusr  0.10 csys =  2.94 CPU)
Result: PASS

What I meant is that the test 'Hold object used correctly' successes when we are expecting it to fail.
Comment 12 Jesse Maseto 2016-08-18 13:57:05 UTC
Created attachment 54569 [details] [review]
Bug 16942 - Confirm hold results in ugly error

Confirming a hold to set it to waiting will result in an DBIC error in
master.

Test Plan:
1) Attempt to check in an item on hold and confirm the hold
2) Note the error
3) Apply this patch
4) Repeat step 1
5) Note there is no error!

Signed-off-by: Mirko Tietgen <mirko@abunchofthings.net>

https://bugs.koha-community.org/show_bug.cgi?id=14942

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Comment 13 Jesse Maseto 2016-08-18 13:57:14 UTC
Created attachment 54570 [details] [review]
Bug 16942 [QA Followup] - Add unit test

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Comment 14 Kyle M Hall 2016-08-18 15:37:33 UTC
> % git fetch origin
> % git reset --hard origin/master
> % git clean -f -d
> %  prove t/db_dependent/Letters/TemplateToolkit.t
> t/db_dependent/Letters/TemplateToolkit.t .. ok     
> All tests successful.
> Files=1, Tests=13,  4 wallclock secs ( 0.06 usr  0.00 sys +  2.78 cusr  0.10
> csys =  2.94 CPU)
> Result: PASS
> 
> What I meant is that the test 'Hold object used correctly' successes when we
> are expecting it to fail.

Are you applying the "Bug 16942 [QA Followup] - Add unit test" patch to master before running the tests? If you apply that patch, the test should fail. Then, if you apply "Bug 16942 - Confirm hold results in ugly error" the test should pass.
Comment 15 Jonathan Druart 2016-08-18 16:03:35 UTC
(In reply to Kyle M Hall from comment #14)
> > % git fetch origin
> > % git reset --hard origin/master
> > % git clean -f -d
> > %  prove t/db_dependent/Letters/TemplateToolkit.t
> > t/db_dependent/Letters/TemplateToolkit.t .. ok     
> > All tests successful.
> > Files=1, Tests=13,  4 wallclock secs ( 0.06 usr  0.00 sys +  2.78 cusr  0.10
> > csys =  2.94 CPU)
> > Result: PASS
> > 
> > What I meant is that the test 'Hold object used correctly' successes when we
> > are expecting it to fail.
> 
> Are you applying the "Bug 16942 [QA Followup] - Add unit test" patch to
> master before running the tests? If you apply that patch, the test should
> fail. Then, if you apply "Bug 16942 - Confirm hold results in ugly error"
> the test should pass.

I completely understood that.

So maybe you should explain me why the following test pass:

180 $prepared_letter = GetPreparedLetter(
181     (
182         module      => 'test',
183         letter_code => 'TEST_HOLD',
184         tables      => { 
185             reserves => [ $patron->{borrowernumber}, $biblio->id() ]
186         },
187     )
188 );
189 is( $prepared_letter->{content}, $hold->id(), 'Hold object used correctly' )

What does reserves => { $borrowernumber, $biblionumber } mean?
How do you know that $borrowernumber is a borrowernumber?
It looks like something magic and to me it should not work.
Comment 16 Kyle M Hall 2016-08-19 11:44:15 UTC
> So maybe you should explain me why the following test pass:
> 
> 180 $prepared_letter = GetPreparedLetter(
> 181     (
> 182         module      => 'test',
> 183         letter_code => 'TEST_HOLD',
> 184         tables      => { 
> 185             reserves => [ $patron->{borrowernumber}, $biblio->id() ]
> 186         },
> 187     )
> 188 );
> 189 is( $prepared_letter->{content}, $hold->id(), 'Hold object used
> correctly' )
> 
> What does reserves => { $borrowernumber, $biblionumber } mean?
> How do you know that $borrowernumber is a borrowernumber?
> It looks like something magic and to me it should not work.

Ah, I see now! I'll look into that!
Comment 17 Kyle M Hall 2016-09-01 18:34:14 UTC
I looked into this and it's working exactly as intended! Basically, the tables hashref is a list of key/value pairs. The value can either be a) a hashref or b) an arrayref. If it is a hashref, the keys are pulled out as would be expected, based on the key name. For the arrayref, they simply need to be ordered correctly. In the case of Holds, the lookup key is the fk combo borrowernumber/biblionumber, in that order. 

(In reply to Kyle M Hall from comment #16)
> > So maybe you should explain me why the following test pass:
> > 
> > 180 $prepared_letter = GetPreparedLetter(
> > 181     (
> > 182         module      => 'test',
> > 183         letter_code => 'TEST_HOLD',
> > 184         tables      => { 
> > 185             reserves => [ $patron->{borrowernumber}, $biblio->id() ]
> > 186         },
> > 187     )
> > 188 );
> > 189 is( $prepared_letter->{content}, $hold->id(), 'Hold object used
> > correctly' )
> > 
> > What does reserves => { $borrowernumber, $biblionumber } mean?
> > How do you know that $borrowernumber is a borrowernumber?
> > It looks like something magic and to me it should not work.
> 
> Ah, I see now! I'll look into that!
Comment 18 Kyle M Hall 2016-09-01 18:34:46 UTC
Pushed to master for Koha 16.11!
Comment 19 Jonathan Druart 2016-09-05 09:21:21 UTC
(In reply to Kyle M Hall from comment #17)
> I looked into this and it's working exactly as intended! Basically, the
> tables hashref is a list of key/value pairs. The value can either be a) a
> hashref or b) an arrayref. If it is a hashref, the keys are pulled out as
> would be expected, based on the key name. For the arrayref, they simply need
> to be ordered correctly. In the case of Holds, the lookup key is the fk
> combo borrowernumber/biblionumber, in that order. 

I'd suggest to remove this behavior and only supports hashref, would you agree? See bug 17246.
Comment 20 Mason James 2017-05-24 00:58:08 UTC
(In reply to Kyle M Hall from comment #18)
> Pushed to master for Koha 16.11!

Blocked by Enhancement (bz14757), skipping for 16.05.x
Comment 21 Fridolin Somers 2017-09-12 15:18:47 UTC
(In reply to Mason James from comment #20)
> (In reply to Kyle M Hall from comment #18)
> > Pushed to master for Koha 16.11!
> 
> Blocked by Enhancement (bz14757), skipping for 16.05.x

Resolved fixed then