-
Notifications
You must be signed in to change notification settings - Fork 1
Rework Optable API configuration and request forming #35
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
base: master
Are you sure you want to change the base?
Conversation
Remove `app` path parameter. Add `tenant` and `originSlug` parameters. Make host dynamic and apply path `v2`. Move all configuration parameters in Config class and reuse it everywhere. Separate `GoogleAdIdManager` and `LocalStorage` logic from the network client. Rename `Client` to `NetworkClient`. Move hash logic to separate `TypeHasher` class. Separate network interceptors. Refactor SDK code.
Support secret api keys. Fetch user agent only once. Write unit tests with the mock web server.
android_sdk/src/main/java/co/optable/android_sdk/core/UserAgentHolder.kt
Show resolved
Hide resolved
Remove custom network Adapter. Use native Retrofit adapter. Add Kotlin suspension.
Migrate from TypeHasher to IdentifiersEncoder. Add all identifier types. Cover with unit tests.
Use callbacks API. Rewrite demo apps to make the examples more readable. Use shorter logs. Create Optable SDK instance once during application creation and reuse everywhere. Move fragments to one folder.
Add missed "reg" query param. Make Consents mutable. Provide `receiveGaidAutomatically` property. Refactor main methods javadoc.
| public void onCreate() { | ||
| super.onCreate(); | ||
|
|
||
| OptableConfig config = new OptableConfig(this, "prebidtest", "js-sdk", "ca.edge.optable.co"); |
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.
I wonder if host is obligatory to provide? can it work using with a default na.edge.optable.co host?
|
|
||
| Map<String, List<String>> audiences = targeting.getAudiences(); | ||
| if (audiences != null) { | ||
| for (Map.Entry<String, List<String>> entry : audiences.entrySet()) { |
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.
Here is how audience object is converted into GAM key-value pairs:
function TargetingKeyValues(tdata: TargetingResponse | null): TargetingKeyValues {
const result: TargetingKeyValues = {};
if (!tdata) {
return result;
}
for (const identifiers of tdata.audience ?? []) {
if (identifiers.keyspace) {
if (!(identifiers.keyspace in result)) {
result[identifiers.keyspace] = [];
}
result[identifiers.keyspace].push(...identifiers.ids.map((el) => el.id));
}
}
return result;
}audience is an array of objects:
"audience": [
{
"provider": "optable.co",
"ids": [
{
"id": "15f30409"
}
],
"keyspace": "optable",
"rtb_segtax": 5001
}
],each of these objects becomes a single key-value pair.
| private void applyOptableToGam(AdManagerAdRequest.Builder builder, @Nullable OptableTargeting targeting) { | ||
| if (targeting == null) return; | ||
|
|
||
| Map<String, List<String>> audiences = targeting.getAudiences(); |
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.
this (incorrect) code of generating a GAM kv pair is repeated - perhaps it should become part of the Optable SDK library gamTargeting() function?
| val audiences = targeting.audiences | ||
| if (audiences != null) { | ||
| for (entry in audiences.entries) { | ||
| builder.addCustomTargeting(entry.key, entry.value) |
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.
please see above comment re: custom in the Java sample - let's make a library function on OptableTargeting gamTargetingKeywords() or getGAMTargetingKeywords()
| return | ||
| } | ||
|
|
||
| val ids = OptableIdentifiers.Builder().raw(listOf(id)).build() |
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.
please make sure we don't double hash the email as oeid is already sha256-hashed email. but looks like you pass it raw here, which should be correct.
| result.addIfNotNull(IPV4, ids.ipv4Address, ::removeWhitespaces) | ||
| result.addIfNotNull(IPV6, ids.ipv6Address, ::normalize) | ||
| result.addIfNotNull(IDFA, ids.appleIdfa, ::normalize) | ||
| result.addIfNotNull(GAID, ids.googleGaid, ::normalize) |
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.
do we need to normalize GAID - maybe it is worth passing it as is, as system returns it?
| result.addIfNotNull(POSTAL, ids.postalCode, ::normalize) | ||
| result.addIfNotNull(IPV4, ids.ipv4Address, ::removeWhitespaces) | ||
| result.addIfNotNull(IPV6, ids.ipv6Address, ::normalize) | ||
| result.addIfNotNull(IDFA, ids.appleIdfa, ::normalize) |
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.
this is kinda funny to have on Android :) but let it be
| * We first convert the Uri to a lowercase string then re-parse it so that we are | ||
| * not dependent on case-sensitivity of the "oeid" query parameter | ||
| */ | ||
| fun eidFromUrl(urlString: String): String? { |
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.
this name is a bit confusing, as eid is an extended id in OpenRTB, here we talk rather about a prefixed ID param
| /** | ||
| * Generates a list of encoded Enriched Identifiers (EIDs) based on the current contents of the dictionary. | ||
| */ | ||
| fun generateEnrichedIds(): List<String> { |
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.
EIDs are Extended Identifiers, not EnrichedIds: https://github.com/InteractiveAdvertisingBureau/openrtb2.x/blob/main/2.6.md#3227---object-eid-
Please correct to generateEIDs - also please verify what's the function name in iOS if it is meant to be a public API - need to make sure we call it the same
| class OptableIdentifiersTest { | ||
|
|
||
| @Test | ||
| fun `generateEnrichedIds empty`() { |
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.
see a comment about naming in the OptableIdentifiers, that EID means "extended identifier", not "enriched identifier"
Closes #33.
Breaking changes:
apppath parameter.tenantandoriginSlugquery parameters.v2.Other features:
apiKey.Configclass and reuse it everywhere.GoogleAdIdManagerandLocalStoragelogic from the network client.ClienttoNetworkClient.TypeHasherclass.