-
Notifications
You must be signed in to change notification settings - Fork 54
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
25376 fix mailing address form issue #721
base: main
Are you sure you want to change the base?
25376 fix mailing address form issue #721
Conversation
/gcbrun |
Temporary Url for review: https://business-filings-dev--pr-721-ldoyrfrw.web.app |
Further testing are showing that this fix isn't working the way it seemed to previously - cancelling pull request. |
…ed up in future director name change requests Signed-off-by: Thayne Werdal <[email protected]>
…wDirector function in the editDirector function. It was causing the mailing address to set its fields to null. Signed-off-by: Thayne Werdal <[email protected]>
c0911b2
to
d10267a
Compare
… been added to the director list Signed-off-by: Thayne Werdal <[email protected]>
|
/gcbrun |
@severinbeauvais @JazzarKarim This one is now ready for review |
Temporary Url for review: https://business-filings-dev--pr-721-ldoyrfrw.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 so far.
Do you have a list of test cases (say, in the ticket)? Have you tested with Coops and corps ("base companies" such as BC, BEN, etc)?
Can you click the "Ready for review" button in the panel near the bottom of this PR? |
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 👍 (Please don't forget to mark this PR as ready for review).
There were no test cases in the ticket itself - but I ran through what was there - BC, ULC, BEN, CCC all work fine. Coop does not have mailing address, just delivery address, but that still seems to function as expected as well. |
I meant, did you create a set of test cases in the ticket (for QA to review)? Looks good. I'm going to approve momentarily. |
Added in test instructions for the two scenarios that previously caused issues, and steps for testing. https://app.zenhub.com/workspaces/entities---olga-65af15f59e89f5043c2911f7/issues/gh/bcgov/entity/25376 |
/gcbrun |
Temporary Url for review: https://business-filings-dev--pr-721-ldoyrfrw.web.app SB says, try this: https://business-filings-dev--pr-721-ldoyrfrw.web.app/BC0887509/standalone-directors?filingId=0 |
I tested the preview URL with the original bug description, and the labels looked OK and the filing went through. Good job! Merge when ready. |
Issue #: bcgov/entity#25376
Added a call to the current function that clears the address forms directly after pushing new director. If this is not done, it seems like the mailing address from the new director will overwrite the mailing address of any director you try to change the name of. It overwrites with null values in the mailing address, not allowing the user to fully commit changes.
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).