Skip to content
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

Merged
merged 9 commits into from
Jan 7, 2021

Conversation

avinashlng1080
Copy link
Contributor

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

@avinashlng1080 avinashlng1080 added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Dec 29, 2020
@avinashlng1080 avinashlng1080 marked this pull request as draft December 29, 2020 20:04
@avinashlng1080 avinashlng1080 marked this pull request as ready for review December 29, 2020 20:20
Copy link
Contributor

@enahum enahum left a 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.

app/database/server/models/custom_emoji.ts Show resolved Hide resolved
app/database/server/models/role.ts Outdated Show resolved Hide resolved
app/database/server/models/system.ts Outdated Show resolved Hide resolved
types/database/role.d.ts Outdated Show resolved Hide resolved
types/database/system.d.ts Outdated Show resolved Hide resolved
@avinashlng1080
Copy link
Contributor Author

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.

@avinashlng1080 avinashlng1080 requested a review from enahum January 5, 2021 14:56
@enahum enahum requested a review from josephbaylon January 6, 2021 21:42
@enahum
Copy link
Contributor

enahum commented Jan 6, 2021

@josephbaylon can you please review the tests for this PR?

Copy link
Contributor

@migbot migbot left a 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: [
Copy link
Contributor

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?

Copy link
Contributor

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.

Copy link
Contributor Author

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.

@migbot migbot removed the 2: Dev Review Requires review by a core commiter label Jan 6, 2021
@josephbaylon josephbaylon added the Build Apps for PR Build the mobile app for iOS and Android to test label Jan 6, 2021
@mattermod mattermod removed the Build Apps for PR Build the mobile app for iOS and Android to test label Jan 6, 2021
@mattermod
Copy link
Contributor

Building app in separate branch.

@mattermod
Copy link
Contributor

@josephbaylon
Copy link
Contributor

@avinashlng1080 @enahum Looks like app building is failing

@enahum
Copy link
Contributor

enahum commented Jan 6, 2021

@josephbaylon we should not attempt to build v2 at this time

Copy link
Contributor

@josephbaylon josephbaylon left a 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.

@josephbaylon josephbaylon added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Jan 6, 2021
@avinashlng1080 avinashlng1080 merged commit 3e8c275 into v2 Jan 7, 2021
@avinashlng1080 avinashlng1080 deleted the MM_30476_part_isolated_models branch January 7, 2021 09:48
@josephbaylon josephbaylon changed the title MM_30476 [v2] Added all isolated tables from the server schema MM-30476 [v2] Added all isolated tables from the server schema Apr 2, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4: Reviews Complete All reviewers have approved the pull request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants