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

videoHeroesAdapter: support eids #11974

Closed
wants to merge 23 commits into from

Conversation

videoheroes
Copy link

Type of change

  • Bugfix

  • Feature

  • New bidder adapter

  • Updated 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

Added support of eids array to provide our exchange servers with user data from user submodules

Copy link

Tread carefully! This PR adds 1 linter error (possibly disabled through directives):

  • modules/videoheroesBidAdapter.js (+1 error)

Copy link

Whoa there, partner! 🌵🤠 We wrangled some duplicated code in your PR:

Reducing code duplication by importing common functions from a library not only makes our code cleaner but also easier to maintain. Please move the common code from both files into a library and import it in each. We hate that we have to mention this, however, commits designed to hide from this utility by renaming variables or reordering an object are poor conduct. We will not look upon them kindly! Keep up the great work! 🚀

@patmmccann
Copy link
Collaborator

patmmccann commented Jul 15, 2024

Hi, it appears your adapter and the brave adapter are duplicates submitted by the same person. https://github.com/prebid/Prebid.js/commits?author=thebraveio Please solve before working in either one further, as with the addition of libraries to the project, we're now trying to eliminate large blocks of duplicated code

@videoheroes
Copy link
Author

Hi guys ! Ok we will change code to avoid duplicates, its different companies, but started by same team and code

@patmmccann
Copy link
Collaborator

Great! Can we lend a hand in any way. One example is #11951 another is #11877

Copy link

Tread carefully! This PR adds 53 linter errors (possibly disabled through directives):

  • modules/videoheroesBidAdapter.js (+53 errors)

Copy link

Tread carefully! This PR adds 44 linter errors (possibly disabled through directives):

  • modules/videoheroesBidAdapter.js (+44 errors)

Copy link

Tread carefully! This PR adds 43 linter errors (possibly disabled through directives):

  • modules/videoheroesBidAdapter.js (+43 errors)

Copy link

Tread carefully! This PR adds 12 linter errors (possibly disabled through directives):

  • modules/videoheroesBidAdapter.js (+12 errors)

Copy link

Tread carefully! This PR adds 1 linter error (possibly disabled through directives):

  • modules/videoheroesBidAdapter.js (+1 error)


return { id: br.transactionId, w: size[0], h: size[1] };
};
return deviceObj;
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not just take the device object core has prepared for you on bidderRequest.ortb2.device? eg https://github.com/prebid/Prebid.js/pull/12053/files

Copy link
Author

Choose a reason for hiding this comment

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

never used it before on any prebid connectors, interesting, so if it can replace all device regular data, lets update it

Copy link

Tread carefully! This PR adds 1 linter error (possibly disabled through directives):

  • modules/videoheroesBidAdapter.js (+1 error)

@videoheroes
Copy link
Author

videoheroes commented Jul 31, 2024

seems like bidderRequest.ortb2 not stable and guarantee to be available, but i do not include old device prepare for else cases - bidderRequest.ortb2?.device || {}

@videoheroes
Copy link
Author

videoheroes commented Aug 1, 2024

I do not why, but there on test browser causing random errors after building

Copy link

github-actions bot commented Aug 1, 2024

Tread carefully! This PR adds 1 linter error (possibly disabled through directives):

  • modules/videoheroesBidAdapter.js (+1 error)

Copy link

github-actions bot commented Aug 1, 2024

Tread carefully! This PR adds 1 linter error (possibly disabled through directives):

  • modules/videoheroesBidAdapter.js (+1 error)

@videoheroes
Copy link
Author

Still different strange errors after added ortb2

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.

Please remove all your commits designed to hide your code duplication from the robot

@videoheroes
Copy link
Author

Please remove all your commits designed to hide your code duplication from the robot

@videoheroes videoheroes closed this Aug 5, 2024
@videoheroes videoheroes reopened this Aug 5, 2024
@videoheroes
Copy link
Author

videoheroes commented Aug 5, 2024

I will develop Native req / res convertor on another way, as you do not like when am using object to convert it and found there duplicate way (but i found a lot of adapters with same ways also with absolutely same object like I did ), which is still the best variant I guess

@videoheroes
Copy link
Author

videoheroes commented Aug 5, 2024

Has been updated as you asked for, now native full from scratched - without using convert object and without used old one parts, can not say I like this change, but its another way to explain logic compatible with our exchange systems and old clients
Could you please check it again ?

root added 2 commits August 5, 2024 16:46
…rebuild as it tests errors not depend on code)
…rebuild as trere test errors not depend on code)
@videoheroes
Copy link
Author

About commits with changing line - I made it to rebuild projects, as there all time errors not about adapter, more situate like some adapters servers results, etc

  1. calls addPaapiConfig when there is no bid in the response
    S2S Adapter response handler when the response contains ext.prebid.fledge
    AssertError: expected stub to be called with arguments
    at Object.fail (node_modules/sinon/pkg/sinon.js:142:21)
    at failAssertion (node_modules/sinon/pkg/sinon.js:101:16)
    at assert. [as calledWith] (node_modules/sinon/pkg/sinon.js:126:13)
    at expectFledgeCalls (/tmp/_karma_webpack_716925/commons.js:395628:22)
    at Context. (/tmp/_karma_webpack_716925/commons.js:395659:9)

  2. Logger: post-timeout check without bid response
    pubmatic analytics adapter when handling events
    AssertionError: expected 2 to equal 1
    at Context. (/tmp/_karma_webpack_716925/commons.js:402825:34)

  3. should handle case when callback return falsy value
    weboramaRtdProvider Handle Set Targeting and Bid Request Add site-centric data (contextual)
    AssertionError: expected 3 to equal +0
    at Context. (/tmp/_karma_webpack_716925/commons.js:488326:48)

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.

2 participants