|
Description
Martin Renvoize (ashimema)
2023-12-21 09:03:57 UTC
Created attachment 160171 [details] [review] Bug 35625: Add is_system to additional_fields table Created attachment 160172 [details] [review] Bug 35625: DBIC Schema update Created attachment 160173 [details] [review] Bug 35625: Update api definition Created attachment 160174 [details] [review] Bug 35625: Update UI to prevent deletion/edition Created attachment 160175 [details] [review] Bug 35625: Prevent deletion at controller Created attachment 160176 [details] [review] Bug 35625: Add boolean flag to schema 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? I use it in Bug 34324 - Merge OPACProblemReport and CatalogConcern functions 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. 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. 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 ;) (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. Then maybe this needs to be abstracted, to make it more flexible. 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) 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? 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. Created attachment 173567 [details] [review] Bug 35625: Add is_system to additional_fields table Created attachment 173568 [details] [review] Bug 35625: Update api definition Created attachment 173569 [details] [review] Bug 35625: Update UI to prevent deletion/edition Created attachment 173570 [details] [review] Bug 35625: Prevent deletion at controller Created attachment 173571 [details] [review] Bug 35625: Add boolean flag to schema Just a rebase to get the rest of the tree working again Created attachment 173573 [details] [review] Bug 35625: Add is_system to additional_fields table Created attachment 173574 [details] [review] Bug 35625: DBIC Schema update Created attachment 173575 [details] [review] Bug 35625: Update api definition Created attachment 173576 [details] [review] Bug 35625: Update UI to prevent deletion/edition Created attachment 173577 [details] [review] Bug 35625: Prevent deletion at controller Created attachment 173578 [details] [review] Bug 35625: Add boolean flag to schema |