-
Notifications
You must be signed in to change notification settings - Fork 46
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
20779 Implemented Easy Legal Name Fix #570
20779 Implemented Easy Legal Name Fix #570
Conversation
/gcbrun |
As I have a PR that I've been working on for a couple of weeks (!) and am final-testing, I'd really appreciate if my PR could go first and then you could apply your changes on top. Is that OK? |
Absolutely, I wanted to mention this to you. Some of it also overlaps with what I have here. So, you can merge yours first and then I'll rebase. No problem! |
86004c2
to
fe72e66
Compare
/gcbrun |
[] | ||
)) | ||
|
||
// store Provisions Removed | ||
if (filing.alteration.provisionsRemoved) this.setProvisionsRemoved(true) | ||
|
||
// store Office Addresses **from snapshot** (because we don't change office addresses in an alteration) | ||
this.setOfficeAddresses(cloneDeep(entitySnapshot.addresses)) | ||
this.setOfficeAddresses(cloneDeep(entitySnapshot?.addresses || 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.
In the future, I think it would be better to check if addresses exists before setting them (and same for all other data below). That way, we can leave the initial values in the store and don't need to re-assign those values here as fallbacks. (Initial values in 1 place only.)
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.
Got it, noted! Let me know if you want me to change that here.
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.
Not here. Future.
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.
That's a lot of changes post-merge but they look OK to me.
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
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!
Issue #: /bcgov/entity#20779
Description of changes:
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).