-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
modules: Implement SmartRTB adapter and spec. #3575
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for submitting this new adapter.
There was one point of feedback on the code, please take a look when you have the chance.
While trying to test the adapter with the params in the description, I wasn't getting any object response from the server; the call to http://pubs.smrtb.com/json/publisher/prebid
completed with a 204 OK.
Below is a copy of the request object that was executed, can you take a look to see if anything is amiss?
{"start_time":1551126396115,"tmax":120,"language":"en-US","site":{"domain":"ap.localhost","iframe":false,"url":null,"https":false,"referrer":"http://ap.localhost:9999/integrationExamples/gpt/hello_world.html?pbjs_debug=true"},"imps":[{"pub_id":123,"med_id":"m_123","zone_id":"z_abc","bid_id":"2ae840036721c5","imp_id":"75fd1f36-d614-4105-bb11-1ebf93de10ec","sizes":[[300,250],[300,600]]}]}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Found an additional item that needs to be updated.
@jsnellbaker Fixed the for-loop syntax for IE and internalized getDomain(). Please verify. |
@evanmsmrtb Thanks for making the code updates; those look fine now. Did you have the chance to look at the issue with test bid not returning? I'm seeing that problem when using the test params on the |
Yes, this has now been corrected. You should see sample bids returned with these parameters:
|
@jsnellbaker Please review updated params and for-loop syntax. |
@evanmsmrtb I apologize for the delay; the code changes do look fine. But I tested with the parameters in the previous comment, but I'm still not getting a response from the server. The status of the request is
Do you see anything incorrect or unexpected in the above? |
@jsnellbaker No problem. It appears you caught us while we were working on parts of our API. Please try again - I have confirmed it is up again and returning bids. |
@evanmsmrtb I tried again but it's not connecting. I setup a test variant where I disabled the internal timeouts for the auction and on our ajax request to allow the server ample time to respond. However it just resulted in a I attached a screenshot of the request details/headers/payload from this test. Could the issue with the alias I have setup to run the local server? Are you doing something on the server when any variant of |
@jsnellbaker We located and fixed what was causing this. Please try again - third time's the charm. |
@evanmsmrtb Thanks for digging into this. I tried again and verified the ad is delivering successfully. LGTM |
* modules: Implement SmartRTB adapter and spec. * Fix for-loop syntax to support IE; refactor getDomain out of exported set.
* modules: Implement SmartRTB adapter and spec. * Fix for-loop syntax to support IE; refactor getDomain out of exported set.
Type of change
Description of change
Addition of SmartRTB / smrtb.com prebid module.