Description
Martin Renvoize (ashimema)
2023-11-30 11:00:02 UTC
Created attachment 159390 [details] [review] Bug 35451: Add record_table to additional_field_values This patch updates the database to include a record_table field in the additional_field_values table and adds an index on that field. This should facilitate more performant joins. Created attachment 159391 [details] [review] Bug 35451: Record table in set_additional_fields Created attachment 159392 [details] [review] Bug 35451: Update relations Created attachment 159393 [details] [review] Bug 35451: Schema Created attachment 159394 [details] [review] Bug 35451: Fix test data Needs unit tests adding, but posting here as a proof of concept for now looking for feedback. Created attachment 159747 [details] [review] Bug 35451: Add record_table to additional_field_values This patch updates the database to include a record_table field in the additional_field_values table and adds an index on that field. This should facilitate more performant joins. Created attachment 159748 [details] [review] Bug 35451: Record table in set_additional_fields Created attachment 159749 [details] [review] Bug 35451: Update relations Created attachment 159750 [details] [review] Bug 35451: Schema Created attachment 159751 [details] [review] Bug 35451: Fix test data Is there a test plan for this? I get this message for a dependency when trying to apply the patches for this bug: git bz apply 35451 Bug 35451 Depends on bug 35044 (Signed Off) Follow? [(y)es, (n)o] y Cannot apply cleanly patches from bug 35044. Everything will be left dirty. git bz apply --continue will not continue the process if patches from other bug reports need to be applied. Once bug 35044 is fixed, I'll have another go at testing. My test plan was to: 1. Apply the patches 2. Update the database: updatedatabase 3. Restart everything (flush_memcached, restart_all) 4. Run the tests - they should pass: prove t/db_dependent/Koha/Objects/Mixin/AdditionalFields.t I wasn't sure what else needs to be tested, or if this is sufficient. Bug 35044 is now in main, however I am now getting the sha1/fake ancestor error: git bz apply 35451 Bug 35451 - Add tablename field to additional_field_values 159747 - Bug 35451: Add record_table to additional_field_values 159748 - Bug 35451: Record table in set_additional_fields 159749 - Bug 35451: Update relations 159750 - Bug 35451: Schema 159751 - Bug 35451: Fix test data Apply? [(y)es, (n)o, (i)nteractive] y Applying: Bug 35451: Add record_table to additional_field_values Applying: Bug 35451: Record table in set_additional_fields Applying: Bug 35451: Update relations Applying: Bug 35451: Schema error: sha1 information is lacking or useless (Koha/Schema/Result/AdditionalFieldValue.pm). error: could not build fake ancestor Patch failed at 0001 Bug 35451: Schema Created attachment 173147 [details] [review] Bug 35451: Add record_table to additional_field_values This patch updates the database to include a record_table field in the additional_field_values table and adds an index on that field. This should facilitate more performant joins. Created attachment 173148 [details] [review] Bug 35451: Record table in set_additional_fields Created attachment 173149 [details] [review] Bug 35451: Update relations Created attachment 173150 [details] [review] Bug 35451: Schema Created attachment 173151 [details] [review] Bug 35451: Fix test data This shouldn't drastically change anything.. it's a performance piece and only really makes a difference where large numbers of additional attributes are defined. No real test plan per say. Created attachment 173421 [details] [review] Bug 35451: Add record_table to additional_field_values This patch updates the database to include a record_table field in the additional_field_values table and adds an index on that field. This should facilitate more performant joins. Signed-off-by: David Nind <david@davidnind.com> Created attachment 173422 [details] [review] Bug 35451: Record table in set_additional_fields Signed-off-by: David Nind <david@davidnind.com> Created attachment 173423 [details] [review] Bug 35451: Update relations Signed-off-by: David Nind <david@davidnind.com> Created attachment 173424 [details] [review] Bug 35451: Schema Signed-off-by: David Nind <david@davidnind.com> Created attachment 173425 [details] [review] Bug 35451: Fix test data Signed-off-by: David Nind <david@davidnind.com> Testing notes (using KTD) - main aim was to make sure everything works as expected: 1. Apply the patch. 2. Update the database: updatedatabase 3. Restart everything: restart_all 4. Run the tests - these should pass: prove t/db_dependent/Koha/Objects/Mixin/AdditionalFields.t 5. Make sure additional fields for acquisition order baskets continue to work as expected: 5.1 Go to Administration > Additional parameters > Additional fields 5.2 Select the table to add additional fields to (in this case, Order baskets (aqbasket)) 5.3 Add a new field: 5.3.1 + New field 5.3.2 Name: TESTFIELD 5.3.3 Repeatable: select 5.3.4 Searchable: select 5.4 Add a new basket to "My Vendor" using the additional field 5.4.1 Go to Acquisitions 5.4.2 In the "Search vendors" section, click "Search" 5.4.3 Add a new order basket: + New > Basket . Basket name: bz35451 . TESTFIELD (add two entries): This is a test + This is a second test 5.4.4 Check that you can edit and delete the entries for your additional field 5.5 Bonus (optional): add different types of additional fields (such as authorized values, or not repeatable) and test that they work SQL === Before the patch, table for additional_field_values - no record_table column: MariaDB [koha_kohadev]> select * from additional_field_values; +----+----------+-----------+-----------------------+ | id | field_id | record_id | value | +----+----------+-----------+-----------------------+ | 1 | 1 | 2 | This is a test | | 2 | 1 | 2 | This is a second test | +----+----------+-----------+-----------------------+ 2 rows in set (0.000 sec) After patch, table for additional_field_values - now has a record_table column: MariaDB [koha_kohadev]> select * from additional_field_values;; +----+----------+--------------+-----------+-----------------------+ | id | field_id | record_table | record_id | value | +----+----------+--------------+-----------+-----------------------+ | 5 | 1 | aqbasket | 2 | This is a test | | 6 | 1 | aqbasket | 2 | This is a second test | +----+----------+--------------+-----------+-----------------------+ 2 rows in set (0.000 sec) Thanks for the testing and signoff David :) Comment on attachment 173421 [details] [review] Bug 35451: Add record_table to additional_field_values Review of attachment 173421 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/kohastructure.sql @@ +242,4 @@ > CREATE TABLE `additional_field_values` ( > `id` int(11) NOT NULL AUTO_INCREMENT COMMENT 'primary key identifier', > `field_id` int(11) NOT NULL COMMENT 'foreign key references additional_fields(id)', > + `record_table` varchar(255) NOT NULL DEFAULT '' COMMENT 'tablename of the related record', Backticks, which should not be used https://wiki.koha-community.org/wiki/Coding_Guidelines#SQL6:_Backticks But there are already on all other lines >_< So I don't know. Comment on attachment 173422 [details] [review] Bug 35451: Record table in set_additional_fields Review of attachment 173422 [details] [review]: ----------------------------------------------------------------- ::: Koha/Object/Mixin/AdditionalFields.pm @@ +86,5 @@ > + record_table => $table, > + record_id => $self->id, > + value => $value, > + } > + )->store; It seems a text editor tidied the whole file. Which causes the patch to not apply due to a conflict around here. And hides other changes. Comment on attachment 173424 [details] [review] Bug 35451: Schema Review of attachment 173424 [details] [review]: ----------------------------------------------------------------- ::: Koha/Schema/Result/AdditionalFieldValue.pm @@ +71,5 @@ > { data_type => "integer", is_auto_increment => 1, is_nullable => 0 }, > "field_id", > { data_type => "integer", is_foreign_key => 1, is_nullable => 0 }, > + "record_table", > + { data_type => "varchar", default_value => "", is_nullable => 0, size => 255 }, The changes on this file should have changed the DBIx checksum Comment on attachment 173423 [details] [review] Bug 35451: Update relations Review of attachment 173423 [details] [review]: ----------------------------------------------------------------- ::: Koha/Schema/Result/Subscription.pm @@ -578,5 @@ > return { > "$args->{foreign_alias}.record_id" => { -ident => "$args->{self_alias}.subscriptionid" }, > - > - "$args->{foreign_alias}.field_id" => > - { -in => \'(SELECT id FROM additional_fields WHERE tablename = "subscription")' }, This change is in the part protected by the checksum but the checksum didn't change. Created attachment 178332 [details] [review] Bug 35451: Add record_table to additional_field_values This patch updates the database to include a record_table field in the additional_field_values table and adds an index on that field. This should facilitate more performant joins. Created attachment 178333 [details] [review] Bug 35451: Record table in set_additional_fields Created attachment 178334 [details] [review] Bug 35451: Update relations Created attachment 178335 [details] [review] Bug 35451: Schema Created attachment 178336 [details] [review] Bug 35451: Fix test data Created attachment 178337 [details] [review] Bug 35451: Add record_table to additional_field_values This patch updates the database to include a record_table field in the additional_field_values table and adds an index on that field. This should facilitate more performant joins. Signed-off-by: David Nind <david@davidnind.com> Created attachment 178338 [details] [review] Bug 35451: Record table in set_additional_fields Signed-off-by: David Nind <david@davidnind.com> Created attachment 178339 [details] [review] Bug 35451: Update relations Signed-off-by: David Nind <david@davidnind.com> Created attachment 178340 [details] [review] Bug 35451: DBIC Schema Signed-off-by: David Nind <david@davidnind.com> Created attachment 178341 [details] [review] Bug 35451: Fix test data Signed-off-by: David Nind <david@davidnind.com> (In reply to Victor Grousset/tuxayo from comment #31) > Comment on attachment 173423 [details] [review] [review] > Bug 35451: Update relations > > Review of attachment 173423 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: Koha/Schema/Result/Subscription.pm > @@ -578,5 @@ > > return { > > "$args->{foreign_alias}.record_id" => { -ident => "$args->{self_alias}.subscriptionid" }, > > - > > - "$args->{foreign_alias}.field_id" => > > - { -in => \'(SELECT id FROM additional_fields WHERE tablename = "subscription")' }, > > This change is in the part protected by the checksum but the checksum didn't > change. Check again.. it's after the checksum (In reply to Victor Grousset/tuxayo from comment #30) > Comment on attachment 173424 [details] [review] [review] > Bug 35451: Schema > > Review of attachment 173424 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: Koha/Schema/Result/AdditionalFieldValue.pm > @@ +71,5 @@ > > { data_type => "integer", is_auto_increment => 1, is_nullable => 0 }, > > "field_id", > > { data_type => "integer", is_foreign_key => 1, is_nullable => 0 }, > > + "record_table", > > + { data_type => "varchar", default_value => "", is_nullable => 0, size => 255 }, > > The changes on this file should have changed the DBIx checksum This IS the dbic dump patch! (In reply to Victor Grousset/tuxayo from comment #28) > Comment on attachment 173421 [details] [review] [review] > Bug 35451: Add record_table to additional_field_values > > Review of attachment 173421 [details] [review] [review]: > ----------------------------------------------------------------- > > ::: installer/data/mysql/kohastructure.sql > @@ +242,4 @@ > > CREATE TABLE `additional_field_values` ( > > `id` int(11) NOT NULL AUTO_INCREMENT COMMENT 'primary key identifier', > > `field_id` int(11) NOT NULL COMMENT 'foreign key references additional_fields(id)', > > + `record_table` varchar(255) NOT NULL DEFAULT '' COMMENT 'tablename of the related record', > > Backticks, which should not be used > https://wiki.koha-community.org/wiki/Coding_Guidelines#SQL6:_Backticks > > But there are already on all other lines >_< So I don't know. This isn't IN the table name.. it's quoting the table name.. which is correct as far as I'm aware. QA: Looking here Created attachment 180806 [details] [review] Bug 35451: Add record_table to additional_field_values This patch updates the database to include a record_table field in the additional_field_values table and adds an index on that field. This should facilitate more performant joins. Signed-off-by: David Nind <david@davidnind.com> [EDIT] chmod 755 for atomicupdate/bug_35451.pl Created attachment 180807 [details] [review] Bug 35451: Record table in set_additional_fields Signed-off-by: David Nind <david@davidnind.com> Created attachment 180808 [details] [review] Bug 35451: Update relations Signed-off-by: David Nind <david@davidnind.com> Created attachment 180809 [details] [review] Bug 35451: DBIC Schema Signed-off-by: David Nind <david@davidnind.com> Created attachment 180810 [details] [review] Bug 35451: Fix test data Signed-off-by: David Nind <david@davidnind.com> Trivial rebase. Added chmod on atomic update. Acquisition/TransferOrder.t .. 1/15 # Failed test 'transfered order has one additional field value' # at Acquisition/TransferOrder.t line 160. # got: '0' # expected: '1' # Looks like you planned 2 tests but ran 1. # Looks like you failed 1 test of 1 run. # Failed test 'TransferOrder should copy additional fields' # at Acquisition/TransferOrder.t line 165. Can't call method "value" on an undefined value at Acquisition/TransferOrder.t line 161. # Looks like your test exited with 11 just after 15. Acquisition/TransferOrder.t .. Dubious, test returned 11 (wstat 2816, 0xb00) Please check other tests too using AdditionalFields Created attachment 182401 [details] [review] Bug 35451: Add record_table to additional_field_values This patch updates the database to include a record_table field in the additional_field_values table and adds an index on that field. This should facilitate more performant joins. Signed-off-by: David Nind <david@davidnind.com> Created attachment 182402 [details] [review] Bug 35451: Record table in set_additional_fields Signed-off-by: David Nind <david@davidnind.com> Created attachment 182403 [details] [review] Bug 35451: Update relations Signed-off-by: David Nind <david@davidnind.com> Created attachment 182404 [details] [review] Bug 35451: DBIC Schema Signed-off-by: David Nind <david@davidnind.com> Created attachment 182405 [details] [review] Bug 35451: Fix test data Signed-off-by: David Nind <david@davidnind.com> Created attachment 182406 [details] [review] Bug 35451: (follow-up) Fix C4::Acquisition::TransferOrder This patch adds table name to the additional fields clone found insdie TransferOrder. Created attachment 182407 [details] [review] Bug 35451: Add record_table to additional_field_values This patch updates the database to include a record_table field in the additional_field_values table and adds an index on that field. This should facilitate more performant joins. Signed-off-by: David Nind <david@davidnind.com> Created attachment 182408 [details] [review] Bug 35451: Record table in set_additional_fields Signed-off-by: David Nind <david@davidnind.com> Created attachment 182409 [details] [review] Bug 35451: Update relations Signed-off-by: David Nind <david@davidnind.com> Created attachment 182410 [details] [review] Bug 35451: DBIC Schema Signed-off-by: David Nind <david@davidnind.com> Created attachment 182411 [details] [review] Bug 35451: Fix test data Signed-off-by: David Nind <david@davidnind.com> Created attachment 182412 [details] [review] Bug 35451: (follow-up) Fix C4::Acquisition::TransferOrder This patch adds table name to the additional fields clone found insdie TransferOrder. Created attachment 182413 [details] [review] Bug 35451: (follow-up) Save join in C4::Serials::DelSubscription We can use the new record_table field to save ourselves a join in here too. (In reply to Marcel de Rooy from comment #52) > Acquisition/TransferOrder.t .. 1/15 > # Failed test 'transfered order has one additional field value' > # at Acquisition/TransferOrder.t line 160. > # got: '0' > # expected: '1' > # Looks like you planned 2 tests but ran 1. > # Looks like you failed 1 test of 1 run. > > # Failed test 'TransferOrder should copy additional fields' > # at Acquisition/TransferOrder.t line 165. > Can't call method "value" on an undefined value at > Acquisition/TransferOrder.t line 161. > # Looks like your test exited with 11 just after 15. > Acquisition/TransferOrder.t .. Dubious, test returned 11 (wstat 2816, 0xb00) > > Please check other tests too using AdditionalFields Good catch, thanks :) |