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

18535 - Add Amalgamating Businesses Part 1 #585

Merged
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 30 additions & 7 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

4 changes: 2 additions & 2 deletions package.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
{
"name": "business-create-ui",
"version": "5.6.3",
"version": "5.6.4",
"private": true,
"appName": "Create UI",
"sbcName": "SBC Common Components",
Expand All @@ -17,7 +17,7 @@
"@bcrs-shared-components/approval-type": "1.0.19",
"@bcrs-shared-components/base-address": "2.0.3",
"@bcrs-shared-components/breadcrumb": "2.1.15",
"@bcrs-shared-components/business-lookup": "1.1.30",
"@bcrs-shared-components/business-lookup": "1.2.3",
"@bcrs-shared-components/certify": "2.1.15",
"@bcrs-shared-components/completing-party": "2.1.30",
"@bcrs-shared-components/confirm-dialog": "1.2.1",
Expand Down
250 changes: 250 additions & 0 deletions src/components/Amalgamation/AmalgamatingBusinesses.vue
Original file line number Diff line number Diff line change
@@ -0,0 +1,250 @@
<template>
<div id="amalgamating-businesses">
<v-btn
id="btn-add-amalgamating-business"
outlined
color="primary"
class="btn-outlined-primary"
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 added to have the special color when button is disabled.

Copy link
Collaborator

Choose a reason for hiding this comment

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

When is this ever disabled?

Copy link
Collaborator

Choose a reason for hiding this comment

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

NVM - when you're prompting for a business name, the add buttons are disbabled.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Did we add this class elsewhere? What does the button look like without it?

Copy link
Collaborator Author

@JazzarKarim JazzarKarim Dec 1, 2023

Choose a reason for hiding this comment

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

Yes Sev!

class="btn-outlined-primary mt-6"

Left side is with it while right side is without it:
with and without

Copy link
Collaborator

Choose a reason for hiding this comment

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

Great! I forgot about that. Good job :)

:disabled="addAmalgatingBusinessDisabled"
@click="onAddBusinessClick()"
>
<v-icon>mdi-domain-plus</v-icon>
<span>Add an Amalgamating Business</span>
</v-btn>

<v-btn
v-if="isRoleStaff"
id="btn-add-amalgamating-foreign-business"
outlined
color="primary"
class="ml-2 btn-outlined-primary"
:disabled="addAmalgatingForeignBusinessDisabled"
@click="onAddForeignBusinessClick()"
>
<v-icon>mdi-domain-plus</v-icon>
<span>Add an Amalgamating Foreign Business</span>
</v-btn>

<!-- Add an Amalgamating Business button clicked -->
<v-expand-transition>
<v-card
v-if="isAddingAmalgamatingBusiness"
flat
class="section-container mt-4"
>
<v-row
no-gutters
>
<v-col
cols="12"
sm="3"
>
<label>Add a Business Registered in BC</label>
</v-col>

<v-col
cols="12"
sm="8"
class="ml-8"
>
<span>Enter the name or the incorporation number of the registered BC business
to add to this application.
</span>
<BusinessLookup
:showErrors="false"
:businessLookup="initialBusinessLookupObject"
:BusinessLookupServices="BusinessLookupServices"
label="Business Name or Incorporation Number"
@setBusiness="saveAmalgamatingBusiness($event)"
/>
<v-row
class="justify-end mr-0 mt-2"
severinbeauvais marked this conversation as resolved.
Show resolved Hide resolved
>
<v-btn
id="app-cancel-btn"
large
outlined
color="primary"
@click="addAmalgamatingBusinessCancel()"
>
<span>Cancel</span>
</v-btn>
</v-row>
</v-col>
</v-row>
</v-card>
</v-expand-transition>

<!-- Add an Amalgamating Foreign Business button clicked -->
<v-expand-transition>
<v-card
v-if="isAddingAmalgamatingForeignBusiness"
flat
class="section-container mt-4"
>
<v-row
no-gutters
>
<v-col
cols="12"
sm="3"
>
<label>Add a Foreign Business</label>
</v-col>

<v-col
cols="12"
sm="8"
class="ml-8"
>
<span>**TODO**</span>
Copy link
Collaborator Author

@JazzarKarim JazzarKarim Dec 1, 2023

Choose a reason for hiding this comment

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

To be done in part 2 ticket.

</v-col>

<!-- extra column is for possible action button -->
</v-row>
</v-card>
</v-expand-transition>
<v-row class="mt-4 ml-1">
<ul>
Amalgamating Businesses: <br><br>
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 all debug code here (can be removed at any point). This is just to show what happens whenever a business is added.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'll be in there soon 😁

<li
v-for="(business, index) in amalgamatingBuinesses"
:key="index"
>
<template v-if="business.foundingDate">
Legal Name: {{ business.legalName }} <br>
Legal Type: {{ business.legalType }} <br>
Mailing Address: {{ business.officeAddress.mailingAddress }} <br>
State: {{ business.state }} <br>
Good Standing: {{ business.goodStanding }} <br>
</template>
<template v-else>
Legal Name: {{ business.name }} <br>
Legal Type: {{ business.legalType }} <br>
Identifier: {{ business.identifier }} <br>
Status: {{ business.status }}
</template>
</li>
</ul>
</v-row>
</div>
</template>

<script lang="ts">
import { Component, Mixins } from 'vue-property-decorator'
import { Action, Getter } from 'pinia-class'
import { useStore } from '@/store/store'
import { CommonMixin } from '@/mixins'
import { BusinessLookupServices, LegalServices } from '@/services'
import { BusinessLookup } from '@bcrs-shared-components/business-lookup'
import { BusinessIF, BusinessLookupIF, EmptyBusinessLookup } from '@/interfaces'

@Component({
components: {
BusinessLookup
}
})
export default class AmalgamatingBusinesses extends Mixins(CommonMixin) {
@Getter(useStore) getAmalgamatingBusinesses!: Array<BusinessIF>
Copy link
Collaborator Author

@JazzarKarim JazzarKarim Dec 1, 2023

Choose a reason for hiding this comment

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

Not using this getter for now but eventually we'll have to use this getter here when continuing a draft.

@Getter(useStore) isAmalgamationFilingHorizontal!: boolean
@Getter(useStore) isRoleStaff!: boolean

@Action(useStore) setAmalgamatingBusinesses!: (x: Array<BusinessIF>) => void

// Local properties
amalgamatingBusinessesValid = false
amalgamatingBuinesses = []
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo

initialBusinessLookupObject = EmptyBusinessLookup

// Add an Amalgamating Business button properties
isAddingAmalgamatingBusiness = false
addAmalgatingBusinessDisabled = false

// Add an Amalgamating Foreign Business button properties
isAddingAmalgamatingForeignBusiness = false
addAmalgatingForeignBusinessDisabled = false

readonly BusinessLookupServices = BusinessLookupServices

// Cancel button in "Add an Amalgamating Business" is pressed.
addAmalgamatingBusinessCancel (): void {
this.isAddingAmalgamatingBusiness = false
// Enable buttons
this.addAmalgatingBusinessDisabled = false
this.addAmalgatingForeignBusinessDisabled = false
Copy link
Collaborator

Choose a reason for hiding this comment

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

Would it make more sense for the button to disable themselves when "is adding" is true? Curious.

Copy link
Collaborator Author

@JazzarKarim JazzarKarim Dec 1, 2023

Choose a reason for hiding this comment

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

Unless I'm missing something, but my code already does that Sev!

For example, on line 182, the "Add an Amalgamating Business" button disables itself when it is pressed (the panel is open is we're adding businesses).

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you're setting extra boolean variables that the buttons use.

I was thinking that the logic would be in the template itself, eg,

<v-btn
  :disabled="isAddingAmalgamatingBusiness || isAddingAmalgamatingForeignBusiness"

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Love this! Will definitely do that, thanks Sev

}

// "Add an Amalgamating Business" button is pressed.
onAddBusinessClick (): void {
this.isAddingAmalgamatingBusiness = true
this.isAddingAmalgamatingForeignBusiness = false
// Disable buttons
this.addAmalgatingBusinessDisabled = true
this.addAmalgatingForeignBusinessDisabled = true
}

// "Add an Amalgamating Foreign Business" button is pressed.
onAddForeignBusinessClick (): void {
this.isAddingAmalgamatingBusiness = false
this.isAddingAmalgamatingForeignBusiness = true
// Disable buttons (Please comment out the lines below or remove in part 2)
// this.addAmalgatingBusinessDisabled = true
// this.addAmalgatingForeignBusinessDisabled = true
}

async saveAmalgamatingBusiness (businessLookup: BusinessLookupIF): Promise<void> {
// Get the amalgamating business information
// Will have a different format depending on the business
let business = await LegalServices.fetchBusinessInfo(businessLookup.identifier)
.then((response) => {
return response?.data?.business
}).catch(() => {
return businessLookup
})
Comment on lines +185 to +190
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In here, it's different from what business we're fetching. Some of the businesses have all the information as in the BusinessIF while others (like A companies), don't have that. They have the format of BusinessLookupResultIF. I'm here capturing both depending on whether the response is OK.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you provide some tests to demonstrate all flows, please?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or, can you show the data from each scenario?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Absolutely Sev.

So, for businesses that are in LEAR, here's the data that I'm getting from the lookup:
big json

Otherwise, this is what I'm getting:
small json

So, I wanted to handle both flows/scenarios although the ticket mentions to assume all searched businesses are in LEAR.

Copy link
Collaborator

Choose a reason for hiding this comment

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

The risk with fallbacks is that they can make it look like things are working normally but something is actually wrong. In this case I think I'd prefer some error handling if the fetched business is not in LEAR.

Copy link
Collaborator

@severinbeauvais severinbeauvais Dec 1, 2023

Choose a reason for hiding this comment

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

I don't know if we need to use any of the data from the business lookup (except identifier), since we'll get it all from the "fetch business" LEAR call anyway. Maybe one way will seem best as you work on this.


// Get the address of the amalgamating business
if (businessLookup.identifier && business.foundingDate) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why do you have these checks? I think you don't need them.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The first one, you're right. For the second one, it is to handle whether the business was from LEAR. I'm unable to fetch the address if it's not a LEAR business. The foundingDate is a field only found in the big JSON above.

Copy link
Collaborator

Choose a reason for hiding this comment

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

If the business isn't in LEAR then fetchBusinessInfo above would have failed, too. (But your fallback hid this.)

I think, as part of a small refactor and making all network calls at once, you can just assume the business isn't in LEAR if any of the calls fails. (Actually, the Promise.all call would fail.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

For the second one, it is to handle whether the business was from LEAR. I'm unable to fetch the address if it's not a LEAR business. The foundingDate is a field only found in the big JSON above.

I think what I proposed is a better way, but if we kept the current code then you should add a code comment saying why you're checking the founding date. (I looked to see if you needed it for anything and it wasn't referenced, so I was confused.)

const addresses = await LegalServices.fetchAddresses(businessLookup.identifier)
.then((data) => {
// SP and GP have businessOffice instead of registeredOffice
return data?.registeredOffice || data?.businessOffice
}).catch(() => {
return undefined
})
if (addresses) {
business.officeAddress = addresses
}
Comment on lines +194 to +203
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Some businesses don't have an address (this is also showcased in the UXPin). If we were able to fetch the address, put it in the business object.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think all businesses have an address. What are you referring to?

Copy link
Collaborator Author

@JazzarKarim JazzarKarim Dec 1, 2023

Choose a reason for hiding this comment

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

The officeAddress field is being returned as empty from the fetchBusinessInfo call above. I'm making this call to get the addresses (same call as adding org or person):

const addresses = await LegalServices.fetchAddresses(businessLookup.identifier)

Sev, is this a bad thing (2 calls in this function)? Do you think that'll be bad performance?

Copy link
Collaborator

Choose a reason for hiding this comment

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

fetchBusinessInfo returns only the business record. I think you will always have to call fetchAddresses to fetch them.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sev, is this a bad thing (2 calls in this function)? Do you think that'll be bad performance?

We knew this coming in. In fact, I think you also need to make an Auth call to fetch the contact info email address.

However, instead of doing all 3 network calls sequentially, you can do them in parallel as we do here or in Edit UI.

}

// If the amalgamating businesses array is not empty, check if identifier already exists.
// If identifier already exists, don't add the business to the array.
if (this.amalgamatingBuinesses.length > 0) {
const businessExists = this.amalgamatingBuinesses.find(function (id) {
return id.identifier === business.identifier
})
if (!businessExists) this.amalgamatingBuinesses.push(business)
} else {
this.amalgamatingBuinesses.push(business)
}

// Set the amalgamated businesses array in the store.
this.setAmalgamatingBusinesses(this.amalgamatingBuinesses)
// Close the "Add an Amalgamating Business" Panel.
this.addAmalgamatingBusinessCancel()
}
}
</script>

<style lang="scss" scoped>
@import '@/assets/styles/theme.scss';

.v-btn:not(.v-btn--round).v-size--default {
height: 44px;
}

// Overriding the section container class right padding.
.section-container {
padding-right: 0rem;
Copy link
Collaborator

Choose a reason for hiding this comment

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

pr-0 doesn't work on the v-card element?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It does actually but it looked a bit better doing this (a bit less spacing). IDK, maybe I'm imagining things haha. Will go for pr-0.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Once upon a time, Cam and I had to seriously clean up a lot of styling in a UI. (Guess which one!) We quickly learned to love the Vuetify layout helper classes right in the template (esp for whitespace).

For other CSS, it sometimes makes more sense to have it in a style block. For example: common (base) CSS, or CSS you want to apply to all similiar elements.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep, you're absolutely right. Will definitely be removing this!

Copy link
Collaborator

Choose a reason for hiding this comment

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

(Yes, in some cases I wish the linter would allow us to have multiple things on the same line, but alas.)

For example, <p id="boo" class="font-weight-bold:">BOO!</p> is nice and simple, but lint wants it on 6 lines!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Lint is a double-edged sword isn't it 😆

}

</style>
Original file line number Diff line number Diff line change
@@ -1,15 +1,7 @@
import { AmalgamationTypes, ApprovalTypes, RelationshipTypes } from '@/enums'
import { CourtOrderIF } from '@/interfaces'
import { AmalgamationTypes } from '@/enums'
import { BusinessIF } from '@/interfaces'

export interface AmalgamationStateIF {
applicationDate?: string // YYYY-MM-DD
approvalType: ApprovalTypes
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed all the useless fields here for now. We'll eventually going to have to add lots of stuff in here depending on the ticket.

approvalTypeValid: boolean
businessNameValid: boolean
courtOrder?: CourtOrderIF
expiry?: string // YYYY-MM-DD
noticeDate?: string // YYYY-MM-DD
relationships?: RelationshipTypes[]
restorationTypeValid: boolean
amalgamatingBusinesses: Array<BusinessIF>
type: AmalgamationTypes
}
12 changes: 1 addition & 11 deletions src/store/state/state-model.ts
Original file line number Diff line number Diff line change
Expand Up @@ -206,17 +206,7 @@ export const stateModel: StateModelIF = {
isAutoPopulatedBusinessNumber: false
},
amalgamation: {
applicationDate: null,
approvalType: null,
approvalTypeValid: true,
businessNameValid: false,
courtOrder: {
fileNumber: null
},
expiry: null,
noticeDate: null,
relationships: [],
restorationTypeValid: false,
amalgamatingBusinesses: [],
type: null
},
restoration: {
Expand Down
Loading
Loading