Bug 35451 - Add tablename field to additional_field_values
Summary: Add tablename field to additional_field_values
Status: Signed Off
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement
Assignee: Martin Renvoize (ashimema)
QA Contact: Testopia
URL:
Keywords:
Depends on: 35044 11844
Blocks: 32751
  Show dependency treegraph
 
Reported: 2023-11-30 11:00 UTC by Martin Renvoize (ashimema)
Modified: 2025-02-19 14:32 UTC (History)
7 users (show)

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


Attachments
Bug 35451: Add record_table to additional_field_values (3.16 KB, patch)
2023-11-30 12:11 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Record table in set_additional_fields (3.56 KB, patch)
2023-11-30 12:11 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Update relations (3.64 KB, patch)
2023-11-30 12:11 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Schema (1.64 KB, patch)
2023-11-30 12:11 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Fix test data (1023 bytes, patch)
2023-11-30 12:11 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Add record_table to additional_field_values (3.13 KB, patch)
2023-12-12 16:35 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Record table in set_additional_fields (3.56 KB, patch)
2023-12-12 16:35 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Update relations (3.64 KB, patch)
2023-12-12 16:35 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Schema (1.64 KB, patch)
2023-12-12 16:35 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Fix test data (1023 bytes, patch)
2023-12-12 16:35 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Add record_table to additional_field_values (3.13 KB, patch)
2024-10-22 13:46 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Record table in set_additional_fields (3.56 KB, patch)
2024-10-22 13:46 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Update relations (3.64 KB, patch)
2024-10-22 13:46 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Schema (1.18 KB, patch)
2024-10-22 13:46 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Fix test data (1023 bytes, patch)
2024-10-22 13:46 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Add record_table to additional_field_values (3.18 KB, patch)
2024-10-28 00:09 UTC, David Nind
Details | Diff | Splinter Review
Bug 35451: Record table in set_additional_fields (3.62 KB, patch)
2024-10-28 00:09 UTC, David Nind
Details | Diff | Splinter Review
Bug 35451: Update relations (3.68 KB, patch)
2024-10-28 00:09 UTC, David Nind
Details | Diff | Splinter Review
Bug 35451: Schema (1.23 KB, patch)
2024-10-28 00:09 UTC, David Nind
Details | Diff | Splinter Review
Bug 35451: Fix test data (1.04 KB, patch)
2024-10-28 00:09 UTC, David Nind
Details | Diff | Splinter Review
Bug 35451: Add record_table to additional_field_values (3.51 KB, patch)
2025-02-19 14:26 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Record table in set_additional_fields (1.31 KB, patch)
2025-02-19 14:26 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Update relations (4.32 KB, patch)
2025-02-19 14:26 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Schema (1.38 KB, patch)
2025-02-19 14:26 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Fix test data (1.34 KB, patch)
2025-02-19 14:27 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Add record_table to additional_field_values (3.56 KB, patch)
2025-02-19 14:28 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Record table in set_additional_fields (1.36 KB, patch)
2025-02-19 14:28 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Update relations (4.36 KB, patch)
2025-02-19 14:28 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: DBIC Schema (1.43 KB, patch)
2025-02-19 14:28 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 35451: Fix test data (1.39 KB, patch)
2025-02-19 14:28 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize (ashimema) 2023-11-30 11:00:02 UTC
Currently we rely on a subquery in the additional_field_values relationships we're adding in various places.

For performance and code clarity it would be sensible to add a tablename field to the additional_field_values table to denote which table the value is pertaining to.. we could index this too.

This would allow us to drop the subquery and use an indexed field instead during joins.
Comment 1 Martin Renvoize (ashimema) 2023-11-30 12:11:44 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.
Comment 2 Martin Renvoize (ashimema) 2023-11-30 12:11:46 UTC
Created attachment 159391 [details] [review]
Bug 35451: Record table in set_additional_fields
Comment 3 Martin Renvoize (ashimema) 2023-11-30 12:11:49 UTC
Created attachment 159392 [details] [review]
Bug 35451: Update relations
Comment 4 Martin Renvoize (ashimema) 2023-11-30 12:11:51 UTC
Created attachment 159393 [details] [review]
Bug 35451: Schema
Comment 5 Martin Renvoize (ashimema) 2023-11-30 12:11:54 UTC
Created attachment 159394 [details] [review]
Bug 35451: Fix test data
Comment 6 Martin Renvoize (ashimema) 2023-11-30 12:12:45 UTC
Needs unit tests adding, but posting here as a proof of concept for now looking for feedback.
Comment 7 Martin Renvoize (ashimema) 2023-12-12 16:35:13 UTC
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.
Comment 8 Martin Renvoize (ashimema) 2023-12-12 16:35:15 UTC
Created attachment 159748 [details] [review]
Bug 35451: Record table in set_additional_fields
Comment 9 Martin Renvoize (ashimema) 2023-12-12 16:35:18 UTC
Created attachment 159749 [details] [review]
Bug 35451: Update relations
Comment 10 Martin Renvoize (ashimema) 2023-12-12 16:35:20 UTC
Created attachment 159750 [details] [review]
Bug 35451: Schema
Comment 11 Martin Renvoize (ashimema) 2023-12-12 16:35:23 UTC
Created attachment 159751 [details] [review]
Bug 35451: Fix test data
Comment 12 Anneli Österman 2023-12-13 12:00:09 UTC
Is there a test plan for this?
Comment 13 David Nind 2024-03-23 07:32:04 UTC
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.
Comment 14 David Nind 2024-09-29 18:51:55 UTC
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
Comment 15 Martin Renvoize (ashimema) 2024-10-22 13:46:00 UTC
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.
Comment 16 Martin Renvoize (ashimema) 2024-10-22 13:46:03 UTC
Created attachment 173148 [details] [review]
Bug 35451: Record table in set_additional_fields
Comment 17 Martin Renvoize (ashimema) 2024-10-22 13:46:06 UTC
Created attachment 173149 [details] [review]
Bug 35451: Update relations
Comment 18 Martin Renvoize (ashimema) 2024-10-22 13:46:09 UTC
Created attachment 173150 [details] [review]
Bug 35451: Schema
Comment 19 Martin Renvoize (ashimema) 2024-10-22 13:46:11 UTC
Created attachment 173151 [details] [review]
Bug 35451: Fix test data
Comment 20 Martin Renvoize (ashimema) 2024-10-22 13:47:41 UTC
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.
Comment 21 David Nind 2024-10-28 00:09:17 UTC
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>
Comment 22 David Nind 2024-10-28 00:09:19 UTC
Created attachment 173422 [details] [review]
Bug 35451: Record table in set_additional_fields

Signed-off-by: David Nind <david@davidnind.com>
Comment 23 David Nind 2024-10-28 00:09:22 UTC
Created attachment 173423 [details] [review]
Bug 35451: Update relations

Signed-off-by: David Nind <david@davidnind.com>
Comment 24 David Nind 2024-10-28 00:09:25 UTC
Created attachment 173424 [details] [review]
Bug 35451: Schema

Signed-off-by: David Nind <david@davidnind.com>
Comment 25 David Nind 2024-10-28 00:09:28 UTC
Created attachment 173425 [details] [review]
Bug 35451: Fix test data

Signed-off-by: David Nind <david@davidnind.com>
Comment 26 David Nind 2024-10-28 00:21:00 UTC
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)
Comment 27 Martin Renvoize (ashimema) 2025-01-07 14:16:47 UTC
Thanks for the testing and signoff David :)
Comment 28 Victor Grousset/tuxayo 2025-02-04 19:58:37 UTC
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 29 Victor Grousset/tuxayo 2025-02-04 21:15:51 UTC
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 30 Victor Grousset/tuxayo 2025-02-04 21:21:14 UTC
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 31 Victor Grousset/tuxayo 2025-02-04 21:24:04 UTC
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.
Comment 32 Martin Renvoize (ashimema) 2025-02-19 14:26:50 UTC
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.
Comment 33 Martin Renvoize (ashimema) 2025-02-19 14:26:53 UTC
Created attachment 178333 [details] [review]
Bug 35451: Record table in set_additional_fields
Comment 34 Martin Renvoize (ashimema) 2025-02-19 14:26:56 UTC
Created attachment 178334 [details] [review]
Bug 35451: Update relations
Comment 35 Martin Renvoize (ashimema) 2025-02-19 14:26:59 UTC
Created attachment 178335 [details] [review]
Bug 35451: Schema
Comment 36 Martin Renvoize (ashimema) 2025-02-19 14:27:02 UTC
Created attachment 178336 [details] [review]
Bug 35451: Fix test data
Comment 37 Martin Renvoize (ashimema) 2025-02-19 14:28:22 UTC
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>
Comment 38 Martin Renvoize (ashimema) 2025-02-19 14:28:25 UTC
Created attachment 178338 [details] [review]
Bug 35451: Record table in set_additional_fields

Signed-off-by: David Nind <david@davidnind.com>
Comment 39 Martin Renvoize (ashimema) 2025-02-19 14:28:28 UTC
Created attachment 178339 [details] [review]
Bug 35451: Update relations

Signed-off-by: David Nind <david@davidnind.com>
Comment 40 Martin Renvoize (ashimema) 2025-02-19 14:28:31 UTC
Created attachment 178340 [details] [review]
Bug 35451: DBIC Schema

Signed-off-by: David Nind <david@davidnind.com>
Comment 41 Martin Renvoize (ashimema) 2025-02-19 14:28:34 UTC
Created attachment 178341 [details] [review]
Bug 35451: Fix test data

Signed-off-by: David Nind <david@davidnind.com>
Comment 42 Martin Renvoize (ashimema) 2025-02-19 14:30:03 UTC
(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
Comment 43 Martin Renvoize (ashimema) 2025-02-19 14:30:26 UTC
(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!
Comment 44 Martin Renvoize (ashimema) 2025-02-19 14:32:02 UTC
(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.