Here is a possible solution. It just sets a default value if no value is specified when saving a rule. There is also an atomic update to update rules already in the database. We are testing it and would appreciate your feedback so I'm setting it as "Needs Signoff". Please advise me if this not correct.

I am not sure the patch is the correct way to fix it.
If no value has not been filled in, we shouldn't default.

In my opinion we should better improve the way we calculate/retrieve the rules instead.

It's possible to search the circulation rules for the branch code, category code, and item type with either the specified values or null. The results can be sorted to get the most relevant value for each of the 3 parameters. These 3 values can be used to search for the rule. If there is no rule, the default value of the rule can be used.

This should give the same result as the patch, but it makes looking up a rule a little more complicated.

If it's better to keep the ability to inherit or fall back to a less specific set of rules, it would be necessary to specify if a value should be default or inherited when defining the rules (e.g. using a special value or checking a box). Rules that should be inherited wouldn't be saved and the current method of finding rules should fall back to the most relevant value of the rule.

There is a problem since some rules depend on other rules (e.g. lengthunit). It would be necessary to either make sure that these rules are retrieved from the same set of rules or make these values self-contained (e.g. length and unit in the same value).

As it is now, it's possible to retrieve the lengthunit from a set of rules without an issuelength and the issuelength from a less specific set of rules with a different lengthunit.

The problem was that certain blank values weren't saved and it seemed reasonable to use default values when those values are blank.

Do users expect these rules to fall back to higher rules? In the test case, hard due date for books is left blank and should not fall back to the rule for all items. So in this case, using the default value is expected.

It is possible to allow falling back, but I'm not sure if it's the solution to this bug

Which values do people think should be used? How should rules be calculated?

(In reply to Jonathan Druart from comment #4)
> I am not sure the patch is the correct way to fix it.
> If no value has not been filled in, we shouldn't default.
> 
> In my opinion we should better improve the way we calculate/retrieve the
> rules instead.

I think this fix is correct for current use of rules as a 'row' - we have all the current rules inthe table grouped, and don't expect to fall back on any individual column if a row is set for a patron/category type

I do think there are other issues:
bug 28774  made rentaldiscount not save if blank, but creates the same issue here of defaulting to a higher rule level
unseen_renewals of '0' seems to translate to 'unlimited' - I don't think that's correct

(In reply to Nick Clemens from comment #7)
> (In reply to Jonathan Druart from comment #4)
> > I am not sure the patch is the correct way to fix it.
> > If no value has not been filled in, we shouldn't default.
> > 
> > In my opinion we should better improve the way we calculate/retrieve the
> > rules instead.
> 
> I think this fix is correct for current use of rules as a 'row' - we have
> all the current rules inthe table grouped, and don't expect to fall back on
> any individual column if a row is set for a patron/category type
> 
> I do think there are other issues:
> bug 28774  made rentaldiscount not save if blank, but creates the same issue
> here of defaulting to a higher rule level
> unseen_renewals of '0' seems to translate to 'unlimited' - I don't think
> that's correct

It appears that unseen_renewals is saved even when blank, so I removed it from the patch. Would somebody please confirm this?

(In reply to Kevin Carnes from comment #9)
> (In reply to Nick Clemens from comment #7)
> > (In reply to Jonathan Druart from comment #4)
> > > I am not sure the patch is the correct way to fix it.
> > > If no value has not been filled in, we shouldn't default.
> > > 
> > > In my opinion we should better improve the way we calculate/retrieve the
> > > rules instead.
> > 
> > I think this fix is correct for current use of rules as a 'row' - we have
> > all the current rules inthe table grouped, and don't expect to fall back on
> > any individual column if a row is set for a patron/category type
> > 
> > I do think there are other issues:
> > bug 28774  made rentaldiscount not save if blank, but creates the same issue
> > here of defaulting to a higher rule level
> > unseen_renewals of '0' seems to translate to 'unlimited' - I don't think
> > that's correct
> 
> It appears that unseen_renewals is saved even when blank, so I removed it
> from the patch. Would somebody please confirm this?

Ok, unseen_renewals is only saved when UnseenRenewals is enabled. It now defaults to "" which is unlimited. '0' means none are allowed. It only starts counting unseen renewals when UnseenRenewals is not unlimited. It might make more sense to always count unseen renewals.

The default value for rentaldiscount is 0, so it won't be blank and won't be affected by bug 28774.

I also updated onboarding.pl for the Web installer. I don't know if there are any more rules that should be fixed there.

I tried to test, but I am unable to get the original "problematic" behaviour.

My rules are as follows:
Patron category: Teacher
Item type: Books
Current checkouts allowed: 10
Current on-site checkouts allowed: 10
Loan period: 10
Hard due date: None defined
(I didn't fill out anything else)

Patron category: Teacher
Item type: All
Current checkouts allowed: 10
Current on-site checkouts allowed: 10
Loan period: 10
Hard due date : Exactly on 2022-06-30
(I didn't fill out anything else)

I also have the All/All rule that comes with a sandbox
Patron category: All
Item type: All
Current checkouts allowed: 5
Current on-site checkouts allowed: 5
Loan period: 5
Hard due date: None defined
(etc.)

I checked out 39999000003857 (item-level itype = Music) to 23529000139858, and the due date was 2022-06-30 (today)
I checked out 39999000010114 (item-level itype = Books) to the same patron and the due date was 2022-07-10 (ten days from now)

I'm not sure what I did differently than the test plan.

(In reply to Caroline Cyr La Rose from comment #16)
> I tried to test, but I am unable to get the original "problematic" behaviour.
> 
> My rules are as follows:
> Patron category: Teacher
> Item type: Books
> Current checkouts allowed: 10
> Current on-site checkouts allowed: 10
> Loan period: 10
> Hard due date: None defined
> (I didn't fill out anything else)
> 
> Patron category: Teacher
> Item type: All
> Current checkouts allowed: 10
> Current on-site checkouts allowed: 10
> Loan period: 10
> Hard due date : Exactly on 2022-06-30
> (I didn't fill out anything else)
> 
> I also have the All/All rule that comes with a sandbox
> Patron category: All
> Item type: All
> Current checkouts allowed: 5
> Current on-site checkouts allowed: 5
> Loan period: 5
> Hard due date: None defined
> (etc.)
> 
> I checked out 39999000003857 (item-level itype = Music) to 23529000139858,
> and the due date was 2022-06-30 (today)
> I checked out 39999000010114 (item-level itype = Books) to the same patron
> and the due date was 2022-07-10 (ten days from now)
> 
> I'm not sure what I did differently than the test plan.

I tested this on the master branch and received the hard due date for both items. Was the patch for this bug ever applied on the sandbox? If the rules were created when the patch was applied, that would explain your results.

You can try deleting the teacher rules and adding them again. You can also try testing it on another sandbox without any patches.

Sigh. I was able to reproduce the original behaviour in a fresh sandbox, but even after applying the patch in the sandbox, the behaviour stayed the same... I will try on my dev install

It worked on my local installation :)

Created attachment 138310 [details] [review]
Bug 29012 (QA follow-up) Use q{} instead of '' in smart-rules.pl

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Pushed to master for 22.11.

Nice work everyone, thanks!

Backported to 22.05.x for 22.05.06

DANGER!!! ALERT!!!
The atomic update FILLS database for BRANCHES rules which not existed before, and values take precedence but are still hidden from the operator!

for example, if you had holdallowed for some branch set up, that is table 5 on the page, not 1st, after the atomic update, you will have hidden issuedate=0 for 1st table, and the operator never will notice this until it gets into the raw database.
But a single value of issuedate=0 will become applied OVER "all" branches rules because branches have higher precedence.

Those who installed this on production will get issues with the circulation_rules table because of atomic update. So:


On smart-rules.pl (Home > Administration > Circulation and fine rules):
we have 5 tables:

    1. Defining circulation and fine rules for all libraries
       (... for XXX branch, if branch selected)
    2. Default checkout, hold and return policy
       (... for XXX branch, if branch selected)
    3. Checkout, hold policy by patron category 
       (... for XXX branch, if branch selected)
    4. Lost item fee refund on return policy
       (... for XXX branch, if branch selected)
    5. Holds policy by item type
       (... for XXX branch, if branch selected)

and with dropdown, we can select rules for "Standard rules for all libraries"
or switch to ANY of the available branches represented by the whole page.

THE ISSUE:

... and this atomic update was intended to add "empty strings" and "zeroes" to:
issuelength, hardduedate, unseenrenewalsallowed, rentaldiscount,
decreaseloanholds values,

and the intention of atomuc update looks like was to select ANY combination of branch/itemtype/category of else in DB, then for those, we should add those "zeroes" and "empty strings". Like it was "some rows in table 1 if only, then we add empty/zeroes".

But those "any combination" also covers rules belonging NOT just to table 1 but also to tables 2..5, so if you don't have any rule for BRANCH in table 1 but you had something for the branch in tables 2,3,4,5 this atomic update adds orphan values for 'issuelength', 'hardduedate', 'unseenrenewalsallowed', 'rentaldiscount', 'decreaseloanholds'.

And, EVEN WORSE: because those rules even will not be shown for the operator as RULES in table 1, but they still affect the results in circulation analysis as bigger priority rules than those without branches (i.e. "*"/all) so zero or '' value will be used as rule result for calculations.

And nothing will be shown to the operator (!!!).

For example, we just had on our production having "issuedate" "0" for some branches added by this atomic update because those branches had 'holdallowed' settings in table 5 (but nothing in table 1 in that per-branch-page!). But rule became efficient for all requests and made a real mess when borrowers had "today" issue return date & loan length (!!!). More complexity: this now can be fixed only by manually removing trash from circulation_rules but requires analysis, argh!

so we need now to make new ticket & new atomic update for those affected to HEAL orphan rules from database (pretty complicated task as I see, btw).

(In reply to Andrew Nugged from comment #27)
> so we need now to make new ticket & new atomic update for those affected to
> HEAL orphan rules from database (pretty complicated task as I see, btw).

Please create a new blocker bug, as this has already been released!

and another bad thing:

   unseen_renewals_allowed
vs
   unseenrenewalsallowed

(sic!)

we have 'unseen_renewals_allowed' everywhere in codebase, except in this atomic update we have 'unseenrenewalsallowed'. And now DB is polluted even more :).

(In reply to Katrin Fischer from comment #28)

> Please create a new blocker bug, as this has already been released!

yes, creating

> Please create a new blocker bug, as this has already been released!

This hasn't been released. 22.05.06 is due for this month.

22.05 and 21.11 rmaints got and email about reverting this

ok, so everything what was done above this message is OBSOLETE now,

Kevin Carnes, are you ok with committing here new solution from base?

(please mind the patches if something were useful in Bug 31727 too).

This patchset has been reverted from 22.05.x

Created attachment 141869 [details] [review]
Bug 29012: Add default values for blank circulation rules that weren't saved to the database

There are 5 fields that are not set if no value is provided when saving/editing a rule in Administration->Circulation and fines rules
- issuelength
- hardduedate
- unseenrenewalsallowed
- rentaldiscount
- decreaseloanholds

This is problematic because it gives the impression these rules are set as blank, but in reality they don't exist and the rule will fal back to the higher level

To test:
1 - Set a rule for
    Patron category: Teacher
    Itemtype: All
    Hard due date: (Today)
    Lona period: 10
2 - Set a rule for
    Patron category: Teacher
    Itemtype: Books
    Hard due date: (leave blank)
    Loan period: 10
3 - Expected behaviour is Book item will checkout to teacher for 10 days, all other types will be due yesterday at 25:59:00
4 - Checkout an non-book item type to teacher
5 - Hard due date applies
6 - Checkout a 'book' item type to teacher
7 - Hard due date applies - FAIL

(In reply to Andrew Nugged from comment #33)
> ok, so everything what was done above this message is OBSOLETE now,
> 
> Kevin Carnes, are you ok with committing here new solution from base?
> 
> (please mind the patches if something were useful in Bug 31727 too).

Ok, now the patch includes all the changes including bugs 31727 and 31737, but without the new atomic update.

Kevin, I have conflicts applying to the current master. Can you check?

Slava/Kevin: I think it's because Tomás not yet reverted the master from old 29012.

Created attachment 141876 [details] [review]
Revert "Bug 29012: DBRev 22.06.00.024"

This reverts commit 51cef8c0ec1eca244ebafb853a9164bde65941cb.

Created attachment 141877 [details] [review]
Revert "Bug 29012: (QA follow-up) Use q{} instead of '' in smart-rules.pl"

This reverts commit 6f8e77b4e899d6f71fb8987975ba916a65dc8229.

Created attachment 141878 [details] [review]
Revert "Bug 29012: Add default values for blank circulation rules that weren't saved to the database"

This reverts commit cf397ac3bcbc93a508954c836d1cb90a84fb2ac6.

Created attachment 141879 [details] [review]
Bug 29012: Add default values for blank circulation rules that weren't saved to the database

There are 5 fields that are not set if no value is provided when saving/editing a rule in Administration->Circulation and fines rules
- issuelength
- hardduedate
- unseenrenewalsallowed
- rentaldiscount
- decreaseloanholds

This is problematic because it gives the impression these rules are set as blank, but in reality they don't exist and the rule will fal back to the higher level

To test:
1 - Set a rule for
    Patron category: Teacher
    Itemtype: All
    Hard due date: (Today)
    Lona period: 10
2 - Set a rule for
    Patron category: Teacher
    Itemtype: Books
    Hard due date: (leave blank)
    Loan period: 10
3 - Expected behaviour is Book item will checkout to teacher for 10 days, all other types will be due yesterday at 25:59:00
4 - Checkout an non-book item type to teacher
5 - Hard due date applies
6 - Checkout a 'book' item type to teacher
7 - Hard due date applies - FAIL

(In reply to Slava Shishkin from comment #37)
> Kevin, I have conflicts applying to the current master. Can you check?

I added some revert patches. They can be removed when no longer needed. Otherwise I can make a patch that works with the patch already in master.

Good, thanks.

As we have time before release, and we have fixes - do we need to revert, or can we just fix the DB updates here and push those?

(In reply to Nick Clemens from comment #45)
> As we have time before release, and we have fixes - do we need to revert, or
> can we just fix the DB updates here and push those?

I think just fixing the DB updates here should be fine. Perhaps I misunderstood that this would be reverted (https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727#c14). Since it hasn't been reverted yet, perhaps it's still possible to cancel the revert and just fix it here.

(In reply to Kevin Carnes from comment #46)
> (In reply to Nick Clemens from comment #45)
> > As we have time before release, and we have fixes - do we need to revert, or
> > can we just fix the DB updates here and push those?
> 
> I think just fixing the DB updates here should be fine. Perhaps I
> misunderstood that this would be reverted
> (https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727#c14). Since
> it hasn't been reverted yet, perhaps it's still possible to cancel the
> revert and just fix it here.

Can you provide a fix? Thanks!

(In reply to Kevin Carnes from comment #46)
> (In reply to Nick Clemens from comment #45)
> > As we have time before release, and we have fixes - do we need to revert, or
> > can we just fix the DB updates here and push those?
> 
> I think just fixing the DB updates here should be fine. Perhaps I
> misunderstood that this would be reverted
> (https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727#c14). Since
> it hasn't been reverted yet, perhaps it's still possible to cancel the
> revert and just fix it here.

Hi Kevin, 

Yes, please remove the revert patches and replace the original patches, along with a new patch to fix the DB update as a followup

The code was reverted from stable - and i suggest we don't backport even after the fix

Created attachment 141951 [details] [review]
Bug 29012: Fix existing update to prevent inserting incorrect data

Both updates now rely on 'suspension_chargeperiod' as the source of truth
for existence of a rule. Spot checking some existing production databases this
seems to be acceptable.

This alters the update to only add rules where a 'suspension_chargeperiod' rule exists
and a value for the rule is not already set.

NOTE: While the table has a unique key:
UNIQUE KEY `branchcode` (`branchcode`,`categorycode`,`itemtype`,`rule_name`),

This key doesn't prevent duplicates on NULLs, i.e.:

branchcode  | categorycode  | itemtype  | rule_name                 | rule_value
NULL        | NULL          | BK        | unseen_renewals_allowed   | 1
NULL        | NULL          | BK        | unseen_renewals_allowed   | 2
NULL        | NULL          | BK        | unseen_renewals_allowed   | 3

So we need to both check that any rule is present and that the specific rule is not.
The 'INSERT IGNORE' won't IGNORE existing values except for the most specific rules

Test plan:
 0 - Edit and save existing default rule in DB (sample data does not include suspension_chargeperiod)
 1 - Set some rules for "Default checkout, hold and return policy" for "Standard rule" and some specific libraries
 2 - cp installer/data/mysql/db_revs/220600024.pl installer/data/mysql/atomicupdate/
 3 - SELECT * FROM circulation rules, note the number of rules that exist
 4 - updatedatabase
 5 - Check the DB again, you should now have rules as described in the bug
     i.e. You will have 5 new rules per 'Default checkout, hold and return policy'
 6 - apply 1st patch on bug
 7 - rm installer/data/mysql/atomicupdate/220600024.pl (so we don't add again)
 8 - updatedatabase
 9 - Check DB, the extra rules have been deleted
10 - Apply 2nd patch
11 - cp installer/data/mysql/db_revs/220600024.pl installer/data/mysql/atomicupdate/
12 - rm installer/data/mysql/atomicupdate/bug_31727.pl (so we can confirm update isn't adding rules)
13 - updatedatabase
14 - Confirm no change in rules since step 9
15 - git reset --hard (get deleted update back)
16 - rm installer/data/mysql/atomicupdate/220600024.pl

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>

Created attachment 141952 [details] [review]
Bug 29012: Fix undef warnings around unseen_renewals_allowed in smart-rules.pl

Trivial fix.

To reproduce:
1. Create or edit a rule on /cgi-bin/koha/admin/smart-rules.pl - i.e. submit the form (make POST request)
2. Notice the mentioned above warning in plack-intranet-error.log
3. Apply the patch.
4. Repeat step 1.
5. That warning in log file should be no more.

Created attachment 142248 [details] [review]
Bug 29012: Fix existing update to prevent inserting incorrect data

Both updates now rely on 'suspension_chargeperiod' as the source of truth
for existence of a rule. Spot checking some existing production databases this
seems to be acceptable.

This alters the update to only add rules where a 'suspension_chargeperiod' rule exists
and a value for the rule is not already set.

NOTE: While the table has a unique key:
UNIQUE KEY `branchcode` (`branchcode`,`categorycode`,`itemtype`,`rule_name`),

This key doesn't prevent duplicates on NULLs, i.e.:

branchcode  | categorycode  | itemtype  | rule_name                 | rule_value
NULL        | NULL          | BK        | unseen_renewals_allowed   | 1
NULL        | NULL          | BK        | unseen_renewals_allowed   | 2
NULL        | NULL          | BK        | unseen_renewals_allowed   | 3

So we need to both check that any rule is present and that the specific rule is not.
The 'INSERT IGNORE' won't IGNORE existing values except for the most specific rules

Test plan:
 0 - Edit and save existing default rule in DB (sample data does not include suspension_chargeperiod)
 1 - Set some rules for "Default checkout, hold and return policy" for "Standard rule" and some specific libraries
 2 - cp installer/data/mysql/db_revs/220600024.pl installer/data/mysql/atomicupdate/
 3 - SELECT * FROM circulation rules, note the number of rules that exist
 4 - updatedatabase
 5 - Check the DB again, you should now have rules as described in the bug
     i.e. You will have 5 new rules per 'Default checkout, hold and return policy'
 6 - apply 1st patch on bug
 7 - rm installer/data/mysql/atomicupdate/220600024.pl (so we don't add again)
 8 - updatedatabase
 9 - Check DB, the extra rules have been deleted
10 - Apply 2nd patch
11 - cp installer/data/mysql/db_revs/220600024.pl installer/data/mysql/atomicupdate/
12 - rm installer/data/mysql/atomicupdate/bug_31727.pl (so we can confirm update isn't adding rules)
13 - updatedatabase
14 - Confirm no change in rules since step 9
15 - git reset --hard (get deleted update back)
16 - rm installer/data/mysql/atomicupdate/220600024.pl

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Created attachment 142249 [details] [review]
Bug 29012: Fix undef warnings around unseen_renewals_allowed in smart-rules.pl

Trivial fix.

To reproduce:
1. Create or edit a rule on /cgi-bin/koha/admin/smart-rules.pl - i.e. submit the form (make POST request)
2. Notice the mentioned above warning in plack-intranet-error.log
3. Apply the patch.
4. Repeat step 1.
5. That warning in log file should be no more.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Pushed to master for 22.11.

Nice work everyone, thanks!

Thanks!

Pushed to 21.11 for 21.11.12

(In reply to Arthur Suzuki from comment #54)
> Thanks!
> 
> Pushed to 21.11 for 21.11.12

This was reverted from 22.05, I think we should not backport this one

sorry koha-release updatebz script did update that one but it actually has been reverted.