Bug 35451

Summary:	Add tablename field to additional_field_values
Product:	Koha	Reporter:	Martin Renvoize <martin.renvoize>
Component:	Architecture, internals, and plumbing	Assignee:	Martin Renvoize <martin.renvoize>
Status:	Needs Signoff ---	QA Contact:	Testopia <testopia>
Severity:	enhancement
Priority:	P5 - low	CC:	anneli.osterman, david, jonathan.druart, julian.maurice, pedro.amorim, tomascohen
Version:	Main
Hardware:	All
OS:	All
See Also:	https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35044
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

Description Martin Renvoize 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 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 2023-11-30 12:11:46 UTC

Created attachment 159391 [details] [review]
Bug 35451: Record table in set_additional_fields

Comment 3 Martin Renvoize 2023-11-30 12:11:49 UTC

Created attachment 159392 [details] [review]
Bug 35451: Update relations

Comment 4 Martin Renvoize 2023-11-30 12:11:51 UTC

Created attachment 159393 [details] [review]
Bug 35451: Schema

Comment 5 Martin Renvoize 2023-11-30 12:11:54 UTC

Created attachment 159394 [details] [review]
Bug 35451: Fix test data

Comment 6 Martin Renvoize 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 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 2023-12-12 16:35:15 UTC

Created attachment 159748 [details] [review]
Bug 35451: Record table in set_additional_fields

Comment 9 Martin Renvoize 2023-12-12 16:35:18 UTC

Created attachment 159749 [details] [review]
Bug 35451: Update relations

Comment 10 Martin Renvoize 2023-12-12 16:35:20 UTC

Created attachment 159750 [details] [review]
Bug 35451: Schema

Comment 11 Martin Renvoize 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.