13058 COLIN API failure - Front and Backend Change validation of the first name field in Coop IA UI and/or Corp Party table

lewischenstudio commented 2 years ago

Description of changes:

change the max length of "middleNameRules" to 20

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

lewischenstudio commented 2 years ago

Please follow the instructions in the README.

I don't know if you ran the unit tests.

I see you didn't update Storybook or commit its updated files.

I followed the README instructions and got the following issues/questions:

npm i and npm run storybook:build both failed for Node 16.15.0. I changed Node to v12.22.12, and both commands succeeded.
npm run storybook succeeded for Node v12.22.12 but not for 16.15.0.
npm run test:unit failed while running the tests for Node v12.22.12, and there are missing components.
Contents in ./docs are updated. Should I include them in this PR?

Update:

npm commands are now working properly for Node 16
npm i will completely update the package.json
npm run test:unit is now working properly with no errors.
npm run storybook:build will update the contents in docs folder.
Should I include all of those changes in this PR?

severinbeauvais commented 2 years ago

npm i and npm run storybook:build both failed for Node 16.15.0. I changed Node to v12.22.12, and both commands succeeded.

npm run storybook succeeded for Node v12.22.12 but not for 16.15.0.

npm run test:unit failed while running the tests for Node v12.22.12, and there are missing components.

Contents in ./docs are updated. Should I include them in this PR?

You should be using Node v12 as per the readme:

Update:

npm commands are now working properly for Node 16

npm i will completely update the package.json

npm run test:unit is now working properly with no errors.

npm run storybook:build will update the contents in docs folder.

Should I include all of those changes in this PR?

I believe all steps do not work correctly under Node v16. Do not commit the updated package-lock.json file.

Yes, you should commit the Storybook changes (in docs folder).

Try running the unit tests for only your component. If they don't work then please fix them or create a tech debt ticket to fix them later. I am not concerned about the tests for the other components at this time.

severinbeauvais commented 2 years ago

@lewischenstudio How did you get past this?

severinbeauvais commented 2 years ago

If we can get everything working under Node 16 then let's update the package-lock.json file, the README.txt file, and the Storybook files. But it's not working on my workstation.

lewischenstudio commented 2 years ago

If we can get everything working under Node 16 then let's update the package-lock.json file, the README.txt file, and the Storybook files. But it's not working on my workstation.

I first used Node 12 then tried with Node 16. Maybe some of the packages were installed prior to using Node 16. I will try again with Node 12, but I should expect npm i succeed with Node 12.

lewischenstudio commented 2 years ago

@lewischenstudio How did you get past this?

I didn't get past this with Node 16 for the first time. After I used Node 12 and then tried this with Node 16, I passed everything on npm i.

severinbeauvais commented 2 years ago

I didn't get past this with Node 16 for the first time. After I used Node 12 and then tried this with Node 16, I passed everything on npm i.

Do you have reproducible steps that work on all actions in a new (clean) branch? If so then we can update the readme etc. That would be nice :)

lewischenstudio commented 2 years ago

I didn't get past this with Node 16 for the first time. After I used Node 12 and then tried this with Node 16, I passed everything on npm i.

Do you have reproducible steps that work on all actions in a new (clean) branch? If so then we can update the readme etc. That would be nice :)

I don't have the reproducible steps, as I am not sure what really happened. npm i failed for Node 16 first. I then installed Node 12 with nvm install 12.22.12 in one terminal. I was using VS Code. I then switched to another terminal, which was using the Node 16 on startup. Then I did npm i on this terminal with Node 16, and the installation passed. But the problem is that npm i with Node 16 will completely change the package-lock.json file.

severinbeauvais commented 2 years ago

I used nvm to change to Node 12, checked out this PR locally, and all the actions succeeded, ie:

cd repos
cd bcrs-shared-components
nvm use 12
gh pr checkout 140
npm i
lerna bootstrap --hoist
npm run test:unit CompletingParty
npm run storybook:build
npm run storybook

Please follow the readme file. Do not commit the package-log.json file. Do commit the changed docs files (there should only be a few of them).

lewischenstudio commented 2 years ago

I used nvm to change to Node 12, checked out this PR locally, and all the actions succeeded, ie:
cd repos
cd bcrs-shared-components
nvm use 12
gh pr checkout 140
npm i
lerna bootstrap --hoist
npm run test:unit CompletingParty
npm run storybook:build
npm run storybook
Please follow the readme file. Do not commit the package-log.json file. Do commit the changed docs files (there should only be a few of them).

That's what I did for the latest commit. It uses Node 12 and there is no change in the package-lock.json file. The commit includes files from the docs folder, the file to change the middle name rules and the unit test file.

severinbeauvais commented 2 years ago

I used nvm to change to Node 12, checked out this PR locally, and all the actions succeeded, ie:
cd repos
cd bcrs-shared-components
nvm use 12
gh pr checkout 140
npm i
lerna bootstrap --hoist
npm run test:unit CompletingParty
npm run storybook:build
npm run storybook
Please follow the readme file. Do not commit the package-log.json file. Do commit the changed docs files (there should only be a few of them).

You have too many changed files for Storybook. I suspect you built Storybook with Node 14. You have to set your environment to Node 12 first and then do the actions as in the readme.

Your PR should look like this (which I did following the instructions): https://github.com/bcgov/bcrs-shared-components/pull/141

lewischenstudio commented 2 years ago

I used nvm to change to Node 12, checked out this PR locally, and all the actions succeeded, ie:
cd repos
cd bcrs-shared-components
nvm use 12
gh pr checkout 140
npm i
lerna bootstrap --hoist
npm run test:unit CompletingParty
npm run storybook:build
npm run storybook
Please follow the readme file. Do not commit the package-log.json file. Do commit the changed docs files (there should only be a few of them).
You have too many changed files for Storybook. I suspect you built Storybook with Node 14. You have to set your environment to Node 12 first and then do the actions as in the readme.

Your PR should look like this (which I did following the instructions): #141

I followed the steps exactly and npm run storybook:build updated the "docs" folder with these files. Which version of Node 12 are you using? The README doesn't specify, and I'm using the latest Node vesrion: 12.22.12.

severinbeauvais commented 2 years ago

I followed the steps exactly and npm run storybook:build updated the "docs" folder with these files. Which version of Node 12 are you using? The README doesn't specify, and I'm using the latest Node vesrion: 12.22.12.

severin@DESKTOP-2XZPXD3:~/repos/bcrs-shared-components$ nvm use 12
Now using node v12.22.8 (npm v6.14.15)

You may have some leftover node modules from your v16 attempts. Try deleting the node_modules folder and then do npm i.

Also, if you use nvm and you set the node version in one terminal window, another terminal window may be using a different version. For this reason I usually work in a single terminal window at a time.

severinbeauvais commented 2 years ago

I followed the steps exactly and npm run storybook:build updated the "docs" folder with these files. Which version of Node 12 are you using? The README doesn't specify, and I'm using the latest Node vesrion: 12.22.12.

Another thought: your working changed files might be trying to revert the changes you've already committed. Try committing your current changes and then checking this PR for the actual changed files.

lewischenstudio commented 2 years ago

I followed the steps exactly and npm run storybook:build updated the "docs" folder with these files. Which version of Node 12 are you using? The README doesn't specify, and I'm using the latest Node vesrion: 12.22.12.
severin@DESKTOP-2XZPXD3:~/repos/bcrs-shared-components$ nvm use 12
Now using node v12.22.8 (npm v6.14.15)
You may have some leftover node modules from your v16 attempts. Try deleting the node_modules folder and then do npm i.

Also, if you use nvm and you set the node version in one terminal window, another terminal window may be using a different version. For this reason I usually work in a single terminal window at a time.

No luck here. I've tried everything, including v12.22.8. npm run storybook:build always updates many files in the "docs" folder.

lewischenstudio commented 2 years ago

I think this PR is blocked from my side. Can you merge your PR instead?

severinbeauvais commented 2 years ago

I think this PR is blocked from my side. Can you merge your PR instead?

Sure. Please close this one.

lewischenstudio commented 2 years ago

I think this PR is blocked from my side. Can you merge your PR instead?

Sure. Please close this one.

Thanks 👍

bcgov / bcrs-shared-components

13058 COLIN API failure - Front and Backend Change validation of the first name field in Coop IA UI and/or Corp Party table #140