Bug 18930

Summary: Move 'refund lost item fee rules' into the `circulation_rules` table
Product: Koha Reporter: Kyle M Hall <kyle>
Component: Architecture, internals, and plumbingAssignee: Jonathan Druart <jonathan.druart>
Status: CLOSED FIXED QA Contact: Josef Moravec <josef.moravec>
Severity: enhancement    
Priority: P5 - low CC: agustinmoyano, alexbuckley, fridolin.somers, jonathan.druart, josef.moravec, katrin.fischer, martin.renvoize, tomascohen, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=14048
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
**Reports note**: This changes the database schema, reports referencing the `refund_lost_item_fee_rules` table will need to be updated
Version(s) released in:
19.11.00
Bug Depends on: 18928    
Bug Blocks: 18936, 22284, 23323    
Attachments: Bug 18930 - Move lost item refund rules to circulation_rules table
Bug 18930 - Move lost item refund rules to circulation_rules table
Bug 18930: Move lost item refund rules to circulation_rules table
Bug 18930: Do not insert '*' in circulation_rules
Bug 18930: Move lost item refund rules to circulation_rules table
Bug 18930: Do not insert '*' in circulation_rules
Bug 18930: Move lost item refund rules to circulation_rules table
Bug 18930: Do not insert '*' in circulation_rules
Bug 18930: Move lost item refund rules to circulation_rules table
Bug 18930: Do not insert '*' in circulation_rules
Bug 18930: (follow-up) Make DB update idempotent
Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find parameters in smart-rules.pl
Bug 18930: (follow-up) Amend search parameter in Koha::RefundLostItemFeeRules->_default_rule
Bug 18930: (follow-up) Change variable used in refund select for default options
Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find
Bug 18930: (follow-up) Amend search parameter in Koha::RefundLostItemFeeRules->_default_rule
Bug 18930: (follow-up) Change variable used in refund select for default options
Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find
Bug 18930: (follow-up) Amend search parameter in Koha::RefundLostItemFeeRules->_default_rule
Bug 18930: (follow-up) Change variable used in refund select for default options
Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find
Bug 18930: (follow-up) Amend search parameter in Koha::RefundLostItemFeeRules->_default_rule
Bug 18930: (follow-up) Change variable used in refund select for default options
Bug 18930: Move lost item refund rules to circulation_rules table
Bug 18930: Do not insert '*' in circulation_rules
Bug 18930: (follow-up) Make DB update idempotent
Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find
Bug 18930: (follow-up) Amend search parameter in Koha::RefundLostItemFeeRules->_default_rule
Bug 18930: (follow-up) Change variable used in refund select for default options
Bug 18930: Move lost item refund rules to circulation_rules table
Bug 18930: Do not insert '*' in circulation_rules
Bug 18930: (follow-up) Make DB update idempotent
Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find
Bug 18930: (follow-up) Amend search parameter in Koha::RefundLostItemFeeRules->_default_rule
Bug 18930: (follow-up) Change variable used in refund select for default options
Bug 18930: (QA follow-up) Remove debug code

Description Kyle M Hall 2017-07-12 14:47:18 UTC
This patch will move the list item refund rules from a dedicated table to the circulation_rules table.
Comment 1 Kyle M Hall 2017-07-25 15:29:15 UTC
Created attachment 65243 [details] [review]
Bug 18930 - Move lost item refund rules to circulation_rules table

This patch will move the list item refund rules from a dedicated table to the circulation_rules table.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Verify lost item refund rules remain unchanged
Comment 2 Alex Buckley 2017-07-30 20:43:02 UTC
Hi Kyle 

After running updatedatabase.pl successfully when I try to load Koha it throws this load error. This also persists after dropping and recreating the db and going through the web installer: 

Software error:

Can't locate Koha/CirculationRule.pm in @INC (you may need to install the Koha::CirculationRule module) (@INC contains: /home/vagrant/kohaclone /etc/perl /usr/local/lib/x86_64-linux-gnu/perl/5.20.2 /usr/local/share/perl/5.20.2 /usr/lib/x86_64-linux-gnu/perl5/5.20 /usr/share/perl5 /usr/lib/x86_64-linux-gnu/perl/5.20 /usr/share/perl/5.20 /usr/local/lib/site_perl .) at /home/vagrant/kohaclone/Koha/RefundLostItemFeeRules.pm line 25.
BEGIN failed--compilation aborted at /home/vagrant/kohaclone/Koha/RefundLostItemFeeRules.pm line 25.
Compilation failed in require at /home/vagrant/kohaclone/C4/Circulation.pm line 54.
BEGIN failed--compilation aborted at /home/vagrant/kohaclone/C4/Circulation.pm line 54.
Compilation failed in require at /home/vagrant/kohaclone/C4/Overdues.pm line 31.
BEGIN failed--compilation aborted at /home/vagrant/kohaclone/C4/Overdues.pm line 31.
Compilation failed in require at /home/vagrant/kohaclone/C4/Members.pm line 30.
BEGIN failed--compilation aborted at /home/vagrant/kohaclone/C4/Members.pm line 30.
Compilation failed in require at /home/vagrant/kohaclone/installer/onboarding.pl line 25.
BEGIN failed--compilation aborted at /home/vagrant/kohaclone/installer/onboarding.pl line 25.
Comment 3 Alex Buckley 2017-10-04 04:01:40 UTC
Actually one of the dependencies further down the stream (this patch depends on 18928, and that in turn depends on 18925, and that in turn depends on 18887) of this patch is bug 18887 and when applying that it would not apply successfully. 

So until that is rebased against master then testing this patch will not be possible.
Comment 4 Jesse Weaver 2018-01-29 00:20:27 UTC
Created attachment 71007 [details] [review]
Bug 18930 - Move lost item refund rules to circulation_rules table

This patch will move the list item refund rules from a dedicated table to the circulation_rules table.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Verify lost item refund rules remain unchanged
Comment 5 Katrin Fischer 2018-07-11 05:54:40 UTC
Who should this be assigned to?
Comment 6 Jonathan Druart 2018-10-24 22:45:39 UTC
Created attachment 81101 [details] [review]
Bug 18930: Move lost item refund rules to circulation_rules table

This patch will move the list item refund rules from a dedicated table to the circulation_rules table.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Verify lost item refund rules remain unchanged
Comment 7 Alex Buckley 2018-10-25 04:22:32 UTC
The updatedatabase.pl fails when there are records in the lost item refund rules table. See below updatedatabase.pl output:

DEV atomic update: bug_18930.perl
DBD::mysql::db do failed: Cannot add or update a child row: a foreign key constraint fails (`koha_kohadev`.`circulation_rules`, CONSTRAINT `circ_rules_ibfk_1` FOREIGN KEY (`branchcode`) REFERENCES `branches` (`branchcode`) ON DELETE CASCADE ON UPDATE CASCADE) [for Statement "
            INSERT INTO circulation_rules ( categorycode, branchcode, itemtype, rule_name, rule_value )
            SELECT NULL, branchcode, NULL, 'refund', refund
            FROM refund_lost_item_fee_rules
        "] at (eval 1209) line 4.
Upgrade to XXX done (Bug 18930 - Move lost item refund rules to circulation_rules table)
kohadev-koha@kohadevbox:/home/vagrant/kohaclone/installer/data/mysql$ exit
Comment 8 Jonathan Druart 2018-10-25 13:17:16 UTC
Created attachment 81148 [details] [review]
Bug 18930: Do not insert '*' in circulation_rules

We must insert NULL instead, otherwise the FK constraint will fail
Comment 9 Jonathan Druart 2018-12-06 18:53:19 UTC
Created attachment 82937 [details] [review]
Bug 18930: Move lost item refund rules to circulation_rules table

This patch will move the list item refund rules from a dedicated table to the circulation_rules table.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Verify lost item refund rules remain unchanged
Comment 10 Jonathan Druart 2018-12-06 18:53:25 UTC
Created attachment 82938 [details] [review]
Bug 18930: Do not insert '*' in circulation_rules

We must insert NULL instead, otherwise the FK constraint will fail
Comment 11 Jonathan Druart 2019-02-21 20:31:46 UTC
Created attachment 85492 [details] [review]
Bug 18930: Move lost item refund rules to circulation_rules table

This patch will move the list item refund rules from a dedicated table to the circulation_rules table.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Verify lost item refund rules remain unchanged
Comment 12 Jonathan Druart 2019-02-21 20:31:51 UTC
Created attachment 85493 [details] [review]
Bug 18930: Do not insert '*' in circulation_rules

We must insert NULL instead, otherwise the FK constraint will fail
Comment 13 Tomás Cohen Arazi 2019-03-06 13:50:13 UTC
Created attachment 86169 [details] [review]
Bug 18930: Move lost item refund rules to circulation_rules table

This patch will move the list item refund rules from a dedicated table to the circulation_rules table.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Verify lost item refund rules remain unchanged

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Tomás Cohen Arazi 2019-03-06 13:50:19 UTC
Created attachment 86170 [details] [review]
Bug 18930: Do not insert '*' in circulation_rules

We must insert NULL instead, otherwise the FK constraint will fail

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 15 Tomás Cohen Arazi 2019-03-06 13:50:29 UTC
Created attachment 86171 [details] [review]
Bug 18930: (follow-up) Make DB update idempotent

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 16 Agustín Moyano 2019-05-07 03:24:41 UTC
Created attachment 89404 [details] [review]
Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find parameters in smart-rules.pl
Comment 17 Agustín Moyano 2019-05-07 03:24:45 UTC
Created attachment 89405 [details] [review]
Bug 18930: (follow-up) Amend search parameter in Koha::RefundLostItemFeeRules->_default_rule
Comment 18 Agustín Moyano 2019-05-07 03:24:50 UTC
Created attachment 89406 [details] [review]
Bug 18930: (follow-up) Change variable used in refund select for default options
Comment 19 Agustín Moyano 2019-05-07 03:26:56 UTC
(In reply to Agustín Moyano from comment #16)
> Created attachment 89404 [details] [review] [review]
> Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find parameters
> in smart-rules.pl

Added this patch because since RefundLostItemFeeRules is based on CirculationRules, find parameters should not have '*' as branchcode, and a rule_name must be specified
Comment 20 Agustín Moyano 2019-05-07 03:28:36 UTC
(In reply to Agustín Moyano from comment #17)
> Created attachment 89405 [details] [review] [review]
> Bug 18930: (follow-up) Amend search parameter in
> Koha::RefundLostItemFeeRules->_default_rule

This patch was because branchcode was '*', which for CirculationRules should be undef to find default values.
Comment 21 Agustín Moyano 2019-05-07 03:30:50 UTC
(In reply to Agustín Moyano from comment #18)
> Created attachment 89406 [details] [review] [review]
> Bug 18930: (follow-up) Change variable used in refund select for default
> options

This change was because when default rules where undefined, select value always showed 'No' option, while 'use default' options of specific branches showed 'Use default (yes)'
Comment 22 Victor Grousset/tuxayo 2019-05-24 13:59:31 UTC
Hi :)

I'm not sure I understood how to test this, here is my test plan.

1. Set "Refund lost item fee" to "no" for all libraries
2. And for a particular library
3. Apply this patch and (as suggested by git BZ) it's dependency bug 18930
4. Run updatedatabase.pl [1]
5. Check that "Refund lost item fee" is still "no" for all libraries and the particular one


[1]
koha@koha-master:~$ ~/src/installer/data/mysql/updatedatabase.pl
DEV atomic update: bug_18928.perl
Upgrade to XXX done (Bug 18928 - Move holdallowed, hold_fulfillment_policy, returnbranch to circulation_rules)
DEV atomic update: bug_18930.perl
Upgrade to XXX done (Bug 18930 - Move lost item refund rules to circulation_rules table)
Comment 23 Jonathan Druart 2019-05-24 20:47:53 UTC
Yes Victor, you can also test that the behavior is correct. You will find the test plan of the feature on bug 14048.
Comment 24 Jonathan Druart 2019-05-24 21:35:26 UTC
Comment on attachment 89404 [details] [review]
Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find parameters in smart-rules.pl

We should not need this patch. If the rule_name param is needed to make it works, something else is broken.
Comment 25 Agustín Moyano 2019-06-03 23:59:02 UTC
Created attachment 90280 [details] [review]
Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find
Comment 26 Agustín Moyano 2019-06-03 23:59:08 UTC
Created attachment 90281 [details] [review]
Bug 18930: (follow-up) Amend search parameter in Koha::RefundLostItemFeeRules->_default_rule
Comment 27 Agustín Moyano 2019-06-03 23:59:13 UTC
Created attachment 90282 [details] [review]
Bug 18930: (follow-up) Change variable used in refund select for default options
Comment 28 Agustín Moyano 2019-06-04 00:05:18 UTC
Created attachment 90283 [details] [review]
Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find
Comment 29 Agustín Moyano 2019-06-04 00:05:24 UTC
Created attachment 90284 [details] [review]
Bug 18930: (follow-up) Amend search parameter in Koha::RefundLostItemFeeRules->_default_rule
Comment 30 Agustín Moyano 2019-06-04 00:05:29 UTC
Created attachment 90285 [details] [review]
Bug 18930: (follow-up) Change variable used in refund select for default options
Comment 31 Agustín Moyano 2019-06-04 00:08:09 UTC
(In reply to Jonathan Druart from comment #24)
> Comment on attachment 89404 [details] [review] [review]
> Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find parameters
> in smart-rules.pl
> 
> We should not need this patch. If the rule_name param is needed to make it
> works, something else is broken.

Hello Jonathan, I agree with you, so I patched Koha::RefundLostItemFeeRules->find

Regards.
Comment 32 Jonathan Druart 2019-06-04 23:23:57 UTC Comment hidden (obsolete)
Comment 33 Jonathan Druart 2019-06-04 23:24:36 UTC
Agustín, can you provide tests for the changes you made please?
Comment 34 Agustín Moyano 2019-06-12 22:27:39 UTC
Created attachment 90541 [details] [review]
Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find
Comment 35 Agustín Moyano 2019-06-12 22:27:43 UTC
Created attachment 90542 [details] [review]
Bug 18930: (follow-up) Amend search parameter in Koha::RefundLostItemFeeRules->_default_rule
Comment 36 Agustín Moyano 2019-06-12 22:27:47 UTC
Created attachment 90543 [details] [review]
Bug 18930: (follow-up) Change variable used in refund select for default options
Comment 37 Agustín Moyano 2019-06-12 22:30:53 UTC
(In reply to Jonathan Druart from comment #33)
> Agustín, can you provide tests for the changes you made please?

Hi Jonathan, I just provided tests for find function.

Thanks.
Comment 38 Agustín Moyano 2019-06-12 22:33:45 UTC
Created attachment 90544 [details] [review]
Bug 18930: Move lost item refund rules to circulation_rules table

This patch will move the list item refund rules from a dedicated table to the circulation_rules table.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Verify lost item refund rules remain unchanged

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 39 Agustín Moyano 2019-06-12 22:33:49 UTC
Created attachment 90545 [details] [review]
Bug 18930: Do not insert '*' in circulation_rules

We must insert NULL instead, otherwise the FK constraint will fail

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 40 Agustín Moyano 2019-06-12 22:33:53 UTC
Created attachment 90546 [details] [review]
Bug 18930: (follow-up) Make DB update idempotent

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Signed-off-by: Agustin Moyano <agustinmoyano@theke.io>
Comment 41 Agustín Moyano 2019-06-12 22:33:57 UTC
Created attachment 90547 [details] [review]
Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find
Comment 42 Agustín Moyano 2019-06-12 22:34:02 UTC
Created attachment 90548 [details] [review]
Bug 18930: (follow-up) Amend search parameter in Koha::RefundLostItemFeeRules->_default_rule
Comment 43 Agustín Moyano 2019-06-12 22:34:06 UTC
Created attachment 90549 [details] [review]
Bug 18930: (follow-up) Change variable used in refund select for default options
Comment 44 Josef Moravec 2019-07-04 04:48:55 UTC
Created attachment 91254 [details] [review]
Bug 18930: Move lost item refund rules to circulation_rules table

This patch will move the list item refund rules from a dedicated table to the circulation_rules table.

Test Plan:
1) Apply this patch
2) Run updatedatabase.pl
3) Verify lost item refund rules remain unchanged

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 45 Josef Moravec 2019-07-04 04:48:59 UTC
Created attachment 91255 [details] [review]
Bug 18930: Do not insert '*' in circulation_rules

We must insert NULL instead, otherwise the FK constraint will fail

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 46 Josef Moravec 2019-07-04 04:49:04 UTC
Created attachment 91256 [details] [review]
Bug 18930: (follow-up) Make DB update idempotent

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

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 47 Josef Moravec 2019-07-04 04:49:08 UTC
Created attachment 91257 [details] [review]
Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 48 Josef Moravec 2019-07-04 04:49:13 UTC
Created attachment 91258 [details] [review]
Bug 18930: (follow-up) Amend search parameter in Koha::RefundLostItemFeeRules->_default_rule

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 49 Josef Moravec 2019-07-04 04:49:17 UTC
Created attachment 91259 [details] [review]
Bug 18930: (follow-up) Change variable used in refund select for default options

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 50 Josef Moravec 2019-07-04 04:49:22 UTC
Created attachment 91260 [details] [review]
Bug 18930: (QA follow-up) Remove debug code

Signed-off-by: Josef Moravec <josef.moravec@gmail.com>
Comment 51 Martin Renvoize 2019-07-15 11:21:20 UTC
Nice work!

Pushed to master for 19.11.00
Comment 52 Fridolin Somers 2019-07-30 07:44:02 UTC
Enhancement not pushed to 19.05.x