-
Notifications
You must be signed in to change notification settings - Fork 45
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
18235 Fixed xpro mappings #732
Conversation
XLL = CorpTypeCd.XPRO_LIMITED_LIABILITY_PARTNERSHIP, | ||
XLP = CorpTypeCd.XPRO_LIMITED_PARTNERSHIP, | ||
XL = CorpTypeCd.XPRO_LL_PARTNR, |
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 sorted this.
EntityTypes.XLL, // maps to Corp Type "XL" | ||
EntityTypes.XLP, // maps to Corp Type "XP" | ||
EntityTypes.XSO, // maps to Corp Type "XS" | ||
EntityTypes.XUL // maps to Corp Type "A" |
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.
The root case of this bug is that these lists use Namerequest-specific codes, but we check the legal type (corp type) from the business search result, which is LEAR codes.
So we need to keep in mind this mapping between the codes 😒
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 really really wish we didn't have this mismatch between EntityTypes
and CorpTypeCd
😞 It's the source for a lot of confusion.
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 know. This will keep biting us.
I just created this ticket:
https://app.zenhub.com/workspaces/entities-team-space-6143567664fb320019b81f39/issues/gh/bcgov/entity/18241
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 agree as well, would be really nice to have the codes consistent.
EntityTypes.XL, | ||
EntityTypes.XLL, | ||
EntityTypes.XLP, | ||
EntityTypes.XP |
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.
The bug ticket referred to name "722380 ALBERTA LTD.", which has corp type "A", so the list above needs to include "XUL". I have tested the new code and it's fine for this example.
Testing should also be done for:
- restorations for businesses of type "A", "LLC" and "XS"
- name changes for businesses of type "LLC", "XL", "XP" and "XS"
^^ I've documented the above in the ticket.
- fixed some xpro mappings - deleted duplicate code lock - added fallbacks in case FF isn't available
89b765b
to
1ef76f4
Compare
- added default FF values
/gcbrun |
this.isNumberedCompany && | ||
!this.isSupportedContinuationIn(this.getEntityTypeCd) | ||
) return true | ||
|
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.
Duplicate of lines 556-561 above. (Probably a merge issue some time ago.)
Or we really, really wanted to make sure we return True in this scenario.
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.
Hahahahahaha, might as well add a 3rd one. To be extra, extra, EXTRA sure 🤣
Temporary Url for review: https://namerequest-dev--pr-732-ghqiul2n.web.app |
isSupportedIncorporationRegistration (type: EntityTypes): boolean { | ||
const supportedEntites = GetFeatureFlag('supported-incorporation-registration-entities') | ||
return supportedEntites.includes(type) | ||
} |
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.
Moved down so it's beside the other FF getters.
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!
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
Issue #: bcgov/entity#18235
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 namerequest license (Apache 2.0).