Description
Lucas Gass (lukeg)
2024-05-15 15:22:20 UTC
Created attachment 166834 [details] [review] Bug 36868: Add system preference AutoDeleteFromCartWhenHoldPlaced Created attachment 166835 [details] [review] Bug 36868: Logic for staff interface Created attachment 167558 [details] [review] Bug 36868: Logic for staff interface Created attachment 167695 [details] [review] Bug 36868: Add logic for OPAC Created attachment 167699 [details] [review] Bug 36868: Add system preference AutoDeleteFromCartWhenHoldPlaced Created attachment 167700 [details] [review] Bug 36868: Logic for staff interface Created attachment 167701 [details] [review] Bug 36868: Add logic for OPAC 1. APPLY PATCH, updatedatabase, restart_all 2. Search for the new AutoDeleteFromCartWhenHoldPlaced system preference 3. Turn it on for both staff and OPAC 4. From the staff interface put some items in your cart. 5. Place a hold on all of them. 6. Once the hold(s) have been successfully done those items should no longer be in your cart. 7. Put more items into your staff cart, place a hold on some of them but not all. 8. Only the items you have placed a hold on should be removed. 9. Go to the OPAC and put some items in your cart. 10. Place a hold on all of them, those items should now be deleted from the cart. 11. Place more items into your OPAC cart, place a hold on some of them but not all. 12. Only the items you have placed a hold on should be removed. 13. Switch the AutoDeleteFromCartWhenHoldPlaced to only be for 'staff'. 14. Try steps 4-8 again. 15. Switch the AutoDeleteFromCartWhenHoldPlaced to only be for 'opac'. 16. Try steps 9 -12 again. After adding items to cart, from the cart if I select all and place a hold, after completion, it only removes one of the items from the cart and the others stay. Placing the holds one by one worked though. Also, qa script didn't like the ordering placement of the sys. pref in installer/data/mysql/mandatory/sysprefs.sql Created attachment 167851 [details] [review] Bug 36868: (follow-up) Fix staff interface code Created attachment 167852 [details] [review] Bug 36868: Add system preference AutoDeleteFromCartWhenHoldPlaced Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 167853 [details] [review] Bug 36868: Logic for staff interface Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 167854 [details] [review] Bug 36868: Add logic for OPAC 1. APPLY PATCH, updatedatabase, restart_all 2. Search for the new AutoDeleteFromCartWhenHoldPlaced system preference 3. Turn it on for both staff and OPAC 4. From the staff interface put some items in your cart. 5. Place a hold on all of them. 6. Once the hold(s) have been successfully done those items should no longer be in your cart. 7. Put more items into your staff cart, place a hold on some of them but not all. 8. Only the items you have placed a hold on should be removed. 9. Go to the OPAC and put some items in your cart. 10. Place a hold on all of them, those items should now be deleted from the cart. 11. Place more items into your OPAC cart, place a hold on some of them but not all. 12. Only the items you have placed a hold on should be removed. 13. Switch the AutoDeleteFromCartWhenHoldPlaced to only be for 'staff'. 14. Try steps 4-8 again. 15. Switch the AutoDeleteFromCartWhenHoldPlaced to only be for 'opac'. 16. Try steps 9 -12 again. Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 167855 [details] [review] Bug 36868: (follow-up) Fix staff interface code Signed-off-by: Sam Lau <samalau@gmail.com> Why not a checkbox instead of a new syspref? Seems more flexible. (In reply to Jonathan Druart from comment #14) > Why not a checkbox instead of a new syspref? > > Seems more flexible. I like any solution that doesn't require a syspref. Can you explain what you mean? A checkbox where? (In reply to Lucas Gass (lukeg) from comment #15) > (In reply to Jonathan Druart from comment #14) > > Why not a checkbox instead of a new syspref? > > > > Seems more flexible. > > I like any solution that doesn't require a syspref. Can you explain what you > mean? A checkbox where? On the "Place holds" page. If at least one record is in the cart we could have a "Remove from cart" checkbox that will behave like what you have done with the pref if checkbed. Created attachment 177760 [details] [review] Bug 36868: Add system preference AutoDeleteFromCartWhenHoldPlaced Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 177761 [details] [review] Bug 36868: Logic for staff interface Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 177762 [details] [review] Bug 36868: Add logic for OPAC 1. APPLY PATCH, updatedatabase, restart_all 2. Search for the new AutoDeleteFromCartWhenHoldPlaced system preference 3. Turn it on for both staff and OPAC 4. From the staff interface put some items in your cart. 5. Place a hold on all of them. 6. Once the hold(s) have been successfully done those items should no longer be in your cart. 7. Put more items into your staff cart, place a hold on some of them but not all. 8. Only the items you have placed a hold on should be removed. 9. Go to the OPAC and put some items in your cart. 10. Place a hold on all of them, those items should now be deleted from the cart. 11. Place more items into your OPAC cart, place a hold on some of them but not all. 12. Only the items you have placed a hold on should be removed. 13. Switch the AutoDeleteFromCartWhenHoldPlaced to only be for 'staff'. 14. Try steps 4-8 again. 15. Switch the AutoDeleteFromCartWhenHoldPlaced to only be for 'opac'. 16. Try steps 9 -12 again. Signed-off-by: Sam Lau <samalau@gmail.com> Created attachment 177763 [details] [review] Bug 36868: (follow-up) Fix staff interface code Signed-off-by: Sam Lau <samalau@gmail.com> Comment on attachment 177760 [details] [review] Bug 36868: Add system preference AutoDeleteFromCartWhenHoldPlaced Review of attachment 177760 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref @@ +763,5 @@ > - pref: HoldRatioDefault > class: integer > - . > + - > + - Automatically delete items from cart when placing on hold on the Phrasing looks weird: «when placing on hold on the OPAC». Typo or maybe it's just an uncommon phrasing? Created attachment 178640 [details] [review] Bug 36868: (QA follow-up): fix exec permission The new syspref needs to be added to Hea (C4/UsageStats.pm) https://wiki.koha-community.org/wiki/Database_updates#Additional_hints ------- QA script: They looks either false positives or have their enforcement suspended (tt_valid) so nothing to worry here. FAIL installer/data/mysql/mandatory/sysprefs.sql FAIL semicolon simicolon found instead of comma at line 877 FAIL sysprefs_order Not blocker: Sysprefs hidelostitems is bad placed (see bug 10610) FAIL koha-tmpl/intranet-tmpl/prog/en/modules/reserve/request.tt FAIL tt_valid lines 388, 937, 1096 FAIL koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt FAIL tt_valid lines 368 ------ I'll continue with testing asap. Created attachment 178700 [details] [review] Bug 36868: (QA follow-up): fix typo in DBRev and DBRev skeleton Hijack this follow-up also fix the source (skeleton) which lead this typo to have 40 occurences! Created attachment 178701 [details] [review] Bug 36868: (QA follow-up): fix typo in DBRev and DBRev skeleton Hijacking this follow-up to also fix the source (skeleton) which lead this typo to have 40 occurences! 1. Apply and update DB 2. AutoDeleteFromCartWhenHoldPlaced = both or OPAC only 3. Go to OPAC and on search results add 3 records to cart 4. One one result, place hold 5. "item has been removed from cart", count = 2 6. From the current "Your summary" page, cancel the hold 7. Now we want to go back to the search results to test more case. So we will use the back button a few times. 8. Go back once 9. "item has been removed from cart", count = 1 Not great but doesn't really matter if it's just there. 10. Go back more times to get back to the search results 11. cart count = 3 Not really a problem, it's just the old value because we are going back. 12. Click on any link to go to any page 13. cart count = 1 Oh no it was persistent! >_< 14. click on cart 15. 2 items Confirmation that cart count is now desynced >_< (better than removing another of the remaining records) Instead of cancelling the hold, I could have went to any page and hit back, it would have likely(speculation) done the same. I didn't test on staff interface. Comment on attachment 177762 [details] [review] Bug 36868: Add logic for OPAC Review of attachment 177762 [details] [review]: ----------------------------------------------------------------- ::: koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt @@ +1077,5 @@ > [% INCLUDE 'datatables.inc' %] > <script> > + [% IF successful_holds && Koha.Preference('AutoDeleteFromCartWhenHoldPlaced').grep('opac').size %] > + [% FOREACH success IN successful_holds.split('\|') %] > + delSingleRecord('[% success | html %]'); Having a function call dropped in a <script> of page it likely the cause. Such technique need the call to be idempotent. Like making the count always be calculated from the cart content instead a separate variable? Even without going as far as such of a refactor, it looks like a bug that passing an already removed record would trigger the correct message but still decrement the count. Or somehow having the call only done once. (self deleting <script> tag? 😆) |