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

VERIFIED FIXED in Firefox OS v2.1

Status

Firefox OS
Gaia::Contacts
VERIFIED FIXED
4 years ago
4 years ago

People

(Reporter: RolandK, Assigned: Adrián de la Rosa)

Tracking

({regression})

unspecified
2.1 S4 (12sep)
ARM
Gonk (Firefox OS)
regression

Firefox Tracking Flags

(blocking-b2g:2.1+, b2g-v2.0 unaffected, b2g-v2.1 verified, b2g-v2.2 verified)

Details

(Whiteboard: [2.1-flame-test-run-1])

Attachments

(2 attachments)

(Reporter)

Description

4 years ago
Created attachment 8472638 [details]
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
(Reporter)

Comment 1

4 years ago
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
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(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.
blocking-b2g: --- → 2.1?
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(pbylenga)
Keywords: qaurgent, qawanted, smoketest
QA Contact: jmercado
QA-work is currently blocked until we can successfully host and work with .mar files.
Clearing qawanted per comment 3
Keywords: qaurgent, qawanted
Hi Adrian,

could this be cause by the remove of IAC for DataStore?
Flags: needinfo?(hola)
(Assignee)

Comment 6

4 years ago
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.
Flags: needinfo?(hola)
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?
Flags: needinfo?(hola)
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?
(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.
Assignee: nobody → francisco
(Assignee)

Comment 10

4 years ago
(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.
Flags: needinfo?(hola)
blocking. it's a identifiable regression.
blocking-b2g: 2.1? → 2.1+
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.
Blocks: 1045521
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.
Just realised, thanks to Anthony, this bug is assigned to me, Adrian will you work on it?

If not, please assing me back.

Thanks!
Assignee: francisco → hola
(Assignee)

Comment 15

4 years ago
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.
Just wanted to convey that this is a blocker for our QA group so it should be prioritized accordingly.
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.
Keywords: smoketest
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.
Flags: needinfo?(hola)
(Assignee)

Comment 19

4 years ago
We'll talk about this after today's daily. Thanks.
Flags: needinfo?(hola)
(Assignee)

Comment 20

4 years ago
Created attachment 8478220 [details]
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.
(Assignee)

Comment 21

4 years ago
Comment on attachment 8478220 [details]
Pull request #23269

I tested it updating just the communications app, and it seems to work.
Attachment #8478220 - Flags: review?(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
Attachment #8478220 - Flags: review?(crdlc) → review-
Flags: needinfo?(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!
Flags: needinfo?(francisco)
(Assignee)

Comment 24

4 years ago
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?
Attachment #8478220 - Flags: feedback?(francisco)
Attachment #8478220 - Flags: feedback?(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
Attachment #8478220 - Flags: feedback?(crdlc) → feedback+
(Assignee)

Comment 26

4 years ago
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.
Attachment #8478220 - Flags: review?(francisco)
Attachment #8478220 - Flags: review?(crdlc)
Attachment #8478220 - Flags: review-
Attachment #8478220 - Flags: feedback?(francisco)
Comment on attachment 8478220 [details]
Pull request #23269

LGTM, please before merging take a look at some minor comments in Github. Thanks a lot
Attachment #8478220 - Flags: review?(crdlc) → review+
(Assignee)

Comment 28

4 years ago
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 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!
Attachment #8478220 - Flags: review?(francisco) → review+
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(dharris)
Whiteboard: [2.1-flame-test-run-1]
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(dharris)
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
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
status-b2g-v2.1: affected → fixed
Target Milestone: --- → 2.1 S4 (12sep)
status-b2g-v2.2: --- → fixed
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
Status: RESOLVED → VERIFIED
QA Whiteboard: [QAnalyst-Triage+] → [QAnalyst-Triage?]
status-b2g-v2.1: fixed → verified
status-b2g-v2.2: fixed → verified
Flags: needinfo?(ktucker)
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(ktucker)
You need to log in before you can comment on or make changes to this bug.