Bug 18231 - Add the missing id-column to koha.issuingrules-table for DBIC fixtures
Summary: Add the missing id-column to koha.issuingrules-table for DBIC fixtures
Status: CLOSED INVALID
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Olli-Antti Kivilahti
QA Contact: Kyle M Hall
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2017-03-08 15:40 UTC by Olli-Antti Kivilahti
Modified: 2020-11-30 21:45 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 18231 - Add the missing id-column to koha.issuingrules-table for DBIC fixtures (4.15 KB, patch)
2017-03-08 15:41 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 18231 - Add the missing id-column to koha.issuingrules-table for DBIC fixtures (4.18 KB, patch)
2017-03-09 14:28 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 18231 - Add the missing id-column to koha.issuingrules-table for DBIC fixtures (4.36 KB, patch)
2017-03-09 14:59 UTC, Marc Véron
Details | Diff | Splinter Review
Bug 18231 - Add the missing id-column to koha.issuingrules-table for DBIC fixtures (4.37 KB, patch)
2017-03-15 13:19 UTC, Olli-Antti Kivilahti
Details | Diff | Splinter Review
Bug 18231 - Add the missing id-column to koha.issuingrules-table for DBIC fixtures (4.36 KB, patch)
2017-03-17 15:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18231: Make sure we will not add the column twice (6.05 KB, patch)
2017-03-17 15:00 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 18231 - Add the missing id-column to koha.issuingrules-table for DBIC fixtures (4.23 KB, patch)
2017-07-10 14:54 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18231: Make sure we will not add the column twice (2.10 KB, patch)
2017-07-10 14:54 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18231: (follow-up) Fix t/db_dependent/Koha/Objects.t (2.08 KB, patch)
2017-07-10 14:54 UTC, Lari Taskula
Details | Diff | Splinter Review
Bug 18231 - Add the missing id-column to koha.issuingrules-table for DBIC fixtures (4.28 KB, patch)
2017-09-08 04:36 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18231: Make sure we will not add the column twice (2.15 KB, patch)
2017-09-08 04:37 UTC, Alex Buckley
Details | Diff | Splinter Review
Bug 18231: (follow-up) Fix t/db_dependent/Koha/Objects.t (2.18 KB, patch)
2017-09-08 04:40 UTC, Alex Buckley
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Olli-Antti Kivilahti 2017-03-08 15:40:31 UTC
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.
Comment 1 Olli-Antti Kivilahti 2017-03-08 15:41:26 UTC
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
Comment 2 Marc Véron 2017-03-09 13:49:01 UTC
- 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.
Comment 3 Olli-Antti Kivilahti 2017-03-09 14:28:35 UTC
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
Comment 4 Olli-Antti Kivilahti 2017-03-09 14:30:01 UTC
(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.
Comment 5 Marc Véron 2017-03-09 14:59:49 UTC
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>
Comment 6 Olli-Antti Kivilahti 2017-03-09 15:01:18 UTC
Community thanks you Marc :)
Comment 7 Marcel de Rooy 2017-03-10 09:05:37 UTC
Olli: Did you know that we have an Assignee field ?
Comment 8 Jonathan Druart 2017-03-15 12:14:01 UTC
unique key is not named the same.
Comment 9 Olli-Antti Kivilahti 2017-03-15 13:08:49 UTC
(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?
Comment 10 Olli-Antti Kivilahti 2017-03-15 13:09:08 UTC
(In reply to Marcel de Rooy from comment #7)
> Olli: Did you know that we have an Assignee field ?

Now I know :)
Comment 11 Olli-Antti Kivilahti 2017-03-15 13:19:12 UTC
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>
Comment 12 Olli-Antti Kivilahti 2017-03-15 13:20:04 UTC
(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.
Comment 13 Jonathan Druart 2017-03-17 15:00:50 UTC
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>
Comment 14 Jonathan Druart 2017-03-17 15:00:57 UTC
Created attachment 61219 [details] [review]
Bug 18231: Make sure we will not add the column twice
Comment 15 Jonathan Druart 2017-03-17 15:02:06 UTC
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.
Comment 16 Kyle M Hall 2017-03-31 15:15:49 UTC
Pushed to master for 17.05, thanks Olli!
Comment 17 Kyle M Hall 2017-03-31 15:25:17 UTC
(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 18 Mark Tompsett 2017-05-19 03:00:16 UTC
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. :)
Comment 19 Lari Taskula 2017-07-10 14:54:09 UTC
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>
Comment 20 Lari Taskula 2017-07-10 14:54:21 UTC
Created attachment 64967 [details] [review]
Bug 18231: Make sure we will not add the column twice
Comment 21 Lari Taskula 2017-07-10 14:54:37 UTC
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
Comment 22 Lari Taskula 2017-07-10 14:58:16 UTC
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".
Comment 23 Lari Taskula 2017-07-10 14:58:54 UTC
(In reply to Lari Taskula from comment #22)
> by comment #1
* by comment #18
Comment 24 Alex Buckley 2017-09-08 04:36:24 UTC
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>
Comment 25 Alex Buckley 2017-09-08 04:37:51 UTC
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>
Comment 26 Alex Buckley 2017-09-08 04:40:42 UTC
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>
Comment 27 Katrin Fischer 2018-01-03 23:09:39 UTC
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:
Comment 28 Katrin Fischer 2020-02-08 11:06:26 UTC
issuingrules is gone - so this turns into "INVALID".