Skip to content

Conversation

jdposthuma
Copy link
Contributor

@jdposthuma jdposthuma commented Jun 7, 2021

New Pull Request Checklist

Issue Description

This PR fixes the open issue.

Related issue: #7416

Approach

Simply added a truthy check

TODOs before merging

  • Add test cases
  • Add entry to changelog

@dplewis
Copy link
Member

dplewis commented Jun 8, 2021

@jdposthuma Thanks for the contribution. Can you go though the ToDo list? Add a test case, update the change log, etc?

@jdposthuma
Copy link
Contributor Author

@jdposthuma Thanks for the contribution. Can you go though the ToDo list? Add a test case, update the change log, etc?

@dplewis - Done. I've added to the changelog.

None of the other items seemed valid:

  • Add test cases

I haven't been able to capture (or simulate) the failing class

  • Add changes to documentation (guides, repository pages, in-code descriptions)

Too insignificant to add to docs

  • Add security check

Doesn't change security

  • Add new Parse Error codes to Parse JS SDK

No new parse error code

@mtrezza
Copy link
Member

mtrezza commented Jun 9, 2021

Except for the test case, you can delete the non-applicable TODOs from the list.

The change makes sense, even if there wasn't a reported issue, just for code quality. However, it would be good to have a test case, to make your fix sustainable and prevent a future change from reverting your fix by mistake. And to verify that it solved the issue. Maybe you can give it another try?

I classified this as bug of low severity due to the missing test case verification.

@mtrezza mtrezza added type:bug Impaired feature or lacking behavior that is likely assumed S4 labels Jun 9, 2021
@codecov
Copy link

codecov bot commented Jun 9, 2021

Codecov Report

Merging #7421 (213e98e) into master (129f7bf) will decrease coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 213e98e differs from pull request most recent head 94d387f. Consider uploading reports for the commit 94d387f to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #7421      +/-   ##
==========================================
- Coverage   93.92%   93.92%   -0.01%     
==========================================
  Files         181      181              
  Lines       13245    13247       +2     
==========================================
+ Hits        12441    12442       +1     
- Misses        804      805       +1     
Impacted Files Coverage Δ
src/LiveQuery/QueryTools.js 94.73% <100.00%> (+0.05%) ⬆️
src/batch.js 91.37% <0.00%> (-1.73%) ⬇️
src/Adapters/Files/GridFSBucketAdapter.js 79.50% <0.00%> (-0.82%) ⬇️
src/RestWrite.js 93.76% <0.00%> (-0.16%) ⬇️
src/Adapters/Storage/Mongo/MongoStorageAdapter.js 93.02% <0.00%> (+0.43%) ⬆️

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 129f7bf...94d387f. Read the comment docs.

@mtrezza
Copy link
Member

mtrezza commented Jun 11, 2021

@jdposthuma Can you please rebase on master?

@jdposthuma
Copy link
Contributor Author

@jdposthuma Can you please rebase on master?

Done

@jdposthuma
Copy link
Contributor Author

@mtrezza - Test case added.

I'm really glad you pushed for it because my original fix had a logic problem. 🙏

@jdposthuma
Copy link
Contributor Author

Hi @mtrezza - do I need to do anything else to get this to merge?

Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Just the changelog entry

Copy link
Member

@mtrezza mtrezza left a comment

Choose a reason for hiding this comment

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

Looks good to me, thanks for this PR!

@mtrezza mtrezza requested a review from dplewis June 21, 2021 20:56
Copy link
Member

@davimacedo davimacedo left a comment

Choose a reason for hiding this comment

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

LGTM!

@davimacedo davimacedo merged commit d8dc524 into parse-community:master Jun 21, 2021
@jdposthuma
Copy link
Contributor Author

Thanks guys! When's the next release planned?

@mtrezza
Copy link
Member

mtrezza commented Jun 23, 2021

See #7271 (comment)

@mtrezza mtrezza added type:bug Impaired feature or lacking behavior that is likely assumed and removed type:bug Impaired feature or lacking behavior that is likely assumed S4 labels Jul 11, 2021
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0-beta.1

@parseplatformorg parseplatformorg added the state:released-beta Released as beta version label Nov 1, 2021
@mtrezza mtrezza mentioned this pull request Mar 12, 2022
@parseplatformorg
Copy link
Contributor

🎉 This change has been released in version 5.0.0

@parseplatformorg parseplatformorg added the state:released Released as stable version label Mar 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

state:released Released as stable version state:released-beta Released as beta version

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants