14670 Added ledger component for restoration extension filings #468
Conversation
|
You have successfully added a new SonarCloud configuration ``. As part of the setup process, we have scanned this repository and found no existing alerts. In the future, you will see all code scanning alerts on the repository Security tab. |
severinbeauvais
force-pushed
the
14670_phase1
branch
from
1b0bbd1
to
b3ca739
Compare
Codecov Report
@@ Coverage Diff @@
## main #468 +/- ##
==========================================
- Coverage 82.88% 81.71% -1.18%
==========================================
Files 109 130 +21
Lines 2104 2560 +456
Branches 659 845 +186
==========================================
+ Hits 1744 2092 +348
- Misses 360 467 +107
- Partials 0 1 +1
|
severinbeauvais
force-pushed
the
14670_phase1
branch
from
b3ca739
to
6a3da56
Compare
src/components/EntityInfo.vue
Outdated
| </dd> | ||
| </template> | ||
| </dl> | ||
| <Tombstone /> |
There was a problem hiding this comment.
If you have a better name for this, let me know!
There was a problem hiding this comment.
I think it's recommended to use two words for component names. What about EntityKeyFacts? See: https://v2.vuejs.org/v2/style-guide/?redirect=true#Multi-word-component-names-essential
There was a problem hiding this comment.
Funny thing here: I tried another 1-word component elsewhere and my tools complained. But it didn't complain here. 🤷♂️
I'll think about this... Maybe I can componentize the left side data and name this right side data at the same time.
There was a problem hiding this comment.
Please see new commit and resolve this conversation if you are satisfied.
severinbeauvais
requested review from
jonathan-longe,
ketaki-deodhar and
JazzarKarim
- imported latest shared corp-type-module - imported latest Vuetify - replaced some getLegalName with getEntityName - added some getLegalName fallback strings - fixed some typing errors - added handling for Restoration Extension filings - added Limited Restoration Extension Filing component - renamed isAdminFreeze -> isAdminFrozen - changed some Enum Mixin calls to Enum Utilities - deleted obsolete Enum Mixin methods - cleaned up EntityInfo first and second lines - moved EntityInfo tombstone data to new component - renamed some IDs - added some getters to get rid of `@State` usage - deleted some unneeded getter refs - created Tombstone component - added getEntityName getter - moved some computed to store getters - created/updated/fixed unit tests - misc cleanup
severinbeauvais
force-pushed
the
14670_phase1
branch
from
6a3da56
to
d0ae65e
Compare
| // add properties for limited restoration extensions | ||
| if (EnumUtilities.isTypeRestorationExtension(filing)) { | ||
| item.isTypeLimitedRestorationExtension = true | ||
| item.expiry = filing.data.restoration?.expiry | ||
| } | ||
|
|
There was a problem hiding this comment.
Would it be possible to add these attributes inside the LimitedRestorationExtension filing history component?
There was a problem hiding this comment.
Yes but this is Phase 1 of this work -- using the existing architecture. In my next PR, I will change things over so this code is in individual filing sub-components.
There was a problem hiding this comment.
Are you OK with leaving this until Phase 2 of this work (next PR)?
src/components/Dashboard/FilingHistoryList/LimitedRestorationExtensionFiling.vue
Show resolved
Hide resolved
src/components/EntityInfo.vue
Outdated
| <template v-if="!!businessId"> | ||
| <!-- First line --> | ||
| <div id="entity-legal-name" aria-label="Entity Legal Name"> | ||
| {{ getEntityName || 'Unknown Name' }} | ||
| </div> |
There was a problem hiding this comment.
What do you think of creating two components, EntityInfoBusinessId and EntityInfoTempRegId? It feels like we're mixing concerns by combining the two.
There was a problem hiding this comment.
I'll look into this but it is a bit beyond the scope of this PR.
There was a problem hiding this comment.
I'm working on this (and old "Tombstone" sub-component) atm. I think you'll like what I've done 😁
There was a problem hiding this comment.
Please see new commit and resolve this conversation if you are satisfied.
src/components/Tombstone.vue
Outdated
| <template v-if="!!registrationDate"> | ||
| <dt class="mr-2">Registration Date:</dt> | ||
| <dd id="registration-date">{{ registrationDate }}</dd> | ||
| </template> |
There was a problem hiding this comment.
What do you think about creating a component for the items? It looks like you have only two variants, a display version and an edit version. The benefit is that if the business needs to modify the key fact displayed all we need to do is change the data supplied. We don't have to add an if-then.
There was a problem hiding this comment.
There is no edit version here; only items that may or may not apply to the currently loaded entity.
Sorry if I don't understand your suggestion. What do you mean?
src/components/Tombstone.vue
Outdated
| /** The Business ID string, if available. */ | ||
| get businessId (): string { | ||
| return sessionStorage.getItem('BUSINESS_ID') | ||
| } |
There was a problem hiding this comment.
What do you think about passing the businessId in as a prop? If we eventually want to have the businessId passed to the view as route parameter, this makes this refactoring easier in the future.
There was a problem hiding this comment.
Good idea!
There was a problem hiding this comment.
Please see new commit and resolve this conversation if you are satisfied.
| /** DEPRECATED Returns True if item status is Cancelled. */ | ||
| isStatusCancelled (item: any): boolean { | ||
| return (item.status === FilingStatus.CANCELLED) | ||
| } | ||
|
|
||
| /** DEPRECATED Returns True if item status is Completed. */ | ||
| isStatusCompleted (item: any): boolean { | ||
| return (item.status === FilingStatus.COMPLETED) | ||
| } | ||
|
|
||
| /** DEPRECATED Returns True if item status is Corrected. */ | ||
| isStatusCorrected (item: any): boolean { | ||
| return (item.status === FilingStatus.CORRECTED) | ||
| } | ||
|
|
||
| /** DEPRECATED Returns True if item status is Deleted. */ | ||
| isStatusDeleted (item: any): boolean { | ||
| return (item.status === FilingStatus.DELETED) | ||
| } | ||
|
|
There was a problem hiding this comment.
Nice! Get rid of those deprecated methods.
|
|
||
| <p> | ||
| The period of restoration was successfuly extended and is active | ||
| <strong>until {{ expiryDateFriendly }}</strong>. At the end of the extended limited |
There was a problem hiding this comment.
Nice! This will help with testing in my extension section ticket in Edit UI! 😄
- moved Entity Info menu to new component - renamed Tombstone -> EntityDefinitions - misc cleanup - fixed a getter name - updated/created unit test suites for affected components
|
Kudos, SonarCloud Quality Gate passed!
|
| <template v-if="!!businessId"> | ||
| <!-- Title --> | ||
| <div id="entity-legal-name" aria-label="Entity Legal Name"> | ||
| {{ getEntityName || 'Unknown Name' }} | ||
| </div> | ||
|
|
||
| <!-- Subtitle --> | ||
| <div> | ||
| <span id="business-description">{{ businessDescription }}</span> | ||
|
|
||
| <span id="limited-restoration" class="ml-3" v-if="isInLimitedRestoration"> | ||
| <span class="ml-3">Active until {{ getLimitedRestorationActiveUntil || 'Unknown' }}</span> | ||
| <v-chip class="primary mt-n1 ml-3 pointer-events-none font-weight-bold" | ||
| small label text-color="white">LIMITED RESTORATION</v-chip> | ||
| </span> | ||
|
|
||
| <span id="authorized-to-continue-out" v-if="isAuthorizedToContinueOut"> | ||
| <v-chip class="primary mt-n1 ml-3 pointer-events-none font-weight-bold" | ||
| small label text-color="white">AUTHORIZED TO CONTINUE OUT</v-chip> | ||
| </span> | ||
| </div> | ||
| </template> |
There was a problem hiding this comment.
I know this may seem like a pain, but can we separate the code for entities with a businessId from entities with a tempRegNumber into their own components? I eventually would like to have separate dashboard routes for entities with a businessID and entities with a tempRegNumber. Separating these concerns will make this easier.
There was a problem hiding this comment.
I'd rather not, at this time anyway. Separate routes sound a good idea for the future, but it's still fair to have separate views use common components that check a variable to display A vs B. Also, I like the fact that the Entity Info's header content is all in one file. Also, I don't want to assume that businesses vs pre-businesses (temp reg number) will continue to be handled separately in the current way. Also, this isn't a complex component to break apart in the future if the need arises (the need isn't here yet). Finally, there are many higher-priority, higher-gain things to work first/instead.
Here's an idea of what we'll be up against if we try to split up businesses from pre-businesses -- the A vs B code is in a lot of places so that we can reuse components that don't change very much.
| <template v-if="!!tempRegNumber"> | ||
| <!-- Title --> | ||
| <div id="ia-reg-name" aria-label="Incorporation Application or Registration Entity Name"> | ||
| {{ getEntityName || 'Unknown Name' }} | ||
| </div> | ||
|
|
||
| <!-- Subtitle --> | ||
| <div id="ia-reg-description" aria-label="Incorporation Application or Registration Description"> | ||
| {{ iaRegDescription }} | ||
| </div> | ||
| </template> |
There was a problem hiding this comment.
Here's the template for the EntityHeaderTempReg
There was a problem hiding this comment.
If you don't want to see this template when you're in this file, code-fold it.
At the moment, I prefer to keep like content together instead of managing more component files. Also see my previous comment on this file.
| <template v-if="!!registrationDate"> | ||
| <dt class="mr-2">Registration Date:</dt> | ||
| <dd id="registration-date">{{ registrationDate }}</dd> | ||
| </template> |
There was a problem hiding this comment.
Apologies for my cryptic comment earlier! What do you think of making the following a component?
<template>
<dt class="mr-2">{{ title }}:</dt>
<dd :id="id">{{ value }}</dd>
</template>And this a component too?
<template>
<dt class="mr-2">{{ title }}:</dt>
<dd :id="id" class="cursor-pointer" @click="editBusinessProfile()">
<span>{{ value }}</span>
<v-btn :id="'change' + id + 'button'" small text color="primary">
<v-icon small>mdi-pencil</v-icon>
<span>Change</span>
</v-btn>
</dd>
</template>I know this may seem crazy to create tiny components like this but I think you'll see it makes our code more flexible. By displaying these components by iterating over an array of data, it makes it very easy to change in the future if the business changes the requirements.
There was a problem hiding this comment.
We don't need that flexibility yet. Let's change it when/if we need to.
Right now I prefer to keep this entire "description list" together.
| <span v-if="isHistorical"> | ||
| <v-chip | ||
| id="historical-chip" | ||
| class="primary mt-n1 ml-4 pointer-events-none" small label text-color="white" | ||
| > | ||
| <strong>HISTORICAL</strong> | ||
| </v-chip> | ||
| <span class="font-14 mx-3">{{ getReasonText || 'Unknown Reason' }}</span> | ||
| </span> |
There was a problem hiding this comment.
This looks like a separate concern that deserves it's own component.
There was a problem hiding this comment.
I see a continuum between mega components and mini components. In this case, I prefer to keep all there "header menu" items together.
(One rule I use to keep things together is "are they related"? One rule I use for splitting out sub-components is "are they used in multiple places? (DRY)". In this case, I have other concerns and priorities, and I'm not dissatisfied with this code.)
| <span v-if="!!businessId && isAllowed(AllowableActions.ADD_STAFF_COMMENT)"> | ||
| <StaffComments | ||
| :axios="axios" | ||
| :businessId="businessId" | ||
| :maxLength="2000" | ||
| /> | ||
| </span> |
There was a problem hiding this comment.
Rather than writing conditionals for each allowable action, would it be possible to iterate over the allowable actions returned by the API and then use a <component :is=""> to render these menu items dynamically? It seems to me that this would make our code easier to extend and maintain.
There was a problem hiding this comment.
That's a good idea but I doubt it: the possible actions (and whether they're allowed or not) are sprinkled into different places in the code/components. It boils down to our desire to show a consistent page with various actions but those actions are in different places (header, right-side components, staff filings dropdown, etc).
This could be investigated further, but IMO it's not the highest priority in this UI, not even in the top 3-4 (ie, behind Filing History, Todo List, App.vue, local filings, and more).
| <template> | ||
| <menu id="entity-menu"> |
There was a problem hiding this comment.
What do you think about either moving <menu> back to the parent or creating a wrapper with a slot so the subcomponents can be listed like we were thinking about doing with the YourCompany component?
There was a problem hiding this comment.
I'd really like to keep this as a self-contained component.
By the way, refactoring EntityInfo is not at all in scope for this ticket; I've updated some things "off the side of my desk" as a result of some conversations you and I had. IMO this has already hijacked too much time from this ticket. I really like the changes that I made here so far, and yes this could be extended, but now these extra asks are stalling development. Please create a new ticket if you feel strongly that the Entity Info needs to be further refactored, but we reallly do have more important things to improve first.
| <v-btn | ||
| small text color="primary" | ||
| id="download-summary-button" | ||
| @click="emitDownloadBusinessSummary()" | ||
| v-on="on" | ||
| > |
There was a problem hiding this comment.
When this button is clicked, it:
- calls a method,
emitDownloadBusinessSummary() - which emits an event called,
downloadBusinessSummary - which is received by the parent, "EntityInfo"
- which calls another method called,
emitDownloadBusinessSummary - which emits another method called
downloadBusinessSummary - which is received by the parent, "App.vue"
- which calls another method called,
downloadBusinessSummary - which calls the API and downloads the requested document
Would it be possible to call a store action instead or, if you don't have time, create a refactoring ticket that explains the issue and describes a resolution?
There was a problem hiding this comment.
| <v-btn | ||
| small text color="primary" | ||
| id="dissolution-button" | ||
| :disabled="hasBlocker" | ||
| @click="promptDissolve()" | ||
| v-on="on" | ||
| > |
There was a problem hiding this comment.
This button and several other buttons in this component appear to also have a long chains of emits that are passed back to the parent and then the grandparent. Can we either reduce the length of the chains or create tech debt tickets to refactor this?
There was a problem hiding this comment.
|
@jonathan-longe Thanks for all your forward-looking comments -- I really appreciate (and support) your efforts to move us towards a more flexible architecture. However, this ticket/PR was only meant to add a specific functionality using the existing architecture -- I am using ticket 15692 to refactor a variety of things, but even then the scope needs to be limited so we can balance cleanup vs new features. I heard we have support to spend more time up front on this (pay now instead of pay later) but I'm trying to be careful to be concise in approved tickets, and creating other tickets for future work (or not creating tickets if the value isn't clear today -- they can be created later). For example, THIS ticket was estimated as a 3 and it's already used up 5 worth of time. And 15692 was also estimated as a 3 and maybe should be a 5 (due to revamped unit tests and QA). I'm OK with more tickets but I don't want to blow up current tickets. Please continue your thorough reviews and spring of ideas, and please don't be upset if I push back a little :) |
|
Jonathan, I'm not going to overrule you and merge this ticket as is. Please let me know if I have your approval (and if you're comfortable that outstanding issues are being handled appropriately). |
|
Understood @severinbeauvais I definitely don't want to hold you up or become a PITA :-) Please go ahead and merge the code you're comfortable merging. |
Issue #: bcgov/entity#14670
Description of changes:
First commit:
@StateusageSecond commit:
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the business-filings-ui license (Apache 2.0).