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

AdSpirit Bid Adapter #2419

Merged
merged 15 commits into from
Jun 4, 2018
Merged

AdSpirit Bid Adapter #2419

merged 15 commits into from
Jun 4, 2018

Conversation

SpreeGorilla
Copy link
Contributor

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
  • Other

Description of change

Adding AdSpirit bid adapter and aliases

  • test parameters for validating bids
{
  bidder: 'adspirit',
  params: {
    placementId: [the placementId],
	 host: host url provided for the publisher by AdSpirit
  }
}
{
  bidder: 'connectad',
  params: {
    placementId: [the placementId],
  }
}
{
  bidder: 'xapadsmedia',
  params: {
    placementId: [the placementId],
  }
}

Other information

docs at prebid/prebid.github.io#396

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.

@SpreeGorilla Thanks for submitting this adapter. There are a few items that need to be addressed:

  • Please remove the hello_world.html, pbjs_ucfunnel_gpt.html and release.md files from this PR. The only files that should be included in a new adapter submission are your bid adapter.js file, the corresponding .md file and your bid adapter_spec.js unit test file.
  • The test param data is not working, what's the ideal value that should used for the host param?
  • I can't confirm this yet, but I expect that in the interpretResponse function - you'll need to access the response information by looking in the serverResponse.body field, not the parent object.

Can you please review and make the necessary updates? Let me know if you have any questions.

@jsnellbaker
Copy link
Collaborator

Hi @SpreeGorilla have you had the chance to review the feedback? Please let me know if you have any questions. Thanks!

@SpreeGorilla
Copy link
Contributor Author

Hi @jsnellbaker,
thanks for your review.
I'm a bit confused about your first point. Should I remove only the 3 mentioned files OR everything but my contributed files (In this case Travis would fail, I guess?)?
For tests you could use "help.adspirit.de" as host parameter.
You seem right about the interpretResponse function, I'll update that with the next commit.

@jsnellbaker
Copy link
Collaborator

Hi @SpreeGorilla,

To clarify - please remove the 3 noted files (hello_world.html, pbjs_ucfunnel_gpt.html and release.md files) from the PR.

If it's easier - we could also just close this PR, you could then reset the files that don't need to be changed, and open a new PR with just the intended files for your adapter. Please let me know what you think. Thanks.

@SpreeGorilla
Copy link
Contributor Author

Ok, let's stay with this PR. I made the requested changes...

@jsnellbaker
Copy link
Collaborator

Hi @SpreeGorilla thanks for making the attempt. But with the latest commit, I think this would actually delete the hello_world.html and pbjs_ucfunnel_gpt.html files from the repo if we were to merge it.

Given the circumstances, can you obtain a copy of the current master versions of these files and upload them into this PR? It's not ideal, but I think it's the easiest option at this point to move forward with the merge. Thanks and sorry for the back/forth on these files.

@jsnellbaker
Copy link
Collaborator

@SpreeGorilla Thanks for making the changes in your unit test to resolve the Travis errors.

One last thing for this PR - could you update the test information in the .md file to include the host value that works? You suggested to use help.adspirit.de before, so if this is a good value to use going forward for testing - we should add that to the page.

Thanks.

@jsnellbaker
Copy link
Collaborator

@SpreeGorilla I was retesting the adapter after all the changes, but I'm currently getting a 204 no content response from the server when the request goes out. Below is a copy of the request URL as it gets executed:

http://help.adspirit.de/rtb/getbid.php?rtbprovider=prebid&pid=123&ref=http%3A%2F%2Ftest.mysite.com%3A9999%2FintegrationExamples%2Fgpt%2Fhello_world.html%3Fpbjs_debug%3Dtrue&scx=1680&scy=1050&wcx=1680&wcy=358&async=adspirit28534&t=4464?

Could you take a look?

@stale
Copy link

stale bot commented May 23, 2018

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 May 23, 2018
@stale stale bot removed the stale label May 24, 2018
@jsnellbaker
Copy link
Collaborator

Hello @SpreeGorilla Have you had the chance to look into my previous comments? I'm still seeing a No Content 204 response from your server when trying to test the delivery.

Additionally, could you update the host param in your .md file to a value that's usable for testing? This information would help down the road when we'd need to verify any changes for your adapter.

@SpreeGorilla
Copy link
Contributor Author

Ok, I finally managed to tinker something. You can use host = "n1test.adspirit.de" and placementId = 5 for testing (see .md file).

@jsnellbaker
Copy link
Collaborator

@SpreeGorilla Unfortunately, I'm still seeing the 204 No Content response from the server when I'm trying to test the adapter. Even with the new params, it's still the same.

Below is a copy of the request URL:
http://n1test.adspirit.de/rtb/getbid.php?rtbprovider=prebid&pid=5&ref=http%3A%2F%2Fap.localhost%3A9999%2FintegrationExamples%2Fgpt%2Fhello_world.html%3Fpbjs_debug%3Dtrue&scx=1680&scy=1050&wcx=1680&wcy=358&async=adspirit74753&t=12898?

Is there anything malformed in the URL that's generated? Is there anything missing?

@SpreeGorilla
Copy link
Contributor Author

Oh sorry, I didn't consider the async parameter. Now it's working.

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.

Hi @SpreeGorilla thanks for making the change. I am now successfully seeing a response from the server.

However I'm seeing some issues from that bid response that should be reviewed. Please see below.

let host = spec.getBidderHost(bidObj);
let adm = '<scr' + 'ipt>window.inDapIF=false</scr' + 'ipt><scr' + 'ipt src="//' + host + SCRIPT_URL + '"></scr' + 'ipt>' + '<ins id="' + bidObj.adspiritConId + '"></ins>' + adData.adm;
const bidResponse = {
requestId: bidRequest.bidId,
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is returning an undefined value which is preventing the bid from being accepted later in the code.

It looks like it should be bidObj.bidId instead of bidRequest.bidId.

cpm: cpm,
width: adData.w,
height: adData.h,
creativeId: adData.placement_id,
Copy link
Collaborator

@jsnellbaker jsnellbaker May 30, 2018

Choose a reason for hiding this comment

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

Not sure if this is just an issue with the test placement, but this is also currently resulting in an undefined. It seems like the placement_id field isn't in the server response body, is that expected?

The response body I currently see from testing only has the following fields:

  • cpm
  • adm
  • w
  • h

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks for the hints! Fixed.

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.

LGTM

@jsnellbaker
Copy link
Collaborator

New docs PR is prebid/prebid.github.io#421

@jsnellbaker jsnellbaker merged commit f81a0bd into prebid:master Jun 4, 2018
Pupis pushed a commit to adform/Prebid.js that referenced this pull request Jun 7, 2018
* renewed everything in hope to make progress

* -

* -

* -

* -

* -

* -

* -

* -

* -

* -

* updated test params

* updated test params

* -
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
* renewed everything in hope to make progress

* -

* -

* -

* -

* -

* -

* -

* -

* -

* -

* updated test params

* updated test params

* -
StefanWallin pushed a commit to mittmedia/Prebid.js that referenced this pull request Sep 28, 2018
* renewed everything in hope to make progress

* -

* -

* -

* -

* -

* -

* -

* -

* -

* -

* updated test params

* updated test params

* -
ghost pushed a commit to devunrulymedia/Prebid.js that referenced this pull request Jan 30, 2019
* renewed everything in hope to make progress

* -

* -

* -

* -

* -

* -

* -

* -

* -

* -

* updated test params

* updated test params

* -
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.

2 participants