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

Fixes usage of ledger advance settings in the ledger table #8066

Merged
merged 1 commit into from
Apr 5, 2017

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Apr 4, 2017

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

Resolves #7987

Auditors

@mrose17 @diracdeltas

Test Plan

  • enable payments
  • visit site for at least 8 seconds or until it appears in the synopsis table
  • set minimum visits to 5 instead of 1
  • site shouldn't be in the ledger table

@NejcZdovc NejcZdovc added this to the 0.14.2 milestone Apr 4, 2017
@NejcZdovc NejcZdovc self-assigned this Apr 4, 2017
Copy link
Member

@mrose17 mrose17 left a comment

Choose a reason for hiding this comment

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

LGTM.

@diracdeltas
Copy link
Member

test plan worked, but some synopsis tests may be failing due to this: https://travis-ci.org/brave/browser-laptop/jobs/218608197#L3227. they failed locally for me too.

@diracdeltas
Copy link
Member

specifically this fails on this branch but passes on master (locally):

> npm run test -- --grep='synopsis'

> [email protected] test /Users/yan/repos/browser-laptop
> cross-env NODE_ENV=test mocha "test/**/*Test.js" "--grep=synopsis"



  synopsis
    ✓ no table if empty synopsis (46ms)
    1) creates synopsis table after visiting a site
    2) can sort synopsis table
    ✓ can disable site (2683ms)

  PublisherToggle component
    default behaviour (when autoSuggest is ON)
      ✓ show as enabled if site is on synopsis list (siteSettings is empty)
      ✓ show as enabled if hostPattern is on siteSettings list (synopsis is empty)
      ✓ show as enabled if url exists in locationInfo (both synopsis and siteSettings are empty)


  5 passing (46s)
  2 failing

  1) synopsis creates synopsis table after visiting a site:
     Promise was rejected with the following reason: timeout
  Error

  2) synopsis can sort synopsis table:
     WaitUntilTimeoutError: element ([data-l10n-id="publisher"]) still not visible after 10000ms
      at isVisible("[data-l10n-id="publisher"]") - waitForVisible.js:37:22

Resolves brave#7987

Auditors: @mrose17 @diracdeltas

Test Plan:
- enable payments
- visit site for at least 8 seconds or until it appears in the synopsis table
- set minimum visits to 5 instead of 1
- site shouldn't be in the ledger table
@NejcZdovc NejcZdovc force-pushed the hotfix/#7987-ledger-eligible branch from d149ed3 to 77c499c Compare April 4, 2017 23:56
@NejcZdovc
Copy link
Contributor Author

@diracdeltas fixed, I needed to add back one property, because it's used in ledger-publisher, so we can use this hack for test (visit time is set to 0s when in executing tests)

@diracdeltas
Copy link
Member

lgtm, thanks for looking into this

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