Bug 31727 - Orphan records in circulation_rules and false positives after attempt to fix values to be empty or zeroes instead of missing
Summary: Orphan records in circulation_rules and false positives after attempt to fix ...
Status: CLOSED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low critical (vote)
Assignee: Kevin Carnes
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2022-10-10 16:08 UTC by Andrii Nugged
Modified: 2023-06-08 22:26 UTC (History)
9 users (show)

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


Attachments
Bug 31727: Orphan records in circulation_rules and false positives after attempt to fix values to be empty or zeroes instead of missing (2.42 KB, patch)
2022-10-11 07:25 UTC, Kevin Carnes
Details | Diff | Splinter Review
Bug 31727: Orphan records in circulation_rules and false positives after attempt to fix values to be empty or zeroes instead of missing (2.47 KB, patch)
2022-10-11 12:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31727: Fix existing update to prevent inserting incorrect data (4.11 KB, patch)
2022-10-11 12:56 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 31727 - Database query results from testing (17.83 KB, text/plain)
2022-10-11 20:25 UTC, David Nind
Details
Bug 31727: Remove unseenrenewalsallowed rules that duplicate unseen_renewals_allowed rules (2.03 KB, patch)
2022-10-12 12:59 UTC, Kevin Carnes
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Andrii Nugged 2022-10-11 06:15:20 UTC Comment hidden (obsolete)
Comment 3 Kevin Carnes 2022-10-11 07:24:10 UTC
Thank for catching these problems. I made the false assumption that the database table was only for the first table of rules and I used the template name instead of the database column name for unseen_renewals_allowed.

I've made an atomic update that removes these rules when there is no suspension_chargeperiod rule (this is a rule that should be made when saving the first table of rules). It also renames any remaining unseenrenewalsallowed to unseen_renewals_allowed.

It would be helpful if somebody else could check this to make sure it's right. It's possible that suspension_chargeperiod is not a accurate check or it might be better to only delete them if the still have the default values. All corrections are appreciated.
Comment 4 Kevin Carnes 2022-10-11 07:25:45 UTC
Created attachment 141595 [details] [review]
Bug 31727: Orphan records in circulation_rules and false positives after attempt to fix values to be empty or zeroes instead of missing
Comment 5 Andrii Nugged 2022-10-11 08:22:39 UTC
(I rewrote the ticket description because I lost one table "3" new in master):

On smart-rules.pl (Home > Administration > Circulation and fine rules):
we have 8 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. Default / Waiting hold cancellation policy
       (... for XXX branch, if branch selected)
    4. Checkout, hold policy by patron category 
       (... for XXX branch, if branch selected)
    5. Daily open article requests limit
       (... for XXX branch, if branch selected)
    6. Article request fees
       (... for XXX branch, if branch selected)
    7. Lost item fee refund on return policy
       (... for XXX branch, if branch selected)
    8. 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.

Attempt to fix missing values in Bug 29012 had the atomic update, which filled up the circulation_rules table with values if ANY combination of "branch+category+itemtype" was found in the circulation_rules table. This had false positives for items from above-mentioned table 2..8 when assumed that only key/value pairs from table 1 should be those which affect the decision to add or not" new empty or zeroed item.

This made the problem: after this atomic update, we have the circulation_rules table randomly filled with orphan elements for "table 1" even if no rules in table 1 for that branch were present in real.

Worse: those orphan items are never shown as rows of real rules for the operator so not visible.

Bad: those orphan items, for example, "issuelength" of 0 when it has some BRANCH overlaps ANY rule without branches and discretely changes behavior when issuing items.

EXAMPLE:
- Open smart-rules.pl and choose in the first dropdown "Select a library : " some BRANCH_NAME so this page will reload into BRANCH_NAME-targeted mode for the whole page.
- Edit the second table "Default checkout, hold and return policy for BRANCH_NAME" making value holdallowed to be set into something, and save it. You will see / still have "table 1" empty. At this time key/value pair will be added to the database for this BRANCH_NAME, and this is exactly what later will mislead that atomic update:
- make the atomic update from Bug 29012 to fill those "zeroes and empties"
- check the smart-rules.pl page for BRANCH_NAME-targeted mode: there will still be no rules in the 1st, "Defining circulation and fine rules for XXX branch" table,
- but now check the database in raw mode: you will see that there will be a record: BRANCH_NAME will have, for example, "issuelength" of 0 key/value pair.
- OUCH!


What to fix:

Now we need to invent a new atomic update to be applied after this one (and this is very non-trivial because we somehow need to preserve other proper values), to fix the database from orphan values. This anyway can lead to very unpredictable results.

Another thing to consider is that we should even if unsure what is orphans - is to reveal all hidden values, i.e. append and populate other values to make a "full row" appear for the operator in the interface, so at least human can SEE wrong with those rules and then decide/delete but make impossible to have "remnants" and orphans hidden.

PROBLEM 2:

   unseen_renewals_allowed
vs
   unseenrenewalsallowed

(sic!)

we have 'unseen_renewals_allowed' everywhere in the codebase, except in this atomic update we have 'unseenrenewalsallowed'. And now DB is polluted even more :).
This also should be fixed - and more: we might now have unseen_renewals_allowed and unseenrenewalsallowed both in database.
Comment 6 Andrii Nugged 2022-10-11 08:23:55 UTC
(In reply to Kevin Carnes from comment #4)

ok good, let us check these requests/atomic on our live "happened to be" productions...
Comment 7 Nick Clemens 2022-10-11 12:56:47 UTC
Created attachment 141600 [details] [review]
Bug 31727: Orphan records in circulation_rules and false positives after attempt to fix values to be empty or zeroes instead of missing

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 8 Nick Clemens 2022-10-11 12:56:51 UTC
Created attachment 141601 [details] [review]
Bug 31727: 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 9 David Nind 2022-10-11 20:25:05 UTC
Created attachment 141618 [details]
Bug 31727 - Database query results from testing

I've had a go at testing (using KTD).

After step 9, I have one extra rule - I'm not sure if this is an issue.

If it isn't an issue, I'm happy to sign off.

What I did:

1. Changed default circulation and fines rule:
   - Suspension in days (day): 2
   - Max. suspension duration (day): 5
   - Suspension charging interval: 1

2. Default checkout, hold and return policy
   - Standard rule for all libraries:
     . Total current checkouts allowed: 20
     . Total current on-site checkouts allowed: 20
     . Maximum total holds allowed (count): 25
     . Hold policy: From any library
     . Hold pickup library match: Not set
     . Return policy: Not set
  
3. Created "Default checkout, hold and return policy" rules for Franklin, Midway, and Springfield:
   - Just used different numbers for the first 3 settings
   - Set hold policy differently for all three

4. Result for step 3 - 47 rows/rules [see attachment].

5. Result for step 5 - now have 63 rows/rules: 47 + 15 (3 libraries x 5 extra rules) + 1 (extra rule for unseenrenewalsallowed for NULL - see row 51) [see attachment].

6. Result for step 9 - now have 48 row/rules: still have one extra rule - row 51 [see attachment].
Comment 10 Andrii Nugged 2022-10-12 06:49:36 UTC
I did some selection of key/value pairs, their allowances of values, and spreading over tables on smart-rules.pl page:

https://wiki.koha-community.org/wiki/Circulation_rules_values_tech_list

This might be helpful for analysis (Kevin, others)
Comment 11 Kevin Carnes 2022-10-12 12:59:26 UTC
Created attachment 141725 [details] [review]
Bug 31727: Remove unseenrenewalsallowed rules that duplicate unseen_renewals_allowed rules
Comment 12 Kevin Carnes 2022-10-12 13:08:32 UTC
(In reply to David Nind from comment #9)
> Created attachment 141618 [details]
> Bug 31727 - Database query results from testing
> 
> I've had a go at testing (using KTD).
> 
> After step 9, I have one extra rule - I'm not sure if this is an issue.
> 

That extra rule is because I used the wrong name for the unseen_renewals_allowed rule. So if there were already rules that had an unseen_renewals_allowed, they now also have an unseenrenewalsallowed rule since it is added when there is no "unseenrenewalsallowed" rule.

I think the solution is to remove "unseenrenewalsallowed" rules when there is a similar "unseen_renewals_allowed" rule, before renaming the remaining "unseenrenewalsallowed" to "unseen_renewals_allowed".

Another option, especially now that you have an extra duplicate, is to remove an "unseen_renewals_allowed" rule when then are two for same parameters.
Comment 13 Victor Grousset/tuxayo 2022-10-13 15:23:07 UTC
bug 29012 hasn't been released. It's in the 22.05 and 21.11 branches but not released yet.

22.05 and 21.11 rmaints got and email about reverting 29012
Comment 14 Victor Grousset/tuxayo 2022-10-13 15:23:36 UTC
> bug 29012 hasn't been released

So I don't know if that changes the strategy for this bug.
Comment 15 Nick Clemens 2022-10-13 15:30:34 UTC
(In reply to Victor Grousset/tuxayo from comment #14)
> > bug 29012 hasn't been released
> 
> So I don't know if that changes the strategy for this bug.

It means we don't need to provide the cleanup patch, but should simply confirm the amended update patches prevent any problems
Comment 16 Andrii Nugged 2022-10-13 16:45:37 UTC
(In reply to Victor Grousset/tuxayo from comment #14)
> > bug 29012 hasn't been released
> 
> So I don't know if that changes the strategy for this bug.

We SHOULD change strategy, and that's great - this means we don't need to make complicated "reversal atomic updates" for the older wrong atomic update which was very complicated and anyway dangerous. That's great.
Comment 17 Andrii Nugged 2022-10-13 16:53:16 UTC
this ticket is not needed anymore because the contents of 29012 will be obsoleted and re-done.


to Kevin Carnes:

let's go back to Bug 29012 and obsolete everything there and make again a full new solution taking the idea but do that now as we now know.