[REM-3040] Display ads tracking

[ZSnake]

Add functions to emit tracking events for display ads media impressions view and click

[Copilot]

Pull request overview

Adds support for emitting tracking events for display ads (media) impression view and click, including request models, API paths, DataManager wiring, and tests/documentation.

Changes:

• Add media tracking request bodies and API endpoints for display ad view/click events.
• Introduce mediaServiceUrl configuration and persist it via PreferencesHelper.
• Add unit + integration tests for media tracking and document the new public APIs in the README.

Reviewed changes

Copilot reviewed 12 out of 12 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
library/src/main/java/io/constructor/data/remote/ConstructorApi.kt	Adds Retrofit endpoints for media impression tracking via dynamic @Url.
library/src/main/java/io/constructor/data/remote/ApiPaths.kt	Adds URL constants for ad behavioral actions and media impression endpoints.
library/src/main/java/io/constructor/data/model/tracking/MediaImpressionViewRequestBody.kt	New request body model for display ad view tracking.
library/src/main/java/io/constructor/data/model/tracking/MediaImpressionClickRequestBody.kt	New request body model for display ad click tracking.
library/src/main/java/io/constructor/data/local/PreferencesHelper.kt	Persists mediaServiceUrl in shared preferences.
library/src/main/java/io/constructor/data/interceptor/RequestInterceptor.kt	Attempts to include ad behavioral endpoints in the redaction/behavioral handling list.
library/src/main/java/io/constructor/data/DataManager.kt	Builds media-service URLs and routes new tracking calls through Retrofit.
library/src/main/java/io/constructor/core/ConstructorIoConfig.kt	Adds mediaServiceUrl to SDK configuration.
library/src/main/java/io/constructor/core/ConstructorIo.kt	Exposes trackMediaImpressionView/Click public APIs and internal implementations.
library/src/test/java/io/constructor/core/ConstructorIoMediaTrackingTest.kt	Adds MockWebServer-based unit tests for media tracking endpoints.
library/src/test/java/io/constructor/core/ConstructorIoIntegrationMediaTrackingTest.kt	Adds real-network integration tests for media tracking endpoints.
README.md	Documents new media impression tracking APIs.

Comments suppressed due to low confidence (1)

README.md:652

• The new Markdown code block under “Media Impression Events” is not closed (missing a terminating ```). This will cause the rest of the README to render as code.

ConstructorIo.trackMediaImpressionClick(
    bannerAdId = "abc123",
    placementId = "home"
)

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

[constructor-claude-bedrock]

Code Review Summary

This PR adds display ad tracking support for media impression view and click events. The implementation is well-structured and consistent with existing patterns. It correctly introduces a new mediaServiceUrl, a new v2/ad_behavioral_action prefix for the interceptor, and a buildMediaUrl helper in DataManager. Tests cover happy-path, 500-error, and timeout scenarios, which is great. A few issues need attention before merging.

---

Detailed Feedback

[File: library/src/main/java/io/constructor/data/model/tracking/MediaImpressionViewRequestBody.kt & MediaImpressionClickRequestBody.kt]

1. Duplicate data classes — should be a single shared class

MediaImpressionViewRequestBody and MediaImpressionClickRequestBody are byte-for-byte identical. This duplication will lead to maintenance drift over time. Since the only distinction is the endpoint URL (which is already handled by the caller), a single MediaImpressionRequestBody class should serve both tracking calls:

// Single class for both view and click:
@JsonClass(generateAdapter = true)
data class MediaImpressionRequestBody(
        @Json(name = "banner_ad_id") val bannerAdId: String,
        @Json(name = "placement_id") val placementId: String,
        ...
) : Serializable

---

[File: library/src/main/java/io/constructor/data/model/tracking/MediaImpressionViewRequestBody.kt Line: L13]

2. Missing @suppress KDoc comment — inconsistent with existing request bodies

All other request body classes in the package (e.g. ItemDetailLoadRequestBody, GenericResultClickRequestBody) carry a /** @suppress */ KDoc comment above the class. Both new request body files omit it.

/**
 * @suppress
 */
@JsonClass(generateAdapter = true)
data class MediaImpressionViewRequestBody(

---

[File: library/src/main/java/io/constructor/data/model/tracking/MediaImpressionViewRequestBody.kt Lines: L14–L22 and MediaImpressionClickRequestBody.kt Lines: L14–L22]

3. Single-letter field names c, i, s are unclear — missing KDoc or inline comments

The fields c, i, s follow the existing convention, but unlike the other request bodies, there's no @suppress or any descriptive context. The existing classes are also not documented, but since this is new code, adding brief inline comments would greatly improve maintainability:

@Json(name = "c") val c: String,   // client version
@Json(name = "i") val i: String,   // device identity (GUID)
@Json(name = "s") val s: Int,      // session ID

---

[File: library/src/main/java/io/constructor/data/model/tracking/MediaImpressionViewRequestBody.kt Line: L18]

4. _dt field is non-nullable Long — inconsistent with all other request bodies

In every other *RequestBody class, _dt is typed as Long? (nullable). In the two new classes it is typed as Long (non-null). This is a minor inconsistency; align it with the rest:

// All other request bodies:
@Json(name= "_dt") val _dt: Long?

// New bodies (should follow same pattern):
@Json(name = "_dt") val dt: Long?

Additionally, the field is named dt (without the leading underscore) in the new classes but _dt in all existing ones. While the JSON name "_dt" is preserved correctly, the inconsistent Kotlin property name is a minor style issue.

---

[File: library/src/main/java/io/constructor/core/ConstructorIo.kt Lines: L1960–L1984]

5. Missing analyticsTags support — functional gap vs. other tracking events

All comparable tracking methods (trackItemDetailLoadedInternal, trackGenericResultClickInternal) pass analyticsTags through mergeAnalyticsTags(configMemoryHolder.defaultAnalyticsTags, analyticsTags). The new media impression methods do not accept or forward analyticsTags at all. If this is intentional (media endpoints don't support analytics tags), it should be documented. If not intentional, this is a missing feature.

---

[File: library/src/main/java/io/constructor/data/DataManager.kt Lines: L324–L339]

6. buildMediaUrl always uses preferencesHelper.port — ignores the default 443 for the media host

The media service URL (behavior.media-cnstrc.com) is a different host from the main API. When the app is in production, the scheme will be https and the port will be 443. However, when a custom port is set (e.g. for testing with MockWebServer), the port will be overridden. This is likely intentional, but it means the port from the main service config bleeds into the media URL. Consider whether the media service should use its own port setting or whether this shared port is correct.

More critically, for production use with https + the default port 443, HttpUrl.Builder will include :443 in the URL string when port() is explicitly called with 443. This should still work because standard HTTP clients normalize it, but it's worth verifying. You can avoid it entirely by only setting port if it differs from the default:

if (preferencesHelper.port != 443) {
    builder.port(preferencesHelper.port)
}

---

[File: library/src/main/java/io/constructor/core/ConstructorIoConfig.kt Line: L30]

7. Missing newline at end of file

The diff shows \ No newline at end of file was removed from the old version and still the file ends without a trailing newline. Please add a trailing newline to this file to keep consistent with the rest of the codebase.

---

[File: library/src/test/java/io/constructor/core/ConstructorIoIntegrationMediaTrackingTest.kt Lines: L59–L75]

8. Integration test makes a real network call in @Before — fragile and environment-dependent

The fetchBannerAdId() call in setup() issues a live HTTP request to display.media-cnstrc.com using a hardcoded API key. This makes the integration tests dependent on network availability and the stability of the external service. The test will fail silently in CI environments with no network access, or if the test key expires. Consider:

• Adding a @Ignore annotation with a note that this requires a live environment, or
• Separating these into a dedicated integration test suite (e.g. androidTest) that can be explicitly opted into, rather than a JVM unit test.

---

[File: library/src/test/java/io/constructor/core/ConstructorIoMediaTrackingTest.kt Line: L99 and L114]

9. observer.assertError { true } is too permissive

The 500-error tests use observer.assertError { true }, which passes for any error, including unexpected ones like NullPointerException. For clearer test intent, assert on the specific error type expected from a non-2xx HTTP response:

observer.assertError(HttpException::class.java)

(using Retrofit's HttpException, or whatever error type the API layer emits for 5xx responses.)

---

[File: library/src/test/java/io/constructor/core/ConstructorIoMediaTrackingTest.kt Lines: L122–L135 and L138–L151]

10. Timeout tests assert 500 status but only care about timeout — misleading setup

The timeout tests set setResponseCode(500) in the mock response, but the test verifies SocketTimeoutException before the response is ever received. The response code is irrelevant in this case. Use setResponseCode(204) (matching the happy-path test) to avoid implying 500s cause timeouts:

val mockResponse = MockResponse()
        .setResponseCode(204)
        .setBodyDelay(5, TimeUnit.SECONDS)
mockServer.enqueue(mockResponse)

---

[File: library/src/test/java/io/constructor/core/ConstructorIoMediaTrackingTest.kt Line: L127]

11. Trailing semicolons in test code

val request = awaitRequest();  // line 127 — trailing semicolon

Kotlin does not require semicolons; these are style inconsistencies. The rest of the PR's test code doesn't use them. Remove them.

---

Conclusion

The overall approach is sound and follows the existing codebase patterns well. The main issues to address before merging are:

1. Merge the two identical request body classes into one (issue add timestamp parameter to all requests #1)
2. Add analyticsTags support or explicitly document the omission (issue Ch1700/input focus event #5)
3. Fix the integration test to not make live network calls from a standard unit test (issue limit categories per item #8)
4. Minor: Add @suppress KDoc to request body classes, align _dt nullability, and clean up trailing semicolons/assertError { true }.