-
Notifications
You must be signed in to change notification settings - Fork 31
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
changed owner group type as NA #2011
changed owner group type as NA #2011
Conversation
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.
I'm very nervous about home owner changes, as these components support a dozen flows with a dozen different rule sets.
If you confident in these changes however and it's been weighed as a critical fix, then the code looks good to me.
/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.
LGTM
I will keep the current rule as it is, and add new condition for |
Temporary Url for review: https://bcregistry-assets-dev--pr-2011-3hw2g56l.web.app |
Totally up to you, is the fix required for all Transfers? |
Nope, just for |
/gcbrun |
Temporary Url for review: https://bcregistry-assets-dev--pr-2011-3hw2g56l.web.app |
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!
Issue #: /bcgov/entity#22264
Description of changes:
Since we push deleted owner under
if (ownerGroup.owners.some(owner => owner.action === ActionTypes.REMOVED))
condition, we don't needownerGroup.owners.filter(owner => owner.action === ActionTypes.REMOVED).length > 1
and thetype
is alwaysJOINT
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of the PPR license (Apache 2.0).