-
Notifications
You must be signed in to change notification settings - Fork 49
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
19041 Finished validations + updated certify statements + updated unit tests #646
Conversation
5e1590d
to
64789cb
Compare
} else if (this.isAmalgamationFilingHorizontal || this.isAmalgamationFilingVertical) { | ||
this.$router.push(RouteNames.AMALG_SHORT_INFORMATION).catch(() => {}) | ||
this.$router.replace(RouteNames.AMALG_SHORT_INFORMATION).catch(() => {}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is so that the new route replaces the old one, without pushing the old one (which was invalid for the current filing type) to the browser history. It's a small optimization. It means that, if the user clicks the Back button, they won't navigate to the invalid route.
@@ -300,7 +300,10 @@ export default class FilingTemplateMixin extends Mixins(AmalgamationMixin, DateM | |||
} | |||
|
|||
// restore the Amalgamation Court Approval if it's True or False | |||
if (draftFiling.amalgamationApplication.courtApproval !== null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This code failed if courtApproval did not exist on the amalgamationApplication object (eg, initial draft).
Because courtApproval was undefined (not null), line 304 was run, which saved "undefined" in the store. Then, in AmalgamationStatement.vue, this code set the validity to True (which is incorrect), and the overall filing validation did not catch that the amalgamation statement had not been selected.
Now, it all works correctly.
64789cb
to
1c47366
Compare
1c47366
to
4ad0361
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
- deleted useless isViteRunning getter (was only used in 1 place) - changed re-routing code to not push the wrong route to history - update misc comments - fixed amalgamation statement validation (was undefined on draft restore) - updated certify statements (for regular and short-form amalgamations) - fixed invalid date error in EffectiveDateTime - updated isAmalgamationInformationValid() - fixed amalgamation validation (ignore share structure validity for short-form amalg) - refactored Business Info test suite to handle regular vs horiz vs vert - misc cleanup
4ad0361
to
998fde4
Compare
@@ -238,7 +238,7 @@ export default class EffectiveDateTime extends Mixins(DateMixin) { | |||
|
|||
/** Construct the Date Object for storage */ | |||
private constructDate (): void { | |||
if (this.isFutureEffective) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This failed when dateText was empty string (eg, when changing from immediate to future-effective) -- dateToValidate on line 243 became null and either line 250 or line 257 caused an exception.
this.isAmalgamationFilingHorizontal || | ||
this.isAmalgamationFilingVertical || | ||
this.isCreateShareStructureValid | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Basically, don't check the share structure valid flag (because the share structure page was not displayed), but we can assume they are correct because we got them from the holding/primary company.
/gcbrun |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good 👍
- refactored resource update into separate method for use in multiple places
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…t tests (bcgov#646) * - app version = 5.8.8 - deleted useless isViteRunning getter (was only used in 1 place) - changed re-routing code to not push the wrong route to history - update misc comments - fixed amalgamation statement validation (was undefined on draft restore) - updated certify statements (for regular and short-form amalgamations) - fixed invalid date error in EffectiveDateTime - updated isAmalgamationInformationValid() - fixed amalgamation validation (ignore share structure validity for short-form amalg) - refactored Business Info test suite to handle regular vs horiz vs vert - misc cleanup * - set new resources when holding/primary business is changed * - also update resources when adopting a business' NOA or using a new NR - refactored resource update into separate method for use in multiple places --------- Co-authored-by: Severin Beauvais <severin.beauvais@gov.bc.ca>
Issue #: bcgov/entity#19041
This is a partial commit for the ticket, but it contains some useful fixes that I'd like merged asap.
Description of changes:
- app version = 5.8.8
- deleted useless isViteRunning getter (was only used in 1 place)
- changed re-routing code to not push the wrong route to history
- update misc comments
- fixed invalid date error in EffectiveDateTime
- fixed resources not updated when holding/primary business changes
- fixed amalgamation statement validation (was undefined on draft restore)
- updated certify statements (for regular and short-form amalgamations)
- fixed isAmalgamationInformationValid()
- fixed amalgamation validation (ignore share structure validity for short-form amalg)
- refactored Business Info test suite to handle regular vs horiz vs vert
- misc cleanup
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).