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

JW Player RTD Module : populate content url, title and description #11178

Merged
merged 19 commits into from
Apr 11, 2024

Conversation

karimMourra
Copy link
Collaborator

@karimMourra karimMourra commented Mar 6, 2024

Type of change

  • Feature

  • Does this change affect user-facing APIs or examples documented on http://prebid.org?

Description of change

Includes content url, title and description when enriching the ortb.site.content.

Other information

Docs PR: prebid/prebid.github.io#5190

@ChrisHuie ChrisHuie changed the title [JW Player RTD Module]: Populate content url, title and description JW Player RTD Module : populate content url, title and description Mar 11, 2024
modules/jwplayerRtdProvider.js Outdated Show resolved Hide resolved
modules/jwplayerRtdProvider.js Outdated Show resolved Hide resolved
@@ -329,15 +353,30 @@ export function addOrtbSiteContent(ortb2, contentId, contentData) {
content.id = contentId;
}

const currentData = content.data = content.data || [];
if (contentUrl) {
content.url = contentUrl;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Should this override what might be in content already?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes! If a publisher has requested that we enrich the bid request for a given media, then they have determined that the bid request is specific to that media, therefore we should override existing data in that field.

Unfortunately the oRTB spec only has room for 1 content description, which is unrealistic given that multiple videos could be rendering simultaneously on the page.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this feels concerning, let's discuss in committee

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@patmmccann and @dgirardi this has been address by adding configuration params

karimMourra and others added 3 commits March 11, 2024 17:27
Co-authored-by: Demetrio Girardi <demetrio.girardi@proton.me>
Co-authored-by: Demetrio Girardi <demetrio.girardi@proton.me>
@karimMourra
Copy link
Collaborator Author

@patmmccann what does the "on hold" label mean ? Does it block this from getting merged; If so, how can we resolve?

@patmmccann
Copy link
Collaborator

Yes this blocked from merging, it is a breaking change

Copy link
Collaborator

@patmmccann patmmccann left a comment

Choose a reason for hiding this comment

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

This is a breaking change, please require new configuration to enable overwriting existing fields

@@ -329,15 +353,30 @@ export function addOrtbSiteContent(ortb2, contentId, contentData) {
content.id = contentId;
}

const currentData = content.data = content.data || [];
if (contentUrl) {
content.url = contentUrl;
Copy link
Collaborator

Choose a reason for hiding this comment

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

this feels concerning, let's discuss in committee

@@ -71,15 +81,31 @@ export function fetchTargetingInformation(jwTargeting) {
});
}

export function setOverrides(params) {
// For backwards compatibility, default to always unless overridden by Publisher.
overrideContentId = sanitizeOverrideParam(params.overrideContentId, ENRICH_ALWAYS);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Assuming this should be updated for 9, do you have a way to track it - would a TODO comment be helpful?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

added!

@karimMourra
Copy link
Collaborator Author

@patmmccann can we remove the on hold and merge ?

@patmmccann patmmccann merged commit 458036a into prebid:master Apr 11, 2024
4 checks passed
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.

4 participants