Bug 37930

Created attachment 171556 [details] [review]
Bug 37930: Add a method to initialise the AV store

This patch adds a method to initialise the authorised values in a vue app. It will automatically load authorised values into the app and store them in the authorised values store based on the entries in a matrix file. The intent behind this is to make adding authorised values into a Vue app much simpler and more DRY, especially when creating a new vue app. You will just need to add your app into the avMatrix.js file and add a call to the initialisation method in your app.

Created attachment 171557 [details] [review]
Bug 37930: Update the ERM and Preservation apps

Created attachment 171558 [details] [review]
Bug 37930: Update cypress tests

Test plan:
1) Run the cypress tests for ERM and Preservation
2) All tests should still pass

This is an idea for discussion - the main intent is to make the handling of authorised values DRY across all Vue apps and to make it much easier to inject them into both existing and new apps

The avMatrix file controls which authorised values are injected into the app so to use an additional value category in your app you would just need to add a line into that file under the relevant module

For new apps, you would just add an entry into the matrix file and call the initialisation method somewhere in your home component and the front loading of all the authorised values would be handled for you

I think we should keep one file per app. Otherwise we will end up with a big ugly and hard-to-maintain authorisedValues/avMatrix.js.

Why not simply have the av list in the store of the app (eg. stores/erm.js)?

That would mean we could remove useAVStore and use the mainStore instead. We would retrieve the AVs from the config:
  this.config.avs.get("my_av")
  this.config.avs.get_lib_from_av("my_av", value)

Also for discussion :)

Created attachment 171628 [details] [review]
Bug 37930: [DO NOT PUSH] Alternate concept

Alternative patch taking into account feedback in comment 5 on the bug. This patch removes the authorised values store and migrates the methods to the main store. Authorised values are now added into the module specific store and replaced on module load with the actual values

To test I've added a console log of the values in the store to the AgreementsList component so apply all patches on the bug and then navigate to the agreements table in ERM. The module will throw errors as I haven't been through and changed all the instances where we would need to swap the AV store for the ERM store (there are a lot of instances and this is just a proof of concept for discussion for now). Check that the logged values are as expected. If you have the Vue devtools you'll be able to see the store values and check them directly in there

(In reply to Jonathan Druart from comment #5)
> I think we should keep one file per app. Otherwise we will end up with a big
> ugly and hard-to-maintain authorisedValues/avMatrix.js.
> 
> Why not simply have the av list in the store of the app (eg. stores/erm.js)?
> 
> That would mean we could remove useAVStore and use the mainStore instead. We
> would retrieve the AVs from the config:
>   this.config.avs.get("my_av")
>   this.config.avs.get_lib_from_av("my_av", value)
> 
> Also for discussion :)

I like the idea of using the module specific store so the new patch uses that methodology. I've opted not to include them in the config object as we overwrite that when we fetch the config and that wipes the existing authorised values we would have fetched

Have a look at the patch above and see what you think - I've not changed all of the places we would need to remove the AV store yet as this may need changes and that would take time to keep repeating.

That seems better to me.

A couple of notes:
* "loaded" is not called when the pref is off (and so "loading..." is displayed on the screen)

* it would be nice to still have the av related functions (get_lib_from_av, map_av_dt_filter, loadAuthorisedValues) in a dedicated file. They were isolated and it was good. We should try to keep that in mind, to keep stores/main.js short and clean. But I would not consider it blocker.

Created attachment 172863 [details] [review]
Bug 37930: Change how we load authorised values in Vue

This patch adds a method to initialise the authorised values in a vue app. It will automatically load authorised values into the app and store them in the store for your app. The intent behind this is to make adding authorised values into a Vue app much simpler and more DRY, especially when creating a new vue app. You will just need to add your desired authorised values into the store for your app and they will be loaded automatically

Created attachment 172864 [details] [review]
Bug 37930: Restore AV store to hold methods

Created attachment 172865 [details] [review]
Bug 37930: Modify AgreementsList and FormAdd to support new AV concept

Test plan
1) Navigate to ERM module
2) Click into agreements, page should load as normal
3) Click New agreement, form should load as expected
4) Fill in the required fields - note that the Status field is correctly populated with authorised values
5) Save new agreeement
6) Note that the status column in the table is correctly populated

I've restored the AVstore to hold the methods - this means we now need to pass the authorised values into the methods themselves rather than accessing them using "this" in the store

I've made the changes to AgreementsList and FormAdd to show the new concept working, if we're happy with this I'll update the rest of the instances and the cypress tests :)

I've also fixed the loading issue

Created attachment 172886 [details] [review]
Bug 37930: Change how we load authorised values in Vue

This patch adds a method to initialise the authorised values in a vue app. It will automatically load authorised values into the app and store them in the store for your app. The intent behind this is to make adding authorised values into a Vue app much simpler and more DRY, especially when creating a new vue app. You will just need to add your desired authorised values into the store for your app and they will be loaded automatically

Created attachment 172887 [details] [review]
Bug 37930: Modify AgreementsList and FormAdd to support new AV concept

Test plan
1) Navigate to ERM module
2) Click into agreements, page should load as normal
3) Click New agreement, form should load as expected
4) Fill in the required fields - note that the Status field is correctly populated with authorised values
5) Save new agreeement
6) Note that the status column in the table is correctly populated

Having the methods outside of mainstore involved a lot more passing data back and forth so I've moved them back into main store and the AVs are loaded into there. This means it is much easier to access them in the components

Created attachment 172888 [details] [review]
Bug 37930: Modify AgreementsList and FormAdd to support new AV concept

Test plan
1) Navigate to ERM module
2) Click into agreements, page should load as normal
3) Click New agreement, form should load as expected
4) Fill in the required fields - note that the Status field is correctly populated with authorised values
5) Save new agreeement
6) Note that the status column in the table is correctly populated

Created attachment 172979 [details] [review]
Bug 37930: Modify AgreementsList and FormAdd to support new AV concept

Test plan
1) Navigate to ERM module
2) Click into agreements, page should load as normal
3) Click New agreement, form should load as expected
4) Fill in the required fields - note that the Status field is correctly populated with authorised values
5) Save new agreeement
6) Note that the status column in the table is correctly populated

Created attachment 173069 [details] [review]
Bug 37930: Modify AgreementsList and FormAdd to support new AV concept

Test plan
1) Navigate to ERM module
2) Click into agreements, page should load as normal
3) Click New agreement, form should load as expected
4) Fill in the required fields - note that the Status field is correctly populated with authorised values
5) Save new agreeement
6) Note that the status column in the table is correctly populated

Signed-off-by: esther <esther@bywatersolutions.com>

1. The code in beforeCreate needs to be adjusted. There could be a race: we should not have this.loaded() called twice.

2. functions are defined in 2 different places (store/main and store/authorised-values)

3. Other apps need to be adjusted.

4. 
(In reply to Matt Blenkinsop from comment #15)
> Having the methods outside of mainstore involved a lot more passing data
> back and forth so I've moved them back into main store and the AVs are
> loaded into there. This means it is much easier to access them in the
> components

Could you push a branch with this version and explain exactly what was the problematic part? Moving av code to main is not ideal IMO.

We are still in the discussion step here it seems, not ready for inclusion.