Bug 35451

Summary: Add tablename field to additional_field_values
Product: Koha Reporter: Martin Renvoize (ashimema) <martin.renvoize>
Component: Architecture, internals, and plumbingAssignee: Martin Renvoize (ashimema) <martin.renvoize>
Status: Signed Off --- QA Contact: Marcel de Rooy <m.de.rooy>
Severity: enhancement    
Priority: P5 - low CC: anneli.osterman, david, jonathan.druart, julian.maurice, m.de.rooy, pedro.amorim, tomascohen, victor
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35044
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
Circulation function:
Bug Depends on: 35044, 11844    
Bug Blocks: 32751    
Attachments: Bug 35451: Add record_table to additional_field_values
Bug 35451: Record table in set_additional_fields
Bug 35451: Update relations
Bug 35451: Schema
Bug 35451: Fix test data
Bug 35451: Add record_table to additional_field_values
Bug 35451: Record table in set_additional_fields
Bug 35451: Update relations
Bug 35451: Schema
Bug 35451: Fix test data
Bug 35451: Add record_table to additional_field_values
Bug 35451: Record table in set_additional_fields
Bug 35451: Update relations
Bug 35451: Schema
Bug 35451: Fix test data
Bug 35451: Add record_table to additional_field_values
Bug 35451: Record table in set_additional_fields
Bug 35451: Update relations
Bug 35451: Schema
Bug 35451: Fix test data
Bug 35451: Add record_table to additional_field_values
Bug 35451: Record table in set_additional_fields
Bug 35451: Update relations
Bug 35451: Schema
Bug 35451: Fix test data
Bug 35451: Add record_table to additional_field_values
Bug 35451: Record table in set_additional_fields
Bug 35451: Update relations
Bug 35451: DBIC Schema
Bug 35451: Fix test data
Bug 35451: Add record_table to additional_field_values
Bug 35451: Record table in set_additional_fields
Bug 35451: Update relations
Bug 35451: DBIC Schema
Bug 35451: Fix test data
Bug 35451: Add record_table to additional_field_values
Bug 35451: Record table in set_additional_fields
Bug 35451: Update relations
Bug 35451: DBIC Schema
Bug 35451: Fix test data
Bug 35451: (follow-up) Fix C4::Acquisition::TransferOrder
Bug 35451: Add record_table to additional_field_values
Bug 35451: Record table in set_additional_fields
Bug 35451: Update relations
Bug 35451: DBIC Schema
Bug 35451: Fix test data
Bug 35451: (follow-up) Fix C4::Acquisition::TransferOrder
Bug 35451: (follow-up) Save join in C4::Serials::DelSubscription

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 Comment hidden (obsolete)
Comment 2 Martin Renvoize (ashimema) 2023-11-30 12:11:46 UTC Comment hidden (obsolete)
Comment 3 Martin Renvoize (ashimema) 2023-11-30 12:11:49 UTC Comment hidden (obsolete)
Comment 4 Martin Renvoize (ashimema) 2023-11-30 12:11:51 UTC Comment hidden (obsolete)
Comment 5 Martin Renvoize (ashimema) 2023-11-30 12:11:54 UTC Comment hidden (obsolete)
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 Comment hidden (obsolete)
Comment 8 Martin Renvoize (ashimema) 2023-12-12 16:35:15 UTC Comment hidden (obsolete)
Comment 9 Martin Renvoize (ashimema) 2023-12-12 16:35:18 UTC Comment hidden (obsolete)
Comment 10 Martin Renvoize (ashimema) 2023-12-12 16:35:20 UTC Comment hidden (obsolete)
Comment 11 Martin Renvoize (ashimema) 2023-12-12 16:35:23 UTC Comment hidden (obsolete)
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 Comment hidden (obsolete)
Comment 16 Martin Renvoize (ashimema) 2024-10-22 13:46:03 UTC Comment hidden (obsolete)
Comment 17 Martin Renvoize (ashimema) 2024-10-22 13:46:06 UTC Comment hidden (obsolete)
Comment 18 Martin Renvoize (ashimema) 2024-10-22 13:46:09 UTC Comment hidden (obsolete)
Comment 19 Martin Renvoize (ashimema) 2024-10-22 13:46:11 UTC Comment hidden (obsolete)
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 Comment hidden (obsolete)
Comment 22 David Nind 2024-10-28 00:09:19 UTC Comment hidden (obsolete)
Comment 23 David Nind 2024-10-28 00:09:22 UTC Comment hidden (obsolete)
Comment 24 David Nind 2024-10-28 00:09:25 UTC Comment hidden (obsolete)
Comment 25 David Nind 2024-10-28 00:09:28 UTC Comment hidden (obsolete)
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 Comment hidden (obsolete)
Comment 33 Martin Renvoize (ashimema) 2025-02-19 14:26:53 UTC Comment hidden (obsolete)
Comment 34 Martin Renvoize (ashimema) 2025-02-19 14:26:56 UTC Comment hidden (obsolete)
Comment 35 Martin Renvoize (ashimema) 2025-02-19 14:26:59 UTC Comment hidden (obsolete)
Comment 36 Martin Renvoize (ashimema) 2025-02-19 14:27:02 UTC Comment hidden (obsolete)
Comment 37 Martin Renvoize (ashimema) 2025-02-19 14:28:22 UTC Comment hidden (obsolete)
Comment 38 Martin Renvoize (ashimema) 2025-02-19 14:28:25 UTC Comment hidden (obsolete)
Comment 39 Martin Renvoize (ashimema) 2025-02-19 14:28:28 UTC Comment hidden (obsolete)
Comment 40 Martin Renvoize (ashimema) 2025-02-19 14:28:31 UTC Comment hidden (obsolete)
Comment 41 Martin Renvoize (ashimema) 2025-02-19 14:28:34 UTC Comment hidden (obsolete)
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.
Comment 45 Marcel de Rooy 2025-04-11 06:30:06 UTC
QA: Looking here
Comment 46 Marcel de Rooy 2025-04-11 06:56:40 UTC Comment hidden (obsolete)
Comment 47 Marcel de Rooy 2025-04-11 06:56:43 UTC Comment hidden (obsolete)
Comment 48 Marcel de Rooy 2025-04-11 06:56:46 UTC Comment hidden (obsolete)
Comment 49 Marcel de Rooy 2025-04-11 06:56:49 UTC
Created attachment 180809 [details] [review]
Bug 35451: DBIC Schema

Signed-off-by: David Nind <david@davidnind.com>
Comment 50 Marcel de Rooy 2025-04-11 06:56:52 UTC
Created attachment 180810 [details] [review]
Bug 35451: Fix test data

Signed-off-by: David Nind <david@davidnind.com>
Comment 51 Marcel de Rooy 2025-04-11 06:57:46 UTC
Trivial rebase. Added chmod on atomic update.
Comment 52 Marcel de Rooy 2025-04-11 06:58:15 UTC
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
Comment 53 Martin Renvoize (ashimema) 2025-05-13 19:10:25 UTC
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>
Comment 54 Martin Renvoize (ashimema) 2025-05-13 19:10:29 UTC
Created attachment 182402 [details] [review]
Bug 35451: Record table in set_additional_fields

Signed-off-by: David Nind <david@davidnind.com>
Comment 55 Martin Renvoize (ashimema) 2025-05-13 19:10:32 UTC
Created attachment 182403 [details] [review]
Bug 35451: Update relations

Signed-off-by: David Nind <david@davidnind.com>
Comment 56 Martin Renvoize (ashimema) 2025-05-13 19:10:35 UTC
Created attachment 182404 [details] [review]
Bug 35451: DBIC Schema

Signed-off-by: David Nind <david@davidnind.com>
Comment 57 Martin Renvoize (ashimema) 2025-05-13 19:10:39 UTC
Created attachment 182405 [details] [review]
Bug 35451: Fix test data

Signed-off-by: David Nind <david@davidnind.com>
Comment 58 Martin Renvoize (ashimema) 2025-05-13 19:10:42 UTC
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.
Comment 59 Martin Renvoize (ashimema) 2025-05-13 19:39:50 UTC
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>
Comment 60 Martin Renvoize (ashimema) 2025-05-13 19:39:54 UTC
Created attachment 182408 [details] [review]
Bug 35451: Record table in set_additional_fields

Signed-off-by: David Nind <david@davidnind.com>
Comment 61 Martin Renvoize (ashimema) 2025-05-13 19:39:57 UTC
Created attachment 182409 [details] [review]
Bug 35451: Update relations

Signed-off-by: David Nind <david@davidnind.com>
Comment 62 Martin Renvoize (ashimema) 2025-05-13 19:40:01 UTC
Created attachment 182410 [details] [review]
Bug 35451: DBIC Schema

Signed-off-by: David Nind <david@davidnind.com>
Comment 63 Martin Renvoize (ashimema) 2025-05-13 19:40:04 UTC
Created attachment 182411 [details] [review]
Bug 35451: Fix test data

Signed-off-by: David Nind <david@davidnind.com>
Comment 64 Martin Renvoize (ashimema) 2025-05-13 19:40:08 UTC
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.
Comment 65 Martin Renvoize (ashimema) 2025-05-13 19:40:11 UTC
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.
Comment 66 Martin Renvoize (ashimema) 2025-05-13 19:40:56 UTC
(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 :)