Bug 15494 - Block renewals by arbitrary item values
Summary: Block renewals by arbitrary item values
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL: http://devs.bywatersolutions.com/proj...
Keywords:
: 20213 (view as bug list)
Depends on:
Blocks: 22866 27154 32478
  Show dependency treegraph
 
Reported: 2016-01-06 16:27 UTC by Nicole C. Engard
Modified: 2023-05-04 12:43 UTC (History)
11 users (show)

See Also:
Change sponsored?: Sponsored
Patch complexity: Medium patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This enhancement offers the possibility to prevent renewals given certain item values. Using a yaml syntax the library can specify certain item field values that, when matched, will prevent renewals for affected items. If using automatic renewal notices your notice should be updated to account for the new reason that renewals may be denied "item_denied_renewal"
Version(s) released in:


Attachments
Bug 15494 - Unit tests (5.62 KB, patch)
2017-05-24 14:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed (2.20 KB, patch)
2017-05-24 14:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Update display of renewal errors (7.03 KB, patch)
2017-05-24 14:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Add syspref (3.60 KB, patch)
2017-05-24 14:46 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Unit tests (5.66 KB, patch)
2017-10-10 16:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed (2.19 KB, patch)
2017-10-10 16:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Update display of renewal errors (7.08 KB, patch)
2017-10-10 16:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Add syspref (3.60 KB, patch)
2017-10-10 16:35 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Unit tests (5.71 KB, patch)
2017-10-14 23:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed (2.24 KB, patch)
2017-10-14 23:09 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 15494 - Update display of renewal errors (7.14 KB, patch)
2017-10-14 23:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 15494 - Add syspref (3.66 KB, patch)
2017-10-14 23:10 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 15494 - Followup: Make sure items in tests are not overdue (1.47 KB, patch)
2018-01-05 15:41 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Unit tests (6.44 KB, patch)
2018-01-17 17:57 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed (2.34 KB, patch)
2018-01-17 17:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Update display of renewal errors (7.14 KB, patch)
2018-01-17 17:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Add syspref (3.85 KB, patch)
2018-01-17 17:58 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Unit tests (6.48 KB, patch)
2018-02-02 09:02 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed (2.38 KB, patch)
2018-02-02 09:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15494 - Update display of renewal errors (7.17 KB, patch)
2018-02-02 09:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15494 - Add syspref (3.89 KB, patch)
2018-02-02 09:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15494: (QA follow-up) Additional polishing (4.45 KB, patch)
2018-02-02 09:03 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15494 - Unit tests (6.53 KB, patch)
2018-02-02 09:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed (2.44 KB, patch)
2018-02-02 09:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15494 - Update display of renewal errors (7.23 KB, patch)
2018-02-02 09:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15494 - Add syspref (3.95 KB, patch)
2018-02-02 09:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15494: (QA follow-up) Additional polishing (4.45 KB, patch)
2018-02-02 09:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 15494 - Unit tests (6.55 KB, patch)
2018-09-21 20:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed (2.40 KB, patch)
2018-09-21 20:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Update display of renewal errors (7.12 KB, patch)
2018-09-21 20:17 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Add syspref (3.96 KB, patch)
2018-09-21 20:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494: (QA follow-up) Additional polishing (4.42 KB, patch)
2018-09-21 20:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494: Move yaml syspref code to its own sub (5.33 KB, patch)
2018-09-21 20:18 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494: Update tests to generate their own data (no delete) (2.28 KB, patch)
2018-09-24 11:07 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Unit tests (6.55 KB, patch)
2018-10-10 11:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed (2.40 KB, patch)
2018-10-10 11:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Update display of renewal errors (7.12 KB, patch)
2018-10-10 11:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494 - Add syspref (3.96 KB, patch)
2018-10-10 11:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494: (QA follow-up) Additional polishing (4.42 KB, patch)
2018-10-10 11:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494: (follow-up) Move yaml syspref code to its own sub in SySpref object (11.51 KB, patch)
2018-10-10 11:00 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494: (follow-up) Update tests to generate their own data (no delete) (2.28 KB, patch)
2018-10-10 11:01 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 15494: Unit tests (6.61 KB, patch)
2018-10-28 16:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 15494: Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed (2.46 KB, patch)
2018-10-28 16:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 15494: Update display of renewal errors (7.22 KB, patch)
2018-10-28 16:04 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 15494: Add syspref (4.01 KB, patch)
2018-10-28 16:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 15494: (QA follow-up) Additional polishing (4.48 KB, patch)
2018-10-28 16:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 15494: (follow-up) Move yaml syspref code to its own sub in SySpref object (11.57 KB, patch)
2018-10-28 16:05 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 15494: (follow-up) Update tests to generate their own data (no delete) (2.34 KB, patch)
2018-10-28 16:05 UTC, Katrin Fischer
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nicole C. Engard 2016-01-06 16:27:14 UTC
This development would allow a library to prevent the renewal of items based on the values of any field in the items table including but not limited to any itemlost value.

Work to be done:
1) Add new system preference ItemsDeniedRenewal patterned after OpacHiddenItems
2) Modify C4::Circulation::CanBookBeRenewed to check this system preference


Seeking Co-sponsors: http://devs.bywatersolutions.com/projects/make-longoverduelost-items-not-renewable/
Comment 1 Nick Clemens 2017-05-24 14:46:16 UTC Comment hidden (obsolete)
Comment 2 Nick Clemens 2017-05-24 14:46:19 UTC Comment hidden (obsolete)
Comment 3 Nick Clemens 2017-05-24 14:46:22 UTC Comment hidden (obsolete)
Comment 4 Nick Clemens 2017-05-24 14:46:25 UTC Comment hidden (obsolete)
Comment 5 Katrin Fischer 2017-10-08 09:51:08 UTC Comment hidden (obsolete)
Comment 6 Nick Clemens 2017-10-10 16:35:30 UTC Comment hidden (obsolete)
Comment 7 Nick Clemens 2017-10-10 16:35:33 UTC Comment hidden (obsolete)
Comment 8 Nick Clemens 2017-10-10 16:35:36 UTC Comment hidden (obsolete)
Comment 9 Nick Clemens 2017-10-10 16:35:39 UTC Comment hidden (obsolete)
Comment 10 Katrin Fischer 2017-10-14 23:09:20 UTC
This seems to work quite well! Also tested renew through checkout, renewal tab and OPAC.

When renewing the item from the list of checkouts and using the renewal limit override functionality, the message displayed is:

Renew failed: item_denied_renewal

This could be improved, maybe with a small follow-up?
Comment 11 Katrin Fischer 2017-10-14 23:09:52 UTC
Created attachment 68146 [details] [review]
Bug 15494 - Unit tests

prove t/db_dependent/Circulation.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 12 Katrin Fischer 2017-10-14 23:09:56 UTC
Created attachment 68147 [details] [review]
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 13 Katrin Fischer 2017-10-14 23:10:00 UTC
Created attachment 68148 [details] [review]
Bug 15494 - Update display of renewal errors

To test:
1 - Define a rule to deny renewal
2 - Checkout an item meeting this rule
3 - Attempt renew an item via the OPAC
4 - Attempt renewal via SIP
5 - Attempt renewal via staff client
6 - Verify errors are reasonable

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 14 Katrin Fischer 2017-10-14 23:10:04 UTC
Created attachment 68149 [details] [review]
Bug 15494 - Add syspref

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 15 Marcel de Rooy 2018-01-05 07:12:25 UTC Comment hidden (obsolete)
Comment 16 Nick Clemens 2018-01-05 10:54:05 UTC Comment hidden (obsolete)
Comment 17 Marcel de Rooy 2018-01-05 15:14:09 UTC Comment hidden (obsolete)
Comment 18 Nick Clemens 2018-01-05 15:41:06 UTC
Created attachment 70295 [details] [review]
Bug 15494 - Followup: Make sure items in tests are not overdue

prove -v t/db_dependent/Circulation.t
Comment 19 Nick Clemens 2018-01-05 15:41:56 UTC Comment hidden (obsolete)
Comment 20 Marcel de Rooy 2018-01-05 16:07:03 UTC Comment hidden (obsolete)
Comment 21 Marcel de Rooy 2018-01-05 16:49:34 UTC
+ unless ($item)    { carp "No item passed in!" && return }
Not our preferred coding style

+ return () if (! $yaml =~ /\S/ );
Just return here. The only call is in scalar context. We do not need a list.
Shorter: return if $yaml !~ /\S/;

+    eval {
+        $denyingrules = YAML::Load($yaml);
+    };
How much time are we spending on doing this? What is the performance effect? If the pref always looks like field1: [val]\nfield2: [val2] we can just split on newline and split on ':', etc. Give me some convincing numbers please.

+        return ();
Same as above

+        my $val;
+        if (exists $item->{$field}) {
+            $val = $item->{$field};
+        }
+        $val = '' unless defined $val;
Why not in just one line: my $val = $item->{$field} // '';

+            - How to write these rules is documented on the <a href="http://wiki.koha-community.org/wiki/OpacHiddenItems" target="_blank">Koha wiki</a>.
This is from the preference file. Not sure if this is the preferred way. A wiki page could be gone just as easily. Why not just provide an example as close as possible to the pref itself. Just another 40 chars ?

Just curious too if this supports denying renewal on an empty string? If so, the conversion from undef to empty string might not be what we want. Can we signal empty string or undef to be a blocking value? [No blocker for me btw :)]

Changing status. Please make some adjustments.
Comment 22 Nick Clemens 2018-01-17 17:57:58 UTC
Created attachment 70629 [details] [review]
Bug 15494 - Unit tests

prove -v t/db_dependent/Circulation.t
Comment 23 Nick Clemens 2018-01-17 17:58:01 UTC
Created attachment 70630 [details] [review]
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed
Comment 24 Nick Clemens 2018-01-17 17:58:04 UTC
Created attachment 70631 [details] [review]
Bug 15494 - Update display of renewal errors

To test:
1 - Define a rule to deny renewal
2 - Checkout an item meeting this rule
3 - Attempt renew an item via the OPAC
4 - Attempt renewal via SIP
5 - Attempt renewal via staff client
6 - Verify errors are reasonable

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 25 Nick Clemens 2018-01-17 17:58:08 UTC
Created attachment 70632 [details] [review]
Bug 15494 - Add syspref
Comment 26 Nick Clemens 2018-01-17 18:02:35 UTC
(In reply to Marcel de Rooy from comment #21)
> + unless ($item)    { carp "No item passed in!" && return }
> Not our preferred coding style

This was just copying others in this file, changed to "return unless $item"

> + return () if (! $yaml =~ /\S/ );
> Just return here. 
Removed

> +    eval {
> +        $denyingrules = YAML::Load($yaml);
> +    };
> How much time are we spending on doing this? What is the performance effect?
Was about .037 seconds (with 2 fields and 1000 values)
Using split is about .014 seconds, okay, I switched

> +        return ();
Yup

> Why not in just one line: my $val = $item->{$field} // '';
Good point

>Why not just provide an example as close as possible to the pref itself?

Okay

> Just curious too if this supports denying renewal on an empty string? If so,
> the conversion from undef to empty string might not be what we want. 
Okay, can use "" or NULL for specific now (see the pref explanation)


> Changing status. Please make some adjustments.
Improved, squashed, sent back for SO
Comment 27 Marcel de Rooy 2018-02-02 07:08:05 UTC
Working on this one now
Comment 28 Marcel de Rooy 2018-02-02 09:02:57 UTC
Created attachment 71129 [details] [review]
Bug 15494 - Unit tests

prove -v t/db_dependent/Circulation.t
Comment 29 Marcel de Rooy 2018-02-02 09:03:01 UTC
Created attachment 71130 [details] [review]
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed
Comment 30 Marcel de Rooy 2018-02-02 09:03:06 UTC
Created attachment 71131 [details] [review]
Bug 15494 - Update display of renewal errors

To test:
1 - Define a rule to deny renewal
2 - Checkout an item meeting this rule
3 - Attempt renew an item via the OPAC
4 - Attempt renewal via SIP
5 - Attempt renewal via staff client
6 - Verify errors are reasonable

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 31 Marcel de Rooy 2018-02-02 09:03:10 UTC
Created attachment 71132 [details] [review]
Bug 15494 - Add syspref
Comment 32 Marcel de Rooy 2018-02-02 09:03:15 UTC
Created attachment 71133 [details] [review]
Bug 15494: (QA follow-up) Additional polishing

[1] Fix two typos in Circulation.t.
    Although the test does not fail, line 2127 contains two typos.
    Changing INVISILE to INVISIBLE :)
    And type should be itype.
[2] Remove $yaml as leftover from older code.
[3] Add a next when the split on /:/ does not give two results. This will
    prevent uninit warnings (although still disabled now in Circulation).
[4] For the same reason we should switch the lines for NULL and empty
    string. The undefs you insert should trigger a warn.
[5] The line for empty string should not insert undef, but empty string.
    For the same reason adding the condition defined($_) ...
    And proving it by adding two tests for the opposite values of
    callnumber and itemnotes.
[6] Adding a strip spaces around the fieldname. User friendly..

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 33 Marcel de Rooy 2018-02-02 09:05:25 UTC
Created attachment 71134 [details] [review]
Bug 15494 - Unit tests

prove -v t/db_dependent/Circulation.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 34 Marcel de Rooy 2018-02-02 09:05:30 UTC
Created attachment 71135 [details] [review]
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 35 Marcel de Rooy 2018-02-02 09:05:35 UTC
Created attachment 71136 [details] [review]
Bug 15494 - Update display of renewal errors

To test:
1 - Define a rule to deny renewal
2 - Checkout an item meeting this rule
3 - Attempt renew an item via the OPAC
4 - Attempt renewal via SIP
5 - Attempt renewal via staff client
6 - Verify errors are reasonable

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 36 Marcel de Rooy 2018-02-02 09:05:39 UTC
Created attachment 71137 [details] [review]
Bug 15494 - Add syspref

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 37 Marcel de Rooy 2018-02-02 09:05:45 UTC
Created attachment 71138 [details] [review]
Bug 15494: (QA follow-up) Additional polishing

[1] Fix two typos in Circulation.t.
    Although the test does not fail, line 2127 contains two typos.
    Changing INVISILE to INVISIBLE :)
    And type should be itype.
[2] Remove $yaml as leftover from older code.
[3] Add a next when the split on /:/ does not give two results. This will
    prevent uninit warnings (although still disabled now in Circulation).
[4] For the same reason we should switch the lines for NULL and empty
    string. The undefs you insert should trigger a warn.
[5] The line for empty string should not insert undef, but empty string.
    For the same reason adding the condition defined($_) ...
    And proving it by adding two tests for the opposite values of
    callnumber and itemnotes.
[6] Adding a strip spaces around the fieldname. User friendly..

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 38 Marcel de Rooy 2018-02-02 09:09:04 UTC
Has been tested already before, spent sufficient time on changes now.
Looks good to me.
No blocker, but slightly confusing in staff checkouts table:

0 Renewal denied by syspref
(10 of 10 renewals remaining)

If I can't renew, what is the value of these 10? :)

Passed QA with follow-up for some minor issues.
Comment 39 Jonathan Druart 2018-02-02 14:55:06 UTC
1. In tests, prefer Koha::IssuingRules->search->update($what_you_need_to_be_set);
instead of the DELETE then INSERT. It makes the changes more readable in my opinion.

2. What about "NULL" string values? :)

3. Should not we have the same code for 
OpacHiddenItems and ItemsDeniedRenewal (and UpdateNotForLoanStatusOnCheckin?)
There is also UpdateItemLocationOnCheckin (bug 14576)

The minimum would be a new get_rules_from_prefs sub

Marking as FQA for discussion
Comment 40 Jonathan Druart 2018-02-02 14:56:39 UTC
We should also tell people that a new item_denied_renewal condition must be added to AUTO_RENEWALS
Comment 41 Katrin Fischer 2018-02-02 17:28:31 UTC
(In reply to Jonathan Druart from comment #40)
> We should also tell people that a new item_denied_renewal condition must be
> added to AUTO_RENEWALS

Can you explain that one? People?
Comment 42 Jonathan Druart 2018-02-02 17:41:49 UTC
s/people/Koha administrators

In the update DB process I guess, a warning to tell this new code is added and must be taken into account in the template of the notice.
Comment 43 Katrin Fischer 2018-02-02 18:37:03 UTC
Ah! Not sure the reason is available in the template tho.
Comment 44 Nick Clemens 2018-09-21 20:17:47 UTC
Created attachment 79249 [details] [review]
Bug 15494 - Unit tests

prove -v t/db_dependent/Circulation.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 45 Nick Clemens 2018-09-21 20:17:52 UTC
Created attachment 79250 [details] [review]
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 46 Nick Clemens 2018-09-21 20:17:57 UTC
Created attachment 79251 [details] [review]
Bug 15494 - Update display of renewal errors

To test:
1 - Define a rule to deny renewal
2 - Checkout an item meeting this rule
3 - Attempt renew an item via the OPAC
4 - Attempt renewal via SIP
5 - Attempt renewal via staff client
6 - Verify errors are reasonable

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 47 Nick Clemens 2018-09-21 20:18:02 UTC
Created attachment 79252 [details] [review]
Bug 15494 - Add syspref

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 48 Nick Clemens 2018-09-21 20:18:06 UTC
Created attachment 79253 [details] [review]
Bug 15494: (QA follow-up) Additional polishing

[1] Fix two typos in Circulation.t.
    Although the test does not fail, line 2127 contains two typos.
    Changing INVISILE to INVISIBLE :)
    And type should be itype.
[2] Remove $yaml as leftover from older code.
[3] Add a next when the split on /:/ does not give two results. This will
    prevent uninit warnings (although still disabled now in Circulation).
[4] For the same reason we should switch the lines for NULL and empty
    string. The undefs you insert should trigger a warn.
[5] The line for empty string should not insert undef, but empty string.
    For the same reason adding the condition defined($_) ...
    And proving it by adding two tests for the opposite values of
    callnumber and itemnotes.
[6] Adding a strip spaces around the fieldname. User friendly..

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 49 Nick Clemens 2018-09-21 20:18:11 UTC
Created attachment 79254 [details] [review]
Bug 15494: Move yaml syspref code to its own sub

To test:
1 - prove -v t/db_dependent/Koha/Util/SystemPreferences.t
Comment 50 Nick Clemens 2018-09-21 20:21:25 UTC
(In reply to Jonathan Druart from comment #39)
> 1. In tests, prefer
> Koha::IssuingRules->search->update($what_you_need_to_be_set);
> instead of the DELETE then INSERT. It makes the changes more readable in my
> opinion.

I generally agree, but circ rules are messy and this file has the same code existing. Should be refactored as a whole

 
> 2. What about "NULL" string values? :)

I think we expect a certain level of input from the user and are fairly thorough here

> 3. Should not we have the same code for 
> OpacHiddenItems and ItemsDeniedRenewal (and UpdateNotForLoanStatusOnCheckin?)
> There is also UpdateItemLocationOnCheckin (bug 14576)
> 
> The minimum would be a new get_rules_from_prefs sub

Moved to its own sub/module - changes for other prefs should be on their own reports

> Ah! Not sure the reason is available in the template tho.

The reasons are stored in the issues table, they should be available to templates
Comment 51 Nick Clemens 2018-09-24 11:07:10 UTC
Created attachment 79293 [details] [review]
Bug 15494: Update tests to generate their own data (no delete)
Comment 52 Nick Clemens 2018-10-09 15:09:28 UTC
(In reply to Nick Clemens from comment #50)
> (In reply to Jonathan Druart from comment #39)
> > 1. In tests, prefer
> > Koha::IssuingRules->search->update($what_you_need_to_be_set);
> > instead of the DELETE then INSERT. It makes the changes more readable in my
> > opinion.
> 
> I generally agree, but circ rules are messy and this file has the same code
> existing. Should be refactored as a whole
> 
okay, found an easy way to fix, updated.
Comment 53 Jonathan Druart 2018-10-09 16:54:55 UTC
Should not Koha::Util::SystemPreferences->get_yaml_pref_hash be a method of Koha::Config::SysPref instead?
Comment 54 Nick Clemens 2018-10-10 11:00:31 UTC
Created attachment 80327 [details] [review]
Bug 15494 - Unit tests

prove -v t/db_dependent/Circulation.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 55 Nick Clemens 2018-10-10 11:00:36 UTC
Created attachment 80328 [details] [review]
Bug 15494 - Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 56 Nick Clemens 2018-10-10 11:00:41 UTC
Created attachment 80329 [details] [review]
Bug 15494 - Update display of renewal errors

To test:
1 - Define a rule to deny renewal
2 - Checkout an item meeting this rule
3 - Attempt renew an item via the OPAC
4 - Attempt renewal via SIP
5 - Attempt renewal via staff client
6 - Verify errors are reasonable

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 57 Nick Clemens 2018-10-10 11:00:45 UTC
Created attachment 80330 [details] [review]
Bug 15494 - Add syspref

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 58 Nick Clemens 2018-10-10 11:00:51 UTC
Created attachment 80331 [details] [review]
Bug 15494: (QA follow-up) Additional polishing

[1] Fix two typos in Circulation.t.
    Although the test does not fail, line 2127 contains two typos.
    Changing INVISILE to INVISIBLE :)
    And type should be itype.
[2] Remove $yaml as leftover from older code.
[3] Add a next when the split on /:/ does not give two results. This will
    prevent uninit warnings (although still disabled now in Circulation).
[4] For the same reason we should switch the lines for NULL and empty
    string. The undefs you insert should trigger a warn.
[5] The line for empty string should not insert undef, but empty string.
    For the same reason adding the condition defined($_) ...
    And proving it by adding two tests for the opposite values of
    callnumber and itemnotes.
[6] Adding a strip spaces around the fieldname. User friendly..

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 59 Nick Clemens 2018-10-10 11:00:56 UTC
Created attachment 80332 [details] [review]
Bug 15494: (follow-up) Move yaml syspref code to its own sub in SySpref object

To test:
1 - prove -v t/db_dependent/Koha/Config/SysPrefs.t
Comment 60 Nick Clemens 2018-10-10 11:01:01 UTC
Created attachment 80333 [details] [review]
Bug 15494: (follow-up) Update tests to generate their own data (no delete)
Comment 61 Katrin Fischer 2018-10-28 16:04:20 UTC
Created attachment 81411 [details] [review]
Bug 15494: Unit tests

prove -v t/db_dependent/Circulation.t

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 62 Katrin Fischer 2018-10-28 16:04:50 UTC
Created attachment 81412 [details] [review]
Bug 15494: Add _items_denied_renewal sub to C4:Circulation::CanBookBeRenewed

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 63 Katrin Fischer 2018-10-28 16:04:56 UTC
Created attachment 81413 [details] [review]
Bug 15494: Update display of renewal errors

To test:
1 - Define a rule to deny renewal
2 - Checkout an item meeting this rule
3 - Attempt renew an item via the OPAC
4 - Attempt renewal via SIP
5 - Attempt renewal via staff client
6 - Verify errors are reasonable

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 64 Katrin Fischer 2018-10-28 16:05:02 UTC
Created attachment 81414 [details] [review]
Bug 15494: Add syspref

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 65 Katrin Fischer 2018-10-28 16:05:08 UTC
Created attachment 81415 [details] [review]
Bug 15494: (QA follow-up) Additional polishing

[1] Fix two typos in Circulation.t.
    Although the test does not fail, line 2127 contains two typos.
    Changing INVISILE to INVISIBLE :)
    And type should be itype.
[2] Remove $yaml as leftover from older code.
[3] Add a next when the split on /:/ does not give two results. This will
    prevent uninit warnings (although still disabled now in Circulation).
[4] For the same reason we should switch the lines for NULL and empty
    string. The undefs you insert should trigger a warn.
[5] The line for empty string should not insert undef, but empty string.
    For the same reason adding the condition defined($_) ...
    And proving it by adding two tests for the opposite values of
    callnumber and itemnotes.
[6] Adding a strip spaces around the fieldname. User friendly..

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 66 Katrin Fischer 2018-10-28 16:05:14 UTC
Created attachment 81416 [details] [review]
Bug 15494: (follow-up) Move yaml syspref code to its own sub in SySpref object

To test:
1 - prove -v t/db_dependent/Koha/Config/SysPrefs.t

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 67 Katrin Fischer 2018-10-28 16:05:19 UTC
Created attachment 81417 [details] [review]
Bug 15494: (follow-up) Update tests to generate their own data (no delete)

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 68 Nick Clemens 2018-11-07 23:16:01 UTC
Awesome work all!

Pushed to master for 18.11
Comment 69 Martin Renvoize 2018-11-16 12:51:33 UTC
Enhancement, will not be backported to 18.05.x series.
Comment 70 Katrin Fischer 2020-04-01 23:12:20 UTC
*** Bug 20213 has been marked as a duplicate of this bug. ***
Comment 71 Jonathan Druart 2020-12-04 14:20:59 UTC
Nick, why did you copy/pasted get_yaml_pref_hash in Koha/Config/SysPref.pm and Koha/Util/SystemPreferences.pm?

Koha/Config/SysPref.pm:sub get_yaml_pref_hash {
Koha/Util/SystemPreferences.pm:sub get_yaml_pref_hash {
Comment 72 Jonathan Druart 2020-12-04 15:01:05 UTC
(In reply to Jonathan Druart from comment #71)
> Nick, why did you copy/pasted get_yaml_pref_hash in Koha/Config/SysPref.pm
> and Koha/Util/SystemPreferences.pm?
> 
> Koha/Config/SysPref.pm:sub get_yaml_pref_hash {
> Koha/Util/SystemPreferences.pm:sub get_yaml_pref_hash {

Opened bug 27154.