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

Pubmatic: Making Adslot param optional #3788

Merged
merged 59 commits into from
May 1, 2019

Conversation

pm-harshad-mane
Copy link
Contributor

Type of change

  • Feature

Description of change

With the addition of default site & ad tag support in at PubMatic Openbid, it's no longer a mandatory requirement to send adslot name parameter.

New behavior:

  • adSlot name should be optional
  • If adSlot name is not present, we should send the first size as w and h, and other sizes in the format array

pm-manasi-moghe and others added 30 commits December 19, 2018 15:13
merging prebid master into fork
merge from prebid master to fork
merge from prebid master to fork
merge from prebid master to fork
Merge prebid master into fork
merge latest prebid into fork master
merge latest prebid from master into branch
merge prebid master into fork
merge latest prebid master into branch
@pm-harshad-mane
Copy link
Contributor Author

Hello @jsnellbaker , @jaiminpanchal27
Can you please review the PR?

@jaiminpanchal27 jaiminpanchal27 self-assigned this Apr 29, 2019
@jaiminpanchal27
Copy link
Collaborator

@pm-harshad-mane Have assigned this to myself. I will review soon

Copy link
Collaborator

@jaiminpanchal27 jaiminpanchal27 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. Minor change requested

});

// commenting following test-case as we are making param adSlot optional
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please remove this code if not needed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted commented test cases

@@ -1002,7 +992,7 @@ export const spec = {
transformBidParams: function (params, isOpenRtb) {
return utils.convertTypes({
'publisherId': 'string',
'adSlot': 'string'
'adSlot': 'string' // TODO: do we need to change here as well?
Copy link
Collaborator

Choose a reason for hiding this comment

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

No this is fine as it is. Type will be converted if it exists

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed the todo comment

@bretg
Copy link
Collaborator

bretg commented May 1, 2019

@jaiminpanchal27 - ready to merge?

@bretg
Copy link
Collaborator

bretg commented May 1, 2019

looks to me like the requested changes were made. Merging.

@bretg bretg merged commit 2302336 into prebid:master May 1, 2019
@bretg
Copy link
Collaborator

bretg commented May 1, 2019

docs PR prebid/prebid.github.io#1299

@pm-harshad-mane
Copy link
Contributor Author

Thank you @bretg , @jaiminpanchal27

jacekburys-quantcast pushed a commit to jacekburys-quantcast/Prebid.js that referenced this pull request May 15, 2019
* changes for multiformat support

* added new constant NATIVE_ASSETS in PubMatic adapter

* removed NATIVE_ASSET_KEY reference in PubMatic adapter

* removed reference of const NATIVE_ASSET_ID from PubMatic adapter

* removed reference of const NATIVE_ASSET_DATA_TYPE in PubMatic adapter

* using _commonNativeRequestObject in PubMatic adapter to avoid repeating code block

* removed new-lines in const declaration

* generating NATIVE_ASSET_REVERSE_ID from NATIVE_ASSETS

* renamed NATIVE_ASSET_REVERSE_ID to NATIVE_ASSET_ID_TO_KEY_MAP

* little modification in _checkParamDataType in PubMatic adapter

* a minor improvement

* using let instead of var

* added NATIVE_ASSET_KEY_TO_ASSET_MAP and combining switch cases

* lint update

* removed some stale comments

* using LOG_WARN_PREFIX

* using const UNDEFINED

* added a logWarn in catch

* using arrow functions

* code review changes

* making adSlot parameter optional

* added a test case for without adSlot config

* changes to checkMediaType function

* suppress warning of missing mediaTypes.banner for native and video impression

* ignore fluid size, if present, in banner impression

* fix for ignoring fluid size in banner impression

* added relevant comments and test cases for fluid case in banner request

* added sample config for multiformat adunit

* commented redundant check

* marking adSlot param as optional in examples

* fixed some lint errors

* removed commnented code

* removed a trailing white space

* removed a todo comment as suggested in review

* deleting commented test cases
VideoReach pushed a commit to VideoReach/Prebid.js that referenced this pull request Aug 1, 2019
* changes for multiformat support

* added new constant NATIVE_ASSETS in PubMatic adapter

* removed NATIVE_ASSET_KEY reference in PubMatic adapter

* removed reference of const NATIVE_ASSET_ID from PubMatic adapter

* removed reference of const NATIVE_ASSET_DATA_TYPE in PubMatic adapter

* using _commonNativeRequestObject in PubMatic adapter to avoid repeating code block

* removed new-lines in const declaration

* generating NATIVE_ASSET_REVERSE_ID from NATIVE_ASSETS

* renamed NATIVE_ASSET_REVERSE_ID to NATIVE_ASSET_ID_TO_KEY_MAP

* little modification in _checkParamDataType in PubMatic adapter

* a minor improvement

* using let instead of var

* added NATIVE_ASSET_KEY_TO_ASSET_MAP and combining switch cases

* lint update

* removed some stale comments

* using LOG_WARN_PREFIX

* using const UNDEFINED

* added a logWarn in catch

* using arrow functions

* code review changes

* making adSlot parameter optional

* added a test case for without adSlot config

* changes to checkMediaType function

* suppress warning of missing mediaTypes.banner for native and video impression

* ignore fluid size, if present, in banner impression

* fix for ignoring fluid size in banner impression

* added relevant comments and test cases for fluid case in banner request

* added sample config for multiformat adunit

* commented redundant check

* marking adSlot param as optional in examples

* fixed some lint errors

* removed commnented code

* removed a trailing white space

* removed a todo comment as suggested in review

* deleting commented test cases
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants