1040618 - [B2G][OTA] Several issues opening smart collections migrated from 1.x

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::Everything.me, defect)

Description

[Cristian Rodriguez (:crdlc)]

Scenario:

* v.1.3

http://i.imgur.com/gKPB2s2.png

Predefined SC: Social, Games and Music
Installed SC: Astrology, Funny and I'm so Bored
Bookmarks: Flickr, Facebook and Twitter

* After OTA (2.0) the vertical home looks properly

http://i.imgur.com/EGF6ICc.png

but...

Predefined SC: 

The spinner does not disappear for Social and Music (pinned apps) and nor pinned apps neither web results are displayed
Games SC works fine (it does not have pinned apps)

Installed SC:

This message is always displayed "Connect to the internet to get apps for I'm So Bored" (I have connection and good signal)

Bookmarks: all of them work fine

Comment 1

[Cristian Rodriguez (:crdlc)]

Attachment: Trace opening social sc

Comment 2

[Cristian Rodriguez (:crdlc)]

Attachment: Descriptor migrated for social sc

Comment 3

[Cristian Rodriguez (:crdlc)]

Attachment: Trace opening funny sc

Comment 4

[Cristian Rodriguez (:crdlc)]

Attachment: Descriptor migrated for funny sc

Comment 5

[Cristian Rodriguez (:crdlc)]

Amir, Ran

   Is there any issue in the descriptor generation during the migration [1] or are the bugs in collection app consuming data? or both?
 
[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/homescreen/js/grid_components.js#L142

Comment 6

[Johan Lorenzo [:jlorenzo]]

FYI, it's not possible to test your upgrade on the Flame device right now, because of bug 1040146.

Comment 7

[Cristian Rodriguez (:crdlc)]

Yes, I know, but I modified my OTA here [1] returning true :)

https://github.com/mozilla-b2g/gaia/blob/master/shared/js/version_helper.js#L32

(In reply to Johan Lorenzo [:jlorenzo] from comment #6)
> FYI, it's not possible to test your upgrade on the Flame device right now,
> because of bug 1040146.

Comment 8

[Gregor Wagner [:gwagner]]

Assigning to Cristian since he is investigating. Please re-assign if there is a better owner.

Comment 9

[Amir Nissim (:amirn)]

Attachment: Pull Request

Looks like any collection with pinned apps is not rendering because of dedupe.js error:
> E/GeckoConsole(  711): [JavaScript Error: "TypeError: result.dedupeId is undefined" {file: "app://collection.gaiamobile.org/shared/js/dedupe.js" line: 57}]

This happens because the pinned items are missing an 'identifier' property.
We were not migrating pinned apps data correctly :(

Cristian, can you please test and review the patch?

It should work for:
1. bookmarks from homescreen added to collection
2. mozapps from homescreen added to collection
3. E.me web results added to collection (long tap > 'add to top of collection')

(did not test myself since I don't know how to simulate OTA updates)

Comment 10

[Cristian Rodriguez (:crdlc)]

ok, thanks a lot, I have to test this pr. It takes a few hours :)

Comment 11

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8459302 [details] [review]
Pull Request

We are in the correct way although there are some issues that should be fixed

1) There is a ReferenceError in the patch (see github)

2) Bookmarks from homescreen added to collection

It does not work. I bookmarked Facebook from Social SC in the old home. After this, I drop the new bookmark to social SC but it does not appear in 2.0

http://i.imgur.com/W4O5uZY.png

3) mozapps from homescreen added to collection

It works fine for regular moz apps and with entry points like "Phone"

4) E.me web results added to collection in 1.3 (long tap > 'add to top of collection')

It works fine too. I added to the social collection "Line" and it appears in 2.0 in the correct place

5) Once migrated if I try to add a web result to top of collection but I have this log

[JavaScript Error: "TypeError: this.toGridObject(...) is undefined" {file: "app://collection.gaiamobile.org/js/objects.js" line: 347}]

it works fine if I try to do the same for Games or Music so it seems that happens only for modified pinned arrays in 1.3 by the user interaction

6) No pre-installed SC cannot be launched (see log attached after posting this review)

Relevant info migrated for Funny:

query:undefined
categoryId:"1e84a07a-4d41-4fdb-a42e-47b204c5900b"
cName:"funny"

Relevant info migrated for I'm so bored:

categoryId:"8fa4b908-366b-4c53-9054-0358a9bd0e40"
query:"I'm So Bored"
cName:"i'm so bored"


7) Social SC keeps the icon from 1.3 instead the new one. It didn't happened last week

Comment 12

[Cristian Rodriguez (:crdlc)]

Attachment: Funny log.txt

Comment 13

[Cristian Rodriguez (:crdlc)]

Regarding to point 7) Social SC keeps the icon from 1.3 instead the new one. After investigating a bit I realized that if the pre-installed didn't updated its icon this will be

app://collection.... (it implies that the icon will be the new one)

but how the icon was updated the icon is

data:image/png;base64,iVBORw0KGgoA.......

Comment 14

[Cristian Rodriguez (:crdlc)]

Regarding to point 6, the first time that non-preinstalled SCs are opened the data is like this:

categoryId:"1e84a07a-4d41-4fdb-a42e-47b204c5900b"
query:"Funny"

and

categoryId:"8fa4b908-366b-4c53-9054-0358a9bd0e40"
query:"I'm So Bored"

Comment 15

[Amir Nissim (:amirn)]

(In reply to Cristian Rodriguez (:crdlc) from comment #13)
> Regarding to point 7) Social SC keeps the icon from 1.3 instead the new one.
> After investigating a bit I realized that if the pre-installed didn't
> updated its icon this will be
> 
> app://collection.... (it implies that the icon will be the new one)
> 
> but how the icon was updated the icon is
> 
> data:image/png;base64,iVBORw0KGgoA.......

Yes, if the user changes the collection, the pre-defined icon will be out of sync.
We can:
1) get the collection from the homescreen. GridManager.getIcon(collectionId) , or
2) the migration will create the icons again

I think 2 is better. Can you tell me where is the relevant migration code?

Comment 16

[Cristian Rodriguez (:crdlc)]

1) I gonna check you latest patch in order to know if non-preinstalled SCs works fine regarding to background

2) I try to figure out how to fix the issue related to updated pre-defined icon

Comment 17

[Amir Nissim (:amirn)]

(In reply to Cristian Rodriguez (:crdlc) from comment #14)
> Regarding to point 6, the first time that non-preinstalled SCs are opened
> the data is like this:
> 
> categoryId:"1e84a07a-4d41-4fdb-a42e-47b204c5900b"
> query:"Funny"
> 
> and
> 
> categoryId:"8fa4b908-366b-4c53-9054-0358a9bd0e40"
> query:"I'm So Bored"

I think I found the problem:
https://github.com/EverythingMe/gaia/blob/1040618-migrate/apps/homescreen/js/grid_components.js#L112

Comment 18

[Amir Nissim (:amirn)]

(In reply to Cristian Rodriguez (:crdlc) from comment #16)
> 1) I gonna check you latest patch in order to know if non-preinstalled SCs
> works fine regarding to background
> 
> 2) I try to figure out how to fix the issue related to updated pre-defined
> icon

Pushed one just now. I hope it will fix 1 and 6

Comment 19

[Cristian Rodriguez (:crdlc)]

testing, thanks

Comment 20

[Cristian Rodriguez (:crdlc)]

Attachment: 1040618-non-preinstalled-sc.patch

Please Amir add this patch to your pr to fix the point 6

Comment 21

[Cristian Rodriguez (:crdlc)]

I gonna fix the point 7 (Social SC keeps the icon from 1.3 instead the new one)

Comment 22

[Cristian Rodriguez (:crdlc)]

Attachment: 1040618-icon-for-preinstalled-scs.patch

Please Amir add this patch to fix the icon for pre-installed SCs

Comment 23

[Cristian Rodriguez (:crdlc)]

remaining work:

2) Bookmarks from homescreen added to collection

It does not work. I bookmarked Facebook from Social SC in the old home. After this, I drop the new bookmark to social SC but it does not appear in 2.0

5) Once migrated if I try to add a web result to top of collection but I have this log

[JavaScript Error: "TypeError: this.toGridObject(...) is undefined" {file: "app://collection.gaiamobile.org/js/objects.js" line: 347}]

it works fine if I try to do the same for Games or Music so it seems that happens only for modified pinned arrays in 1.3 by the user interaction

I gonna investigate about point 2

Comment 24

[Cristian Rodriguez (:crdlc)]

Attachment: 1040618-bookmarks-and-top-collection.patch

The last one, this patch fixes points 2 and 5

Comment 25

[Cristian Rodriguez (:crdlc)]

so when you apply the three patches to your pull request I will test all together, thanks Amir

Comment 26

[Amir Nissim (:amirn)]

Comment on attachment 8459302 [details] [review]
Pull Request

Crisitian, update PR with your patches.
Thanks for you help!

Comment 27

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8459302 [details] [review]
Pull Request

Please fix this comment https://github.com/mozilla-b2g/gaia/pull/21964/files#discussion_r15163818

Comment 28

[Cristian Rodriguez (:crdlc)]

Comment on attachment 8459302 [details] [review]
Pull Request

This is r+ once addressed the last comment about appURL -> appUrl. It works like a charm. Thanks Amir

Comment 29

[Amir Nissim (:amirn)]

(In reply to Cristian Rodriguez (:crdlc) from comment #27)
> Comment on attachment 8459302 [details] [review]
> Pull Request
> 
> Please fix this comment
> https://github.com/mozilla-b2g/gaia/pull/21964/files#discussion_r15163818

oops. fixed. thanks :)

Comment 30

[Amir Nissim (:amirn)]

master: https://github.com/mozilla-b2g/gaia/commit/7543ff4

Comment 31

[Cristian Rodriguez (:crdlc)]

V2.0:

https://github.com/mozilla-b2g/gaia/commit/daca3135197e0041e26470b8872455144f3155af