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

21310 Implement Authorization Date min date validation #701

Merged
merged 3 commits into from
Jun 4, 2024

Conversation

severinbeauvais
Copy link
Collaborator

@severinbeauvais severinbeauvais commented Jun 3, 2024

Issue #: bcgov/entity#21310

Description of changes:

Commit 1:

  • app version = 5.10.20
  • added Authorization Date min date validation

Commit 2:

  • trimmed whitespace from text input fields
  • now deep-watch objects

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

Screenshots:

Minimum date is May 16, 2001 because that's the date of registration in BC.
image

image

Minimum date is Jan 15, 20024 because that's the date of incorporation in home jurisdiction.
image

image

@severinbeauvais severinbeauvais self-assigned this Jun 3, 2024
- added Authorization Date min date validation
this.getExistingBusinessInfo.homeIncorporationDate ||
null
)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fall back to null because 'undefined' is not reactive (ie, getters and watchers won't recognize its change).

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.

Tested, LGTM

@@ -132,7 +132,7 @@
sm="9"
>
<v-text-field
v-model="business.homeIdentifier"
v-model.trim="business.homeIdentifier"
Copy link
Collaborator Author

@severinbeauvais severinbeauvais Jun 4, 2024

Choose a reason for hiding this comment

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

This modifier automatically trims leading and trailing spaces. (Note: on screen, it looks like there are trailing spaces, but in the variable itself, those spaces aren't there.)

Ref: https://vuejs.org/guide/essentials/forms#trim

cc: @JazzarKarim

Copy link
Collaborator

Choose a reason for hiding this comment

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

Very very nice.

(v) => !!v || 'Identifying Number is required',
(v) => (v && v.length <= 50) || 'Cannot exceed 50 characters'
(v) => !!v?.trim() || 'Identifying Number is required',
(v) => (v && v.trim().length <= 50) || 'Cannot exceed 50 characters'
Copy link
Collaborator Author

@severinbeauvais severinbeauvais Jun 4, 2024

Choose a reason for hiding this comment

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

Apparently, Vuetify doesn't trim the value before applying the rules, so trim() is needed here to determine if the value is falsy (including empty string) and to corrrectly check its length.

@@ -640,10 +640,8 @@ export default class ExtraproRegistration extends Mixins(DateMixin) {

/** Emits form validity. */
@Watch('isBusinessActive')
@Watch('business.homeJurisdiction')
@Watch('business.homeIncorporationDate')
@Watch('business', { deep: true })
Copy link
Collaborator Author

@severinbeauvais severinbeauvais Jun 4, 2024

Choose a reason for hiding this comment

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

The old code assumed the form validity would change and catch any data changes, but that's incorrect. For example, I could change a home identifier or home legal name character and the form validity wouldn't change, and also line 649 wouldn't be called, so the page wouldn't warn you if you tried to leave with unsaved changes.

Instead of adding those extra watchers, in the new code I simply deep-watch the object (ie, all properties). The downside is that this code is called every time a character changes, but the performance hit is not noticeable.

cc: @JazzarKarim

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes a lot of sense.

@bcgov bcgov deleted a comment from bcregistry-sre Jun 4, 2024
@severinbeauvais
Copy link
Collaborator Author

/gcbrun

@bcregistry-sre
Copy link
Collaborator

Temporary Url for review: https://business-create-dev--pr-701-1jrgf7gx.web.app

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!

@severinbeauvais severinbeauvais merged commit 90487df into bcgov:main Jun 4, 2024
4 of 5 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.

4 participants