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

topRTBBidAdapter #3817

Merged
merged 16 commits into from
Aug 6, 2019
Merged

topRTBBidAdapter #3817

merged 16 commits into from
Aug 6, 2019

Conversation

Unnamalai57
Copy link
Contributor

New Bidder Adapter - topRTBBidAdapter

                bidder: 'topRTB',
                params: {
                    adUnitId: cd95dffec6b645afbc4e5aa9f68f2ff3
                }

Contact email of the adapter’s maintainer
[email protected]

@robertrmartinez
Copy link
Collaborator

@Unnamalai57 Hi! I am taking a look now.

Please update your test which is failing in Circle CI?

@robertrmartinez
Copy link
Collaborator

@Unnamalai57 Hello, just a reminder to please fix the failing tests.

Will re-review once complete.

@robertrmartinez
Copy link
Collaborator

If you would like this merged into the next Prebid Release, please make the changes to fix your failing tests.

@Unnamalai57
Copy link
Contributor Author

If you would like this merged into the next Prebid Release, please make the changes to fix your failing tests.

@robertrmartinez may I know what are the tests have been failed in topRTBBidAdapter?

@robertrmartinez
Copy link
Collaborator

You can see it says "Some checks were not successful" and you can click details which will take you to this link:
https://circleci.com/gh/prebid/Prebid.js/2537?utm_campaign=vcs-integration-link&utm_medium=referral&utm_source=github-build-link

image

@robertrmartinez
Copy link
Collaborator

image

image

@robertrmartinez
Copy link
Collaborator

I attempted to rerun them again, and they still fail
https://circleci.com/gh/prebid/Prebid.js/2568

sorry. Please try and fix when possible.

@Unnamalai57
Copy link
Contributor Author

I attempted to rerun them again, and they still fail
https://circleci.com/gh/prebid/Prebid.js/2568

sorry. Please try and fix when possible.

ok sure @robertrmartinez .Let me fix it

@Unnamalai57
Copy link
Contributor Author

@robertrmartinez I have fixed the test failing issue. Could you please verify it?

Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

Please see comments and respond with questions or fixes

@@ -0,0 +1,70 @@
import * as utils from '../src/utils';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Your test coverage is just barely at 80%

Would love to see those numbers jump up more soon. Will not reject PR for this, but please try to increase coverage on future PR's!

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@robertrmartinez could you please tell me how can I enhance the testing to achieve 100%

import {BANNER, VIDEO} from '../src/mediaTypes';

const BIDDER_CODE = 'topRTB';
const ENDPOINT_URL = 'http://ssp.toprtb.com:8080/sspNew/rest/ReqAd?ref=www.google.com&hbid=0&adUnitId=';
Copy link
Collaborator

Choose a reason for hiding this comment

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

All adapters must support HTTPS

Plus I am not sure having port numbers in the endpoint is something we allow either.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please address this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi you still do not support HTTPS which is a requirement for prebid.js adapters.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @robertrmartinez
SSL configuration is in progress. Will update as soon as completed

modules/topRTBBidAdapter.js Outdated Show resolved Hide resolved
modules/topRTBBidAdapter.js Outdated Show resolved Hide resolved
modules/topRTBBidAdapter.js Outdated Show resolved Hide resolved
modules/topRTBBidAdapter.md Outdated Show resolved Hide resolved
modules/topRTBBidAdapter.js Show resolved Hide resolved
@Unnamalai57
Copy link
Contributor Author

@robertrmartinez I have just removed console log from topRTBAdapter.js. May I know in what case testing failed in CircleCI

import {BANNER, VIDEO} from '../src/mediaTypes';

const BIDDER_CODE = 'topRTB';
const ENDPOINT_URL = 'http://ssp.toprtb.com:8080/sspNew/rest/ReqAd?ref=www.google.com&hbid=0&adUnitId=';
Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi you still do not support HTTPS which is a requirement for prebid.js adapters.

@stale
Copy link

stale bot commented Jul 3, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 3, 2019
@Unnamalai57
Copy link
Contributor Author

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

Still checking with SSL configurations. Will complete it

@stale stale bot removed the stale label Jul 6, 2019
@stale
Copy link

stale bot commented Jul 20, 2019

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Jul 20, 2019
@bretg bretg removed the stale label Jul 22, 2019
@bretg
Copy link
Collaborator

bretg commented Jul 22, 2019

@Unnamalai57 - any progress on this?

@Unnamalai57
Copy link
Contributor Author

@Unnamalai57 - any progress on this?

Hi @bretg
code has been committed with the updation of SSL

@bretg
Copy link
Collaborator

bretg commented Jul 28, 2019

Great - back to @robertrmartinez

@robertrmartinez
Copy link
Collaborator

Thank you for the updates @Unnamalai57

I am still unable to get a valid bid response using the example you provided.

image

A bidders params should be reliable and always return a valid bid response.

Is there something in particular that I need to do to have your endpoint return a test bid?

@Unnamalai57
Copy link
Contributor Author

Thank you for the updates @Unnamalai57

I am still unable to get a valid bid response using the example you provided.

image

A bidders params should be reliable and always return a valid bid response.

Is there something in particular that I need to do to have your endpoint return a test bid?

Hi @robertrmartinez
I will give you the adunitId which gives the valid bid Response. Because the given adunit doesn't contain ad.

Copy link
Collaborator

@robertrmartinez robertrmartinez left a comment

Choose a reason for hiding this comment

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

Thank you, glad to finally get this in! :)

@robertrmartinez robertrmartinez merged commit 0183f23 into prebid:master Aug 6, 2019
sa1omon pushed a commit to gamoshi/Prebid.js that referenced this pull request Nov 28, 2019
* toprtbBidAdapter

* topRTB adapter hbid queryparam added

* toprtbBidAdapter-ssp aws url

* topRTBBidAdapter

* Video format topRTBBidAdapter

* Video format topRTBBidAdapter

* deviceType added

* tracking new added

* impression event added

* fix test failed

* fix testing failed

* removed debug logs

* added single quotes in adUnitId

* remove console log

* code committed to add SSL

* adunitId changes for valid response
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants