Closed Bug 1031094 Opened 5 years ago Closed 5 years ago

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

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(blocking-b2g:2.0+, b2g-v2.0 verified, b2g-v2.1 fixed)

RESOLVED FIXED
2.0 S5 (4july)
blocking-b2g 2.0+
Tracking Status
b2g-v2.0 --- verified
b2g-v2.1 --- fixed

People

(Reporter: jlorenzo, Assigned: cwiiis)

References

Details

(Keywords: regression, Whiteboard: [systemsfe])

Attachments

(4 files, 1 obsolete file)

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.
Bad user experience.
blocking-b2g: --- → 2.0?
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking+]
Whiteboard: [systemsfe]
Did this problem happen in 1.4 with the old e.me search implementation?
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking+] → [VH-FL-blocking-][VH-FC-blocking?]
Keywords: qawanted
No, it doesn't. The icon is still at the top of the homescreen in 1.4.
QA Whiteboard: [VH-FL-blocking-][VH-FC-blocking?] → [VH-FL-blocking-][VH-FC-blocking+]
Keywords: qawantedregression
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.
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.
Flags: needinfo?(chrislord.net)
I'll take a look at this.
Assignee: nobody → chrislord.net
Status: NEW → ASSIGNED
Flags: needinfo?(chrislord.net)
blocking-b2g: 2.0? → 2.0+
I have a patch that fixes the issue, I'm working on an integration test to make sure this doesn't regress.
Target Milestone: --- → 2.0 S5 (4july)
Attached file Don't unpin uninstalled bookmarks (obsolete) —
Attachment #8448210 - Flags: review?(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!
Attachment #8448210 - Flags: review?(kgrandon) → feedback+
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.
Attachment #8448210 - Flags: review?(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!
Attachment #8448210 - Flags: review?(kgrandon) → review+
Squashed commit with last revision made. Carrying f+/r+
Attachment #8448210 - Attachment is obsolete: true
Attachment #8449312 - Flags: review+
Attachment #8449312 - Flags: feedback+
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!
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
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
Flags: needinfo?(hlu)
Attached file logcat_1730.txt
Add flame v2.1 logcat.
Add NI?whsu to follow up
Flags: needinfo?(hlu) → needinfo?(whsu)
Thanks Shally! :)

A bug was submitted. (Bug 1118699)
Flags: needinfo?(whsu)
See Also: → 1118699
You need to log in before you can comment on or make changes to this bug.