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

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect, P1)

Description

[Sorina Florean [:sflorean]]

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.

Comment 1

[Nevin Chen(Not active on Bugzilla)]

Hi Jing Wei
Do you know if we change the HomConfig

Comment 2

[Nevin Chen(Not active on Bugzilla)]

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!

Comment 3

[Nevin Chen(Not active on Bugzilla)]

Turns out it's a regression from a late landing patch bug 1367069.
See https://bugzilla.mozilla.org/show_bug.cgi?id=1367069#c8

Comment 4

[Nevin Chen(Not active on Bugzilla)]

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.

Comment 5

[Nevin Chen(Not active on Bugzilla)]

I just set the flag for the first time. Please correct me if I'm wrong

Comment 6

[Jim Mathies [:jimm]]

Can we resolve this WFM now that bug 1367069 has been backed out?

Comment 7

[Nevin Chen(Not active on Bugzilla)]

I'll fix this for 57 (hope I can get the patch uplift to 57 beta this week)

Comment 8

[Nevin Chen(Not active on Bugzilla)]

(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!

Comment 9

[Sorina Florean [:sflorean]]

(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.

Comment 10

[Nevin Chen(Not active on Bugzilla)]

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.

Comment 11

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1399388 - Only when new page is exactly 'about:home' will be treated as default home page.

Review commit: https://reviewboard.mozilla.org/r/187204/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187204/

Comment 12

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8915940 [details]
Bug 1399388 - Only when new page is exactly 'about:home' will be treated as default home page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187204/diff/1-2/

Comment 13

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8915940 [details]
Bug 1399388 - Only when new page is exactly 'about:home' will be treated as default home page.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187204/diff/2-3/

Comment 14

[Wesly Huang (EPM)]

since bug 1367069 is backed out this is no regression here and not necessary as P1.

Comment 15

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Susheel, can you ask someone who uses switchboard how important this is?

Comment 16

[: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.

Comment 17

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

[triage] Bulk edit: product thinks leanplum is high priority: P1 rank 1.

Comment 18

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Product team would like this fixed in 60, tracked as blocking to ensure it's closely monitored.

Comment 19

[Vlad Baicu]

Attachment: Bug 1399388 - Leanplum deeplink differentiation rework

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.

Comment 20

[:sdaswani]

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.

Comment 21

[Vlad Baicu]

Attachment: Bug 1399388 - Leanplum deeplink check for dev builds

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.

Comment 22

[:sdaswani]

Vlad I'm a bit confused about this latest patch - does it solve the issue cited? We can discuss live tomorrow.

Comment 23

[: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.

Comment 24

[:sdaswani]

Ritu, can we get this uplifted to 60 today? It's a small patch and the contractors and I confirmed it works.

Comment 25

[:sdaswani]

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]:

Comment 26

[Ryan VanderMeulen [:RyanVM]]

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).

Comment 27

[:sdaswani]

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?

Comment 28

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 29

[Vlad Baicu]

Attachment: Bug 1399388 - Only when new page is exactly about:home will be treated as default home page

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

Comment 30

[Ryan VanderMeulen [:RyanVM]]

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).

Comment 31

[Vlad Baicu]

Attachment: Bug 1399388 Only when new page is exactly about:home will be treated as default home page

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.

Comment 32

[Ryan VanderMeulen [:RyanVM]]

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!

Comment 33

[:sdaswani]

Do I need to do anything here Vlad?

Comment 34

[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.

Comment 35

[:sdaswani]

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.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
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

Comment 37

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/34bc360b2860

Comment 38

[Julien Cristau [:jcristau]]

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

Comment 39

[Cristian Brindusan [:cbrindusan]]

https://hg.mozilla.org/releases/mozilla-beta/rev/e595eb784023