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

AB infrastructure #357

Merged
merged 10 commits into from
Sep 6, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions Podfile
Original file line number Diff line number Diff line change
Expand Up @@ -29,11 +29,10 @@ def all_pods
pod 'SnapKit', '4.0.0'

pod 'FirebaseCore', '5.1.0'
pod 'FirebaseAppIndexing', '1.2.0'
pod 'FirebaseMessaging', '3.1.0'
pod 'FirebaseAnalytics', '5.1.0'
pod 'FirebaseMessaging' , '3.1.0'
pod 'FirebaseAnalytics' , '5.1.0'
pod 'FirebaseRemoteConfig', '3.0.1'

pod 'Amplitude-iOS', '4.3.0'

pod 'AppsFlyerFramework', '4.8.8'
Expand Down
17 changes: 16 additions & 1 deletion Stepic.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -313,6 +313,8 @@
085290CB1FB5C4550053F22D /* ExplorePresenter.swift in Sources */ = {isa = PBXBuildFile; fileRef = 085290C51FB5C4550053F22D /* ExplorePresenter.swift */; };
085290CC1FB5C4550053F22D /* ExplorePresenter.swift in Sources */ = {isa = PBXBuildFile; fileRef = 085290C51FB5C4550053F22D /* ExplorePresenter.swift */; };
085290CD1FB5C4550053F22D /* ExplorePresenter.swift in Sources */ = {isa = PBXBuildFile; fileRef = 085290C51FB5C4550053F22D /* ExplorePresenter.swift */; };
0853A3E0213DC18F00931F72 /* ABTestService.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0853A3DF213DC18F00931F72 /* ABTestService.swift */; };
0853A3E2213E829D00931F72 /* ABSocialAuthString.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0853A3E1213E829D00931F72 /* ABSocialAuthString.swift */; };
085514EB1CFB09760080CB88 /* CellWebViewHelper.swift in Sources */ = {isa = PBXBuildFile; fileRef = 085514EA1CFB09760080CB88 /* CellWebViewHelper.swift */; };
0856E2562039769F00575394 /* AnalyticsEvents.swift in Sources */ = {isa = PBXBuildFile; fileRef = 08D035201D65A252003515C6 /* AnalyticsEvents.swift */; };
0858B7091CFF271200459A6A /* TagDetectionUtil.swift in Sources */ = {isa = PBXBuildFile; fileRef = 0858B7081CFF271200459A6A /* TagDetectionUtil.swift */; };
Expand Down Expand Up @@ -5260,6 +5262,8 @@
084F7AA91E76EF780088368A /* LastStep+CoreDataProperties.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = "LastStep+CoreDataProperties.swift"; sourceTree = "<group>"; };
084F7AAC1E775A210088368A /* LastStepsAPI.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = LastStepsAPI.swift; sourceTree = "<group>"; };
085290C51FB5C4550053F22D /* ExplorePresenter.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ExplorePresenter.swift; sourceTree = "<group>"; };
0853A3DF213DC18F00931F72 /* ABTestService.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ABTestService.swift; sourceTree = "<group>"; };
0853A3E1213E829D00931F72 /* ABSocialAuthString.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = ABSocialAuthString.swift; sourceTree = "<group>"; };
085514EA1CFB09760080CB88 /* CellWebViewHelper.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = CellWebViewHelper.swift; sourceTree = "<group>"; };
0858B7081CFF271200459A6A /* TagDetectionUtil.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = TagDetectionUtil.swift; sourceTree = "<group>"; };
0859AE941E4F19B600A0D206 /* FillBlanksQuizViewController.swift */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.swift; path = FillBlanksQuizViewController.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -5745,7 +5749,6 @@
2C2FD77E210735E000609621 /* LessonTableViewCell.xib */ = {isa = PBXFileReference; lastKnownFileType = file.xib; path = LessonTableViewCell.xib; sourceTree = "<group>"; };
2C2FD7832107448F00609621 /* LessonsService.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LessonsService.swift; sourceTree = "<group>"; };
2C2FD7852107453800609621 /* LessonsServiceImpl.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LessonsServiceImpl.swift; sourceTree = "<group>"; };
2C2FD7892107507200609621 /* LessonPlainObject.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LessonPlainObject.swift; sourceTree = "<group>"; };
2C2FD78C210753AD00609621 /* LessonsPresenterProtocol.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LessonsPresenterProtocol.swift; sourceTree = "<group>"; };
2C2FD78E210753CD00609621 /* LessonsPresenter.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LessonsPresenter.swift; sourceTree = "<group>"; };
2C2FD7902107540500609621 /* LessonsView.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = LessonsView.swift; sourceTree = "<group>"; };
Expand Down Expand Up @@ -7177,6 +7180,15 @@
name = Explore;
sourceTree = "<group>";
};
0853A3DE213D8F6100931F72 /* AB tests */ = {
isa = PBXGroup;
children = (
0853A3DF213DC18F00931F72 /* ABTestService.swift */,
0853A3E1213E829D00931F72 /* ABSocialAuthString.swift */,
);
name = "AB tests";
sourceTree = "<group>";
};
085514E91CFB09170080CB88 /* WebViewHelpers */ = {
isa = PBXGroup;
children = (
Expand Down Expand Up @@ -7909,6 +7921,7 @@
08DE94131B8C58AC00D278AB /* Stepic */ = {
isa = PBXGroup;
children = (
0853A3DE213D8F6100931F72 /* AB tests */,
08484EDF211AF41A0006266F /* Stories */,
2CE664DE20F5204D0082F3FE /* Downloader */,
088E73E92060124B00D458E3 /* ApiRequestRetrier.swift */,
Expand Down Expand Up @@ -14284,6 +14297,7 @@
0829B83C1E9D05AE009B4A84 /* Certificate+CoreDataProperties.swift in Sources */,
083540611CE5DD5000BDFEA5 /* NotificationReactionHandler.swift in Sources */,
088E73ED20614DFC00D458E3 /* UpdateRequestMaker.swift in Sources */,
0853A3E0213DC18F00931F72 /* ABTestService.swift in Sources */,
081A14FF20D963090016364E /* AmplitudeAnalyticsEvents.swift in Sources */,
08CA59EE1BBFC962008DC44D /* ButtonExtension.swift in Sources */,
082E35B020B5B14A006E28F9 /* StorageData.swift in Sources */,
Expand Down Expand Up @@ -14543,6 +14557,7 @@
0888D1091F1E42A000A16863 /* CodeElementsSize.swift in Sources */,
080AA2321EA024290079272F /* CoursesAPIPaginatedMock.swift in Sources */,
2C5DF14B1FED2C51003B1177 /* StepReversedCardView.swift in Sources */,
0853A3E2213E829D00931F72 /* ABSocialAuthString.swift in Sources */,
08DF78B91F5E0C9000AEEA85 /* StringHelper.swift in Sources */,
2C23C5DE1F6AB10800FC2B7C /* SocialAuthPresenter.swift in Sources */,
083F2B141E9D8EF800714173 /* CertificatesViewController.swift in Sources */,
Expand Down
27 changes: 27 additions & 0 deletions Stepic/ABSocialAuthString.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,27 @@
//
// ABSocialAuthString.swift
// Stepic
//
// Created by Ostrenkiy on 04.09.2018.
// Copyright © 2018 Alex Karpov. All rights reserved.
//

import Foundation
import UIKit

class ABSocialAuthString: ActiveABTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Давай помечать класс как final, если не планируется от него наследоваться

let ID = "ab_social_auth_string"
Copy link
Contributor

Choose a reason for hiding this comment

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

ID -> id (4.6 в стайлгайде)


let controlValue: String = NSLocalizedString("SignInTitleSocial", comment: "")
Copy link
Contributor

Choose a reason for hiding this comment

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

: String не нужно


func value(group: String) -> String? {
switch group {
case "control":
return controlValue
Copy link
Contributor

Choose a reason for hiding this comment

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

Предлагаю начать указывать явно self везде, как мы решили в стайлгайде. Все равно иначе придется кому-то потом терять время и править везде + так постепенно появится привычка

case "test":
return NSLocalizedString("SignInTitleSocialTest", comment: "")
default:
return nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Смысл делать value(group:) optional, если мы в сервисе везде пишем ?? test.controlValue?

Copy link
Contributor

Choose a reason for hiding this comment

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

А еще можно строки превратить в enum, где для всех неизвестных значений будет определяться кейс

}
}
}
53 changes: 53 additions & 0 deletions Stepic/ABTestService.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,53 @@
//
// ABTestService.swift
// Stepic
//
// Created by Ostrenkiy on 03.09.2018.
// Copyright © 2018 Alex Karpov. All rights reserved.
//

import Foundation

protocol ActiveABTest {
Copy link
Contributor

Choose a reason for hiding this comment

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

Почему Active? Есть другие?

associatedtype ValueType
var ID: String { get }
func value(group: String) -> ValueType?
var controlValue: ValueType { get }
Copy link
Contributor

Choose a reason for hiding this comment

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

Почему бы не добавить сюда еще var testValue: ValueType { get }? Тогда реализация value будет общая (просто switch по группе) и ее можно вынести в экстеншен к протоколу.

}

class ABTestService<T: ActiveABTest> {

var test: T

private let defaults = UserDefaults.standard

init(test: T) {
self.test = test
}

var value: T.ValueType {
if let resultGroup = RemoteConfig.shared.string(forKey: test.ID + "_result") {
Copy link
Contributor

Choose a reason for hiding this comment

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

if let resultGroup = RemoteConfig.shared.string(forKey: test.ID + "_result"), 
   !resultGroup.isEmpty {
    defaults.set(resultGroup, forKey: test.ID)
    return test.value(group: resultGroup) ?? test.controlValue
}

Без лишней вложенности.

if !resultGroup.isEmpty {
defaults.set(resultGroup, forKey: test.ID)
return test.value(group: resultGroup) ?? test.controlValue
}
}
if let group = defaults.value(forKey: test.ID) as? String {
return test.value(group: group) ?? test.controlValue
} else {
if RemoteConfig.shared.fetchComplete {
guard
let group = RemoteConfig.shared.string(forKey: test.ID),
Copy link
Contributor

Choose a reason for hiding this comment

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

Давай не будем изобретать новое форматирование длинных гвардов

let value = test.value(group: group)
else {
return test.controlValue
}
defaults.set(group, forKey: test.ID)
AnalyticsUserProperties.shared.setAmplitudeProperty(key: test.ID, value: group)
return value
} else {
return test.controlValue
}
}
}
}
2 changes: 1 addition & 1 deletion Stepic/AnalyticsUserProperties.swift
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ class AnalyticsUserProperties {

static let shared = AnalyticsUserProperties()

private func setAmplitudeProperty(key: String, value: Any?) {
func setAmplitudeProperty(key: String, value: Any?) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Кмк, лучше сделать так же как и для других проперти:

func setABProperty(test: ABTest.IDType, value: ABTest.Group) { 
    self.setAmplitudeProperty(...)
}

if let v = value {
Amplitude.instance().setUserProperties([key: v])
} else {
Expand Down
3 changes: 1 addition & 2 deletions Stepic/AppDelegate.swift
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@ import UIKit
import MediaPlayer
import FirebaseCore
import FirebaseMessaging
import FirebaseAppIndexing
import FirebaseInstanceID
import IQKeyboardManagerSwift
import SVProgressHUD
Expand Down Expand Up @@ -46,7 +45,7 @@ class AppDelegate: UIResponder, UIApplicationDelegate {
print("Could not initialize audio session")
}

FIRAppIndexing.sharedInstance().registerApp(Tokens.shared.firebaseId)
// FIRAppIndexing.sharedInstance().registerApp(Tokens.shared.firebaseId)
Ostrenkiy marked this conversation as resolved.
Show resolved Hide resolved

FBSDKApplicationDelegate.sharedInstance().application(application, didFinishLaunchingWithOptions: launchOptions)

Expand Down
8 changes: 7 additions & 1 deletion Stepic/RemoteConfig.swift
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@

import Foundation
import FirebaseRemoteConfig
import FirebaseInstanceID

enum RemoteConfigKeys: String {
case showStreaksNotificationTrigger = "show_streaks_notification_trigger"
Expand Down Expand Up @@ -86,9 +87,10 @@ class RemoteConfig {
}

private func fetchCloudValues() {
let fetchDuration: TimeInterval = 43200
var fetchDuration: TimeInterval = 43200
#if DEBUG
activateDebugMode()
fetchDuration = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

Почему не поле класса и не в activateDebugMode?

#endif
FirebaseRemoteConfig.RemoteConfig.remoteConfig().fetch(withExpirationDuration: fetchDuration) {
[weak self]
Expand All @@ -110,4 +112,8 @@ class RemoteConfig {
let debugSettings = RemoteConfigSettings(developerModeEnabled: true)
FirebaseRemoteConfig.RemoteConfig.remoteConfig().configSettings = debugSettings
}

func string(forKey key: String) -> String? {
Copy link
Contributor

Choose a reason for hiding this comment

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

Это конечно плохо, потому что тогда уж лучше переписать весь класс и сделать новый гибкий интерфейс. А получается, что какие-то ключи у нас обернуты в проперти, а какие-то мы дергаем без обертки 🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Если мы так будем переписывать каждый класс, в котором что-то не нравится, не будем по пол года релизиться.

return FirebaseRemoteConfig.RemoteConfig.remoteConfig().configValue(forKey: key).stringValue
}
}
6 changes: 2 additions & 4 deletions Stepic/SocialAuthHeaderView.swift
Original file line number Diff line number Diff line change
Expand Up @@ -13,9 +13,7 @@ class SocialAuthHeaderView: UICollectionReusableView {

@IBOutlet weak var titleLabel: StepikLabel!

override func awakeFromNib() {
super.awakeFromNib()

titleLabel.setTextWithHTMLString(NSLocalizedString("SignInTitleSocial", comment: ""))
func setup(title: String) {
titleLabel.setTextWithHTMLString(title)
}
}
8 changes: 7 additions & 1 deletion Stepic/SocialAuthPresenter.swift
Original file line number Diff line number Diff line change
Expand Up @@ -39,13 +39,15 @@ class SocialAuthPresenter {
var stepicsAPI: StepicsAPI
var authAPI: AuthAPI
var notificationStatusesAPI: NotificationStatusesAPI
var abSocialAuthService: ABTestService<ABSocialAuthString>

var pendingAuthProviderInfo: SocialProviderInfo?

init(authAPI: AuthAPI, stepicsAPI: StepicsAPI, notificationStatusesAPI: NotificationStatusesAPI, view: SocialAuthView) {
init(authAPI: AuthAPI, stepicsAPI: StepicsAPI, notificationStatusesAPI: NotificationStatusesAPI, abSocialAuthService: ABTestService<ABSocialAuthString>, view: SocialAuthView) {
self.authAPI = authAPI
self.stepicsAPI = stepicsAPI
self.notificationStatusesAPI = notificationStatusesAPI
self.abSocialAuthService = abSocialAuthService
Copy link
Contributor

Choose a reason for hiding this comment

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

Если у нас в модуле будет 10 A/B тестов, мы будем иметь 10 сервисов? Это, конечно, не очень и стоит подумать, как переделать так, чтобы было удобно.

self.view = view

// TODO: create NSNotification.Name extension
Expand Down Expand Up @@ -125,6 +127,10 @@ class SocialAuthPresenter {
}
}

var socialAuthHeaderString: String {
Copy link
Contributor

Choose a reason for hiding this comment

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

Проперти наверх

return abSocialAuthService.value
}

@objc private func didAuthCodeReceive(_ notification: NSNotification) {
print("social auth: auth code received")

Expand Down
8 changes: 5 additions & 3 deletions Stepic/SocialAuthViewController.swift
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ class SocialAuthViewController: UIViewController {

localize()

presenter = SocialAuthPresenter(authAPI: ApiDataDownloader.auth, stepicsAPI: ApiDataDownloader.stepics, notificationStatusesAPI: NotificationStatusesAPI(), view: self)
presenter = SocialAuthPresenter(authAPI: ApiDataDownloader.auth, stepicsAPI: ApiDataDownloader.stepics, notificationStatusesAPI: NotificationStatusesAPI(), abSocialAuthService: ABTestService(test: ABSocialAuthString()), view: self)
presenter?.update()

UIApplication.shared.statusBarStyle = UIStatusBarStyle.default
Expand Down Expand Up @@ -225,8 +225,10 @@ extension SocialAuthViewController: UICollectionViewDelegate, UICollectionViewDa

func collectionView(_ collectionView: UICollectionView, viewForSupplementaryElementOfKind kind: String, at indexPath: IndexPath) -> UICollectionReusableView {
if kind == UICollectionElementKindSectionHeader {
let header = collectionView.dequeueReusableSupplementaryView(ofKind: kind, withReuseIdentifier: SocialAuthHeaderView.reuseId, for: indexPath)
return header
if let header = collectionView.dequeueReusableSupplementaryView(ofKind: kind, withReuseIdentifier: SocialAuthHeaderView.reuseId, for: indexPath) as? SocialAuthHeaderView {
header.setup(title: presenter?.socialAuthHeaderString ?? "")
return header
}
}
return UICollectionReusableView()
}
Expand Down
3 changes: 3 additions & 0 deletions Stepic/en.lproj/Localizable.strings
Original file line number Diff line number Diff line change
Expand Up @@ -554,6 +554,9 @@ PersonalDeadline = "Personal deadline";
DeadlineModeQuestion = "How much time would you like to spend studying on this course?";
PersonalDeadlineWidgetSuggestionText = "Constant learning is a key to success. We can smartly create personal learning schedule for you, so you'll never forget to study!";

/* Sign in AB */
SignInTitleSocialTest = "<b>Contunue</b> with";
Copy link
Contributor

Choose a reason for hiding this comment

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

Continue


/* ExamEGERussian */
"ErrorMessage" = "Sorry, something went wrong: please try again later.";
"Ok" = "Ok";
Expand Down
3 changes: 3 additions & 0 deletions Stepic/ru.lproj/Localizable.strings
Original file line number Diff line number Diff line change
Expand Up @@ -555,6 +555,9 @@ PersonalDeadline = "Персональный дедлайн";
DeadlineModeQuestion = "Сколько времени вы готовы тратить на изучение этого курса?";
PersonalDeadlineWidgetSuggestionText = "Постоянное обучение - путь к успеху. Мы можем создать для вас персональное умное расписание - так вы никогда не забудете получить порцию новых знаний!";

/* Sign in AB */
SignInTitleSocialTest = "<b>Продолжить</b> с помощью";

/* ExamEGERussian */
"ErrorMessage" = "Извините, что-то пошло не так: повторите попытку позже.";
"Ok" = "Ok";
Expand Down