Bug 29033 - Add C4::Context->multivalue_preference
Summary: Add C4::Context->multivalue_preference
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: unspecified
Hardware: All All
: P5 - low enhancement (vote)
Assignee: Tomás Cohen Arazi
QA Contact: Marcel de Rooy
URL:
Keywords: rel_23_11_candidate
Depends on:
Blocks:
 
Reported: 2021-09-15 18:56 UTC by Tomás Cohen Arazi
Modified: 2024-03-19 13:34 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This addition adds a simple way to retrieve pipe delimited list type system preferences as arrays.
Version(s) released in:
23.11.00


Attachments
Bug 29033: Add C4::Context->multivalue_preference (2.37 KB, patch)
2021-09-16 12:01 UTC, Tomás Cohen Arazi
Details | Diff | Splinter Review
Bug 29033: Add C4::Context->multivalue_preference (2.42 KB, patch)
2021-09-18 22:14 UTC, David Nind
Details | Diff | Splinter Review
Bug 29033: Add C4::Context->multivalue_preference (2.52 KB, patch)
2022-10-21 06:39 UTC, Marcel de Rooy
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 2021-09-15 18:56:36 UTC
I've seen many places in which a syspref is read, and then split used to convert the contents into a list.

It seems it would be simple to add a method to do that.
Comment 1 Tomás Cohen Arazi 2021-09-16 12:01:13 UTC
Created attachment 124921 [details] [review]
Bug 29033: Add C4::Context->multivalue_preference

I've seen several places in which a syspref is retrieved and then
splitted using split and the fact they are pipe-separated strings.

It seems it would be simple (and handy) to add a method to do that.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/Context.t
=> SUCCESS: Tests pass, a pipe-separated syspref is correctly retrieved
as an arrayref.
3. Sign off :-D

Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Comment 2 David Nind 2021-09-18 22:14:16 UTC
Created attachment 125031 [details] [review]
Bug 29033: Add C4::Context->multivalue_preference

I've seen several places in which a syspref is retrieved and then
splitted using split and the fact they are pipe-separated strings.

It seems it would be simple (and handy) to add a method to do that.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/Context.t
=> SUCCESS: Tests pass, a pipe-separated syspref is correctly retrieved
as an arrayref.
3. Sign off :-D

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

Signed-off-by: David Nind <david@davidnind.com>
Comment 3 David Cook 2021-09-20 00:14:33 UTC
I don't know about this one...

There are also multi value preferences that don't use pipes. Many preferences like "NotesToHide" and "ILS-DI:AuthorizedIPs" use commas instead.
Comment 4 David Cook 2021-09-20 00:17:18 UTC
"SubfieldsToAllowForRestrictedBatchmod", "SubfieldsToAllowForRestrictedEditing", and "SubfieldsToUseWhenPrefill" are separated by spaces. 

"AdvancedSearchLanguages" is separated by pipe or comma, which would make this method particularly problematic...
Comment 5 Tomás Cohen Arazi 2021-09-20 11:27:26 UTC
(In reply to David Cook from comment #3)
> I don't know about this one...
> 
> There are also multi value preferences that don't use pipes. Many
> preferences like "NotesToHide" and "ILS-DI:AuthorizedIPs" use commas instead.

In my opinion, that's a totally different problem, which we could cleary address by making all multi-valued sysprefs behave consistently.

Thanks for the examples, BTW.
Comment 6 David Cook 2021-09-20 23:44:38 UTC
(In reply to Tomás Cohen Arazi from comment #5)
> In my opinion, that's a totally different problem, which we could cleary
> address by making all multi-valued sysprefs behave consistently.

I was thinking that a bit as well. Don't we have some multi-value preferences that use select lists as well? I'm not sure how those work though. 

I've been debating whether it would be better to return different data types with C4::Context->preference or if having C4::Context->multivalue_preference makes sense... and I think you might be right. (I mean if we were starting over, it would make sense for the preference to be an object and then that object would have the appropriate methods, but we have to work with what we have...)
Comment 7 Marcel de Rooy 2021-10-07 07:00:27 UTC
+sub multivalue_preference {
+    my ( $self, $preference ) = @_;
+
+    my $syspref = $self->preference($preference) // q{};
+    my $values  = [ split qr{\|}, $syspref ];
+
+    return $values;
+}

I agree that we need a parameter here to pass other separators.
Multi value, multi sepa :)
Comment 8 Tomás Cohen Arazi 2021-10-28 18:43:39 UTC
(In reply to Marcel de Rooy from comment #7)
> +sub multivalue_preference {
> +    my ( $self, $preference ) = @_;
> +
> +    my $syspref = $self->preference($preference) // q{};
> +    my $values  = [ split qr{\|}, $syspref ];
> +
> +    return $values;
> +}
> 
> I agree that we need a parameter here to pass other separators.
> Multi value, multi sepa :)

Counter proposal:
1. Accept this as-is
2. Implement a nice UI to abstract users from the separator being used
3. Migrate sysprefs into  using this
Comment 9 Marcel de Rooy 2021-10-29 06:20:09 UTC
(In reply to Tomás Cohen Arazi from comment #8)
> (In reply to Marcel de Rooy from comment #7)
> > +sub multivalue_preference {
> > +    my ( $self, $preference ) = @_;
> > +
> > +    my $syspref = $self->preference($preference) // q{};
> > +    my $values  = [ split qr{\|}, $syspref ];
> > +
> > +    return $values;
> > +}
> > 
> > I agree that we need a parameter here to pass other separators.
> > Multi value, multi sepa :)
> 
> Counter proposal:
> 1. Accept this as-is
> 2. Implement a nice UI to abstract users from the separator being used
> 3. Migrate sysprefs into  using this

Point 1: No ! :)
Point 2: Fine with me.
Point 3: Great. (Just gradually move away from sending the sepa param.)

You need 5 mins to do the thing. We need 5 years for the rest ;)
Comment 10 Tomás Cohen Arazi 2022-01-03 11:54:47 UTC
(In reply to Marcel de Rooy from comment #9)
> (In reply to Tomás Cohen Arazi from comment #8)
> > (In reply to Marcel de Rooy from comment #7)
> > > +sub multivalue_preference {
> > > +    my ( $self, $preference ) = @_;
> > > +
> > > +    my $syspref = $self->preference($preference) // q{};
> > > +    my $values  = [ split qr{\|}, $syspref ];
> > > +
> > > +    return $values;
> > > +}
> > > 
> > > I agree that we need a parameter here to pass other separators.
> > > Multi value, multi sepa :)
> > 
> > Counter proposal:
> > 1. Accept this as-is
> > 2. Implement a nice UI to abstract users from the separator being used
> > 3. Migrate sysprefs into  using this
> 
> Point 1: No ! :)
> Point 2: Fine with me.
> Point 3: Great. (Just gradually move away from sending the sepa param.)
> 
> You need 5 mins to do the thing. We need 5 years for the rest ;)

Well, it is always the same chicken-egg situation: one proposes something, for enhancing things, and waits for feedback before moving into changing all the things. And some people want you to do all the things to get convinced of the idea. It's a hard one. I'm fine with ad-hoc splitting code for now. But this is something we could do better, specially point 2. Plus, I don't think there are that many sysprefs that require this (many == more than 20).
Comment 11 Katrin Fischer 2022-01-08 22:50:19 UTC
I think Tomas is right about that we should make the prefs all behave the same (using the same separator) as this is a nuisance right now and requires constant 'looking up' of the syntax to use for each.

Having a nice GUI component would make it even nicer.

I am always a little doubful about wrapping a one liner in perl into an additional method. Basically this:

my $values  = [ split qr{\|}, $syspref ];

So I see a lot of gain in making things more consistent and refining the GUI, but I am not sure as it is right now a separate method is really required?
Comment 12 Tomás Cohen Arazi 2022-01-08 23:23:06 UTC
I always prefer to have a high level method that you just call without knowing how it is internally implemented, and the POD tells you what it will return. It is consistent with some other methods we have for accessing sysprefs even. I don't want the callers to need to know how that syspref needs to be splitter, the same way I think it is non sense for end users to need to worry about that on the front end 

If there is consensus, I can spend a few hours migrating all sysprefs to use the same separator. Keep in mind that we might need to escape whichever separator we pick on sysprefs that used a different one.
Comment 13 Katrin Fischer 2022-01-08 23:32:51 UTC
I am more thinking of people coming new to the project - it's really hard to learn all the 'internals' already right now. You have to get familiar with a whole lot of things. That's just another perspective.

I wonder if some prefs have chosen the separator because of the contents not containing it or because the way they are used makes it easier to use them like that (ILS-DI:AuthorizedIPs maybe?)

I have filed a separate bug for the prefs - bug 29829, so maybe we could move the discussion about the 'end user side' there.
Comment 14 Tomás Cohen Arazi 2022-01-08 23:41:43 UTC
(In reply to Katrin Fischer from comment #13)
> I have filed a separate bug for the prefs - bug 29829, so maybe we could
> move the discussion about the 'end user side' there.

Great idea. My point was to start a discussion about all this, do we are in a good track.
Comment 15 Marcel de Rooy 2022-01-10 08:44:35 UTC
Back to SO
Comment 16 Jonathan Druart 2022-02-01 15:21:51 UTC
Next steps? Is there a plan here?
Comment 17 Tomás Cohen Arazi 2022-10-05 12:32:07 UTC
Can we move this one forward? I keep seeing devs adding random separators and it really makes no sense not having a high-level way to handle multivalued sysprefs.

Jonathan and all: the next steps are moving all multivalued sysprefs into this. And adding a generic JS tool to deal with multivalued sysprefs in a way users don't need to deal with the (internal) separator.

But please, first things first.
Comment 18 Marcel de Rooy 2022-10-05 12:40:13 UTC
My comment9 still holds.
You did not spend these 5 minutes yet.
For me this is a FQA ?
Comment 19 Tomás Cohen Arazi 2022-10-05 12:45:50 UTC
(In reply to Marcel de Rooy from comment #18)
> My comment9 still holds.
> You did not spend these 5 minutes yet.
> For me this is a FQA ?

I disagree with the 'all or nothing' argument. That's why some really nice devs have sitted for years without people willing to test them.

I don't like the idea of over-complicating it with a separator param (that will need to have its own unit tests, handling the escaping that is required on each case, etc). But...

... Now that I think about this, we could probably just stick to YAML internally on the DB. And not even deal with splitting manually.

Thoughts?

PS: Honestly, I didn't know what to answer to your comment. The amount of time needed to do the separator thing is probably the same as moving sysprefs into making them all use the same separator. And I have my personal preferences on what to spend time on too heh
Comment 20 Marcel de Rooy 2022-10-05 12:53:08 UTC
(In reply to Tomás Cohen Arazi from comment #19)
> I disagree with the 'all or nothing' argument. That's why some really nice
> devs have sitted for years without people willing to test them.

Maybe we are not on the same level here? But imo you want all or nothing, and I recommend a gradual approach. Note that we several prefs using other separators still.
 
> I don't like the idea of over-complicating it with a separator param (that
> will need to have its own unit tests, handling the escaping that is required
> on each case, etc). But...

This really should be trivial. 

> ... Now that I think about this, we could probably just stick to YAML
> internally on the DB. And not even deal with splitting manually.

Sounds good but also a larger project.

> PS: Honestly, I didn't know what to answer to your comment. The amount of
> time needed to do the separator thing is probably the same as moving
> sysprefs into making them all use the same separator. And I have my personal
> preferences on what to spend time on too heh

You say All or nothing here?
Comment 21 Tomás Cohen Arazi 2022-10-05 12:56:09 UTC
(In reply to Marcel de Rooy from comment #20)
> 
> You say All or nothing here?

I think we are generally on the same page, but with a slightly different approach. Trying to explain: I want to implement the ideal solution and then migrate things one by one into it. You are proposing a somewhat intermediate thing that will need to be redone (removing the separator). The goal is the same.

I'll try to provide an implementation of the YAML version ASAP.
Comment 22 Marcel de Rooy 2022-10-05 12:58:09 UTC
(In reply to Tomás Cohen Arazi from comment #21)
> (In reply to Marcel de Rooy from comment #20)
> > 
> > You say All or nothing here?
> 
> I think we are generally on the same page, but with a slightly different
> approach. Trying to explain: I want to implement the ideal solution and then
> migrate things one by one into it. You are proposing a somewhat intermediate
> thing that will need to be redone (removing the separator). The goal is the
> same.
> 
> I'll try to provide an implementation of the YAML version ASAP.

Great!
Comment 23 Marcel de Rooy 2022-10-21 06:39:15 UTC
(In reply to Tomás Cohen Arazi from comment #21)
> I think we are generally on the same page, but with a slightly different
> approach. Trying to explain: I want to implement the ideal solution and then
> migrate things one by one into it. You are proposing a somewhat intermediate
> thing that will need to be redone (removing the separator). The goal is the
> same.

Oke. Lets push the ideal solution then ;)
Comment 24 Marcel de Rooy 2022-10-21 06:39:39 UTC
Created attachment 142291 [details] [review]
Bug 29033: Add C4::Context->multivalue_preference

I've seen several places in which a syspref is retrieved and then
splitted using split and the fact they are pipe-separated strings.

It seems it would be simple (and handy) to add a method to do that.

To test:
1. Apply this patch
2. Run:
   $ kshell
  k$ prove t/Context.t
=> SUCCESS: Tests pass, a pipe-separated syspref is correctly retrieved
as an arrayref.
3. Sign off :-D

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

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

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 25 Tomás Cohen Arazi 2023-10-10 13:56:02 UTC
Pushed to master for 23.11.

Nice work everyone, thanks!
Comment 26 Fridolin Somers 2023-10-12 20:56:19 UTC
Enhancement not pushed to 23.05.x
I'll push if is becomes a needed dependancy
Comment 27 Jonathan Druart 2024-03-18 15:28:48 UTC
And we are not using it anywhere? Next step?
Comment 28 Tomás Cohen Arazi 2024-03-19 12:56:19 UTC
(In reply to Jonathan Druart from comment #27)
> And we are not using it anywhere? Next step?

I might have made a mistake/oversight when pushed this. I was sure I abandoned it.

I'll file a follow-up bug for using it.
Comment 29 Martin Renvoize 2024-03-19 13:01:24 UTC
I could have sworn there were uses... the brain fades.
Comment 30 Tomás Cohen Arazi 2024-03-19 13:16:04 UTC
(In reply to Martin Renvoize from comment #29)
> I could have sworn there were uses... the brain fades.

All good :-D I'm gonna do the follow-ups