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.

Sign up for GitHub

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

Jump to bottom

Gumgum: ADTS-134 Fetch IDL envelope and pass to ad server if available #7095

Merged
merged 1 commit into from
Jun 29, 2021
Merged

Gumgum: ADTS-134 Fetch IDL envelope and pass to ad server if available #7095

merged 1 commit into from
Jun 29, 2021

Conversation

lbenmore
Copy link
Contributor

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

Check for the availability of IDL envelope from LiveRamp and send with request

@lbenmore lbenmore force-pushed the ADTS-134-fetch-idl-envelope branch 3 times, most recently from 5a30fe6 to 32b344a Compare June 24, 2021 17:08
FilipStamenkovic
FilipStamenkovic suggested changes Jun 24, 2021
View reviewed changes
Copy link
Contributor

@FilipStamenkovic FilipStamenkovic left a comment

Choose a reason for hiding this comment

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

Please open docs PR to include userIds to your bid adapter.

Also, you have separate code for trade desk id, in function:

_getTradeDeskIDParam

Perhaps you don't need this function now?

@lbenmore lbenmore force-pushed the ADTS-134-fetch-idl-envelope branch from f92eb5e to 37689c4 Compare June 24, 2021 17:51
@lbenmore
Copy link
Contributor Author

@FilipStamenkovic thanks for pointing that out! i've consolidated how we're sending userId values and made it more flexible. will update with docs PR shortly

@lbenmore lbenmore force-pushed the ADTS-134-fetch-idl-envelope branch from 37689c4 to 417f654 Compare June 24, 2021 17:54
@lbenmore
Copy link
Contributor Author

@FilipStamenkovic oh sorry, this change actually doesn't affect our documentation as this is not a new parameter expected but a retrieval of available data

@FilipStamenkovic
Copy link
Contributor

@lbenmore I know you didn't add any new parameter, but you need to update your supported userIds in the docs.
Right now, it's stated that you support only unifiedId:
https://github.com/prebid/prebid.github.io/blob/master/dev-docs/bidders/gumgum.md

You need to add other ids.

@FilipStamenkovic
Copy link
Contributor

Some of these ids you are not passing correctly.
For some of them you are passing: [object Object] in your request (see screenshot).

image

Can you fix this?

@lbenmore
Copy link
Contributor Author

lbenmore commented Jun 25, 2021 •
edited
Loading

@FilipStamenkovic, ah I see. I've updated the docs with a PR here: prebid/prebid.github.io#3066. I also added modifications so that we should now be able to send proper ID values for any fields given, though our ad server is currently only supporting the unified ID and identity link

FilipStamenkovic
FilipStamenkovic suggested changes Jun 28, 2021
View reviewed changes
modules/userId/eids.js Outdated
@@ -215,7 +215,7 @@ const USER_IDS_CONFIG = {
};

// this function will create an eid object for the given UserId sub-module
function createEidObject(userIdData, subModuleKey) {
export function createEidObject(userIdData, subModuleKey) {
Copy link
Contributor

Choose a reason for hiding this comment

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

You shouldn't be modifying pbjs core with this PR.

modules/gumgumBidAdapter.js Outdated
@@ -3,6 +3,7 @@ import * as utils from '../src/utils.js'
import { BANNER, VIDEO } from '../src/mediaTypes.js';

import { config } from '../src/config.js'
import { createEidObject } from './userId/eids.js'
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of modifying pbjs, try using: createEidsArray from eids.js.
It's already exported and I see it's being used by a lot of other bidders already.

@lbenmore lbenmore force-pushed the ADTS-134-fetch-idl-envelope branch from 0991ab7 to e6eaa89 Compare June 28, 2021 17:01
@lbenmore
Copy link
Contributor Author

@FilipStamenkovic updated to not use createEidObject function (and remove export of it). our ad server is expecting these values named in a way that doesn't work with createEidsArray so our getEids function has been modified to retrieve and set the values accordingly

@FilipStamenkovic FilipStamenkovic merged commit 4136adb into prebid:master Jun 29, 2021
@lbenmore lbenmore deleted the ADTS-134-fetch-idl-envelope branch January 4, 2022 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@FilipStamenkovic FilipStamenkovic FilipStamenkovic approved these changes

Assignees

@FilipStamenkovic FilipStamenkovic

Labels
feature minor
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lbenmore @FilipStamenkovic @jsnellbaker