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

Prebid Server: fixes User ID support, also adds OpenRTB 'cur' property #3884

Closed
wants to merge 6 commits into from
Closed

Prebid Server: fixes User ID support, also adds OpenRTB 'cur' property #3884

wants to merge 6 commits into from

Conversation

idettman
Copy link
Contributor

@idettman idettman commented Jun 4, 2019 •
edited
Loading

Type of change

  • Bugfix

Description of change

Other information

Prebid.js issue #3627

@idettman idettman force-pushed the fix-prebidserver-userid-support branch 2 times, most recently from 5de59ce to 4bf46ee Compare June 5, 2019 00:51
@idettman idettman changed the title Fix Prebid Server UserId Support Prebid Server Adapter: Add ortb cur, fix for userid pbserver adapter Jun 5, 2019
@idettman idettman changed the title Prebid Server Adapter: Add ortb cur, fix for userid pbserver adapter Prebid Server Adapter: add ortb cur, fix for userid by prebid server adapter Jun 5, 2019
@idettman idettman added needs 2nd review Core module updates require two approvals from the core team needs review labels Jun 5, 2019
@idettman idettman force-pushed the fix-prebidserver-userid-support branch from 30c24ec to ee98402 Compare June 5, 2019 23:48
@idettman idettman requested a review from jsnellbaker June 5, 2019 23:53
@idettman idettman changed the title Prebid Server Adapter: add ortb cur, fix for userid by prebid server adapter Prebid Server: fixes User ID support, userid data, adds OpenRTB 'cur' Jun 5, 2019
@idettman idettman changed the title Prebid Server: fixes User ID support, userid data, adds OpenRTB 'cur' Prebid Server: fixes User ID support, also adds OpenRTB 'cur' property Jun 6, 2019
jsnellbaker
jsnellbaker requested changes Jun 7, 2019
View reviewed changes
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.

Hi @idettman

Thanks for putting these changes together; it overall looks good. There are some thoughts/questions I had at some areas which I noted down below.

Important note however - I ran into an issue while testing the prebidServer adapter. Can you take a look into it?

modules/prebidServerBidAdapter/index.js Outdated
// pass user id values from the User ID Module if enabled
const userId = utils.deepAccess(bidRequests, '0.bids.0.userId');
if (!utils.isEmpty(userId)) {
Object.assign(request.user.ext.tpid, userId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure if this error is caused by this logic, but when I loaded the prebidServer_example.html without any cookies - I got a 400 Bad Request from the ORTB request.

The error was:
Invalid request: request.user.ext object is not valid: json: cannot unmarshal object into Go struct field ExtUser.tpid of type []openrtb_ext.ExtUserTpID

Inside the request object, the user field had the following:
"user":{"ext":{"tpid":{}}}

Copy link
Collaborator

Choose a reason for hiding this comment

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

@jsnellbaker - can you paste the ORTB request here? Would like try it against PBS-Go and PBS-Java.

Copy link
Collaborator

@jsnellbaker jsnellbaker Jun 10, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

@bretg sure, please see below:

{"id":"348d5124-3a91-41a3-8715-66f8eee2ef1b","source":{"tid":"348d5124-3a91-41a3-8715-66f8eee2ef1b"},"tmax":1000,"imp":[{"id":"div-gpt-ad-1460505748561-0","ext":{"appnexus":{"use_pmt_rule":false,"placement_id":13144370}},"banner":{"format":[{"w":300,"h":250}]}}],"test":1,"ext":{"prebid":{"targeting":{"includewinners":true,"includebidderkeys":false}}},"site":{"publisher":{"id":"1"},"page":"http://test.localhost:9999/integrationExamples/gpt/prebidServer_example.html?pbjs_debug=true"},"device":{"w":1680,"h":291},"user":{"ext":{"tpid":{}}}}

Copy link
Collaborator

Choose a reason for hiding this comment

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

You're right - PBS-Java gives this error: Invalid request format: request.user.ext.tpid[0].source missing required field: "source". Removing "tpid":{} clears it up.

@idettman - please check this case -- if the tpid object is empty, we can't send it or PBS chokes. Thanks.

modules/prebidServerBidAdapter/index.js Outdated
request.ext.prebid = Object.assign(request.ext.prebid, _s2sConfig.extPrebid);
}

_appendSiteAppDevice(request);

const digiTrust = _getDigiTrustQueryParams();
if (digiTrust) {
request.user = { ext: { digitrust: digiTrust } };
if (!request.user || !request.user.ext) {
request.user = { ext: { digitrust: digiTrust } };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just to clarify on something, if there was a case where request.user existed but request.user.ext didn't - this logic would overwrite the previous value(s) under the user object. In looking at the surrounding logic/code, I don't think this would be an issue - but wanted to raise it just to be sure.

If there is a doubt, perhaps this could be moved a bit lower (around were we're checking for user.tpid)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah the possibility of a user obj with properties being overwritten was real, I like the idea of moving the block down

modules/prebidServerBidAdapter/index.js Outdated
} else {
request.user.ext = { consent: consentString };
request.user = { ext: { consent: consentString } };
Copy link
Collaborator

Choose a reason for hiding this comment

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

Given the series of nested object assignments we're using in this part of the file, would it make sense to create a new utils function similar to deepAccess where it will go through the object chain provided and try to assign the value to the deepest level that exists? This would help simplify some of the code around here.

@idettman
Copy link
Contributor Author

Ready for re-review

@bretg
Copy link
Collaborator

bretg commented Jun 17, 2019

Sorry Isaac, but the user.ext.tpid part of the PR needs to be deferred due to #3900.

@idettman idettman closed this Jun 17, 2019
@idettman idettman deleted the fix-prebidserver-userid-support branch July 24, 2019 11:00
@idettman idettman restored the fix-prebidserver-userid-support branch July 24, 2019 11:00
@idettman idettman deleted the fix-prebidserver-userid-support branch July 24, 2019 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jsnellbaker jsnellbaker jsnellbaker requested changes

@bretg bretg bretg left review comments

@harpere harpere Awaiting requested review from harpere

@jaiminpanchal27 jaiminpanchal27 Awaiting requested review from jaiminpanchal27

Assignees

@idettman idettman

Labels
needs review needs 2nd review Core module updates require two approvals from the core team on hold
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@idettman @bretg @jsnellbaker