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

Send week of installation and ref parameter to updater #12135

Merged
merged 2 commits into from
Dec 4, 2017

Conversation

aekeus
Copy link
Member

@aekeus aekeus commented Nov 29, 2017

  • On first installation store the date of the previous Monday in
    session
  • Accept ref parameter in build-package to store a promo
  • identifier
  • Send woi and ref parameters on update requests

Auditors: @bsclifton, @bbondy

Test Plan:

  • Clear session file
  • Build browser executable REF=promo1 CHANNEL=dev npm run build-package
  • Open built binary (use the --user-data-dir-name=brave-development flag so you don't overwrite your production profile)
  • Close browser
  • Inspect session file (~/Library/Application Support/session-store-1)
    a) Ensure weekOfInstallation has date of previous Monday
    b) Ensure promoCode has the value promo1
  • Open browser
  • Check for an update
  • Open updateLog.log
    a) Ensure query has woi and ref parameters

Implements: #12134

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.

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

@aekeus aekeus added this to the Backlog (Prioritized) milestone Nov 29, 2017
@aekeus aekeus requested a review from bsclifton November 29, 2017 16:24

require('../../braveUnit')

describe('update date handling', function () {
Copy link
Member

Choose a reason for hiding this comment

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

awesome! Thanks for adding this 😄

@codecov-io
Copy link

codecov-io commented Nov 29, 2017

Codecov Report

Merging #12135 into master will increase coverage by 0.03%.
The diff coverage is 50%.

@@            Coverage Diff             @@
##           master   #12135      +/-   ##
==========================================
+ Coverage   54.96%      55%   +0.03%     
==========================================
  Files         275      275              
  Lines       26549    26569      +20     
  Branches     4270     4273       +3     
==========================================
+ Hits        14593    14613      +20     
  Misses      11956    11956
Flag Coverage Δ
#unittest 55% <50%> (+0.03%) ⬆️
Impacted Files Coverage Δ
js/stores/appStore.js 15.97% <0%> (-0.07%) ⬇️
app/dates.js 100% <100%> (+56.52%) ⬆️
app/updater.js 26.13% <28.57%> (-1.31%) ⬇️

@alexwykoff alexwykoff modified the milestones: Backlog (Prioritized), 0.20.x (Beta Channel) Nov 29, 2017
@bsclifton bsclifton modified the milestones: 0.20.x (Beta Channel), 0.19.x Hotfix 7 (Release channel) Dec 1, 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.

The lastMonday function is not working as expected ☹️ I wrote a unit test which causes it to fail. Can you check it out?

  * On first installation store the date of the previous Monday in
    session
  * Accept ref parameter in build-package to store a promo
  * identifier
  * Send woi and ref parameters on update requests

Auditors: @clifton, @bbondy

Test Plan:

  * Clear session file
  * Build browser executable `REF=promo1 CHANNEL=dev npm run
  * build-package`
  * Open built binary
  * Close browser
  * Inspect session file
    a) Ensure weekOfInstallation has date of previous Monday
  * Open browser
  * Check for an update
  * Open updateLog.log
    a) Ensure query has `woi` and `ref` parameters
const d = new Date(exampleDate)
assert.equal(dates.lastMonday(d), '2017-11-13', 'previous Monday')
})
it('returns YYYY-MM-DD of today if today is monday', function () {
Copy link
Member

Choose a reason for hiding this comment

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

@aekeus here's the test I added regarding the previous comment

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 good 😄 👍 Thanks for clarification about the behavior. I added tests for the rest of the file and then also added a test to show that it'll return today (if today is Monday)

@bsclifton bsclifton merged commit d11c391 into master Dec 4, 2017
@bsclifton bsclifton deleted the woi-ref-params branch December 4, 2017 17:50
bsclifton added a commit that referenced this pull request Dec 4, 2017
Send week of installation and ref parameter to updater
bsclifton added a commit that referenced this pull request Dec 4, 2017
Send week of installation and ref parameter to updater
@bsclifton
Copy link
Member

bsclifton commented Dec 4, 2017

master d11c391
0.21.x 68afde8
0.20.x 7ccb2a4
0.19.x 72b934e

bsclifton added a commit that referenced this pull request Dec 4, 2017
Send week of installation and ref parameter to updater
@bsclifton bsclifton modified the milestones: 0.19.x Hotfix 7 (Release channel), 0.19.x + C63 (Release Channel) Dec 7, 2017
@bsclifton bsclifton modified the milestones: 0.19.x + C63 (Release Channel), 0.19.x Private Search Dec 14, 2017
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