Skip to content

Added Fixes for _PushStatus _SCHEMA Insertion and Ensuring Order for _PushStatus Row Update #2375

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

Closed
wants to merge 1 commit into from

Conversation

sprabs
Copy link

@sprabs sprabs commented Jul 24, 2016

_PushStatus sometimes gets injected to the _SCHEMA collection, causing:

  1. Parse Dashboard through parse.com to stop working.
  2. Push notifications sent through parse.com to stop working.

These code changes also ensure that the _PushStatus update during push notification processing is correct (first from 'pending' to 'running' and only then from 'running' to 'succeeded’).

@codecov-io
Copy link

codecov-io commented Jul 24, 2016

Current coverage is 91.84% (diff: 95.00%)

Merging #2375 into master will increase coverage by <.01%

@@             master      #2375   diff @@
==========================================
  Files            95         95          
  Lines         10708      10723    +15   
  Methods        1309       1312     +3   
  Messages          0          0          
  Branches       1741       1741          
==========================================
+ Hits           9835       9849    +14   
- Misses          873        874     +1   
  Partials          0          0          

Powered by Codecov. Last update fa49f05...2696b8b

@madkid66
Copy link

@sprabs, We were running into the same issue. The fix suggested here seems to help after testing extensively by sending several push notifications; Thank you.

@flovilmart, can you please look at the changes suggested here and comment; Thank you.

@sprabs
Copy link
Author

sprabs commented Jul 27, 2016

@flovilmart I wanted to follow up with you to see if you had any suggestions. This fix is in our repo and has been working well for almost a week now. Looks like others have tried it out, too.

Happy to address any questions you have. I'd like to merge it to master soon so that my repo isn't diverged.

@flovilmart
Copy link
Contributor

The ordering problem should already be taken care of by he push status handler since 2.2.17. Can you confirm the problem still exist with master without the patch? I wanted to avoid blocking as you did.

@sprabs
Copy link
Author

sprabs commented Jul 27, 2016

@flovilmart Sure, I'll try that.

However, I want to convey that there are two issues that are addressed in this pr. Even if you handle the ordering properly, the schema injection could potentially still happen (this is covered by my fix in SchemaController.js).

@flovilmart
Copy link
Contributor

@sprabs can you confirm or infirm that 2.2.17 is still broken?

@flovilmart
Copy link
Contributor

@sprabs ping?

@sprabs
Copy link
Author

sprabs commented Aug 12, 2016

@flovilmart Sorry for the delay. We wanted to test it extensively. After reverting my changes and pulling the latest, I think we can confirm that your fix also fixes this particular issue we saw. Thanks for adding the fix!

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.

4 participants