-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
Geolocation RTD module: initial release #10012
Conversation
modules/geolocationRtdProvider.js
Outdated
if (done) return; | ||
done = true; | ||
geolocation && adUnits && adUnits.forEach((unit) => { | ||
unit.bids && unit.bids.forEach(bid => bid.geolocation = cleanObj(geolocation)); |
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.
Instead of tacking this on to bid requests, it's IMO better to fill this out as FPD (setting the fields in requestBidsObject.ortb2Fragments.global.device.geo
). That way a number of bidders will automatically forward that data, and activity controls will automatically control the accuracy.
Otherwise this relies entirely on more work in bid adapters so that they may pick this up. Looking at the spec it looks like most of the fields have a spot with the exception of those about velocity, I'd still put those somewhere under geo.ext
.
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'd prefer the browser doesn't even ask for precise geo unless https://github.com/InteractiveAdvertisingBureau/Global-Privacy-Platform/blob/87935c4b574e974ff20ff5c1b9641374878031e2/Sections/US-National/IAB%20Privacy%E2%80%99s%20National%20Privacy%20Technical%20Specification.md?plain=1#L229 permitted it, or in the EU, special Feature number one. This seems preferable to geo gets asked for then rounded without the proper consent. Does activityControls allow for that? we'd want to prevent invoking this altogether, not just manipulating its input to bid requests.
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.
@patmmccann just to be clear, i need to check consent or just ensure that consent string provided ?
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.
@AdmixerTech after discussing this offline, this should not use the browser API if the transmitPreciseGeo
activity is denied. Activities are very new, but the basic idea is to do something like
if (isActivityAllowed(ACTIVITY_TRANSMIT_PRECISE_GEO, activityParams(MODULE_TYPE_RTD, 'geolocation'))) {
// call browser geo api
}
Some examples of the activity API in use for reference:
Prebid.js/src/storageManager.js
Lines 32 to 35 in bdd56a7
valid: isAllowed(ACTIVITY_ACCESS_DEVICE, activityParams(moduleType, mod, { | |
[ACTIVITY_PARAM_STORAGE_TYPE]: storageType | |
})) | |
}; |
Prebid.js/src/adapterManager.js
Lines 281 to 285 in bdd56a7
function isS2SAllowed(s2sConfig) { | |
return dep.isAllowed(ACTIVITY_FETCH_BIDS, activityParams(MODULE_TYPE_PREBID, PBS_ADAPTER_NAME, { | |
[ACTIVITY_PARAM_S2S_NAME]: s2sConfig.configName | |
})); | |
} |
modules/geolocationRtdProvider.js
Outdated
}); | ||
}); | ||
if (auctionDelay > 0) { | ||
setTimeout(complete, auctionDelay); |
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.
the rtd parent module handles auction delay and waitforit, no need for you to
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.
remove auction delay move geo to ortb2Fragments.global.device.geo
modules/geolocationRtdProvider.js
Outdated
} | ||
function init(moduleConfig) { | ||
geolocation = void 0; | ||
if (!isFn(navigator?.permissions?.query) || !isFn(navigator?.geolocation?.getCurrentPosition)) { |
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.
we need to suppress this behavior when gpp sid 7 indicates no consent to precise location or GDPR special feature 1 doesnt exist for the vendor(s) https://vendor-list.consensu.org/v2/vendor-list.json .
this doesnt seem to be blocked by additional activity control work
The ID PMC agrees that the module should first check the CMP api to determine whether the user has revoked location sharing before prompting the user for permission to geo locate. |
Generally this is looking pretty good; we should add some discussion of interaction with activity control to the doc |
Hi! Here are the docs prebid/prebid.github.io#4680 |
hi! the docs are ready. can you please put a label on that this pr is ready to be merged? |
Thanks! @dgirardi could you give another sign off? |
Hi @dgirardi ! Can you please check this pull request? Also I have a question when next release is planned? |
* Update README.md update * add geolocation rtd provider * move config to prams remove auction delay move geo to ortb2Fragments.global.device.geo * tslint * geolocation cmp allowance * lint fixes * test fixes * test fixes * testing * testing 2 * testing 3 * testing 4 --------- Co-authored-by: Daria Boyko <dariaboiko03@gmail.com> Co-authored-by: dariaboyko <73283727+dariaboyko@users.noreply.github.com>
Type of change
Description of change
add geolocation rtd provider
Other information
prebid.github.io pull #4607