Closed
Bug 1053477
Opened 10 years ago
Closed 10 years ago
[B2G][OTA][Facebook] Facebook sync settings not preserved performing OTA from 2.0 to 2.1 in Contacts app
Categories
(Firefox OS Graveyard :: Gaia::Contacts, defect)
Tracking
(blocking-b2g:2.1+, b2g-v2.0 unaffected, b2g-v2.1 verified, b2g-v2.2 verified)
Tracking | Status | |
---|---|---|
b2g-v2.0 | --- | unaffected |
b2g-v2.1 | --- | verified |
b2g-v2.2 | --- | verified |
People
(Reporter: rkunkel, Assigned: hola)
References
Details
(Keywords: regression, Whiteboard: [2.1-flame-test-run-1])
Attachments
(2 files)
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•10 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)
Comment 2•10 years ago
|
||
[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.
Updated•10 years ago
|
QA Contact: jmercado
Comment 3•10 years ago
|
||
QA-work is currently blocked until we can successfully host and work with .mar files.
Comment 5•10 years ago
|
||
Hi Adrian, could this be cause by the remove of IAC for DataStore?
Flags: needinfo?(hola)
Assignee | ||
Comment 6•10 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)
Comment 7•10 years ago
|
||
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)
Comment 8•10 years ago
|
||
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•10 years ago
|
||
(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•10 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)
Comment 12•10 years ago
|
||
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
Comment 13•10 years ago
|
||
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•10 years ago
|
||
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•10 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.
Comment 16•10 years ago
|
||
Just wanted to convey that this is a blocker for our QA group so it should be prioritized accordingly.
Comment 17•10 years ago
|
||
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
Comment 18•10 years ago
|
||
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•10 years ago
|
||
We'll talk about this after today's daily. Thanks.
Flags: needinfo?(hola)
Assignee | ||
Comment 20•10 years ago
|
||
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•10 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 22•10 years ago
|
||
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)
Comment 23•10 years ago
|
||
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•10 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 25•10 years ago
|
||
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•10 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 27•10 years ago
|
||
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•10 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 29•10 years ago
|
||
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+
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?]
Flags: needinfo?(dharris)
Whiteboard: [2.1-flame-test-run-1]
Updated•10 years ago
|
QA Whiteboard: [QAnalyst-Triage?] → [QAnalyst-Triage+]
Flags: needinfo?(dharris)
Comment 30•10 years ago
|
||
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
Closed: 10 years ago
Resolution: --- → FIXED
Updated•10 years ago
|
Target Milestone: --- → 2.1 S4 (12sep)
Updated•10 years ago
|
status-b2g-v2.2:
--- → fixed
Comment 31•10 years ago
|
||
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?]
Flags: needinfo?(ktucker)
Updated•10 years ago
|
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.
Description
•