-
Notifications
You must be signed in to change notification settings - Fork 2k
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
A1Media RTD module : initial release #10141
A1Media RTD module : initial release #10141
Conversation
modules/a1MediaRtdProvider.js
Outdated
const day = `0${d.getDate()}`.slice(-2); | ||
const todaysDate = `${d.getFullYear()}${month}${day}`; | ||
|
||
const scriptUrl = `${SCRIPT_URL}/${todaysDate}/${tagname}.min.js`; |
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.
Can you add some discussion of why you need external script loaded to return segments to your documentation? Is there a reason you cannot lock this script to a version instead of this date stuff?
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 reason for needing an external script is not just to acquire data from the current page, but also to analyze accumulated data periodically. Instead of using a version, the script is loaded based on the date for a specific reason. The A1Media script has many customizable elements for the deployed site. Therefore, there is an issue where the script needs to be changed immediately upon request from the site. By loading the script based on the date, it ensures that the script is refreshed within a maximum of one day.
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 add this to your documentation
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 added this to markdown file and prebid/prebid.github.io#4672 too
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.
Thanks! Publishers generally prefer non-dynamic scripts so you don't break them with changes. We'll have to review this requested exception in committee.
This architecture, where each publisher has a date and pub specific script is unique. Perhaps it would be best to just not link to it from the project at all and take the endpoint as configuration.
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.
Ok - sorry- I'll admit I thought you guys were a bid adapter at first. An RTD module is more permissive and nuanced. That rule states:
A Real-Time Data module may load external code if it requires publisher registration and there’s a prominent disclosure on the module documentation. The idea is that a publisher will not include the module if they don’t approve of the external code, and since they’ve registered for the service, they must approve. The text of the disclosure may differ if the vendor allows Prebid to do regular reviews of a strictly versioned proprietary library.
However, I don't understand what benefit you gain by putting the date in the URL. We will discuss it again internally. The proposed documented disclosure message was way too complicated. If we allow RTD modules to break this rule than publishers should not have to understand the date nuance. If anything, such a disclosure should say something simple like:
This module loads unverified external code that could change without warning.
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.
As advised, we changed architecture to load external script to non-dynamic.
It just loads the script using the name passed from configuration.
modules/a1MediaRtdProvider.js
Outdated
const a1gid = getStorageData(A1_AUD_KEY); | ||
const a1Ortb2 = { | ||
user: { | ||
buyeruid: a1gid, |
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 the wrong place I'm sure; can you add some detail on this id and perhaps we can find the right place.
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.
It is a 1st party id that assigned to users by A1Media for the purpose of analysis and targeting.It is a 1st party user id. If you suggest the right place, we consider update that and user.ext.a1tg(below issue).
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.
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 moved from user.buyeruid
to user.ext.eids
buyeruid: a1gid, | ||
ext: { | ||
a1tg: a1seg | ||
} |
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.
Also appears incorrect; why not use user.data object and register a segtax? Otherwise, use user.ext.data
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 moved from user.ext.a1tg
to user.data
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.
If you're going to use user.data, please reserve a segtax id with a pr to segtax.md on iabtl GitHub and populate it in this object as well
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 registered segtax ( InteractiveAdvertisingBureau/openrtb#136 )
and added segtax id
expect(subModuleObj.init(configWithParams)).to.be.true; | ||
expect(window.linkback.l).to.be.true; | ||
expect(loadExternalScript.called).to.be.true; | ||
expect(loadExternalScript.args[0][0]).to.deep.equal('https://linkback.contentsfeed.com/src/20230610/lb4test.min.js'); |
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.
Fails
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.
updated comparing tag url.
modules/.submodules.json
Outdated
@@ -77,7 +77,8 @@ | |||
"sirdataRtdProvider", | |||
"timeoutRtdProvider", | |||
"weboramaRtdProvider", | |||
"zeusPrimeRtdProvider" | |||
"zeusPrimeRtdProvider", | |||
"a1MediaRtdProvider" |
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.
these are alphabetical
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 fixed it to alphabetical
modules/a1MediaRtdProvider.js
Outdated
const a1gid = getStorageData(A1_AUD_KEY); | ||
const a1Ortb2 = { | ||
user: { | ||
buyeruid: a1gid, |
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.
- add reason for load external script to Documentation - fix 1st party id and segment place - update rtdModule by alphabetical
We fixed for requested changes. thank you |
- add segtax to user.data
* Create A1Media RTD Submodule * update description * add import expect property from chai for test * fix fake timestamp for test * Fix for Requested Changes - add reason for load external script to Documentation - fix 1st party id and segment place - update rtdModule by alphabetical * update load script to non-dynamic - add segtax to user.data --------- Co-authored-by: ChangsikChoi <>
Type of change
Description of change
Add A1Media RTB provider