Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

17206 implement society NR flows #651

Merged
merged 10 commits into from
Aug 17, 2023

Conversation

eve-git
Copy link
Collaborator

@eve-git eve-git commented Aug 14, 2023

Issue #: /bcgov/entity#17206

Description of changes:
implement society NR flows.

new environment variable: VUE_APP_SOCIETIES_ONLINE_HOME_URL="https://dev.bcregistry.ca/societies/"
new feature flag: enable-society

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the namerequest license (Apache 2.0).

],
REH: [
EntityType.CR,
EntityType.CP,
EntityType.CC,
EntityType.UL,
EntityType.FI,
EntityType.BC
EntityType.BC,
EntityType.SO
],
CHG: entityTypesBC.filter(ent => ent !== EntityType.PAR && ent !== EntityType.PA),
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any change needed for CHG? (I see PAR in there, which is a designation....?

Why is SO already part of MVE list?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry I am not familiar with this part. It is old code.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you test around this code to see what it might do and whether it works correctly? Ask Mihai for help if needed.

Copy link
Collaborator

@severinbeauvais severinbeauvais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good and clean. Nice! Please see my small comments.

Copy link
Collaborator

@severinbeauvais severinbeauvais left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but please get another review or 2.

@@ -103,6 +103,11 @@ export class CommonMixin extends Vue {
return supportedEntites.includes(nr?.entity_type_cd)
}

/** in case Societies NR needs to be released AFTER the way of navigating changes (feature branch) */
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cool!

Copy link
Collaborator

@JazzarKarim JazzarKarim left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great Eve! 👍

@JazzarKarim
Copy link
Collaborator

/gcbrun

@pwei1018
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-651-dk4f535f.web.app

@eve-git
Copy link
Collaborator Author

eve-git commented Aug 17, 2023

@severinbeauvais I've unit tested. @pwei1018 provided a temporary url for review. Does it mean the code merge will be applied after it has been tested?

@@ -10,6 +10,7 @@ VUE_APP_DASHBOARD_URL="op://web-url/$APP_ENV/business/DASHBOARD_URL"
VUE_APP_ENTITY_SELECTOR_URL="op://web-url/$APP_ENV/entity-selector/ENTITY_SELECTOR_URL"
VUE_APP_PAYMENT_PORTAL_URL="op://web-url/$APP_ENV/pay/PAYMENT_PORTAL_URL"
VUE_APP_SITEMINDER_LOGOUT_URL="op://web-url/$APP_ENV/siteminder/SITEMINDER_LOGOUT_URL"
VUE_APP_SOCIETIES_ONLINE_HOME_URL="op://web-url/$APP_ENV/bcregistry/SOCIETIES_URL/"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You need to remove the '/' at the end of string.

@JazzarKarim
Copy link
Collaborator

JazzarKarim commented Aug 17, 2023

@severinbeauvais I've unit tested. @pwei1018 provided a temporary url for review. Does it mean the code merge will be applied after it has been tested?

Eve, the temporary URL is automatically generated after anyone commenting /gcbru (I didn't put an n in the end because it'll run the cloud build and generate another link). The reason we do that so that other developers can check/test the work and in this case, it's a feature branch. The link will be used in UX Assurance and in QA since the changes won't be in DEV after merging.

@eve-git
Copy link
Collaborator Author

eve-git commented Aug 17, 2023

@JazzarKarim please commit /gcbrun again because I have a new commit. Thanks.
So, can I merge the code?

@pwei1018
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-651-dk4f535f.web.app

@eve-git eve-git merged commit d6508ce into bcgov:feature-way-of-navigating Aug 17, 2023
5 checks passed
@JazzarKarim
Copy link
Collaborator

@JazzarKarim please commit /xxxxx again because I have a new commit. Thanks. So, can I merge the code?

Sorry Eve, I just saw this. Sure thing! Btw, you can also run the command yourself like just what happened 👍

Copy link
Collaborator

@ketaki-deodhar ketaki-deodhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 👍

@severinbeauvais
Copy link
Collaborator

@severinbeauvais I've unit tested. ... Does it mean the code merge will be applied after it has been tested?

Karim may have answered this... The PR temporary URL allows preliminary testing before the PR is merged. If you're asking about the feature branch -- we will need a PR to merge the feature branch into main, once everything is complete.

Ask again if you still have questions.

JazzarKarim pushed a commit to JazzarKarim/namerequest that referenced this pull request Aug 24, 2023
* implement society NR flows

* implement society

* update version

* according to code review

* according to code review

* according to code review

* FF enable-society value is disabled

* according to code review

* remove '/' from url
JazzarKarim pushed a commit to JazzarKarim/namerequest that referenced this pull request Aug 24, 2023
* implement society NR flows

* implement society

* update version

* according to code review

* according to code review

* according to code review

* FF enable-society value is disabled

* according to code review

* remove '/' from url
severinbeauvais pushed a commit to severinbeauvais/namerequest that referenced this pull request Aug 29, 2023
* implement society NR flows

* implement society

* update version

* according to code review

* according to code review

* according to code review

* FF enable-society value is disabled

* according to code review

* remove '/' from url
JazzarKarim pushed a commit to JazzarKarim/namerequest that referenced this pull request Sep 8, 2023
* implement society NR flows

* implement society

* update version

* according to code review

* according to code review

* according to code review

* FF enable-society value is disabled

* according to code review

* remove '/' from url
JazzarKarim pushed a commit that referenced this pull request Sep 11, 2023
* implement society NR flows

* implement society

* update version

* according to code review

* according to code review

* according to code review

* FF enable-society value is disabled

* according to code review

* remove '/' from url
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants