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.
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>
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>
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.
"SubfieldsToAllowForRestrictedBatchmod", "SubfieldsToAllowForRestrictedEditing", and "SubfieldsToUseWhenPrefill" are separated by spaces. "AdvancedSearchLanguages" is separated by pipe or comma, which would make this method particularly problematic...
(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.
(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...)
+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 :)
(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
(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 ;)
(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).
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?
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.
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.
(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.
Back to SO
Next steps? Is there a plan here?
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.
My comment9 still holds. You did not spend these 5 minutes yet. For me this is a FQA ?
(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
(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?
(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.
(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!
(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 ;)
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>
Pushed to master for 23.11. Nice work everyone, thanks!
Enhancement not pushed to 23.05.x I'll push if is becomes a needed dependancy
And we are not using it anywhere? Next step?
(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.
I could have sworn there were uses... the brain fades.
(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