1053477 - [B2G][OTA][Facebook] Facebook sync settings not preserved performing OTA from 2.0 to 2.1 in Contacts app

Status: VERIFIED FIXED

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

Description

[Roland Kunkel [:RolandK]]

Attachment: logcat

Description:
When the user syncs and imports contacts from Facebook, these settings are not saved after performing the OTA. All the contacts are imported still, however, in Contact Settings it shows that Facebook friends sync has not been enabled yet.
  
Repro Steps:
1) Update a Flame device to BuildID: 20140813000201
2) Connect to WiFi
3) Update channel and url to allow user to update to 2.1 from 2.0
4) Open Contacts > Settings > Sync Friends > select all and import
5) Settings > Device Information > Check now
6) Pull down the notification bar and accept the update
7) Once update is complete, Open Contacts > Settings
8) Observe that Sync Friends is not enabled
 
Actual:
Sync Friends is not enabled after OTA
 
Expected:
Sync Friends is enabled after OTA
 
Environmental Variables:
Device: Flame 2.1 Master
BuildID: 20140813040202
Gaia: 9f35fca9d818b26c06aa6b7e5c0bef25886f8f20
Gecko: 7fc96293ada8
Version: 34.0a1 (2.1 Master)
Firmware: V123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0
 
Repro frequency: 100%
See attached: logcat

Comment 1

[Roland Kunkel [:RolandK]]

This issue also occurs on the latest Flame 2.1 512MB build:

Enviromental Variables:
-----------------------
Environmental Variables:
Device: Flame 2.1 Master 512MB
BuildID: 20140813040202
Gaia: 9f35fca9d818b26c06aa6b7e5c0bef25886f8f20
Gecko: 7fc96293ada8
Version: 34.0a1 (2.1 Master)
Firmware: V123
User Agent: Mozilla/5.0 (Mobile; rv:33.0) Gecko/33.0 Firefox/33.0

Sync Friends is not enabled after performing OTA from 2.0 to 2.1

-

This issue does not occur on the latest 2.0 Flame 512MB or 2.0 Flame 319MB builds: 

Enviromental Variables:
-----------------------
Device:  Flame 2.0 512MB
BuildID: 20140813000201
Gaia: cade2fdbb2230670788dcf2fc7b100f4a37b6458
Gecko: a7c673dae1ed
Version: 32.0 (2.0)
Firmware: v123
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Enviromental Variables:
-----------------------
Device:  Flame 2.0 319MB
BuildID: 20140813000201
Gaia: cade2fdbb2230670788dcf2fc7b100f4a37b6458
Gecko: a7c673dae1ed
Version: 32.0 (2.0)
Firmware: v123
User Agent: Mozilla/5.0 (Mobile; rv:32.0) Gecko/32.0 Firefox/32.0

Sync Friends is still enabled after performing OTA from v123 + Gaia to 2.0

Comment 2

[Peter Bylenga [:PBylenga]]

[Blocking Requested - why for this release]:
Regression and possibly a data loss.  Contacts are still imported but the fact that we're sync'd is not.

Qawanted to see if it's possible to get a regression window by hosting a .mar file of a previous build to OTA to.

Comment 3

[Joshua Mitchell (Inactive)]

QA-work is currently blocked until we can successfully host and work with .mar files.

Comment 4

[Jason Smith [:jsmith]]

Clearing qawanted per comment 3

Comment 5

[Francisco Jordano [:arcturus] [:francisco]]

Hi Adrian,

could this be cause by the remove of IAC for DataStore?

Comment 6

[Adrián de la Rosa]

Yes. In previous versions this info was stored in asyncStorage, but now we use a Datastore and this use case was not taken into account. So the result, as we can see, is that fb related data stored in asyncStorage is still stored in the device but it's unreachable once the device is updated and uses Datastore instead.

Comment 7

[Jason Smith [:jsmith]]

Adrian - Can you point out what bug # caused this change? Also, can you look into this to ensure there's a data migration path present here?

Comment 8

[Anthony Ricaud (:rik)]

So all we need is to have the code read from Datastore and if it's empty, read from asyncStorage and then write into Datastore?

Comment 9

[[:fabrice] Fabrice Desré]

(In reply to Anthony Ricaud (:rik) from comment #8)
> So all we need is to have the code read from Datastore and if it's empty,
> read from asyncStorage and then write into Datastore?

It looks like this should work since we haven't split up the contacts app from the comms app. Assigning to Francisco since he owns the contacts app.

Comment 10

[Adrián de la Rosa]

(In reply to Jason Smith [:jsmith] from comment #7)
> Adrian - Can you point out what bug # caused this change? Also, can you look
> into this to ensure there's a data migration path present here?

The bug that provoked the change is bug #1045521. 

As Anthony said, we need to check if there is something to import into the Datastore when the app starts. The data is stored in the app localStorage through asyncStorage AFAIK.

Comment 11

[Wesley Huang [:wesley_huang] (EPM) (NI me)]

blocking. it's a identifiable regression.

Comment 12

[Anthony Ricaud (:rik)]

Adrián: Just in case, this is not what I said :) To be sure the app is fast for most cases, you should check the Datastore first (most common case) and only if it's empty, perform a migration of data.

Comment 13

[Francisco Jordano [:arcturus] [:francisco]]

Also I will add to keep this change out of the critical path, is not the same thna the FB migration, this case is not critical and can be initiated after we render the contacts list.

Comment 14

[Francisco Jordano [:arcturus] [:francisco]]

Just realised, thanks to Anthony, this bug is assigned to me, Adrian will you work on it?

If not, please assing me back.

Thanks!

Comment 15

[Adrián de la Rosa]

No problem, I will work on it. But if you don't mind I would like to talk to you (maybe over IRC) one of these days to discuss a bit how to tackle this.

Comment 16

[Anthony Ricaud (:rik)]

Just wanted to convey that this is a blocker for our QA group so it should be prioritized accordingly.

Comment 17

[Peter Bylenga [:PBylenga]]

We changed our Smoke Test upgrade paths from '2.0 -> 2.1' to '2.1 -> 2.1'.  Because of this, this issue no longer blocks Smoke Tests, removing keyword.

Comment 18

[Francisco Jordano [:arcturus] [:francisco]]

Hei Adrian,

do you need help here? As Anthony commented this is adquiring some relevant priority to unblcok our QA colleagues.

If you need some help or quick review just let me know.

Comment 19

[Adrián de la Rosa]

We'll talk about this after today's daily. Thanks.

Comment 20

[Adrián de la Rosa]

Attachment: Pull request #23269

I haven't been able to test this on my device yet. I will do it tomorrow when I learn how to do an OTA from my branch (I don't have enough time today!). Meanwhile, I have uploaded this PR in case someone can try it faster than me and merge it if everything is OK, as this is a blocking bug.

Comment 21

[Adrián de la Rosa]

Comment on attachment 8478220 [details]
Pull request #23269

I tested it updating just the communications app, and it seems to work.

Comment 22

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8478220 [details]
Pull request #23269

IMHO the "checkOldStorage" variable is a bad solution where third parties have to know whether the access token should be taken from the new store or old one. I guess that we could do it here https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/contacts.js#L284. 

Something like:

if (!config || !config.accessTokenMigrated) {
  LazyLoader.load('/shared/js/contacts/import/facebook/fb_utils.js', function() {
    // Get access token from old store and set it in ImportStatusData
    utils.cookie.update({
      accessTokenMigrated: true
    });
  });
}

Again, before doing anything, wait for Jordano's feedback because he is the owner and he has the overall view

Comment 23

[Francisco Jordano [:arcturus] [:francisco]]

Hi,

I agree with Cristian, unfortunately this is a process that we will need to add at in the critical path, as a separate action.

Right now, we do an action for checking if we did the migration from indexeddb to Datastore, I was hoping that we can do both actions together, as Cristian comments, we could group this:

https://github.com/mozilla-b2g/gaia/blob/master/apps/communications/contacts/js/contacts.js#L284-L292

And create a file that takes cares of migrations, datastore, token storage, etc.

Thanks!

Comment 24

[Adrián de la Rosa]

Comment on attachment 8478220 [details]
Pull request #23269

As suggested, now I created a new module that handles migrations. I haven't been able to test it yet but I would like to know if this seems like a valid solution to you. Is this similar to what you had in mind?

Comment 25

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8478220 [details]
Pull request #23269

LGTM, I wrote a comment on github and also I would like to add unit tests for the new library migrator.js. The rest is OK for me. Thanks a lot

Comment 26

[Adrián de la Rosa]

Comment on attachment 8478220 [details]
Pull request #23269

I updated the pull request with the unit tests. I finally get to simulate an OTA from v2.0 to v2.1 with this patch and it works.

Comment 27

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8478220 [details]
Pull request #23269

LGTM, please before merging take a look at some minor comments in Github. Thanks a lot

Comment 28

[Adrián de la Rosa]

I solved what you commented on Github, take a look if you want. Now I'll wait for TBPL and I'll wait to see if Francisco has to add something.

Thanks, Cristian!

Comment 29

[Francisco Jordano [:arcturus] [:francisco]]

Comment on attachment 8478220 [details]
Pull request #23269

Some minor comments on github.

Wont block on them as we can do follow ups.

Great work Adrian!

Comment 30

[Francisco Jordano [:arcturus] [:francisco]]

Carring over the result of the last build, before gaia-try were close, since we just bumped the cookie version.

https://tbpl.mozilla.org/?rev=3323e3e2f5fc0ab94ed1b54714277796fd8b766e&tree=Gaia-Try

Merging, thanks everyone for the work!

Landed:

https://github.com/mozilla-b2g/gaia/commit/8a9f635af51ea6a24cff5affb41d26300fa55610

Comment 31

[Duane Dixon [:ddixon]]

Issue is verified fixed on Flame 2.1, 2.2 builds (Full Flash, nightly, 319 MB memory). 

Actual Results: Synced Facebook contacts persist after 2.0-->2.1 and 2.1-->2.2 OTA update. 

Device: Flame 2.1
BuildID: 20141029001202
Gaia: eb0aab0f13c78c7ac378ad860e865c4b6eaf669f
Gecko: 318019f80a8e
Gonk: 48835395daa6a49b281db62c50805bd6ca24077e
Version: 34.0 (2.1)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:34.0) Gecko/34.0 Firefox/34.0

Device: Flame 2.2
BuildID: 20141029040208
Gaia: 35e87ac4324f0f3abd93dcc70d61c9f37256a0f5
Gecko: 7e3c85754d32
Gonk: 05aa7b98d3f891b334031dc710d48d0d6b82ec1d
Version: 36.0a1 (2.2)
Firmware: V188
User Agent: Mozilla/5.0 (Mobile; rv:36.0) Gecko/36.0 Firefox/36.0