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

Fixes twitch event logging #13834

Merged
merged 1 commit into from
Apr 18, 2018
Merged

Conversation

NejcZdovc
Copy link
Contributor

@NejcZdovc NejcZdovc commented Apr 16, 2018

Resolves #13828

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:

  • clean profile
  • enable payments
  1. live stream
  • visit live stream and make sure that is added to the ledger table
  • make sure that time is recorded correctly
  1. VOD
  • visit vod and make sure that is added to the ledger table (with icon)
  • watch this vod and make sure that time is reported correctly
  • try seeking, play and pause and again make sure that time is recorded correctly

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

@NejcZdovc NejcZdovc added this to the 0.22.x Release 3 (Beta channel) milestone Apr 16, 2018
@NejcZdovc NejcZdovc self-assigned this Apr 16, 2018
@codecov-io
Copy link

codecov-io commented Apr 16, 2018

Codecov Report

Merging #13834 into master will increase coverage by 0.08%.
The diff coverage is 96.55%.

@@            Coverage Diff             @@
##           master   #13834      +/-   ##
==========================================
+ Coverage   56.52%   56.61%   +0.08%     
==========================================
  Files         283      283              
  Lines       28817    28848      +31     
  Branches     4777     4782       +5     
==========================================
+ Hits        16289    16331      +42     
+ Misses      12528    12517      -11
Flag Coverage Δ
#unittest 56.61% <96.55%> (+0.08%) ⬆️
Impacted Files Coverage Δ
app/common/constants/twitchEvents.js 100% <ø> (ø) ⬆️
app/browser/api/ledger.js 62.84% <100%> (+0.41%) ⬆️
app/common/lib/ledgerUtil.js 96.12% <93.33%> (+2.73%) ⬆️

Resolves brave#13828

Auditors:

Test Plan:
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.

Test plan works as expected

@NejcZdovc NejcZdovc requested a review from ryanml April 17, 2018 16:42
@NejcZdovc NejcZdovc modified the milestones: 0.22.x Release 3 (Beta channel), 0.22.x w/ Chromium 66 Apr 17, 2018
Copy link
Contributor

@jasonrsadler jasonrsadler left a comment

Choose a reason for hiding this comment

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

@NejcZdovc I'm seeing errors spammed to the console:
[40142:32003:0417/143759.495127:ERROR:upload_data_presenter.cc(73)] Not implemented reached in virtual void extensions::RawDataPresenter::FeedNext(const net::UploadElementReader &)

@NejcZdovc
Copy link
Contributor Author

@jasonrsadler not sure what this is, but i think is not related to this PR.

cc @bridiver @darkdh for the input about this error

@jasonrsadler jasonrsadler self-requested a review April 17, 2018 19:53
Copy link
Contributor

@jasonrsadler jasonrsadler left a comment

Choose a reason for hiding this comment

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

Sorry. I does it on master as well. Everything else looks good

@darkdh
Copy link
Member

darkdh commented Apr 17, 2018

that is not related to the changes of this PR.
It just hits NOTIMPLEMENTED(); of chromium code

@NejcZdovc NejcZdovc merged commit 99a941b into brave:master Apr 18, 2018
NejcZdovc added a commit that referenced this pull request Apr 18, 2018
NejcZdovc added a commit that referenced this pull request Apr 18, 2018
@NejcZdovc
Copy link
Contributor Author

master 99a941b
0.23 5096262
0.22 8b8d098

NejcZdovc added a commit that referenced this pull request Apr 18, 2018
@NejcZdovc
Copy link
Contributor Author

0.22.x-c66 f39f6d2

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.

5 participants