-
Notifications
You must be signed in to change notification settings - Fork 16
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
24656 - Handle NoW Draft in ToDoList #118
24656 - Handle NoW Draft in ToDoList #118
Conversation
Signed-off-by: Qin <[email protected]>
/gcbrun |
Temporary Url for review: https://business-dashboard-dev--pr-118-9dk5tahd.web.app |
Signed-off-by: Qin <[email protected]>
Signed-off-by: Qin <[email protected]>
@@ -10,8 +10,7 @@ VUE_APP_BUSINESSES_URL="https://dev.account.bcregistry.gov.bc.ca/" | |||
VUE_APP_BCONLINE_URL="https://d1.bconline.gov.bc.ca/" | |||
VUE_APP_BUSINESS_EDIT_URL="https://dev.edit.business.bcregistry.gov.bc.ca" | |||
VUE_APP_BUSINESS_CREATE_URL="https://dev.create.business.bcregistry.gov.bc.ca" | |||
# old dashboard | |||
VUE_APP_DASHBOARD_URL="https://dev.business.bcregistry.gov.bc.ca/" | |||
VUE_APP_BUSINESS_FILING_URL="https://dev.business.bcregistry.gov.bc.ca/" | |||
|
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.
Rename it to Filing URL
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 like this.
package.json
Outdated
@@ -19,7 +19,7 @@ | |||
}, | |||
"dependencies": { | |||
"@bcrs-shared-components/corp-type-module": "^1.0.16", | |||
"@bcrs-shared-components/enums": "^1.1.12", | |||
"@bcrs-shared-components/enums": "1.1.18", | |||
"@bcrs-shared-components/interfaces": "^1.1.16", |
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.
try to update the enum dependency, try to test if it fixes the filing type error
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.
Did you run pnpm i
after this?
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.
You may need to commit the lock file, too.
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.
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.
Thank you! Just committed the lock file
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 looks good, and so does the lock file. Resolving.
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.
@patrickpeinanw Can we keep this change and the lock file change? I assume the unit tests will be OK with just an update to the enums shared package.
@@ -41,7 +41,7 @@ export const doResumePayment = (item: TodoItemI): boolean => { | |||
export const doResumeFiling = (item: TodoItemI): void => { | |||
const { currentBusinessIdentifier } = useBcrosBusiness() | |||
const { bootstrapIdentifier } = useBcrosBusinessBootstrap() | |||
const { goToBusinessDashboard, goToCreatePage, goToEditPage } = useBcrosNavigate() | |||
const { goToFilingUI, goToCreatePage, goToEditPage } = useBcrosNavigate() | |||
|
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.
renamed
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.
Thanks!
(PS The dashboard code will be removed from Filings UI in the near future, so it will truly be only a filings UI.)
/gcbrun |
Signed-off-by: Qin <[email protected]>
/gcbrun |
Temporary Url for review: https://business-dashboard-dev--pr-118-9dk5tahd.web.app |
updated the enum dependency, now working: |
I see some failed checks. (In the previous PR, these checks passed.) This may be related to the lockfile issue 🤷♂️ |
Signed-off-by: Qin <[email protected]>
Signed-off-by: Qin <[email protected]>
Tried to manually update the enum dependency version, but the checks failed again. @severinbeauvais I will revert back to the old dependencies, like Patrick said Is it ok to leave the enum dependency change now? Then the Resume will not work, until we update it. |
Signed-off-by: Qin <[email protected]>
@patrickpeinanw Reverting the lock file back, all checks passed in my previous commit, but now it failed again. Do you know why or could you please re-run the failed test? |
/gcbrun |
Temporary Url for review: https://business-dashboard-dev--pr-118-9dk5tahd.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.
I don't know who's going to merge first, but please update the app version.
My hope is that Patrick approves the enum update and package lock change. Arwen, you had one commit where you updated the enum and lock file. Did the unit tests pass? |
It depends on Patrick's expert opinion. It also depends on whether the unit tests pass with this change. |
No, all tests failed when updating the package and lock file |
Darn. Well, do you want to update the unit tests in this PR or another one? |
Just tried, seems not easy, better in a new PR |
Signed-off-by: Qin <[email protected]>
OK, let's do that. I'll approve this PR in a minute. Try to get an approval from Patrick as well. |
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 to me! Thank you for updating the naming for Filing UI. goToBusinessDashboard
and dashboardOldUrl
were confusing.
Feel free to update the dependency version if needed. I am Ok with doing it either here or in another PR.
Re the failing cypress test, such failure occurs sometimes, and it is not always the same one. Re-running the test would usually remove the error but this time we got no luck. It is a tech debt that has been bugging us for a long time.
@severinbeauvais could you please merge it? I don't have admin right. |
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.
👍 👍
*Issue:*bcgov/entity#24656
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 namex license (Apache 2.0).