bcgov / entity

ServiceBC Registry Team working on Legal Entities
Apache License 2.0
23 stars 57 forks source link

SBC Header: configuration is missing to correctly fetch PayBC status #6801

Closed severinbeauvais closed 2 years ago

severinbeauvais commented 3 years ago

Describe the bug in current situation A common component or service makes a network to fetch the PayBC status. In some situations, this fails in the Entity UIs (eg, Filings UI) because of an incorrect URL.

It's possible some configuration is only set (ie, saved in session storage) if the user first went through the BCROS home pages (ie, auth-web). Edit UI has a workaround to save AUTH_API_CONFIG object... maybe this should be expanded to contain the PayBC URL (in all UIs). Or maybe a better mechanism overall needs to be implemented so no workaround is needed (in any of several UIs).

image.png

The correct URL is https://status-api-dev.apps.silver.devops.gov.bc.ca/api/v1/status/PAYBC.

Ideally, this needs to be resolved before alterations go live on April 20th -Linda

severinbeauvais commented 3 years ago

I found the method that gets the Status API URL: https://github.com/bcgov/sbc-common-components/blob/c212629a2a73901e7e64a069aba6ec148b241213/vue/sbc-common-components/src/util/config-helper.ts#L24

This code is called by status.services.ts::getServiceStatus(). Apparently this fails if the session storage key has not been configured.

The Entity UIs could add that key to their config. Is this the right solution @sumesh-aot @saravanpa-aot ?

Milan-Freshworks commented 3 years ago

@saravanpa-aot Could you please clarify and discuss together.

jdyck-fw commented 3 years ago

@Milan-Freshworks & @saravanpa-aot - We are looking for an update on how / when this will be addressed.

saravanpa-aot commented 3 years ago

As @severinbeauvais said , I think Entity UI can add the key [url] to the configs. That should resolve the issue.

severinbeauvais commented 3 years ago

@saravanpa-aot What about "a better mechanism overall needs to be implemented so no workaround is needed" ?

saravanpa-aot commented 3 years ago

@saravanpa-aot What about "a better mechanism overall needs to be implemented so no workaround is needed" ?

Hm.definitely a good idea and something to ponder about...I cant think of anything right now... do you have any ideas? If we are sure if all of them shares same storage and all navigation starts from auth-web , there could be a better mechanism, auth-web can set in session and all app will work...But otherwise , I don't know if we can do something...

severinbeauvais commented 3 years ago

I think we both understand the problems:

  1. every app will need to provide a config key for the header's status check (lots of duplicate/common config)
  2. user's don't always go through auth-web to get to app; they may have bookmarked a URL and go straight there; in this case, the config keys set by auth-web don't get set

Since we are talking about config common to all apps (in a particular environment), would it make sense to have a common config file that SbcHeader (or some other sbc-common-components module) loads? And then the apps can load their own config files with whatever they individually need?

Examples of some common config examples:

If the above makes sense then I would prefer to pursue that instead of continuing to add work-around keys (keys that the app doesn't use but must provide for the SbcHeader).

thorwolpert commented 3 years ago

@sumesh-aot started down this path in 1Password. It's on @pwei1018 list after the OCP4 migration is off the plate.

there's lots of duplicate, but common configs and namings that'll get paired down. maybe a short paper to discuss approaches so things like this aren't missed would help.

severinbeauvais commented 3 years ago

@jdyck-fw Please bring this ticket up at the appropriate meeting so someone starts working on it. Thanks :)

jdyck-fw commented 3 years ago

BA's please discuss this at the guild meeting. Is this work that Patrick would be doing? If so, we need to remove the Entities milestone.

severinbeauvais commented 3 years ago

As discussed in Entities standup today, this seems like it is only a couple of weeks away... If it will be longer than this then we might consider including the header's needed config in the Entity apps anyway as an interim solution.

lmcclung commented 3 years ago

@pwei1018 Can you get this done this sprint? If not, what would the ETA?

pwei1018 commented 3 years ago

I added VUE_APP_STATUS_ROOT_API in the configmap (1password) for all three business uis. The issue should be fix now. The duplicate keys issue is on my task list. I might start working on it in next or two sprints.

severinbeauvais commented 3 years ago

It's likely the UIs will need to read in this new key and store it in session storage.

lmcclung commented 3 years ago

@pwei1018 thank you!

@severinbeauvais does that mean this is just a verify then? Or does Entities still need to do something before alteration release?

severinbeauvais commented 3 years ago

My concern about duplicate configuration had been resolved by Patrick's recent changes. (Thanks!)

But, yes, some of our UIs need to be updated to load and store the necessary keys.

I don't think this is required for alterations specifically. It's config needed for the SBC Header to show things like PayBC status, or possibly banners.

severinbeauvais commented 3 years ago

While we're here, are there any other keys that SBC Header, Keycloak, etc needs that our UIs don't already load?

lmcclung commented 3 years ago

@pwei1018 would you be able to answer Severin's question above?

janisrogers commented 3 years ago

@pwei1018 Can you look at this?

pwei1018 commented 3 years ago

I need to dig into code. Or, forward this question to relationship team.

lmcclung commented 2 years ago

@mstanton1 would you be able to get clarification from your team to Sev's question:

While we're here, are there any other keys that SBC Header, Keycloak, etc needs that our UIs don't already load?

sumesh-aot commented 2 years ago

While we're here, are there any other keys that SBC Header, Keycloak, etc needs that our UIs don't already load?

@severinbeauvais can you please provide your current config? we can compare and let you know any missing keys.

severinbeauvais commented 2 years ago

This is the kind of "workaround" that some of our UIs use to set keys that we don't need, but that auth needs: https://github.com/bcgov/business-create-ui/blob/7e87e969cdc120acf449b071a16c54cd66645238/src/utils/config-helper.ts#L46

The keys that we use can be seen in OCP config maps, eg: https://console.apps.silver.devops.gov.bc.ca/k8s/ns/3b2420-dev/configmaps/business-create-dev-ui-configuration

sumesh-aot commented 2 years ago

@severinbeauvais Patrick has plans to create a common bucket for UI config in 1password, which would take care of these issues. So for now, I think "workaround" solutions would be good enough.

severinbeauvais commented 2 years ago

Sure. so

are there any other keys that SBC Header, Keycloak, etc needs that our UIs don't already load?

sumesh-aot commented 2 years ago

VUE_APP_STATUS_ROOT_API is the only key missing I can find. @saravanpa-aot can you take a look as well? I see that the key is in config, but cannot find it being set here https://github.com/bcgov/business-create-ui/blob/7e87e969cdc120acf449b071a16c54cd66645238/src/utils/config-helper.ts#L46. so your config seems to be correct, but not code.

severinbeauvais commented 2 years ago

OK, I will create a "workaround ticket" so that Entities Team can add code to load the VUE_APP_STATUS_ROOT_API value that the SbcHeader needs. Please let us know ASAP if there are other such keys that need additional session code.

However, a longer term solution is needed so that this workaround is not needed for all UIs (including, eg, PPR). Sumesh/ Saravan/ Patrick, can you use THIS ticket to come up with a solution that does not require each UI to load the necessary Sbc keys?

cc: @thorwolpert @kialj876

severinbeauvais commented 2 years ago

The Entity UIs workaround ticket is #7694.

severinbeauvais commented 2 years ago

@pwei1018 Will you also update Namerequest UI?

severinbeauvais commented 2 years ago

@jdyck-fw Can we add this ticket to all our next releases (Edit UI, Filings UI, Create UI and NR UI)?

pwei1018 commented 2 years ago

To avoid duplicate keys configuration, I modified the following items of sbc-common-components:

  1. change the key new to match new configuration of 1password.
  2. change to read the session keys directly instead of read from auth-api-config session key.

https://github.com/bcgov/sbc-common-components/pull/228

severinbeauvais commented 2 years ago

Test Notes

This ticket updates the sbc-common-components dependency from v2.4.18 to v2.4.19, thus these changes need to be verified as well. These changes are ^^^ (whatever Patrick wrote above) ^^^.

pwei1018 commented 2 years ago

This ticket is ready for QA.