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.

Sign up for GitHub

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

Jump to bottom

18202 show both named and numbered option for LTD company #735

Merged
merged 13 commits into from
Oct 26, 2023
Merged

18202 show both named and numbered option for LTD company #735

merged 13 commits into from
Oct 26, 2023

Conversation

tshyun24
Copy link
Contributor

Issue #: /bcgov/entity#18202

Description of changes: Using isNumberedCompany to determine whether we have the COLIN option, instead of using isSupportedRestoration previously

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

@chenhongjing
Copy link
Collaborator

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-735-526soleu.web.app

@severinbeauvais severinbeauvais changed the title show both named and numbered option for LTD company 18202 show both named and numbered option for LTD company Oct 23, 2023
@severinbeauvais
Copy link
Collaborator

severinbeauvais commented Oct 23, 2023 •
edited
Loading

Please rebase and update the package files. done

Please provide some sample businesses that:

  • are and are not supported by FF
  • can be only named and can be named/numbered

@tshyun24 tshyun24 force-pushed the 18202-missing-numbered-option branch from 0a99f37 to 71e797f Compare October 23, 2023 22:53
@tshyun24
Copy link
Contributor Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-735-526soleu.web.app

@tshyun24
Copy link
Contributor Author

tshyun24 commented Oct 24, 2023 •
edited
Loading

Some sample businesses
image

  • Supported by FF : BC0013141 (FF value: UL), BC0871194 (FF value: BC)
  • Not supported by FF: CP1002580 (FF value: CP)
  • Can only be named: A0076494 (Xpro business)
  • Can be named/numbered: BC0024185

@JazzarKarim
Copy link
Collaborator

  • Supported by FF : BC0013141 (FF value: UL), BC0871194 (FF value: BC)
  • Not supported by FF: CP1002580 (FF value: CP)
  • Can only be named: A0076494 (Xpro business)
  • Can be named/numbered: BC0024185

Shaoyun, for coops, we don't want the numbered and named radio buttons.
In DEV:
coop in dev
Unless something has changed, this is the expected behavior for coops. They're like a special case. I was having a look in your temp URL, I can see that we got the radio button options.

Also, for "A0076494", I got stuck:
A business shaoyun

Please have a look in DEV and see the expected result. There's the "Business's full legal name in home jurisdiction" component missing.

@tshyun24
Copy link
Contributor Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-735-526soleu.web.app

@tshyun24
Copy link
Contributor Author

@JazzarKarim @severinbeauvais
push a new comment, hopefully fix all the bugs above

@JazzarKarim
Copy link
Collaborator

JazzarKarim commented Oct 25, 2023 •
edited
Loading

@JazzarKarim @severinbeauvais push a new comment, hopefully fix all the bugs above

So I've tested different types of companies, looks great! Shaoyun, can you please take a SC from your local when the supported-restoration-entities FF is false and when a ULC for example is selected (just like how we discussed yesterday)?

@tshyun24
Copy link
Contributor Author

So I've tested different types of companies, looks great! Shaoyun, can you please take a SC from your local when the supported-restoration-entities FF is false and when a ULC for example is selected (just like how we discussed yesterday)?

Yes, I just create a ULC business (BC0871496) and make it historical
image
It looks like that in DEV. I think I should not have the Restore Now option? Cause it doesn't include the FF for ULC
image
And the below is in local, same with DEV
image

@JazzarKarim
Copy link
Collaborator

JazzarKarim commented Oct 25, 2023 •
edited
Loading

So I've tested different types of companies, looks great! Shaoyun, can you please take a SC from your local when the supported-restoration-entities FF is false and when a ULC for example is selected (just like how we discussed yesterday)?

Yes, I just create a ULC business (BC0871496) and make it historical

What if the FF value is false? Will it show the same thing but instead of "Restore Now", it's "Go To Corporate Online to Register"?

You can set this line to return false to test that! https://github.com/tshyun24/namerequest/blob/f495ad9e87a33ba472479575c3edab334510b7e8/src/mixins/common-mixin.ts#L209

@tshyun24
Copy link
Contributor Author

What if the FF value is false? Will it show the same thing but instead of "Restore Now", it's "Go To Corporate Online to Register"?

You can set this line to return false to test that! https://github.com/tshyun24/namerequest/blob/f495ad9e87a33ba472479575c3edab334510b7e8/src/mixins/common-mixin.ts#L209

Show "Go To Corporate Online to Register" when I set FF as false
image

@tshyun24
Copy link
Contributor Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-735-526soleu.web.app

@JazzarKarim
Copy link
Collaborator

What if the FF value is false? Will it show the same thing but instead of "Restore Now", it's "Go To Corporate Online to Register"?
You can set this line to return false to test that! https://github.com/tshyun24/namerequest/blob/f495ad9e87a33ba472479575c3edab334510b7e8/src/mixins/common-mixin.ts#L209

Show "Go To Corporate Online to Register" when I set FF as false image

Looks great! This is basically what we want.

@tshyun24
Copy link
Contributor Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-735-526soleu.web.app

@tshyun24
Copy link
Contributor Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-735-526soleu.web.app

@tshyun24
Copy link
Contributor Author

/gcbrun

@tshyun24
Copy link
Contributor Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://namerequest-dev--pr-735-526soleu.web.app

JazzarKarim
JazzarKarim approved these changes Oct 26, 2023
View reviewed changes
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.

I just double checked that the different types look good. I also reached out to Shaoyun for confirmation on a couple of stuff.

Looks great! Awesome job 👍

@tshyun24 tshyun24 merged commit d8d9a10 into bcgov:main Oct 26, 2023
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JazzarKarim JazzarKarim JazzarKarim approved these changes

@severinbeauvais severinbeauvais severinbeauvais approved these changes

@ketaki-deodhar ketaki-deodhar Awaiting requested review from ketaki-deodhar

@leodube-aot leodube-aot Awaiting requested review from leodube-aot

Assignees

@tshyun24 tshyun24

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@tshyun24 @chenhongjing @bcregistry-sre @severinbeauvais @JazzarKarim