Skip to content

Ensure update(settings:type:) is only called with .initial once. #276

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

Merged
merged 1 commit into from
Nov 21, 2023
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.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion Sources/Segment/Plugins.swift
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ extension Plugin {
}
}


// MARK: - Adding/Removing Plugins

extension DestinationPlugin {
Expand Down Expand Up @@ -127,6 +126,7 @@ extension DestinationPlugin {
plugin.configure(analytics: analytics)
}
timeline.add(plugin: plugin)
analytics?.updateIfNecessary(plugin: plugin)
return plugin
}

Expand Down Expand Up @@ -189,6 +189,7 @@ extension Analytics {
public func add(plugin: Plugin) -> Plugin {
plugin.configure(analytics: self)
timeline.add(plugin: plugin)
updateIfNecessary(plugin: plugin)
return plugin
}

Expand Down
50 changes: 36 additions & 14 deletions Sources/Segment/Settings.swift
Original file line number Diff line number Diff line change
Expand Up @@ -108,20 +108,44 @@ extension Settings: Equatable {
}

extension Analytics {
internal func update(settings: Settings, type: UpdateType) {
apply { (plugin) in
// tell all top level plugins to update.
update(plugin: plugin, settings: settings, type: type)
internal func update(settings: Settings) {
guard let system: System = store.currentState() else { return }
apply { plugin in
plugin.update(settings: settings, type: updateType(for: plugin, in: system))
if let destPlugin = plugin as? DestinationPlugin {
destPlugin.apply { subPlugin in
subPlugin.update(settings: settings, type: updateType(for: subPlugin, in: system))
}
}
}
}

internal func update(plugin: Plugin, settings: Settings, type: UpdateType) {
plugin.update(settings: settings, type: type)
// if it's a destination, tell it's plugins to update as well.
if let dest = plugin as? DestinationPlugin {
dest.apply { (subPlugin) in
subPlugin.update(settings: settings, type: type)
internal func updateIfNecessary(plugin: Plugin) {
guard let system: System = store.currentState() else { return }
// if we're already running, update has already been called for existing plugins,
// so we just wanna call it on this one if it hasn't been done already.
if system.running, let settings = system.settings {
let alreadyInitialized = system.initializedPlugins.contains { p in
return plugin === p
}
if !alreadyInitialized {
store.dispatch(action: System.AddPluginToInitialized(plugin: plugin))
plugin.update(settings: settings, type: .initial)
} else {
plugin.update(settings: settings, type: .refresh)
}
}
}

internal func updateType(for plugin: Plugin, in system: System) -> UpdateType {
let alreadyInitialized = system.initializedPlugins.contains { p in
return plugin === p
}
if alreadyInitialized {
return .refresh
} else {
store.dispatch(action: System.AddPluginToInitialized(plugin: plugin))
return .initial
}
}

Expand All @@ -135,6 +159,7 @@ extension Analytics {
operatingMode.run(queue: DispatchQueue.main) {
if let state: System = self.store.currentState(), let settings = state.settings {
self.store.dispatch(action: System.UpdateSettingsAction(settings: settings))
self.update(settings: settings)
}
self.store.dispatch(action: System.ToggleRunningAction(running: true))
}
Expand All @@ -145,9 +170,6 @@ extension Analytics {

let writeKey = self.configuration.values.writeKey
let httpClient = HTTPClient(analytics: self)
let systemState: System? = store.currentState()
let hasSettings = (systemState?.settings?.integrations != nil && systemState?.settings?.plan != nil)
let updateType = (hasSettings ? UpdateType.refresh : UpdateType.initial)

// stop things; queue in case our settings have changed.
store.dispatch(action: System.ToggleRunningAction(running: false))
Expand All @@ -158,7 +180,7 @@ extension Analytics {
// this will cause them to be cached.
self.store.dispatch(action: System.UpdateSettingsAction(settings: s))
// let plugins know we just received some settings..
self.update(settings: s, type: updateType)
self.update(settings: s)
}
}
// we're good to go back to a running state.
Expand Down
36 changes: 30 additions & 6 deletions Sources/Segment/State.swift
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ struct System: State {
let settings: Settings?
let running: Bool
let enabled: Bool
let initializedPlugins: [Plugin]

struct UpdateSettingsAction: Action {
let settings: Settings
Expand All @@ -23,7 +24,8 @@ struct System: State {
let result = System(configuration: state.configuration,
settings: settings,
running: state.running,
enabled: state.enabled)
enabled: state.enabled,
initializedPlugins: state.initializedPlugins)
return result
}
}
Expand All @@ -35,7 +37,8 @@ struct System: State {
return System(configuration: state.configuration,
settings: state.settings,
running: running,
enabled: state.enabled)
enabled: state.enabled,
initializedPlugins: state.initializedPlugins)
}
}

Expand All @@ -46,7 +49,8 @@ struct System: State {
return System(configuration: state.configuration,
settings: state.settings,
running: state.running,
enabled: enabled)
enabled: enabled,
initializedPlugins: state.initializedPlugins)
}
}

Expand All @@ -57,7 +61,8 @@ struct System: State {
return System(configuration: configuration,
settings: state.settings,
running: state.running,
enabled: state.enabled)
enabled: state.enabled,
initializedPlugins: state.initializedPlugins)
}
}

Expand All @@ -73,7 +78,26 @@ struct System: State {
return System(configuration: state.configuration,
settings: settings,
running: state.running,
enabled: state.enabled)
enabled: state.enabled,
initializedPlugins: state.initializedPlugins)
}
}

struct AddPluginToInitialized: Action {
let plugin: Plugin

func reduce(state: System) -> System {
var initializedPlugins = state.initializedPlugins
if !initializedPlugins.contains(where: { p in
return plugin === p
}) {
initializedPlugins.append(plugin)
}
return System(configuration: state.configuration,
settings: state.settings,
running: state.running,
enabled: state.enabled,
initializedPlugins: initializedPlugins)
}
}
}
Expand Down Expand Up @@ -147,7 +171,7 @@ extension System {
settings = Settings(writeKey: configuration.values.writeKey, apiHost: HTTPClient.getDefaultAPIHost())
}
}
return System(configuration: configuration, settings: settings, running: false, enabled: true)
return System(configuration: configuration, settings: settings, running: false, enabled: true, initializedPlugins: [Plugin]())
}
}

Expand Down
3 changes: 0 additions & 3 deletions Sources/Segment/Timeline.swift
Original file line number Diff line number Diff line change
Expand Up @@ -55,9 +55,6 @@ public class Timeline {
internal class Mediator {
internal func add(plugin: Plugin) {
plugins.append(plugin)
if let settings = plugin.analytics?.settings() {
plugin.update(settings: settings, type: .initial)
}
}

internal func remove(plugin: Plugin) {
Expand Down
44 changes: 44 additions & 0 deletions Tests/Segment-Tests/Analytics_Tests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,50 @@ final class Analytics_Tests: XCTestCase {
wait(for: [expectation], timeout: 1.0)
}

func testDestinationInitialUpdateOnlyOnce() {
// need to clear settings for this one.
UserDefaults.standard.removePersistentDomain(forName: "com.segment.storage.test")

let expectation = XCTestExpectation(description: "MyDestination Expectation")
let myDestination = MyDestination {
expectation.fulfill()
return true
}

var settings = Settings(writeKey: "test")
if let existing = settings.integrations?.dictionaryValue {
var newIntegrations = existing
newIntegrations[myDestination.key] = true
settings.integrations = try! JSON(newIntegrations)
}
let configuration = Configuration(writeKey: "test")
configuration.defaultSettings(settings)
let analytics = Analytics(configuration: configuration)

let ziggy1 = ZiggyPlugin()
analytics.add(plugin: myDestination)
analytics.add(plugin: ziggy1)

waitUntilStarted(analytics: analytics)

analytics.track(name: "testDestinationEnabled")

let ziggy2 = ZiggyPlugin()
analytics.add(plugin: ziggy2)

let dest = analytics.find(key: myDestination.key)
XCTAssertNotNil(dest)
XCTAssertTrue(dest is MyDestination)

wait(for: [expectation], timeout: 1.0)

XCTAssertEqual(myDestination.receivedInitialUpdate, 1)
XCTAssertEqual(ziggy1.receivedInitialUpdate, 1)
XCTAssertEqual(ziggy2.receivedInitialUpdate, 1)

}


func testDestinationEnabled() {
// need to clear settings for this one.
UserDefaults.standard.removePersistentDomain(forName: "com.segment.storage.test")
Expand Down
7 changes: 7 additions & 0 deletions Tests/Segment-Tests/Support/TestUtilities.swift
Original file line number Diff line number Diff line change
Expand Up @@ -42,13 +42,18 @@ class GooberPlugin: EventPlugin {
class ZiggyPlugin: EventPlugin {
let type: PluginType
var analytics: Analytics?
var receivedInitialUpdate: Int = 0

var completion: (() -> Void)?

required init() {
self.type = .enrichment
}

func update(settings: Settings, type: UpdateType) {
if type == .initial { receivedInitialUpdate += 1 }
}

func identify(event: IdentifyEvent) -> IdentifyEvent? {
var newEvent = IdentifyEvent(existing: event)
newEvent.userId = "ziggy"
Expand Down Expand Up @@ -78,6 +83,7 @@ class MyDestination: DestinationPlugin {
let trackCompletion: (() -> Bool)?

let disabled: Bool
var receivedInitialUpdate: Int = 0

init(disabled: Bool = false, trackCompletion: (() -> Bool)? = nil) {
self.key = "MyDestination"
Expand All @@ -88,6 +94,7 @@ class MyDestination: DestinationPlugin {
}

func update(settings: Settings, type: UpdateType) {
if type == .initial { receivedInitialUpdate += 1 }
if disabled == false {
// add ourselves to the settings
analytics?.manuallyEnableDestination(plugin: self)
Expand Down