Bug 16942

*** Bug 16904 has been marked as a duplicate of this bug. ***

Kyle, the following test should fail without this patch:
  is( $prepared_letter->{content}, $hold->id(), 'Hold object used correctly' );        

Could you please adapt it?

(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.

*** Bug 17115 has been marked as a duplicate of this bug. ***

(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

(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.

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>

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>

> % 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.

(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.

> 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!

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!

Pushed to master for Koha 16.11!

(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.

(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

(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