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

Native: privacyLink is now converted to ortb.privacy #10271

Merged
merged 3 commits into from
Aug 10, 2023

Conversation

musikele
Copy link
Contributor

Type of change

  • Bugfix

Description of change

In legacy native, users could request privacyLink. Current prebid ORTB implementation was not considering this value. We decided that, if privacyLink: { required: true }, we set ortb.privacy = 1.

Other information

Discussion: #10249

@jsnellbaker
Copy link
Collaborator

Will there be changes needed to support the response side of things; if a bidder returns a privacyLink URL in their bid response, is there logic to handle it in the nativeOrtbResponse and convert it for the legacy response (to the bid.native.privacyLink field)?

@@ -480,6 +480,11 @@ export function toOrtbNativeRequest(legacyNativeAssets) {
continue;
}

if (key === 'privacyLink') {
ortb.privacy = 1;
Copy link
Collaborator

Choose a reason for hiding this comment

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

If I say privacyLink: {required: false} should this still be set to 1?

Copy link
Collaborator

Choose a reason for hiding this comment

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

in ORTB the flag signals support and not whether the link is required, so maybe this is OK - but we should set required: false in the inverse logic?

Copy link
Contributor Author

@musikele musikele Aug 1, 2023

Choose a reason for hiding this comment

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

My takes on this

If I say privacyLink: {required: false} should this still be set to 1?

I'd say the presence of the attribute means that you'd like to set privacy to 1.

in ORTB the flag signals support and not whether the link is required, so maybe this is OK - but we should set required: false in the inverse logic?

Agree, will fix

@dgirardi
Copy link
Collaborator

I believe this also needs a change in legacyPropertiesToOrtbNative, to address @jsnellbaker's point.

@musikele musikele requested a review from dgirardi August 1, 2023 13:26
src/native.js Outdated
@@ -696,8 +704,11 @@ export function legacyPropertiesToOrtbNative(legacyNative) {
// in general, native trackers seem to be neglected and/or broken
response.jstracker = Array.isArray(value) ? value.join('') : value;
break;
case 'privacyLink':
response.privacy = 1;
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 value be a string of the URL that the adapters return for the old privacyLink; which I suppose should be the value variable for this particular function?

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 you are definitely right ----

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.

Assuming we're not doing any privacyIcon work in this PR (see #10249 (comment)), I think these changes look good and should cover the different scenarios of converting the values back/forth.

@patmmccann patmmccann merged commit 34846c7 into prebid:master Aug 10, 2023
4 checks passed
santii7395 pushed a commit to themaven-net/Prebid.js that referenced this pull request Aug 28, 2023
* fix for privacyLink in native prebid#10249

* handle privacy link in response too

* privacy link should be returned in response

---------

Co-authored-by: Michele Nasti <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants