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

18201 Fix error after pressing Incorporate now #749

Merged
merged 4 commits into from
Dec 5, 2023
Merged

Conversation

eve-git
Copy link
Collaborator

@eve-git eve-git commented Dec 1, 2023

Issue #: bcgov/entity#18201

Description of changes:
An account id is required to go further incorp. If user back to NR page without logged in, the error occurred since no account id retrieved. If account id === 0, just simply go back to NR with out error.

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).

package-lock.json Outdated Show resolved Hide resolved
@severinbeauvais
Copy link
Collaborator

Eve, please assign yourself to this PR (so that the check can run), and then enter "gcbrun" to create the preview site where reviewers (and you) can test it. Thanks.

@eve-git eve-git self-assigned this Dec 1, 2023
@eve-git
Copy link
Collaborator Author

eve-git commented Dec 1, 2023

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-749-1sj098ao.web.app

@eve-git
Copy link
Collaborator Author

eve-git commented Dec 1, 2023

@JazzarKarim Do you think adding a condition as account.id != 0 before invoking this.incorporateNow is good @ https://github.com/bcgov/namerequest/blob/main/src/App.vue#L272

@severinbeauvais
Copy link
Collaborator

@JazzarKarim Do you think adding a condition as account.id != 0 before invoking this.incorporateNow is good @ https://github.com/bcgov/namerequest/blob/18201/src/App.vue#L272

Why not use the isAuthenticated getter? It's better for code to NOT know about details (such as account object) and instead to call helpers/getters for that -- it encapsulates the complexity into one place only.

@eve-git
Copy link
Collaborator Author

eve-git commented Dec 4, 2023

/gcbrun

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.

LGTM after last commit! 👍

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-749-1sj098ao.web.app

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.

@eve-git eve-git merged commit 43a847d into bcgov:main Dec 5, 2023
4 of 5 checks passed
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.

4 participants