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

Adagio Bid Adapter|Analytics Adapter|RTD Module: enforce enable tids #11938

Closed

Conversation

Abyfall
Copy link
Contributor

@Abyfall Abyfall commented Jul 8, 2024

Type of change

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

Description of change

To ensure compatibility with our backend while staying compliant with the behavior introduced in #9781 we for some time generated our own specific auction id.

We will now rely on source.tid, thus requiring publishers to set enableTIDs to true.
The analyticsAdapter won't start if this option is not set to true

Other information

Reflecting changes in documentation prebid/prebid.github.io#5485

@Abyfall Abyfall force-pushed the adagio-feature/enforce-enable-tids branch from 07d3d71 to fb8d3ed Compare July 8, 2024 10:32
Copy link

github-actions bot commented Jul 8, 2024

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. Keep up the great work! 🚀

Copy link

github-actions bot commented Jul 8, 2024

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

  • modules/adagioBidAdapter.js (+1 error)
  • test/spec/modules/adagioRtdProvider_spec.js (+1 error)

@patmmccann
Copy link
Collaborator

You're not able to require publishers to set this, it's up to them

@Abyfall
Copy link
Contributor Author

Abyfall commented Jul 8, 2024

You're not able to require publishers to set this, it's up to them

Hi @patmmccann thanks for the feedback, this requirement is for analytics only. If enableTIDs = false it is not possible to join a "bid" with an analytics beacon because the bid won't have the auction_id, knowing that we prefer to "stop" the analytics module and avoid to send useless requests through the network. Is something wrong with that?

Copy link

github-actions bot commented Jul 8, 2024

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

If enableTIDs = false it is not possible to join a "bid" with an analytics beacon because the bid won't have the auction_id,

Fairly sure bidderRequestId is in both objects, you guys are just not geting it in your adapter

const aucId = deepAccess(bidderRequest, 'ortb2.site.ext.data.adg_rtd.uid') || generateUUID()

here is how you should get it to give yourself a join key

id: bidderRequest.bidderRequestId,

@patmmccann
Copy link
Collaborator

Please also continue to make duplication progress, for example this can be imported from adagioUtils

function _getGppConsent(bidderRequest) {

@Abyfall
Copy link
Contributor Author

Abyfall commented Jul 10, 2024

Hello @patmmccann following discussion on Slack I'm closing this PR. I will work no a new one and propose it. Thanks for your hindsights.

@Abyfall Abyfall closed this Jul 10, 2024
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.

None yet

2 participants