Bug 33847 - Database update replaces undefined rules with defaults rather than the value that would be used
Summary: Database update replaces undefined rules with defaults rather than the value ...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 29012
Blocks:
  Show dependency treegraph
 
Reported: 2023-05-26 15:41 UTC by Kyle M Hall
Modified: 2024-01-04 20:40 UTC (History)
8 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:
24.05.00,23.11.02,23.05.08


Attachments
Bug 33847: Database update replaces undefined rules with defaults rather than the value that would be used (5.54 KB, patch)
2023-05-26 15:49 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 33847: Database update replaces undefined rules with defaults rather than the value that would be used (5.69 KB, patch)
2023-06-14 14:48 UTC, Kevin Carnes
Details | Diff | Splinter Review
Bug 33847: Database update replaces undefined rules with defaults rather than the value that would be used (5.78 KB, patch)
2023-06-23 07:00 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 33847: Rewrite to use SQL (5.00 KB, patch)
2023-11-17 14:36 UTC, Kevin Carnes
Details | Diff | Splinter Review
Bug 33847: Database update replaces undefined rules with defaults rather than the value that would be used (5.82 KB, patch)
2023-11-30 14:37 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 33847: Rewrite to use SQL (5.07 KB, patch)
2023-11-30 14:37 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 33847: Database update replaces undefined rules with defaults rather than the value that would be used (6.83 KB, patch)
2023-12-01 08:57 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2023-05-26 15:41:57 UTC
Bug 29012 introduces a database update that sets the default values for rules that are required but undefined. This functionally changes the results of the circulation rules.

Instead, this update should find value that is being used for that rule combo and use that as the rule value, only using the default in the case that the derived rule doesn't exist or has a null value.
Comment 1 Kyle M Hall 2023-05-26 15:49:35 UTC
Created attachment 151764 [details] [review]
Bug 33847: Database update replaces undefined rules with defaults rather than the value that would be used

Bug 29012 introduces a database update that sets the default values for rules that are required but undefined. This functionally changes the results of the circulation rules.

Instead, this update should find value that is being used for that rule combo and use that as the rule value, only using the default in the case that the derived rule doesn't exist or has a null value.

Test Plan:
1) Check out Koha 22.05.05
2) Create a default all/all/all rule, 3 other rules. Ensure they all have issuelength set to 7, with one of the non-default rules having an issuelength of 14.
3) Delete all but one of the non-default rules with the following query:
   Delete from circulation_rules where rule_name  = 'issuelength' and ( issuelength != 14 or ( branchcode is null and categorycode is null and itemtype is null ) ) limit 2;
4) Check out Koha 22.05.06
4) Run updatedatabase.pl
5) Note the rules were recreated with the value 0
6) Repeat steps 1-4
7) Apply this patch
8) Run updatedatabase.pl
9) Note the rules were recreated, but the value is the derived value from the all/all/all rule!
Comment 2 Kevin Carnes 2023-06-14 14:48:59 UTC
Created attachment 152339 [details] [review]
Bug 33847: Database update replaces undefined rules with defaults rather than the value that would be used

Bug 29012 introduces a database update that sets the default values for rules that are required but undefined. This functionally changes the results of the circulation rules.

Instead, this update should find value that is being used for that rule combo and use that as the rule value, only using the default in the case that the derived rule doesn't exist or has a null value.

Test Plan:
1) Check out Koha 22.05.05
2) Create a default all/all/all rule, 3 other rules. Ensure they all have Loan period set to 7, with one of the non-default rules having a Loan period of 14.
3) Delete all but one of the non-default rules with the following query:
   Delete from circulation_rules where rule_name  = 'issuelength' and ( rule_value != 14 and not ( branchcode is null and categorycode is null and itemtype is null ) ) limit 2;
4) Check out 254f721320
5) Run updatedatabase.pl and restart
6) Note the rules were recreated with the value 0
7) Repeat steps 1-4
8) Apply this patch
9) If you're using the same database, set the version to 22.0600023 and restart
10) Run updatedatabase.pl
11) Note the rules were recreated, but the value is the derived value from the all/all/all rule!

Signed-off-by: Kevin Carnes <kevin.carnes@ub.lu.se>
Comment 3 Marcel de Rooy 2023-06-23 06:45:05 UTC
QA: Looking here
Comment 4 Marcel de Rooy 2023-06-23 07:00:42 UTC
Created attachment 152613 [details] [review]
Bug 33847: Database update replaces undefined rules with defaults rather than the value that would be used

Bug 29012 introduces a database update that sets the default values for rules that are required but undefined. This functionally changes the results of the circulation rules.

Instead, this update should find value that is being used for that rule combo and use that as the rule value, only using the default in the case that the derived rule doesn't exist or has a null value.

Test Plan:
1) Check out Koha 22.05.05
2) Create a default all/all/all rule, 3 other rules. Ensure they all have Loan period set to 7, with one of the non-default rules having a Loan period of 14.
3) Delete all but one of the non-default rules with the following query:
   Delete from circulation_rules where rule_name  = 'issuelength' and ( rule_value != 14 and not ( branchcode is null and categorycode is null and itemtype is null ) ) limit 2;
4) Check out 254f721320
5) Run updatedatabase.pl and restart
6) Note the rules were recreated with the value 0
7) Repeat steps 1-4
8) Apply this patch
9) If you're using the same database, set the version to 22.0600023 and restart
10) Run updatedatabase.pl
11) Note the rules were recreated, but the value is the derived value from the all/all/all rule!

Signed-off-by: Kevin Carnes <kevin.carnes@ub.lu.se>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 5 Marcel de Rooy 2023-06-23 07:02:09 UTC
This looks good to me for installs that did not yet pass the dbrev.
But what about installs that already got some wrong values? Could we warn or do something in a new dbrev too ?
Comment 6 Tomás Cohen Arazi 2023-06-23 12:38:20 UTC
We shouldn't be using our business classes on the DBRev, as they can change in the future and thus break this DBRev.
Comment 7 Marcel de Rooy 2023-06-23 13:23:48 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> We shouldn't be using our business classes on the DBRev, as they can change
> in the future and thus break this DBRev.

Good point
Comment 8 Katrin Fischer 2023-06-26 06:01:39 UTC
This goes as far back as 21.11.12 - we should fix asap.
Comment 9 Katrin Fischer 2023-06-30 18:40:16 UTC
So does this mean we need to find a way to do the fix only using SQL?
Comment 10 Kyle M Hall 2023-07-03 12:11:40 UTC
(In reply to Tomás Cohen Arazi from comment #6)
> We shouldn't be using our business classes on the DBRev, as they can change
> in the future and thus break this DBRev.

I'm reading this as using DBIC directly is ok. Is that correct?
Comment 11 David Cook 2023-07-18 00:21:55 UTC
(In reply to Kyle M Hall from comment #10)
> (In reply to Tomás Cohen Arazi from comment #6)
> > We shouldn't be using our business classes on the DBRev, as they can change
> > in the future and thus break this DBRev.
> 
> I'm reading this as using DBIC directly is ok. Is that correct?

I agree that we shouldn't be using business classes in the DBRev. 

I think that DBIC directly should be OK so long as only core functionality is used. (A number of DBIC classes like Suggestion.pm, Itemtype.pm, ErmAgreement.pm, et cetera have additional code added after "DO NOT MODIFY THIS OR ANYTHING ABOVE", which is added logic and not drawn directly from the database.)

In the past, Ruby on Rails used to warn about using models in DB migrations but those warnings appear to have disappeared from recent documentation: https://guides.rubyonrails.org/v3.2/migrations.html#using-models-in-your-migrations
Comment 12 Tomás Cohen Arazi 2023-07-18 12:08:19 UTC
(In reply to Katrin Fischer from comment #9)
> So does this mean we need to find a way to do the fix only using SQL?

(In reply to Kyle M Hall from comment #10)
> I'm reading this as using DBIC directly is ok. Is that correct?

Use PLAIN SQL.

Best regards
Comment 13 Tomás Cohen Arazi 2023-07-18 12:09:23 UTC
(In reply to David Cook from comment #11)
> (In reply to Kyle M Hall from comment #10)
> > (In reply to Tomás Cohen Arazi from comment #6)
> > > We shouldn't be using our business classes on the DBRev, as they can change
> > > in the future and thus break this DBRev.
> > 
> > I'm reading this as using DBIC directly is ok. Is that correct?
> 
> I agree that we shouldn't be using business classes in the DBRev. 
> 
> I think that DBIC directly should be OK so long as only core functionality
> is used. (A number of DBIC classes like Suggestion.pm, Itemtype.pm,
> ErmAgreement.pm, et cetera have additional code added after "DO NOT MODIFY
> THIS OR ANYTHING ABOVE", which is added logic and not drawn directly from
> the database.)

There will always be risks using the generated model as well. Let's stick to plain SQL.
Comment 14 Kevin Carnes 2023-11-17 14:36:04 UTC
Created attachment 159087 [details] [review]
Bug 33847: Rewrite to use SQL
Comment 15 Emily Lamancusa 2023-11-30 14:37:40 UTC
Created attachment 159410 [details] [review]
Bug 33847: Database update replaces undefined rules with defaults rather than the value that would be used

Bug 29012 introduces a database update that sets the default values for rules that are required but undefined. This functionally changes the results of the circulation rules.

Instead, this update should find value that is being used for that rule combo and use that as the rule value, only using the default in the case that the derived rule doesn't exist or has a null value.

Test Plan:
1) Check out Koha 22.05.05
2) Create a default all/all/all rule, 3 other rules. Ensure they all have Loan period set to 7, with one of the non-default rules having a Loan period of 14.
3) Delete all but one of the non-default rules with the following query:
   Delete from circulation_rules where rule_name  = 'issuelength' and ( rule_value != 14 and not ( branchcode is null and categorycode is null and itemtype is null ) ) limit 2;
4) Check out 254f721320
5) Run updatedatabase.pl and restart
6) Note the rules were recreated with the value 0
7) Repeat steps 1-4
8) Apply this patch
9) If you're using the same database, set the version to 22.0600023 and restart
10) Run updatedatabase.pl
11) Note the rules were recreated, but the value is the derived value from the all/all/all rule!

Signed-off-by: Kevin Carnes <kevin.carnes@ub.lu.se>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 16 Emily Lamancusa 2023-11-30 14:37:42 UTC
Created attachment 159411 [details] [review]
Bug 33847: Rewrite to use SQL

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 17 Emily Lamancusa 2023-11-30 14:48:52 UTC
Testing notes:
Followed the test plan from the first patch. I also tested the other four affected fields (hardduedate, unseen_renewals_allowed, rentaldiscount and decreaseloanholds), and all fields were correctly filled with derived values.

It occurs to me as I write this, though, that I only created rules that inherited from the All/All/All default rule, and didn't test that the values are derived from the *correct* rule if there are multiple related rules defined in the hierarchy. I'm going to test that case before I mark the bug as signed off.
Comment 18 Emily Lamancusa 2023-11-30 15:17:51 UTC
> It occurs to me as I write this, though, that I only created rules that
> inherited from the All/All/All default rule, and didn't test that the values
> are derived from the *correct* rule if there are multiple related rules
> defined in the hierarchy. I'm going to test that case before I mark the bug
> as signed off.

Tested issuelength with the following rules:
 -----------------------------------------------------
| Branch  | Patron category | Item type | issuelength |
 -----------------------------------------------------
| CPL     | Student         | Reference |             |
| CPL     | Student         | All       |  3          |
| CPL     | All             | Reference |  1          |
| CPL     | All             | All       |  5          |
 ------------------------------------------------------
| All     | Student         | Reference |             |
| All     | Student         | All       |  7          |
| All     | All             | Reference |  1          |
| All     | All             | All       |  5          |
 ------------------------------------------------------

The missing CPL/Student/Reference value correctly inherited the CPL/Student/All value of 3, and the missing All/Student/Reference value correctly inherited the All/Student/All value of 7.

Marking as Signed off :)
Comment 19 Marcel de Rooy 2023-12-01 08:13:09 UTC
QA: Revisiting
Comment 20 Marcel de Rooy 2023-12-01 08:13:42 UTC
 FAIL   installer/data/mysql/db_revs/220600024.pl
   FAIL   forbidden patterns
                forbidden pattern: IFNULL (must be replaced by COALESCE) (line 33)
Comment 21 Marcel de Rooy 2023-12-01 08:14:11 UTC
Probably we should allow IFNULL in the mysql folder :)
Comment 22 Marcel de Rooy 2023-12-01 08:56:04 UTC
Looks good to me. Any particular reason why you replaced the original suspension_chargeperiod by fine btw ?
Comment 23 Marcel de Rooy 2023-12-01 08:57:47 UTC
Created attachment 159428 [details] [review]
Bug 33847: Database update replaces undefined rules with defaults rather than the value that would be used

Bug 29012 introduces a database update that sets the default values for rules that are required but undefined. This functionally changes the results of the circulation rules.

Instead, this update should find value that is being used for that rule combo and use that as the rule value, only using the default in the case that the derived rule doesn't exist or has a null value.

Test Plan:
1) Check out Koha 22.05.05
2) Create a default all/all/all rule, 3 other rules. Ensure they all have Loan period set to 7, with one of the non-default rules having a Loan period of 14.
3) Delete all but one of the non-default rules with the following query:
   Delete from circulation_rules where rule_name  = 'issuelength' and ( rule_value != 14 and not ( branchcode is null and categorycode is null and itemtype is null ) ) limit 2;
4) Check out 254f721320
5) Run updatedatabase.pl and restart
6) Note the rules were recreated with the value 0
7) Repeat steps 1-4
8) Apply this patch
9) If you're using the same database, set the version to 22.0600023 and restart
10) Run updatedatabase.pl
11) Note the rules were recreated, but the value is the derived value from the all/all/all rule!

Signed-off-by: Kevin Carnes <kevin.carnes@ub.lu.se>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>

Bug 33847: Rewrite to use SQL

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
[EDIT] Squashed, and added reference to new bug too
Comment 24 Marcel de Rooy 2023-12-01 09:20:25 UTC
Btw: does <=> work for MariaDB? Yes, see:
https://mariadb.com/kb/en/null-safe-equal
Comment 25 Katrin Fischer 2023-12-19 12:44:00 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 26 Fridolin Somers 2024-01-03 15:08:15 UTC
Pushed to 23.11.x for 23.11.02
Comment 27 Lucas Gass 2024-01-04 20:40:15 UTC
Backported to 23.05.x for upcoming 23.05.08