21033 Use different entity types for continuation in NRs #763
Conversation
- added missing entity type (CS) to enum - added cont in entity type data - added cont in types to list of BC types - updated MVE request action mapping for cont in - updated getters to return alternate entity type data for cont in vs BC
pnpm-lock.yaml
Outdated
There was a problem hiding this comment.
Note that this repo now uses pnpm (instead of npm) so you get a different lock file. Most of the shell commands are the same.
There was a problem hiding this comment.
This is good to know. Thanks.
| @@ -11,6 +11,7 @@ export enum EntityTypes { | |||
| CCC = CorpTypeCd.CCC_CONTINUE_IN, | |||
| CP = CorpTypeCd.COOP, | |||
| CR = CorpTypeCd.CORPORATION, | |||
| CS = CorpTypeCd.CONT_IN_SOCIETY, | |||
There was a problem hiding this comment.
... Even though we have that other feature flag to redirect users straight to Society Online if they choose a society-anything NR.
| @@ -281,6 +281,177 @@ export const EntityTypesBcData: EntityI[] = [ | |||
| } | |||
| ] | |||
|
|
|||
| /** List of entity types for Continuation In NRs. */ | |||
There was a problem hiding this comment.
This is almost the same as the EntityTypesBcData, above, except with only the 6 cont in entity types. The category and blurbs are the same.
(Yes, cont in coop is still CP. This is an ongoing discussion and may be fixed in a separate ticket.)
| EntityTypes.PAR | ||
| EntityTypes.PAR, | ||
| EntityTypes.SO, | ||
| EntityTypes.UL | ||
| ] |
There was a problem hiding this comment.
I added 5 cont in types (since there is no new cont in type for CP), and sorted this list.
-> C, CBEN, CCC, CS and CUL
| obj = EntityTypesContInData.find(ent => ent.value === entity) | ||
| } else { | ||
| obj = EntityTypesBcData.find(ent => ent.value === entity) | ||
| } |
There was a problem hiding this comment.
@JazzarKarim , @jamespaologarcia :
I don't love this implementation. An alternative is to keep using the EntityTypesBcData list and the "non-cont in" entity types (eg, ULC instead of CUL), but then swap the entity types later in the processing. What do you think? Which makes the most sense to you?
Without seriously refactoring a lot of stuff, we're kind of stuck with needing to toggle between "BC" and "cont in" types somewhere.
There was a problem hiding this comment.
I'm OK with this as is. I'm thinking that since this is a very special case which I don't think will re-occur, we can keep as is here.
Now, if we stumble upon something like this in the future, we might want to reconsider but we don't want to have lots of conditions here. Thoughts? I'm OK with the alternative as well.
There was a problem hiding this comment.
I think the readability makes up for the potential code duplication because it makes it clear right away why you're changing entity types.
There was a problem hiding this comment.
Great point, James. Thanks.
Remember this when you following this architecture for your next project 😉
| } catch (err) { | ||
| console.error('entityTypesBC() =', err) // eslint-disable-line no-console | ||
| return EntityTypesBcData | ||
| return [] |
There was a problem hiding this comment.
The old code masked any errors here. The new code should be safe and make any errors obvious.
There was a problem hiding this comment.
Note that any error here should be a code error that we should catch during testing, so displaying "nothing" is a good option.
If this was a runtime error (eg, not static data) then it should be handled differently as a regular user might see it.
| if (x.value === EntityTypes.CUL) { | ||
| x.shortlist = true | ||
| x.rank = 2 | ||
| } |
There was a problem hiding this comment.
Limited Company (CR or C) is automatically added as the first element in the short list.
| // Shortlist order: Limited Company, Cooperative Association | ||
| if (x.value === EntityTypes.CP) { | ||
| x.shortlist = true | ||
| x.rank = 2 | ||
| } else if ([EntityTypes.FR, EntityTypes.GP, EntityTypes.UL].includes(x.value)) { | ||
| x.shortlist = null | ||
| x.rank = null |
There was a problem hiding this comment.
This else-if was not needed.
|
@eve-git , is that code-coverage error a remnant of the pnpm change you recently did? |
- added cont in codes to numbered entity types list
|
/gcbrun |
|
Last commit LGTM Sev 👍 |
|
Temporary Url for review: https://namerequest-dev--pr-763-m2wjrmib.web.app |
|
Folks, sorry, but this solution got messy -- it was sending the continuation in codes (eg, "C") instead of the regular codes (eg, "CR") to NameX API, which was crashing (HTTP 500). So I went with the alternate solution, which turns out to be much simpler. It will be in another PR. Update: The new PR is #764. |
Issue #: bcgov/entity#21033
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).