Skip to content

[FSSDK-11690] chore: remove layer id from holdout #604

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.

Sign up for GitHub

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 2 commits into from
Jul 15, 2025
Merged

[FSSDK-11690] chore: remove layer id from holdout #604

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
79c0d06
Remove layer id from holdout
muzahidul-opti Jul 14, 2025
c904db8
Remove layer id from equatable condition
muzahidul-opti Jul 15, 2025
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
8 changes: 4 additions & 4 deletions Sources/Data Model/Holdout.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,6 @@ struct Holdout: Codable, ExperimentCore {
var id: String
var key: String
var status: Status
var layerId: String
var variations: [Variation]
var trafficAllocation: [TrafficAllocation]
var audienceIds: [String]
Expand All @@ -36,20 +35,21 @@ struct Holdout: Codable, ExperimentCore {
var excludedFlags: [String]

enum CodingKeys: String, CodingKey {
case id, key, status, layerId, variations, trafficAllocation, audienceIds, audienceConditions, includedFlags, excludedFlags
case id, key, status, variations, trafficAllocation, audienceIds, audienceConditions, includedFlags, excludedFlags
}

var variationsMap: [String: OptimizelyVariation] = [:]
// replace with serialized string representation with audience names when ProjectConfig is ready
var audiences: String = ""
// Not necessary for HO
var layerId: String = ""
Comment on lines +44 to +45
Copy link
Contributor

@jaeopt jaeopt Jul 14, 2025

Choose a reason for hiding this comment

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

Can't we remove this?

Copy link
Contributor Author

@muzahidul-opti muzahidul-opti Jul 15, 2025

Choose a reason for hiding this comment

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

ExperimentCore protocol requires the implementation of layerId. There are two ways: we can either provide a default value via protocol extension or implement layerId explicitly in each confirming type. The second approach is much clearer to me.


init(from decoder: Decoder) throws {
let container = try decoder.container(keyedBy: CodingKeys.self)

id = try container.decode(String.self, forKey: .id)
key = try container.decode(String.self, forKey: .key)
status = try container.decode(Status.self, forKey: .status)
layerId = try container.decode(String.self, forKey: .layerId)
variations = try container.decode([Variation].self, forKey: .variations)
trafficAllocation = try container.decode([TrafficAllocation].self, forKey: .trafficAllocation)
audienceIds = try container.decode([String].self, forKey: .audienceIds)
Expand All @@ -65,8 +65,8 @@ extension Holdout: Equatable {
return lhs.id == rhs.id &&
lhs.key == rhs.key &&
lhs.status == rhs.status &&
lhs.layerId == rhs.layerId &&
lhs.variations == rhs.variations &&
lhs.layerId == rhs.layerId &&
Copy link
Contributor

@jaeopt jaeopt Jul 14, 2025

Choose a reason for hiding this comment

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

Can't remove this line?

Copy link
Contributor Author

@muzahidul-opti muzahidul-opti Jul 15, 2025

Choose a reason for hiding this comment

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

Yes we can.

lhs.trafficAllocation == rhs.trafficAllocation &&
lhs.audienceIds == rhs.audienceIds &&
lhs.audienceConditions == rhs.audienceConditions &&
Expand Down
1 change: 0 additions & 1 deletion Tests/OptimizelyTests-Common/BatchEventBuilderTests_Events.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,6 @@ class BatchEventBuilderTests_Events: XCTestCase {
"status": "Running",
"id": "holdout_4444444",
"key": "holdout_key",
"layerId": "10420273888",
"trafficAllocation": [
["entityId": "holdout_variation_a11", "endOfRange": 10000] // 100% traffic allocation
],
Expand Down
4 changes: 0 additions & 4 deletions Tests/OptimizelyTests-Common/DecisionServiceTests_Holdouts.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,7 +111,6 @@ class DecisionServiceTests_Holdouts: XCTestCase {
"status": "Running",
"id": "holdout_4444444",
"key": "holdout_key",
"layerId": "10420273888",
"trafficAllocation": [
["entityId": "holdout_variation_a11", "endOfRange": 1000] // 10% traffic allocation
],
Expand All @@ -133,7 +132,6 @@ class DecisionServiceTests_Holdouts: XCTestCase {
"status": "Running",
"id": "holdout_global",
"key": "holdout_global",
"layerId": "10420273888",
"trafficAllocation": [
["entityId": "holdout_global_variation", "endOfRange": 500]
],
Expand All @@ -155,7 +153,6 @@ class DecisionServiceTests_Holdouts: XCTestCase {
"status": "Running",
"id": "holdout_included",
"key": "holdout_included",
"layerId": "10420273889",
"trafficAllocation": [
["entityId": "holdout_included_variation", "endOfRange": 1000]
],
Expand All @@ -177,7 +174,6 @@ class DecisionServiceTests_Holdouts: XCTestCase {
"status": "Running",
"id": "holdout_excluded",
"key": "holdout_excluded",
"layerId": "10420273890",
"trafficAllocation": [
["entityId": "holdout_excluded_variation", "endOfRange": 1000]
],
Expand Down
1 change: 0 additions & 1 deletion Tests/OptimizelyTests-Common/OptimizelyUserContextTests_Decide_Holdouts.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,6 @@ class OptimizelyUserContextTests_Decide_Holdouts: XCTestCase {
"status": "Running",
"id": "id_holdout",
"key": "key_holdout",
"layerId": "10420273888",
"trafficAllocation": [
["entityId": "id_holdout_variation", "endOfRange": 500]
],
Expand Down
1 change: 0 additions & 1 deletion Tests/OptimizelyTests-Common/OptimizelyUserContextTests_Decide_With_Holdouts_Reasons.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,6 @@ class OptimizelyUserContextTests_Decide_With_Holdouts_Reasons: XCTestCase {
"status": "Running",
"id": "id_holdout",
"key": "key_holdout",
"layerId": "10420273888",
"trafficAllocation": [
["entityId": "id_holdout_variation", "endOfRange": 500]
],
Expand Down
16 changes: 0 additions & 16 deletions Tests/OptimizelyTests-DataModel/HoldoutTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,7 +35,6 @@ class HoldoutTests: XCTestCase {
static var sampleData: [String: Any] = ["id": "11111",
"key": "background",
"status": "Running",
"layerId": "22222",
"variations": [HoldoutTests.variationData],
"trafficAllocation": [HoldoutTests.trafficAllocationData],
"audienceIds": ["33333"],
Expand All @@ -44,7 +43,6 @@ class HoldoutTests: XCTestCase {
static var sampleDataWithIncludedFlags: [String: Any] = ["id": "55555",
"key": "background",
"status": "Running",
"layerId": "22222",
"variations": [HoldoutTests.variationData],
"trafficAllocation": [HoldoutTests.trafficAllocationData],
"audienceIds": ["33333"],
Expand All @@ -54,7 +52,6 @@ class HoldoutTests: XCTestCase {
static var sampleDataWithExcludedFlags: [String: Any] = ["id": "3333",
"key": "background",
"status": "Running",
"layerId": "22222",
"variations": [HoldoutTests.variationData],
"trafficAllocation": [HoldoutTests.trafficAllocationData],
"audienceIds": ["33333"],
Expand All @@ -76,7 +73,6 @@ extension HoldoutTests {
XCTAssert(model.id == "11111")
XCTAssert(model.key == "background")
XCTAssert(model.status == .running)
XCTAssert(model.layerId == "22222")
XCTAssert(model.variations == [try! OTUtils.model(from: HoldoutTests.variationData)])
XCTAssert(model.trafficAllocation == [try! OTUtils.model(from: HoldoutTests.trafficAllocationData)])
XCTAssert(model.audienceIds == ["33333"])
Expand All @@ -91,7 +87,6 @@ extension HoldoutTests {
XCTAssert(model.id == "55555")
XCTAssert(model.key == "background")
XCTAssert(model.status == .running)
XCTAssert(model.layerId == "22222")
XCTAssert(model.variations == [try! OTUtils.model(from: HoldoutTests.variationData)])
XCTAssert(model.trafficAllocation == [try! OTUtils.model(from: HoldoutTests.trafficAllocationData)])
XCTAssert(model.audienceIds == ["33333"])
Expand All @@ -107,7 +102,6 @@ extension HoldoutTests {
XCTAssert(model.id == "3333")
XCTAssert(model.key == "background")
XCTAssert(model.status == .running)
XCTAssert(model.layerId == "22222")
XCTAssert(model.variations == [try! OTUtils.model(from: HoldoutTests.variationData)])
XCTAssert(model.trafficAllocation == [try! OTUtils.model(from: HoldoutTests.trafficAllocationData)])
XCTAssert(model.audienceIds == ["33333"])
Expand All @@ -126,7 +120,6 @@ extension HoldoutTests {
XCTAssert(model.id == "11111")
XCTAssert(model.key == "background")
XCTAssert(model.status == .running)
XCTAssert(model.layerId == "22222")
XCTAssert(model.variations == [try! OTUtils.model(from: HoldoutTests.variationData)])
XCTAssert(model.trafficAllocation == [try! OTUtils.model(from: HoldoutTests.trafficAllocationData)])
XCTAssert(model.audienceIds == ["33333"])
Expand Down Expand Up @@ -156,14 +149,6 @@ extension HoldoutTests {
XCTAssertNil(model)
}

func testDecodeFailWithMissingLayerId() {
var data: [String: Any] = HoldoutTests.sampleData
data["layerId"] = nil

let model: Holdout? = try? OTUtils.model(from: data)
XCTAssertNil(model)
}

func testDecodeFailWithMissingVariations() {
var data: [String: Any] = HoldoutTests.sampleData
data["variations"] = nil
Expand Down Expand Up @@ -203,7 +188,6 @@ extension HoldoutTests {
let commonData: [String: Any] = ["id": "11111",
"key": "background",
"status": "Running",
"layerId": "22222",
"variations": [HoldoutTests.variationData],
"trafficAllocation": [HoldoutTests.trafficAllocationData],
"audienceIds": [],
Expand Down
Loading