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

Brainx adapter: initial commit #12413

Open
wants to merge 8 commits into
base: master
Choose a base branch
from
Open

Conversation

Hugh0222
Copy link

@Hugh0222 Hugh0222 commented Nov 5, 2024

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

Other information

@patmmccann patmmccann changed the title First add adpater Brainx adapter: initial commit Nov 5, 2024
data.user = {
buyeruid: generateUUID()
}
data.device.ip = navigator.ip || '202.100.48.46';
Copy link
Collaborator

Choose a reason for hiding this comment

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

what is this fallback lol :)

// console.log('bidRequests-==========', bidRequests);
// console.log('bidderRequest-==========', bidderRequest);
data.user = {
buyeruid: generateUUID()
Copy link
Collaborator

Choose a reason for hiding this comment

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

this is odd, it will change every request

@Hugh0222
Copy link
Author

Hugh0222 commented Nov 6, 2024 via email

}
return {
method: METHOD,
url: `${String(deepAccess(bidRequests[0], 'params.endpoint'))}?token=${String(deepAccess(bidRequests[0], 'params.pubId'))}`,
Copy link
Collaborator

Choose a reason for hiding this comment

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

endpoint cannot be a required parameter. It is incredibly awkward for publishers to have to enter a hostname on thousands of adunits when this is a global config.

It may be an optional parameter, but a default is required.

You may also ask the publisher to

pbjs.setConfig({
   "brainx": {
      "endpoint": "URL"
   }
});

At least this is not then replicated thousands of times for each adunit.

Copy link
Author

Choose a reason for hiding this comment

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

OK, thank you for your reply. We will continue to make adjustments

Copy link
Author

Choose a reason for hiding this comment

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

Hello, bretg, we have made adjustments to the above issues. Please review it again.

Copy link
Collaborator

@bretg bretg left a comment

Choose a reason for hiding this comment

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

my review only covers that endpoint is no longer a required param

@bretg
Copy link
Collaborator

bretg commented Nov 13, 2024

docs PR prebid/prebid.github.io#5689

@Hugh0222
Copy link
Author

我的评论仅涵盖端点不再是必需参数

Hi bretg, I was wondering when the next merge code will be released?

@bretg
Copy link
Collaborator

bretg commented Nov 25, 2024

@ChrisHuie - please assign this to someone for proper review.

@Hugh0222 - sorry, we do not have a committed service-level agreement on how long it takes a pull request to get through review. It depends on the complexity and quality of the code as well as the availability of qualified reviewers.

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.

3 participants