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 ucfunnelBidAdapter #1990

Merged
merged 19 commits into from
Feb 2, 2018
Merged

Update ucfunnelBidAdapter #1990

merged 19 commits into from
Feb 2, 2018

Conversation

RyanChouTw
Copy link
Contributor

@RyanChouTw RyanChouTw commented Dec 21, 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: 'ucfunnel',
  params: {
    adid: "test-ad-83444226E44368D1E32E49EEBE6D29"
  }
}

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

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

Other information

@snapwich
Copy link
Collaborator

snapwich commented Jan 8, 2018

You need to include tests (with 80%+ coverage) with your adapter for it to be merged. Also your pull-request description is just the template, you need to fill in the details.

@snapwich snapwich self-requested a review January 8, 2018 18:18
Copy link
Collaborator

@snapwich snapwich left a comment

Choose a reason for hiding this comment

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

Needs updates

@RyanChouTw
Copy link
Contributor Author

@snapwich we add the test case of ucfunnelBidAdapter for v1.0.

expect(request[0].url).to.equal(URL);
});
it('bidRequest data', () => {
const request = spec.buildRequests(bidReq);
Copy link
Collaborator

Choose a reason for hiding this comment

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

There's no reason this needs to be three separate tests. It will just make the test suite run longer for no discernible reason.

@RyanChouTw
Copy link
Contributor Author

@snapwich We combine all tests for bid request into single one.

@mkendall07 mkendall07 added this to the Prebid 1.3 release milestone Jan 24, 2018
Copy link
Collaborator

@snapwich snapwich left a comment

Choose a reason for hiding this comment

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

LGTM

@snapwich snapwich added the LGTM label Feb 2, 2018
@mkendall07 mkendall07 merged commit d7c9350 into prebid:master Feb 2, 2018
@RyanChouTw
Copy link
Contributor Author

RyanChouTw commented May 15, 2018

Hello @snapwich

We expect this commit can be merged into Prebid:master. And then ucfunnel adapter will be released and listed in the page http://prebid.org/download.html?
However, it didn't happen. Do you know how we can make ucfunnel adapter officially released in Prebidjs 1.x.x?
Thank you

dluxemburg pushed a commit to Genius/Prebid.js that referenced this pull request Jul 17, 2018
* Add a new ucfunnel Adapter and test page

* Add a new ucfunnel Adapter and test page

* 1. Use prebid lib in the repo to keep updated
2. Replace var with let
3. Put JSON.parse(JSON.stringify()) into try catch block

* utils.getTopWindowLocation is a function

* Change to modules from adapters

* Migrate to module design

* [Dev Fix] Remove width and height which can be got from ad unit id

* Update ucfunnelBidAdapter to fit into new spec

* Correct the endpoint. Fix the error of query string

* Add test case for ucfunnelBidAdapter

* Fix lint error

* Update version number

* Combine all checks on bid request
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.

5 participants