Test::DBIx::Class fixtures won't work if the resultset doesn't support last_insert_id. This is only supported with a integer primary key. This patch adds one to issuingrules.
Created attachment 60921 [details] [review] Bug 18231 - Add the missing id-column to koha.issuingrules-table for DBIC fixtures Many DBIx-stuffs depend on last_insert_id
- Is there any test plan? - The atomic update file is named "Bug-18226-Remove_mock_dbh.sql", but we are on Bug 18231, I think it should be renamed.
Created attachment 60955 [details] [review] Bug 18231 - Add the missing id-column to koha.issuingrules-table for DBIC fixtures Many DBIx-stuffs depend on last_insert_id
(In reply to Marc Véron from comment #2) > - Is there any test plan? > t/db_dependent/Reserves.t t/db_dependent/Circulation/CalcFine.t + others. All tests pass, no regression detected. > - The atomic update file is named "Bug-18226-Remove_mock_dbh.sql", but we > are on Bug 18231, I think it should be renamed. Fixed.
Created attachment 60964 [details] [review] Bug 18231 - Add the missing id-column to koha.issuingrules-table for DBIC fixtures Many DBIx-stuffs depend on last_insert_id Followed test plan from comment #4, works as expected Signed-off-by: Marc Véron <veron@veron.ch>
Community thanks you Marc :)
Olli: Did you know that we have an Assignee field ?
unique key is not named the same.
(In reply to Jonathan Druart from comment #8) > unique key is not named the same. Prior to this patch there was no UNIQUE KEY? If you mean the index naming convention doesn't follow a established pattern, maybe it is easiest just let the DB-engine name the index?
(In reply to Marcel de Rooy from comment #7) > Olli: Did you know that we have an Assignee field ? Now I know :)
Created attachment 61102 [details] [review] Bug 18231 - Add the missing id-column to koha.issuingrules-table for DBIC fixtures Many DBIx-stuffs depend on last_insert_id Followed test plan from comment #4, works as expected Signed-off-by: Marc Véron <veron@veron.ch>
(In reply to Jonathan Druart from comment #8) > unique key is not named the same. I think you meant the issue where both the PRIMARY KEY and the UNIQUE CONSTRAINT had the same index name? That is now fixed.
Created attachment 61218 [details] [review] Bug 18231 - Add the missing id-column to koha.issuingrules-table for DBIC fixtures Many DBIx-stuffs depend on last_insert_id Followed test plan from comment #4, works as expected Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 61219 [details] [review] Bug 18231: Make sure we will not add the column twice
Olli, you are not supposed to modify a patch once it has been signed off. Take a look at the second patch to know how to process for DB updates adding/modifying columns.
Pushed to master for 17.05, thanks Olli!
(In reply to Kyle M Hall from comment #16) > Pushed to master for 17.05, thanks Olli! Cancel that! Patch set needs a small rebase!
Comment on attachment 61219 [details] [review] Bug 18231: Make sure we will not add the column twice Review of attachment 61219 [details] [review]: ----------------------------------------------------------------- I was just expecting the .perl and removal of the .sql when I went to eyeball this. ::: Koha/Schema/Result/BorrowerAttributeType.pm @@ +154,5 @@ > ); > > > +# Created by DBIx::Class::Schema::Loader v0.07042 @ 2017-03-17 11:43:14 > +# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:0jCQN4xSJYbQC5e7fGn5WQ Why add this? ::: Koha/Schema/Result/Issue.pm @@ +222,5 @@ > ); > > > +# Created by DBIx::Class::Schema::Loader v0.07042 @ 2017-03-17 11:43:14 > +# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:zGLKwoUmq4pcvY16ei3VPg Why add this? ::: Koha/Schema/Result/Issuingrule.pm @@ +330,5 @@ > ); > > > +# Created by DBIx::Class::Schema::Loader v0.07042 @ 2017-03-17 11:43:14 > +# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:3K9yYzzg9IYRAuxSXd7CPg Why add this? ::: Koha/Schema/Result/OldIssue.pm @@ +207,5 @@ > ); > > > +# Created by DBIx::Class::Schema::Loader v0.07042 @ 2017-03-17 11:43:14 > +# DO NOT MODIFY THIS OR ANYTHING ABOVE! md5sum:yTyCAau9EZHBgdqN4Ptljg Why add this? ::: Koha/Schema/Result/SearchField.pm @@ +48,4 @@ > =head2 type > > data_type: 'enum' > + extra: {list => ["","string","date","number","boolean","sum"]} Is this from another bug? @@ +65,4 @@ > "type", > { > data_type => "enum", > + extra => { list => ["", "string", "date", "number", "boolean", "sum"] }, Is this from another bug? I think it may be valid, but I don't think this bug originally added it. ::: installer/data/mysql/atomicupdate/Bug-18231-id_to_ussingrules_table.perl @@ +1,3 @@ > +$DBversion = 'XXX'; # will be replaced by the RM > +if( CheckVersion( $DBversion ) ) { > + unless ( column_exists( 'issuingrules', 'issuingrules_id' ) ) { This column_exists() has been a fantastic add, since it was put in. :)
Created attachment 64966 [details] [review] Bug 18231 - Add the missing id-column to koha.issuingrules-table for DBIC fixtures Many DBIx-stuffs depend on last_insert_id Followed test plan from comment #4, works as expected Signed-off-by: Marc Véron <veron@veron.ch>
Created attachment 64967 [details] [review] Bug 18231: Make sure we will not add the column twice
Created attachment 64968 [details] [review] Bug 18231: (follow-up) Fix t/db_dependent/Koha/Objects.t This Bug broke a test in t/db_dependent/Koha/Objects.t. This patch replaces now failing test with an equal one but targetting a different Koha-object. To test: 1. Apply first & second patch in this Bug and run updatedatabase.pl 2. prove t/db_dependent/Koha/Objects.t 3. Observe following failure: DBIx::Class::ResultSet::find(): find() expects either a column/value hashref, or a list of values corresponding to the columns of the specified unique constraint 'primary' at /home/koha/Koha/Koha/Objects.pm line 227 4. Apply this patch 5. prove t/db_dependent/Koha/Objects.t 6. Observe test passing
Noticed a broken test in "t/db_dependent/Koha/Objects.t". Rebased patches on top of current master, removed things pointed out by comment #1 (Please let me know if I shouldn't had done so and I will fix it), switched to "Needs Signoff".
(In reply to Lari Taskula from comment #22) > by comment #1 * by comment #18
Created attachment 66969 [details] [review] Bug 18231 - Add the missing id-column to koha.issuingrules-table for DBIC fixtures Many DBIx-stuffs depend on last_insert_id Followed test plan from comment #4, works as expected Signed-off-by: Marc Véron <veron@veron.ch> Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Created attachment 66970 [details] [review] Bug 18231: Make sure we will not add the column twice Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
Created attachment 66971 [details] [review] Bug 18231: (follow-up) Fix t/db_dependent/Koha/Objects.t This Bug broke a test in t/db_dependent/Koha/Objects.t. This patch replaces now failing test with an equal one but targetting a different Koha-object. To test: 1. Apply first & second patch in this Bug and run updatedatabase.pl 2. prove t/db_dependent/Koha/Objects.t 3. Observe following failure: DBIx::Class::ResultSet::find(): find() expects either a column/value hashref, or a list of values corresponding to the columns of the specified unique constraint 'primary' at /home/koha/Koha/Koha/Objects.pm line 227 4. Apply this patch 5. prove t/db_dependent/Koha/Objects.t 6. Observe test passing Followed test plan, patch worked as described Signed-off-by: Alex Buckley <alexbuckley@catalyst.net.nz>
There is a complaint in the QA tools about a file being added and removed - this is due to the change from .perl to .sql for the file. As all 3 patches have different authors, this can't be fixed by squashing (not a blocker in my opinion). FAIL installer/data/mysql/atomicupdate/Bug-18231-id_to_ussingrules_table.sql FAIL git manipulation The file has been added and deleted in the same patchset t/db_dependent/Koha/IssuingRules.t are ok. BUT: Cloning isusing rules from one of the library (default to Union for example) fails:
issuingrules is gone - so this turns into "INVALID".