[LeanPlum]: Top Sites panel open instead of Bookmarks Panel

RESOLVED FIXED in Firefox 60

Status

()

defect
P1
normal
Rank:
1
RESOLVED FIXED
2 years ago
Last year

People

(Reporter: sflorean, Assigned: vlad.baicu)

Tracking

(Blocks 1 bug)

57 Branch
Firefox 61
ARM
Android
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox55 unaffected, firefox56 unaffected, firefox57 wontfix, firefox59 wontfix, firefox60blocking fixed, firefox61 fixed)

Details

(Whiteboard: [Leanplum] [60])

Attachments

(1 attachment, 4 obsolete attachments)

Devices: 
 - HTC 10 (Android 7.0);
 - Nexus 5 (Android 6.0.1);
 - HTC Desire 820 (Android 6.0.1);
 - Motorola Nexus 6 (Android 7.1.1);

Prerequisites:
 - install Switchboard Experiments and activate LeanPlum.

Steps to reproduce:
 1. Open Fennec and save one or multiple bookmarks;
 2. Close Fennec and wait to receive F push notification.
 3. Tap on Notification.

Expected result:
 The user is taken to firefox://bookmark_list (Bookmarks Tab).

Actual result:
 The user is taken to Top Sites Panel.
 
Notes:
 - On Samsung Galaxy Note 4 (Android 5.0.1), second time when F Push notification was displayed, the user was taken on bookmarks panel.
Hi Jing Wei
Do you know if we change the HomConfig
Hi Jing Wei
Do you know if we change the HomConfig?
Previously I can load bookmarks panel using AboutPages.getURLForBuiltinPanelType(HomeConfig.PanelType.BOOKMARKS)
But now it seems not working. I'll check this after I arrive the office. Thanks!
Flags: needinfo?(topwu.tw)
Turns out it's a regression from a late landing patch bug 1367069.
See https://bugzilla.mozilla.org/show_bug.cgi?id=1367069#c8
Assignee: nobody → cnevinchen
Flags: needinfo?(topwu.tw)
Keywords: regression
Priority: -- → P1
See Also: → 1367069
After further investigation, firefox://bookmark_list deep link won't work when app is not running. 
But after bug 1367069, it's even not working when app is running so we still need to back it out.
I just set the flag for the first time. Please correct me if I'm wrong
Target Milestone: --- → Firefox 57
Can we resolve this WFM now that bug 1367069 has been backed out?
Flags: needinfo?(cnevinchen)
I'll fix this for 57 (hope I can get the patch uplift to 57 beta this week)
Flags: needinfo?(cnevinchen)
(In reply to Nevin Chen [:nechen] from comment #7)
> I'll fix this for 57 (hope I can get the patch uplift to 57 beta this week)
I think backing bug 1367069 maybe enough.

Hi Sorina
Could you please help test on Nightly and see if this is fixed now?
Thanks!
Flags: needinfo?(sorina.florean)
(In reply to Nevin Chen [:nechen] from comment #8)
> (In reply to Nevin Chen [:nechen] from comment #7)
> > I'll fix this for 57 (hope I can get the patch uplift to 57 beta this week)
> I think backing bug 1367069 maybe enough.
> 
> Hi Sorina
> Could you please help test on Nightly and see if this is fixed now?
> Thanks!

Hi Nevin, 
Tested on latest Nightly build and the behavior is this:
- Nightly is closed from task manager; tapping on F notification opens Top Sites Panel.
- Nightly is in background; tapping on F notification opens Bookmarks Panel.
Flags: needinfo?(sorina.florean)
Thanks Sorina.
This is the expected according to current design. 

Hi Joe
If we want to make :
Nightly is closed from task manager; tapping on F notification opens "Top Sites Panel"  really opens "Bookmarks Panel"

We need to have another fix and put it in sprint planing.
Flags: needinfo?(wehuang)
Flags: needinfo?(jcheng)
since bug 1367069 is backed out this is no regression here and not necessary as P1.
Flags: needinfo?(wehuang)
Keywords: regression
Priority: P1 → P2
Susheel, can you ask someone who uses switchboard how important this is?
Flags: needinfo?(jcheng) → needinfo?(sdaswani)
Mike I spoke to Jean. She wants a fix but in the months time frame, not weeks. So we can live with it while we find an engineer to work on it.
Flags: needinfo?(sdaswani)
Priority: -- → P3
[triage] Bulk edit: product thinks leanplum is high priority: P1 rank 1.
Assignee: cnevinchen → nobody
Rank: 1
Priority: P3 → P1
Whiteboard: [Leanplum]
Whiteboard: [Leanplum] → [Leanplum] [60]
Product team would like this fixed in 60, tracked as blocking to ensure it's closely monitored.
Target Milestone: Firefox 57 → ---
Assignee: nobody → vlad.baicu
Instead of the reported behaviour, I have noticed together with Sorina that all deeplinks originating from LP weren't working. 

The issue was a check done in Launcher activity for an "uid" parameter in the deeplink intent that was null. I saw that in the past, this restriction was applied by Nevin for Bug 1380950. I checked for the origins of this parameter, however I was unable to find anything related to it & LP. Nevin, did this parameter used to come in the payload of the notification from LP ? Maybe i'm missing something here.

Meanwhile, I have reworked the intent differentiation by setting an extra for the intent in LeanplumPushService which I then check for in LauncherActivity and the deeplinks now work as intended.
Flags: needinfo?(cnevinchen)
Attachment #8963213 - Flags: review?(michael.l.comella)
Vlad as a security measure all deep links require a UID parameter. I will ask Nevin whats the method to get the uid for debug/dev builds.
Flags: needinfo?(cnevinchen) → needinfo?(sdaswani)
Deeplinks set in LP dashboard need to be set with the query '?uid={{User ID}}' for newer builds of Fennec which have this restriction in place, thank you for clearing that up. 

I have changed the way to retrieve the localUid value in the same that is being sent to LP to have deeplinks work on dev builds as well if switchboard experiments are enabled.

The issue signaled by Sorina still reproduces, however I have reviewed and tested Nevin's patch and I can confirm that it resolves the issue.
Attachment #8963213 - Attachment is obsolete: true
Attachment #8963213 - Flags: review?(michael.l.comella)
Flags: needinfo?(sdaswani)
Attachment #8963695 - Flags: review?(michael.l.comella)
Attachment #8963695 - Flags: review?(michael.l.comella)
Vlad I'm a bit confused about this latest patch - does it solve the issue cited? We can discuss live tomorrow.
Flags: needinfo?(vlad.baicu)
Attachment #8915940 - Flags: review?(max) → review?(sdaswani)
Comment on attachment 8915940 [details]
Bug 1399388 - Only when new page is exactly 'about:home' will be treated as default home page.

https://reviewboard.mozilla.org/r/187204/#review238198

This code looks good and I've confirmed it works.
Attachment #8915940 - Flags: review?(sdaswani) → review+
Ritu, can we get this uplifted to 60 today? It's a small patch and the contractors and I confirmed it works.
Flags: needinfo?(rkothari)
Comment on attachment 8915940 [details]
Bug 1399388 - Only when new page is exactly 'about:home' will be treated as default home page.

Approval Request Comment
[Feature/Bug causing the regression]: Deep links
[User impact if declined]: Leanplum campaigns are limited by this bug
[Is this code covered by automated tests?]: No
[Has the fix been verified in Nightly?]: No, but verified in a dev build.
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]:
[Is the change risky?]: No
[Why is the change risky/not risky?]: only affects deep links which are used by Leanplum only
[String changes made/needed]:
Attachment #8915940 - Flags: approval-mozilla-beta?
This patch hasn't landed on trunk yet, which is usually something we require before uplifting to Beta. Also, the attached patch isn't in a landable form (wrong diff format & no commit information included).
Thanks for the info Ryan!

Vlad get you get Nevin's original patch in landable form and I will re-review and hopefully we can get it landed appropriately?
Flags: needinfo?(rkothari)
Sorry, I could have been more helpful in my last reply too. Running |./mach mercurial-setup| should take care of getting Mercurial properly configured. Then it's just a matter of giving it a proper commit message and re-exporting.
Thank you Ryan, I have re-configured my mercurial and re-exported the patch through intellij from the current revision, hopefully it's in landable form now
Attachment #8915940 - Attachment is obsolete: true
Attachment #8915940 - Flags: approval-mozilla-beta?
Flags: needinfo?(vlad.baicu)
Attachment #8964600 - Flags: review?(sdaswani)
Attachment #8963695 - Attachment is obsolete: true
Comment on attachment 8964600 [details] [diff] [review]
Bug 1399388 - Only when new page is exactly about:home will be treated as default home page

Looks like you've got the correct diff format now, but the patch is still missing commit information (author, commit message, etc).
Flags: needinfo?(vlad.baicu)
Attachment #8964600 - Flags: feedback-
Sorry about that, re-created it and added Nevin as an author.

The commit info is missing a MozReview commit id, currently I don't have access.
Attachment #8964600 - Attachment is obsolete: true
Attachment #8964600 - Flags: review?(sdaswani)
Flags: needinfo?(vlad.baicu)
Attachment #8964625 - Flags: review?(sdaswani)
Comment on attachment 8964625 [details] [diff] [review]
Bug 1399388 Only when new page is exactly about:home will be treated as default home page

The MozReview commit ID is fine to not have. Looks good!
Attachment #8964625 - Flags: feedback+
Do I need to do anything here Vlad?
Flags: needinfo?(vlad.baicu)
(In reply to :sdaswani from comment #27)
> Thanks for the info Ryan!
> 
> Vlad get you get Nevin's original patch in landable form and I will
> re-review and hopefully we can get it landed appropriately?

I have added the review tag as a response to the question in order to flag that the patch is in landable form and can go further through the procedures.
Flags: needinfo?(vlad.baicu)
Attachment #8964625 - Flags: review?(sdaswani) → review+
Comment on attachment 8915940 [details]
Bug 1399388 - Only when new page is exactly 'about:home' will be treated as default home page.

Approval Request Comment
[Feature/Bug causing the regression]: Our deep linking caused the regression. 
[User impact if declined]: Users would be taken to the wrong screen when they get leanplum messages.
[Is this code covered by automated tests?]: No.
[Has the fix been verified in Nightly?]: I don't think so, as I'm not if it's landed in nightly yet? But the devs tested it against local builds.
[Needs manual test from QE? If yes, steps to reproduce]: Open a bookmarks deep link, and the bookmarks page should be opened, instead of top sites.
[List of other uplifts needed for the feature/fix]: None.
[Is the change risky?]: No.
[Why is the change risky/not risky?]: It's a small fix and it's only ever exercised when a deep link is followed.
[String changes made/needed]: None.
Attachment #8915940 - Flags: approval-mozilla-beta?
Attachment #8915940 - Flags: checkin?
Keywords: checkin-needed
Attachment #8915940 - Flags: checkin?
Attachment #8915940 - Flags: approval-mozilla-beta?
Attachment #8964625 - Flags: approval-mozilla-beta?
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/34bc360b2860
Only when new page is exactly 'about:home' will be treated as default home page. r=sdaswani
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/34bc360b2860
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment on attachment 8964625 [details] [diff] [review]
Bug 1399388 Only when new page is exactly about:home will be treated as default home page

fennec leanplum fix for beta60
Attachment #8964625 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.