Skip to content
This repository has been archived by the owner on Dec 11, 2019. It is now read-only.

Adds some additional banners #12136

Merged
merged 1 commit into from
Nov 29, 2017
Merged

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Nov 29, 2017

This PR switches promotions to the production server!!

Please test this PR with LEDGER_ENVIRONMENT=staging npm run start, so that you will still be using staging.

Submitter Checklist:

  • Submitted a ticket for my issue if one did not already exist.
  • Used Github auto-closing keywords in the commit message.
  • Added/updated tests for this change (for new code or code which already has tests).
  • Ran git rebase -i to squash commits (if needed).
  • Tagged reviewers and labelled the pull request as needed.

Fixed recovery promotional removal
Removes staging flag
Fixes fake visits

Resolves #12131
Resolves #12098
Resolves #11394

Auditors:

Test Plan:

a) Regular promotion

  1. enable wallet
  2. wait for promotion to appear
  3. click on claim
  4. make sure that success message appears

b) Network problem

  1. enable wallet
  2. wait for promotion to appear
  3. disable internet
  4. click on claim
  5. make sure that general error message appears

Reviewer Checklist:

Tests

  • Adequate test coverage exists to prevent regressions
  • Tests should be independent and work correctly when run individually or as a suite ref
  • New files have MPL2 license header

@codecov-io
Copy link

codecov-io commented Nov 29, 2017

Codecov Report

Merging #12136 into master will decrease coverage by 0.1%.
The diff coverage is 28.81%.

@@            Coverage Diff             @@
##           master   #12136      +/-   ##
==========================================
- Coverage   55.03%   54.93%   -0.11%     
==========================================
  Files         275      275              
  Lines       26497    26549      +52     
  Branches     4258     4270      +12     
==========================================
+ Hits        14582    14584       +2     
- Misses      11915    11965      +50
Flag Coverage Δ
#unittest 54.93% <28.81%> (-0.11%) ⬇️
Impacted Files Coverage Δ
js/constants/appConstants.js 100% <ø> (ø) ⬆️
app/locale.js 35.23% <ø> (ø) ⬆️
js/actions/appActions.js 19.32% <0%> (-0.11%) ⬇️
...r/components/preferences/payment/enabledContent.js 75.24% <15.62%> (-10.64%) ⬇️
app/browser/reducers/ledgerReducer.js 48.04% <40%> (-0.51%) ⬇️
app/browser/api/ledger.js 49.26% <41.66%> (-0.08%) ⬇️
app/common/state/ledgerState.js 75.72% <71.42%> (-0.15%) ⬇️
js/stores/appStoreRenderer.js 91.17% <0%> (-8.83%) ⬇️
app/renderer/components/reduxComponent.js 84.37% <0%> (-6.25%) ⬇️
js/stores/windowStore.js 26.71% <0%> (-0.3%) ⬇️
... and 2 more

@srirambv
Copy link
Collaborator

The rest is easy doesn't have a line break before it. Is this expected?
image

@srirambv
Copy link
Collaborator

srirambv commented Nov 29, 2017

Getting error Error retrieving promotion SyntaxError: Unexpected token # in JSON at position 0 while launching the browser

Edit: works fine when run with LEDGER_ENVIRONMENT=staging npm run start

@LaurenWags
Copy link
Member

Looks good on MacOS

@kjozwiak
Copy link
Member

Resolves #12131
Resolves #12098
Resolves #11394

Checked these against Ubuntu... Everything worked as expected... Will test more thoroughly once this PR has been merged.

LaurenWags
LaurenWags previously approved these changes Nov 29, 2017
Copy link
Member

@LaurenWags LaurenWags left a comment

Choose a reason for hiding this comment

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

Tested utilizing the test plan specified and LEDGER_ENVIRONMENT=staging npm run start and tests were as expected.

dispatch({
actionType: appConstants.APP_ON_PROMOTION_RESPONSE
actionType: appConstants.APP_ON_PROMOTION_RESPONSE,
error
Copy link
Member

Choose a reason for hiding this comment

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

Are we wanting to call this error? It seems like this also contains the success response?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed

bsclifton
bsclifton previously approved these changes Nov 29, 2017
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Changes look great! 😄 Comment left about one field which I think could be named better. Otherwise 👍

Fixed recovery promotional removal
Removes staging flag
Fixes fake visits

Resolves brave#12131
Resolves brave#12098
Resolves brave#11394

Auditors:

Test Plan:
Copy link
Member

@bsclifton bsclifton left a comment

Choose a reason for hiding this comment

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

Code changes look great! 👍 Thanks for updating the response param name 😄

@bsclifton bsclifton merged commit 81f7991 into brave:master Nov 29, 2017
bsclifton added a commit that referenced this pull request Nov 29, 2017
bsclifton added a commit that referenced this pull request Nov 29, 2017
@bsclifton
Copy link
Member

master 81f7991
0.21.x 03e093e
0.20.x 204a094
0.19.x @NejcZdovc to help with this one...

NejcZdovc pushed a commit that referenced this pull request Nov 29, 2017
@NejcZdovc
Copy link
Contributor Author

0.19.x 23d2f5a

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants