Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

Tie leanplum to Telemetry toggle #945

Closed
boek opened this issue Mar 11, 2019 · 14 comments
Closed

Tie leanplum to Telemetry toggle #945

boek opened this issue Mar 11, 2019 · 14 comments

Comments

@boek
Copy link
Contributor

boek commented Mar 11, 2019

┆Issue is synchronized with this Jira Task

@boek boek added the size S label Mar 11, 2019
@boek boek added this to the Sprint 3-1 (Milestone 3) milestone Mar 11, 2019
@boek boek self-assigned this Mar 11, 2019
boek added a commit to boek/fenix that referenced this issue Mar 11, 2019
@ghost ghost added in progress labels Mar 11, 2019
boek added a commit to boek/fenix that referenced this issue Mar 11, 2019
@boek boek closed this as completed in dae42a7 Mar 11, 2019
@ghost ghost removed in progress labels Mar 11, 2019
@bsurd
Copy link

bsurd commented Mar 15, 2019

Tested this and Leanplum notifications are still being sent after disabling the Telemetry toggle on all devices.

I've set leanplum to send a notification whenever the app was started or resumed.

Devices & build:

  • Samsung Galaxy Note 8 (Android 9)
  • Samsung Galaxy S8 (Android 9)
  • Nexus 5 (Android 6.0.1)
  • Build: 1.0.1911 (#10731207)
  • log.txt
  • Video

Re-opening the issue.

@bsurd bsurd reopened this Mar 15, 2019
@bsurd
Copy link

bsurd commented Mar 15, 2019

Is blocking #864.

@boek
Copy link
Contributor Author

boek commented Mar 19, 2019

@bsurd #1042 should fix this going forward

@boek boek closed this as completed Mar 19, 2019
@boek boek added the eng:qa:needed QA Needed label Mar 19, 2019
@boek boek reopened this Mar 19, 2019
@boek boek closed this as completed Mar 20, 2019
@bsurd
Copy link

bsurd commented Mar 27, 2019

Checked on several devices and the issue is still reproducing, after disabling the telemetry switch I am still receiving notifications. Tried starting the app with the toggle disabled by default and notifications were still received. Only when pausing the notification in the dashboard did I not receive them anymore.

Please let me know if you need any other information that could be of help.

Re-opening.

@bsurd bsurd reopened this Mar 27, 2019
@vesta0 vesta0 removed the size S label Mar 27, 2019
@boek
Copy link
Contributor Author

boek commented Mar 27, 2019

@bsurd unfortunately Leanplum doesn't offer us a way to stop the service once it has been started in the app. So the grand effect only happens the next time the application is run. When you disable the telemetry toggle in the app we do stop sending events immediately. The next time you start the app Leanplum won't be spun up.

@boek
Copy link
Contributor Author

boek commented Mar 27, 2019

@vesta0 @bbinto would love some input on if this behavior is OK.

@kbrosnan
Copy link
Contributor

This breaks expectation of the toggle. The standard behavior on Android is that a toggle takes effect immediately.

@boek
Copy link
Contributor Author

boek commented Mar 27, 2019

@kbrosnan The toggle is working immediately from the aspect of collecting Telemetry. This is referring to using another feature within Leanplum which is sending a push / message to a subset of users which technically still works because Leanplum has been initialized.

The other solution we could do is force-restart the app when the user clicks that toggle which isn't a great experience either. Do you know what Fennec does in this case?

@bbinto
Copy link
Contributor

bbinto commented Mar 27, 2019

@kbrosnan - do you know how this was implemented for Fennec, I assume that would have the same issue, no?

@bsurd
Copy link

bsurd commented Mar 28, 2019

Petru-Mugurel Lingurar could clarify the situation since he worked on Leanplum on Fennec. From my knowledge, it's possible to stop the notifications as well. There are some explanations here.

@Mugurell
Copy link
Contributor

As Bogdan said on Fennec we do have the option to stop Leanplum altogether, something that was implemented in bug 1454686.

While Leanplum doesn't officially expose APIs for stopping a currently running session we used some internal APIs (public methods) to achieve what we wanted: stop all notifications, in-app banners, telemetry and events tracking - Fennec changeset

I checked with Fenix and I see that here Leanplum is used as an external dependency while on Fennec we actually include the code inside and sprinkle it with our modifications. One important such modification is making Leanplum#stop() public.
Otherwise, Leanplum#setIsTestModeEnabled() - the one method that basically disables Leanplum is accessibile in Fenix also.

@vesta0 vesta0 added P1 Current sprint and removed P1 Current sprint labels Apr 1, 2019
@bsurd bsurd removed the eng:qa:needed QA Needed label Apr 1, 2019
@bsurd
Copy link

bsurd commented Apr 1, 2019

Removing the QA needed flag, please re-add as soon as this is ready for testing.

@bsurd bsurd moved this from Ready for QA to Ready for Dev in Fenix Sprint Kanban Apr 1, 2019
boek added a commit to boek/fenix that referenced this issue Apr 3, 2019
boek added a commit to boek/fenix that referenced this issue Apr 3, 2019
boek added a commit to boek/fenix that referenced this issue Apr 3, 2019
@boek boek added the eng:qa:needed QA Needed label Apr 4, 2019
@project-bot project-bot bot moved this from Ready for Dev to Ready for QA in Fenix Sprint Kanban Apr 4, 2019
@boek
Copy link
Contributor Author

boek commented Apr 4, 2019

Thanks for the info @Mugurell!

@bsurd This should be working now!

@boek boek closed this as completed Apr 4, 2019
@ghost ghost removed the P1 Current sprint label Apr 4, 2019
@project-bot project-bot bot moved this from Ready for QA to Done in Fenix Sprint Kanban Apr 4, 2019
@bsurd
Copy link

bsurd commented Apr 5, 2019

Can confirm, everything works as intended now. Verified on several devices and didn't encounter any other problems.

@bsurd bsurd added eng:qa:verified QA Verified and removed eng:qa:needed QA Needed labels Apr 5, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants