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

Session runPreMigrations is not working #10595

Closed
NejcZdovc opened this issue Aug 19, 2017 · 8 comments · Fixed by #12796
Closed

Session runPreMigrations is not working #10595

NejcZdovc opened this issue Aug 19, 2017 · 8 comments · Fixed by #12796

Comments

@NejcZdovc
Copy link
Contributor

NejcZdovc commented Aug 19, 2017

Describe the issue you encountered:
Session upgrade is not working anymore, because now we merge default app state and current session data. Because of that things that should not exist (before 0.21 for example bookmarks (https://github.com/brave/browser-laptop/blob/master/app/sessionStore.js#L631) and bookmarkFolders (https://github.com/brave/browser-laptop/blob/master/app/sessionStore.js#L587)) are now empty lists or objects. Because of that all our checks are failing.

This was broken with #10305, were we merged this data https://github.com/brave/browser-laptop/pull/10305/files#diff-9e0c81a9a3f51c8a895685e3183a1452R754.

@NejcZdovc NejcZdovc added this to the 0.18.x Hotfix (Release Channel) milestone Aug 19, 2017
@bbondy
Copy link
Member

bbondy commented Aug 19, 2017

We used to have an upgrade version value in session store, I don't see it anymore. And you could control whether an upgrade path is run or not. I think that would be the best way to control whether to have migrations steps or not. If it's not there anymore then you can just create a new one and if it isn't present then go to the migration step and set that version up one.

@bsclifton bsclifton self-assigned this Aug 22, 2017
@bbondy
Copy link
Member

bbondy commented Aug 23, 2017

I think you can use lastAppVersion, if it's not equal or greater than the current version, or doesn't exist, then you can assume pre-migrations need to run.

@bsclifton
Copy link
Member

bsclifton commented Aug 23, 2017

This should only be an issue with 0.21 and newer (specifically with site split). I verified this by writing tests for the existing migrations, which worked OK. You can see those here: 9ad7b4f

You can view the 0.18.x sessionStore here (before bookmark, bookmarkFolders, etc)

module.exports.runPreMigrations = (data) => {
(which doesn't have bookmarks, bookmarkFolders, etc)

@bsclifton
Copy link
Member

I'd like to propose moving this to 0.21.x (where site split happened). @NejcZdovc, maybe you can help me write the tests 😄

@bsclifton bsclifton assigned bsclifton and NejcZdovc and unassigned bsclifton Aug 23, 2017
@bsclifton bsclifton modified the milestones: 0.21.x (Nightly Channel), 0.18.x Hotfix (Release Channel) Aug 23, 2017
@bsclifton
Copy link
Member

More progress made with 0dfd283

(I've been writing tests and then fixing the issue after the test fails)

@bbondy bbondy modified the milestones: 0.21.x (Developer Channel), 0.20.x (Beta Channel) Oct 25, 2017
@NejcZdovc NejcZdovc removed their assignment Oct 27, 2017
@bsclifton
Copy link
Member

bsclifton commented Nov 27, 2017

We're finally coming around to 0.20.x 😄 To recap, here's the work I've done so far:

Commit 1 - add unit tests for existing code
master/0.21.x/0.20.x 9ad7b4f

Commit 2 - adding tests for site split code migrations; fixing as needed
master/0.21.x/0.20.x 0dfd283

Commit 3 - fixing more of the site split code migrations
master 5f3a54e
0.21.x 08e3e4b
0.20.x ff4c74d

@bsclifton
Copy link
Member

Covered by automated tests- but manual test plan captured in #12369 😄 👍

@bsclifton bsclifton reopened this Jan 22, 2018
@bsclifton
Copy link
Member

Opening after @NejcZdovc found an issue with cache

NejcZdovc added a commit to NejcZdovc/browser-laptop that referenced this issue Jan 23, 2018
bsclifton added a commit that referenced this issue Jan 23, 2018
Fixes cache migration from 0.19 to 0.20
bsclifton added a commit that referenced this issue Jan 23, 2018
Fixes cache migration from 0.19 to 0.20
bsclifton added a commit that referenced this issue Jan 23, 2018
Fixes cache migration from 0.19 to 0.20
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.