1214188 - Call setDataStoreId after merge in history adapter?

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Sync, defect, P3)

Description

[Michiel de Jong [:michielbdejong]]

Reading the code, it seems to me that https://github.com/mozilla-b2g/gaia/blob/master/apps/sync/js/adapters/history.js#L128 should also be followed by a call to setDataStoreId? (to discuss with Sean)

Comment 1

[Sean Lee [:seanlee][:weilonge]]

Hi Michiel, Thanks again for finding this.
Just like you said, it should follow setDataStoreId.

Comment 2

[Sean Lee [:seanlee][:weilonge]]

Hey Michiel,

I would like to add 'setDataStoreId(place.fxsyncId, id);' here for both addPlace and deletePlace:
https://github.com/mozilla-b2g/gaia/blob/master/apps/sync/js/adapters/history.js#L144

so the original setDataStoreId can be removed. 
https://github.com/mozilla-b2g/gaia/blob/master/apps/sync/js/adapters/history.js#L130

Comment 3

[Michiel de Jong [:michielbdejong]]

OK! I'll have a look at that.

Same issue exists bookmarks adapter https://github.com/mozilla-b2g/gaia/blob/master/apps/sync/js/adapters/bookmarks.js#L124 btw.

Comment 4

[GitHub Autolander Bot]

Attachment: [gaia] michielbdejong:1214188-setDataStoreId-after-merge > mozilla-b2g:master

Comment 5

[Michiel de Jong [:michielbdejong]]

Needs a unit test, still.

Comment 6

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8674110 [details] [review]
[gaia] michielbdejong:1214188-setDataStoreId-after-merge > mozilla-b2g:master

Thanks for working on this, Michiel!
I think the matching table has to be updated only when the new record is coming.
In the case of your patch, both bookmark and history are updating an existing record. That's not necessary to update it again.

This issue is worth to consider again in our new bookmark store.

Thank you. :)

Comment 7

[Michiel de Jong [:michielbdejong]]

Right, we won't need this for the TV, but we'll need it for the phone. Foxfooders will already have history entries (and to a lesser extent also bookmark entries) on the phone before a history or bookmarks entry comes in from sync, the same URL.

Comment 8

[Michiel de Jong [:michielbdejong]]

This bug is phone-only, cannot happen on TV yet.

Comment 9

[Michiel de Jong [:michielbdejong]]

(In reply to Michiel de Jong [:michielbdejong] from comment #8)
> This bug is phone-only, cannot happen on TV yet.

Correction, on the TV this cannot happen for history records, and for bookmarks records, it's fixed here: https://github.com/mozilla-b2g/gaia/pull/32578/files#diff-6dac2f6d58d68fbc11a10804f9946d4fR116 as part of bug 1215086.

Will do the fix for history as a P3 priority.

Comment 10

[Sean Lee [:seanlee][:weilonge]]

Comment on attachment 8674110 [details] [review]
[gaia] michielbdejong:1214188-setDataStoreId-after-merge > mozilla-b2g:master

LGTM :)

Comment 11

[Michiel de Jong [:michielbdejong]]

Rebased on master.

Comment 12

[Sean Lee [:seanlee][:weilonge]]

Hey Michiel, I leave one comment for this patch.

Comment 13

[Michiel de Jong [:michielbdejong]]

Comment on attachment 8674110 [details] [review]
[gaia] michielbdejong:1214188-setDataStoreId-after-merge > mozilla-b2g:master

Already got r+ from Sean and addressed his comments.

Comment 14

[Fernando Jiménez Moreno [:ferjm]]

Comment on attachment 8674110 [details] [review]
[gaia] michielbdejong:1214188-setDataStoreId-after-merge > mozilla-b2g:master

Thank you Michiel. It'd be great to have a test for this.

Comment 15

[Michiel de Jong [:michielbdejong]]

Tests added (and testDataGenerator fixed), and rebased. Let's wait for TreeHerder.

Comment 16

[Michiel de Jong [:michielbdejong]]

TreeHerder is green.

Comment 17

[Fernando Jiménez Moreno [:ferjm]]

Gaia is closed :\ so we have to wait a little bit.

Comment 18

[Fernando Jiménez Moreno [:ferjm]]

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