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

Adlive bid adapter #3109

Merged
merged 11 commits into from
Oct 19, 2018
Merged

Adlive bid adapter #3109

merged 11 commits into from
Oct 19, 2018

Conversation

mifanich
Copy link
Contributor

@mifanich mifanich commented Sep 20, 2018

Type of change

  • Bugfix
  • Feature
  • New 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

  • test parameters for validating bids
{
  bidder: 'adlive',
  params: {
    hashes: ['1e100887dd614b0909bf6c49ba7f69fdd1360437']
  }
}

Be sure to test the integration with your adserver using the Hello World sample page.

  • contact email of the adapter’s maintainer
  • official adapter submission

For any changes that affect user-facing APIs or example code documented on http://prebid.org, please provide:

Other information

@jsnellbaker jsnellbaker requested review from jsnellbaker and removed request for mike-chowla October 4, 2018 13:07
Copy link
Collaborator

@jsnellbaker jsnellbaker left a comment

Choose a reason for hiding this comment

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

@mifanich Thanks for submitting your adapter.

I took a look over the code and it mostly seems good. There is one area that can be reviewed (see in-line comment below).

However when I tried to test your adapter with the test params on the hello_world page, I wasn't getting anything back from your server.

In the browser, I was seeing two calls going out to your server. One had a Request Method of OPTIONS that just returned a 200 OK response. The other (which seemed to be the POST but wasn't classified as such in the headers) request just canceled.

Can you take a look at this behavior and see why it may not be working? We'd like to see a test ad working with new adapters to ensure the auction workflow is correct.

}
},

getUserSyncs: function (syncOptions, serverResponses, gdprConsent) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

If you don't have any userSync pixels you want to fire, you can just remove the entire getUserSyncs property from your adapter. It's not required to define it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I do not define this function, tests do not pass.

Copy link
Collaborator

@jsnellbaker jsnellbaker Oct 4, 2018

Choose a reason for hiding this comment

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

Can you clarify what tests do not pass? I disabled that part of your adapter code in the branch copy I checked out and rangulp test. There were no unit test errors from what I saw.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jsnellbaker
I mean circleci tests.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@mifanich I ran the browserstack tests (which is what circleci runs for us automatically) locally with the getUserSyncs code disabled and I didn't see any test failures.

It's possible there was some other issue that happened in the circleci tests you saw that failed that were unrelated to your adapter's code. Can you try to make the change again and push it in this PR so we can retest it?

@jsnellbaker
Copy link
Collaborator

@mifanich Looks like the circleci test without the userSync code ran fine this time. Please let me know about the delivery/response issue when you get the chance. Thanks!

@jsnellbaker
Copy link
Collaborator

@mifanich I tried to test the adapter again with the params you updated in the last commit. However I'm still seeing an error when the request is going out.

OPTIONS https://api.publishers.adlive.io/get?pbjs=1 net::ERR_CONNECTION_REFUSED

I have attached screenshots of the request as seen in the Console and Network tabs. Can you take a look?

adlive_request_console_tab

adlive_request_network_tab

@mifanich
Copy link
Contributor Author

mifanich commented Oct 9, 2018

I have changed test params and improve some logic.
gulp test works fine but circleci tests do not passed.

@jsnellbaker
Copy link
Collaborator

@mifanich Thanks for the update, I'll check again. The circleci failed for an unrelated reason; I restarted the build and it completed fine.

@jsnellbaker
Copy link
Collaborator

@mifanich I retested the adapter and I was seeing a response coming back from the server. However the bid wasn't being accepted as per the logic in the interpretResponse function.

The response contained a 1 in the bidResponse.is_passback, which caused the if statement check at https://github.com/prebid/Prebid.js/pull/3109/files#diff-841514cf584a84bb2bb332698c4b54feR46 to fail to pass. Is this expected?

Below is a copy of the response object I saw:

[{"hash":"1e100887dd614b0909bf6c49ba7f69fdd1360437","content":"<script async src=\"//pagead2.googlesyndication.com/pagead/js/adsbygoogle.js\"></script> <!-- lacuisinedebernard_300x250 --> <ins class=\"adsbygoogle\"      style=\"display:inline-block;width:300px;height:250px\"      data-ad-client=\"ca-pub-7357742692072135\"      data-ad-slot=\"9217654942\"></ins> <script> (adsbygoogle = window.adsbygoogle || []).push({}); </script>","size":[300,250],"is_passback":1,"bids":[{"adapter":"pulsepoint","nobid":true},{"adapter":"districtm","nobid":true},{"adapter":"appnexus","nobid":true}]}]

@mifanich
Copy link
Contributor Author

@mifanich I retested the adapter and I was seeing a response coming back from the server. However the bid wasn't being accepted as per the logic in the interpretResponse function.

The response contained a 1 in the bidResponse.is_passback, which caused the if statement check at https://github.com/prebid/Prebid.js/pull/3109/files#diff-841514cf584a84bb2bb332698c4b54feR46 to fail to pass. Is this expected?

Below is a copy of the response object I saw:

[{"hash":"1e100887dd614b0909bf6c49ba7f69fdd1360437","content":"<script async src=\"//pagead2.googlesyndication.com/pagead/js/adsbygoogle.js\"></script> <!-- lacuisinedebernard_300x250 --> <ins class=\"adsbygoogle\"      style=\"display:inline-block;width:300px;height:250px\"      data-ad-client=\"ca-pub-7357742692072135\"      data-ad-slot=\"9217654942\"></ins> <script> (adsbygoogle = window.adsbygoogle || []).push({}); </script>","size":[300,250],"is_passback":1,"bids":[{"adapter":"pulsepoint","nobid":true},{"adapter":"districtm","nobid":true},{"adapter":"appnexus","nobid":true}]}]

Yes, you are right! If response contains is_passback=1 it means there is no bids. Need to reload page several times because this is production endpoint. Bids may not present.

@mifanich
Copy link
Contributor Author

@jsnellbaker please check updates

@jsnellbaker
Copy link
Collaborator

@mifanich Is there some way you can setup a dedicated test? I've been trying to get something to return off/on for a while now and I haven't seen it once return an actual bid.

@mifanich
Copy link
Contributor Author

@jsnellbaker yes, I will do it for 1-2 days

@jsnellbaker
Copy link
Collaborator

@mifanich Thanks, let me know when it's ready and if I need to use any different params.

@mifanich
Copy link
Contributor Author

@jsnellbaker Fixed.
Please use https://api.publishers.adlive.io/get?pbjs=1&pb_test=1 as POST URL

@jsnellbaker
Copy link
Collaborator

That test param did work and I saw the test ad; LGTM

@jsnellbaker jsnellbaker merged commit 6879fea into prebid:master Oct 19, 2018
@mifanich mifanich deleted the adliveBidAdapter branch October 26, 2018 08:28
idettman pushed a commit to rubicon-project/Prebid.js that referenced this pull request Nov 14, 2018
* send request to endpoint

* tests

* improve tests

* Add description. Small fix

* add maintainer email

* restore origin package.json

* add getUserSyncs function

* remove userSyncs

* change endpoint_url params

* change test params

* change netRevenue to false
ghost pushed a commit to devunrulymedia/Prebid.js that referenced this pull request Jan 30, 2019
* send request to endpoint

* tests

* improve tests

* Add description. Small fix

* add maintainer email

* restore origin package.json

* add getUserSyncs function

* remove userSyncs

* change endpoint_url params

* change test params

* change netRevenue to false
pedrolopezmrf pushed a commit to Marfeel/Prebid.js that referenced this pull request Mar 18, 2019
* send request to endpoint

* tests

* improve tests

* Add description. Small fix

* add maintainer email

* restore origin package.json

* add getUserSyncs function

* remove userSyncs

* change endpoint_url params

* change test params

* change netRevenue to false
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