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

19750 and 19805 - fixed error handling + fixed party index bug #657

Merged
merged 2 commits into from
Feb 15, 2024

Conversation

severinbeauvais
Copy link
Collaborator

@severinbeauvais severinbeauvais commented Feb 14, 2024

Issue #: bcgov/entity#19750 and bcgov/entity#19805

Description of changes:

  • app version = 5.9.2
  • fixed error handling bug (missing await)
  • refactored ListPeopleAndRoles to retain original index for actions
  • removed unneeded code in buildAmalgamationFiling()
  • updated some comments

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

- fixed error handling bug (missing await)
- removed unneeded code in buildAmalgamationFiling()
- updated some comments
@@ -567,7 +567,7 @@ export default class AmalgamatingBusinesses extends Mixins(AmalgamationMixin, Co

// fetch the new holding/primary business' data and update the prepopulated data
// this will overwrite office addresses, directors, share structure and contact info
this.updatePrepopulatedData(business, true)
await this.updatePrepopulatedData(business, true)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Without this, the code continued to line 576.

Instead, we needed to wait for updatePrepopulatedData() to finish and possibly throw an error that would be caught by line 571.

@@ -144,8 +144,7 @@ export default class FilingTemplateMixin extends Mixins(AmalgamationMixin, DateM
amalgamatingBusinesses: this.getAmalgamatingBusinesses,
type: this.getAmalgamationType,
nameRequest: {
legalType: this.getEntityType,
correctNameOption: this.getCorrectNameOption
Copy link
Collaborator Author

@severinbeauvais severinbeauvais Feb 14, 2024

Choose a reason for hiding this comment

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

Not needed as it's (re)set on lines 178+ below.

This is a the "decoy" anti-pattern: it looks like it's doing something but it's really not.

Copy link
Contributor

@PaulGarewal PaulGarewal left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Go to ⚙️ icon above and check "Hide whitespace" to see what I did here more clearly.

@@ -76,146 +76,148 @@
</v-row>

<!-- List Content -->
<v-row
v-for="(orgPerson, index1) in filteredPersonList"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

First, I moved the v-for into its own element (since can't have v-for and v-if in same element).

Second, I added v-if to conditionally show directors. This way, the v-for still iterates over the whole list and index1 is retained, and the actions act on the correct list item.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Example of short-form amalgamation (completing party only):

image

Example of regular amalgamation (completing party is also a director):

image

Copy link
Contributor

@PaulGarewal PaulGarewal left a comment

Choose a reason for hiding this comment

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

May have reviewed too early, will review again tomorrow morning 🤝

@severinbeauvais
Copy link
Collaborator Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

bcregistry-sre commented Feb 15, 2024

Temporary Url for review: https://business-create-dev--pr-657-or2ets8j.web.app

SB says, try these:

Compare the above with the full list of People and Roles on the Review page. You'll see that the completing party is not always index[0] but editing it works correctly in all cases.

@bcgov bcgov deleted a comment from bcregistry-sre Feb 15, 2024
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!

Copy link
Contributor

@jamespaologarcia jamespaologarcia left a comment

Choose a reason for hiding this comment

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

LGTM

@severinbeauvais severinbeauvais merged commit 410150a into bcgov:main Feb 15, 2024
5 checks passed
@severinbeauvais severinbeauvais changed the title 19750 Fixed error handling 19750 and 19805 - fixed error handling + fixed party index bug Feb 15, 2024
JazzarKarim pushed a commit to JazzarKarim/business-create-ui that referenced this pull request Feb 23, 2024
* - app version = 5.9.2
- fixed error handling bug (missing await)
- removed unneeded code in buildAmalgamationFiling()
- updated some comments

* - refactored ListPeopleAndRoles to retain original index for actions

---------

Co-authored-by: Severin Beauvais <severin.beauvais@gov.bc.ca>
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