Bug 26129 - Add a new 'configurations' table
Summary: Add a new 'configurations' table
Status: Failed QA
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: master
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: 2021-07-23 12:54 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
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

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
Created attachment 107718 [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 2 Tomás Cohen Arazi 2020-08-03 17:50:54 UTC
Created attachment 107719 [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 3 Tomás Cohen Arazi 2020-08-03 17:51:01 UTC
Created attachment 107720 [details] [review]
Bug 26129: DBIx::Class schema updates [DO NOT PUSH]
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
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

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