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.

Sign up for GitHub

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

Jump to bottom

Publir Bid Adapter : initial release #10851

Merged
merged 15 commits into from
Mar 12, 2024
Merged

Publir Bid Adapter : initial release #10851

merged 15 commits into from
Mar 12, 2024

Conversation

mdghousesaqlain
Copy link
Contributor

@mdghousesaqlain mdghousesaqlain commented Dec 16, 2023 •
edited
Loading

Type of change

  • Feature

Description of change

How to test

Use Google Chrome
Go to https://www.cravehub.com/test_prebid.html, we are loading Publir Bid Adapter.

{
  bidder: 'publir',
  params: {
    pubId: '13301c0d200f243a475d5c5f'
  }
}

Other information

-> Docs PR

@mdghousesaqlain mdghousesaqlain mentioned this pull request Dec 17, 2023
Merged
7 tasks
@ChrisHuie ChrisHuie changed the title New bidder adapter: Publir Publir Bid Adapter : initial release Dec 19, 2023
@ChrisHuie ChrisHuie requested a review from osazos January 3, 2024 14:35
@mdghousesaqlain
Copy link
Contributor Author

mdghousesaqlain commented Jan 16, 2024 •
edited
Loading

@osazos @ChrisHuie
Hey, Can we have an status update on the approval of our adapter.
We are hoping to get the adapter live end of January.

osazos
osazos reviewed Jan 31, 2024
View reviewed changes
Copy link
Collaborator

@osazos osazos left a comment

Choose a reason for hiding this comment

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

Hi @mdghousesaqlain,

  • I got a CORS error on your endpoint, can you check it?
  • please add more tests to increase coverage rate
  • regarding your documentation:
    • you should mention "video" in addition of "banner" as supported media_types
    • add coppa_supported: true, your code seems to support it

modules/publirBidAdapter.js Outdated
* @returns {object} the common params object
*/
function generatePubGeneralsParams(generalObject, bidderRequest) {
const domain = window.location.hostname;
Copy link
Collaborator

Choose a reason for hiding this comment

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

you should rely on bidderRequest.refererInfo object

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please don't resolve conversation by yourself.

The modification you did is not correct, refererInfo is an object which provide a lot of information. Take a look a this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'm taking all the info but using only bidderRequest.refererInfo.domain on the endpoint, as i need only domain info

modules/publirBidAdapter.js Outdated Show resolved Hide resolved
modules/publirBidAdapter.js Outdated Show resolved Hide resolved
modules/publirBidAdapter.js Outdated Show resolved Hide resolved
modules/publirBidAdapter.js Outdated Show resolved Hide resolved
modules/publirBidAdapter.js Show resolved Hide resolved
osazos
osazos reviewed Feb 1, 2024
View reviewed changes
modules/publirBidAdapter.js Outdated
return;
}
logInfo('onBidWon:', bid);
fetch('//wsfd8lvwt6.execute-api.us-east-1.amazonaws.com/default/publirPrebidImpressionTracker', { method: 'POST', mode: 'no-cors', body: JSON.stringify(bid), credentials: 'include', headers: { 'Content-Type': 'application/json' } });
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use ajax() from import {ajax} from '../src/ajax.js';

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Please review, code modified as suggested.

@mdghousesaqlain
Copy link
Contributor Author

mdghousesaqlain commented Feb 14, 2024 •
edited
Loading

@ChrisHuie @osazos @patmmccann
Can you please review the changes and let us know the feedback, if everything good, can you please approve the PR.

@patmmccann patmmccann removed their request for review February 24, 2024 04:08
@osazos
Copy link
Collaborator

osazos commented Feb 29, 2024

Hi @mdghousesaqlain sounds good.

But there is sill the point about your coverage rate, it is expected to be > 80%.

code under review has at least 80% unit test coverage

Can you improve it? After that, it should be ok.

osazos
osazos reviewed Mar 6, 2024
View reviewed changes
modules/publirBidAdapter.js Outdated Show resolved Hide resolved
@mdghousesaqlain
Copy link
Contributor Author

@osazos
Please let us know, if any modification / suggestion needs to be done.
We are eagerly waiting for the adapter to be approved.

osazos
osazos approved these changes Mar 12, 2024
View reviewed changes
Copy link
Collaborator

@osazos osazos left a comment

Choose a reason for hiding this comment

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

LGTM

@osazos osazos merged commit 404caa4 into prebid:master Mar 12, 2024
4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@patmmccann patmmccann patmmccann left review comments

@osazos osazos osazos approved these changes

@jsnellbaker jsnellbaker Awaiting requested review from jsnellbaker

Assignees

@osazos osazos

Labels
feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@mdghousesaqlain @osazos @patmmccann @jsnellbaker @ChrisHuie