-
Notifications
You must be signed in to change notification settings - Fork 868
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 new purchase state OUT_OF_CREDENTIALS for VPN service #26272
base: master
Are you sure you want to change the base?
Conversation
A Storybook has been deployed to preview UI for the latest push |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be great to have unit test for OUT_OF_CREDENTIALS
state.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bsclifton i went through the changes, i think we should add an exception for android for out of credentials
state as we don't do anything about it in android and we need to know if user is linked or not.
Thanks @deeppandya! I checked and |
OK working on a test now... then it will be ready for a re-review |
The security team is monitoring all repositories for certain keywords. This PR includes the word(s) "login" and so security team members have been added as reviewers to take a look. |
@simonhong added test! The |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
++ 👍🏼
This handles the condition where the all the credentials for a given day are redeemed and there are none left. This shouldn't happen - but can happen if there are network issues. Specifically, when the client doesn't receive response from server after redeeming the credential. Fixes brave/brave-browser#33031
c61a963
to
76d0510
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Took a look through this and don't see anything of concern. @thypon mentioned on slack he's good with it too so approving and removing sec-review label
This handles the condition where the all the credentials for a given day are redeemed and there are none left. This shouldn't happen - but can happen if there are network issues. Specifically, when the client doesn't receive response from server after redeeming the credential.
Fixes brave/brave-browser#33031
Submitter Checklist:
QA/Yes
orQA/No
;release-notes/include
orrelease-notes/exclude
;OS/...
) to the associated issuenpm run test -- brave_browser_tests
,npm run test -- brave_unit_tests
wikinpm run presubmit
wiki,npm run gn_check
,npm run tslint
git rebase master
(if needed)Reviewer Checklist:
gn
After-merge Checklist:
changes has landed on
Test Plan:
Local State
file in a text editor of your choosingsku
>state
areaspent: true
). Find the second credential for that same day and changespent: false
tospent: true
. Now they should both be true - meaning you are out of credentials for the time window you're in (if code asks for new credentials).brave_vpn
>subscriber_credential
and setcredential
to empty string""
. This will force the next launch to try to get new credentials.