AB infrastructure #357
AB infrastructure #357
Conversation
Stepic/ABSocialAuthString.swift
Outdated
| import UIKit | ||
|
|
||
| class ABSocialAuthString: ActiveABTest { | ||
| let ID = "ab_social_auth_string" |
Stepic/ABSocialAuthString.swift
Outdated
| import Foundation | ||
| import UIKit | ||
|
|
||
| class ABSocialAuthString: ActiveABTest { |
There was a problem hiding this comment.
Давай помечать класс как final, если не планируется от него наследоваться
Stepic/ABTestService.swift
Outdated
| associatedtype ValueType | ||
| var ID: String { get } | ||
| func value(group: String) -> ValueType? | ||
| var controlValue: ValueType { get } |
There was a problem hiding this comment.
Почему бы не добавить сюда еще var testValue: ValueType { get }? Тогда реализация value будет общая (просто switch по группе) и ее можно вынести в экстеншен к протоколу.
Stepic/ABTestService.swift
Outdated
| } | ||
|
|
||
| var value: T.ValueType { | ||
| if let resultGroup = RemoteConfig.shared.string(forKey: test.ID + "_result") { |
There was a problem hiding this comment.
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
}Без лишней вложенности.
Stepic/ABTestService.swift
Outdated
| } else { | ||
| if RemoteConfig.shared.fetchComplete { | ||
| guard | ||
| let group = RemoteConfig.shared.string(forKey: test.ID), |
There was a problem hiding this comment.
Давай не будем изобретать новое форматирование длинных гвардов
Stepic/AnalyticsUserProperties.swift
Outdated
| @@ -14,7 +14,7 @@ class AnalyticsUserProperties { | |||
|
|
|||
| static let shared = AnalyticsUserProperties() | |||
|
|
|||
| private func setAmplitudeProperty(key: String, value: Any?) { | |||
| func setAmplitudeProperty(key: String, value: Any?) { | |||
There was a problem hiding this comment.
Кмк, лучше сделать так же как и для других проперти:
func setABProperty(test: ABTest.IDType, value: ABTest.Group) {
self.setAmplitudeProperty(...)
}
Stepic/RemoteConfig.swift
Outdated
| #if DEBUG | ||
| activateDebugMode() | ||
| fetchDuration = 0 |
There was a problem hiding this comment.
Почему не поле класса и не в activateDebugMode?
| @@ -110,4 +112,8 @@ class RemoteConfig { | |||
| let debugSettings = RemoteConfigSettings(developerModeEnabled: true) | |||
| FirebaseRemoteConfig.RemoteConfig.remoteConfig().configSettings = debugSettings | |||
| } | |||
|
|
|||
| func string(forKey key: String) -> String? { | |||
There was a problem hiding this comment.
Это конечно плохо, потому что тогда уж лучше переписать весь класс и сделать новый гибкий интерфейс. А получается, что какие-то ключи у нас обернуты в проперти, а какие-то мы дергаем без обертки 🤔
There was a problem hiding this comment.
Если мы так будем переписывать каждый класс, в котором что-то не нравится, не будем по пол года релизиться.
Stepic/SocialAuthPresenter.swift
Outdated
| @@ -125,6 +127,10 @@ class SocialAuthPresenter { | |||
| } | |||
| } | |||
|
|
|||
| var socialAuthHeaderString: String { | |||
Stepic/SocialAuthPresenter.swift
Outdated
| self.authAPI = authAPI | ||
| self.stepicsAPI = stepicsAPI | ||
| self.notificationStatusesAPI = notificationStatusesAPI | ||
| self.abSocialAuthService = abSocialAuthService |
There was a problem hiding this comment.
Если у нас в модуле будет 10 A/B тестов, мы будем иметь 10 сервисов? Это, конечно, не очень и стоит подумать, как переделать так, чтобы было удобно.
| @@ -38,7 +38,7 @@ class AnalyticsUserProperties { | |||
| } | |||
|
|
|||
| func setUserID(to id: Int?) { | |||
| setAmplitudeProperty(key: "stepik_id", value: id) | |||
| setProperty(key: "stepik_id", value: id) | |||
There was a problem hiding this comment.
У нас по-прежнему есть несколько провайдеров аналитики, поэтому подгонять под протокол меняя названия и модификатор доступа – не самая лучшая идея. Есть предложение сделать протокол не про задание проперти, а про задание проперти, связанной с A/B тестом. Тогда не нужно будет подгонять под протокол методы, а просто сделать отдельный метод, как я предлагал в предыдущем ревью.
There was a problem hiding this comment.
Не понимаю, чем плохо.
У нас есть протокол про проперти.
Я подогнал класс с проперти под этот протокол, просто переименовав один метод, который до этого был вообще приватным.
Другие провайдеры пока это не реализуют, но мы это и не используем.
В чем проблема здесь, я не вижу
There was a problem hiding this comment.
В чем смысл делать отдельный протокол про задание проперти для ab, если это будет то же самое, что и просто задание проперти?
There was a problem hiding this comment.
Можно сказать про SOLID и ISP в нем, но к этому это вообще вряд ли относится, потому что разделить предлагается не по смыслу (не вижу кейсов, в котором задание юзер проперти и юзер проперти в ab-тесте будет различаться хоть как-то).
| @@ -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(), splitTestingService: SplitTestingService(analyticsService: AnalyticsUserProperties.shared, storage: UserDefaults.standard), view: self) | |||
There was a problem hiding this comment.
Почему AnalyticsUserProperties вообще синглтон у нас?
There was a problem hiding this comment.
Понятно, что так когда-то сделали. Но, может быстренько откажемся? К тому же, это не кажется сложным.
Внутри себя он все равно дергает синглтон из SDK аналитики – проблем с тем, что нужно иметь один инстанс нет.
Stepic/en.lproj/Localizable.strings
Outdated
| @@ -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"; | |||
Задача: #APPS-1964
Описание:
Инфраструктура для будущих a/b тестов.
ActiveABTest, в котором можно получить ID теста и значение для имени группы.ABTestServiceотвечает за определение группы пользователя и получение нужного значения.RemoteConfigпришлось немного подкрутить.