|
Description
Kyle M Hall (khall)
2017-07-12 14:47:18 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 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. 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. 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 Who should this be assigned to? 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 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
Created attachment 81148 [details] [review] Bug 18930: Do not insert '*' in circulation_rules We must insert NULL instead, otherwise the FK constraint will fail 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 Created attachment 82938 [details] [review] Bug 18930: Do not insert '*' in circulation_rules We must insert NULL instead, otherwise the FK constraint will fail 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 Created attachment 85493 [details] [review] Bug 18930: Do not insert '*' in circulation_rules We must insert NULL instead, otherwise the FK constraint will fail 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> 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> Created attachment 86171 [details] [review] Bug 18930: (follow-up) Make DB update idempotent Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Created attachment 89404 [details] [review] Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find parameters in smart-rules.pl Created attachment 89405 [details] [review] Bug 18930: (follow-up) Amend search parameter in Koha::RefundLostItemFeeRules->_default_rule Created attachment 89406 [details] [review] Bug 18930: (follow-up) Change variable used in refund select for default options (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 (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. (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)' 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) Yes Victor, you can also test that the behavior is correct. You will find the test plan of the feature on bug 14048. 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. Created attachment 90280 [details] [review] Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find Created attachment 90281 [details] [review] Bug 18930: (follow-up) Amend search parameter in Koha::RefundLostItemFeeRules->_default_rule Created attachment 90282 [details] [review] Bug 18930: (follow-up) Change variable used in refund select for default options Created attachment 90283 [details] [review] Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find Created attachment 90284 [details] [review] Bug 18930: (follow-up) Amend search parameter in Koha::RefundLostItemFeeRules->_default_rule Created attachment 90285 [details] [review] Bug 18930: (follow-up) Change variable used in refund select for default options (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. Augustin, can you provide tests for the changes you made please? Agustín, can you provide tests for the changes you made please? Created attachment 90541 [details] [review] Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find Created attachment 90542 [details] [review] Bug 18930: (follow-up) Amend search parameter in Koha::RefundLostItemFeeRules->_default_rule Created attachment 90543 [details] [review] Bug 18930: (follow-up) Change variable used in refund select for default options (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. 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> 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> 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> Created attachment 90547 [details] [review] Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find Created attachment 90548 [details] [review] Bug 18930: (follow-up) Amend search parameter in Koha::RefundLostItemFeeRules->_default_rule Created attachment 90549 [details] [review] Bug 18930: (follow-up) Change variable used in refund select for default options 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> 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> 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> Created attachment 91257 [details] [review] Bug 18930: (follow-up) Amend Koha::RefundLostItemFeeRules->find Signed-off-by: Josef Moravec <josef.moravec@gmail.com> 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> 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> Created attachment 91260 [details] [review] Bug 18930: (QA follow-up) Remove debug code Signed-off-by: Josef Moravec <josef.moravec@gmail.com> Nice work! Pushed to master for 19.11.00 Enhancement not pushed to 19.05.x |