Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix #6692 - Moving most of the refactored leanplum A/B test changes from v26.0 to master #6736

Merged
merged 4 commits into from
Jun 16, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 36 additions & 6 deletions Client/Application/LeanplumIntegration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,18 @@ enum LPSetupType: String {
case none
}

enum LPState {
case disabled
case willStart
case started(startedState: LPStartedState)
}

enum LPStartedState {
case firstRun
case secondRun
case normalRun
}

class LeanPlumClient {
static let shared = LeanPlumClient()

Expand All @@ -118,14 +130,19 @@ class LeanPlumClient {
private var prefs: Prefs? { return profile?.prefs }
private var enabled: Bool = true
private var setupType: LPSetupType = .none
// Closure delegate for when leanplum has finished starting-up
var finishedStartingLeanplum: (() -> Void)?
// Comes from LeanPlum, used to show device ID in debug menu and finding user in LP dashboard"
var leanplumDeviceId: String? {
return Leanplum.deviceId()
}
// Used for sentry logging to know about the current state of leanplum execution
var lpState: LPState = .disabled
// This defines an external Leanplum varible to enable/disable FxA prepush dialogs.
// The primary result is having a feature flag controlled by Leanplum, and falling back
// to prompting with native push permissions.
private var useFxAPrePush = LPVar.define("useFxAPrePush", with: false)
var enablePocketVideo = LPVar.define("pocketVideo", with: false)

// var introScreenVars = LPVar.define("IntroScreen", with: IntroCard.defaultCards().compactMap({ $0.asDictonary() }))

private func isPrivateMode() -> Bool {
// Need to be run on main thread since isInPrivateMode requires to be on the main thread.
assert(Thread.isMainThread)
Expand All @@ -135,7 +152,7 @@ class LeanPlumClient {
func isLPEnabled() -> Bool {
return enabled && Leanplum.hasStarted()
}

func lpSetupType() -> LPSetupType {
return setupType
}
Expand All @@ -148,6 +165,10 @@ class LeanPlumClient {
self.profile = profile
}

func forceVariableUpdate() {
Leanplum.forceContentUpdate()
}

func recordSyncedClients(with profile: Profile?) {
guard let profile = profile as? BrowserProfile else {
return
Expand Down Expand Up @@ -194,17 +215,26 @@ class LeanPlumClient {

self.setupCustomTemplates()

lpState = .willStart
Leanplum.start(withUserId: nil, userAttributes: attributes, responseHandler: { _ in
self.track(event: .openedApp)

assert(Thread.isMainThread)
self.lpState = .started(startedState: .normalRun)
// https://docs.leanplum.com/reference#callbacks
// According to the doc all variables should be synced when lp start finishes
// Relying on this fact and sending the updated AB test variable
self.finishedStartingLeanplum?()
// We need to check if the app is a clean install to use for
// preventing the What's New URL from appearing.
if self.prefs?.intForKey(PrefsKeys.IntroSeen) == nil {
self.prefs?.setString(AppInfo.appVersion, forKey: LatestAppVersionProfileKey)
self.track(event: .firstRun)
self.lpState = .started(startedState: .firstRun)
} else if self.prefs?.boolForKey("SecondRun") == nil {
self.prefs?.setBool(true, forKey: "SecondRun")
self.track(event: .secondRun)
self.lpState = .started(startedState: .secondRun)
}

self.checkIfAppWasInstalled(key: PrefsKeys.HasFocusInstalled, isAppInstalled: self.focusInstalled(), lpEvent: .downloadedFocus)
Expand Down Expand Up @@ -301,8 +331,8 @@ class LeanPlumClient {
func getSettings() -> LPSettings? {
let bundle = Bundle.main
guard let appId = bundle.object(forInfoDictionaryKey: LPAppIdKey) as? String, !appId.isEmpty,
let productionKey = bundle.object(forInfoDictionaryKey: LPProductionKeyKey) as? String, !productionKey.isEmpty,
let developmentKey = bundle.object(forInfoDictionaryKey: LPDevelopmentKeyKey) as? String, !developmentKey.isEmpty else {
let productionKey = bundle.object(forInfoDictionaryKey: LPProductionKeyKey) as? String, !productionKey.isEmpty,
let developmentKey = bundle.object(forInfoDictionaryKey: LPDevelopmentKeyKey) as? String, !developmentKey.isEmpty else {
return nil
}
return LPSettings(appId: appId, developmentKey: developmentKey, productionKey: productionKey)
Expand Down
47 changes: 4 additions & 43 deletions Client/Frontend/Browser/BrowserViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -463,7 +463,6 @@ class BrowserViewController: UIViewController {

// Setup onboarding user research for A/B testing
onboardingUserResearch = OnboardingUserResearch()
onboardingUserResearch?.lpVariableObserver()
}

fileprivate func setupConstraints() {
Expand Down Expand Up @@ -2004,57 +2003,19 @@ extension BrowserViewController {
}

private func onboardingUserResearchHelper(_ alwaysShow: Bool = false) {
// Condition: Want to see our 1st time launched onboarding again
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All this now resides in the onboarding user research file:

Client/UserResearch/OnboardingUserResearch.swift
https://github.com/mozilla-mobile/firefox-ios/pull/6736/files#diff-e716579b778d971b97c24f89b7163e0c

// Our boolean variable shouldShow is used to present the onboarding
// that was presented to the user during first launch
if alwaysShow {
showProperIntroVC()
return
}
// Condition: Leanplum is disabled
// If leanplum is not enabled then we set the value of onboarding research to true
// True = .variant 1 which is our default Intro View
// False = .variant 2 which is our new Intro View that we are A/B testing against
// and get that from the server
guard LeanPlumClient.shared.getSettings() != nil else {
self.onboardingUserResearch?.updateValue(value: true)
showProperIntroVC()
return
}
// Condition: Update from leanplum server
// Get the A/B test variant from leanplum server
// and update onboarding user reasearch
onboardingUserResearch?.updatedLPVariables = {(lpVariable) -> () in
self.onboardingUserResearch?.updatedLPVariables = nil
print("lp Variable from server \(String(describing: lpVariable?.boolValue()))")
self.onboardingUserResearch?.updateTelemetry()
self.onboardingUserResearch?.updateValue(value: lpVariable?.boolValue() ?? true)
self.showProperIntroVC()
}
// Conditon: Leanplum server too slow
// We don't want our users to be stuck on Onboarding
// Wait 2 second and update the onboarding research variable
// with true (True = .variant 1)
// Ex. Internet connection is unstable due to which
// leanplum isn't loading or taking too much time
DispatchQueue.main.asyncAfter(deadline: .now() + 2) {
guard self.onboardingUserResearch?.updatedLPVariables != nil else {
return
}
Sentry.shared.send(message: "Failed to fetch A/B test variables from LP")
self.onboardingUserResearch?.updatedLPVariables = nil
self.onboardingUserResearch?.updateValue(value: true)
// Setup user research closure and observer to fetch the updated LP Variables
onboardingUserResearch?.updatedLPVariable = {
Copy link
Contributor Author

@nbhasin2 nbhasin2 Jun 3, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First We setup the delegate

self.showProperIntroVC()
}
onboardingUserResearch?.lpVariableObserver()
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Second we call the observers so that closures can be called

}

private func showProperIntroVC() {
// The onboarding screen type should always exist after
// the screen is presented for the 1st time
guard let onboardingScreenType = self.onboardingUserResearch?.onboardingScreenType else {
return
}
let introViewController = IntroViewControllerV2(onboardingType: onboardingScreenType)
let introViewController = IntroViewControllerV2()
introViewController.didFinishClosure = { controller, fxaLoginFlow in
self.profile.prefs.setInt(1, forKey: PrefsKeys.IntroSeen)
controller.dismiss(animated: true) {
Expand Down
2 changes: 1 addition & 1 deletion Client/Frontend/Intro/IntroScreenWelcomeViewV2.swift
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ class IntroScreenWelcomeViewV2: UIView, CardTheme {
return button
}()
// Welcome card items share same type of label hence combining them into a
// struct so we can reuse it
// struct so we can reuse it
private struct WelcomeUICardItem {
var title: String
var description: String
Expand Down
6 changes: 5 additions & 1 deletion Client/Frontend/Intro/IntroViewModelV2.swift
Original file line number Diff line number Diff line change
Expand Up @@ -37,6 +37,10 @@ class IntroViewModelV2 {
// Initializer
init() {
onboardingResearch = OnboardingUserResearch()
screenType = onboardingResearch?.onboardingScreenType
// Case: Older user who has updated to a newer version and is not a first time user
// When user updates from a version of app which didn't have the
// user research onboarding screen type and is trying to Always Show the screen
// from Show Tour, we default to our .version1 of the screen
screenType = onboardingResearch?.onboardingScreenType ?? .versionV1
}
}
26 changes: 21 additions & 5 deletions Client/Frontend/Settings/AppSettingsOptions.swift
Original file line number Diff line number Diff line change
Expand Up @@ -567,19 +567,35 @@ class ToggleOnboarding: HiddenSetting {

class LeanplumStatus: HiddenSetting {
let lplumSetupType = LeanPlumClient.shared.lpSetupType()

override var title: NSAttributedString? {
return NSAttributedString(string: "Leamplum Status: \(lplumSetupType) | Started: \(LeanPlumClient.shared.isRunning())", attributes: [NSAttributedString.Key.foregroundColor: UIColor.theme.tableView.rowText])
return NSAttributedString(string: "LP Setup: \(lplumSetupType) | Started: \(LeanPlumClient.shared.isRunning()) | Device ID: \(LeanPlumClient.shared.leanplumDeviceId ?? "")", attributes: [NSAttributedString.Key.foregroundColor: UIColor.theme.tableView.rowText])
}

override func onClick(_ navigationController: UINavigationController?) {
copyLeanplumDeviceIDAndPresentAlert(by: navigationController)
}

func copyLeanplumDeviceIDAndPresentAlert(by navigationController: UINavigationController?) {
let alertTitle = Strings.SettingsCopyAppVersionAlertTitle
let alert = AlertController(title: alertTitle, message: nil, preferredStyle: .alert)
UIPasteboard.general.string = "\(LeanPlumClient.shared.leanplumDeviceId ?? "")"
navigationController?.topViewController?.present(alert, animated: true) {
DispatchQueue.main.asyncAfter(deadline: .now() + 0.5) {
alert.dismiss(animated: true)
}
}
}
}

class SetOnboardingV2: HiddenSetting {
class ClearOnboardingABVariables: HiddenSetting {
override var title: NSAttributedString? {
return NSAttributedString(string: NSLocalizedString("Debug: Set onboarding type to v2", comment: "Debug option"), attributes: [NSAttributedString.Key.foregroundColor: UIColor.theme.tableView.rowText])
// If we are running an A/B test this will also fetch the A/B test variables from leanplum. Re-open app to see the effect.
return NSAttributedString(string: NSLocalizedString("Debug: Clear onboarding AB variables", comment: "Debug option"), attributes: [NSAttributedString.Key.foregroundColor: UIColor.theme.tableView.rowText])
}

override func onClick(_ navigationController: UINavigationController?) {
OnboardingUserResearch().onboardingScreenType = .versionV2
settings.profile.prefs.removeObjectForKey(PrefsKeys.IntroSeen)
OnboardingUserResearch().onboardingScreenType = nil
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -139,7 +139,10 @@ class AppSettingsTableViewController: SettingsTableViewController {
ChangeToChinaSetting(settings: self),
ToggleOnboarding(settings: self),
LeanplumStatus(settings: self),
ShowEtpCoverSheet(settings: self)
ShowEtpCoverSheet(settings: self),
ToggleOnboarding(settings: self),
LeanplumStatus(settings: self),
ClearOnboardingABVariables(settings: self)
])]

return settings
Expand Down
62 changes: 46 additions & 16 deletions Client/UserResearch/OnboardingUserResearch.swift
Original file line number Diff line number Diff line change
Expand Up @@ -7,25 +7,35 @@ import Leanplum
import Shared

struct LPVariables {
static var showOnboardingScreen = LPVar.define("showOnboardingScreen", with: true)
// Variable Used for AA test
static var showOnboardingScreenAA = LPVar.define("showOnboardingScreen", with: true)
// Variable Used for AB test
static var showOnboardingScreenAB = LPVar.define("showOnboardingScreen_2", with: true)
}

// For LP variable below is the convention we follow
// True = Current Onboarding Screen
// False = New Onboarding Screen
enum OnboardingScreenType: String {
case versionV1 // Default
case versionV2 // New version 2
case versionV1
case versionV2

static func from(boolValue: Bool) -> OnboardingScreenType {
return boolValue ? .versionV1 : .versionV2
}
}

class OnboardingUserResearch {
// Closure delegate
var updatedLPVariables: ((LPVar?) -> Void)?
var updatedLPVariable: (() -> Void)?
// variable
var lpVariable: LPVar?
// Constants
private let onboardingScreenTypeKey = "onboardingScreenTypeKey"
// Saving user defaults
private let defaults = UserDefaults.standard
// Publicly accessible onboarding screen type
var onboardingScreenType:OnboardingScreenType? {
var onboardingScreenType: OnboardingScreenType? {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the setter be private?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Making it private(set) won't allow HiddenSettings to clear AB variables.
I can put a specific method to clear those values in this class if you'd like that. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

np, public is ok, i didn't know it was used elsewhere

set(value) {
if value == nil {
defaults.removeObject(forKey: onboardingScreenTypeKey)
Expand All @@ -42,23 +52,43 @@ class OnboardingUserResearch {
}

// MARK: Initializer
init(lpVariable: LPVar? = LPVariables.showOnboardingScreen) {
init(lpVariable: LPVar? = LPVariables.showOnboardingScreenAB) {
self.lpVariable = lpVariable
}

// MARK: public
func lpVariableObserver() {
Leanplum.onVariablesChanged {
self.updatedLPVariables?(self.lpVariable)
// Condition: Leanplum is disabled; use default intro view
guard LeanPlumClient.shared.getSettings() != nil else {
self.onboardingScreenType = .versionV1
self.updatedLPVariable?()
return
}
// Condition: A/B test variables from leanplum server
LeanPlumClient.shared.finishedStartingLeanplum = {
let showScreenA = LPVariables.showOnboardingScreenAB?.boolValue()
LeanPlumClient.shared.finishedStartingLeanplum = nil
self.updateTelemetry()
let screenType = OnboardingScreenType.from(boolValue: (showScreenA ?? true))
self.onboardingScreenType = screenType
self.updatedLPVariable?()
}
// Condition: Leanplum server too slow; Show default onboarding.
DispatchQueue.main.asyncAfter(deadline: .now() + 2) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: Condition spelling
One liner comment instead of lines 75-80: // Condition: Leanplum server too slow; Show default onboarding.

guard LeanPlumClient.shared.finishedStartingLeanplum != nil else {
return
}
let lpStartStatus = LeanPlumClient.shared.lpState
var lpVariableValue: OnboardingScreenType = .versionV1
// Condition: LP has already started but we missed onStartLPVariable callback
if case .started(startedState: _) = lpStartStatus , let boolValue = LPVariables.showOnboardingScreenAB?.boolValue() {
lpVariableValue = boolValue ? .versionV1 : .versionV2
self.updateTelemetry()
}
self.updatedLPVariable = nil
self.onboardingScreenType = lpVariableValue
self.updatedLPVariable?()
}
}

func updateValue(value: Bool) {
// For LP variable below is the convention
// we are going to follow
// True = Current Onboarding Screen
// False = New Onboarding Screen
onboardingScreenType = value ? .versionV1 : .versionV2
}

func updateTelemetry() {
Expand Down
1 change: 1 addition & 0 deletions Shared/SentryIntegration.swift
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ public enum SentryTag: String {
case general = "General"
case tabManager = "TabManager"
case bookmarks = "Bookmarks"
case leanplum = "Leanplum"
}

public class Sentry {
Expand Down