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.

Sign up for GitHub

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

Jump to bottom

New: ConnectAd PreBid Server Adapter #1473

Closed
wants to merge 18 commits into from
Closed

New: ConnectAd PreBid Server Adapter #1473

wants to merge 18 commits into from

Conversation

rtuschkany
Copy link
Contributor

New PreBid Server Adapter for ConnectAd

@SyntaxNode
Copy link
Contributor

Thank you for your contribution. We'll review this PR early next week.

For now, could you please open a PR in https://github.com/prebid/prebid.github.io to add a doc for this new adapter. More information is found in our new adapter guide: https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html#document-your-adapter

@SyntaxNode SyntaxNode added the needs docs Docs are required for this PR or Issue label Sep 3, 2020
@rtuschkany
Copy link
Contributor Author

I just opened a PR to update our adapter docs: prebid/prebid.github.io#2305

@SyntaxNode SyntaxNode removed the needs docs Docs are required for this PR or Issue label Sep 3, 2020
@SyntaxNode SyntaxNode self-requested a review September 4, 2020 19:08
@SyntaxNode SyntaxNode self-assigned this Sep 4, 2020
SyntaxNode
SyntaxNode reviewed Sep 4, 2020
View reviewed changes
Copy link
Contributor

@SyntaxNode SyntaxNode left a comment

Choose a reason for hiding this comment

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

Thank you for your contribution. The unit test coverage rate is 81% and we strive for higher coverage. I understand that Marshal errors can't be effectively tested, but can you please increase test coverage for other failure branches? such as with:

  • A preprocess error.
  • request.Device.DNT == nil
  • httpRes.StatusCode != http.StatusOK
  • json.Unmarshal(httpRes.Body, &bidResp)
  • unpackImpExt failure branches
  • imp.Banner == nil
  • len(banner.Format) == 0

adapters/connectad/connectad.go Outdated Show resolved Hide resolved
adapters/connectad/connectad.go Outdated Show resolved Hide resolved
adapters/connectad/connectad.go Outdated Show resolved Hide resolved
adapters/connectad/connectad.go Outdated Show resolved Hide resolved
adapters/connectad/connectad.go Outdated Show resolved Hide resolved
adapters/connectad/connectad.go Outdated Show resolved Hide resolved
adapters/connectad/usersync_test.go Outdated Show resolved Hide resolved
openrtb_ext/imp_connectad.go Outdated Show resolved Hide resolved
@SyntaxNode SyntaxNode requested a review from bsardo September 8, 2020 14:15
SyntaxNode
SyntaxNode reviewed Sep 8, 2020
View reviewed changes
Copy link
Contributor

@SyntaxNode SyntaxNode left a comment

Choose a reason for hiding this comment

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

Thank you very much for a quick update. I have just a few more comments.

adapters/connectad/connectad.go Show resolved Hide resolved
adapters/connectad/connectad.go Show resolved Hide resolved
adapters/connectad/connectad.go Outdated Show resolved Hide resolved
adapters/connectad/connectad.go Outdated Show resolved Hide resolved
adapters/connectad/connectad.go Show resolved Hide resolved
adapters/connectad/connectad.go Outdated Show resolved Hide resolved
adapters/connectad/usersync_test.go Outdated Show resolved Hide resolved
@rtuschkany
Copy link
Contributor Author

Thank you very much for a quick update. I have just a few more comments.

Thanks for the very quick review. Just pushed an update

SyntaxNode
SyntaxNode previously approved these changes Sep 9, 2020
View reviewed changes
Copy link
Contributor

@SyntaxNode SyntaxNode left a comment

Choose a reason for hiding this comment

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

Looks great. Thank you. @bsardo will look at this soon for a second approval. Until then, looks like you have a conflict to resolve.

bsardo
bsardo reviewed Sep 10, 2020
View reviewed changes
adapters/connectad/params_test.go Outdated Show resolved Hide resolved
adapters/connectad/params_test.go Show resolved Hide resolved
adapters/connectad/connectadtest/supplemental/nobanner.json Outdated Show resolved Hide resolved
adapters/connectad/connectad.go Show resolved Hide resolved
openrtb_ext/imp_connectad.go Show resolved Hide resolved
@bsardo
Copy link
Collaborator

bsardo commented Sep 10, 2020

Looks good @rtuschkany. Just a couple of questions and suggestions.

@bsardo
Copy link
Collaborator

bsardo commented Sep 14, 2020

Looks good @rtuschkany, just need to resolve the conflict.

@SyntaxNode
Copy link
Contributor

@rtuschkany This looks good to us. We need you to resolve the conflicts before we can merge it in.

@rtuschkany
Copy link
Contributor Author

@rtuschkany This looks good to us. We need you to resolve the conflicts before we can merge it in.

Done!

@SyntaxNode
Copy link
Contributor

Huh. Strange. I still see the "This branch has conflicts that must be resolved" warning on this PR. Did you merge in master for this branch? See https://docs.github.com/en/github/collaborating-with-issues-and-pull-requests/syncing-a-fork

@rtuschkany
Copy link
Contributor Author

@SyntaxNode I had some errors trying to sync. Just opened a new PR (#1505) from a fresh fork. Please close this PR. Thanks!

@SyntaxNode SyntaxNode closed this Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bsardo bsardo bsardo left review comments

@SyntaxNode SyntaxNode SyntaxNode left review comments

Assignees

@bsardo bsardo

@SyntaxNode SyntaxNode

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rtuschkany @SyntaxNode @bsardo