-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
MM-30476 [v2] Added all isolated tables from the server schema #5070
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.
You are also missing the id's in the model type definition.
As I mention in the community server, please add some tests to the PR, we should try and aim to test everything as long as is possible.
Hello @enahum, I did add some tests for the schema part. This ensure that the db structure strictly complies with the structure that we dictate. However, for the models, I went through it and I am not sure how to tackle it. I did add a test suite for the default models which I deemed most relevant out of the set of test suites from Nozbe repo. Over there, the test suites were ensuring that the library actually behaved as expected. But if we replicate the same test suites, we'll be duplicating this intent which is not what we want .... as in we'll be testing the Model class twice. Please let me know your thoughts on this. I think the schema tests are relevant to us and we can drop the tests for the Models. The CI broke because of the TS issue with the model's test. Some of the properties I'm accessing are not exposed to front end users. |
@josephbaylon can you please review the tests for 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.
LGTM, just a question below.
created_at: {name: 'created_at', type: 'number'}, | ||
version_number: {name: 'version_number', type: 'string'}, | ||
}, | ||
columnArray: [ |
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.
What is columnArray
used for?
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 is actually a very good question, as I was unable to see columnArray
in the wdb test examples.
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.
So when defining the 'schema' we make use of the 'appSchema' function which itself feeds upon the output of the function 'tableSchema'.
All in all, 'appSchema' will create an object that would resemble the below structure:
{
version: 1,
tables: {
CustomEmoji: {
name: 'CustomEmoji',
columns: [Object],
columnArray: [Array],
unsafeSql: undefined
},
Role: {
name: 'Role',
columns: [Object],
columnArray: [Array],
unsafeSql: undefined
},
System: {
name: 'System',
columns: [Object],
columnArray: [Array],
unsafeSql: undefined
},
TermsOfService: {
name: 'TermsOfService',
columns: [Object],
columnArray: [Array],
unsafeSql: undefined
}
},
unsafeSql: undefined
}
I got the columnArray
from the schema test from WDB.
Building app in separate branch. |
@avinashlng1080 @enahum Looks like app building is failing |
@josephbaylon we should not attempt to build v2 at this time |
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. Code changes look straightforward.
Summary
This PR is part of a bigger PR that caters for all the models in our database layer.
In here, we have broken down all the 'isolated' tables in the 'server schema' and added all their related files.
Ticket Link
https://mattermost.atlassian.net/browse/MM-30476