Bug 26648

Summary: Prevent internal server error if item attached to old checkout has been removed
Product: Koha Reporter: Martin Renvoize <martin.renvoize>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: arthur.suzuki, bibliwho, caroline.cyr-la-rose, jonathan.druart, lucas, sally.healey
Version: Main   
Hardware: All   
OS: All   
Change sponsored?: --- Patch complexity: Trivial patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
22.11.00, 22.05.08
Bug Depends on:    
Bug Blocks: 31051    
Attachments: Bug 26648: Prevent explosion on missin item
Bug 26648: Prevent explosion in notices on missing item
Bug 26648: Prevent explosion in notices on missing item
Bug 26648: Add tests
Bug 26648: Prevent explosion in notices on missing item
Bug 26648: Add tests
Bug 26648: Add tests

Description Martin Renvoize 2020-10-09 15:12:18 UTC
Whilst playing with notices to see if we could calculate a running total of savings for a library user to add to their issueslip notice I found that if an item relating to an old checkout has been deleted, then the relation accessor can explode
Comment 1 Martin Renvoize 2020-10-09 15:16:39 UTC
Created attachment 111416 [details] [review]
Bug 26648: Prevent explosion on missin item

The item accessor in Koha::Old::Checkouts could explode given a checkout
where the item had been deleted.

To test
1/ Edit the issueslip notice to output some item details from the
borrowers old checkouts

[% FOREACH old_checkout IN borrower.old_checkouts %]
  [% IF old_checkout.item %]
    [% old_checkout.item.price %]
  [% END %]
[% END %]

2/ Delete the item associated with an old checkout for a test patron
3/ Print the issueslip for the patron
4/ Note a server side error is triggered
5/ Apply the patch
6/ Print the slip again and note the price details for old checkouts are
now displayed
7/ Signoff
Comment 2 Martin Renvoize 2020-10-09 15:28:58 UTC
Dumping this example here so I don't loose it

[% USE Price %]
[% savings = 0 %]
[% FOREACH checkout IN borrower.checkouts %]
[% savings = checkout.item.price + savings %]
[% END %]

[% FOREACH old_checkout IN borrower.old_checkouts %]
[% IF old_checkout.item %]
[% savings = old_checkout.item.price + savings %]
[% END %]
[% END %]
You have saved [% savings | $Price %] to date by using the library.
Comment 3 Caroline Cyr La Rose 2020-10-09 20:48:10 UTC
Created attachment 111435 [details] [review]
Bug 26648: Prevent explosion in notices on missing item

The item accessor in Koha::Old::Checkouts could explode given a checkout
where the item had been deleted.

To test
1/ Edit the issueslip notice to output some item details from the
borrowers old checkouts

[% FOREACH old_checkout IN borrower.old_checkouts %]
  [% IF old_checkout.item %]
    [% old_checkout.item.price %]
  [% END %]
[% END %]

2/ Delete the item associated with an old checkout for a test patron
3/ Print the issueslip for the patron
4/ Note a server side error is triggered
5/ Apply the patch
6/ Print the slip again and note the price details for old checkouts are
now displayed
7/ Signoff

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Comment 4 Caroline Cyr La Rose 2020-10-09 20:49:32 UTC
YES! It works!!

I changed the title of the commit to correct a typo and make it more specific. I hope that's ok.
Comment 5 Caroline Cyr La Rose 2020-10-09 20:50:25 UTC
My complete slip for future reference

[%- USE KohaDates -%]
[%- USE Price -%]
[% totalValue = 0 %]
[% totalSavings = 0 %]
<h3><<branches.branchname>></h3>
Checked out by <<borrowers.firstname>> <<borrowers.surname>> <br />
(<<borrowers.cardnumber>>) <br />
 
<<today>><br />
 
<h4>Borrowed today</h4>
[% FOREACH checkout IN checkouts %]
[%~ SET item = checkout.item %]
[%~ SET biblio = checkout.item.biblio %]
[% totalValue = item.price + totalValue %]
[% totalSavings = item.price + totalSavings %]
<p>[% biblio.title %]<br />
Call number: [% item.itemcallnumber %]<br />
Due date: [% checkout.date_due | $KohaDates %]</p>
[% END %]
 
[% IF overdues.count %]
<h4>Overdues</h4>
[% FOREACH overdue IN overdues %]
[%~ SET item = overdue.item %]
[%~ SET biblio = overdue.item.biblio %]
[% totalValue = item.price + totalValue %]
[% totalSavings = item.price + totalSavings %]
<p>[% biblio.title %]<br />
Call number: [% item.itemcallnumber %]<br />
Due date: [% overdue.date_due | $KohaDates %]</p>
[% END %]
[% END %]

[% FOREACH old_checkout IN borrower.old_checkouts %]
  [% IF old_checkout.item %]
    [% old_checkout.item.price %]
[% totalSavings = old_checkout.item.price + totalSavings %]
  [% END %]
[% END %]
 
<p>You saved [% totalValue | $Price %] by using the library today!</p>
<p>Since you've been a member of the library, you saved [% totalSavings | $Price %]!</p>
 
<hr>
 
<h4 style="text-align: center; font-style:italic;">News</h4>
<news>
<div class="newsitem">
<h5 style="margin-bottom: 1px; margin-top: 1px"><b><<opac_news.title>></b></h5>
<p style="margin-bottom: 1px; margin-top: 1px"><<opac_news.content>></p>
<p class="newsfooter" style="font-size: 8pt; font-style:italic; margin-bottom: 1px; margin-top: 1px">Published on <<opac_news.timestamp>></p>
<hr />
</div>
</news>
Comment 6 Caroline Cyr La Rose 2020-10-09 20:52:22 UTC
Oh yeah, we should remove the random price to use in production

[%- USE KohaDates -%]
[%- USE Price -%]
[% totalValue = 0 %]
[% totalSavings = 0 %]
<h3><<branches.branchname>></h3>
Checked out by <<borrowers.firstname>> <<borrowers.surname>> <br />
(<<borrowers.cardnumber>>) <br />
 
<<today>><br />
 
<h4>Borrowed today</h4>
[% FOREACH checkout IN checkouts %]
[%~ SET item = checkout.item %]
[%~ SET biblio = checkout.item.biblio %]
[% totalValue = item.price + totalValue %]
[% totalSavings = item.price + totalSavings %]
<p>[% biblio.title %]<br />
Call number: [% item.itemcallnumber %]<br />
Due date: [% checkout.date_due | $KohaDates %]</p>
[% END %]
 
[% IF overdues.count %]
<h4>Overdues</h4>
[% FOREACH overdue IN overdues %]
[%~ SET item = overdue.item %]
[%~ SET biblio = overdue.item.biblio %]
[% totalValue = item.price + totalValue %]
[% totalSavings = item.price + totalSavings %]
<p>[% biblio.title %]<br />
Call number: [% item.itemcallnumber %]<br />
Due date: [% overdue.date_due | $KohaDates %]</p>
[% END %]
[% END %]

[% FOREACH old_checkout IN borrower.old_checkouts %]
  [% IF old_checkout.item %]
[% totalSavings = old_checkout.item.price + totalSavings %]
  [% END %]
[% END %]
 
<p>You saved [% totalValue | $Price %] by using the library today!</p>
<p>Since you've been a member of the library, you saved [% totalSavings | $Price %]!</p>
 
<hr>
 
<h4 style="text-align: center; font-style:italic;">News</h4>
<news>
<div class="newsitem">
<h5 style="margin-bottom: 1px; margin-top: 1px"><b><<opac_news.title>></b></h5>
<p style="margin-bottom: 1px; margin-top: 1px"><<opac_news.content>></p>
<p class="newsfooter" style="font-size: 8pt; font-style:italic; margin-bottom: 1px; margin-top: 1px">Published on <<opac_news.timestamp>></p>
<hr />
</div>
</news>
Comment 7 Martin Renvoize 2020-10-10 07:03:51 UTC
Really glad it all works out :)
Comment 8 Katrin Fischer 2020-10-15 22:09:04 UTC
Created attachment 111804 [details] [review]
Bug 26648: Prevent explosion in notices on missing item

The item accessor in Koha::Old::Checkouts could explode given a checkout
where the item had been deleted.

To test
1/ Edit the issueslip notice to output some item details from the
borrowers old checkouts

[% FOREACH old_checkout IN borrower.old_checkouts %]
  [% IF old_checkout.item %]
    [% old_checkout.item.price %]
  [% END %]
[% END %]

2/ Delete the item associated with an old checkout for a test patron
3/ Print the issueslip for the patron
4/ Note a server side error is triggered
5/ Apply the patch
6/ Print the slip again and note the price details for old checkouts are
now displayed
7/ Signoff

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 9 Jonathan Druart 2020-10-25 22:36:04 UTC
Please provide a test for this change.
Comment 10 Lucas Gass 2022-10-03 18:28:06 UTC
Created attachment 141287 [details] [review]
Bug 26648: Add tests
Comment 11 Martin Renvoize 2022-10-05 09:47:02 UTC
Created attachment 141333 [details] [review]
Bug 26648: Prevent explosion in notices on missing item

The item accessor in Koha::Old::Checkouts could explode given a checkout
where the item had been deleted.

To test
1/ Edit the issueslip notice to output some item details from the
borrowers old checkouts

[% FOREACH old_checkout IN borrower.old_checkouts %]
  [% IF old_checkout.item %]
    [% old_checkout.item.price %]
  [% END %]
[% END %]

2/ Delete the item associated with an old checkout for a test patron
3/ Print the issueslip for the patron
4/ Note a server side error is triggered
5/ Apply the patch
6/ Print the slip again and note the price details for old checkouts are
now displayed
7/ Signoff

Signed-off-by: Caroline Cyr La Rose <caroline.cyr-la-rose@inlibro.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 12 Martin Renvoize 2022-10-05 09:47:06 UTC
Created attachment 141334 [details] [review]
Bug 26648: Add tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 13 Martin Renvoize 2022-10-05 09:47:33 UTC
Tests added and passing.. Passing QA
Comment 14 PTFS Europe Sandboxes 2022-10-05 14:14:12 UTC
Created attachment 141382 [details] [review]
Bug 26648: Add tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Cab Vinton <director@plaistowlibrary.com>
Comment 15 Tomás Cohen Arazi 2022-10-12 17:59:50 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 16 Lucas Gass 2022-12-05 18:46:05 UTC
Backported to 22.05.x for upcoming 22.05.08
Comment 17 Arthur Suzuki 2022-12-14 15:19:54 UTC
conflicts when trying to apply the tests (file t/db_dependent/Koha/Old/Checkout is missing on my branch).
Won't backport unless asked for