Skip to content
This repository has been archived by the owner on Jul 13, 2023. It is now read-only.

bug: Handle legacy simplepush records as candidate webpush records #1034

Merged
merged 1 commit into from
Oct 4, 2017

Conversation

jrconlin
Copy link
Member

@jrconlin jrconlin commented Oct 3, 2017

Some older records in the database appear to have been miscategorized as
"simplepush". We should try to handle those as "webpush".

Closes #1033

Some older records in the database appear to have been miscategorized as
"simplepush". We should try to handle those as "webpush".

Closes #1033
@jrconlin jrconlin requested review from pjenvey and bbangert and removed request for pjenvey October 3, 2017 23:49
@codecov-io
Copy link

codecov-io commented Oct 4, 2017

Codecov Report

Merging #1034 into master will increase coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1034      +/-   ##
==========================================
+ Coverage   99.76%   99.76%   +<.01%     
==========================================
  Files          55       55              
  Lines        8799     8808       +9     
==========================================
+ Hits         8778     8787       +9     
  Misses         21       21
Impacted Files Coverage Δ
autopush/tests/test_endpoint.py 100% <ø> (ø) ⬆️
autopush/web/registration.py 100% <100%> (ø) ⬆️
autopush/web/webpush.py 100% <100%> (ø) ⬆️
autopush/tests/test_integration.py 100% <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 a9a1330...a0c7dfa. Read the comment docs.

Copy link
Member

@pjenvey pjenvey left a comment

Choose a reason for hiding this comment

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

Why not just reject these? Isn't that what previously occurred?

61b5752#diff-ccbbd52ff880cf4ddb71b18d126d189cL89

@bbangert
Copy link
Member

bbangert commented Oct 4, 2017

I think we shouldn't reject them because the client thinks they're valid. We don't actually verify that router_type is correct in websocket.py. We probably should verify that and force a reset if the router_type is wrong.

@jrconlin
Copy link
Member Author

jrconlin commented Oct 4, 2017

These seem to be edge cases. I'm not exactly sure how they got their route_type set to simplepush since they have a /wpush url. Passing them on as a webpush is fairly harmless since any additional issues would be caught by the appropriate checks later.

Eventually, when we start enforcing the global TTLs we can drop this patch.

@jrconlin jrconlin merged commit 4b9a273 into master Oct 4, 2017
@jrconlin jrconlin deleted the bug/1033 branch October 9, 2017 16:02
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants