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

RixEngine Bid Adapter: add AlgoriX alias #12654

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

Conversation

xiaochang
Copy link
Contributor

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

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! 🚀

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! 🚀

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! 🚀

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

Why are you committing two adapters that are the same?

@patmmccann patmmccann self-assigned this Jan 14, 2025
@xiaochang
Copy link
Contributor Author

Why are you committing two adapters that are the same?

Hi @patmmccann ,

Thank you for taking the time to review the code!

Regarding your question about the two adapters, they are indeed similar but belong to two different companies. There is no connection between them, and their implementations serve distinct purposes within their respective contexts.

Best regards,
Cindy

@patmmccann
Copy link
Collaborator

And you work at both of them and they both have the same name?

@xiaochang
Copy link
Contributor Author

And you work at both of them and they both have the same name?

To clarify, the adapters do not have the same name; they are completely independent and belong to two different companies. There is no connection between them.

@patmmccann
Copy link
Collaborator

patmmccann commented Jan 16, 2025

At a minimum you are the connection between them. Why are you being less than forthcoming?

@xiaochang
Copy link
Contributor Author

At a minimum you are the connection between them. Why are you being less than forthcoming?

I want to clarify that the two projects you mentioned are a year apart, and I am not the connection between them. My role is specifically to develop the prebid.js adapter for our client. If other company clients have similar needs in the future, I may submit adapters for those companies as well.
Furthermore, the Prebid organization does not require developers to act as a connector between different companies. I believe the request you've made exceeds the scope of my work responsibilities, and I must decline to take on this task.

@patmmccann
Copy link
Collaborator

Where do you work?

@Gabriel-Algorix
Copy link

Hi,

I'm Gabriel, Director of Ad Solution Engineering from AlgoriX. There seems to be some misunderstanding here and allow me to jump in with some background.

RixEngine is the SaaS service platform derived from AlgoriX business, while the two entities are independent from each other. Cindy is the shared resource on our development team, and she’s the one who developed the Prebid.js solution for RixEngine.

As an In-app focused ad platform, we didn’t have our dedicated JS developer. And as we are expanding our mobile web business this year, AlgoriX will need our own adaptor; thus we asked Cindy to submit another adaptor on AlgoriX’s behalf.

We have clarified the above with Christian and we are told to communicate here the same. Please do not hesitate to let me know if any further details are needed for our adaptor to be approved.

@patmmccann
Copy link
Collaborator

patmmccann commented Jan 27, 2025

hi @Gabriel-Algorix thanks! Why does the new business need its own adapter. This adapter is basically a perfect copy of the other adapter. One could simply add an alias to the other adapter and issue independent documentation, the isvalid check could see if the correct endpoint is used when there is an alias.

@patmmccann
Copy link
Collaborator

I took the liberty of modifying your pr to implement my suggestion. You'll still need to write tests.

let TOKEN = null;

// We will also allow an alias named 'algorix'
const ALGORIX_REGIONS = ['apac', 'use', 'euc'];
Copy link
Collaborator

Choose a reason for hiding this comment

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

I didn't implement this, but you could detect the region like this

const timezone = Intl.DateTimeFormat().resolvedOptions().timeZone;

@Gabriel-Algorix
Copy link

Hi @patmmccann , thanks for your reply.

And as of why a separate adaptor is needed - for one, the clients of AlgoriX and RixEngine are connected to the respective entities under different contracts. Another main reason is that if a client is to connect to both RixEngine and AlgoriX, it might need two adaptors to coexist. I hope these make sense.

@patmmccann patmmccann changed the title Add new AlgoriX Bid Adapter RixEngine Bid Adapter: add AlgoriX alias Jan 30, 2025
@patmmccann
Copy link
Collaborator

@Gabriel-Algorix the alias approach solves both of these use cases

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