-
Notifications
You must be signed in to change notification settings - Fork 50
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
19897 Fixed foreign jurisdiction region #661
Conversation
- save region value instead of text (x2) - fixed unit test
@@ -316,8 +316,8 @@ export default class AmalgamatingBusinesses extends Mixins(AmalgamationMixin, Co | |||
snackbar = false | |||
snackbarText = '' | |||
errorDialog = false | |||
errorDialogText = undefined as string | |||
errorDialogTitle = undefined as string |
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 was actually causing some template errors. I originally had it this way so the default props would be used if we didn't specify them, but we always specify them anyway so '' is a better initial value.
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.
(From a high level, default prop values are normally used when you DON'T specify the prop at all. If we do specify the prop, but want to use its default value, then we'll have to set it to undefined. Neither of these applies here.)
const tingBusiness = { | ||
type: AmlTypes.FOREIGN, | ||
role: AmlRoles.AMALGAMATING, | ||
foreignJurisdiction: { | ||
region: region.text, | ||
region: JurisdictionLocation.BC, |
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.
@@ -549,7 +548,7 @@ export default class AmalgamatingBusinesses extends Mixins(AmalgamationMixin, Co | |||
type: AmlTypes.FOREIGN, | |||
role: AmlRoles.AMALGAMATING, | |||
foreignJurisdiction: { | |||
region: this.isCan ? this.jurisdiction.text : '', | |||
region: this.isCan ? this.jurisdiction.value : '', // no region outside Canada |
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.
@@ -638,7 +638,7 @@ describe('Amalgamating Businesses - add amalgamating foreign business', () => { | |||
// simulate form data | |||
await wrapper.setData({ | |||
isCan: true, | |||
jurisdiction: { text: 'BC', value: 'CA' }, | |||
jurisdiction: { text: 'Alberta', value: 'AB' }, |
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 is what the jurisdiction
object actually looks like when it's selected.
@@ -654,7 +654,7 @@ describe('Amalgamating Businesses - add amalgamating foreign business', () => { | |||
const business = store.getAmalgamatingBusinesses[0] as any | |||
expect(business.type).toBe(AmlTypes.FOREIGN) | |||
expect(business.role).toBe(AmlRoles.AMALGAMATING) | |||
expect(business.foreignJurisdiction).toEqual({ country: 'CA', region: 'BC' }) | |||
expect(business.foreignJurisdiction).toEqual({ country: 'CA', region: 'AB' }) |
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 changed this from BC to AB since BC isn't "foreign" :)
/gcbrun |
Temporary Url for review: https://business-create-dev--pr-661-wghir2e4.web.app SB says, try this: https://business-create-dev--pr-661-wghir2e4.web.app/amalg-short-information?id=TFEnzE0Tb0&accountid=3040 |
- save region value instead of text (x2) - fixed unit test Co-authored-by: Severin Beauvais <severin.beauvais@gov.bc.ca>
Issue #: bcgov/entity#19897
Description of changes:
By 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).