Bug 29012 - Some rules are not saved when left blank while editing a 'rule' line in smart-rules.pl
Summary: Some rules are not saved when left blank while editing a 'rule' line in smart...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Kevin Carnes
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on:
Blocks: 31737
  Show dependency treegraph
 
Reported: 2021-09-14 12:03 UTC by Nick Clemens
Modified: 2022-10-24 09:32 UTC (History)
14 users (show)

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


Attachments
[PATCH] Bug 29012: Add default values for blank circulation rules that weren't saved to the database (5.67 KB, text/plain)
2021-12-03 09:53 UTC, Kevin Carnes
Details
Bug 29012: Add default values for blank circulation rules that weren't saved to the database (5.66 KB, patch)
2021-12-03 09:56 UTC, Kevin Carnes
Details | Diff | Splinter Review
Bug 29012: Add default values for blank circulation rules that weren't saved to the database (5.02 KB, patch)
2022-02-23 15:28 UTC, Kevin Carnes
Details | Diff | Splinter Review
Bug 29012: Add default values for blank circulation rules that weren't saved to the database (8.05 KB, patch)
2022-02-24 07:34 UTC, Kevin Carnes
Details | Diff | Splinter Review
Bug 29012: Add default values for blank circulation rules that weren't saved to the database (9.65 KB, patch)
2022-02-24 07:51 UTC, Kevin Carnes
Details | Diff | Splinter Review
Bug 29012: Add default values for blank circulation rules that weren't saved to the database (9.91 KB, patch)
2022-05-19 07:06 UTC, Kevin Carnes
Details | Diff | Splinter Review
Bug 29012: Add default values for blank circulation rules that weren't saved to the database (9.97 KB, patch)
2022-06-13 08:40 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 29012: Add default values for blank circulation rules that weren't saved to the database (9.98 KB, patch)
2022-07-26 15:27 UTC, Caroline Cyr La Rose
Details | Diff | Splinter Review
Bug 29012: Add default values for blank circulation rules that weren't saved to the database (10.04 KB, patch)
2022-07-29 12:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29012 (QA follow-up) Use q{} instead of '' in smart-rules.pl (5.51 KB, patch)
2022-07-29 12:18 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 29012: Add default values for blank circulation rules that weren't saved to the database (13.54 KB, patch)
2022-10-14 09:20 UTC, Kevin Carnes
Details | Diff | Splinter Review
Revert "Bug 29012: DBRev 22.06.00.024" (731 bytes, patch)
2022-10-14 12:23 UTC, Kevin Carnes
Details | Diff | Splinter Review
Revert "Bug 29012: (QA follow-up) Use q{} instead of '' in smart-rules.pl" (5.70 KB, patch)
2022-10-14 12:24 UTC, Kevin Carnes
Details | Diff | Splinter Review
Revert "Bug 29012: Add default values for blank circulation rules that weren't saved to the database" (9.10 KB, patch)
2022-10-14 12:24 UTC, Kevin Carnes
Details | Diff | Splinter Review
Bug 29012: Add default values for blank circulation rules that weren't saved to the database (13.54 KB, patch)
2022-10-14 12:25 UTC, Kevin Carnes
Details | Diff | Splinter Review
Bug 29012: Fix existing update to prevent inserting incorrect data (4.11 KB, patch)
2022-10-17 06:27 UTC, Kevin Carnes
Details | Diff | Splinter Review
Bug 29012: Fix undef warnings around unseen_renewals_allowed in smart-rules.pl (1.48 KB, patch)
2022-10-17 06:28 UTC, Kevin Carnes
Details | Diff | Splinter Review
Bug 29012: Fix existing update to prevent inserting incorrect data (4.17 KB, patch)
2022-10-20 14:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29012: Fix undef warnings around unseen_renewals_allowed in smart-rules.pl (1.54 KB, patch)
2022-10-20 14:20 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-09-14 12:03:44 UTC
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
Comment 1 Kevin Carnes 2021-12-03 09:53:49 UTC Comment hidden (obsolete)
Comment 2 Kevin Carnes 2021-12-03 09:56:17 UTC Comment hidden (obsolete)
Comment 3 Kevin Carnes 2021-12-03 10:07:16 UTC
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.
Comment 4 Jonathan Druart 2022-01-19 13:13:45 UTC
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.
Comment 5 Kevin Carnes 2022-01-20 12:46:48 UTC
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.
Comment 6 Kevin Carnes 2022-02-16 09:29:52 UTC
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?
Comment 7 Nick Clemens 2022-02-22 13:52:48 UTC
(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
Comment 8 Kevin Carnes 2022-02-23 15:28:27 UTC Comment hidden (obsolete)
Comment 9 Kevin Carnes 2022-02-23 15:30:38 UTC
(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?
Comment 10 Kevin Carnes 2022-02-24 07:34:48 UTC Comment hidden (obsolete)
Comment 11 Kevin Carnes 2022-02-24 07:51:37 UTC Comment hidden (obsolete)
Comment 12 Kevin Carnes 2022-02-24 07:53:12 UTC
(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.
Comment 13 Lucas Gass 2022-05-18 21:28:39 UTC Comment hidden (obsolete)
Comment 14 Kevin Carnes 2022-05-19 07:06:06 UTC Comment hidden (obsolete)
Comment 15 Martin Renvoize 2022-06-13 08:40:37 UTC Comment hidden (obsolete)
Comment 16 Caroline Cyr La Rose 2022-06-30 14:59:33 UTC
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.
Comment 17 Kevin Carnes 2022-07-01 13:00:41 UTC
(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.
Comment 18 Caroline Cyr La Rose 2022-07-26 15:04:40 UTC
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
Comment 19 Caroline Cyr La Rose 2022-07-26 15:27:05 UTC Comment hidden (obsolete)
Comment 20 Caroline Cyr La Rose 2022-07-26 15:27:45 UTC
It worked on my local installation :)
Comment 21 Kyle M Hall 2022-07-29 12:18:12 UTC Comment hidden (obsolete)
Comment 22 Kyle M Hall 2022-07-29 12:18:23 UTC
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>
Comment 23 Tomás Cohen Arazi 2022-07-29 18:51:16 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 24 Lucas Gass 2022-09-29 14:35:13 UTC
Backported to 22.05.x for 22.05.06
Comment 25 Andrew Nugged 2022-10-10 14:15:06 UTC
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.
Comment 26 Andrew Nugged 2022-10-10 14:21:15 UTC
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!
Comment 27 Andrew Nugged 2022-10-10 14:21:58 UTC
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).
Comment 28 Katrin Fischer 2022-10-10 15:12:51 UTC
(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!
Comment 29 Andrew Nugged 2022-10-10 15:26:26 UTC
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 :).
Comment 30 Andrew Nugged 2022-10-10 15:27:05 UTC
(In reply to Katrin Fischer from comment #28)

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

yes, creating
Comment 31 Victor Grousset/tuxayo 2022-10-13 15:19:52 UTC
> 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.
Comment 32 Victor Grousset/tuxayo 2022-10-13 15:23:08 UTC
22.05 and 21.11 rmaints got and email about reverting this
Comment 33 Andrew Nugged 2022-10-13 16:57:09 UTC
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).
Comment 34 Lucas Gass 2022-10-13 17:35:39 UTC
This patchset has been reverted from 22.05.x
Comment 35 Kevin Carnes 2022-10-14 09:20:31 UTC
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
Comment 36 Kevin Carnes 2022-10-14 09:28:54 UTC
(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.
Comment 37 Slava Shishkin 2022-10-14 12:11:17 UTC
Kevin, I have conflicts applying to the current master. Can you check?
Comment 38 Andrew Nugged 2022-10-14 12:18:33 UTC
Slava/Kevin: I think it's because Tomás not yet reverted the master from old 29012.
Comment 39 Kevin Carnes 2022-10-14 12:23:38 UTC
Created attachment 141876 [details] [review]
Revert "Bug 29012: DBRev 22.06.00.024"

This reverts commit 51cef8c0ec1eca244ebafb853a9164bde65941cb.
Comment 40 Kevin Carnes 2022-10-14 12:24:33 UTC
Created attachment 141877 [details] [review]
Revert "Bug 29012: (QA follow-up) Use q{} instead of '' in smart-rules.pl"

This reverts commit 6f8e77b4e899d6f71fb8987975ba916a65dc8229.
Comment 41 Kevin Carnes 2022-10-14 12:24:58 UTC
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.
Comment 42 Kevin Carnes 2022-10-14 12:25:19 UTC
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
Comment 43 Kevin Carnes 2022-10-14 12:27:05 UTC
(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.
Comment 44 Slava Shishkin 2022-10-14 12:42:25 UTC
Good, thanks.
Comment 45 Nick Clemens 2022-10-14 12:53:30 UTC
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?
Comment 46 Kevin Carnes 2022-10-14 13:03:16 UTC
(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.
Comment 47 Tomás Cohen Arazi 2022-10-14 18:56:51 UTC
(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!
Comment 48 Nick Clemens 2022-10-14 18:57:50 UTC
(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
Comment 49 Kevin Carnes 2022-10-17 06:27:48 UTC
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>
Comment 50 Kevin Carnes 2022-10-17 06:28:14 UTC
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.
Comment 51 Tomás Cohen Arazi 2022-10-20 14:20:19 UTC
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>
Comment 52 Tomás Cohen Arazi 2022-10-20 14:20:27 UTC
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>
Comment 53 Tomás Cohen Arazi 2022-10-20 14:44:41 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 54 Arthur Suzuki 2022-10-22 22:07:28 UTC
Thanks!

Pushed to 21.11 for 21.11.12
Comment 55 Nick Clemens 2022-10-23 14:24:44 UTC
(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
Comment 56 Arthur Suzuki 2022-10-24 08:07:44 UTC
sorry koha-release updatebz script did update that one but it actually has been reverted.