Skip to content

fix: Handle increment on nested fields deep level #1301

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

Merged
merged 2 commits into from
Feb 23, 2021

Conversation

sadortun
Copy link
Contributor

Fix #1299

@codecov
Copy link

codecov bot commented Feb 17, 2021

Codecov Report

Merging #1301 (a2b78c7) into master (10002de) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##            master     #1301   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           60        60           
  Lines         5823      5837   +14     
  Branches      1306      1312    +6     
=========================================
+ Hits          5823      5837   +14     
Impacted Files Coverage Δ
src/ObjectStateMutations.js 100.00% <100.00%> (ø)
src/Push.js 100.00% <100.00%> (ø)
src/RESTController.js 100.00% <100.00%> (ø)

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 e0a4da2...ff88610. Read the comment docs.

@davimacedo
Copy link
Member

@sadortun thanks for the PR. Could you please add a test case?

@sadortun
Copy link
Contributor Author

@davimacedo Here you go ! Merry Christmas :) !

Please ensure you check/test for other possible side effects :) I'm new to the wonderful world of Parse and i would really like to avoid destroying thousands of client apps :) hehehe

@dplewis
Copy link
Member

dplewis commented Feb 19, 2021

@sadortun Can you add an integration test in integration/test/ParseObjectTest.js

I wrote a test and it fails. I think this is what you were trying to fix no? If not write a test that closely represent this issue. The Contribution Guide will show you how to run integration test and make sure to run npm run watch:node

fit('can increment nested on undefined field', async () => {
    const obj = new TestObject();
    obj.increment('a.b.c.d');
    await obj.save();

    console.log(obj.get('a')); // undefined

    const query = new Parse.Query(TestObject);
    const result = await query.get(obj.id);
    console.log(result.get('a')); // undefined
  });

@sadortun
Copy link
Contributor Author

@dplewis thanks for the hint, I'll look at it!

@dplewis
Copy link
Member

dplewis commented Feb 19, 2021

@sadortun The following tests pass with your fix.

it('can increment nested four levels', async () => {
    const obj = new TestObject({ a: { b: { c: { d: 1 } } } });
    await obj.save();
    obj.increment('a.b.c.d');
    console.log(obj.get('a').b.c.d); // 2

    await obj.save();
    console.log(obj.get('a').b.c.d); // 2

    const query = new Parse.Query(TestObject);
    const result = await query.get(obj.id);
    console.log(result.get('a').b.c.d); // 2
  });

The failing test I posted is a separate issue, its failing because serverData doesn't exist when you increment. This is caused by: https://github.com/sadortun/Parse-SDK-JS/blob/patch-1/src/ParseObject.js#L744

To fix you need to remove that and do an update on the server side.

For your fix, can you add the passing test I posted and we can merge this in. We can do a separate issue for the bug I found.

@sadortun
Copy link
Contributor Author

Thanks @dplewis ill get back to you later today or early tomorow

@sadortun
Copy link
Contributor Author

Hi @dplewis Took some time, but i'm back ! I added the test.

Also FYI, seems like mustache is required but not in devDependencies

I should also probably add/exclude your failing test from #1301 (comment) to ensure it's documented and get fixed :) Let me know!

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.

LGTM! I'm going to open a PR for that other issue I pointed out.

Also FYI, seems like mustache is required but not in devDependencies

I don't know what that is.

@dplewis dplewis merged commit 1abb841 into parse-community:master Feb 23, 2021
@dplewis dplewis changed the title When traversing fields, ensure we create them as we go to avoid acces… fix: Handle increment on nested fields deep level Feb 23, 2021
@sadortun
Copy link
Contributor Author

Awesome! Any idea when this (and the other PR) will be available in a release ?

Thanks a lot you guys are awesome!

@dplewis
Copy link
Member

dplewis commented Feb 23, 2021

Thanks we try to do a release every 2 weeks. You can use the master branch if you need it.

https://github.com/parse-community/Parse-SDK-JS#want-to-ride-the-bleeding-edge

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.

How to increment a nested object field ?
3 participants