This is blocker for 23.05

Created attachment 150204 [details] [review]
Bug 33606: Add a erm/config route to retrieve the ERM config needed for the Vue app

This could be extended later in bug 32968 to pass the permission of the
logged in user.

This looks like a reasonably approach to me.. very minor wonder if whether the path is right.. /erm/config or whether it should be more generalised as /config/erm or /config?module=erm

On other minor wonder.. should we perhaps add a 'type' key.. so denote the config content type (yaml, csv, string etc).. Would that help in future for parsing the various different content we have in sysprefs?

I feel like we either (a) refactor everything and do server-side rendering as we do now with Template::Toolkit, or (b) provide a way to fetch the module configuration, as the patch proposes.

I think (a) is not doable or worth. So I pick (b).

I generally agree with an endpoint like /config/:module_name for retrieving things.
I'd prefer we come up with something more generic than just passing the allow-listed sysprefs. We could talk about allowed-to-use features like:

{
    features: {
        packages: [ add, update, delete, list ]
    }
}

If we look at our current approach (C4::Auth) it is easy to spot that we also add information about the permissions the current user has (CAN_<permission>).

I'm not sure about the data structure. We should just copy what other projects do.

Example found in the wild:

https://www.permify.co/post/implementing-role-based-access-control-in-vue-js

(In reply to Tomás Cohen Arazi from comment #6)
> Example found in the wild:
> 
> https://www.permify.co/post/implementing-role-based-access-control-in-vue-js

Yes but this is "user.json", we are not going into that direction (and I don't think we should). Or maybe we should, but then we remove the 'config' idea and build a big structure with everything for every modules.

https://irc.koha-community.org/koha/2023-05-09#i_2485635

"""
it could certainly be /config/erm
i.e. constrained to specific modules
and instead of exposing the sysprefs
list enabled modules
or components
in terms of your solution, it is just a matter of naming things more generically, and consistently
"""

Please be explicit, I don't understand.
I am exposing ERMModule and ERMProviders. Why should I make the code obscure and hide that behind an other config key? Having the syspref names in tt/vue to grep make things easy.

If it's really important for you, I am suggesting
/config/erm
{
    enabled => 1|0,
    providers => ['local', 'ebsco'],
}

For now.

Then we will add the permission list later (for bug 32968), and it could be something like:
{
    enabled => 1|0,
    providers => ['local', 'ebsco'],
    permissions => ['agreements.write', 'eholdings.write'], etc.
}

Would that work for everybody?

I think the user/permissions/permify approach is best left to bug 32986.  We're mixing requirements here.

As for configuration keys themselves.. I'm more torn now.. I can see both sides.. it's really nice to have consistency.. but should that consistency be permission names being consistent across DB/Controller/API/TT vs consistenct between different API endpoints.

I like Jonathans suggestion (without permissions added).. but at the same time I can see as we move to more and more vue in the client we may just want to look cross module for preferences and then the overlapping keys could get confusing?

How about

`/configurations` or `/settings` that returns an array of key:value pair objects...

[
  { 'ERMModule': true },
  { 'ERMProviders': [ 'A', 'B', 'C' ] }
]

and we could allow searching on key..

Or perhaps

[
  {
    id: 'ERMModule',
    value: true
  }
  {
    id: 'ERMProviders',
    value: [ 'A', 'B', 'C' ]
  }
]

What's going to be easiest to use from code whilst also being easy to filter in the API?

Clearly different preferences have different security concerns.. perhaps we'd need a new field in the perferences table to denote 'public', 'staff', 'internal' or something to allow things like API keys to require a higher privilege to see on the API?

Created attachment 151276 [details] [review]
Bug 33606: Add a erm/config route to retrieve the ERM config needed for the Vue app

This could be extended later in bug 32968 to pass the permission of the
logged in user.

Created attachment 151277 [details] [review]
Bug 33606: Mark the endpoint as experimental

Created attachment 151333 [details] [review]
Bug 33606: Add a erm/config route to retrieve the ERM config needed for the Vue app

This could be extended later in bug 32968 to pass the permission of the
logged in user.

Created attachment 151334 [details] [review]
Bug 33606: Mark the endpoint as experimental

Created attachment 151350 [details] [review]
Bug 33606: Add a erm/config route to retrieve the ERM config needed for the Vue app

This could be extended later in bug 32968 to pass the permission of the
logged in user.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Created attachment 151351 [details] [review]
Bug 33606: Mark the endpoint as experimental

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Created attachment 151352 [details] [review]
Bug 33606: Fix settings

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

This fixes the issue.
A non-superlibrarian user no longer requires parameters => 'manage_sysprefs' to properly access ERM, although the issues in bug 32971 remain.

I want to come up with a better long-term solution but I can't think of one that doesn't imply rearranging how we handle permissions at a core level.

Removed from 23.05.00 blocker list, too late.
Not a good idea to push such changes on release day...

-    description: This resource returns a list of options needed for the ERM Vue app
+    description: This resource returns a list of options needed for the ERM Vue app. EXPERIMENTAL - DO NOT RELY on this, it is subject to change!

Shouldnt we just extend this list and not change the two entries we just added?

Created attachment 152614 [details] [review]
Bug 33606: Add a erm/config route to retrieve the ERM config needed for the Vue app

This could be extended later in bug 32968 to pass the permission of the
logged in user.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 152615 [details] [review]
Bug 33606: Mark the endpoint as experimental

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 152616 [details] [review]
Bug 33606: Fix settings

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 152617 [details] [review]
Bug 33606: (QA follow-up) Cosmetic changes

Even the POD name wasnt changed after copying :)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Sorry... Please rebase

Created attachment 153001 [details] [review]
Bug 33606: Add a erm/config route to retrieve the ERM config needed for the Vue app

This could be extended later in bug 32968 to pass the permission of the
logged in user.

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 153002 [details] [review]
Bug 33606: Mark the endpoint as experimental

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 153003 [details] [review]
Bug 33606: Fix settings

Signed-off-by: Pedro Amorim <pedro.amorim@ptfs-europe.com>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Created attachment 153004 [details] [review]
Bug 33606: (QA follow-up) Cosmetic changes

Even the POD name wasnt changed after copying :)

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>

Pushed to master for 23.11.

Nice work everyone, thanks!

Requires bug 33169 awaiting news there before pushing to 23.05.x series.

Could this be backported to 22.11.xx?

Hi Laura, this will be backported to 22.11.x but not before it goes into 23.05. There's a bit of follow-up work needed for 33169 as well which is a dependency

*** Bug 34830 has been marked as a duplicate of this bug. ***

Frido, ping!

I propose to try this after this month imminent release

It looks like one bug i fixes is that only superlibrarians can use the ERM in 23.05 right now (see bug 34830) - Are you worried about the size?

I would need advice on backport of 33169 since it contains FA 6 parts

(In reply to Jonathan Druart from comment #1)
> This is blocker for 23.05

Love that comment, 2023-04-25...

(In reply to Fridolin Somers from comment #39)
> I would need advice on backport of 33169 since it contains FA 6 parts

I pushed a branch to my repo, with 33169+33606. It should be tested.
I've noticed some icons missing.

I won't have more time (actually the energy) to spent in this.

(In reply to Jonathan Druart from comment #41)
> (In reply to Fridolin Somers from comment #39)
> > I would need advice on backport of 33169 since it contains FA 6 parts
> 
> I pushed a branch to my repo, with 33169+33606. It should be tested.
> I've noticed some icons missing.
> 
> I won't have more time (actually the energy) to spent in this.

https://gitlab.com/joubu/Koha/-/commits/bug_33606-23.05

Can we have this backported now?

(In reply to Katrin Fischer from comment #43)
> Can we have this backported now?

Without this the ERM module is not usable in 23.05 - what is needed to resolve this?

(In reply to Katrin Fischer from comment #44)
> (In reply to Katrin Fischer from comment #43)
> > Can we have this backported now?
> 
> Without this the ERM module is not usable in 23.05 - what is needed to
> resolve this?

Test the branch I provided?

I am going to test the 23.05 branch.

Why is there no test plan on this bug report?

* Checked out the remote branch https://gitlab.com/joubu/Koha/-/commits/bug_33606-23.05
* Rebased on latest 23.05.x
* yarn build & restart_all
* ERMModule, ERMProviders EBSCO and Local activated
* As superlibrarian
  * Verified all pages are accessible and look as expected
  * Fixed missing FA icons in navigation (see follow-up)
* Create a user with limited permissions: only ERM, catatalogue
  On accessing the ERM module I see:
   Something went wrong: Error: Authorization failure. Missing required   
permission(s).
*  Added borrowers permission: same error
*  Added manage_sysprefs: same error

I don't know if I tested wrong or if this needs more work. I'll attach my follow up patch for the icons to bug 33169. Jonathan, maybe you could add it to the branch?

(In reply to Katrin Fischer from comment #47)
> Why is there no test plan on this bug report?
> 
> * Checked out the remote branch
> https://gitlab.com/joubu/Koha/-/commits/bug_33606-23.05
> * Rebased on latest 23.05.x
> * yarn build & restart_all
> * ERMModule, ERMProviders EBSCO and Local activated
> * As superlibrarian
>   * Verified all pages are accessible and look as expected
>   * Fixed missing FA icons in navigation (see follow-up)
> * Create a user with limited permissions: only ERM, catatalogue
>   On accessing the ERM module I see:
>    Something went wrong: Error: Authorization failure. Missing required   
> permission(s).
> *  Added borrowers permission: same error
> *  Added manage_sysprefs: same error
> 
> I don't know if I tested wrong or if this needs more work. I'll attach my
> follow up patch for the icons to bug 33169. Jonathan, maybe you could add it
> to the branch?

It's getting 403 on /vendors. Add permission acquisition: vendors_manage.
If you think it's a bug, open a separate bug report ;)

(In reply to Jonathan Druart from comment #48)
> (In reply to Katrin Fischer from comment #47)
> > Why is there no test plan on this bug report?
> > 
> > * Checked out the remote branch
> > https://gitlab.com/joubu/Koha/-/commits/bug_33606-23.05
> > * Rebased on latest 23.05.x
> > * yarn build & restart_all
> > * ERMModule, ERMProviders EBSCO and Local activated
> > * As superlibrarian
> >   * Verified all pages are accessible and look as expected
> >   * Fixed missing FA icons in navigation (see follow-up)
> > * Create a user with limited permissions: only ERM, catatalogue
> >   On accessing the ERM module I see:
> >    Something went wrong: Error: Authorization failure. Missing required   
> > permission(s).
> > *  Added borrowers permission: same error
> > *  Added manage_sysprefs: same error
> > 
> > I don't know if I tested wrong or if this needs more work. I'll attach my
> > follow up patch for the icons to bug 33169. Jonathan, maybe you could add it
> > to the branch?
> 
> It's getting 403 on /vendors. Add permission acquisition: vendors_manage.
> If you think it's a bug, open a separate bug report ;)

ie. certainly the same behaviour on master.

(In reply to Jonathan Druart from comment #49)
> (In reply to Jonathan Druart from comment #48)
> > (In reply to Katrin Fischer from comment #47)
> > > Why is there no test plan on this bug report?
> > > 
> > > * Checked out the remote branch
> > > https://gitlab.com/joubu/Koha/-/commits/bug_33606-23.05
> > > * Rebased on latest 23.05.x
> > > * yarn build & restart_all
> > > * ERMModule, ERMProviders EBSCO and Local activated
> > > * As superlibrarian
> > >   * Verified all pages are accessible and look as expected
> > >   * Fixed missing FA icons in navigation (see follow-up)
> > > * Create a user with limited permissions: only ERM, catatalogue
> > >   On accessing the ERM module I see:
> > >    Something went wrong: Error: Authorization failure. Missing required   
> > > permission(s).
> > > *  Added borrowers permission: same error
> > > *  Added manage_sysprefs: same error
> > > 
> > > I don't know if I tested wrong or if this needs more work. I'll attach my
> > > follow up patch for the icons to bug 33169. Jonathan, maybe you could add it
> > > to the branch?
> > 
> > It's getting 403 on /vendors. Add permission acquisition: vendors_manage.
> > If you think it's a bug, open a separate bug report ;)
> 
> ie. certainly the same behaviour on master.

It's bug 32971. But I think vendor_manage is much better than needing manage_sysprefs. Adding vendor_manage fixed my problems and now my 'permission restricted' user can use the ERM module.

How should we proceed, do you want to add my follow-up to your branch? Or should I try and attach the patch sets for 23.05 to both bugs?

I think I get it.

I've picked from branch https://gitlab.com/joubu/Koha/-/commits/bug_33606-23.05 :

b702d1c6e7 Bug 33606: (QA follow-up) Cosmetic changes
e89d5599e0 Bug 33606: Fix settings
d0c2829f10 Bug 33606: Mark the endpoint as experimental
5078f9f4df Bug 33606: Add a erm/config route to retrieve the ERM config needed for the Vue app

Display looks ok with superlibrarian and user with catalog+erm+vendor_manage

Pushed to 23.05.x for 23.05.05

Thanks again for everyone helping ;)

Nice work everyone!

Pushed to oldstable for 22.11.x