Description
Aleisha Amohia
2016-02-09 20:02:48 UTC
Created attachment 47831 [details] [review] Bug 15775 - User should know when they are blocked from checkouts on OPAC When a user has been blocked from checkouts in the staff client for having too many fines, they will now see a message about this in the OPAC To test: - Apply Bug 15374, then this patch - This patch will be easiest to test if you keep three tabs open - system preferences, a patron detail page, and OPAC summary page - Set some system preferences: - OPACFineNoRenewals = 2 - noissuescharge = 4 - OpacRenewalAllowed = Allow - Set up a patron with fines of $3 - Log in to the OPAC and verify the message from patch from Bug 15374, saying the user cannot renew books. Confirm link to accounts page works. - Set OpacRenewalAllowed = Don't allow - Refresh OPAC Summary and confirm the message disappears - Set OpacRenewalAllowed = Allow - Give the patron total fines of $5 (add $2) - Refresh OPAC Summary and verify message appears saying user is BLOCKED from checkouts. Confirm link to accounts page works - Set OpacRenewalAllowed = Don't allow - Refresh OPAC Summary and confirm message is still there Sponsored-by: Catalyst IT Created attachment 47833 [details] [review] Bug 15775 - User should know when they are blocked from checkouts on OPAC When a user has been blocked from checkouts in the staff client for having too many fines, they will now see a message about this in the OPAC To test: - Apply Bug 15374, then this patch - This patch will be easiest to test if you keep three tabs open - system preferences, a patron detail page, and OPAC summary page - Set some system preferences: - OPACFineNoRenewals = 2 - noissuescharge = 4 - OpacRenewalAllowed = Allow - Set up a patron with fines of $3 - Log in to the OPAC and verify the message from patch from Bug 15374, saying the user cannot renew books. Confirm link to accounts page works. - Set OpacRenewalAllowed = Don't allow - Refresh OPAC Summary and confirm the message disappears - Set OpacRenewalAllowed = Allow - Give the patron total fines of $5 (add $2) - Refresh OPAC Summary and verify message appears saying user is BLOCKED from checkouts. Confirm link to accounts page works - Set OpacRenewalAllowed = Don't allow - Refresh OPAC Summary and confirm message is still there Sponsored-by: Catalyst IT Hi Aleisha, If I apply this patch on top of Bug 15374, I get merge conflicts. Created attachment 47937 [details] [review] Bug 15775 - User should know when they are blocked from checkouts on OPAC When a user has been blocked from checkouts in the staff client for having too many fines, they will now see a message about this in the OPAC To test: - This patch will be easiest to test if you keep three tabs open - system preferences, a patron detail page, and OPAC summary page - Set some system preferences: - OPACFineNoRenewals = 2 - noissuescharge = 4 - OpacRenewalAllowed = Allow - Set up a patron with fines of $3 - Log in to the OPAC and verify the message from patch from Bug 15374, saying the user cannot renew books. Confirm link to accounts page works. - Set OpacRenewalAllowed = Don't allow - Refresh OPAC Summary and confirm the message disappears - Set OpacRenewalAllowed = Allow - Give the patron total fines of $5 (add $2) - Refresh OPAC Summary and verify message appears saying user is BLOCKED from checkouts. Confirm link to accounts page works - Set OpacRenewalAllowed = Don't allow - Refresh OPAC Summary and confirm message is still there Sponsored-by: Catalyst IT (In reply to Marc Véron from comment #3) > Hi Aleisha, > > If I apply this patch on top of Bug 15374, I get merge conflicts. Hi Marc, please try applying on it's own (not on top of Bug 15374). I used the patch from 15374 to write this patch but I realise now it probably doesn't need to be applied on top of the other one. Hi Aleisha, I gave up Bug 15374 because it would have conflicts on top of this Bug. It certainly will make it easier to go on with such enhancements. Plese feel free to re-use code from 15374 on top of your patch, if applicable. (In reply to Marc Véron from comment #6) > Hi Aleisha, > > I gave up Bug 15374 because it would have conflicts on top of this Bug. It > certainly will make it easier to go on with such enhancements. > > Plese feel free to re-use code from 15374 on top of your patch, if > applicable. Hi, thanks Marc! I think I actually wrote the patch for this on top of your patch from 15374 anyway so we kind of wrote this patch together :) I will recommit this patch with the commit message from 15374 and edit the bug report since we are now testing for all that translatability stuff too. Created attachment 48044 [details] [review] Bug 15775 - User should know when they are blocked from checkouts on OPAC + Translatability fixes for OPAC Summary page When a user has been blocked from checkouts in the staff client for having too many fines, they will now see a message about this in the OPAC To test: - This patch will be easiest to test if you keep three tabs open - system preferences, a patron detail page, and OPAC summary page - Set some system preferences: - OPACFineNoRenewals = 2 - noissuescharge = 4 - OpacRenewalAllowed = Allow - Set up a patron with fines of $3 - Log in to the OPAC and verify the message from patch from Bug 15374, saying the user cannot renew books. Confirm link to accounts page works. - Set OpacRenewalAllowed = Don't allow - Refresh OPAC Summary and confirm the message disappears - Set OpacRenewalAllowed = Allow - Give the patron total fines of $5 (add $2) - Refresh OPAC Summary and verify message appears saying user is BLOCKED from checkouts. Confirm link to accounts page works - Set OpacRenewalAllowed = Don't allow - Refresh OPAC Summary and confirm message is still there Translability tests: - Log in to OPAC - with blocked /debarred user (with and without comment and date) - with user who has to much fines - with user who has his card marked as lost - with user with an account that is about to expire - with user with expired account - with user with uncertain address information - with user who has got a circulation message (from checkout page) - with user who has a OPAC note - Verify that information is correct and displays nicely and that links in messages work - Carefully examine code - Search for regressions Sponsored-by: Catalyst IT Wanted to test, but patch does not apply: Auto-merging opac/opac-user.pl CONFLICT (content): Merge conflict in opac/opac-user.pl Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt Failed to merge in the changes. Created attachment 49434 [details] [review] Bug 15775 - User should know when they are blocked from checkouts on OPAC + Translatability fixes for OPAC Summary page EDIT: Fixing merge conflicts When a user has been blocked from checkouts in the staff client for having too many fines, they will now see a message about this in the OPAC To test: - This patch will be easiest to test if you keep three tabs open - system preferences, a patron detail page, and OPAC summary page - Set some system preferences: - OPACFineNoRenewals = 2 - noissuescharge = 4 - OpacRenewalAllowed = Allow - Set up a patron with fines of $3 - Log in to the OPAC and verify the message from patch from Bug 15374, saying the user cannot renew books. Confirm link to accounts page works. - Set OpacRenewalAllowed = Don't allow - Refresh OPAC Summary and confirm the message disappears - Set OpacRenewalAllowed = Allow - Give the patron total fines of $5 (add $2) - Refresh OPAC Summary and verify message appears saying user is BLOCKED from checkouts. Confirm link to accounts page works - Set OpacRenewalAllowed = Don't allow - Refresh OPAC Summary and confirm message is still there Translability tests: - Log in to OPAC - with blocked /debarred user (with and without comment and date) - with user who has to much fines - with user who has his card marked as lost - with user with an account that is about to expire - with user with expired account - with user with uncertain address information - with user who has got a circulation message (from checkout page) - with user who has a OPAC note - Verify that information is correct and displays nicely and that links in messages work - Carefully examine code - Search for regressions Sponsored-by: Catalyst IT Tests: ...are OK Translability tests: - Log in to OPAC (...) - with user who has got a circulation message (from checkout page) The circulation message I added on the checkout page displays a bullet only, but no text. (It displays properly without patch). Other translatability tests are OK. Regression happens here: https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=49434&action=diff#a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt_sec2 Created attachment 49473 [details] [review] Bug 15775 - User should know when they are blocked from checkouts on OPAC + Translatability fixes for OPAC Summary page EDIT: Fixing merge conflicts EDIT: Fix from Comment 11 / Comment 12 When a user has been blocked from checkouts in the staff client for having too many fines, they will now see a message about this in the OPAC To test: - This patch will be easiest to test if you keep three tabs open - system preferences, a patron detail page, and OPAC summary page - Set some system preferences: - OPACFineNoRenewals = 2 - noissuescharge = 4 - OpacRenewalAllowed = Allow - Set up a patron with fines of $3 - Log in to the OPAC and verify the message from patch from Bug 15374, saying the user cannot renew books. Confirm link to accounts page works. - Set OpacRenewalAllowed = Don't allow - Refresh OPAC Summary and confirm the message disappears - Set OpacRenewalAllowed = Allow - Give the patron total fines of $5 (add $2) - Refresh OPAC Summary and verify message appears saying user is BLOCKED from checkouts. Confirm link to accounts page works - Set OpacRenewalAllowed = Don't allow - Refresh OPAC Summary and confirm message is still there Translability tests: - Log in to OPAC - with blocked /debarred user (with and without comment and date) - with user who has to much fines - with user who has his card marked as lost - with user with an account that is about to expire - with user with expired account - with user with uncertain address information - with user who has got a circulation message (from checkout page) - with user who has a OPAC note - Verify that information is correct and displays nicely and that links in messages work - Carefully examine code - Search for regressions Sponsored-by: Catalyst IT Created attachment 49516 [details] [review] Bug 15775 - User should know when they are blocked from checkouts on OPAC + Translatability fixes for OPAC Summary page EDIT: Fixing merge conflicts EDIT: Fix from Comment 11 / Comment 12 When a user has been blocked from checkouts in the staff client for having too many fines, they will now see a message about this in the OPAC To test: - This patch will be easiest to test if you keep three tabs open - system preferences, a patron detail page, and OPAC summary page - Set some system preferences: - OPACFineNoRenewals = 2 - noissuescharge = 4 - OpacRenewalAllowed = Allow - Set up a patron with fines of $3 - Log in to the OPAC and verify the message from patch from Bug 15374, saying the user cannot renew books. Confirm link to accounts page works. - Set OpacRenewalAllowed = Don't allow - Refresh OPAC Summary and confirm the message disappears - Set OpacRenewalAllowed = Allow - Give the patron total fines of $5 (add $2) - Refresh OPAC Summary and verify message appears saying user is BLOCKED from checkouts. Confirm link to accounts page works - Set OpacRenewalAllowed = Don't allow - Refresh OPAC Summary and confirm message is still there Translability tests: - Log in to OPAC - with blocked /debarred user (with and without comment and date) - with user who has to much fines - with user who has his card marked as lost - with user with an account that is about to expire - with user with expired account - with user with uncertain address information - with user who has got a circulation message (from checkout page) - with user who has a OPAC note - Verify that information is correct and displays nicely and that links in messages work - Carefully examine code - Search for regressions Sponsored-by: Catalyst IT Followed test plan, works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Who is the author of this patch? The author commit is Marc, is it right? Comment on attachment 49516 [details] [review] Bug 15775 - User should know when they are blocked from checkouts on OPAC + Translatability fixes for OPAC Summary page Review of attachment 49516 [details] [review]: ----------------------------------------------------------------- I really like the use of the Price plugin for formatting - I'd love to see that used everywhere and improved to show the currency symbol in some cases. Found a small regression in testing (see code comments below): ::: opac/opac-user.pl @@ +126,5 @@ > +my $checkoutsblockedamount = C4::Context->preference( 'noissuescharge' ); > +$checkoutsblockedamount ||=0; > +if ( C4::Context->preference( 'noissuescharge' ) && $borr->{amountoutstanding} > $checkoutsblockedamount ) { > + $borr->{'flagged'} = 1; > + $canrenew = 0; I think this change is not correct. noissuecharge should have no effect on renewals, as we have OPACFineNoRenewals to do that. With noissuecharge 5.00, OPACFineNoRenewals 100 and a fine of 5.01 on my account I can now no longer renew items I can renew without the patch. If I set noissuecharge to 5.02 I can renew with the patch applied. @@ +128,5 @@ > +if ( C4::Context->preference( 'noissuescharge' ) && $borr->{amountoutstanding} > $checkoutsblockedamount ) { > + $borr->{'flagged'} = 1; > + $canrenew = 0; > + $template->param( > + checkouts_blocked_amount => sprintf ( '%.02f', $checkoutsblockedamount ), Why not format the amount in the template like in the other cases? (In reply to Jonathan Druart from comment #15) > Who is the author of this patch? > The author commit is Marc, is it right? This Bug 15775 is related to Bug 15374 I abandoned 15374 in favour of 15775, and sigend off 15775 See https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=15374#c35 No problem for me if authorship is changed to Aleisha. Created attachment 63710 [details] [review] Bug 15775: [FOLLOWUP] fixing merge conflicts and using Price plugin for formatting Hi, I tried applying the patch, but it doesn't seem to work. I get conflicts... Using index info to reconstruct a base tree... M koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt M opac/opac-user.pl Falling back to patching base and 3-way merge... Auto-merging opac/opac-user.pl CONFLICT (content): Merge conflict in opac/opac-user.pl Auto-merging koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt CONFLICT (content): Merge conflict in koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-user.tt Failed to merge in the changes. Patch failed at 0001 Bug 15775 - User should know when they are blocked from checkouts on OPAC + Translatability fixes for OPAC Summary page Created attachment 98540 [details] [review] Bug 15775: Show message on OPAC summary if checkouts blocked due to fines To test: 1) Set maxoutstanding syspref to 5 2) Create a manual invoice for a borrower for $6 3) Log in to the OPAC as this borrower 4) Go to opac-user.pl, your summary page 5) Confirm there is a warning box explaining that the borrower is blocked from checkouts and holds due to fines 6) Back on the staff side, pay some or all of the borrower's fines so they are no longer over the max outstanding amount 7) Refresh the summary page in the OPAC and confirm the warning is gone Sponsored-by: Catalyst IT This patch had gotten a little messy and looks like much of the content of the patch has now been dealt with in master, so I've written a new patch to deal simply with the bug title. Ready for testing. Created attachment 98545 [details] [review] Bug 15775: Show message on OPAC summary if checkouts blocked due to fines To test: 1) Set maxoutstanding syspref to 5 2) Create a manual invoice for a borrower for $6 3) Log in to the OPAC as this borrower 4) Go to opac-user.pl, your summary page 5) Confirm there is a warning box explaining that the borrower is blocked from checkouts and holds due to fines 6) Back on the staff side, pay some or all of the borrower's fines so they are no longer over the max outstanding amount 7) Refresh the summary page in the OPAC and confirm the warning is gone Sponsored-by: Catalyst IT Signed-off-by: David Nind <david@davidnind.com> We got 3 different prefs for OPAC renewal, checkouts and holds. This patch uses maxoutstanding = holds. Changed the wording of the warning slightly to reflect this. I could imagine doing warns for the other 2 prefs might be nice as well. Created attachment 99840 [details] [review] Bug 15775: Show message on OPAC summary if holds are blocked due to fines To test: 1) Set maxoutstanding syspref to 5 2) Create a manual invoice for a borrower for $6 3) Log in to the OPAC as this borrower 4) Go to opac-user.pl, your summary page 5) Confirm there is a warning box explaining that the borrower is blocked from holds due to fines 6) Back on the staff side, pay some or all of the borrower's fines so they are no longer over the max outstanding amount 7) Refresh the summary page in the OPAC and confirm the warning is gone Sponsored-by: Catalyst IT Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> Created attachment 99841 [details] [review] Bug 15775: Change wording slightly maxoutstanding only blocks holds, for checkouts we got noissuechage. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> It looks like I missed changing status here. Nice work everyone! Pushed to master for 20.05 backported to 19.11.x for 19.11.05 not backporting enhancement to 19.05.x |