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
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
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.
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>
YES! It works!! I changed the title of the commit to correct a typo and make it more specific. I hope that's ok.
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>
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>
Really glad it all works out :)
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>
Please provide a test for this change.
Created attachment 141287 [details] [review] Bug 26648: Add tests
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>
Created attachment 141334 [details] [review] Bug 26648: Add tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Tests added and passing.. Passing QA
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>
Pushed to master for 22.11. Nice work everyone, thanks!
Backported to 22.05.x for upcoming 22.05.08
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