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

Update Vertoz adapter for Prebid 1.0 #2104

Merged
merged 2 commits into from
Feb 23, 2018
Merged

Update Vertoz adapter for Prebid 1.0 #2104

merged 2 commits into from
Feb 23, 2018

Conversation

mohit546
Copy link
Contributor

@mohit546 mohit546 commented Feb 5, 2018

    * Migrating to Prebid 1.0
    * spec added

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: 'vertoz',
  params: {
    placementId: "VZ-HB-B784382V6C6G3C"
  }
}

        * Migrating to Prebid 1.0
        * spec added
@matthewlane matthewlane self-requested a review February 6, 2018 18:12
@matthewlane matthewlane self-assigned this Feb 6, 2018
@mkendall07 mkendall07 added this to the Prebid 1.4.0 milestone Feb 12, 2018
Copy link
Collaborator

@matthewlane matthewlane left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, a few changes requested. Also, when testing this adapter with the test parameters, I'm getting a response of {"slotBidId":"274058c816c8fe","statusText":"Application_Error"} from the endpoint, please check and advise if these params are still valid for testing


export const spec = {
code: BIDDER_CODE,
aliases: [], // short code
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 aliases defined, this line can be dropped

}
return bidResponses;
},
getUserSyncs: function (syncOptions, serverResponses) {
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 need any user syncing, this function can be removed

expect(request.method).to.equal('POST');
});
});
});
Copy link
Collaborator

Choose a reason for hiding this comment

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

These tests are only providing 68.97% coverage of the adapter, we require at least 80% before merge. Removing the unneeded code mentioned in the other comments should bump the percentage up, and then testing the interpretResponse should help getting there

@matthewlane
Copy link
Collaborator

matthewlane commented Feb 13, 2018

@mohit546 Please also submit a PR to the docs repo to update your adapter file to add prebid_1_0_supported: true to the header and update any bidder params as necessary

- removed unneeded code
@matthewlane
Copy link
Collaborator

Docs PR: prebid/prebid.github.io#616

Copy link
Collaborator

@matthewlane matthewlane left a comment

Choose a reason for hiding this comment

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

Changes look good, tests now have great than 80% coverage. @mohit546 Still receiving {"slotBidId":"274058c816c8fe","statusText":"Application_Error"} when requesting test bids though, please check. Once we can validate bid responses this should be good for merge

@Prebid-Vertoz
Copy link
Contributor

Hi @matthewlane,
Can you please share request here so that we can find more.
Because we are unable to create this issue on our side.

@matthewlane
Copy link
Collaborator

@Prebid-Vertoz Sure. After running gulp serve, and placing the vertoz params on the hello_world test page, the request I'm seeing to http://hb.vrtzads.com/vzhbidder/bid? is

{"q":"%7B%22_vzPlacementId%22%3A%22VZ-HB-B784382V6C6G3C%22%2C%22_rqsrc%22%3A%22http%3A%2F%2Flocalhost%3A9999%2FintegrationExamples%2Fgpt%2Fhello_world.html%22%2C%22_cb%22%3A1518816756%2C%22_slotBidId%22%3A%2223d814c7aad797%22%2C%22_cbn%22%3A%22pbjs%22%7D"}

and the response is

{"slotBidId":"23d814c7aad797","statusText":"Application_Error"}

@Prebid-Vertoz
Copy link
Contributor

Hi @matthewlane,
Cause behind getting "Application Error" in response is "localhost:9999" in "_rqsrc" parameter.
This issue is arising because we are using a third party library for domain validation and it is not allowing
":" in domain.
Is it possible to test it with some valid domain to get a proper response?

@matthewlane
Copy link
Collaborator

@Prebid-Vertoz Moved my test page to a domain without a port in the domain, request is now

{"q":"%7B%22_vzPlacementId%22%3A%22VZ-HB-B784382V6C6G3C%22%2C%22_rqsrc%22%3A%22http%3A%2F%2Fmlane.devnxs.net%2Fvertoz%2F%22%2C%22_cb%22%3A1519153843%2C%22_slotBidId%22%3A%224f809a85a752f3%22%2C%22_cbn%22%3A%22pbjs%22%7D"}

and response is now

{"slotBidId":"4f809a85a752f3","statusText":"InvalidRequest"}

Please advise about this error. If you have a publicly available test page that requests and returns bids with this adapter I can check that as well

@Prebid-Vertoz
Copy link
Contributor

Prebid-Vertoz commented Feb 21, 2018

@matthewlane , thanks for response. " http://filmgrapevine.com/prebid_test_page.html " this is a test page where bids are requested and returned from the adapter.

@matthewlane
Copy link
Collaborator

@Prebid-Vertoz Great, I'm able to access that page. Current response from Vertoz there is

{"vzhPlacementId":"VZ-HB-B784382V6C6G3C","slotBidId":"21613f3032bf11","statusText":"NO_BIDS"}

please check. As soon as I can verify a valid bid response rather than a no_bids response this should be good for merge

@matthewlane matthewlane removed this from the Prebid 1.4.0 milestone Feb 21, 2018
@Prebid-Vertoz
Copy link
Contributor

Hi @matthewlane ,
Please check now. You should get a bid in return.

@mkendall07 mkendall07 added this to the Prebid 1.5.0 milestone Feb 23, 2018
@matthewlane matthewlane merged commit 9d55719 into prebid:master Feb 23, 2018
mizmaar3 added a commit to widespace-os/Prebid.js that referenced this pull request Feb 28, 2018
* master:
  Update Vertoz adapter for Prebid 1.0 (prebid#2104)
  Add multiple bids request  (prebid#2136)
  [NEW Adapter] RTBHouseBidAdapter (prebid#2184)
  Update Innity Adapter to Prebid.js v1.0 (prebid#2180)
  Update Adyoulike Adapter to prebid 1.0 (prebid#2077)
  Change bidderCode for DAN Marketplace Bid Adapter (prebid#2183)
  only count bid timeouts if bidder didn't call done. fixes prebid#2146 (prebid#2154)
  [Edit BidAdapter] rxrtb adapter for Perbid.js 1.0 (prebid#2182)
  Update NasmediaAdmixer adapter (prebid#2164)
  only do video caching if we don't already have a videoCacheKey (prebid#2143)
@mohit546
Copy link
Contributor Author

mohit546 commented Mar 9, 2018

@matthewlane ,
tried to download vertoz adaptor 1.4.0 from http://prebid.org/
got email
selection_008

but getting following error on link
selection_009

saw release notes
vertoz adaptor listed in 1.5.0, but not in 1.4.0

@matthewlane
Copy link
Collaborator

@mohit546 Thanks for letting me know, the docs pr was merged before the download page got updated for 1.5.0. I reverted the commit and will add it back once the page is updated and it should work properly then

@mohit546
Copy link
Contributor Author

mohit546 commented May 28, 2018

@matthewlane i am not seeing vertoz adaptor in 1.12.0, and there is no option for 1.0.0. do i have to update adaptor for latest version ?

@matthewlane
Copy link
Collaborator

@mohit546 Sorry about that, re-reverted the docs in prebid/prebid.github.io#805 and Vertoz should be on the download page now

dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
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.

4 participants