-
Notifications
You must be signed in to change notification settings - Fork 83
SDK-26 Add placement id list as a parameter for syncMessages #985
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
SDK-26 Add placement id list as a parameter for syncMessages #985
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #985 +/- ##
==========================================
+ Coverage 69.62% 69.72% +0.10%
==========================================
Files 109 111 +2
Lines 8954 8981 +27
==========================================
+ Hits 6234 6262 +28
+ Misses 2720 2719 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| placements.append(placement) | ||
| if placementIds == nil || requested.contains(placementId) { | ||
| let placement = Placement(placementId: placementId, embeddedMessages: messages) | ||
| placements.append(placement) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not quite understood this part. is it trying to add placement to list of placement once it sees it has new messages for that placement?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, pretty much. It’s not “seeing new messages per placement”—the newMessages boolean gates the whole response. Once newMessages is true, it adds a placement entry for each placementId in mockMessages that was requested (or all of them if placementIds is nil), using whatever messages are stored there.
81343f1 to
f682deb
Compare
iOS SDK PR Review: Add Placement IDs Parameter to syncMessagesTicket: SDK-26 SummaryThis PR adds an optional What ChangedAPI Surface Changes
Implementation Details
Test Coverage
✅ Strengths1. Clean Backward Compatibilitypublic extension IterableEmbeddedManagerProtocol {
func syncMessages(completion: @escaping () -> Void) {
syncMessages(placementIds: nil, completion: completion)
}
}Existing code continues to work without modifications—excellent API design. 2. Correct Placement Preservation Logicif let placementIds, !placementIds.isEmpty {
let requestedPlacementIds = Set(placementIds)
for (placementId, currentMessages) in currentMessagesSnapshot where !requestedPlacementIds.contains(placementId) {
fetchedMessagesDict[placementId] = currentMessages
}
}This ensures syncing placement 1 doesn't clear cached messages for placement 2—critical for proper behavior. 3. Thread-Safe Message Accesslet currentMessagesSnapshot: [Int: [IterableEmbeddedMessage]] = self.messageProcessingQueue.sync {
self.messages
}Proper synchronization when reading shared state. 4. Minimal, Focused DiffChanges are surgical—no unnecessary refactoring or scope creep. Respects rule #14. 🚨 Critical Issue: Query Parameter Format MismatchThe ProblemiOS Implementation (this PR): // swift-sdk/Internal/api-client/Request/RequestCreator.swift:517
args[JsonKey.Embedded.placementIds] = placementIds.map(String.init).joined(separator: ",")Generates: Android Implementation (MOB-8301): // Builds multiple parameters
for (Long placementId : placementIds) {
if (isFirst) {
pathBuilder.append("placementIds=").append(placementId);
isFirst = false;
} else {
pathBuilder.append("&placementIds=").append(placementId);
}
}Generates: Why This MattersThese are different query string formats:
Most REST frameworks parse these differently. One SDK will likely fail if the backend only supports one format. Required ActionBefore merging:
Suggested Fix (if Android format is correct)func createGetEmbeddedMessagesRequest(placementIds: [Int]?) -> Result<IterableRequest, IterableError> {
// ... existing auth checks ...
var path = Const.Path.getEmbeddedMessages
var args: [String: String] = [:]
if let packageName = Bundle.main.appPackageName {
args[JsonKey.Embedded.packageName] = packageName
}
setCurrentUser(inDict: &args)
// Build query string manually to support repeated parameters
if let placementIds, !placementIds.isEmpty {
var components = URLComponents()
components.queryItems = args.map { URLQueryItem(name: $0.key, value: $0.value) }
placementIds.forEach { id in
components.queryItems?.append(URLQueryItem(name: "placementIds", value: String(id)))
}
path += "?" + (components.query ?? "")
return .success(.get(GetRequest(path: path, args: nil)))
}
return .success(.get(createGetRequest(forPath: path, withArgs: args)))
}Minor Suggestions1. Edge Case Test: Empty ArrayCurrent test covers func testSyncMessagesWithEmptyPlacementIdsArray() {
// Verify empty array behaves same as nil (fetches all)
manager.syncMessages(placementIds: []) { }
// Should fetch all placements
}2. DocumentationConsider adding inline docs to the new method: /// Syncs embedded messages from the server
/// - Parameters:
/// - placementIds: Optional array of placement IDs to fetch. If nil or empty, fetches all placements.
/// - completion: Called when sync completes
func syncMessages(placementIds: [Int]?, completion: @escaping () -> Void)Test Plan Verification✅ Existing functionality preserved RecommendationDO NOT MERGE until query parameter format is verified with backend team. Once confirmed:
Questions for PR Author
|
sumeruchat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Above is the commend by Claude. I think the critical issue is a valid concern we should match the parameter format in iOS and Android
Good catch on the difference between the query parameters! I'm checking with the Channels team! |
f682deb to
9ec88f8
Compare
7508b4a to
2b3be47
Compare
🔹 Jira Ticket(s)
✏️ Description
This PR updates the syncMessages call to pass placement IDs as a list