Support iface condition in <Settings> entries

[MikeTaylor]

Entries passed into the <Settings> component can have an iface element specifying an interface that must be present in order for the settings page to be enabled (analogous to the perm element).

Background

I'm working on a bug (UITEN-266) that would be easier to fix if this change were in place, I'm trying to rush it through in time for me to use it now, but it seems like this is a facility that <Settings> ought to provide.

What do y'all think?

[github-actions]

Jest Unit Test Statistics

0 tests  ±0   0 ✔️ ±0   0s ⏱️ ±0s
0 suites ±0   0 💤 ±0 
0 files   ±0   0 ❌ ±0 

Results for commit 66af63b. ± Comparison against base commit 7c70ed5.

♻️ This comment has been updated with latest results.

[github-actions]

BigTest Unit Test Statistics

    1 files  ±0      1 suites  ±0   15s ⏱️ -2s
441 tests ±0  405 ✔️ ±0  36 💤 ±0  0 ❌ ±0 
443 runs  ±0  407 ✔️ ±0  36 💤 ±0  0 ❌ ±0 

Results for commit 66af63b. ± Comparison against base commit 7c70ed5.

♻️ This comment has been updated with latest results.

[sonarqubecloud]

Kudos, SonarCloud Quality Gate passed!   

0 Bugs
0 Vulnerabilities
0 Security Hotspots
0 Code Smells

No Coverage information
0.0% Duplication

[skomorokh]

I think it makes perfect sense especially the symmetry with perm but that it needn't be any rush as it's tiny, just make a local copy of this for now since it won't be in smart-components until quesnelia.

I do think we had this big effort at one point to have all the routes in index.js and tried to avoid nesting them which had some reasonable goals but I think two places is still much better than n places and it does make sense to have the settings under settings/index.js so I'm not too bothered by this existing overall. Especially because I do hope one day we can have searchable settings and this'd help that...

[skomorokh]

(it's also a small change so maybe it could be in the same patch release as your fix which'd save the hassle of copying/uncopying if Zak is game...)

[MikeTaylor]

Thanks for thoughts on this, @skomorokh. For the short-term need, I didn't make a copy of <Settings>, I just included a little extra code in the application itself: please see (and indeed review) https://github.com/folio-org/ui-tenant-settings/pull/375/files

[skomorokh]

Oh ya, makes sense: this is ultimately really simple, not worth trying to coordinate things but really worth updating the tool so it's streamlined for the next time.

[skomorokh]

Hm, but should iface accept an array? And perm?

[MikeTaylor]

Perm already accepts an "array" — more precisely, a string that is a comma-separated list of permission names, all of which must be present. See https://github.com/folio-org/stripes-core/blob/8d7ac446b13a6c79cc9c79174fc7f7e12e2da134/src/Stripes.js#L102

[skomorokh]

Can't say I'm a fan of that but ehh I guess? At any rate, it seems to me that it might be even more likely that you'll want to specify multiple interfaces than perms so that could be something to consider (potentially for a future PR--it's certainly better to have one interface than no such functionality so if you don't have time to tack that on I'd sooner you kept this alive for merging rather than close it and nothing be done)

[MikeTaylor]

Can't say I'm a fan of that but ehh I guess?

I hear you, but it's been that way for six years: folio-org/stripes-core@2790af7
And was done in response to a genuine irritant: https://issues.folio.org/browse/STCOR-98

So I would not want to change it now.

[skomorokh]

So I would not want to change it now.

Oh ya, exactly. Which sadly(ish) means that we probably should wind up replicating it for iface.

[MikeTaylor]

Maybe. But the situation with hasInterface is a bit more complex than hasPerm: unlike a permission, and interface is not simply either there or not: it's present at a specific version, and the hasInterface method takes an optional second parameter, a version number that we require. So if we want to the full generality here, we'd probably want a way to let the iface element of a <Settings> entry specify that, too.

This is more complex than I want to get at this stage. If it turns out to be a real need, we can find a backwards-compatible way to do it later.

[skomorokh]

Intersting. I'd expect the extra parameter to hasInterface would be very exceptional as the module would define the acceptable range in the optionalInterfaces in package.json would it not? So you'd only need that additional refinement when your particular hasInterface check is more granular than the module's overall requirement.

[MikeTaylor]

That makes sense. I don't remember the history here. But it's been this way since hasInterface was first introduced in 2017, in response to STRIPES-401.

[zburke]

My inclination would be to support this interface: string impl now and later we can add interface: { name:string, version:string }, matching the shape of props accepted by <IfInterface>.

I would prefer interface to iface but I won't fight you on this. (Edit: Mike points out interface is a reserved word we should avoid.)

I can get this into a Poppy patch for bugfest if you want it. I need to publish a release later today.

[vashjs]

tenant-setting

Hi @MikeTaylor, after this change we have bug in tenant-settings - UITEN-268. If we follow a direct link (you must be logged out) to the tenant (for example https://folio-snapshot.dev.folio.org/settings/tenant-settings), then the routes for the locations are not loaded and looks like this:

[MikeTaylor]

Hmm. That will be because Stripes is rendering this Settings page before it's finished doing its own setup (loading the information about which interfaces Okapi knows about). That seems like a Stripes bug to me. Thoughts, @zburke @mkuklis @skomorokh?

[zburke]

@vashjs , I don't follow. You mention "after this change" but this is a draft PR. How can UITEN-268 be related given this hasn't been merged yet?

[vashjs]

@vashjs , I don't follow. You mention "after this change" but this is a draft PR. How can UITEN-268 be related given this hasn't been merged yet?

@zburke In this thread, Mike mentioned that he merged changes for tenant-settings into this PR . It's a reason why I'm writing here

[MikeTaylor]

@vashjs Then you should probably comment on that PR, not this one.

[MikeTaylor]

@vashjs Thanks for moving it across. I'll pick it up there.