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.

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

24678 Updated NR error dialog #603

Merged
merged 2 commits into from
Dec 5, 2024
Merged

Conversation

severinbeauvais
Copy link
Collaborator

@severinbeauvais severinbeauvais commented Dec 4, 2024

Issue #: bcgov/entity#24678

Description of changes:

  • app version = 4.11.11
  • added a base class
  • updated NR error dialog title
  • refactored NR error dialog body (now 4 cases)
  • updated unit tests

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

@severinbeauvais severinbeauvais self-assigned this Dec 4, 2024
@severinbeauvais
Copy link
Collaborator Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

bcregistry-sre commented Dec 5, 2024

Temporary Url for review: https://business-edit-dev--pr-603-xuwh1999.web.app

SB says, try these filings:

With these name requests (using ph: 1234):

  • ULC change of name: NR 0556528
  • ULC to BC conversion: NR 8143115
  • SP change of name: NR 6570270
  • BC change of name: NR 0853481
  • CP change of name: NR 4962552
  • BC restoration (BC0402998): NR 5621920

See ticket for screenshots.

- removed wrong-looking page alert icon
- added a base class
- refactored NR error check
- updated NR error dialog title
- refactored NR error dialog text (now 3 cases with sub-cases) including new error text
- added CHG and CNV NRs as valid options for Alteration filings
- removed CNV NR as a valid option for corp Correction filings
- added CHG NR as a valid option for Limited Restoration to Full filings
- added CHG NR as a valid option for Special Resolution filings
- updated unit tests
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.

LGTM

src/App.vue Outdated Show resolved Hide resolved
Copy link
Collaborator

@ArwenQin ArwenQin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good

Copy link
Collaborator

@ketaki-deodhar ketaki-deodhar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good!

@@ -100,7 +100,8 @@
v-if="bannerText"
tile
dense
type="warning"
class="mb-0"
color="warning"
Copy link
Collaborator Author

@severinbeauvais severinbeauvais Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before (warning icon is mostly hidden, incorrect whitespace below alert):
image

After:
image

@@ -55,7 +55,8 @@ const vuetify = new Vuetify({
error: '#d3272c', // same as $app-red
success: '#1a9031', // same as $app-green
gray7: '#495057', // same as $gray7
gray9: '#212529' // Same as $gray9
gray9: '#212529', // Same as $gray9
warning: '#fb8c00'
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the hex code for a v-alert of type "warning".

@@ -23,6 +24,10 @@ export const AlterationResourceBc: ResourceIF = {
CorrectNameOptions.CORRECT_NEW_NR,
CorrectNameOptions.CORRECT_NAME_TO_NUMBER
],
nameRequestTypes: [
NrRequestActionCodes.CHANGE_NAME,
NrRequestActionCodes.CONVERSION
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either of these NR types can be used for an alteration.

Ditto below, etc.

@@ -25,8 +25,7 @@ export const CorrectionResourceBc: ResourceIF = {
subtitle: null // not used
},
nameRequestTypes: [
NrRequestActionCodes.CHANGE_NAME,
NrRequestActionCodes.CONVERSION
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A conversion NR is not valid for a correction (since we do not change business type in a correction).

@@ -27,6 +27,7 @@ export const RestorationResourceBc: ResourceIF = {
subtitle: null // not used
},
nameRequestTypes: [
NrRequestActionCodes.CHANGE_NAME,
NrRequestActionCodes.RESTORE
Copy link
Collaborator Author

@severinbeauvais severinbeauvais Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the business is in limited restoration then it's active, and Namerequest UI won't find it in a historical search for a restoration NR. So, this allows the use of a change of name NR for a limited-restored business.

@@ -23,10 +23,10 @@ export const SpecialResolutionResourceCp: ResourceIF = {
correctNameOptions: [
CorrectNameOptions.CORRECT_NEW_NR
],
typeChangeInfo: 'You cannot change the business type of a Cooperative Association. You must form a new' +
'business and dissolve this business once the new business is registered.',
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was missing a space between "new" and "business".


nr.request_action_cd = 'CHG'
expect(vm.isNrInvalid(nr)).toBe(true)
expect(vm.isNrInvalid(nr)).toBe(false) // valid
Copy link
Collaborator Author

@severinbeauvais severinbeauvais Dec 5, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Name Request Mixin now allows 3 types of NRs (CNV, CHG, REH). It's the component itself that will check the NR type against the allowed types from the resources (depending on the filing type).

@severinbeauvais severinbeauvais merged commit f830cd6 into bcgov:main Dec 5, 2024
8 of 10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants