Summary: | Prevent internal server error if item attached to old checkout has been removed | ||
---|---|---|---|
Product: | Koha | Reporter: | Martin Renvoize (ashimema) <martin.renvoize> |
Component: | Architecture, internals, and plumbing | Assignee: | Martin Renvoize (ashimema) <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 | ||
GIT URL: | 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
|
Circulation function: | |
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 (ashimema)
2020-10-09 15:12:18 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 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 |