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

TPMN Bid Adapter: Change End-Point & Support Video #10611

Merged
merged 12 commits into from
Nov 13, 2023

Conversation

tpmn-admin
Copy link
Contributor

@tpmn-admin tpmn-admin commented Oct 16, 2023

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

  1. Change End-Point
  2. Change Request Format (payload -> Ortb)
  3. Support Video MediaType
  4. Support OutStream Video
  5. Update Document
  6. Add Example (/integrationExamples/gpt/tpmn_example.html)

Other information

Copy link

@AbdullahWins AbdullahWins left a comment

Choose a reason for hiding this comment

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

Good Job!

}
if (bid.params.inventoryId) rtbData.ext = {};
if (bid.params.inventoryId) rtbData.ext.inventoryId = bid.params.inventoryId
if (bid.params.bcat) rtbData.bcat = bid.params.bcat;
Copy link
Collaborator

Choose a reason for hiding this comment

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

you have accept openrtb specification of bcat as well, you can take your params as an override

Copy link
Contributor Author

Choose a reason for hiding this comment

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

da8e928

update openrtb block attributes


function getBannerSizes(bidRequest) {
return parseSizes(deepAccess(bidRequest, 'mediaTypes.banner.sizes') || bidRequest.sizes);
VIDEO_ORTB_PARAMS.forEach((param) => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

this seems unnecessary; the ortb convertor is already doing all this for you. Is that correct @dgirardi ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

i think.. this is needed to check which parameters are allowed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this example does this with less code, as long as the parameters you're checking match the ones used in ortbConverter

Copy link
Contributor Author

@tpmn-admin tpmn-admin Oct 18, 2023

Choose a reason for hiding this comment

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

2699115

I modified the code in the direction you suggested. ([this example] (https://github.com/prebid/Prebid.js/blob/master/libraries/ortbConverter/README.md#example-taking-video-parameters-from-bidrequestparamsvideo) )


Revert "remove VIDEO_ORTB_PARAMS, BANNER_ORTB_PARAMS"

I followed your recommendations, but the desired results were different.
First of all, the test case did not work as expected.

  1. It is judged that overrides processing does not operate normally in TestCase.
  2. When I tried to compare the contents of 'buildRequests()' in TestCase, it did not match.

'mimes',
'minduration',
'maxduration',
'placement',
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 missing plcmt

Copy link
Contributor Author

Choose a reason for hiding this comment

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

792fc2c
add plcmt

8bf7567
instream video is sound on default, outstream video sound off default

@tpmn-admin
Copy link
Contributor Author

Hello. @dgirardi

I would like to request a review of the TPMN prebid.js pull request once again.

Regarding the code simplification using the previously mentioned 'ortbConverter,' we have decided to 'Revert' it.

We initially aimed to streamline the code using 'ortbConverter,' but the results generated using 'buildRequests' in the test cases behaved differently than expected.

In the test case 'should use bidder video params if they are set,' it does not perform as desired. We would like the new 'video parameter' settings to properly take effect when specifying them. The issue we encountered is that the new settings are not being reflected correctly, resulting in undesired outcomes.

The current code is working as intended at the moment.

I am requesting a code review in its current state.

Additionally, I'd like to inquire if there are any other points for consideration.

dgirardi added a commit to dgirardi/Prebid.js that referenced this pull request Oct 25, 2023
@dgirardi
Copy link
Collaborator

@tpmn-admin we're always looking to improve so I'm interested in more details on what was not working! I posted here the changes that I would submit for ortbConverter. Some notes:

  • I was not able to reproduce the video test failure, just following the example seems to work
  • My commit still has 1 test failure for ✖ should build correct request for banner bid with both w, h, because ortbConverter sets format instead of w and h. IMO format is better because it allows for multiple sizes, but of course it's your choice.
  • GDPR (and USP, GPP, etc) is also handled by ortbConverter, as well as site, so I removed those parts. I updated the tests as well since they need to be set up in a slightly different way.
  • Prebid does not expect to find battr in either mediaTypes.video or mediaTypes.banner - it's not in the list of options for those objects. I updated a test case to set it in params.

@tpmn-admin

This comment was marked as resolved.

@tpmn-admin tpmn-admin reopened this Oct 31, 2023
@tpmn-admin
Copy link
Contributor Author

@dgirardi

  1. I followed the guide(30ed60e) and used ortbConverter, removing unnecessary code in the process.
  2. I realized later that I hadn't considered the 'test-all-features-disabled' case when writing the test code, so I made the necessary adjustments.
  3. Now, the tests are running good.

Copy link
Collaborator

@dgirardi dgirardi left a comment

Choose a reason for hiding this comment

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

I forgot to mention the feature disabled tests - sorry! This LGTM pending log cleanup that I mention below if you agree.

}
if (uspConsent && uspConsent.consentString) {
policyParam += `&ccpa_consent=${uspConsent.consentString}`;
utils.logWarn('Building bidResponse');
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if I missed these in my previous review - I don't think they belong here, perhaps this was to help during development? Prebid already logs requests and responses.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added it while creating example.html, and it was reflected as is. I will remove that part.

@tpmn-admin
Copy link
Contributor Author

@patmmccann Please request a code review once again.

@tpmn-admin
Copy link
Contributor Author

@dgirardi
It looks like not progressing. What else should we do for the next steps?
Please let me know if I'm missing anything.

@patmmccann patmmccann merged commit 76e967a into prebid:master Nov 13, 2023
4 checks passed
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.

4 participants