Bug 35625 - Add support for system flag to additional fields
Summary: Add support for system flag to additional fields
Status: Needs Signoff
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on: 35044
Blocks: 32751 34324
  Show dependency treegraph
 
Reported: 2023-12-21 09:03 UTC by Martin Renvoize
Modified: 2024-04-23 18:58 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:


Attachments
Bug 35625: Add is_system to additional_fields table (2.14 KB, patch)
2023-12-21 09:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35625: DBIC Schema update (1.63 KB, patch)
2023-12-21 09:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35625: Update api definition (883 bytes, patch)
2023-12-21 09:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35625: Update UI to prevent deletion/edition (2.02 KB, patch)
2023-12-21 09:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35625: Prevent deletion at controller (1.43 KB, patch)
2023-12-21 09:49 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 35625: Add boolean flag to schema (901 bytes, patch)
2023-12-21 09:49 UTC, Martin Renvoize
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Martin Renvoize 2023-12-21 09:03:57 UTC
Additional fields could be used to extend some multi-use tables by default without needing to extend the initial table.  It would be helpful to be able to mark such additional fields with the is_system flag to prevent user deletion.
Comment 1 Martin Renvoize 2023-12-21 09:49:10 UTC
Created attachment 160171 [details] [review]
Bug 35625: Add is_system to additional_fields table
Comment 2 Martin Renvoize 2023-12-21 09:49:12 UTC
Created attachment 160172 [details] [review]
Bug 35625: DBIC Schema update
Comment 3 Martin Renvoize 2023-12-21 09:49:15 UTC
Created attachment 160173 [details] [review]
Bug 35625: Update api definition
Comment 4 Martin Renvoize 2023-12-21 09:49:17 UTC
Created attachment 160174 [details] [review]
Bug 35625: Update UI to prevent deletion/edition
Comment 5 Martin Renvoize 2023-12-21 09:49:20 UTC
Created attachment 160175 [details] [review]
Bug 35625: Prevent deletion at controller
Comment 6 Martin Renvoize 2023-12-21 09:49:22 UTC
Created attachment 160176 [details] [review]
Bug 35625: Add boolean flag to schema
Comment 7 Katrin Fischer 2023-12-23 13:44:25 UTC
Do you have a use case/example maybe?

I am a little hesitant because the linked fields are harder to query for libraries and we need to join another table (performance?). Would we need guidelines/agreement about when to use additional fields and when to extend the original tables?
Comment 8 Martin Renvoize 2023-12-24 08:23:13 UTC
I use it in Bug 34324 - Merge OPACProblemReport and CatalogConcern functions
Comment 9 Jonathan Druart 2024-01-04 07:28:02 UTC
The purpose of this table/ft was to add specific data. Flagging a fields as "is mandatory for the system to work correctly" is wrong by design IMO. It should be a column in the original table.
Comment 10 Pedro Amorim 2024-01-04 10:28:31 UTC
Hey, adding my 2 cents here in what I hope to be a constructive addition.

(In reply to Jonathan Druart from comment #9)
> The purpose of this table/ft was to add specific data. Flagging a fields as
> "is mandatory for the system to work correctly" is wrong by design IMO. It
> should be a column in the original table.

I can definitely see where this argument is coming from, although I don't 100% agree with it being wrong by design. This is still specific data, it's just additional fields that Koha's code will rely on and can't be removed.

Some of the alternatives here are:
- Add the columns page_url and biblio_id to the ticket table, even though only ticket.source=catalog will use the biblio_id column and only ticket.source=opac_problem will use the page_url, this would get more and more confusing and hard to maintain as we go in the future (i.e. every new single type of ticket will potentially require new niche fields for their own).

- Add a new 'tickets_attributes' table (and have this be the 4th relational metadata table we have in Koha, alongside additional_fields, patron_attributes and illrequestattributes. This would allow for a ticket of source=catalog to have the biblio_id and the ticket of source=opac_problem to have the page_url, without adding new columns to the table and making it flexible enough for future ticket types (sources). The problem here is that all this logic already exists in additional_fields.

- Have a 'other_stuff' column in the tickets table that would store a json object that could store whatever in it and logic would handle what it wants to retrieve/store depending on the ticket.source (like plugin_data). This is not ideal either.

----

The advantages of having an additional_field:is_system are:
- This is_system additional_field will automatically benefit from all the features from additional_fields (it may potentially be an authorized value list, repeatable, searchable, filterable, and so on) AND it would not pollute the tickets table (or any other table that would need to make use of this) with niche fields only used by some instantiations of the table, and not others.
- Existing code from additional_fields is reused, thus any future fixes and enhancements also benefit this is_system additional_field. Whereas in any of the alternatives above if you need a specific field to be searchable on the datatable for example you will need to update the data table code. By using the additional_fields with is_system, the programmer just needs to set the field as searchable and nothing else needs to be done, again, benefiting from the additional_fields native features.
Comment 11 Jonathan Druart 2024-01-04 11:20:11 UTC
Then maybe you need 2 different DB tables.

One perl module (generic ticket) that will be inherited by the two specific ticket modules.

It's not a bad thing to have different DB tables when your objects are different. Then if you need repeatable fields, you create yet another table with  1-n relationship. It's... relational models and database normalization ;)
Comment 12 Pedro Amorim 2024-01-04 12:11:26 UTC
(In reply to Jonathan Druart from comment #11)
> Then maybe you need 2 different DB tables.
> 
> One perl module (generic ticket) that will be inherited by the two specific
> ticket modules.
> 
> It's not a bad thing to have different DB tables when your objects are
> different. Then if you need repeatable fields, you create yet another table
> with  1-n relationship. It's... relational models and database normalization
> ;)

Yes but you're highlighting exactly why this approach is worth considering in the first place. Why re-implement anything that's already been implemented at the additional_fields level?

We already have patron_custom_attributes and additional_fields, 2 implementations of what is essentially the same thing, but now need to be maintained and supported individually. This is exactly what we're aiming to prevent here: implement yet another redundant metadata table that needs to do something that's already implemented in Koha.
Comment 13 Jonathan Druart 2024-01-04 12:31:54 UTC
Then maybe this needs to be abstracted, to make it more flexible.
Comment 14 Katrin Fischer 2024-01-22 20:46:39 UTC
So this feels no like we are trying to implement something like inheritance in the database. We have a "base" ticket table and we have an additional table for any specifics. I think I get the idea now.

The idea is very similar to the illrequestattributes, more than to the patron_attributes. The patron_attributes implement limited functionality for workflows, while the illrequestattributes are the data storage for any additional data a backend might need to function properly.

At the moment I believe we are thinking to unify the functionality of:
* OPAC problem reports
* Catalog concerns
* <a third thing I don't manage to remember right now>

As always there is pros and cons to everything. The problems I see with additional attributes in their key/value form are:
* No FK constraints on database level.
* No proper datatypes. We store everything as a varchar. This can complicate validation and reporting.
* Ease of writing reports with all columns (libraries have repeatedly told me they find it hard)

I think there is a use case here... but I am not totally persuaded yet that it's strong enough for the examples given.

For the reasons mentioned above, I feel like at least biblio_id might be better off as a column allowing for FK etc. and easy joining with other tables. And logging it could be of some use for the problem reports too (a lot of opac-pages are "biblio record" based (detail, isbd, marc, request, article_request ...) and logging it could allow for interesting reporting)
Comment 15 Pedro Amorim 2024-01-23 09:43:22 UTC
Thanks for the input Katrin, these are fair points.

> * No FK constraints on database level.
I'm not sure what the relevant disadvantages of this apply here. Possibility for stuff like child rows without corresponding parent rows for example, this can (and should) be prevented by being enforced through code at the application level.

> * No proper datatypes. We store everything as a varchar. This can complicate
> validation and reporting.
Datatypes (and validation) is another thing we should handle at application level imo for these use cases (related metadata tables).
When adding a new additional_field/patron_attribute_type the user should also be able to specify its data type (text, number, currency, date, av_list, relationship, etc).
This will require a overhaul to patron_attribute_types, yes, but an overhaul is already required anyway (merging additional_fields and patron_attribute_types).

> * Ease of writing reports with all columns (libraries have repeatedly told
> me they find it hard)
It's already the case for additional_fields and patron_attribute_types, we're not introducing a new pattern in terms of ease (or lack thereof) of writing reports. This could possibly be mitigated by having a few more examples in the wiki people can copy off from?
Comment 16 Katrin Fischer 2024-01-28 14:17:14 UTC
I don't want this sitting for too long without a decision/solution to not block the depending development, but I'd really like to get some more eyes/brains on it.
Adding some more in CC, please feel free to also ask on IRC/mailing list/IRC meeting as you see fit.