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

increase version range for pinnedTopSites cleanup #13429

Merged
merged 1 commit into from
Mar 19, 2018
Merged

increase version range for pinnedTopSites cleanup #13429

merged 1 commit into from
Mar 19, 2018

Conversation

cezaraugusto
Copy link
Contributor

@cezaraugusto cezaraugusto commented Mar 13, 2018

Different fixes were tried for this but all needed to iterate over the list to check for duplicates and ended bulking startup time.

This fix is the same take as previous with a wider range of versions. This is better for performance as it does not iterate over the list on startup/every pin interaction but instead cleans the state at update time so no user-space affected.

fix #12941

Test Plan:

  1. Hack your session-store-1 adding a few duplicated objects inside pinnedTopSites list.
  2. Run Brave
  3. Quit
  4. Check session-store-1 file again. There should be no duplicates
  5. Run Brave again. There should be no pinned top sites

Test Plan (QA specific):

  1. Install 0.19.x - have a bunch of top sites
  2. Upgrade to 0.20.30 which doesn't have the fix. Verify still broken
  3. Update to this release (0.22.x) and verify issue is now fixed

@cezaraugusto cezaraugusto added this to the 0.22.x (Developer Channel) milestone Mar 13, 2018
@cezaraugusto cezaraugusto self-assigned this Mar 13, 2018
@codecov-io
Copy link

codecov-io commented Mar 13, 2018

Codecov Report

Merging #13429 into master will increase coverage by 0.01%.
The diff coverage is 87.5%.

@@            Coverage Diff             @@
##           master   #13429      +/-   ##
==========================================
+ Coverage   56.25%   56.26%   +0.01%     
==========================================
  Files         283      283              
  Lines       28353    28363      +10     
  Branches     4674     4675       +1     
==========================================
+ Hits        15950    15959       +9     
- Misses      12403    12404       +1
Flag Coverage Δ
#unittest 56.26% <87.5%> (+0.01%) ⬆️
Impacted Files Coverage Δ
app/sessionStore.js 88.22% <87.5%> (+0.02%) ⬆️

@bsclifton
Copy link
Member

@cezaraugusto besides the dupes, this should fix folks who missed your fix (fixing their top sites).

Did we want to capture that scenario in the test plan? ex:

  1. Install 0.19.x - have a bunch of top sites
  2. Upgrade to 0.20.30 which doesn't have the fix. Verify still broken
  3. Update to this release and verify issue is now fixed

// allowing duplicated items. See #12941
let pinnedTopSitesCleanup = false
try {
pinnedTopSitesCleanup = compareVersions(data.lastAppVersion, '0.22.00') < 1
Copy link
Member

@bsclifton bsclifton Mar 14, 2018

Choose a reason for hiding this comment

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

Should we have another check for this, so it only runs once?

Specifically we could capture this in the state in an area called "migrations" (see example which was removed with ccee122#diff-b4e400e260f324745d12616a8c9c1fc6L377 - was removed since nothing else is using it).

When fix is ran you could set this value to current timestamp and default the value (in app/sessionStore.js) to undefined

Copy link
Member

Choose a reason for hiding this comment

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

Also, note to self: we'll want to update this version before we ship. We might want to create an issue to track that

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- left comment about having this only run once and asking about test plan. Let me know when it's ready for re-review I'll run the test plan 😄 👍

@bsclifton bsclifton mentioned this pull request Mar 19, 2018
10 tasks
@cezaraugusto
Copy link
Contributor Author

re #13429 (comment) I included your take (thanks) in the test plan. Most users did receive the fix, this PR specifically target all of them and unless someone manages to update from 0.19.x to something >0.22.00, all users should get this fix.

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.

Thanks for the test plan update! 😄 👍 Great work, sir!

@bsclifton bsclifton merged commit 9e91ffe into master Mar 19, 2018
@bsclifton bsclifton deleted the 12941 branch March 19, 2018 22:59
bsclifton added a commit that referenced this pull request Mar 19, 2018
increase version range for pinnedTopSites cleanup
bsclifton added a commit that referenced this pull request Mar 19, 2018
increase version range for pinnedTopSites cleanup
@bsclifton
Copy link
Member

master 9e91ffe
0.23.x 23e50ad
0.22.x f1d4f72

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.

top site tiles not being populated when updating profile from 0.19.147
3 participants