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

Deletes publisher-data.json when wallet is deleted #14759

Merged
merged 1 commit into from
Aug 10, 2018

Conversation

ryanml
Copy link
Contributor

@ryanml ryanml commented Jul 16, 2018

Fixes #14758

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.
  • Request a security/privacy review as needed. (Ask a Brave employee to help if you cannot access this document.)

Test Plan:

  1. Run Brave with a clean profile from this branch
  2. Enable Payments
  3. Wait ~15 seconds and ensure that publisher-data.json is present in the user profile folder
  4. Delete Wallet
  5. Ensure that publisher-data.json and ledger-state.json are deleted.
  6. Re-enable payments
  7. Wait ~15 seconds and ensure that publisher-data.json is generated again

Reviewer Checklist:

  • Request a security/privacy review as needed if one was not already requested.

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

@jasonrsadler
Copy link
Contributor

If we delete wallet and then re enable, do we need to get the publisher-data back?

@ryanml
Copy link
Contributor Author

ryanml commented Jul 18, 2018

@jasonrsadler re enabling payments will recall initialize and the list will be fetched again

@NejcZdovc
Copy link
Contributor

@ryanml please add this to test plan as well

@ryanml
Copy link
Contributor Author

ryanml commented Jul 18, 2018

@NejcZdovc test plan updated

@NejcZdovc
Copy link
Contributor

@ryanml thank you

Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

7 step is failing for me. File is not generated. It's not created even if I restart the browser

@ryanml
Copy link
Contributor Author

ryanml commented Jul 18, 2018

@NejcZdovc should be fine to re-try now, small oversight :D

NejcZdovc
NejcZdovc previously approved these changes Jul 18, 2018
Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

++ will merge after rel 4 is released

Copy link
Contributor

@NejcZdovc NejcZdovc left a comment

Choose a reason for hiding this comment

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

Step 5 is failing for me

@ryanml
Copy link
Contributor Author

ryanml commented Jul 18, 2018

@NejcZdovc the test plan is working as expected for me. @jasonrsadler can you confirm?

@jasonrsadler
Copy link
Contributor

jasonrsadler commented Jul 18, 2018

Step 5 works for me but step 7 fails. publisher-data.json does not get recreated. Test plan works for me (edit for having to merge updates in)

@ryanml ryanml dismissed NejcZdovc’s stale review July 19, 2018 02:32

should be good to check again if you re-pull

@NejcZdovc
Copy link
Contributor

@ryanml please rebase

@ryanml
Copy link
Contributor Author

ryanml commented Jul 31, 2018

@NejcZdovc rebased

@bsclifton bsclifton merged commit 4888635 into brave:master Aug 10, 2018
bsclifton added a commit that referenced this pull request Aug 10, 2018
Deletes publisher-data.json when wallet is deleted
bsclifton added a commit that referenced this pull request Aug 10, 2018
Deletes publisher-data.json when wallet is deleted
@bsclifton
Copy link
Member

master 4888635
0.24.x 2d8d3af
0.23.x f0bc8c0

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.

4 participants