|
Description
Katrin Fischer
2015-09-30 16:17:30 UTC
To test: - Borrower has some fines and checkouts - Leave OPACFineNoRenewals empty/blank - Try to renew items in the OPAC - notice error message - Add a big value to OpacFineNoRenewals - notice renewals work System preference needs to be reworded or behaviour fixed :) Created attachment 46845 [details] [review] 14930 - Leaving OpacFineNoRenewals blank blocks renewals, but should disable feature To Test - - Borrower has some fines and checkouts - Leave OPACFineNoRenewals empty/blank - Try to renew items in the OPAC - notice error message - Add a big value to Opac Created attachment 46846 [details] [review] 14930 - Leaving OpacFineNoRenewals blank blocks renewals, but should disable feature To Test - - Borrower has some fines and checkouts - Leave OPACFineNoRenewals empty/blank - Try to renew items in the OPAC - notice error message - Add a big value to Opac Created attachment 47271 [details] [review] 14930 - Leaving OpacFineNoRenewals blank blocks renewals, but should disable feature To Test - Borrower has some fines and checkouts - Leave OPACFineNoRenewals empty/blank - Try to renew items in the OPAC - notice error message - Add a big value to OpacFineNoRenewals - notice renewals work Leaving the OPACFineNoRenewals empty will disable the feature. Patch works as expected.(Amended to format the commit message and to remove a warn on line 112 / MV) Signed-off-by: Marc Véron <veron@veron.ch> Should not we take 0 into account? (In reply to Jonathan Druart from comment #6) > Should not we take 0 into account? I agree. Allowing 0 would mean a patron must have no fines in order to renew items. I think allowing 0 makes sense. It would also be good to note in the system preference description that leaving it empty will disable the feature ( i.e. patrons will always be able to renew items, no matter how much they owe in fines ). (In reply to Kyle M Hall from comment #7) > (In reply to Jonathan Druart from comment #6) > > Should not we take 0 into account? > > I agree. Allowing 0 would mean a patron must have no fines in order to renew > items. I think allowing 0 makes sense. It would also be good to note in the > system preference description that leaving it empty will disable the feature > ( i.e. patrons will always be able to renew items, no matter how much they > owe in fines ). ...and maybe the wording should reflect the >0 and 0 cases (with and without displaying the limit), and in a translatable way (sentence not split by tags). Created attachment 47466 [details] [review] Bug 14930 [QA Followup] - Allow OpacFineNoRenewals to be 0 (In reply to Marc Véron from comment #8) > (In reply to Kyle M Hall from comment #7) > > (In reply to Jonathan Druart from comment #6) > > > Should not we take 0 into account? > > > > I agree. Allowing 0 would mean a patron must have no fines in order to renew > > items. I think allowing 0 makes sense. It would also be good to note in the > > system preference description that leaving it empty will disable the feature > > ( i.e. patrons will always be able to renew items, no matter how much they > > owe in fines ). > > ...and maybe the wording should reflect the >0 and 0 cases (with and without > displaying the limit), and in a translatable way (sentence not split by > tags). You mean the wording displayed to the patron, correct? (In reply to Kyle M Hall from comment #10) > (In reply to Marc Véron from comment #8) > > (In reply to Kyle M Hall from comment #7) > > > (In reply to Jonathan Druart from comment #6) > > > > Should not we take 0 into account? > > > > > > I agree. Allowing 0 would mean a patron must have no fines in order to renew > > > items. I think allowing 0 makes sense. It would also be good to note in the > > > system preference description that leaving it empty will disable the feature > > > ( i.e. patrons will always be able to renew items, no matter how much they > > > owe in fines ). > > > > ...and maybe the wording should reflect the >0 and 0 cases (with and without > > displaying the limit), and in a translatable way (sentence not split by > > tags). > > You mean the wording displayed to the patron, correct? Yes, correct :-) (In reply to Marc Véron from comment #11) > (In reply to Kyle M Hall from comment #10) > > (In reply to Marc Véron from comment #8) > > > (In reply to Kyle M Hall from comment #7) > > > > (In reply to Jonathan Druart from comment #6) > > > > > Should not we take 0 into account? > > > > > > > > I agree. Allowing 0 would mean a patron must have no fines in order to renew > > > > items. I think allowing 0 makes sense. It would also be good to note in the > > > > system preference description that leaving it empty will disable the feature > > > > ( i.e. patrons will always be able to renew items, no matter how much they > > > > owe in fines ). > > > > > > ...and maybe the wording should reflect the >0 and 0 cases (with and without > > > displaying the limit), and in a translatable way (sentence not split by > > > tags). > > > > You mean the wording displayed to the patron, correct? > > Yes, correct :-) The template already deals with that. I think the translatability should be in a separate bug since we don't need to modify the template for the fines amount. Created attachment 47652 [details] [review] 14930 - Leaving OpacFineNoRenewals blank blocks renewals, but should disable feature To Test - Borrower has some fines and checkouts - Leave OPACFineNoRenewals empty/blank - Try to renew items in the OPAC - notice error message - Add a big value to OpacFineNoRenewals - notice renewals work Leaving the OPACFineNoRenewals empty will disable the feature. Patch works as expected.(Amended to format the commit message and to remove a warn on line 112 / MV) Signed-off-by: Marc Véron <veron@veron.ch> Created attachment 47653 [details] [review] Bug 14930 [QA Followup] - Allow OpacFineNoRenewals to be 0 Works as expected. Signed-off-by: Marc Véron <veron@veron.ch> (...) > The template already deals with that. I think the translatability should be > in a separate bug since we don't need to modify the template for the fines > amount. Ah, there was already a bug for the translatability: Bug 15374 - Translatability: Fix issues on OPAC summary page Created attachment 47792 [details] [review] Bug 14930 - Leaving OpacFineNoRenewals blank blocks renewals, but should disable feature To Test - Borrower has some fines and checkouts - Leave OPACFineNoRenewals empty/blank - Try to renew items in the OPAC - notice error message - Add a big value to OpacFineNoRenewals - notice renewals work Leaving the OPACFineNoRenewals empty will disable the feature. Patch works as expected.(Amended to format the commit message and to remove a warn on line 112 / MV) Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Created attachment 47793 [details] [review] Bug 14930 [QA Followup] - Allow OpacFineNoRenewals to be 0 Works as expected. Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> Pushed to Master - Should be in the May 2016 Release. Thanks Patches pushed to 3.22.x, will be in 3.22.4 This patch has been pushed to 3.20.x, will be in 3.20.9. |