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

Add notification to suggest users use Brave Payments #4141

Merged
merged 2 commits into from
Sep 22, 2016

Conversation

ayumi
Copy link
Contributor

@ayumi ayumi commented Sep 21, 2016

  • 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).

Fix #3257 (Finishes the remaining task)

  • Record firstRunTimestamp into session-state
  • 10 days from firstRunTimestamp, show notification suggesting user tries Payments
  • If the user enables Payments they won't see the notification

Auditors: @bsclifton @diracdeltas

Test Plan:

  1. Edit ledger.js to reduce showNotifications delay interval to 3 * msecs.second
  2. Run Brave with an existing profile
  3. Confirm there's no notification
  4. Close Brave
  5. Update userData session-store-1 firstRunTimestamp to be way in the past e.g. from '14..' to '13..' (~/Library/Application\ Support/session-store-1)
  6. Open Brave
  7. Confirm notification

Auditors: @bsclifton

Test Plan:

Upgrading existing userData
1. Be on `master` and close Brave
2. Open session-store-1 with less (e.g. ~/Library/Application\ Support/brave-development/session-store-1)
3. See firstRunTimestamp is not there
4. Check out this branch
5. Open Brave, then close it
6. View again session-store-1 and see firstRunTimestamp is there
8. Restart Brave again, then close it
9. See firstRunTimestamp is see firstRunTimestamp is unchanged

New profile
1. Delete userData dir (e.g. ~/Library/Application\ Support/brave-development)
2. Open Brave
3. Close Brave
4. View session-store-1 and see firstRunTimestamp
@ayumi ayumi force-pushed the feature/first-run-timestamp branch from 04ca066 to ab440b2 Compare September 21, 2016 22:13
@@ -383,6 +391,9 @@ eventStore.addChangeListener(() => {
var initialize = (onoff) => {
enable(onoff)

// Check if relevant browser notifications should be shown every 15 minutes
setInterval(showNotifications, 15 * msecs.minute)
Copy link
Member

Choose a reason for hiding this comment

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

clearInterval never gets called on this, so i think a new timer is added every time the user clicks the on/off switch. this may lead to notifications being shown more often than intended or something

Copy link
Member

Choose a reason for hiding this comment

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

maybe add a global var to keep track of whether setInterval has already been called and do nothing here if it has

Copy link
Contributor Author

Choose a reason for hiding this comment

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

thanks good call– it was originally there but i removed it (oops).
i added it back

@diracdeltas
Copy link
Member

otherwise 👍

@ayumi ayumi force-pushed the feature/first-run-timestamp branch from ab440b2 to 47d7aaa Compare September 22, 2016 01:17
Auditors: @bsclifton @diracdeltas

Test Plan:
1. Edit `ledger.js` to reduce showNotifications delay interval to 3 * msecs.second
2. Run Brave with an existing profile
3. Confirm there's no notification
4. Close Brave
5. Update userData session-store-1 firstRunTimestamp to be way in the past e.g. from '14..' to '13..' (~/Library/Application\ Support/session-store-1)
6. Open Brave
7. Confirm notification
@ayumi ayumi force-pushed the feature/first-run-timestamp branch from 47d7aaa to 35971f5 Compare September 22, 2016 01:51
@ayumi ayumi merged commit d46e582 into master Sep 22, 2016
@ayumi ayumi deleted the feature/first-run-timestamp branch September 22, 2016 01:55
@bsclifton
Copy link
Member

👍

@luixxiul luixxiul added this to the 0.12.2dev milestone Sep 23, 2016
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