924139 - Devise and implement a mechanism to migrate FB data between indexedDB and DataStore

Status: VERIFIED FIXED

(Firefox OS Graveyard :: Gaia::Contacts, defect, P2)

Description

[Jose Manuel Cantera]

In v1.3 all FB Data will live in a DataStore. For users convenience We will need to migrate older versions data present in the old indexedDB to the new DataStore. We need to decide how and when to do this:

A/ At runtime
B/ At any point in build time / OS update time

Just asking first for feedback on them best way to do it

Comment 1

[Andrea Marchesini [:baku]]

> A/ At runtime

This requires some UI... because all the work has to be done in the main thread.
Then, how to be sure that the user doesn't kill the app before the ending of the operation?

> B/ At any point in build time / OS update time

If we can, I prefer this. Fabrice, what do you think about this?

Comment 2

[[:fabrice] Fabrice Desré]

I can't see how this can happen at build time, since users won't rebuild themselves. So the best option is to the migration at first run after update. Since it's application specific this should happen in gaia though...

Comment 3

[Joe Cheng [:jcheng] (please needinfo)]

triage: this is targeted for 1.3. would not block 1.3. please land it in master directly when ready

Comment 4

[Jose Manuel Cantera]

Attachment: 14051.html

Comment 5

[Jose Manuel Cantera]

(In reply to Joe Cheng [:jcheng] from comment #3)
> triage: this is targeted for 1.3. would not block 1.3. please land it in
> master directly when ready

if we don't have this in v1.3 th euser impact would be high as he will lose all his Facebook Data, she will have to re-import re-link

Comment 6

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8338344 [details]
14051.html

Jose, this is looking pretty good, but I have a few concerns I put on Github.  In particular, I think given the amount of code here we probably should have a unit test for datastore_migrator.js.  I'd like to look at it again before landing, so r- for now.

I also agree this should be a blocker for 1.3 if FB datastore is shipping in 1.3.

Thanks!

Comment 7

[Joe Cheng [:jcheng] (please needinfo)]

triage; per comment 5, 1.3+ to get it into v1.3

Comment 8

[Jose Manuel Cantera]

Comment on attachment 8338344 [details]
14051.html

Hi Ben,

Now we have tests and the code is much more robust. Please could you re-review it?

thanks!

Comment 9

[Ben Kelly [:bkelly, not reviewing]]

Thanks Jose.  I will review this first thing tomorrow morning.  I was caught up in a long bisection today that would have been painful to interrupt.

Comment 10

[Ben Kelly [:bkelly, not reviewing]]

Comment on attachment 8338344 [details]
14051.html

While I had difficulty testing this on my device due to bug 938406, the code looks good and the tests pass.

I did have a question about how the user recovers when errors occur.  Should they just re-sync with FB?  Can we automatically do that in the background if they hit an error during migration?

Also, I think we should start using the following pattern in async test code:

  done(function() {
    // test some code that might assert or throw
  });

This makes sure mocha properly reports errors even if its in an async context.

Anyway, if we can address those items, r=me.

Also, I think we should add the verifyme keyword after this lands to have QA double check that a migration from a 1.2 build to 1.3 works as expected.

Thanks!

Comment 11

[Jose Manuel Cantera]

(In reply to Ben Kelly [:bkelly] from comment #10)
> Comment on attachment 8338344 [details]
> 14051.html
> 
> While I had difficulty testing this on my device due to bug 938406, the code
> looks good and the tests pass.
> 
> I did have a question about how the user recovers when errors occur.  Should
> they just re-sync with FB?  Can we automatically do that in the background
> if they hit an error during migration?
> 

Yes, we can recover from errors. See for instance this test https://github.com/mozilla-b2g/gaia/pull/14051/files#diff-ee495aa1f222b7770cd216c23d62cddeR236

> Also, I think we should start using the following pattern in async test code:
> 
>   done(function() {
>     // test some code that might assert or throw
>   });
> 

I've never seeing that pattern in Gaia code nor in Mocha examples I know of. Probably you are meaning that we should assign error callbacks to done as done can take an error object as parameter and avoid calling extra assert.fail. That's what I actually have done. 

> This makes sure mocha properly reports errors even if its in an async
> context.
> 
> Anyway, if we can address those items, r=me.
> 
> Also, I think we should add the verifyme keyword after this lands to have QA
> double check that a migration from a 1.2 build to 1.3 works as expected.
> 
> Thanks!

Comment 12

[Jose Manuel Cantera]

QA. To verify this bug the following steps must be done:

A/ Have a Firefox OS 1.2 device. Import FB Contacts
B/ Trigger a FOTA for v1.3.
C/ Open the Contacts App and stay idle for about 5 seconds. Initially you will see that FB data is not loaded. 

Then the migration should start (see adb logcat) and finally the FB data should be appearing for those contacts on the screen and the rest. 

Ni Isabel to make her aware of this.

Comment 13

[Ben Kelly [:bkelly, not reviewing]]

(In reply to Jose M. Cantera from comment #11) 
> > Also, I think we should start using the following pattern in async test code:
> > 
> >   done(function() {
> >     // test some code that might assert or throw
> >   });
> > 
> 
> I've never seeing that pattern in Gaia code nor in Mocha examples I know of.
> Probably you are meaning that we should assign error callbacks to done as
> done can take an error object as parameter and avoid calling extra
> assert.fail. That's what I actually have done. 

I had not seen in widespread use in gaia, but I also see plenty of cases in gaia where an assert fires and we don't get a stack trace.  Its kind of annoying to solve the problem when that happens.

This pattern was recently shown to me by gsvelto.  It basically wraps the function passed to done() in a try/catch so any exceptions are properly reported.  It seems a bit more elegant than re-writing try/catch everywhere.

Comment 14

[Jose Manuel Cantera]

https://github.com/mozilla-b2g/gaia/commit/0f2768c9a426c7a405b20dac32a2b46b24a747a0

Comment 15

[Noemí Freire (:noemi)]

ni to John Ford, in case he can help us with the uplift to v1.3 branch. It seems the specific v1.3 status flag has not been created yet ("status-b2g-v1.3") so we can't mark v1.3 as "affected"

Thanks!

Comment 16

[Noemí Freire (:noemi)]

(In reply to Noemí Freire (:noemi) from comment #15)
> ni to John Ford, in case he can help us with the uplift to v1.3 branch. It
> seems the specific v1.3 status flag has not been created yet
> ("status-b2g-v1.3") so we can't mark v1.3 as "affected"
> 
> Thanks!

setting "status-firefox28" flag, please clear it if it is not the one used for referring to v1.3 branch.

Comment 17

[John Ford [:jhford] CET/CEST Berlin Time]

[v1.3 022cf2f] Merge pull request #14051 from jmcanterafonseca/fb_migration_datastore

Comment 18

[sarsenyev]

Cannot verify the bug from comment 12.
B/ Trigger a FOTA for v1.3.
Cannot FOTA to 1.3 from 1.2

Comment 19

[Loli (:lolimartinezcr)]

I need build VIA OTA. For this reason this bug isn't tested still.

Comment 20

[Loli (:lolimartinezcr)]

Tested 1.1 -> 1.3 on a Unagi:
1.1
Build ID: 20140303131755
Platfomr version: 18.1
Git Commint: c5cb252e5d1aa45d14f3a2ea182e73e

1.3
Build ID: 20140303045237
Platform version: 28.0
Git Commit: d51d2e09


Tested 1.2 -> 1.3 on a Unagi:
1.2
Build ID: 20140303105246
Platfomr version: 26.0
Git Commit:  539a25e1887b902b8b25038c547048e691

1.3
Build ID: 20140303045237
Platform version: 28.0
Git Commit: d51d2e09

(In a test with 1900 contacts, you will see that FB data is not loaded. If you want to know
 the result for contact you have to hope more 1 minute this time depends of number of contacts to load.)