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

Illumin Bid Adapter: initial release #10375

Merged
merged 6 commits into from
Sep 27, 2023

Conversation

saar120
Copy link
Contributor

@saar120 saar120 commented Aug 21, 2023

Type of change

  • New bidder adapter

Description of change

Illumin Bid Adapter
maintainer email: [email protected]

{
        bidder: ‘illumin’,
        params: {
          cId: ‘562524b21b1c1f08117fc7f9’,
          pId: ‘59ac17c192832d0011283fe3’,
          bidFloor: 0.0001,
          ext: {
            param1: ‘loremipsum’,
            param2: ‘dolorsitamet’
          }
     }
 }

Other information

prebid/prebid.github.io#4813

Sorry, something went wrong.

saar120 added 4 commits August 1, 2023 16:21

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
@ChrisHuie ChrisHuie changed the title IlluminBidAdapter: adapter submission Illumin Bid Adapter: adapter submission Aug 23, 2023
@ChrisHuie ChrisHuie requested a review from mmoschovas August 23, 2023 13:07
@saar120
Copy link
Contributor Author

saar120 commented Aug 29, 2023

@mmoschovas @ChrisHuie
Is there anything we can do to merge this one?

}

export function extractCID(params) {
return params.cId || params.CID || params.cID || params.CId || params.cid || params.ciD || params.Cid || params.CiD;
Copy link
Contributor

Choose a reason for hiding this comment

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

Question regarding these three extract methods. Is there a reason we are checking multiple combinations when the params are spec'd out to be cId, pId and subDomain? This seems unnecessary to me and throwing a warning during validation seems appropriate here if the property name is not correct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mmoschovas We are checking this for backward compatibility and minor typos of publishers.

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe I am missing something. Is this not a new adapter? Im not sure where backwards compatibility is coming into play. I also understand the intent, I just do not understand why you are doing it considering it seems these are random alternative values and not comprehensive. Either way, I believe you should allow publishers who do not follow the specification to receive the error and not attempt to handle unknowns

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mmoschovas
I've changed the extract method to support only the given properties.
Can we merge this?

@ChrisHuie ChrisHuie changed the title Illumin Bid Adapter: adapter submission Illumin Bid Adapter: initial release Sep 21, 2023
@mmoschovas mmoschovas merged commit 94a3e5b into prebid:master Sep 27, 2023
2 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.

3 participants