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.

Sign up for GitHub

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

Jump to bottom

Save Method for Parse.Config #684

Merged
merged 6 commits into from
Nov 14, 2018
Merged

Save Method for Parse.Config #684

merged 6 commits into from
Nov 14, 2018

Conversation

Moumouls
Copy link
Member

@Moumouls Moumouls commented Oct 21, 2018 •
edited
Loading

Allow to use .save() on Parse.Config when a masterKey is defined to avoid the use of Parse._request #684

  • Write Method
  • Write Test
  • Valid the save() then get() strategy @flovilmart ?
  • Write Documentation Guide
  • Write Documentation API

@codecov
Copy link

codecov bot commented Oct 29, 2018 •
edited
Loading

Codecov Report

Merging #684 into master will increase coverage by 0.05%.
The diff coverage is 96.42%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #684      +/-   ##
==========================================
+ Coverage   85.71%   85.77%   +0.05%     
==========================================
  Files          48       48              
  Lines        3886     3902      +16     
  Branches      885      886       +1     
==========================================
+ Hits         3331     3347      +16     
  Misses        555      555
Impacted Files Coverage Δ
src/CoreManager.js 100% <100%> (ø) ⬆️
src/ParseConfig.js 87.5% <96.29%> (+3.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e0c51ab...71ecb71. Read the comment docs.

flovilmart
flovilmart suggested changes Nov 10, 2018
View reviewed changes
Copy link
Contributor

@flovilmart flovilmart left a comment

Choose a reason for hiding this comment

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

Thanks for the Pr, please address the changes and we’ll merge it!

src/ParseConfig.js Show resolved Hide resolved
flovilmart
flovilmart suggested changes Nov 12, 2018
View reviewed changes
Copy link
Contributor

@flovilmart flovilmart left a comment

Choose a reason for hiding this comment

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

Please remove the params options in save as it is of no use (the SDK will reject later when not using the master key).

Can you also add a test that ensures when calling save, the original values are not overridden on the server when not passed?

@Moumouls
Copy link
Member Author

Not passed values can't be overriden during the.save() according to the documentation : Parse Rest Guide Config
It's a Parse-Server test and not a Parse SDK test ?

flovilmart
flovilmart approved these changes Nov 12, 2018
View reviewed changes
src/__tests__/ParseConfig-test.js Outdated
@@ -142,16 +142,6 @@ describe('ParseConfig', () => {
});
});

it('cant save a config object without masterkey', (done) => {
ParseConfig.save({str: 'hello2','num':46}).then((config) => {
expect(config).toBe(-1)
Copy link
Contributor

Choose a reason for hiding this comment

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

THis should still fail.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do I have to re-commit this test ?

@Moumouls Moumouls mentioned this pull request Nov 12, 2018
Merged
@Moumouls
Copy link
Member Author

I just wrote the guide doc, the API doc is missing
PR Docs JS Guide Config

@Moumouls
Copy link
Member Author

What's missing here for merge ?

@flovilmart
Copy link
Contributor

Nothing, I just merged back master and was waiting for the CI to complete!

Thanks!

@flovilmart flovilmart merged commit 3162d52 into parse-community:master Nov 14, 2018
@dplewis dplewis mentioned this pull request Jan 16, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@flovilmart flovilmart flovilmart approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Moumouls @flovilmart