Sponsored by Halland County Library This patch is a requirement to Bug #12771. Instead of keeping 'branchcode' and 'categorytype' in the table overduerules_transport_types, a foreign key named 'overduerules_fk' now references the appropriate entry in the table overduerules. This will be necessary once we start managing more than 3 levels of overdue notice.
Created attachment 35561 [details] [review] Remove columns branchcode, categorytype from table overduerules_transport_types Sponsored by Halland County Library This patch is a _requirement_ to Bug #12771, and will be necessary once we start managing more than 3 levels of overdue notice In the database : - A new column and primary key, 'overduerules_id', is added to the table 'overduerules' - A new column referencing a foreign key, 'overduerules_id', is added to the table 'overduerules_transport_types' - The columns 'branchcode' and 'categorytype' are removed from the table 'overduerules_transport_types' In C4/Overdues.pm : - The SQL query of the function 'GetOverdueMessageTransportTypes' is changed to take into account the new design of 'overduerules_transport_types' In t/db_dependent/Overdues.t : - The INSERT calls before the tests were changed to take into account the new design of 'overduerules' and 'overduerules_transport_types' TEST PLAN --------- 1) Apply the patch 2) Run the script installer/data/mysql/atomicupdate/bug_13624_overduerules_transport_types.pl - It will create new tables and migrate the data 3) Run Overdues.t. Tests should be successful
Quick note: 1/ You should add the DB changes to the kohastructure.sql file 2/ This patch should not be pushed as it, it will break tools/overduerules.pl If I am not wrong, could you confirm that and explain how you would like to see the patches integrated into master? You have just said "this patch is a requirement to bug 12771".
Created attachment 35587 [details] [review] [PATCH] Bug #13624: Remove columns branchcode, categorytype from table overduerules_transport_types Sponsored by Halland County Library This patch is a _requirement_ to Bug #12771, and will be necessary once we start managing more than 3 levels of overdue notice In installer/data/mysql/atomicupdate/bug_13624_overduerules_transport_types.pl - A new column and primary key, 'overduerules_id', is added to the table 'overduerules' - A new column referencing a foreign key, 'overduerules_id', is added to the table 'overduerules_transport_types' - The columns 'branchcode' and 'categorytype' are removed from the table 'overduerules_transport_types' - Data is migrated from the old tables to the new ones In installer/data/mysql/kohastructure.sql : - Table 'overduerules' - Added a primary key named 'overduerules_id' - Table 'overduerules_transport_types' - Added a foreign key named 'overduerules_id' - Dropped columns 'branchcode', 'categorycode' since we now have them referenced through 'overduerules_id' In tools/overduerules.pl - INSERT and DELETE queries on 'overduerules_transport_types' were changed to reflect the new schema In C4/Overdues.pm : - The SQL query of the function 'GetOverdueMessageTransportTypes' is changed to take into account the new design of 'overduerules_transport In t/db_dependent/Overdues.t : - The INSERT calls before the tests were changed to take into account the new design of 'overduerules' and 'overduerules_transport_types' TEST PLAN --------- 1) Apply the patch 2) Run the script installer/data/mysql/atomicupdate/bug_13624_overduerules_transport_types.pl - It will create new tables and migrate the data 3) Run Overdues.t. Tests should be successful modified: C4/Overdues.pm new file: installer/data/mysql/atomicupdate/bug_13624_overduerules_transport_types.pl modified: installer/data/mysql/kohastructure.sql modified: t/db_dependent/Overdues.t modified: tools/overduerules.pl
(In reply to Jonathan Druart from comment #2) > Quick note: > 1/ You should add the DB changes to the kohastructure.sql file > 2/ This patch should not be pushed as it, it will break tools/overduerules.pl > If I am not wrong, could you confirm that and explain how you would like to > see the patches integrated into master? > You have just said "this patch is a requirement to bug 12771". 1) The changes to kohastructure.sql were added in the newest patch 2) Indeed, tools/overduerules.pl had to be slightly changed in order to make things work. The new patch should fix any issues with that file. I'm submitting #13624 separate from #12771 in order to make 12771's patch a little lighter. The patch I just submitted can be applied as is. Does it answer your question?
(In reply to Charles Farmer from comment #4) > (In reply to Jonathan Druart from comment #2) > > Quick note: > > 1/ You should add the DB changes to the kohastructure.sql file > > 2/ This patch should not be pushed as it, it will break tools/overduerules.pl > > If I am not wrong, could you confirm that and explain how you would like to > > see the patches integrated into master? > > You have just said "this patch is a requirement to bug 12771". > > 1) The changes to kohastructure.sql were added in the newest patch > 2) Indeed, tools/overduerules.pl had to be slightly changed in order to make > things work. The new patch should fix any issues with that file. > > I'm submitting #13624 separate from #12771 in order to make 12771's patch a > little lighter. The patch I just submitted can be applied as is. Does it > answer your question? Perfect, thanks!
$ mysql MariaDB [koha]> drop database koha; MariaDB [koha]> create database koha CHARACTER SET utf8 COLLATE utf8_unicode_ci; $ git checkout master $ mysql < installer/data/mysql/kohastructure.sql $ git bz apply 13624 $ perl installer/data/mysql/atomicupdate/bug_13624_overduerules_transport_types.pl DBD::mysql::db do failed: Duplicate foreign key constraint name 'koha/overduerules_fk' at installer/data/mysql/atomicupdate/bug_13624_overduerules_transport_types.pl line 33.
(In reply to Jonathan Druart from comment #6) > DBD::mysql::db do failed: Duplicate foreign key constraint name > 'koha/overduerules_fk' at > installer/data/mysql/atomicupdate/bug_13624_overduerules_transport_types.pl > line 33. I changed the foreign key name and placed the changes in updatedatabase.pl instead of a stand alone file. I hope this solves the problem.
Created attachment 36122 [details] [review] [PATCH] Bug #13624: Remove columns branchcode, categorytype from table overduerules_transport_types Sponsored by Halland County Library This patch is a _requirement_ to Bug #12771, and will be necessary once we start managing more than 3 levels of overdue notice In installer/data/mysql/updatedatabase.pl - A new column and primary key, 'overduerules_id', is added to the table 'overduerules' - A new column referencing a foreign key, 'overduerules_id', is added to the table 'overduerules_transport_types' - The columns 'branchcode' and 'categorytype' are removed from the table 'overduerules_transport_types' - Data is migrated from the old tables to the new ones In installer/data/mysql/kohastructure.sql : - Table 'overduerules' - Added a primary key named 'overduerules_id' - Table 'overduerules_transport_types' - Added a foreign key named 'overduerules_id' - Dropped columns 'branchcode', 'categorycode' since we now have them referenced through 'overduerules_id' In tools/overduerules.pl - INSERT and DELETE queries on 'overduerules_transport_types' were changed to reflect the new schema In C4/Overdues.pm : - The SQL query of the function 'GetOverdueMessageTransportTypes' is changed to take into account the new design of 'overduerules_transport In t/db_dependent/Overdues.t : - The INSERT calls before the tests were changed to take into account the new design of 'overduerules' and 'overduerules_transport_types' TEST PLAN --------- 1) Apply the patch 2) Run the script installer/data/mysql/updatedatabase.pl - It will create new tables and migrate the data 3) Run Overdues.t. Tests should be successful
I received a CONFLICT while trying to apply this patch. -Jesse CONFLICT (content): Merge conflict in installer/data/mysql/updatedatabase.pl Failed to merge in the changes. Patch failed at 0001 Bug #13624: Remove columns branchcode, categorytype from table overduerules_transport_types
Created attachment 36494 [details] [review] Bug #13624: Remove columns branchcode, categorytype from table overduerules_transport_types Sponsored by Halland County Library This patch is a _requirement_ to Bug #12771, and will be necessary once we start managing more than 3 levels of overdue notice In installer/data/mysql/updatedatabase.pl - A new column and primary key, 'overduerules_id', is added to the table 'overduerules' - A new column referencing a foreign key, 'overduerules_id', is added to the table 'overduerules_transport_types' - The columns 'branchcode' and 'categorytype' are removed from the table 'overduerules_transport_types' - Data is migrated from the old tables to the new ones In installer/data/mysql/kohastructure.sql : - Table 'overduerules' - Added a primary key named 'overduerules_id' - Table 'overduerules_transport_types' - Added a foreign key named 'overduerules_id' - Dropped columns 'branchcode', 'categorycode' since we now have them referenced through 'overduerules_id' In tools/overduerules.pl - INSERT and DELETE queries on 'overduerules_transport_types' were changed to reflect the new schema In C4/Overdues.pm : - The SQL query of the function 'GetOverdueMessageTransportTypes' is changed to take into account the new design of 'overduerules_transport In t/db_dependent/Overdues.t : - The INSERT calls before the tests were changed to take into account the new design of 'overduerules' and 'overduerules_transport_types' TEST PLAN --------- 1) Apply the patch 2) Run the script installer/data/mysql/updatedatabase.pl - It will create new tables and migrate the data 3) Run Overdues.t. Tests should be successful
Created attachment 36497 [details] [review] Bug #13624: Remove columns branchcode, categorytype from table overduerules_transport_types Sponsored by Halland County Library This patch is a _requirement_ to Bug #12771, and will be necessary once we start managing more than 3 levels of overdue notice In installer/data/mysql/updatedatabase.pl - A new column and primary key, 'overduerules_id', is added to the table 'overduerules' - A new column referencing a foreign key, 'overduerules_id', is added to the table 'overduerules_transport_types' - The columns 'branchcode' and 'categorytype' are removed from the table 'overduerules_transport_types' - Data is migrated from the old tables to the new ones In installer/data/mysql/kohastructure.sql : - Table 'overduerules' - Added a primary key named 'overduerules_id' - Table 'overduerules_transport_types' - Added a foreign key named 'overduerules_id' - Dropped columns 'branchcode', 'categorycode' since we now have them referenced through 'overduerules_id' In tools/overduerules.pl - INSERT and DELETE queries on 'overduerules_transport_types' were changed to reflect the new schema In C4/Overdues.pm : - The SQL query of the function 'GetOverdueMessageTransportTypes' is changed to take into account the new design of 'overduerules_transport In t/db_dependent/Overdues.t : - The INSERT calls before the tests were changed to take into account the new design of 'overduerules' and 'overduerules_transport_types' TEST PLAN --------- 1) Apply the patch 2) Run the script installer/data/mysql/updatedatabase.pl - It will create new tables and migrate the data 3) Run Overdues.t. Tests should be successful Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
1/ On updating the overdue rules (overduerules.pl), I got: overduerules.pl: DBD::mysql::st execute failed: Column count doesn't match value count at row 1 at /home/koha/src/tools/overduerules.pl line 188. 2/ Provide a complete test plan 3/ The new DB column has to be inserted at the same place as in the kohastructure.sql file. Marked as Failed QA.
Created attachment 37074 [details] [review] Bugs 13624 - Test plan and fixes db scripts for overduerules migrations TEST PLAN 1) Create or improve on overduerules data .1) Intranet -> tools -> overdue notices .2) Make sure to have data in first, second and third tabs .3) Make sure to have checked mixes of Email/Phone/Print/SMS (depending on availability) .4) Make sure to have different letters and delay on first/second/third tab for at least one category .5) Remember it all. 2) Apply patch 3) run installer/data/mysql/updatedatabase.pl 4) run t/db_dependant/Overdues.t 5) Validate Overdue Notice page .1) validate data entered previously is still there .2) Add some more, save changes, validate This patch also - simplifies (somewhat) updatedatabase.pl - modifies kohastructure.sql to match updatedatabase.pl (because I couldn't 'insert after' with a KEY constraint) - fixes database call
Created attachment 38288 [details] [review] Bug 13624 - Remove columns branchcode, categorytype from table overduerules_transport_types This patch is a _requirement_ to Bug #12771, and will be necessary once we start managing more than 3 levels of overdue notice TEST PLAN 1) Create or improve on overduerules data .1) Intranet -> tools -> overdue notices .2) Make sure to have data in first, second and third tabs .3) Make sure to have checked mixes of Email/Phone/Print/SMS (depending on availability) .4) Make sure to have different letters and delay on first/second/third tab for at least one category .5) Remember it all. 2) Apply patch 3) run installer/data/mysql/atomicupdate/update_13624_overduerules_transport_type.pl 4) run t/db_dependant/Overdues.t 5) Validate Overdue Notice page .1) validate data entered previously is still there .2) Add some more, save changes, validate In installer/data/mysql/atomicupdate/update_13624_overduerules_transport_type.pl - A new column and primary key, 'overduerules_id', is added to the table 'overduerules' - A new column referencing a foreign key, 'overduerules_id', is added to the table 'overduerules_transport_types' - The columns 'branchcode' and 'categorytype' are removed from the table 'overduerules_transport_types' - Data is migrated from the old tables to the new ones In installer/data/mysql/kohastructure.sql : - Table 'overduerules' - Added a primary key named 'overduerules_id' - Table 'overduerules_transport_types' - Added a foreign key named 'overduerules_id' - Dropped columns 'branchcode', 'categorycode' since we now have them referenced through 'overduerules_id' In tools/overduerules.pl - INSERT and DELETE queries on 'overduerules_transport_types' were changed to reflect the new schema In C4/Overdues.pm : - The SQL query of the function 'GetOverdueMessageTransportTypes' is changed to take into account the new design of 'overduerules_transport In t/db_dependent/Overdues.t : - The INSERT calls before the tests were changed to take into account the new design of 'overduerules' and 'overduerules_transport_types' Sponsored by : Halland County Library
Created attachment 38289 [details] [review] Bug 13624 - Correcting the atomicupdate/ file. Dev-only patch
There was merging issues with updatedatabase.pl. Now replaced by new method using atomicupdate/ directory. I hope it all works well.
Created attachment 38290 [details] [review] Bug 13624 - Remove columns branchcode, categorytype from table overduerules_transport_types This patch is a _requirement_ to Bug #12771, and will be necessary once we start managing more than 3 levels of overdue notice TEST PLAN 1) Create or improve on overduerules data .1) Intranet -> tools -> overdue notices .2) Make sure to have data in first, second and third tabs .3) Make sure to have checked mixes of Email/Phone/Print/SMS (depending on availability) .4) Make sure to have different letters and delay on first/second/third tab for at least one category .5) Remember it all. 2) Apply patch 3) run installer/data/mysql/atomicupdate/update_13624_overduerules_transport_type.pl 4) run t/db_dependant/Overdues.t 5) Validate Overdue Notice page .1) validate data entered previously is still there .2) Add some more, save changes, validate In installer/data/mysql/atomicupdate/update_13624_overduerules_transport_type.pl - A new column and primary key, 'overduerules_id', is added to the table 'overduerules' - A new column referencing a foreign key, 'overduerules_id', is added to the table 'overduerules_transport_types' - The columns 'branchcode' and 'categorytype' are removed from the table 'overduerules_transport_types' - Data is migrated from the old tables to the new ones In installer/data/mysql/kohastructure.sql : - Table 'overduerules' - Added a primary key named 'overduerules_id' - Table 'overduerules_transport_types' - Added a foreign key named 'overduerules_id' - Dropped columns 'branchcode', 'categorycode' since we now have them referenced through 'overduerules_id' In tools/overduerules.pl - INSERT and DELETE queries on 'overduerules_transport_types' were changed to reflect the new schema In C4/Overdues.pm : - The SQL query of the function 'GetOverdueMessageTransportTypes' is changed to take into account the new design of 'overduerules_transport In t/db_dependent/Overdues.t : - The INSERT calls before the tests were changed to take into account the new design of 'overduerules' and 'overduerules_transport_types' Sponsored by : Halland County Library Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Comment on attachment 38290 [details] [review] Bug 13624 - Remove columns branchcode, categorytype from table overduerules_transport_types Review of attachment 38290 [details] [review]: ----------------------------------------------------------------- Could you please tidy the atomicupdate file a bit? (trailing space, uppercase the mysql kw, etc.) ::: installer/data/mysql/atomicupdate/update_13624_overduerules_transport_type.pl @@ +10,5 @@ > +#if ( CheckVersion($DBversion) ) { > + $dbh->do("SET FOREIGN_KEY_CHECKS=0"); > + $dbh->do("ALTER TABLE overduerules RENAME old_overduerules"); > + $dbh->do("CREATE TABLE overduerules ( > + `overduerules_id` mediumint NOT NULL AUTO_INCREMENT, Why mediumint? @@ +25,5 @@ > + `debarred3` int(1) DEFAULT '0', > + PRIMARY KEY (`overduerules_id`) > + ) ENGINE=InnoDB DEFAULT CHARSET=utf8;"); > + $dbh->do("INSERT INTO overduerules(branchcode, categorycode, delay1, letter1, debarred1, delay2, debarred2, letter2, delay3, letter3, debarred3) SELECT * FROM old_overduerules"); > + $dbh->do("DROP TABLE old_overduerules"); Is not possible to just add the primary key to the existing table? ::: installer/data/mysql/kohastructure.sql @@ -1713,4 @@ > `delay3` int(4) default NULL, -- number of days after the item is overdue that the third notice is sent > `letter3` varchar(20) default NULL, -- foreign key from the letter table to define which notice should be sent as the third notice > `debarred3` int(1) default 0, -- is the patron restricted when the third notice is sent (1 for yes, 0 for no) > - PRIMARY KEY (`branchcode`,`categorycode`) I think we should keep this constraint too.
(In reply to Jonathan Druart from comment #18) > ::: installer/data/mysql/kohastructure.sql > @@ -1713,4 @@ > > `delay3` int(4) default NULL, -- number of days after the item is overdue that the third notice is sent > > `letter3` varchar(20) default NULL, -- foreign key from the letter table to define which notice should be sent as the third notice > > `debarred3` int(1) default 0, -- is the patron restricted when the third notice is sent (1 for yes, 0 for no) > > - PRIMARY KEY (`branchcode`,`categorycode`) > > I think we should keep this constraint too. This is not clear. I meant we should keep branchcode and categorycode as a unique key.
Hi Jonathan, I'll try to reply to these to the best of my knowledge 1) tidying : I'm on it, but I couldn't find much beside 3-4 keywords. No extra space, as this code had been through qa-test before. 2) mediumint : no valid reason whatsoever, reasoning prb lost in time, along with the initial coder. I'll change to int(11) 3) simple ALTER ADD : Well, I remembered trying, so I tried again and could not. The existing constraints, the relation to OTT, the auto_increment... Whatever I tried failed. I won't say it's "simpler this way", but I just couldn't make it work otherwise. 4) Took me a while to remember WHY we want it out. I guess it's premature, but this is the first step to allow for unlimited "steps", and as such that combo cannot be unique. BUT this being the first step, I cannot justify removing it yet, only that it will be more complicated to remove later. I WILL LEAVE IT IN, you being QA and all ;) Mini Patch coming...
Created attachment 39920 [details] [review] Bug 13624 - Tidying the atomicupdate file and kohastructure.sql
Applying: Bug 13624 - Tidying the atomicupdate file and kohastructure.sql fatal: sha1 information is lacking or useless (installer/data/mysql/atomicupdate/update_13624_overduerules_transport_type.pl). Please rebase.
This applies just fine for me off of master.
Sorry but: % git fetch origin % git checkout -b bug_13624 origin/master % git clean -f -d % git bz apply 13624 Bug 13624 - Remove columns branchcode, categorytype from table overduerules_transport_types 38290 - Bug 13624 - Remove columns branchcode, categorytype from table overduerules_transport_types 39920 - Bug 13624 - Tidying the atomicupdate file and kohastructure.sql Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 13624 - Remove columns branchcode, categorytype from table overduerules_transport_types Applying: Bug 13624 - Tidying the atomicupdate file and kohastructure.sql fatal: sha1 information is lacking or useless (installer/data/mysql/atomicupdate/update_13624_overduerules_transport_type.pl). Repository lacks necessary blobs to fall back on 3-way merge. Cannot fall back to three-way merge. Please reupload the patches if they apply for you.
Created attachment 40329 [details] [review] Bug 13624 - Remove columns branchcode, categorytype from table overduerules_transport_types This patch is a _requirement_ to Bug #12771, and will be necessary once we start managing more than 3 levels of overdue notice TEST PLAN 1) Create or improve on overduerules data .1) Intranet -> tools -> overdue notices .2) Make sure to have data in first, second and third tabs .3) Make sure to have checked mixes of Email/Phone/Print/SMS (depending on availability) .4) Make sure to have different letters and delay on first/second/third tab for at least one category .5) Remember it all. 2) Apply patch 3) run installer/data/mysql/atomicupdate/update_13624_overduerules_transport_type.pl 4) run t/db_dependant/Overdues.t 5) Validate Overdue Notice page .1) validate data entered previously is still there .2) Add some more, save changes, validate In installer/data/mysql/atomicupdate/update_13624_overduerules_transport_type.pl - A new column and primary key, 'overduerules_id', is added to the table 'overduerules' - A new column referencing a foreign key, 'overduerules_id', is added to the table 'overduerules_transport_types' - The columns 'branchcode' and 'categorytype' are removed from the table 'overduerules_transport_types' - Data is migrated from the old tables to the new ones In installer/data/mysql/kohastructure.sql : - Table 'overduerules' - Added a primary key named 'overduerules_id' - Table 'overduerules_transport_types' - Added a foreign key named 'overduerules_id' - Dropped columns 'branchcode', 'categorycode' since we now have them referenced through 'overduerules_id' In tools/overduerules.pl - INSERT and DELETE queries on 'overduerules_transport_types' were changed to reflect the new schema In C4/Overdues.pm : - The SQL query of the function 'GetOverdueMessageTransportTypes' is changed to take into account the new design of 'overduerules_transport In t/db_dependent/Overdues.t : - The INSERT calls before the tests were changed to take into account the new design of 'overduerules' and 'overduerules_transport_types' Sponsored by : Halland County Library Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com>
Created attachment 40330 [details] [review] Bug 13624 - Tidying the atomicupdate file and kohastructure.sql
Except for Jonathan's comments, I've uploaded my patches, and they are identical. I don't see any differences. Not sure what status it should be, but I'll throw it to needs sign off instead of "Patch doesn't apply".
Created attachment 41684 [details] [review] Bug 13624 - Remove columns branchcode, categorytype from table overduerules_transport_types This patch is a _requirement_ to Bug #12771, and will be necessary once we start managing more than 3 levels of overdue notice TEST PLAN 1) Create or improve on overduerules data .1) Intranet -> tools -> overdue notices .2) Make sure to have data in first, second and third tabs .3) Make sure to have checked mixes of Email/Phone/Print/SMS (depending on availability) .4) Make sure to have different letters and delay on first/second/third tab for at least one category .5) Remember it all. 2) Apply patch 3) run installer/data/mysql/atomicupdate/update_13624_overduerules_transport_type.pl 4) run t/db_dependant/Overdues.t 5) Validate Overdue Notice page .1) validate data entered previously is still there .2) Add some more, save changes, validate In installer/data/mysql/atomicupdate/update_13624_overduerules_transport_type.pl - A new column and primary key, 'overduerules_id', is added to the table 'overduerules' - A new column referencing a foreign key, 'overduerules_id', is added to the table 'overduerules_transport_types' - The columns 'branchcode' and 'categorytype' are removed from the table 'overduerules_transport_types' - Data is migrated from the old tables to the new ones In installer/data/mysql/kohastructure.sql : - Table 'overduerules' - Added a primary key named 'overduerules_id' - Table 'overduerules_transport_types' - Added a foreign key named 'overduerules_id' - Dropped columns 'branchcode', 'categorycode' since we now have them referenced through 'overduerules_id' In tools/overduerules.pl - INSERT and DELETE queries on 'overduerules_transport_types' were changed to reflect the new schema In C4/Overdues.pm : - The SQL query of the function 'GetOverdueMessageTransportTypes' is changed to take into account the new design of 'overduerules_transport In t/db_dependent/Overdues.t : - The INSERT calls before the tests were changed to take into account the new design of 'overduerules' and 'overduerules_transport_types' Sponsored by : Halland County Library Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com> Signed-off-by: Martin Persson <xarragon@gmail.com>
Created attachment 41685 [details] [review] Bug 13624 - Tidying the atomicupdate file and kohastructure.sql Signed-off-by: Martin Persson <xarragon@gmail.com>
Tested these patches against origin/master commit 75d123d760d86f0e0d9f58d65874b40f6816008d. Both patches merged perfectly and both manual and automated tests passed. Tested with 3 categories and all tabs filled with complex, unique combinations of rules. Verified by screenshot before and after. Code looks good to me but I am unfamiliar with some SQL details.
(In reply to Blou from comment #20) > 4) Took me a while to remember WHY we want it out. I guess it's premature, > but this is the first step to allow for unlimited "steps", and as such that > combo cannot be unique. BUT this being the first step, I cannot justify > removing it yet, only that it will be more complicated to remove later. > I WILL LEAVE IT IN, you being QA and all ;) Sorry, but I think that the constraint should not be removed in this patchset.
From what I see, the last patch *did* keep branchode and category code as a unique key for table overduerules. Unless there are more things to change, this does not warrant any more discussion. Since both available patches have been signed off by Marting Persson, I'm putting this to "Signed off".
(In reply to Charles Farmer from comment #32) > From what I see, the last patch *did* keep branchode and category code as a > unique key for table overduerules. Unless there are more things to change, > this does not warrant any more discussion. > > Since both available patches have been signed off by Marting Persson, I'm > putting this to "Signed off". Do you confirm that the QA concerns have been taken into account, the patches work as advertised and you have tested them?
(In reply to Jonathan Druart from comment #33) To better make progress, I would need to know precisely what you mean by "QA concerns". After reading this thread through and through and opening the most recent patches, I think the only concern that wasn't explicitly addressed, about the UNIQUE key, was covered by the patch. Also, prove was run against t/db_dependant/Overdues.t without errors. There is, however, one odd thing: atomicupdates are only considered by updatedatabase.pl if the file extension is .perl or .sql, not .pl. I'll follow up this post with a new patch that fixes this problem.
Created attachment 43524 [details] [review] Bug 13624 - renaming atomicupdate from .pl to .perl
Created attachment 43571 [details] [review] Bug 13624 - renaming atomicupdate from .pl to .perl I have not tested this very thoroughly e.g. understood the SQL changes etc. since the patch has been signed off twice before. But it seems to me like this patch is OK. Applied, ran automatic tests and saved some values in the overduerules matrix/tabs OK. Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com>
Created attachment 43583 [details] [review] Bug 13624 - Remove columns branchcode, categorytype from table overduerules_transport_types This patch is a _requirement_ to Bug #12771, and will be necessary once we start managing more than 3 levels of overdue notice TEST PLAN 1) Create or improve on overduerules data .1) Intranet -> tools -> overdue notices .2) Make sure to have data in first, second and third tabs .3) Make sure to have checked mixes of Email/Phone/Print/SMS (depending on availability) .4) Make sure to have different letters and delay on first/second/third tab for at least one category .5) Remember it all. 2) Apply patch 3) run installer/data/mysql/atomicupdate/update_13624_overduerules_transport_type.pl 4) run t/db_dependant/Overdues.t 5) Validate Overdue Notice page .1) validate data entered previously is still there .2) Add some more, save changes, validate In installer/data/mysql/atomicupdate/update_13624_overduerules_transport_type.pl - A new column and primary key, 'overduerules_id', is added to the table 'overduerules' - A new column referencing a foreign key, 'overduerules_id', is added to the table 'overduerules_transport_types' - The columns 'branchcode' and 'categorytype' are removed from the table 'overduerules_transport_types' - Data is migrated from the old tables to the new ones In installer/data/mysql/kohastructure.sql : - Table 'overduerules' - Added a primary key named 'overduerules_id' - Table 'overduerules_transport_types' - Added a foreign key named 'overduerules_id' - Dropped columns 'branchcode', 'categorycode' since we now have them referenced through 'overduerules_id' In tools/overduerules.pl - INSERT and DELETE queries on 'overduerules_transport_types' were changed to reflect the new schema In C4/Overdues.pm : - The SQL query of the function 'GetOverdueMessageTransportTypes' is changed to take into account the new design of 'overduerules_transport In t/db_dependent/Overdues.t : - The INSERT calls before the tests were changed to take into account the new design of 'overduerules' and 'overduerules_transport_types' Sponsored by : Halland County Library Signed-off-by: Jesse Maseto <jesse@bywatersolutions.com> Signed-off-by: Martin Persson <xarragon@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 43584 [details] [review] Bug 13624 - Tidying the atomicupdate file and kohastructure.sql Signed-off-by: Martin Persson <xarragon@gmail.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Created attachment 43585 [details] [review] Bug 13624 - renaming atomicupdate from .pl to .perl I have not tested this very thoroughly e.g. understood the SQL changes etc. since the patch has been signed off twice before. But it seems to me like this patch is OK. Applied, ran automatic tests and saved some values in the overduerules matrix/tabs OK. Signed-off-by: Eivin Giske Skaaren <eivin@sysmystic.com> Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Pushed to master, thanks Philippe!
(In reply to Kyle M Hall from comment #40) > Pushed to master, thanks Philippe! Please regenerate the Schema files, some tests are failing.
(In reply to Jonathan Druart from comment #41) > (In reply to Kyle M Hall from comment #40) > > Pushed to master, thanks Philippe! > > Please regenerate the Schema files, some tests are failing. Which tests? I ran update_dbix_class_files.pl and nothing changed in my index...
(In reply to Charles Farmer from comment #42) > (In reply to Jonathan Druart from comment #41) > > (In reply to Kyle M Hall from comment #40) > > > Pushed to master, thanks Philippe! > > > > Please regenerate the Schema files, some tests are failing. > > Which tests? I ran update_dbix_class_files.pl and nothing changed in my > index... It was a RM note, it has been fixed by bug 15478.
On a fresh DB I am having issues with the loss of the 'letternumber' column overdue_notices.pl throws errors for unknown column, I can't save triggers I think the last line below was an accidental deletion? diff --git a/installer/data/mysql/kohastructure.sql b/installer/data/mysql/kohastructure.sql ... @@ -2568,12 +2569,10 @@ CREATE TABLE `message_transport_types` ( DROP TABLE IF EXISTS `overduerules_transport_types`; CREATE TABLE overduerules_transport_types( `id` INT(11) NOT NULL AUTO_INCREMENT, - `branchcode` varchar(10) NOT NULL DEFAULT '', - `categorycode` VARCHAR(10) NOT NULL DEFAULT '', - `letternumber` INT(1) NOT NULL DEFAULT 1,
Comment on attachment 38288 [details] [review] Bug 13624 - Remove columns branchcode, categorytype from table overduerules_transport_types This is the obsoleted patch where it accidentally disappeared.
(In reply to Nick Clemens from comment #44) > On a fresh DB I am having issues with the loss of the 'letternumber' column > overdue_notices.pl throws errors for unknown column, I can't save triggers > I think the last line below was an accidental deletion? Yes. I agree with that assessment.
(In reply to Nick Clemens from comment #44) > On a fresh DB I am having issues with the loss of the 'letternumber' column > > overdue_notices.pl throws errors for unknown column, I can't save triggers > > I think the last line below was an accidental deletion? > > diff --git a/installer/data/mysql/kohastructure.sql > b/installer/data/mysql/kohastructure.sql > ... > @@ -2568,12 +2569,10 @@ CREATE TABLE `message_transport_types` ( > DROP TABLE IF EXISTS `overduerules_transport_types`; > CREATE TABLE overduerules_transport_types( > `id` INT(11) NOT NULL AUTO_INCREMENT, > - `branchcode` varchar(10) NOT NULL DEFAULT '', > - `categorycode` VARCHAR(10) NOT NULL DEFAULT '', > - `letternumber` INT(1) NOT NULL DEFAULT 1, Oops, indeed! @inLibro, please provide a follow-up asap
Hm, looking at the bug - I think we need to fix kohastructure and provide a database update for installations that are missing the column?
So, we need a patch that corrects the pushed-to-master patch, for future installations. AND that patch must also provide a fix to those who already installed?
(In reply to Blou from comment #49) > So, we need a patch that corrects the pushed-to-master patch, for future > installations. AND that patch must also provide a fix to those who already > installed? Yes.
I think there should be existing examples for fixes like this - a change to the kohastructure accompanied by a database update. It should ideally check if the column is there already, so people don't get an error on updating in this case.
Opened bug 16007 for the purpose of correcting the new bug we introduced with this patch.