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
Assignee: Kyle M Hall (khall)
QA Contact: Testopia
URL:
Keywords: Academy
Depends on:
Blocks: 15374
  Show dependency tree / graph
 
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:
Circulation function:

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 (khall) 		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
Show Obsolete (6) View All Add an attachment (proposed patch, testcase, etc.)

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 (khall) 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 (khall) 2016-01-29 16:20:17 UTC Comment hidden (obsolete)
Comment 10 Kyle M Hall (khall) 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 (khall) 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.