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

Sets the defaultSchemas keys in the SchemaCollection #1421

Merged
merged 5 commits into from
Apr 8, 2016

Conversation

flovilmart
Copy link
Contributor

fixes #1388

@@ -1,5 +1,6 @@

import MongoCollection from './MongoCollection';
import { defaultColumns } from '../../../Schema';
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not really happy with that circular dependency, nor the fact that the adapter depends upon parse-server... but otherwise that would defer the responsibility to the router (which is not good). Solution would be to introduce a SchemaController

@codecov-io
Copy link

Current coverage is 92.80%

Merging #1421 into master will increase coverage by +0.01% as of ba3c20c

@@            master   #1421   diff @@
======================================
  Files           86      86       
  Stmts         5441    5449     +8
  Branches      1004    1005     +1
  Methods          0       0       
======================================
+ Hit           5049    5057     +8
  Partial         10      10       
  Missed         382     382       

Review entire Coverage Diff as of ba3c20c

Powered by Codecov. Updated on successful CI builds.

@flovilmart
Copy link
Contributor Author

Put it into the router as it acts as a controller in some ways...

@facebook-github-bot
Copy link

@flovilmart updated the pull request.

@@ -14,16 +14,34 @@ function classNameMismatchResponse(bodyClass, pathClass) {
);
}

function injectDefaultSchema(schema) {
if (Array.isArray(schema)) {
let schemas = schema.map((s) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rather than have a function that accepts a schema or an array of schemas, can we have one that only accepts a single schema, and do the map outside the function if it needs it?

@drew-gross
Copy link
Contributor

Does this test fail if your changes aren't included?

@flovilmart
Copy link
Contributor Author

The test don't fail :/ ... probably because the full schema is set when you explicitly create it... also, just doing a GET won't work as the schema is not found...

@drew-gross
Copy link
Contributor

Dang. Do you think it would be too much work to make a test that creates the legacy data by digging around on Mongo directly?

@flovilmart
Copy link
Contributor Author

Yeah I should probably write a bad schema directly to mongo...

@facebook-github-bot
Copy link

@flovilmart updated the pull request.

function getAllSchemas(req) {
return req.config.database.schemaCollection()
.then(collection => collection.getAllSchemas())
.then(schemas => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to be more concise, you could do

then(schemas => schemas.map(injectDefaultSchema))

Up to you though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that's really valid...

@flovilmart
Copy link
Contributor Author

Pushing the consisensee thing and squashing after

@facebook-github-bot
Copy link

@flovilmart updated the pull request.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants