Bug 26648 - Prevent internal server error if item attached to old checkout has been removed
Summary: Prevent internal server error if item attached to old checkout has been removed
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 31051
  Show dependency treegraph
 
Reported: 2020-10-09 15:12 UTC by Martin Renvoize
Modified: 2023-12-28 20:43 UTC (History)
6 users (show)

See Also:
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


Attachments
Bug 26648: Prevent explosion on missin item (1.24 KB, patch)
2020-10-09 15:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26648: Prevent explosion in notices on missing item (1.32 KB, patch)
2020-10-09 20:48 UTC, Caroline Cyr La Rose
Details | Diff | Splinter Review
Bug 26648: Prevent explosion in notices on missing item (1.38 KB, patch)
2020-10-15 22:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 26648: Add tests (1.33 KB, patch)
2022-10-03 18:28 UTC, Lucas Gass
Details | Diff | Splinter Review
Bug 26648: Prevent explosion in notices on missing item (1.44 KB, patch)
2022-10-05 09:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26648: Add tests (1.39 KB, patch)
2022-10-05 09:47 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 26648: Add tests (1.45 KB, patch)
2022-10-05 14:14 UTC, PTFS Europe Sandboxes
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
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