-
Notifications
You must be signed in to change notification settings - Fork 49
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
15601 Legal Name changes #575
15601 Legal Name changes #575
Conversation
- save operating name from business info - added operating name to state - added alternate names array to business inteface - added operating name store getter and setter - initialized operating name in state
@@ -361,7 +362,7 @@ export default class App extends Mixins(CommonMixin, DateMixin, FilingTemplateMi | |||
readonly window = window | |||
|
|||
/** The Update Current JS Date timer id. */ | |||
private updateCurrentJsDateId = 0 | |||
private updateCurrentJsDateId = null as any // NodeJS.Timeout |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -810,7 +811,7 @@ export default class App extends Mixins(CommonMixin, DateMixin, FilingTemplateMi | |||
// NB: will throw if API error | |||
let draftFiling = await LegalServices.fetchFirstTask(businessId) | |||
|
|||
this.setFilingType(draftFiling.header.name) | |||
this.setFilingType(draftFiling.header.name as FilingTypes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed warning because name is a string.
@@ -194,12 +194,12 @@ export default class AssociationDetails extends Mixins(CommonMixin, DateMixin) { | |||
|
|||
/** The entity name. */ | |||
get entityName (): string { | |||
return this.getBusinessLegalName || GetCorpNumberedDescription(this.getEntityType) | |||
return this.getBusinessLegalName || GetCorpNumberedDescription(this.getEntityType as any) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's a mismatch between CorpTypeCd in GetCorpNumberedDescription() and CorpTypeCd from getEntityType that I couldn't figure out quickly so I just typed it as any.
@@ -10,22 +9,18 @@ const store = useStore() | |||
|
|||
/** Returns legal name. */ | |||
function getLegalName (): string { | |||
const getFilingType: FilingTypes = store.getFilingType | |||
const getBusinessLegalName: string = store.getBusinessLegalName | |||
const getNameRequestApprovedName: string = store.getNameRequestApprovedName |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
getFilingType
was only used once so I moved it there.
getBusinessLegalName
was only needed in some cases so I moved it there.
getNameRequestApprovedName
was only needed in some cases so I moved it there.
^^ Although it looked nice to assign variables ahead of time, in all cases some variables weren't used, so I simplified this.
const legalApiUrl: string = (import.meta.env.VUE_APP_LEGAL_API_URL + import.meta.env.VUE_APP_LEGAL_API_VERSION_2 + '/') | ||
const legalApiUrl: string = ( | ||
import.meta.env.VUE_APP_LEGAL_API_URL + import.meta.env.VUE_APP_LEGAL_API_VERSION_2 + '/' | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Line was too long.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We probably need to fix lint at some point to make it not run the project if there was a lint error. I remember I created a ticket for that back when I did the Vite work but no idea when will we be able to get to that ticket.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think all we need to do is change lint warnings to errors. But this is a problem for another day. (Hope you agree!)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yes, for sure Sev! I also found the ticket: bcgov/entity#17320
A problem for another day 🙌
(import.meta.env.VUE_APP_REGISTRIES_SEARCH_API_URL + import.meta.env.VUE_APP_REGISTRIES_SEARCH_API_VERSION + '/') | ||
const registriesSearchApiUrl: string = ( | ||
import.meta.env.VUE_APP_REGISTRIES_SEARCH_API_URL + import.meta.env.VUE_APP_REGISTRIES_SEARCH_API_VERSION + '/' | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parentheses not used consistently.
VUE_APP_LEGAL_API_URL="https://legal-api-dev.apps.silver.devops.gov.bc.ca" | ||
# for Legal Name feature branch only: | ||
VUE_APP_LEGAL_API_URL="https://business-api-dy4loprnwa-nn.a.run.app" | ||
#VUE_APP_LEGAL_API_URL="https://legal-api-dev.apps.silver.devops.gov.bc.ca" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to be updated when merging to main branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@argush3 ^^
... Whatever we end up doing for the new Legal (Name) API URL ...
VUE_APP_LEGAL_API_URL="op://API/$APP_ENV/legal-api/LEGAL_API_URL" | ||
# for Legal Name feature branch only: | ||
VUE_APP_LEGAL_API_URL="op://API/$APP_ENV/legal-api/LEGAL_NAME_API_URL" | ||
#VUE_APP_LEGAL_API_URL="op://API/$APP_ENV/legal-api/LEGAL_API_URL" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to be updated when merging to main branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@argush3 ^^
... Whatever we end up doing for the new Legal (Name) API URL ...
- added temp API URL to vaults env file - fixed misc code issues - changed DocumentMixin hook from created() to beforeCreate() to avoid type conflict - updated getBusinessLegalName to return operating name for firms
- added AssociationDetails unit tests
|
||
describe('Breadcrumbs for firms', () => { | ||
it('computes breadcrumbs correctly for a SP registration', () => { | ||
const wrapper = shallowWrapperFactory( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to shallow-mount App.vue (meaning the Breadcrumb sub-component is stubbed, not fully rendered) and I check the breadcrumbs
computed instead.
- updated firebase config to allow connecting to Legal API temp URL
@@ -20,7 +20,7 @@ | |||
{ "key" : "X-XSS-Protection", "value" : "1; mode=block" }, | |||
{ | |||
"key": "Content-Security-Policy", | |||
"value": "default-src 'self'; frame-src 'self' *.gov.bc.ca *.hotjar.com *.googleapis.com https://*.nr-data.net https://*.newrelic.com https://*.cac1.pure.cloud; script-src 'self' 'unsafe-eval' 'unsafe-inline' *.gov.bc.ca *.hotjar.com *.googleapis.com https://*.nr-data.net https://*.newrelic.com https://*.cac1.pure.cloud; style-src 'self' 'unsafe-inline' *.cloudflare.com *.googleapis.com; font-src 'self' *.gov.bc.ca *.hotjar.com *.cloudflare.com *.googleapis.com *.gstatic.com *.jsdelivr.net; img-src 'self' data: *.hotjar.com https://*.cac1.pure.cloud; connect-src 'self' *.gov.bc.ca *.launchdarkly.com *.hotjar.com *.postescanada-canadapost.ca *.sentry.io *.apigee.net wss://*.hotjar.com *.hotjar.io https://*.nr-data.net https://shyrka-prod-cac1.s3.ca-central-1.amazonaws.com https://*.newrelic.com https://*.cac1.pure.cloud wss://*.cac1.pure.cloud; manifest-src 'self'; media-src 'self' https://*.cac1.pure.cloud; object-src 'self' https://*.cac1.pure.cloud; child-src 'self' https://*.cac1.pure.cloud;" | |||
"value": "default-src 'self'; frame-src 'self' *.gov.bc.ca *.hotjar.com *.googleapis.com https://*.nr-data.net https://*.newrelic.com https://*.cac1.pure.cloud; script-src 'self' 'unsafe-eval' 'unsafe-inline' *.gov.bc.ca *.hotjar.com *.googleapis.com https://*.nr-data.net https://*.newrelic.com https://*.cac1.pure.cloud; style-src 'self' 'unsafe-inline' *.cloudflare.com *.googleapis.com; font-src 'self' *.gov.bc.ca *.hotjar.com *.cloudflare.com *.googleapis.com *.gstatic.com *.jsdelivr.net; img-src 'self' data: *.hotjar.com https://*.cac1.pure.cloud; connect-src 'self' *.gov.bc.ca *.run.app *.launchdarkly.com *.hotjar.com *.postescanada-canadapost.ca *.sentry.io *.apigee.net wss://*.hotjar.com *.hotjar.io https://*.nr-data.net https://shyrka-prod-cac1.s3.ca-central-1.amazonaws.com https://*.newrelic.com https://*.cac1.pure.cloud wss://*.cac1.pure.cloud; manifest-src 'self'; media-src 'self' https://*.cac1.pure.cloud; object-src 'self' https://*.cac1.pure.cloud; child-src 'self' https://*.cac1.pure.cloud;" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will need to be updated when merging to main branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@argush3 ^^
(Need to remove connect-src *.run.app
.)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great Sev! 👍
- restored nginx config
/gcbrun |
Temporary Url for review: https://business-create-dev--pr-575-vg2a7q4z.web.app SB says: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked through all the changes. Looks good!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
* - app version = 5.6.0 * - updated Legal API URL (temporary) - save operating name from business info - added operating name to state - added alternate names array to business inteface - added operating name store getter and setter - initialized operating name in state * - added temp API URL to example env file - added temp API URL to vaults env file - fixed misc code issues - changed DocumentMixin hook from created() to beforeCreate() to avoid type conflict - updated getBusinessLegalName to return operating name for firms * - updated EntityInfo unit tests * - added ids to some AssociationDetails elements - added AssociationDetails unit tests * - updated App unit tests * - deleted obsolete nginx config - updated firebase config to allow connecting to Legal API temp URL * - commented out invalid comment in Dockerfil * - renamed jest-wrapper-factory -> vitest-wrapper-factory * - reverted Dockerfile - restored nginx config
* - app version = 5.6.0 * - updated Legal API URL (temporary) - save operating name from business info - added operating name to state - added alternate names array to business inteface - added operating name store getter and setter - initialized operating name in state * - added temp API URL to example env file - added temp API URL to vaults env file - fixed misc code issues - changed DocumentMixin hook from created() to beforeCreate() to avoid type conflict - updated getBusinessLegalName to return operating name for firms * - updated EntityInfo unit tests * - added ids to some AssociationDetails elements - added AssociationDetails unit tests * - updated App unit tests * - deleted obsolete nginx config - updated firebase config to allow connecting to Legal API temp URL * - commented out invalid comment in Dockerfil * - renamed jest-wrapper-factory -> vitest-wrapper-factory * - reverted Dockerfile - restored nginx config
Issue #: bcgov/entity#15601
Description of changes:
Commit 1:
Commit 2:
Commit 3:
Commits 4-5-6:
Commits 7-8-9-10:
deleted obsolete nginx configcommented out invalid comment in DockerfileBy submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the bcrs-entities-create-ui license (Apache 2.0).