Bug 14930 - Leaving OpacFineNoRenewals blank blocks renewals, but should disable feature
Summary: Leaving OpacFineNoRenewals blank blocks renewals, but should disable feature
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords: Academy
Depends on:
Blocks: 15374
  Show dependency treegraph
 
Reported: 2015-09-30 16:17 UTC by Katrin Fischer
Modified: 2017-01-10 19:34 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
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 (1.33 KB, patch)
2016-01-19 03:17 UTC, Gus Ellerm
Details | Diff | Splinter 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 (1.33 KB, patch)
2016-01-19 03:17 UTC, Gus Ellerm
Details | Diff | Splinter Review
14930 - Leaving OpacFineNoRenewals blank blocks renewals, but should disable feature (1.51 KB, patch)
2016-01-25 16:48 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14930 [QA Followup] - Allow OpacFineNoRenewals to be 0 (1.56 KB, patch)
2016-01-29 16:20 UTC, Kyle M Hall
Details | Diff | Splinter Review
14930 - Leaving OpacFineNoRenewals blank blocks renewals, but should disable feature (1.51 KB, patch)
2016-02-04 19:29 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14930 [QA Followup] - Allow OpacFineNoRenewals to be 0 (1.71 KB, patch)
2016-02-04 19:30 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 14930 - Leaving OpacFineNoRenewals blank blocks renewals, but should disable feature (1.58 KB, patch)
2016-02-09 11:43 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 14930 [QA Followup] - Allow OpacFineNoRenewals to be 0 (1.78 KB, patch)
2016-02-09 11:43 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Katrin Fischer 2015-09-30 16:17:30 UTC
OPACFineNoRenewals reads:
Only allow patrons to renew their own books on the OPAC if they have less than EUR in fines (leave blank to disable). 

But leaving the field blank causes renewals to be blocked in OPAC:

http://git.koha-community.org/gitweb/?p=koha.git;a=blob;f=opac/opac-user.pl;hb=3007208c70274a2bab326bc3ccf31bced32dab2b#l109
Comment 1 Katrin Fischer 2015-09-30 16:18:39 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
Comment 2 Katrin Fischer 2016-01-17 23:57:24 UTC
System preference needs to be reworded or behaviour fixed :)
Comment 3 Gus Ellerm 2016-01-19 03:17:24 UTC Comment hidden (obsolete)
Comment 4 Gus Ellerm 2016-01-19 03:17:57 UTC Comment hidden (obsolete)
Comment 5 Marc Véron 2016-01-25 16:48:51 UTC Comment hidden (obsolete)
Comment 6 Jonathan Druart 2016-01-27 10:57:00 UTC
Should not we take 0 into account?
Comment 7 Kyle M Hall 2016-01-29 12:35:33 UTC
(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 ).
Comment 8 Marc Véron 2016-01-29 13:08:07 UTC
(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).
Comment 9 Kyle M Hall 2016-01-29 16:20:17 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall 2016-02-03 14:58:35 UTC
(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?
Comment 11 Marc Véron 2016-02-03 15:24:54 UTC
(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  :-)
Comment 12 Kyle M Hall 2016-02-03 16:23:02 UTC
(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.
Comment 13 Marc Véron 2016-02-04 19:29:33 UTC Comment hidden (obsolete)
Comment 14 Marc Véron 2016-02-04 19:30:33 UTC Comment hidden (obsolete)
Comment 15 Marc Véron 2016-02-04 19:38:32 UTC
(...)
> 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
Comment 16 Jonathan Druart 2016-02-09 11:43:07 UTC
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>
Comment 17 Jonathan Druart 2016-02-09 11:43:13 UTC
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>
Comment 18 Brendan Gallagher 2016-02-24 00:08:25 UTC
Pushed to Master - Should be in the May 2016 Release. Thanks
Comment 19 Julian Maurice 2016-02-26 13:02:14 UTC
Patches pushed to 3.22.x, will be in 3.22.4
Comment 20 Frédéric Demians 2016-03-01 16:52:43 UTC
This patch has been pushed to 3.20.x, will be in 3.20.9.