-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
feat: workspace health target column map fix #3932
feat: workspace health target column map fix #3932
Conversation
7baec25
to
1127315
Compare
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.
@magrinj I think the refactoring makes sense.
I've tested and these are my feedbacks:
- default-value doesn't seem to work anymore. I've removed the defaultValue on a text field and it was not detected
- regarding targetColumnMap, the following cases are not handled:
- wrong columnName { "value": "_columnId" } instead of { "value": "columnId" } + rename the column if the column is badly named
- wrong columnName { "value": "sdlkjfls" } instead of { "value": "columnId" } + rename the column in postgres schema if the column is badly named
- targetColumnMap of RELATION should be {}
- targetColumnMap is {"toto": "companyId"}
Is there a way to easily compute the right targetColumnMap from fieldMetadata. If it's too complex, maybe we should not invest too much here as we are going to deprecated targetColumnMap.
In this case, just fix default-value and we can merge. If you see an "easy" way of computing it and fixing it, let's go with that.
Also, fixing targetColumnMap might actually create more "MISSING_COLUMN" cases, as we don't migrate columns but I think it's fine, we can manually fix those, they should be pretty rare actually
packages/twenty-server/src/workspace/workspace-health/services/workspace-fix.service.ts
Outdated
Show resolved
Hide resolved
1127315
to
4baa7f3
Compare
3749b95
to
0aeb022
Compare
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.
Tested, it works well!
This PR is implementing the target-column-map kind in the --fix option of workspace:health command.
The core was created here #3863
Fix #3844