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.

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

Update RMF version matching to ignore build number #1118

Merged
merged 3 commits into from
Dec 6, 2024
+50 −14
Merged

Update RMF version matching to ignore build number #1118

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
bbc0608
Update RMF version matching to ignore build number.
samsymons Dec 6, 2024
3ca813f
Remove unnecessary RMF test suite function.
samsymons Dec 6, 2024
1c9bfc2
Move build number trimming down into the app version matcher.
samsymons Dec 6, 2024
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
2 changes: 1 addition & 1 deletion Sources/RemoteMessaging/Matchers/AppAttributeMatcher.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -81,7 +81,7 @@ public struct CommonAppAttributeMatcher: AttributeMatching {
assertionFailure("BundleIdentifier should not be empty")
}
self.init(bundleId: AppVersion.shared.identifier,
appVersion: AppVersion.shared.versionAndBuildNumber,
appVersion: AppVersion.shared.versionNumber,
isInternalUser: isInternalUser,
statisticsStore: statisticsStore,
variantManager: variantManager)
Expand Down
4 changes: 2 additions & 2 deletions Sources/RemoteMessaging/Model/MatchingAttributes.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,10 +49,10 @@ struct AppIdMatchingAttribute: SingleValueMatching {
}

struct AppVersionMatchingAttribute: StringRangeMatching {
static let defaultMaxValue: String = AppVersion.shared.versionAndBuildNumber
static let defaultMaxValue: String = AppVersion.shared.versionNumber

var min: String = MatchingAttributeDefaults.stringDefaultValue
var max: String = AppVersion.shared.versionAndBuildNumber
var max: String = AppVersion.shared.versionNumber
var value: String = MatchingAttributeDefaults.stringDefaultValue
var fallback: Bool?
}
Expand Down
21 changes: 20 additions & 1 deletion Sources/RemoteMessaging/Model/MatchingAttributesPrototypes/StringRangeMatching.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,7 +42,12 @@ extension StringRangeMatching {
let value = jsonMatchingAttribute[RuleAttributes.value] as? String ?? MatchingAttributeDefaults.stringDefaultValue
let fallback = jsonMatchingAttribute[RuleAttributes.fallback] as? Bool

self.init(min: min, max: max, value: value, fallback: fallback)
self.init(
min: min.trimmingBuildNumber,
max: max.trimmingBuildNumber,
value: value.trimmingBuildNumber,
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These lines exist to avoid breaking existing messages that already define a build number.

This approach means that build number trimming will happen for all StringRangeMatching attributes, which is currently AppVersionMatchingAttribute and OSMatchingAttribute. But, if you would prefer to keep this change limited to the app version attribute, I can override its initializer and push that logic down there instead.

Alternatively, it could be moved into RangeStringNumericMatchingAttribute instead. We have a few options, let me know what you prefer. Thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

The logic we're applying here is specific to the app version attribute, right? I'd rather see it in the AppVersionMatchingAttribute to limit its reach. Unless you foresee this adjustment to be useful for other strings?

fallback: fallback
)
}

init(fallback: Bool?) {
Expand All @@ -61,3 +66,17 @@ extension StringRangeMatching {
return RangeStringNumericMatchingAttribute(min: min, max: max).matches(value: value)
}
}

private extension String {

var trimmingBuildNumber: String {
let components = self.split(separator: ".")

if components.count == 4 {
return components.dropLast().joined(separator: ".")
} else {
return self
}
}

}
25 changes: 16 additions & 9 deletions Tests/RemoteMessagingTests/Matchers/CommonAppAttributeMatcherTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import XCTest
class CommonAppAttributeMatcherTests: XCTestCase {

private var matcher: CommonAppAttributeMatcher!
private let versionNumber = "3.2.1"
samsymons marked this conversation as resolved.
Show resolved Hide resolved

override func setUpWithError() throws {
try super.setUpWithError()
Expand All @@ -36,7 +37,13 @@ class CommonAppAttributeMatcherTests: XCTestCase {
mockStatisticsStore.searchRetentionAtb = "v105-88"

let manager = MockVariantManager(isSupportedReturns: true, currentVariant: MockVariant(name: "zo", weight: 44, isIncluded: { return true }, features: [.dummy]))
matcher = CommonAppAttributeMatcher(statisticsStore: mockStatisticsStore, variantManager: manager)
matcher = CommonAppAttributeMatcher(
bundleId: AppVersion.shared.identifier,
appVersion: versionNumber,
isInternalUser: true,
statisticsStore: mockStatisticsStore,
variantManager: manager
)
}

override func tearDownWithError() throws {
Expand Down Expand Up @@ -66,7 +73,7 @@ class CommonAppAttributeMatcherTests: XCTestCase {
}

func testWhenAppVersionEqualOrLowerThanMaxThenReturnMatch() throws {
let appVersionComponents = AppVersion.shared.versionAndBuildNumber.components(separatedBy: ".").map { $0 }
let appVersionComponents = versionNumber.components(separatedBy: ".").map { $0 }
let appMajorVersion = appVersionComponents[0]
let appMinorVersion = appVersionComponents.suffix(from: 1).joined(separator: ".")

Expand All @@ -82,7 +89,7 @@ class CommonAppAttributeMatcherTests: XCTestCase {
}

func testWhenAppVersionGreaterThanMaxThenReturnFail() throws {
let appVersionComponents = AppVersion.shared.versionAndBuildNumber.components(separatedBy: ".").map { $0 }
let appVersionComponents = versionNumber.components(separatedBy: ".").map { $0 }
let appMajorVersion = appVersionComponents[0]
let lessThanMax = String(Int(appMajorVersion)! - 1)

Expand All @@ -91,12 +98,12 @@ class CommonAppAttributeMatcherTests: XCTestCase {
}

func testWhenAppVersionEqualOrGreaterThanMinThenReturnMatch() throws {
XCTAssertEqual(matcher.evaluate(matchingAttribute: AppVersionMatchingAttribute(min: AppVersion.shared.versionAndBuildNumber, fallback: nil)),
XCTAssertEqual(matcher.evaluate(matchingAttribute: AppVersionMatchingAttribute(min: versionNumber, fallback: nil)),
.match)
}

func testWhenAppVersionLowerThanMinThenReturnFail() throws {
let appVersionComponents = AppVersion.shared.versionAndBuildNumber.components(separatedBy: ".").map { $0 }
let appVersionComponents = versionNumber.components(separatedBy: ".").map { $0 }
let (major, minor, patch) = (appVersionComponents[0], appVersionComponents[1], appVersionComponents[2])
let majorBumped = String(Int(major)! + 1)
let patchBumped = [major, minor, String(Int(patch)! + 1)].joined(separator: ".")
Expand All @@ -105,7 +112,7 @@ class CommonAppAttributeMatcherTests: XCTestCase {
}

func testWhenAppVersionInRangeThenReturnMatch() throws {
let appVersionComponents = AppVersion.shared.versionAndBuildNumber.components(separatedBy: ".").map { $0 }
let appVersionComponents = versionNumber.components(separatedBy: ".").map { $0 }
let (major, minor, patch) = (appVersionComponents[0], appVersionComponents[1], appVersionComponents[2])
let majorBumped = String(Int(major)! + 1)
let patchDecremented = [major, minor, String(Int(patch)! - 1)].joined(separator: ".")
Expand All @@ -115,7 +122,7 @@ class CommonAppAttributeMatcherTests: XCTestCase {
}

func testWhenAppVersionNotInRangeThenReturnFail() throws {
let appVersionComponents = AppVersion.shared.versionAndBuildNumber.components(separatedBy: ".").map { $0 }
let appVersionComponents = versionNumber.components(separatedBy: ".").map { $0 }
let appMajorVersion = appVersionComponents[0]
let greaterThanMax = String(Int(appMajorVersion)! + 1)

Expand All @@ -124,12 +131,12 @@ class CommonAppAttributeMatcherTests: XCTestCase {
}

func testWhenAppVersionSameAsDeviceThenReturnMatch() throws {
XCTAssertEqual(matcher.evaluate(matchingAttribute: AppVersionMatchingAttribute(min: AppVersion.shared.versionAndBuildNumber, max: AppVersion.shared.versionAndBuildNumber, fallback: nil)),
XCTAssertEqual(matcher.evaluate(matchingAttribute: AppVersionMatchingAttribute(min: versionNumber, max: versionNumber, fallback: nil)),
.match)
}

func testWhenAppVersionDifferentToDeviceThenReturnFail() throws {
let appVersionComponents = AppVersion.shared.versionAndBuildNumber.components(separatedBy: ".").map { $0 }
let appVersionComponents = versionNumber.components(separatedBy: ".").map { $0 }
let (major, minor, patch) = (appVersionComponents[0], appVersionComponents[1], appVersionComponents[2])
let patchDecremented = [major, minor, String(Int(patch)! - 1)].joined(separator: ".")

Expand Down
Loading