Closed Bug 1214188 Opened 4 years ago Closed 4 years ago

Call setDataStoreId after merge in history adapter?

Categories

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

ARM
Gonk (Firefox OS)
defect

Tracking

(Not tracked)

RESOLVED FIXED
FxOS-S11 (13Nov)

People

(Reporter: mbdejong, Assigned: mbdejong)

References

Details

(Whiteboard: [partner-cherry-pick])

Attachments

(1 file)

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)
Assignee: nobody → mbdejong
Blocks: 1195647
Flags: needinfo?(selee)
Hi Michiel, Thanks again for finding this.
Just like you said, it should follow setDataStoreId.
Flags: needinfo?(selee)
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
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.
Target Milestone: --- → FxOS-S9 (16Oct)
Attachment #8674110 - Flags: feedback?(selee)
Needs a unit test, still.
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. :)
Attachment #8674110 - Flags: feedback?(selee) → feedback-
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.
Flags: needinfo?(selee)
Target Milestone: FxOS-S9 (16Oct) → FxOS-S11 (13Nov)
This bug is phone-only, cannot happen on TV yet.
(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.
Flags: needinfo?(selee)
Priority: -- → P3
Attachment #8674110 - Flags: review?(selee)
Comment on attachment 8674110 [details] [review]
[gaia] michielbdejong:1214188-setDataStoreId-after-merge > mozilla-b2g:master

LGTM :)
Attachment #8674110 - Flags: review?(selee) → review+
Rebased on master.
Flags: needinfo?(selee)
Hey Michiel, I leave one comment for this patch.
Flags: needinfo?(selee)
Comment on attachment 8674110 [details] [review]
[gaia] michielbdejong:1214188-setDataStoreId-after-merge > mozilla-b2g:master

Already got r+ from Sean and addressed his comments.
Attachment #8674110 - Flags: review?(ferjmoreno)
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.
Attachment #8674110 - Flags: review?(ferjmoreno) → review+
Tests added (and testDataGenerator fixed), and rebased. Let's wait for TreeHerder.
Flags: needinfo?(ferjmoreno)
TreeHerder is green.
Gaia is closed :\ so we have to wait a little bit.
https://github.com/mozilla-b2g/gaia/commit/8bd1fd2a4c7bc1977dfb8e85a1f9ffaf2f31a82e
Status: NEW → RESOLVED
Closed: 4 years ago
Flags: needinfo?(ferjmoreno)
Resolution: --- → FIXED
Whiteboard: [partner-cherry-pick]
You need to log in before you can comment on or make changes to this bug.