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

New Adapter: Clickonometrics #2410

Merged
merged 20 commits into from
Nov 7, 2022
Merged

New Adapter: Clickonometrics #2410

merged 20 commits into from
Nov 7, 2022

Conversation

mjaworskiccx
Copy link
Contributor

No description provided.

@bsardo bsardo changed the title New Adapter: Clickonometrcs New Adapter: Clickonometrics Oct 11, 2022
Copy link
Contributor

@AlexBVolcy AlexBVolcy left a comment

Choose a reason for hiding this comment

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

Please link your docs PR

package openrtb_ext

type ExtImpCcx struct {
PlacementID string `json:"pid"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you rename the json path to be json:"placementId"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Comment on lines +54 to +62
err := &errortypes.BadInput{
Message: "Unexpected status code: 400. Bad request from publisher. Run with request.debug = 1 for more info.",
}
return nil, []error{err}
}

if responseData.StatusCode != http.StatusOK {
err := &errortypes.BadServerResponse{
Message: fmt.Sprintf("Unexpected status code: %d. Run with request.debug = 1 for more info.", responseData.StatusCode),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to cover these two error cases?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@AlexBVolcy
Copy link
Contributor

There is a PR #2370 that is being merged today that will cause conflicts to new adapter PRs.

To resolve these conflicts, you'll need to do the following:

In ccx.go, update the Builder() function signature to include a new parameter server config.Server

In ccx_test.go, update any test that includes a call to Builder() to pass a config.Server object, to that call (i.e. config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "datacenter"})

After the merge, you can look at other adapters to see how the server object is implemented as an example.

return nil, []error{&errortypes.BadInput{
Message: "No impression in the bid request",
}}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

You do not need this check. The core logic of Prebid Server will only call this method if there is at least one impression.

@mjaworskiccx
Copy link
Contributor Author

Done

@bsardo bsardo assigned hhhjort and unassigned VeronikaSolovei9 Oct 18, 2022
@bsardo bsardo requested review from hhhjort and removed request for VeronikaSolovei9 October 18, 2022 17:46
hhhjort
hhhjort previously approved these changes Oct 18, 2022
Copy link
Collaborator

@hhhjort hhhjort left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@AlexBVolcy AlexBVolcy left a comment

Choose a reason for hiding this comment

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

Looks good, one docs related comment

capabilities:
site:
mediaTypes:
- banner
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm noticing a discrepancy between the supported media types in this file vs. what's present in your dev docs. Here it says banner is the media type supported, but the doc says video. Could you address this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for updating your .yaml file, but could you also update your docs PR to include the banner media type as well?

Copy link
Contributor

Choose a reason for hiding this comment

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

@AlexBVolcy I see PR #4127 was opened to address this.

@bsardo
Copy link
Contributor

bsardo commented Oct 26, 2022

@mjaworskiccx can you please merge with master when you get a chance? The 1.19 validation checks are failing because we recently upgraded to 1.19 and that version has made some updates to go fmt that affect the formatting in some of the files you modified. Thanks!

@mjaworskiccx
Copy link
Contributor Author

done

@bsardo
Copy link
Contributor

bsardo commented Oct 31, 2022

Hi @mjaworskiccx, it looks like you have some failing tests due to some recent changes we made on master. You'll need to update the reference to the mxmcherry library with the prebid version of the ortb library.

@mjaworskiccx
Copy link
Contributor Author

done

@bsardo bsardo merged commit 452ea6f into prebid:master Nov 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants