Description
Aleisha Amohia
2022-06-27 04:21:02 UTC
Created attachment 136564 [details] [review] Bug 31051: [WIP] Add OPACShowSavings preference Created attachment 136664 [details] [review] Bug 31051: Show patron savings on the OPAC This new feature shows a patron how much they have saved by using the library rather than purchasing items. Savings are calculated based on item replacement prices. The system preference allows you to choose where to display the savings - the user page, the summary box on the OPAC homepage, or the checkout history page. To test: 1. Update database and restart services 2. Confirm the new OPACShowSavings system preference is found in the OPAC tab of Administration -> global system preferences. There should be no options selected. 3. Find a patron with a checkout history, or check out a few items to a patron. 4. Test with different values of OPACShowSavings and confirm that savings are shown in the expected places. Sponsored-by: Horowhenua Libraries Trust This didn't work at first when using my own test data. I got this error: Can't call method "replacementprice" on an undefined value at /kohadevbox/koha/Koha/Patron.pm line 2206 It seems to come from there being lines in old_issues where the itemnumber is null. I'm not sure if that's normal or not. The only other issue I have is that I don't think "do not display anywhere on the OPAC" should be one of the preference options. It's contradictory to be able to select that *and* the other options. Users can select no options if they want the information not to appear. Sorry the first patch is a mistake, I seem to have missed attaching the correct one. Will fix Created attachment 136698 [details] [review] Bug 31051: Add OPACShowSavings system preference Created attachment 136699 [details] [review] Bug 31051: Show patron savings on the OPAC This new feature shows a patron how much they have saved by using the library rather than purchasing items. Savings are calculated based on item replacement prices. The system preference allows you to choose where to display the savings - the user page, the summary box on the OPAC homepage, or the checkout history page. To test: 1. Update database and restart services 2. Confirm the new OPACShowSavings system preference is found in the OPAC tab of Administration -> global system preferences. There should be no options selected. 3. Find a patron with a checkout history, or check out a few items to a patron. 4. Test with different values of OPACShowSavings and confirm that savings are shown in the expected places. Sponsored-by: Horowhenua Libraries Trust testing note: add an item replacement cost (952$ v) on checkout items it works well Created attachment 136770 [details] [review] Bug 31051: Show patron savings on the OPAC This new feature shows a patron how much they have saved by using the library rather than purchasing items. Savings are calculated based on item replacement prices. The system preference allows you to choose where to display the savings - the user page, the summary box on the OPAC homepage, or the checkout history page. To test: 1. Update database and restart services 2. Confirm the new OPACShowSavings system preference is found in the OPAC tab of Administration -> global system preferences. There should be no options selected. 3. Find a patron with a checkout history, or check out a few items to a patron. 4. Test with different values of OPACShowSavings and confirm that savings are shown in the expected places. Sponsored-by: Horowhenua Libraries Trust Signed-off-by: Marie-Luce <marie-luce.laflamme@inlibro.com> Tested with a UNIMARC sandbox : items replacement prices = 995$g in UNIMARC. It works well too. Created attachment 136772 [details] [review] Bug 31051: Add OPACShowSavings system preference Signed-off-by: Axelle <axelle.clarisse@univ-amu.fr> Created attachment 136773 [details] [review] Bug 31051: Show patron savings on the OPAC This new feature shows a patron how much they have saved by using the library rather than purchasing items. Savings are calculated based on item replacement prices. The system preference allows you to choose where to display the savings - the user page, the summary box on the OPAC homepage, or the checkout history page. To test: 1. Update database and restart services 2. Confirm the new OPACShowSavings system preference is found in the OPAC tab of Administration -> global system preferences. There should be no options selected. 3. Find a patron with a checkout history, or check out a few items to a patron. 4. Test with different values of OPACShowSavings and confirm that savings are shown in the expected places. Sponsored-by: Horowhenua Libraries Trust Signed-off-by: Axelle <axelle.clarisse@univ-amu.fr> Created attachment 136779 [details] [review] Bug 31051: Add OPACShowSavings system preference Signed-off-by: Marie-Luce <marie-luce.laflamme@inlibro.com> Signed-off-by: Axelle <axelle.clarisse@univ-amu.fr> Created attachment 136781 [details] [review] Bug 31051: Show patron savings on the OPAC This new feature shows a patron how much they have saved by using the library rather than purchasing items. Savings are calculated based on item replacement prices. The system preference allows you to choose where to display the savings - the user page, the summary box on the OPAC homepage, or the checkout history page. To test: 1. Update database and restart services 2. Confirm the new OPACShowSavings system preference is found in the OPAC tab of Administration -> global system preferences. There should be no options selected. 3. Find a patron with a checkout history, or check out a few items to a patron. 4. Test with different values of OPACShowSavings and confirm that savings are shown in the expected places. Sponsored-by: Horowhenua Libraries Trust Signed-off-by: Marie-Luce <marie-luce.laflamme@inlibro.com> Signed-off-by: Axelle <axelle.clarisse@univ-amu.fr> Marie-Luce and Axelle tested at the same time and one signoff line was lost. I just reattached the patches with both signoff lines. Looking at this for QA. I have a question : Congratulations, you have saved a total of [% savings | $Price with_symbol => 1 %] by using the [% IF ( LibraryNameTitle ) %][% LibraryNameTitle | html %][% ELSE %]Koha online[% END %] catalog. When LibraryNameTitle is not defined the text will say "you have saved a total of X by using the Koha online catalog". Looks not good, the savings are caused by the checkouts so in the "real life" catalog, not the online. Also often the content of LibraryNameTitle makes a weird text with catalog appended. In my opinion we should use a more simple : Congratulations, you have saved a total of [% savings | $Price with_symbol => 1 %] by using the library. Created attachment 139100 [details] [review] Bug 31051: Add OPACShowSavings system preference Signed-off-by: Marie-Luce <marie-luce.laflamme@inlibro.com> Signed-off-by: Axelle <axelle.clarisse@univ-amu.fr> Created attachment 139101 [details] [review] Bug 31051: Show patron savings on the OPAC This new feature shows a patron how much they have saved by using the library rather than purchasing items. Savings are calculated based on item replacement prices. The system preference allows you to choose where to display the savings - the user page, the summary box on the OPAC homepage, or the checkout history page. To test: 1. Update database and restart services 2. Confirm the new OPACShowSavings system preference is found in the OPAC tab of Administration -> global system preferences. There should be no options selected. 3. Find a patron with a checkout history, or check out a few items to a patron. 4. Test with different values of OPACShowSavings and confirm that savings are shown in the expected places. Sponsored-by: Horowhenua Libraries Trust Signed-off-by: Marie-Luce <marie-luce.laflamme@inlibro.com> Signed-off-by: Axelle <axelle.clarisse@univ-amu.fr> Created attachment 139102 [details] [review] Bug 31051: (follow-up) Simplify savings wording for OPAC Great ;) Created attachment 139408 [details] [review] Bug 31051: Add OPACShowSavings system preference Signed-off-by: Marie-Luce <marie-luce.laflamme@inlibro.com> Signed-off-by: Axelle <axelle.clarisse@univ-amu.fr> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 139409 [details] [review] Bug 31051: Show patron savings on the OPAC This new feature shows a patron how much they have saved by using the library rather than purchasing items. Savings are calculated based on item replacement prices. The system preference allows you to choose where to display the savings - the user page, the summary box on the OPAC homepage, or the checkout history page. To test: 1. Update database and restart services 2. Confirm the new OPACShowSavings system preference is found in the OPAC tab of Administration -> global system preferences. There should be no options selected. 3. Find a patron with a checkout history, or check out a few items to a patron. 4. Test with different values of OPACShowSavings and confirm that savings are shown in the expected places. Sponsored-by: Horowhenua Libraries Trust Signed-off-by: Marie-Luce <marie-luce.laflamme@inlibro.com> Signed-off-by: Axelle <axelle.clarisse@univ-amu.fr> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 139410 [details] [review] Bug 31051: (follow-up) Simplify savings wording for OPAC Signed-off-by: Owen Leonard <oleonard@myacpl.org> Created attachment 139911 [details] [review] Bug 31051: Add OPACShowSavings system preference Signed-off-by: Marie-Luce <marie-luce.laflamme@inlibro.com> Signed-off-by: Axelle <axelle.clarisse@univ-amu.fr> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 139912 [details] [review] Bug 31051: Show patron savings on the OPAC This new feature shows a patron how much they have saved by using the library rather than purchasing items. Savings are calculated based on item replacement prices. The system preference allows you to choose where to display the savings - the user page, the summary box on the OPAC homepage, or the checkout history page. To test: 1. Update database and restart services 2. Confirm the new OPACShowSavings system preference is found in the OPAC tab of Administration -> global system preferences. There should be no options selected. 3. Find a patron with a checkout history, or check out a few items to a patron. 4. Test with different values of OPACShowSavings and confirm that savings are shown in the expected places. Sponsored-by: Horowhenua Libraries Trust Signed-off-by: Marie-Luce <marie-luce.laflamme@inlibro.com> Signed-off-by: Axelle <axelle.clarisse@univ-amu.fr> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 139913 [details] [review] Bug 31051: (follow-up) Simplify savings wording for OPAC Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 139914 [details] [review] Bug 31051: (QA follow-up) Simplify savings wording for reading history Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> This works nicely and I quite like the idea! Only one real blocker and some notes/suggestions: 1) +=head3 get_savings (blocker) We need some unit tests here. 2) issues + old_issues (not blocker) A lot of libraries nowadays choose to anonymize the checkout history rather fast. So as we look at issues and old_issues here, the amount saved will go up and down quite a bit. I wonder if it could make sense to add a note to the pref so libraries have this in mind when activating the feature. 3) Wording (follow-up patch added) I have provided another follow-up for the reading history page following Owen's suggestion as this will also ease translation (combined constructs like x catalog are always a little hard) 4) id (not blocker) It could be nice to have a specific id on the message in the summary page and reading history in case libraries want to do some formatting or rephrasing. Created attachment 139937 [details] [review] Bug 31051: (follow-up) Tests for get_savings and more - Added tests in t/db_dependent/Koha/Patron.t - Added wording to OPACShowSavings syspref about anonymised checkout history - Added IDs to the savings messages on the OPAC Sponsored-by: Horowhenua Libraries Trust I've tried adding something similar to this in an ISSUESLIP and I always run into the problem where things explode when delete an item that was in the patrons old issues. The same thing seems to happen here. If I delete something from a patrons old issues I get: Can't call method "replacementprice" on an undefined value at /kohadevbox/koha/Koha/Patron.pm line 2218 Perhaps if we could make Bug 26648 move it would solve the problem here as well. Aleisha, I think if we make this one dependent on Bug 26648 we can move it back to NSO. Created attachment 143384 [details] [review] Bug 31051: (follow-up) Tests for get_savings and more - Added tests in t/db_dependent/Koha/Patron.t - Added wording to OPACShowSavings syspref about anonymised checkout history - Added IDs to the savings messages on the OPAC - Prevent explosion if a checked out item has been deleted Sponsored-by: Horowhenua Libraries Trust (In reply to Lucas Gass from comment #31) > Aleisha, > > I think if we make this one dependent on Bug 26648 we can move it back to > NSO. Addressed in latest patch, ready for testing. Created attachment 144082 [details] [review] Bug 31051: (follow-up) Tests for get_savings and more - Added tests in t/db_dependent/Koha/Patron.t - Added wording to OPACShowSavings syspref about anonymised checkout history - Added IDs to the savings messages on the OPAC - Prevent explosion if a checked out item has been deleted Sponsored-by: Horowhenua Libraries Trust Signed-off-by: Hammat Wele <hammat.wele@inlibro.com> Created attachment 144759 [details] [review] Bug 31051: Add OPACShowSavings system preference Signed-off-by: Marie-Luce <marie-luce.laflamme@inlibro.com> Signed-off-by: Axelle <axelle.clarisse@univ-amu.fr> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Created attachment 144760 [details] [review] Bug 31051: Show patron savings on the OPAC This new feature shows a patron how much they have saved by using the library rather than purchasing items. Savings are calculated based on item replacement prices. The system preference allows you to choose where to display the savings - the user page, the summary box on the OPAC homepage, or the checkout history page. To test: 1. Update database and restart services 2. Confirm the new OPACShowSavings system preference is found in the OPAC tab of Administration -> global system preferences. There should be no options selected. 3. Find a patron with a checkout history, or check out a few items to a patron. 4. Test with different values of OPACShowSavings and confirm that savings are shown in the expected places. Sponsored-by: Horowhenua Libraries Trust Signed-off-by: Marie-Luce <marie-luce.laflamme@inlibro.com> Signed-off-by: Axelle <axelle.clarisse@univ-amu.fr> Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Created attachment 144761 [details] [review] Bug 31051: (follow-up) Simplify savings wording for OPAC Signed-off-by: Owen Leonard <oleonard@myacpl.org> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Created attachment 144762 [details] [review] Bug 31051: (QA follow-up) Simplify savings wording for reading history Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Created attachment 144763 [details] [review] Bug 31051: (follow-up) Tests for get_savings and more - Added tests in t/db_dependent/Koha/Patron.t - Added wording to OPACShowSavings syspref about anonymised checkout history - Added IDs to the savings messages on the OPAC - Prevent explosion if a checked out item has been deleted Sponsored-by: Horowhenua Libraries Trust Signed-off-by: Hammat Wele <hammat.wele@inlibro.com> Signed-off-by: Fridolin Somers <fridolin.somers@biblibre.com> Fixed QA failure FAIL installer/data/mysql/atomicupdate/bug_31051_-_add_OPACShowSavings_syspref.pl FAIL file permissions File must have the exec flag atomic file permission set to a+x I generally like the feature. I have two QA remarks to make: 1. I expected to see some 'DISTINCT' in the queries, or a 'uniq' in the code. Because I'm not sure how the saving should be counted (i.e. if the book is recalled and I need to borrow it again, then I didn't save two times the replacement prince, but one). I guess this is subject to debate so I would mark 'In discussion' until this is settled. 2. MySQL (and DBIC) has ways to calculate this SUM in a single DB query. So we shouldn't need to look trough many checkouts and items summing things in checkouts+old_checkouts DB queries. This is a blocker in my opinion. Happy to guide/help if needed. Created attachment 146431 [details] [review] Bug 31051: (QA follow-up) Use a single query and avoid duplicated sums This patch makes the `get_savings` method use a single query to calculate the sum of the replacement prices. This way we save one query per item and just rely on the DB features. It has a side effect: we are not summing items twice. Added tests for the 'itenumber is null' pathological but common use case (specially in old_issues), as mentioned by Lucas. Handling for this is added (grep filtering out undefined ones) and also in the return, for the empty case, with // 0. To test: 1. Apply this patch 2. Run: qa -c 6 --run-tests => SUCCESS: All good 3. Sign off :-D Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 146432 [details] [review] Bug 31051: (QA follow-up) Fix misplaced syspref Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> (In reply to Tomás Cohen Arazi from comment #41) > 2. MySQL (and DBIC) has ways to calculate this SUM in a single DB query. So > we shouldn't need to look trough many checkouts and items summing things in > checkouts+old_checkouts DB queries. This is a blocker in my opinion. Happy > to guide/help if needed. I submitted a candidate patch that makes things a bit more optimal. We could still make some joins to have a single query, but this is much better on its own. > 1. I expected to see some 'DISTINCT' in the queries, or a 'uniq' in the > code. Because I'm not sure how the saving should be counted (i.e. if the > book is recalled and I need to borrow it again, then I didn't save two times > the replacement prince, but one). I guess this is subject to debate so I > would mark 'In discussion' until this is settled. My implementation does what I think we should do. So letting the bug 'In discussion' until this is settled. Otherwise it would be ready. I tested the follow-up patches and they work well. Happy for this to be pushed. Pushed to master for 23.05. Nice work everyone, thanks! Jenkins is failing, but I don't recreate locally. Koha_Master/2394 koha_1 | # Failed test 'Savings correctly calculated from current issues' koha_1 | # at t/db_dependent/Koha/Patron.t line 1380. koha_1 | # got: '34583.70' koha_1 | # expected: '34583.7' koha_1 | koha_1 | # Failed test 'Savings correctly calculated from current and old issues' koha_1 | # at t/db_dependent/Koha/Patron.t line 1385. koha_1 | # got: '34583.70' koha_1 | # expected: '34583.7' koha_1 | # Looks like you failed 2 tests of 4. koha_1 | koha_1 | # Failed test 'get_savings tests' koha_1 | # at t/db_dependent/Koha/Patron.t line 1388. koha_1 | # Looks like you failed 1 test of 20. koha_1 | [00:07:57] t/db_dependent/Koha/Patron.t (In reply to Jonathan Druart from comment #47) > Jenkins is failing, but I don't recreate locally. Because it ends with '0'. Created attachment 147189 [details] [review] Bug 31051: Fix test When savings ends with a "0" koha_1 | # Failed test 'Savings correctly calculated from current issues' koha_1 | # at t/db_dependent/Koha/Patron.t line 1380. koha_1 | # got: '34583.70' koha_1 | # expected: '34583.7' koha_1 | koha_1 | # Failed test 'Savings correctly calculated from current and old issues' koha_1 | # at t/db_dependent/Koha/Patron.t line 1385. koha_1 | # got: '34583.70' koha_1 | # expected: '34583.7' koha_1 | # Looks like you failed 2 tests of 4. koha_1 | koha_1 | # Failed test 'get_savings tests' koha_1 | # at t/db_dependent/Koha/Patron.t line 1388. koha_1 | # Looks like you failed 1 test of 20. koha_1 | [00:07:57] t/db_dependent/Koha/Patron.t But, wait. Aren't we expecting "10" (5+5) here? Created attachment 147190 [details] [review] Bug 31051: Generate items correctly in test There was a misplaced bracket in build_object. To generate item you should need build_sample_item anyway. (In reply to Jonathan Druart from comment #47) > Jenkins is failing, but I don't recreate locally. > > Koha_Master/2394 > > koha_1 | # Failed test 'Savings correctly calculated from > current issues' > koha_1 | # at t/db_dependent/Koha/Patron.t line 1380. > koha_1 | # got: '34583.70' > koha_1 | # expected: '34583.7' > koha_1 | > koha_1 | # Failed test 'Savings correctly calculated from > current and old issues' > koha_1 | # at t/db_dependent/Koha/Patron.t line 1385. > koha_1 | # got: '34583.70' > koha_1 | # expected: '34583.7' > koha_1 | # Looks like you failed 2 tests of 4. > koha_1 | > koha_1 | # Failed test 'get_savings tests' > koha_1 | # at t/db_dependent/Koha/Patron.t line 1388. > koha_1 | # Looks like you failed 1 test of 20. > koha_1 | [00:07:57] t/db_dependent/Koha/Patron.t This is interesting, as it passed for me when I tweaked the tests. Anyway, thanks for the fixes, Jonathan! New feature, won't be backported to stable 22.11.x series Can the [% IF savings %] [% savings | $Price with_symbol => 1 %] be used in notices? It would be useful to replace https://wiki.koha-community.org/wiki/Notices_and_Slips_Library#Add_total_value_of_items_borrowed_to_date_on_issue_slip (In reply to Caroline Cyr La Rose from comment #54) > Can the [% IF savings %] [% savings | $Price with_symbol => 1 %] be used in > notices? It would be useful to replace > https://wiki.koha-community.org/wiki/ > Notices_and_Slips_Library#Add_total_value_of_items_borrowed_to_date_on_issue_ > slip Hi Caroline, I tested in CHECKIN and this works: [% USE Price %] [% borrower.get_savings | $Price with_symbol => 1 %] |