[Bug 31727] New: Orphan records in circulation_rules and false positives after attempt to fix values to be empty or zeroes instead of missing
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 Bug ID: 31727 Summary: Orphan records in circulation_rules and false positives after attempt to fix values to be empty or zeroes instead of missing Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: critical Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: nugged@gmail.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com Blocks: 29012 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. 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,3,4,5 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 last (5th) table "Holds policy by item type for BRANCH_NAME" value holdallowed to be set into something, and save it. You will see / still have "table 1" empty. - 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 be still 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. Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29012 [Bug 29012] Some rules are not saved when left blank while editing a 'rule' line in smart-rules.pl -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 --- Comment #1 from Andrew Nugged <nugged@gmail.com> --- On smart-rules.pl (Home > Administration > Circulation and fine rules): we have 7 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. Daily open article requests limit (... for XXX branch, if branch selected) 5. Article request fees (... for XXX branch, if branch selected) 6. Lost item fee refund on return policy (... for XXX branch, if branch selected) 7. 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..7 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 --- Comment #2 from Andrew Nugged <nugged@gmail.com> --- Here I preselected from the code which rule_name's we have (indented column), one can double-check me. This was handy for me to fix databases after the "noise-making" session, but I think we should create an atomic-update normalizer which will wipe out "the noise" from the database (but operators will be surprised! :) ): Defining circulation and fine rules for all libraries Defining circulation and fine rules for "BRANCH_NAME" table 1: article_requests auto_renew cap_fine_to_replacement_price chargeperiod chargeperiod_charge_at daysmode decreaseloanholds fine finedays firstremind hardduedate hardduedatecompare holds_per_day holds_per_record issuelength lengthunit maxissueqty maxonsiteissueqty maxsuspensiondays no_auto_renewal_after no_auto_renewal_after_hard_limit norenewalbefore note on_shelf_recalls onshelfholds opacitemholds overduefinescap recall_due_date_interval recall_overdue_fine recall_shelf_time recalls_allowed recalls_per_record renewalperiod renewalsallowed rentaldiscount reservesallowed suspension_chargeperiod unseen_renewals_allowed Default checkout, hold and return policy Default checkout, hold and return policy for BRANCH_NAME table 2: set-branch-defaults hold_fulfillment_policy holdallowed max_holds patron_maxissueqty patron_maxonsiteissueqty returnbranch Default checkout, hold policy by patron category Checkout, hold policy by patron category for BRANCH_NAME table 3: add-branch-cat max_holds patron_maxissueqty patron_maxonsiteissueqty Default open article requests limit Daily open article requests limit for BRANCH_NAME table 4: add-open-article-requests-limit open_article_requests_limit Default article request fees Article request fees for BRANCH_NAME table 5: set-article-request-fee article_request_fee Default lost item fee refund on return policy Lost item fee refund on return policy for BRANCH_NAME table 6: mod-refund-lost-item-fee-rule lostreturn Default holds policy by item type Holds policy by item type for BRANCH_NAME table 7: set-waiting-hold-cancellation waiting_hold_cancellation -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 Kevin Carnes <kevin.carnes@ub.lu.se> changed: What |Removed |Added ---------------------------------------------------------------------------- Assignee|koha-bugs@lists.koha-commun |kevin.carnes@ub.lu.se |ity.org | CC| |kevin.carnes@ub.lu.se --- Comment #3 from Kevin Carnes <kevin.carnes@ub.lu.se> --- 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. -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 --- Comment #4 from Kevin Carnes <kevin.carnes@ub.lu.se> --- Created attachment 141595 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=141595&action=edit Bug 31727: Orphan records in circulation_rules and false positives after attempt to fix values to be empty or zeroes instead of missing -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 Kevin Carnes <kevin.carnes@ub.lu.se> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |In Discussion -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 --- Comment #5 from Andrew Nugged <nugged@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 --- Comment #6 from Andrew Nugged <nugged@gmail.com> --- (In reply to Kevin Carnes from comment #4) ok good, let us check these requests/atomic on our live "happened to be" productions... -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|In Discussion |Needs Signoff -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #141595|0 |1 is obsolete| | --- Comment #7 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 141600 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=141600&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 --- Comment #8 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 141601 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=141601&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |katrin.fischer@bsz-bw.de, | |martin.renvoize@ptfs-europe | |.com, | |nick@bywatersolutions.com, | |tomascohen@gmail.com -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 David Nind <david@davidnind.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |david@davidnind.com --- Comment #9 from David Nind <david@davidnind.com> --- Created attachment 141618 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=141618&action=edit 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]. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 --- Comment #10 from Andrew Nugged <nugged@gmail.com> --- 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) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 --- Comment #11 from Kevin Carnes <kevin.carnes@ub.lu.se> --- Created attachment 141725 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=141725&action=edit Bug 31727: Remove unseenrenewalsallowed rules that duplicate unseen_renewals_allowed rules -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 --- Comment #12 from Kevin Carnes <kevin.carnes@ub.lu.se> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 Victor Grousset/tuxayo <victor@tuxayo.net> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |victor@tuxayo.net --- Comment #13 from Victor Grousset/tuxayo <victor@tuxayo.net> --- 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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 --- Comment #14 from Victor Grousset/tuxayo <victor@tuxayo.net> ---
bug 29012 hasn't been released
So I don't know if that changes the strategy for this bug. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 --- Comment #15 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 --- Comment #16 from Andrew Nugged <nugged@gmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |RESOLVED Resolution|--- |INVALID --- Comment #17 from Andrew Nugged <nugged@gmail.com> --- 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks|29012 | Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=29012 [Bug 29012] Some rules are not saved when left blank while editing a 'rule' line in smart-rules.pl -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=31727 Andrew Nugged <nugged@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=29012 -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org