Repository navigation
[CDX-566] Migrate to v2 search submit endpoint - #286
TarekAlQaddy wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
The modified public method signature contradicts the stated internal-only migration and can break compatibility.
Review effort: Balanced
Findings: 1
What changed in this PR
Migrates search-submit tracking to the v2 POST endpoint with JSON payloads and default section handling.
Changes:
- Sends search data, group filters, and analytics tags in JSON.
- Defaults the section to configured value or “Products.”
- Updates request and worker tests for v2 behavior.
| File | Description |
|---|---|
ConstructorIOTrackSearchSubmitTests.swift |
Tests v2 tracking behavior and payloads. |
TrackSearchSubmitRequestBuilder.swift |
Tests POST request construction. |
ConstructorIO.swift |
Adds section handling to search submission. |
CIOTrackSearchSubmitData.swift |
Builds the v2 POST request. |
Constants.swift |
Updates the endpoint path. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| */ | ||
| public func trackSearchSubmit(searchTerm: String, originalQuery: String, group: CIOGroup? = nil, analyticsTags: [String: String]? = nil, completionHandler: TrackingCompletionHandler? = nil) { | ||
| let data = CIOTrackSearchSubmitData(searchTerm: searchTerm, originalQuery: originalQuery, group: group, analyticsTags: mergeAnalyticsTags(defaultAnalyticsTags: self.config.defaultAnalyticsTags, newAnalyticsTags: analyticsTags)) | ||
| public func trackSearchSubmit(searchTerm: String, originalQuery: String, group: CIOGroup? = nil, analyticsTags: [String: String]? = nil, section: String? = nil, completionHandler: TrackingCompletionHandler? = nil) { |
There was a problem hiding this comment.
Normal calls to the method compile and work. Such as:
c.trackSearchSubmit(searchTerm: "a", originalQuery: "a")
c.trackSearchSubmit(searchTerm: "a", originalQuery: "a", group: 1)
c.trackSearchSubmit(searchTerm: "a", originalQuery: "a") { _ in }
c.trackSearchSubmit(searchTerm: "a", originalQuery: "a", group: 1, analyticsTags: nil, completionHandler: nil)
We introduced similar updates in the past and there weren't complains.
What doesn't work is calling the method by it's full name which is rare or not re-compiling the SDK after updating the version which is non realistic
![constructor-claude-bedrock[bot]](https://avatars.githubusercontent.com/u/9828194?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
evanyan13
changed the title
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
* Refactor trackSearchSubmit by creating an override * Update trackSearchSubmit usage in project * Rename struct and update tests --------- Co-authored-by: Evan Yan <evan.yan@constructor.io>
There was a problem hiding this comment.
Code Review
This PR migrates trackSearchSubmit to the v2 POST /v2/behavioral_action/search endpoint, introduces CIOTrackSearchSubmitRequest / CIOTrackSearchSubmitFilters as the new public API surface, deprecates the old parameter-based overload, and adds a section parameter — all well-executed with solid test coverage.
Inline comments: 6 discussions added
Overall Assessment:
| builder.build(trackData: tracker) | ||
| let payload = self.payload(builder.getRequest()) | ||
|
|
||
| XCTAssertEqual(payload?["analytics_tags"] as? [String: String], [:], "Empty analytics tags should be sent as-is") |
There was a problem hiding this comment.
Important Issue: The test testTrackSearchSubmitBuilder_WithEmptyAnalyticsTags asserts that an empty analytics tags dict ([:]) is serialised into the body. However, the implementation only adds analytics_tags when analyticsTags != nil — it does not check .isEmpty. An empty dict [:] is non-nil, so it will be sent to the endpoint. Verify with the backend team that sending "analytics_tags": {} is accepted. If the endpoint treats it the same as omitting the field, a consistency argument can be made either way, but the test expectation and the behaviour should at least match intentional design. If empty tags should be omitted (like nil), the fix is:
if let analyticsTags = self.analyticsTags, !analyticsTags.isEmpty {
dict["analytics_tags"] = analyticsTags
}This aligns with how other events in the SDK treat empty collections.
| func testTrackSearchSubmit() { | ||
| let searchTerm = "corn" | ||
| let searchOriginalQuery = "corn" | ||
| private func stubCapturingPayload(_ builder: CIOBuilder, _ capture: @escaping ([String: Any]?) -> Void) { |
There was a problem hiding this comment.
Suggestion: stubCapturingPayload always stubs against searchSubmitURL (the Products section variant), which means any test that passes a different section (e.g. testTrackSearchSubmit_WithSection with "Search Suggestions") cannot use this helper and stubs manually. This is fine as-is, but a small overload taking an explicit URL string would remove the duplication in future tests:
private func stubCapturingPayload(_ builder: CIOBuilder, url: String? = nil, _ capture: @escaping ([String: Any]?) -> Void) {
stub(regex(url ?? searchSubmitURL)) { ... }
}Not a blocker, but would make the helper more reusable.
Similar to the Android SDK PR
Migrating the trackSearchSubmit method to the new v2 endpoint, migration is internal. Also adding
sectionas a new parameter.Couple of notes