There is not foreign key constraint on the column budget_parent_id in the table aqbudget. It is so possible to delete a budget without deleting the associated fund.
(In reply to Baptiste from comment #0) > There is not foreign key constraint on the column budget_parent_id in the > table aqbudget. > It is so possible to delete a budget without deleting the associated fund. 2835 DROP TABLE IF EXISTS `aqbudgets`; 2836 CREATE TABLE `aqbudgets` ( -- information related to Funds 2837 `budget_id` int(11) NOT NULL auto_increment, -- primary key and unique number assigned to each fund by Koha 2838 `budget_parent_id` int(11) default NULL, -- if this fund is a child of another this will include the parent id (aqbudgets.budget_id) ... 2850 `budget_owner_id` int(11) default NULL, -- borrowernumber of the person who owns this fund (borrowers.borrowernumber) 2851 `budget_permission` int(1) default '0', -- level of permission for this fund (used only by the owner, only by the library, or anyone) 2852 PRIMARY KEY (`budget_id`), 2853 KEY `budget_parent_id` (`budget_parent_id`), 2854 KEY `budget_code` (`budget_code`), 2855 KEY `budget_branchcode` (`budget_branchcode`), 2856 KEY `budget_period_id` (`budget_period_id`), 2857 KEY `budget_owner_id` (`budget_owner_id`) 2858 ) ENGINE=InnoDB DEFAULT CHARSET=utf8mb4 COLLATE=utf8mb4_unicode_ci; budget_parent_id could have a constraint on aqbudgetperiods.budget_period_id budget_owner_id should probably be an FK on borrowers.borrowernumber Both should be still allowed to be empty (NULL), as they don't need to always be set. Then we got: budget_id = fund ID budget_parent_id = used for child/sub funds to link them to the parent. I am not sure if you can have a FK constraint on another column in the same table? I can add the changes to the kohastructure and via updatedatabase, but not sure what 'false' date we might expect to clean up here.
Created attachment 110076 [details] [review] Bug 18050: Add FK constraint on aqbudgets.budget_period_id This adds a FK constraint on aqbudgets.budget_period_id so that a fund cannot be added with an invalid aqbudget.budget_period_id. We should not have funds that belong to no budget. In case we have, the update will be skipped and a note displayed. Part1: - Before applying the patch - Make sure you have a budget with some funds linked to it - You will have to change one of the funds to link to an invalid budget with SQL: UPDATE aqbudgets SET budget_period_id = 999 WHERE budget_id = max(budget_id); - Apply patch - Run updatedatabase - verify that you see the hint about 1 existing fund with invalid budget. - Repair your fund with SQL UPDATE aqbudgets SET budget_period_id = ... WHERE budget_id = max(budget_id); (... needs to be your existing budget_period_id) - Run updatedatabase again - verify it runs successfully now. - If you try to change the budget_period_id to a non-existing now with SQL, you will get a database error. The new FK doesn't permit it. Part 2: - Start fresh with the web installer, verify there are no errors on creating the database tables
Created attachment 110078 [details] [review] Bug 18050: Add FK constraint on aqbudgets.budget_period_id This adds a FK constraint on aqbudgets.budget_period_id so that a fund cannot be added with an invalid aqbudget.budget_period_id. We should not have funds that belong to no budget. In case we have, the update will be skipped and a note displayed. Part1: - Before applying the patch - Make sure you have a budget with some funds linked to it - You will have to change one of the funds to link to an invalid budget with SQL: UPDATE aqbudgets SET budget_period_id = 999 WHERE budget_id = max(budget_id); - Apply patch - Run updatedatabase - verify that you see the hint about 1 existing fund with invalid budget. - Repair your fund with SQL UPDATE aqbudgets SET budget_period_id = ... WHERE budget_id = max(budget_id); (... needs to be your existing budget_period_id) - Run updatedatabase again - verify it runs successfully now. - If you try to change the budget_period_id to a non-existing now with SQL, you will get a database error. The new FK doesn't permit it. Part 2: - Start fresh with the web installer, verify there are no errors on creating the database tables
Created attachment 110079 [details] [review] Bug 18050: [Do not push] Schema update
Created attachment 110080 [details] [review] Bug 18050: Add relation alias to schema
Created attachment 110169 [details] [review] Bug 18050: Add FK constraint on aqbudgets.budget_period_id This adds a FK constraint on aqbudgets.budget_period_id so that a fund cannot be added with an invalid aqbudget.budget_period_id. We should not have funds that belong to no budget. In case we have, the update will be skipped and a note displayed. Part1: - Before applying the patch - Make sure you have a budget with some funds linked to it - You will have to change one of the funds to link to an invalid budget with SQL: UPDATE aqbudgets SET budget_period_id = 999 WHERE budget_id = max(budget_id); - Apply patch - Run updatedatabase - verify that you see the hint about 1 existing fund with invalid budget. - Repair your fund with SQL UPDATE aqbudgets SET budget_period_id = ... WHERE budget_id = max(budget_id); (... needs to be your existing budget_period_id) - Run updatedatabase again - verify it runs successfully now. - If you try to change the budget_period_id to a non-existing now with SQL, you will get a database error. The new FK doesn't permit it. Part 2: - Start fresh with the web installer, verify there are no errors on creating the database tables Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 110170 [details] [review] Bug 18050: [Do not push] Schema update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 110171 [details] [review] Bug 18050: Add relation alias to schema Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
You've done a really nice comprehensive job here Katrin, thanks. Signing off, it all looks good and works as expected from my perspective. Minor point however.. this bug used to cover lots of other FK issues around this table too.. I agree with narrowing the scope, but it would be good to open bugs for the other parts, especially as you've taken the time to understand them a bit as shown by your first comment (not expecting you to take on adding them however.. just would be good to have them recorded somewhere before they get lost when this bug is pushed ;) )
I think there is something wrong here: if database contains orphan budget, the updatedatabase will warn about it and go on. One can see the warning and fix it, but there will be no easy way to re-run the update, or one can miss the warning and continue using koha with a wrong schema. That's not the first time I see this pattern, and I think there is a problem with that. What's the point of updatedatabase if it doesn't ensure that the schema is correct at the end ? I think the updatedatabase should just die here, let the admin fix the problem and re-run the updatedatabase process.
(In reply to Julian Maurice from comment #10) > I think there is something wrong here: if database contains orphan budget, > the updatedatabase will warn about it and go on. > One can see the warning and fix it, but there will be no easy way to re-run > the update, > or one can miss the warning and continue using koha with a wrong schema. > > That's not the first time I see this pattern, and I think there is a problem > with that. What's the point of updatedatabase if it doesn't ensure that the > schema is correct at the end ? > I think the updatedatabase should just die here, let the admin fix the > problem and re-run the updatedatabase process. We cannot do anything else in this case. When there are orphan funds (not budget here), then there might be orders linked to this fund - we cannot just delete them. We need to leave it up to the library to fix this. Our updatedatabase mechanism doesn't allow to stop and rerun (as far as I know?) and I think using the existing pattern with a warn is the best we can do here right now and it should not be put on this bug report to fix the underlying issue :)
(In reply to Katrin Fischer from comment #11) > (In reply to Julian Maurice from comment #10) > > I think there is something wrong here: if database contains orphan budget, > > the updatedatabase will warn about it and go on. > > One can see the warning and fix it, but there will be no easy way to re-run > > the update, > > or one can miss the warning and continue using koha with a wrong schema. > > > > That's not the first time I see this pattern, and I think there is a problem > > with that. What's the point of updatedatabase if it doesn't ensure that the > > schema is correct at the end ? > > I think the updatedatabase should just die here, let the admin fix the > > problem and re-run the updatedatabase process. > > We cannot do anything else in this case. When there are orphan funds (not > budget here), then there might be orders linked to this fund - we cannot > just delete them. We need to leave it up to the library to fix this. > > Our updatedatabase mechanism doesn't allow to stop and rerun (as far as I > know?) and I think using the existing pattern with a warn is the best we can > do here right now and it should not be put on this bug report to fix the > underlying issue :) Can't we just die ? In an atomic update file it won't stop the process because the file is eval'd, but once it is in updatedatabase.pl it can stop the process. And if we die before changing the Version in systempreferences, next time updatedatabase is run it will restart from where it died.
I cannot attend the dev meeting this week, but I hope we can find a way to move forward with these kind of fixes.
As discussed in the dev meeting I think this bug should be unblocked whilst we work on the issues with updatedatabse discussed on bug 25078. We should sort out all the existing warn cases that fall into this category at the same time on that bug and not hold up an important improvement for many.
Great job Katrin.. if I may add a comment, in the atomic update, if there are some budget periods that were not found, instead of not adding the FK constraint, I would: 1. Create a temporary table.. something like _bug_18050_aqbudgets where I would copy the offending rows 2. Update aqbudgets.budget_period_id to null, add the FK constraint 3. Emit the warning that some budget periods where not found, and a copy of those rows where preserved in _bug_18050_aqbudgets table If it's ok to you, I can add a follow-up with those steps
Created attachment 111429 [details] [review] Bug 18050: (follow-up) Force adding FK constraint When there are inconsistent budget_period_id in aqbudgets this patch creates the table _bug_18050_aqbudgets with the original data, then sets the column to null and warns the user that there is inconsistent data.
Hi Katrin, I added the follow-up just in case you'd like to check it out when you've got time. Please feel free to remove it if you feel it's not ok. Thanks
(In reply to Agustín Moyano from comment #15) > Great job Katrin.. if I may add a comment, in the atomic update, if there > are some budget periods that were not found, instead of not adding the FK > constraint, I would: > > 1. Create a temporary table.. something like _bug_18050_aqbudgets where I > would copy the offending rows > > 2. Update aqbudgets.budget_period_id to null, add the FK constraint > > 3. Emit the warning that some budget periods where not found, and a copy of > those rows where preserved in _bug_18050_aqbudgets table > > If it's ok to you, I can add a follow-up with those steps Hi Augustin, it's not as simple as that I am afraid. If there are orphan aqbudgets, there could also be orders attached to them. And if there are orders, there could be linked items... the rabbit hole is deeper. Maybe another better approach could be to go the other direction: We add a aqbudgetperiods entry - this would also allow the library to deal with things in the GUI. I am not sure I am able to write that one, so if anyone beats me to it I'd be more than happy.
Augustin, I think I misunderstood initially what you are trying to do. I thought we were copying the data out and then removing it from the original table - but instead we set the id to NULL and just keep the former information. I will give this a test run.
Created attachment 111712 [details] [review] Bug 18050: (follow-up) Force adding FK constraint When there are inconsistent budget_period_id in aqbudgets this patch creates the table _bug_18050_aqbudgets with the original data, then sets the column to null and warns the user that there is inconsistent data. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 111713 [details] [review] Bug 18050: (QA follow-up) Adjust conditions and make use of message text - When we run into invalid data, we use Augustin's suggested solution and copy the data into a separate table, setting the budget_period_id to NULL. - We amend the output using the new $message. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 111751 [details] [review] Bug 18050: Add FK constraint on aqbudgets.budget_period_id This adds a FK constraint on aqbudgets.budget_period_id so that a fund cannot be added with an invalid aqbudget.budget_period_id. We should not have funds that belong to no budget. In case we have, the update will be skipped and a note displayed. Part1: - Before applying the patch - Make sure you have a budget with some funds linked to it - You will have to change one of the funds to link to an invalid budget with SQL: UPDATE aqbudgets SET budget_period_id = 999 WHERE budget_id = max(budget_id); - Apply patch - Run updatedatabase - verify that you see the hint about 1 existing fund with invalid budget. - Repair your fund with SQL UPDATE aqbudgets SET budget_period_id = ... WHERE budget_id = max(budget_id); (... needs to be your existing budget_period_id) - Run updatedatabase again - verify it runs successfully now. - If you try to change the budget_period_id to a non-existing now with SQL, you will get a database error. The new FK doesn't permit it. Part 2: - Start fresh with the web installer, verify there are no errors on creating the database tables Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 111752 [details] [review] Bug 18050: [Do not push] Schema update Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 111753 [details] [review] Bug 18050: Add relation alias to schema Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 111754 [details] [review] Bug 18050: (follow-up) Force adding FK constraint When there are inconsistent budget_period_id in aqbudgets this patch creates the table _bug_18050_aqbudgets with the original data, then sets the column to null and warns the user that there is inconsistent data. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Created attachment 111755 [details] [review] Bug 18050: (QA follow-up) Adjust conditions and make use of message text - When we run into invalid data, we use Augustin's suggested solution and copy the data into a separate table, setting the budget_period_id to NULL. - We amend the output using the new $message. Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Pushed to master for 20.11, thanks to everybody involved!
Created attachment 112529 [details] [review] Bug 18050: move aqbudgetperiods table before aqbudgets, to fix sql error 00:02:16.310 koha_1 | DBD::mysql::st execute failed: Can't create table `koha_kohadev`.`aqbudgets` (errno: 150 "Foreign key constraint is incorrectly formed") at /usr/share/perl5/DBIx/RunSQL.pm line 278, <$args{...}> line 1. 00:02:16.310 koha_1 | Something went wrong loading file /kohadevbox/koha/installer/data/mysql/kohastructure.sql ([SQL ERROR]: CREATE TABLE `aqbudgets` ( -- information related to Funds
Created attachment 112530 [details] [review] follow-up pushed to master. Thanks !
Sorry for missing this and thx for the fix!
backported to 20.05.x for 20.05.06
backported to 19.11.x for 19.11.12
Backported to 19.05.x branch for 19.05.17