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

Add master-key-only option when saving config parameter #910

Merged

Conversation

mtrezza
Copy link
Member

@mtrezza mtrezza commented Sep 5, 2019

This allows to save a config parameter that can be read with master key only using Parse.Config.save(parameters, masterKeyOnly).

An additional argument can be added to specify which of the parameters should be retrievable with master key only, for example:

Parse.Config.save(
    { internal: "i", public: "p" },
    { internal: true }
 );

A config parameter is retrievable without master key if it is not added in the masterKeyOnly argument or is added with value false.

@codecov
Copy link

codecov bot commented Sep 5, 2019

Codecov Report

Merging #910 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #910   +/-   ##
=======================================
  Coverage   91.95%   91.95%           
=======================================
  Files          54       54           
  Lines        5045     5045           
  Branches     1133     1133           
=======================================
  Hits         4639     4639           
  Misses        406      406
Impacted Files Coverage Δ
src/ParseConfig.js 87.95% <100%> (ø) ⬆️

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 2fc8da0...1ca0162. Read the comment docs.

@mtrezza
Copy link
Member Author

mtrezza commented Sep 5, 2019

The code coverage diff seems to be a false positive? The integration tests seem to cover all the changed code.

Copy link
Member

@dplewis dplewis left a comment

Choose a reason for hiding this comment

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

I have a few comments and nits

src/ParseConfig.js Outdated Show resolved Hide resolved
src/ParseConfig.js Outdated Show resolved Hide resolved
src/ParseConfig.js Outdated Show resolved Hide resolved
@dplewis
Copy link
Member

dplewis commented Sep 5, 2019

@mtrezza The code coverage here refers to the unit tests and not the integration tests.

package-lock.json Outdated Show resolved Hide resolved
@mtrezza
Copy link
Member Author

mtrezza commented Sep 6, 2019

What should I do about the decreased coverage? Should I add unit test even though the integration tests cover it already?

Correction: Coverage shows no diff anymore.

@mtrezza mtrezza changed the title Save master key only config parameter Add master-key-only option when saving config parameter Sep 6, 2019
@dplewis
Copy link
Member

dplewis commented Sep 6, 2019

Even tho the coverage is a 100% now. A few unit test is recommended

Add tests here https://github.com/parse-community/Parse-SDK-JS/blob/master/src/__tests__/ParseConfig-test.js#L103

You can check out the ParseObject-test.js

Since you are passing in parameters, you want to check that those parameters get passed into the restController

@mtrezza
Copy link
Member Author

mtrezza commented Sep 6, 2019

Since you are passing in parameters, you want to check that those parameters get passed into the restController

How would I do this, like this?

it('can save a config object with a parameter that be retrieved with masterkey only', async () => {
    ParseConfig.save(
      {internal: 'i', number: 12},
      {internal: true});
    expect(CoreManager.getRESTController().request.mock.calls[0]).toEqual([
      'PUT',
      'config',
      { params: {internal: 'i', number: 12}, masterKeyOnly: {internal: true} },
      { useMasterKey: true }]);
  });

@dplewis
Copy link
Member

dplewis commented Sep 6, 2019

There are a few ways, you can do it. (Don't forget to remove fit)

fit('can get a config object with master key', async () => {
    CoreManager.setRESTController({
      request(method, path, body, options) {
        expect(method).toBe('GET');
        expect(path).toBe('config');
        expect(body).toEqual({});
        expect(options).toEqual({ useMasterKey: true });
        return Promise.resolve({
          params: {
            str: 'hello',
            num: 45
          }
        });
      },
      ajax() {}
    });
    const config = await ParseConfig.get({ useMasterKey: true });
    expect(config.get('str')).toBe('hello');
    expect(config.get('num')).toBe(45);
    const path = Storage.generatePath('currentConfig');
    expect(JSON.parse(Storage.getItem(path))).toEqual({
      str: 'hello',
      num: 45,
    });
  });

@mtrezza
Copy link
Member Author

mtrezza commented Sep 6, 2019

The unit test I added fails because encode results in undefined for all values:

for(const key in attrs){
encodedAttrs[key] = encode(attrs[key])
}

Strange, because that logic was not changed in this PR.

@mtrezza
Copy link
Member Author

mtrezza commented Sep 6, 2019

Amazing! Thanks @dplewis

@dplewis dplewis merged commit b37d687 into parse-community:master Sep 6, 2019
@mtrezza
Copy link
Member Author

mtrezza commented Sep 6, 2019

@dplewis Could you do a release so we can close parse-community/parse-server#5930?

@mtrezza mtrezza deleted the save-master-key-only-config-parameter branch September 6, 2019 12:51
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.

2 participants