Bug 26129 - Add a new 'configurations' table
Summary: Add a new 'configurations' table
Status: REOPENED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-08-03 17:15 UTC by Tomás Cohen Arazi
Modified: 2024-02-28 17:42 UTC (History)
12 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 26129: Add new 'configurations' table (8.46 KB, patch)
2020-08-03 17:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26129: Add Koha::Configuration(s) classes (19.02 KB, patch)
2020-08-03 17:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26129: DBIx::Class schema updates [DO NOT PUSH] (3.50 KB, patch)
2020-08-03 17:51 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26129: Add new 'configurations' table (8.61 KB, patch)
2021-03-29 15:51 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 26129: Add Koha::Configuration(s) classes (19.02 KB, patch)
2021-03-29 15:51 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 26129: DBIx::Class schema updates [DO NOT PUSH] (2.26 KB, patch)
2021-03-29 15:51 UTC, Agustín Moyano
Details | Diff | Splinter Review
Bug 26129: Add new 'configurations' table (8.61 KB, patch)
2021-03-29 19:21 UTC, David Nind
Details | Diff | Splinter Review
Bug 26129: Add Koha::Configuration(s) classes (19.09 KB, patch)
2021-03-29 19:22 UTC, David Nind
Details | Diff | Splinter Review
Bug 26129: DBIx::Class schema updates [DO NOT PUSH] (2.30 KB, patch)
2021-03-29 19:22 UTC, David Nind
Details | Diff | Splinter Review
Bug 26129: Add new 'configurations' table (8.81 KB, patch)
2021-12-23 15:49 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26129: Add Koha::Configuration(s) classes (19.07 KB, patch)
2021-12-23 15:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26129: DBIx::Class schema updates [DO NOT PUSH] (3.43 KB, patch)
2021-12-23 15:50 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26129: Add new 'configurations' table (9.59 KB, patch)
2021-12-23 18:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26129: Add Koha::Configuration(s) classes (19.07 KB, patch)
2021-12-23 18:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26129: DBIx::Class schema updates [DO NOT PUSH] (3.43 KB, patch)
2021-12-23 18:13 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 26129: Add new 'configurations' table (9.64 KB, patch)
2022-02-05 17:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 26129: Add Koha::Configuration(s) classes (19.12 KB, patch)
2022-02-05 17:35 UTC, David Nind
Details | Diff | Splinter Review
Bug 26129: DBIx::Class schema updates [DO NOT PUSH] (3.47 KB, patch)
2022-02-05 17:35 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Tomás Cohen Arazi 2020-08-03 17:15:48 UTC
This table should be designed from the beginning to allow global and per-library configuration entries.

It would replicate the circulation_rules schema, but adapted to be a(n eventual) replacement for system preferences.

This came up on my development for an SMTP configuration page, in which I originally added an smtp_servers page, that could really be generalized like this for other uses.
Comment 1 Tomás Cohen Arazi 2020-08-03 17:50:49 UTC Comment hidden (obsolete)
Comment 2 Tomás Cohen Arazi 2020-08-03 17:50:54 UTC Comment hidden (obsolete)
Comment 3 Tomás Cohen Arazi 2020-08-03 17:51:01 UTC Comment hidden (obsolete)
Comment 4 David Cook 2020-08-03 23:21:48 UTC
I'd like to see a more real-world example for Configurations.t. At the moment, you're storing a hostname as the value, but on Bug 22343 there is actually quite a few different values (e.g. hostname, port, ssl, etc) that need to be stored. 

How would this be stored here?
Comment 5 Tomás Cohen Arazi 2020-08-03 23:38:39 UTC
I've been playing with rewriting Koha::SMTP::Server(s) in terms of this, so we require and set configurations without fetching each value where need. But I found a rock on the road when it comes to using this on the API, so still thinking about this. I like the idea, but I'm not sure how to group things: separate class wrapping the grouped values, or maybe a separate table for grouping things.

Either of them makes it hard to use with objects->search of we want to allow filtering by column.

Storing JSON has the same limitation with current MariaDB/MySql versions
Comment 6 Marcel de Rooy 2020-08-05 11:13:08 UTC
Just reading diagonally, sounds like yet another configuration to manage? Moving all preferences into it would be a dramatic refactoring that we probably do not finish?
Comment 7 Tomás Cohen Arazi 2020-09-11 19:58:31 UTC
The idea is that we have many places in which we define global configurations (sysprefs) and then override it somewhere, locally (e.g. min password length on category config).

In that case, if we wanted to have a screen for managing this with a tree like structure, maybe handling all password-related configs globally or per-library or per-category, a-la circulation rules, we would benefit from this simple design and it would set a clear path forward.

On bug 19889 we clearly added many configurations, in different places, and while it has the flexibility that is required, and we can give pointers to the end-user, it is clear that having a single page to manage all related things, was pointed out by the requestor as something valuable.

And this generic approach on the DB might be the best way to store this. Even if we don't migrate all sysprefs into this.

That said, I moved forward with an smtp_servers table and won't be back to this unless there's some interest.

It felt like the right thing to do, though.
Comment 8 David Cook 2020-09-13 23:47:32 UTC
(In reply to Tomás Cohen Arazi from comment #7)
> It felt like the right thing to do, though.

I like the idea in theory but I'm not 100% sure how I feel about this implementation.

That being said, I can't point to anything technical I don't like about it except for the large number of nulls. And the large number of nulls do make sense for a simple design. 

I don't think I've done much work that requires anything more granular than global-config, so I don't think that I'm best placed to comment on this meaningfully.
Comment 9 Tomás Cohen Arazi 2020-09-14 18:09:27 UTC
(In reply to David Cook from comment #8)
> (In reply to Tomás Cohen Arazi from comment #7)
> > It felt like the right thing to do, though.
> 
> I like the idea in theory but I'm not 100% sure how I feel about this
> implementation.
> 
> That being said, I can't point to anything technical I don't like about it
> except for the large number of nulls. And the large number of nulls do make
> sense for a simple design. 
> 
> I don't think I've done much work that requires anything more granular than
> global-config, so I don't think that I'm best placed to comment on this
> meaningfully.

This is really for multi branch sites that need more granularity. Minor things like the default MARC organization code are defined globally and then overridden with a new column in 'branches'. This pattern can be problematic in my opinion, handling it on a per-case basis.

And adding columns everywhere we want to make a syspref overridden per-branch looks worse than NULL values.
Comment 10 David Cook 2020-09-14 23:15:58 UTC
(In reply to Tomás Cohen Arazi from comment #9)
> This is really for multi branch sites that need more granularity. Minor
> things like the default MARC organization code are defined globally and then
> overridden with a new column in 'branches'. This pattern can be problematic
> in my opinion, handling it on a per-case basis.
> 
> And adding columns everywhere we want to make a syspref overridden
> per-branch looks worse than NULL values.

Those are good points. 

I've wondered a bit about branch_configurations, item_configurations, and patron_configurations, but then there are times when you need combinations of all 3, so having just one configurations table as you've done does make sense. 

Something about the configurations table *feels* wrong though. That said, maybe it's just my mind trying to prematurely optimize. I suppose even if you had 1000 library branches, so long as you indexed by branch, patron category, and item type... you could do a lookup pretty easily. So scale shouldn't be an issue. And having 1 big table versus smaller tables or configuration files makes it easier to add new configuration options (although possibly with a downtime cost at upgrade time for adding columns to all existing rows if you have a large configurations table but I doubt the configurations table would ever grow that large, and if it did then we could split it out more at that point).

I suppose I can't think of any good reason not to do a "configurations" table.
Comment 11 Tomás Cohen Arazi 2021-03-12 15:16:51 UTC
Ok, I'm resurrecting this one. I saw bug 27945, which proposes to add a new attribute to categories, and it will end-up being the same old pattern:

- A syspref for a global/default value
- A per-category value

I will suggest adding a special page for handling article requests configurations (because there will be more, probably) and have a way to set this rate limit the circ-rules way.
Comment 12 David Nind 2021-03-26 19:00:07 UTC
Patches no longer apply 8-(..
Comment 13 Agustín Moyano 2021-03-29 15:51:50 UTC
Created attachment 118970 [details] [review]
Bug 26129: Add new 'configurations' table

This patch introduces a new table to kohastructure.sql and an atomic
update script to add the table on update. It also includes the
corresponding schema file.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 14 Agustín Moyano 2021-03-29 15:51:54 UTC
Created attachment 118971 [details] [review]
Bug 26129: Add Koha::Configuration(s) classes

This patch adds the Koha::Configuration(s) classes and tests. It is
similar to the Koha::CirculationRules(s) classes, but with simpler use
cases for now. There's no special handling for '*' as it is expected to
be passed undef instead.

It allows to set arbitrary configuration entries either globally or for
specific library_id, category_id or item_type.

Attribute names match the ones agreed for the API.

To test:
1. Apply this patches
2. Run:
   $ updatedatabase
=> SUCCESS: The atomic update is applied correctly
3. Run:
   $ reset_all
=> SUCCESS: kohastructure.sql is loaded without issues
4. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Configurations.t
=> SUCCESS: Tests pass!
5. Sign off :-D

Sponsored-by: ByWater Solutions
Comment 15 Agustín Moyano 2021-03-29 15:51:58 UTC
Created attachment 118972 [details] [review]
Bug 26129: DBIx::Class schema updates [DO NOT PUSH]
Comment 16 Agustín Moyano 2021-03-29 15:53:03 UTC
(In reply to David Nind from comment #12)
> Patches no longer apply 8-(..

Hi David, just rebased patches.. could you try again?

Thanks
Comment 17 David Nind 2021-03-29 19:21:58 UTC
Created attachment 118982 [details] [review]
Bug 26129: Add new 'configurations' table

This patch introduces a new table to kohastructure.sql and an atomic
update script to add the table on update. It also includes the
corresponding schema file.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 18 David Nind 2021-03-29 19:22:02 UTC
Created attachment 118983 [details] [review]
Bug 26129: Add Koha::Configuration(s) classes

This patch adds the Koha::Configuration(s) classes and tests. It is
similar to the Koha::CirculationRules(s) classes, but with simpler use
cases for now. There's no special handling for '*' as it is expected to
be passed undef instead.

It allows to set arbitrary configuration entries either globally or for
specific library_id, category_id or item_type.

Attribute names match the ones agreed for the API.

To test:
1. Apply this patches
2. Run:
   $ updatedatabase
=> SUCCESS: The atomic update is applied correctly
3. Run:
   $ reset_all
=> SUCCESS: kohastructure.sql is loaded without issues
4. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Configurations.t
=> SUCCESS: Tests pass!
5. Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: David Nind <david@davidnind.com>
Comment 19 David Nind 2021-03-29 19:22:06 UTC
Created attachment 118984 [details] [review]
Bug 26129: DBIx::Class schema updates [DO NOT PUSH]

Signed-off-by: David Nind <david@davidnind.com>
Comment 20 Martin Renvoize 2021-04-19 10:58:01 UTC
Is there a new example bug where this is getting used?
Comment 21 Nick Clemens 2021-07-23 12:54:43 UTC Comment hidden (obsolete)
Comment 22 Tomás Cohen Arazi 2021-12-23 15:49:58 UTC Comment hidden (obsolete)
Comment 23 Tomás Cohen Arazi 2021-12-23 15:50:03 UTC Comment hidden (obsolete)
Comment 24 Tomás Cohen Arazi 2021-12-23 15:50:09 UTC
Created attachment 128894 [details] [review]
Bug 26129: DBIx::Class schema updates [DO NOT PUSH]
Comment 25 Tomás Cohen Arazi 2021-12-23 15:51:41 UTC
Rebased.

(In reply to Nick Clemens from comment #21)
> Small QA failures:
>  OK	Koha/Schema/Result/Itemtype.pm
>  FAIL	installer/data/mysql/kohastructure.sql
>    FAIL	  boolean_vs_tinyint
> 		The new column (type) for table configurations is using BOOLEAN as type,
> must be TINYINT(1) instead, see the SQL12 coding guideline

This is a false positive. The DB structure defines an ENUM, that can take the string 'boolean' as a possible value, and the QA checks don't notice it is an ENUM type.

>  FAIL	t/db_dependent/Koha/Configurations.t
>    FAIL	  file permissions
> 		File must have the exec flag

Fixed, thanks!
Comment 26 Tomás Cohen Arazi 2021-12-23 15:54:50 UTC
(In reply to Martin Renvoize from comment #20)
> Is there a new example bug where this is getting used?

My plan was to do it for bug 22457. Because it felt like changing the model for each single configuration specific to a category sounds like bad design. And 'circulation_rules' doesn't sound like the use case.

(In reply to Marcel de Rooy from comment #6)
> Just reading diagonally, sounds like yet another configuration to manage?
> Moving all preferences into it would be a dramatic refactoring that we
> probably do not finish?

I agree. I will probably abandon this bug.
Comment 27 Tomás Cohen Arazi 2021-12-23 18:12:29 UTC
(In reply to Nick Clemens from comment #21)
> Small QA failures:
>  OK	Koha/Schema/Result/Itemtype.pm
>  FAIL	installer/data/mysql/kohastructure.sql
>    FAIL	  boolean_vs_tinyint
> 		The new column (type) for table configurations is using BOOLEAN as type,
> must be TINYINT(1) instead, see the SQL12 coding guideline

https://gitlab.com/koha-community/qa-test-tools/-/merge_requests/42
Comment 28 Tomás Cohen Arazi 2021-12-23 18:13:29 UTC
Created attachment 128904 [details] [review]
Bug 26129: Add new 'configurations' table

This patch introduces a new table to kohastructure.sql and an atomic
update script to add the table on update. It also includes the
corresponding schema file.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>
Comment 29 Tomás Cohen Arazi 2021-12-23 18:13:38 UTC
Created attachment 128905 [details] [review]
Bug 26129: Add Koha::Configuration(s) classes

This patch adds the Koha::Configuration(s) classes and tests. It is
similar to the Koha::CirculationRules(s) classes, but with simpler use
cases for now. There's no special handling for '*' as it is expected to
be passed undef instead.

It allows to set arbitrary configuration entries either globally or for
specific library_id, category_id or item_type.

Attribute names match the ones agreed for the API.

To test:
1. Apply this patches
2. Run:
   $ updatedatabase
=> SUCCESS: The atomic update is applied correctly
3. Run:
   $ reset_all
=> SUCCESS: kohastructure.sql is loaded without issues
4. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Configurations.t
=> SUCCESS: Tests pass!
5. Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: David Nind <david@davidnind.com>
Comment 30 Tomás Cohen Arazi 2021-12-23 18:13:48 UTC
Created attachment 128906 [details] [review]
Bug 26129: DBIx::Class schema updates [DO NOT PUSH]
Comment 31 David Nind 2022-02-05 17:35:06 UTC
Created attachment 130189 [details] [review]
Bug 26129: Add new 'configurations' table

This patch introduces a new table to kohastructure.sql and an atomic
update script to add the table on update. It also includes the
corresponding schema file.

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 32 David Nind 2022-02-05 17:35:11 UTC
Created attachment 130190 [details] [review]
Bug 26129: Add Koha::Configuration(s) classes

This patch adds the Koha::Configuration(s) classes and tests. It is
similar to the Koha::CirculationRules(s) classes, but with simpler use
cases for now. There's no special handling for '*' as it is expected to
be passed undef instead.

It allows to set arbitrary configuration entries either globally or for
specific library_id, category_id or item_type.

Attribute names match the ones agreed for the API.

To test:
1. Apply this patches
2. Run:
   $ updatedatabase
=> SUCCESS: The atomic update is applied correctly
3. Run:
   $ reset_all
=> SUCCESS: kohastructure.sql is loaded without issues
4. Run:
   $ kshell
  k$ prove t/db_dependent/Koha/Configurations.t
=> SUCCESS: Tests pass!
5. Sign off :-D

Sponsored-by: ByWater Solutions

Signed-off-by: David Nind <david@davidnind.com>

Signed-off-by: David Nind <david@davidnind.com>
Comment 33 David Nind 2022-02-05 17:35:16 UTC
Created attachment 130191 [details] [review]
Bug 26129: DBIx::Class schema updates [DO NOT PUSH]

Signed-off-by: David Nind <david@davidnind.com>
Comment 34 Jonathan Druart 2022-02-08 17:43:17 UTC
(In reply to Tomás Cohen Arazi from comment #26)
> (In reply to Marcel de Rooy from comment #6)
> > Just reading diagonally, sounds like yet another configuration to manage?
> > Moving all preferences into it would be a dramatic refactoring that we
> > probably do not finish?
> 
> I agree. I will probably abandon this bug.

Hum... What's next here?
Comment 35 Tomás Cohen Arazi 2022-02-08 17:54:20 UTC
(In reply to Jonathan Druart from comment #34)
> (In reply to Tomás Cohen Arazi from comment #26)
> > (In reply to Marcel de Rooy from comment #6)
> > > Just reading diagonally, sounds like yet another configuration to manage?
> > > Moving all preferences into it would be a dramatic refactoring that we
> > > probably do not finish?
> > 
> > I agree. I will probably abandon this bug.
> 
> Hum... What's next here?

I see this pattern will become useful, I thought we would assign SMTP servers to libraries in a circ rules-like fashion, but I couldn't have that dev depend on this one so it moved forward in another direction.

But it is not a priority until devs find it useful.

As of the question, to make it useful:
- It should consider library groups from the beginning
- We need some UI for dealing with this, that can be reused to make configuration pages for modules/tools relying on this.
Comment 36 Jonathan Druart 2022-02-08 18:06:20 UTC
Should we then move it out of the NQA queue and revive it when we will need it?
Comment 37 Tomás Cohen Arazi 2022-02-09 12:37:43 UTC
Bug 29924 could be done with this...
Comment 38 Martin Renvoize 2023-10-26 15:30:15 UTC
Adding bug 18203 to the pile that would benefit from this.