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

Prebid 1.0 compliant Komoona bidder adapter #1743

Merged
merged 8 commits into from
Nov 1, 2017

Conversation

tzafrirb
Copy link
Contributor

@tzafrirb tzafrirb commented Oct 22, 2017

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

  • test parameters for validating bids
{
  bidder: '<bidder name>',
  params: {
    // ...
  }
}

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

Other information

@tzafrirb
Copy link
Contributor Author

Prebid 1.0 compliant Komoona bidder adapter

Copy link
Contributor

@dbemiller dbemiller left a comment

Choose a reason for hiding this comment

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

I'm not seeing any bid when I run the hello_world.html page with the params from the .md file.

I think this has to do with the missing bid params from your interpretResponse... but I didn't go far enough debugging it to find the root of the problem.

export const spec = {
code: BIDDER_CODE,

aliases: ['komoona'],
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just for adapters which have multiple names... since this is the same as your BIDDER_CODE, you can just delete 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.

Removed from adapter file, thanks from the comment

bid.requestId = bid.uuid;
// The creative payload of the returned bid.
bid.ad = bid.creative;
bidResponses.push(bid);
Copy link
Contributor

Choose a reason for hiding this comment

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

There are lots of required params missing from these bids. See: https://github.com/prebid/Prebid.js/pull/1738/files#diff-ac0af2601dcafed83fa9a11b814bf3b5L109

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Our md file was updated with correct placement Id and hb Id for test creative (as well as our servers) and now when using placementId and hbId from md file, the hello_world file will always return a creative

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Working to fix it, it was added to our servers and now testing it out - will ping once live and fixed

@tzafrirb
Copy link
Contributor Author

CI test error was fixed, sorry for that - now Komoona gulp test and lint test pass.
Mandatory fields were added to response from our bid server

@dbemiller
Copy link
Contributor

dbemiller commented Oct 30, 2017

I did a bit of debugging... it looks like the changes in #1742 are causing you problems. We had to make a breaking change to support their use case and maintain a reasonably clean adapter API.

That probably explains our confusion, too... I've been testing with the latest changes from master. You've probably been testing them in your fork, which has slightly older code.

It's a small change. Rather than interpretResponse(body), the bidderFactory is calling your adapter with interpretResponse({ body: body, ... })

…, the bidderFactory is calling your adapter with interpretResponse({ body: body, ... })
@tzafrirb
Copy link
Contributor Author

Thanks for the comment and pointing to correct change - we have sync our fork with master and change adapter to match change #1742

@dbemiller
Copy link
Contributor

Looks good now... thanks!


it('handles JSON.parse errors', () => {
server.respondWith('');
describe('Verify komoonaAdapter build request', () => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

These test cases need to be it block for 2 reasons.

  1. karma is not showing actual count of tests in logs. So even though this is getting executed but count is not correct. I placed it in it block and test count was incremented.
  2. If this test fails than it stops here itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True comment and I accept it - we actually use it for internal products test, not sure why describe was used (probably just copied from old test).

It is fixed now

@jaiminpanchal27 jaiminpanchal27 merged commit 4519cb0 into prebid:master Nov 1, 2017
dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
* Prebid 1.0 compliant bidder adapter

* PlacementId and hbId support display test banner

* Remove aliases

* remove check for aliases, breaks build

* Add bid response test with mandatory params

* change prebid#1742 (prebid#1742): rather than interpretResponse(body), the bidderFactory is calling your adapter with interpretResponse({ body: body, ... })

* replace describe with it
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.

4 participants