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

Fix renderers key node on bid request #685

Merged
merged 3 commits into from
Sep 13, 2023

Conversation

github-lucas-bon
Copy link
Collaborator

@github-lucas-bon github-lucas-bon commented Sep 12, 2023

This pull request is to solve the following issue:

This pull request intended to insert renderers on the following node path from the bid request:
.ext.prebid.sdk.renderers

But the implementation did not match the expectation as you can see here, so the current state is
.ext.sdk.renderers

@github-lucas-bon github-lucas-bon self-assigned this Sep 12, 2023
@github-lucas-bon github-lucas-bon marked this pull request as ready for review September 12, 2023 14:36
@github-lucas-bon github-lucas-bon changed the title Fix renderers on bid request on the correct node Fix renderers key node on bid request Sep 12, 2023
@YuriyVelichkoPI
Copy link
Contributor

Hi @github-lucas-bon !

In the server ticket prebid/prebid-server#2908 the ext.prebid.sdk.renderers is discussed. And as for me, it makes sense and even necessary to put this extension into the prebid object.

If there are no other conversations or specs about using ext.sdk.renderers I believe you should create the PBS issue to follow the requirements in the original PBS ticket.

@github-lucas-bon
Copy link
Collaborator Author

github-lucas-bon commented Sep 13, 2023

Hi @github-lucas-bon !

In the server ticket prebid/prebid-server#2908 the ext.prebid.sdk.renderers is discussed. And as for me, it makes sense and even necessary to put this extension into the prebid object.

If there are no other conversations or specs about using ext.sdk.renderers I believe you should create the PBS issue to follow the requirements in the original PBS ticket.

@YuriyVelichkoPI in the ticket you mentioned the structure fixed here is the one that was pushed, here is where exactly it was pushed. The problem is that there was a mismatch between what was decided on PBS and what was implemented on PBM

@YuriyVelichkoPI
Copy link
Contributor

Sorry, @github-lucas-bon, but I still do not understand.

In the pull request for PBS GO, the path is ext.prebid.sdk.renderers. As it discussed in the prebid/prebid-server#2908.

So if the changes for PBS JAVA assume a different path - ext.sdk.renderers, it looks inconsistent. Both should follow the same spec.

What I'm missing here?

@YuriyVelichkoPI
Copy link
Contributor

@github-lucas-bon , it looks like I understand my confusion.

Everything is fine PBS, and the expected path for both GO and GAVA is ext.prebid.sdk.renderers. But SDK uses ext.sdk.renderers to send the data.

Sorry!

Copy link
Contributor

@YuriyVelichkoPI YuriyVelichkoPI left a comment

Choose a reason for hiding this comment

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

LGTM

# Conflicts:
#	PrebidMobile/PrebidMobile-core/src/test/java/org/prebid/mobile/rendering/models/openrtb/bidRequests/PluginRendererListTest.java
@ChrisHuie ChrisHuie merged commit f6d8069 into master Sep 13, 2023
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.

3 participants