-
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
Category translation module for adpod #3513
Conversation
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 added some comments/questions below. Please take a look when you have the chance. Thanks.
modules/categoryTranslation.js
Outdated
logError('Translation mapping data not found in local storage'); | ||
} | ||
} | ||
return fn.call(this, adUnitCode, bid); |
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.
Just merely a note - the return
here could be removed; it's not needed by the hook functionality unless you're terminating the function early.
src/adapters/bidderFactory.js
Outdated
@@ -341,6 +346,47 @@ export function newBidder(spec) { | |||
} | |||
} | |||
|
|||
if (hooks['checkAdUnitSetup']) { |
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.
To clarify the intent here - is this to check if the hookpoint is there? Or is it to check if you already have a hook function setup in the hook point? If the latter, you'll have to use the hooks['checkAdUnitSetup'].getHooks()
function to read array.
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.
Actually i am getting this error.
"Uncaught TypeError: Cannot read property 'before' of undefined
Hence added this check. Let me know if you have some suggestion.
modules/categoryTranslation.js
Outdated
} | ||
|
||
export function initTranslation() { | ||
hooks['addBidResponse'].before(getFreeWheelCategoryHook, 50); |
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.
Shouldn't this and the other FW specific stuff exist outside this module and in the freewheeladservervideo
file instead?
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.
Purpose of this hook is to convert iab sub category to adserver category. Name is misleading so i updated the name to getAdserverCategoryHook
src/utils.js
Outdated
|
||
export function getDataFromLocalStorage(key) { | ||
if (hasLocalStorage()) { | ||
window.localStorage.getItem(key); |
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.
Should there be a return
here?
@jsnellbaker @matthewlane Updated |
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.
LGTM
nice progress. One suggestion: Can we make |
Thanks. I will update the key to be optional. |
This pull request introduces 1 alert when merging 2b81856 into c2734a7 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
This pull request introduces 1 alert when merging e70ba4e into c2734a7 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
modules/categoryTranslation.js
Outdated
} | ||
|
||
export function initTranslation(...args) { | ||
hooks['addBidResponse'].before(getAdserverCategoryHook, 50); |
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 hook add part needs to be checked if it's already been setup. When using the setConfig
option, this function is being setup twice.
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.
LGTM
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.
looks good! Just a few changes requested.
modules/categoryTranslation.js
Outdated
import { timestamp, logError, setDataInLocalStorage, getDataFromLocalStorage } from '../src/utils'; | ||
|
||
// TODO udpate url once it is uploaded on cdn | ||
const DEFAULT_TRANSLATION_FILE_URL = 'http://acdn.adnxs.com/prebid/test/jp/freewheel-mapping.json'; |
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 shouldn't hardcode a protocol here. Use a protocol-less URL to support HTTPS.
This file can also be minified.
modules/categoryTranslation.js
Outdated
import { ajax } from '../src/ajax'; | ||
import { timestamp, logError, setDataInLocalStorage, getDataFromLocalStorage } from '../src/utils'; | ||
|
||
// TODO udpate url once it is uploaded on cdn |
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.
TODO can removed.
const DEFAULT_TRANSLATION_FILE_URL = 'http://acdn.adnxs.com/prebid/test/jp/freewheel-mapping.json'; | ||
const DEFAULT_IAB_TO_FW_MAPPING_KEY = 'iabToFwMappingkey'; | ||
const DEFAULT_IAB_TO_FW_MAPPING_KEY_PUB = 'iabToFwMappingkeyPub'; | ||
const refreshInDays = 1; |
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.
should we increase default to 7?
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 have kept it 1 because if publisher changes the mapping file, localStorage will be updated in everyone's browser within 24 hours. So per day 1 request.
@@ -345,6 +347,67 @@ export function newBidder(spec) { | |||
} | |||
} | |||
|
|||
export function preloadBidderMappingFile(fn, adUnits) { | |||
let adPodBidders = adUnits | |||
.filter((adUnit) => deepAccess(adUnit, 'mediaTypes.video.context') === ADPOD) |
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.
Should we also check if adpod.brandCategoryExclusion
is true before loading these files? We only need brand category in that case.
src/utils.js
Outdated
} | ||
|
||
export function hasLocalStorage() { | ||
return !!window.localStorage; |
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.
wrap in try/catch. If localstorage is disabled it will throw.
src/adapters/bidderFactory.js
Outdated
let info = bidderSpec.getSpec().getMappingFileInfo(); | ||
let key = (info.localStorageKey) ? info.localStorageKey : bidderSpec.getSpec().code; | ||
let mappingData = getDataFromLocalStorage(key); | ||
if (!mappingData || timestamp() < mappingData.lastUpdated + info.refreshInDays * 24 * 60 * 60 * 1000) { |
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 we make info.refreshInDays
optional? If undefined use the module default.
modules/categoryTranslation.js
Outdated
* 'translationFile': 'http://sample.com' | ||
* } | ||
* }); | ||
* If publisher has not defined translation file than prebid will use default prebid translation file provided here <TODO add url once it is uploaded on cdn> |
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.
Todo here.
This pull request introduces 2 alerts when merging e7e485e into 36f1230 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
This pull request introduces 1 alert when merging d31d8d9 into 36f1230 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
This pull request introduces 1 alert when merging 2e8fe49 into 36f1230 - view on LGTM.com new alerts:
Comment posted by LGTM.com |
Type of change
Description of change
This module adds category translation module to Prebid. This PR serves two purposes.
If Demand partner chooses to use Prebid api, they need to update their adapter to add following function to spec file.
Prebid core will preload mapping file and store in local storage so that when bid responses are back category conversion is seamless. We are storing in localStorage because each adapter can have 20-30 bids depending on size of adpod. This will prevent http calls for each conversion.
To get iab sub category use
Second purpose as mentioned is to convert iabSubCatId to adServerCatId. This is mainly for publishers. Publisher can set brand category translation file using
This file will also be stored in localStorage. If publisher opts not to use their own file, Prebid will use its default mapping file for this conversion.
Other Information
Mapping file structure will be shared soon.