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

Make GlobalConfig work like parse.com #1210

Merged
merged 4 commits into from
Mar 27, 2016
Merged

Conversation

framp
Copy link
Contributor

@framp framp commented Mar 27, 2016

As per title:

  • GlobalConfig will merge the existing values with the new ones
  • The new values with { "__op": "Delete" } will be deleted
  • When PARSE_EXPERIMENTAL_CONFIG_ENABLED or TESTING the server features route will return config, making it work properly with parse-dashboard

A lot of tests are failing with npm test, not sure if it's an issue with my setup (Arch Linux, already have mongodb but it's not running) or with the project. For the time being I'll rely on whatever circleci you guys are using.

npm test spec/ParseGlobalConfig.spec.js seems fine.

@codecov-io
Copy link

Current coverage is 93.02%

Merging #1210 into master will not affect coverage as of c9898c0

@@            master   #1210   diff @@
======================================
  Files           83      83       
  Stmts         5278    5287     +9
  Branches       963     967     +4
  Methods          0       0       
======================================
+ Hit           4910    4918     +8
  Partial         10      10       
- Missed         358     359     +1

Review entire Coverage Diff as of c9898c0

Powered by Codecov. Updated on successful CI builds.

.then(() => ({ response: { result: true } }));
.then(coll => coll.find({ '_id': 1 }, { limit: 1 }))
.then(results => {
const previousConfig = results && results[0] && results[0].params || {};
Copy link
Contributor

Choose a reason for hiding this comment

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

You should do it in one op, and use $unset when op delete is set

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@flovilmart:
The main problem is that I can't merge nested documents with $set afaik.
The whole params object get replaced with the new one.

{"params": {"a":"b"}}
In mongodb:
{"params": {"a":"a", "b":"b"}}
Result:
{"params": {"a":"b"}}
Expected:
{"params": {"a":"b", "b": "b"}}

Copy link
Contributor

Choose a reason for hiding this comment

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

Shoot... It's been poorly designed... Can you try with dot notation? I believe that works also $unset supports dot notation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agree. Dot notation works and some params manipulation works, awesome!

@facebook-github-bot
Copy link

@framp updated the pull request.

@@ -18,20 +18,20 @@ export class GlobalConfigRouter extends PromiseRouter {
}

updateGlobalConfig(req) {
const params = req.body.params;
const update = {};
Object.keys(params).forEach((key) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use reduce instead of out of block scope assignment? Initial value with {$set:{}, $unset:{}}?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, but mongo is complaining if $set or $unset are empty objects

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, so empty initial. I prefer avoiding assigning on out of blocks objects

@facebook-github-bot
Copy link

@framp updated the pull request.

@flovilmart
Copy link
Contributor

Sorry for the pickiness :) but that's great ! Thanks!!!!

@framp
Copy link
Contributor Author

framp commented Mar 27, 2016

Glad to help and thanks for the review!

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

Successfully merging this pull request may close these issues.

None yet

4 participants