1390454 - Apply Banner fix for Leanplum SDK

Status: RESOLVED FIXED

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

Comment 1

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1390454 - Apply Banner fix for Leanplum SDK.

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

Comment 2

[Nevin Chen(Not active on Bugzilla)]

After testing this patch. I'v found that most of the Banner bugs are fixed( click through, cancel not working ...etc)
The last problem is that Photon colored the status bar white. Using Banner will make the status bar black . And after banners disappear it change back to white again. 

I guess there should be some customization at our end.

Comment 3

[Nevin Chen(Not active on Bugzilla)]

Attachment: Add multidex so I can test Leanplum locally

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

Comment 4

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8897349 [details]
Bug 1390454 - Apply Banner fix for Leanplum SDK.

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

Comment 5

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8897349 [details]
Bug 1390454 - Apply Banner fix for Leanplum SDK.

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

Comment 6

[Nevin Chen(Not active on Bugzilla)]

I don't see any change per data we sent to Leanplum.
Most of the change is Leanplum's implementation in Banner, cache inhancement, added utility classes,bug fix ,and PushNotification Channel API upgrade (to API 26) so data review is not needed.

Roughly tested with push notification and in-app diaglog, they are working. But banners needs some tweaking to match our photon theme.

Since underlying API has changed, all related behavior should be verified again. So mutliple phase QA- debug cycle is expected.

Comment 7

[Nevin Chen(Not active on Bugzilla)]

Attachment: Bug 1390454 - Make Banner dialog status bar transparent.

I didn't check if currently the app is using dark theme(LightweightTheme) or not.
Cause that requires some hacky injection to let Leanplum SDK knows the status bar corlor or dark theme or not.

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

Comment 8

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8928083 [details]
Bug 1390454 - Make Banner dialog status bar transparent.

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

Comment 9

[Nevin Chen(Not active on Bugzilla)]

Comment on attachment 8897349 [details]
Bug 1390454 - Apply Banner fix for Leanplum SDK.

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

Comment 10

[Nevin Chen(Not active on Bugzilla)]

Hi Braynt
Just to let you know that the changes here can't let us:
1. Change the width / height of the banner cause it's a new API (we didn't update entire SDK)
2. Customize the look and feel of the Banner

Comment 11

[Max Liu [:maliu]]

Comment on attachment 8928083 [details]
Bug 1390454 - Make Banner dialog status bar transparent.

https://reviewboard.mozilla.org/r/199320/#review204756

Comment 12

[Max Liu [:maliu]]

Comment on attachment 8897349 [details]
Bug 1390454 - Apply Banner fix for Leanplum SDK.

https://reviewboard.mozilla.org/r/168662/#review204758

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/8397dc953ce6
Make Banner dialog status bar transparent. r=maliu
https://hg.mozilla.org/integration/autoland/rev/356df0634637
Apply Banner fix for Leanplum SDK. r=maliu

Comment 14

[Bryant Mao [:bryantmao]]

(In reply to Nevin Chen [:nechen] from comment #10)
> Hi Braynt
> Just to let you know that the changes here can't let us:
> 1. Change the width / height of the banner cause it's a new API (we didn't
> update entire SDK)
> 2. Customize the look and feel of the Banner
so that means we can't change the color and the height like the mock do here? https://mozilla.invisionapp.com/share/YHEHZ5CMA#/screens/264324424

Comment 15

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/8397dc953ce6
https://hg.mozilla.org/mozilla-central/rev/356df0634637

Comment 16

[Barbara Bermes [:barbara] - NI please!]

Susheel and Mike, would you mind confirming if the banner implementation made it into Fennec (as far as I know jcollings mentioned it's still something that needs to be implemented).

Product would like to shift this to a P1.

Comment 17

[:sdaswani]

Jean, can you please send a banner to my Firefox Nightly device and see if it meets the implementation specs you require? Gracias!

Comment 18

[Jean Collings]

(In reply to Susheel Daswani from comment #17)
> Jean, can you please send a banner to my Firefox Nightly device and see if
> it meets the implementation specs you require? Gracias!

I just sent you a banner test (will show when you open the app) and a push notification test. Can you please confirm if you see them (maybe take screenshots) and if it works as it should? Thanks!

Comment 19

[:sdaswani]

Here is what I got. :) https://photos.app.goo.gl/Zy08qECBQNTSnR8u2

How does it look Jean?

FYI this is version 60.0a1.

Comment 20

[Jean Collings]

(In reply to Susheel Daswani from comment #19)
> Here is what I got. :) https://photos.app.goo.gl/Zy08qECBQNTSnR8u2
> 
> How does it look Jean?
> 
> FYI this is version 60.0a1.

Looks great :) Thanks for testing and the screenshots.

Comment 21

[:sdaswani]

Barbara please put this on your master list.

Comment 22

[:sdaswani]

Jean, this needs to be fixed for 60, right? Also, what exactly needs to be fixed?

Comment 23

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

Another one that the product team wants uplifted in 60, tracked as blocking.

Comment 24

[Jean Collings]

(In reply to Susheel Daswani from comment #22)
> Jean, this needs to be fixed for 60, right? Also, what exactly needs to be
> fixed?

I thought we did not have the ability to change the color or height of the banner but we can (just tested it). However, I noticed that when I hit the banner to a few different deeplinks, nothing happens - the banner disappears but does not take me to where I am supposed to go. I tried it with a generic url like https://mozilla.org and that works so it must be the deeplinks.

Can we QA if these links are right?

For Fx58 and higher:
firefox://default_browser?uid={{User ID}}
firefox://sign_up?uid={{User Id}}
firefox://preferences_search?uid={{User Id}}
firefox://save_as_pdf?uid={{User Id}}
firefox://bookmark_list?uid={{User Id}}
firefox://history_list?uid={{User Id}}
firefox://preferences?uid={{User Id}}
firefox://preferences_privacy?uid={{User Id}}
firefox://preferences_notifications?uid={{User Id}}
firefox://preferences_accessibility?uid={{User Id}}
firefox://preferences_accessibility?uid={{User Id}}

Comment 25

[Jean Collings]

I reviewed the deeplinks and they are now working if you input capitalized "ID"

This bug is resolved/fixed.

Comment 26

[Julien Cristau [:jcristau]]

Very confused as to the status here as this was reopened several times without explanation, but from comment 25 I understand that there's no work to do here for 60 so clearing corresponding tracking flags.

Comment 27

[:sdaswani]

(In reply to Julien Cristau [:jcristau] from comment #26)
> Very confused as to the status here as this was reopened several times
> without explanation, but from comment 25 I understand that there's no work
> to do here for 60 so clearing corresponding tracking flags.

Sorry Julien, there was a miscommunication about how to make this work, so it was re-opened mistakenly. It's all good, thanks for removing those flags.