1031094 - [Collection] Removing an e.me app from the homescreen also removes it from the top of the collection

Status: RESOLVED FIXED

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

Description

[Johan Lorenzo [:jlorenzo]]

STR
1. Open a Smart Collection
2. Add a e.me app to the homescreen
3. Add it to the top of the Smart Collection
4. Go to the homescreen and remove it.
5. Go back to the collection

Actual result
The app is moved back with the other. Worst case, if the Internet connection is off while removing the bookmark on the homescreen, the icon is no more present.

Comment 1

[Johan Lorenzo [:jlorenzo]]

Bad user experience.

Comment 2

[Jason Smith [:jsmith]]

Did this problem happen in 1.4 with the old e.me search implementation?

Comment 3

[Johan Lorenzo [:jlorenzo]]

No, it doesn't. The icon is still at the top of the homescreen in 1.4.

Comment 4

[Kevin Grandon :kgrandon]

Appears that this is because we call down into unpin() for both mozApp types and bookmark types. We should probably not do this for bookmarks.

https://github.com/mozilla-b2g/gaia/blob/master/apps/collection/js/native_info.js#L105

The proper fix should include a test which adds a result the the homescreen, pins it, removes the homescreen bookmark, and verifies that the pin is still there.

Comment 5

[Kevin Grandon :kgrandon]

Chris - do you have any cycles to look at this one? The hardest part for this is probably writing the marionette test, which is something we definitely want for this.

Comment 6

[Chris Lord [:cwiiis]]

I'll take a look at this.

Comment 7

[Chris Lord [:cwiiis]]

I have a patch that fixes the issue, I'm working on an integration test to make sure this doesn't regress.

Comment 8

[Chris Lord [:cwiiis]]

Attachment: Don't unpin uninstalled bookmarks

Comment 9

[Kevin Grandon :kgrandon]

Comment on attachment 8448210 [details] [review]
Don't unpin uninstalled bookmarks

Looking good, but I'd like to see another version before leaving an R+. Left a few comments on github, thanks for taking the time to write a test!

Comment 10

[Chris Lord [:cwiiis]]

Comment on attachment 8448210 [details] [review]
Don't unpin uninstalled bookmarks

Ok, attempt 2 is up :) I've answered your comments on github, I couldn't take everything into account, but most of it. We can discuss it later if it isn't quite enough.

Comment 11

[Kevin Grandon :kgrandon]

Comment on attachment 8448210 [details] [review]
Don't unpin uninstalled bookmarks

I would still like to assert on the icon identifier being present, but besides that I think we are good to go. Thanks!

Comment 12

[Chris Lord [:cwiiis]]

Attachment: Don't unpin uninstalled bookmarks

Squashed commit with last revision made. Carrying f+/r+

Comment 13

[Kevin Grandon :kgrandon]

We had some green test runs, and this looks good to me, so landing: https://github.com/mozilla-b2g/gaia/commit/bc215204fb986871f1a26d4c459ba1cfc3f8c5c3

Thanks for working on this test!

Comment 14

[Ryan VanderMeulen [:RyanVM]]

v2.0: https://github.com/mozilla-b2g/gaia/commit/279dced30d00c43c187bf806f302055e6dc9520c

Comment 15

[Shally Li（leave from Mozilla）]

This issue has been failed verified on Flame 2.1.
See attachment: verify_v2.1.mp4.
Reproduce rate: 5/6

Repro STR:
1. Open a Smart Collection.
2. Select a e.me app and long tap,select "Add to Home Screen".
3. Long tap the app and select "Add to top of collection".
4. Go to the homescreen and remove it.
5. Go back to the collection.
**After removed the e.me app from the homescreen,it will also be removed from the top of the collection.

This issue has been successfully verified on Flame 2.0.
See attachment: verified_v2.0.mp4.
Reproduce rate: 0/5

Flame 2.1 build:
Gaia-Rev        ccb49abe412c978a4045f0c75abff534372716c4
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g34_v2_1/rev/18fb67530b22
Build-ID        20141201001201
Version         34.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141201.034405
FW-Date         Mon Dec  1 03:44:15 EST 2014
Bootloader      L1TC00011880

Flame 2.0 build:
Gaia-Rev        8d1e868864c8a8f1e037685f0656d1da70d08c06
Gecko-Rev       https://hg.mozilla.org/releases/mozilla-b2g32_v2_0/rev/c756bd8bf3c3
Build-ID        20141201000201
Version         32.0
Device-Name     flame
FW-Release      4.4.2
FW-Incremental  eng.cltbld.20141201.034308
FW-Date         Mon Dec  1 03:43:18 EST 2014
Bootloader      L1TC00011880

Comment 16

[Shally Li（leave from Mozilla）]

Attachment: verify_v2.1.MP4

Comment 17

[Shally Li（leave from Mozilla）]

Attachment: verified_v2.0.MP4

Comment 18

[Shally Li（leave from Mozilla）]

Attachment: logcat_1730.txt

Add flame v2.1 logcat.

Comment 19

[William Hsu [:whsu]]

Add NI?whsu to follow up

Comment 20

[William Hsu [:whsu]]

Thanks Shally! :)

A bug was submitted. (Bug 1118699)