Skip to content
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

Merged
merged 19 commits into from
Feb 27, 2019
Merged

Conversation

jaiminpanchal27
Copy link
Collaborator

@jaiminpanchal27 jaiminpanchal27 commented Feb 4, 2019

Type of change

  • Feature

Description of change

This module adds category translation module to Prebid. This PR serves two purposes.

  • Provide api to convert demand partners own category into iab sub category. Usage of api is optional but it is responsibility of adapter to return iab sub category.
  • Convert iab-sub-category to freewheel industry if primary adserver if freewheel and demand partner is not returning adServerCatId in bid response.

If Demand partner chooses to use Prebid api, they need to update their adapter to add following function to spec file.

getMappingFileInfo: function() {
    return {
      url: mappingFileUrl, // mapping file json url
      refreshInDays: 7, // since prebid stores mapping data in localstorage you can return in how many days you want to update value stored in localstorage. 
      localStorageKey: `${spec.code}MappingFile` // some unique key to store your mapping json in localstorage
    }
  },

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

/**
   * This function translates proprietary category to iab sub category using your mapping file
   * @param {string} key key returned by getMappingFileInfo function
   * @param {string} pCategory proprietary category returned in bid response
   * @returns {string} iabSubCatId
   */
  utils.getIabSubCategory('key', 'ownCategory');

Second purpose as mentioned is to convert iabSubCatId to adServerCatId. This is mainly for publishers. Publisher can set brand category translation file using

pbjs.setConfig({
    "brandCategoryTranslation": {
       "translationFile": "<url_to_file>"
    }
});

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.

Sorry, something went wrong.

Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jaiminpanchal27

I added some comments/questions below. Please take a look when you have the chance. Thanks.

logError('Translation mapping data not found in local storage');
}
}
return fn.call(this, adUnitCode, bid);
Copy link
Collaborator

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.

@@ -341,6 +346,47 @@ export function newBidder(spec) {
}
}

if (hooks['checkAdUnitSetup']) {
Copy link
Collaborator

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.

Copy link
Collaborator Author

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.

}

export function initTranslation() {
hooks['addBidResponse'].before(getFreeWheelCategoryHook, 50);
Copy link
Collaborator

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?

Copy link
Collaborator Author

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);
Copy link
Collaborator

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?

@jaiminpanchal27
Copy link
Collaborator Author

@jsnellbaker @matthewlane Updated

Copy link
Collaborator

@matthewlane matthewlane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mkendall07
Copy link
Member

nice progress. One suggestion: Can we make key optional and default based on bidderCode? That way the adapter doesn't have to pass it around.
utils.getIabSubCategory is only for getting IAB category not FW cat right? FW cat is done automatically?

@jaiminpanchal27
Copy link
Collaborator Author

Thanks. I will update the key to be optional.
Yes utils.getIabSubCategory is only for getting IAB category. Translation to adServerCatId happenes automatically.

@mkendall07
Copy link
Member

This pull request introduces 1 alert when merging 2b81856 into c2734a7 - view on LGTM.com

new alerts:

  • 1 for Useless conditional

Comment posted by LGTM.com

@mkendall07
Copy link
Member

This pull request introduces 1 alert when merging e70ba4e into c2734a7 - view on LGTM.com

new alerts:

  • 1 for Useless conditional

Comment posted by LGTM.com

}

export function initTranslation(...args) {
hooks['addBidResponse'].before(getAdserverCategoryHook, 50);
Copy link
Collaborator

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.

Copy link
Contributor

@idettman idettman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Member

@mkendall07 mkendall07 left a 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.

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';
Copy link
Member

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.

import { ajax } from '../src/ajax';
import { timestamp, logError, setDataInLocalStorage, getDataFromLocalStorage } from '../src/utils';

// TODO udpate url once it is uploaded on cdn
Copy link
Member

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;
Copy link
Member

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?

Copy link
Collaborator Author

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)
Copy link
Member

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;
Copy link
Member

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.

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) {
Copy link
Member

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.

* '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>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Todo here.

@mkendall07
Copy link
Member

This pull request introduces 2 alerts when merging e7e485e into 36f1230 - view on LGTM.com

new alerts:

  • 2 for Property access on null or undefined

Comment posted by LGTM.com

@mkendall07
Copy link
Member

This pull request introduces 1 alert when merging d31d8d9 into 36f1230 - view on LGTM.com

new alerts:

  • 1 for Invocation of non-function

Comment posted by LGTM.com

@jaiminpanchal27 jaiminpanchal27 merged commit 1f04f55 into master Feb 27, 2019
@mkendall07
Copy link
Member

This pull request introduces 1 alert when merging 2e8fe49 into 36f1230 - view on LGTM.com

new alerts:

  • 1 for Invocation of non-function

Comment posted by LGTM.com

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants