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: Сointraffic #3647

Merged
merged 9 commits into from
Jun 28, 2024
Merged

New Adapter: Сointraffic #3647

merged 9 commits into from
Jun 28, 2024

Conversation

sergeykcointraffic
Copy link
Contributor

@sergeykcointraffic sergeykcointraffic commented Apr 26, 2024

Adds Prebid Server support.

🏷 Type of documentation

  • new bid adapter
  • update bid adapter
  • new feature
  • text edit only (wording, typos)
  • bugfix (code examples)
  • new examples

🏷 Description of change

var bidExt openrtb_ext.ExtBid
err := json.Unmarshal(bid.Ext, &bidExt)
if err == nil && bidExt.Prebid != nil {
return openrtb_ext.ParseBidType(string(bidExt.Prebid.Type))

Choose a reason for hiding this comment

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

Consider this as a suggestion. Prebid server expects the media type to be explicitly set in the adapter response. Therefore, recommends implementing a pattern where the adapter server sets the MType field in the response to accurately determine the media type for the impression.

@sergeykcointraffic sergeykcointraffic marked this pull request as draft April 26, 2024 07:36
@SyntaxNode SyntaxNode changed the title Adds cointraffic adapter New Adapter: cointraffic Apr 26, 2024
endpoint string
}

// Builder builds a new instance of the Foo adapter for the given bidder with the given config.
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 using the template from the docs. Please rename "Foo" to "Cointraffic" for your adapter.

package openrtb_ext

type ExtImpCointraffic struct {
PlacementId string `json:"PlacementId"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Please consider using all lower case "placementid" or snake case "placementId" for bidder parameters.

Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 110d864

cointraffic

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:19:	Builder		100.0%
github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:26:	MakeRequests	83.3%
github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:41:	buildRequest	87.5%
github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:61:	MakeBids	94.1%
total:										(statements)	90.9%

Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, b3204a2

cointraffic

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:19:	Builder		100.0%
github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:26:	MakeRequests	83.3%
github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:41:	buildRequest	87.5%
github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:61:	MakeBids	94.1%
total:										(statements)	90.9%

@sergeykcointraffic sergeykcointraffic changed the title New Adapter: cointraffic New Adapter: Сointraffic May 17, 2024
@sergeykcointraffic sergeykcointraffic marked this pull request as ready for review May 17, 2024 09:04
Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, bf3caf1

cointraffic

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:19:	Builder		100.0%
github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:26:	MakeRequests	83.3%
github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:41:	buildRequest	87.5%
github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:61:	MakeBids	94.1%
total:										(statements)	90.9%

Comment on lines 27 to 31
if len(request.Imp) == 0 {
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.

should exclude this check from adapter PR. Prebid server core framework checks for impression length before invoking adapter code

Comment on lines 62 to 79
if responseData.StatusCode == http.StatusNoContent {
return nil, nil
}

if responseData.StatusCode == http.StatusBadRequest {
err := &errortypes.BadInput{
Message: fmt.Sprintf("Unexpected code: %d. Run with request.debug = 1 for more info.", responseData.StatusCode),
}
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),
}
return nil, []error{err}
}

Copy link
Contributor

Choose a reason for hiding this comment

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

could make use of response utils

func CheckResponseStatusCodeForErrors(response *ResponseData) error {
if response.StatusCode == http.StatusBadRequest {
return &errortypes.BadInput{
Message: fmt.Sprintf("Unexpected status code: %d. Run with request.debug = 1 for more info", response.StatusCode),
}
}
if response.StatusCode != http.StatusOK {
return &errortypes.BadServerResponse{
Message: fmt.Sprintf("Unexpected status code: %d. Run with request.debug = 1 for more info", response.StatusCode),
}
}
return nil
}
func IsResponseStatusCodeNoContent(response *ResponseData) bool {
return response.StatusCode == http.StatusNoContent
}

Comment on lines 84 to 86

bidResponse := adapters.NewBidderResponseWithBidsCapacity(len(response.SeatBid[0].Bid))
bidResponse.Currency = response.Cur
Copy link
Contributor

Choose a reason for hiding this comment

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

adapters.NewBidderResponseWithBidsCapacity initialises bidResponse.Currency with USD value

consider scenario where bidResponse.Currency is empty, assigning response.Cur directly may set bidResponse.Currency to empty value

consider implementing following changes:

	bidResponse := adapters.NewBidderResponseWithBidsCapacity(len(response.SeatBid[0].Bid))
        if response.Cur != "" {
	 bidResponse.Currency = response.Cur
       }

@@ -0,0 +1,10 @@
endpoint: "https://apps.adsgravity.io/pbs/v1/request"
Copy link
Contributor

Choose a reason for hiding this comment

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

% curl -i --location --request POST 'https://apps.adsgravity.io/pbs/v1/request'
HTTP/2 400
date: Wed, 05 Jun 2024 11:16:14 GMT
content-type: text/javascript;charset=UTF-8

endpoint is reachable

Comment on lines +2 to +3
maintainer:
email: [email protected]
Copy link
Contributor

Choose a reason for hiding this comment

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

Prebid team has sent email to verify above maintainers email. Requesting to responding back on email thread

Copy link
Contributor

Choose a reason for hiding this comment

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

@sergeykcointraffic Please reply to email sent from us.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gargcreation1992 We replied to this email a few weeks ago.
Screenshot 2024-06-21 at 11 14 30

Copy link
Contributor

@gargcreation1992 gargcreation1992 Jun 25, 2024

Choose a reason for hiding this comment

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

Screenshot 2024-06-25 at 5 39 53 PM

We have not received it yet. Please send it again. Is the email bouncing? @stsepelin

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

@stsepelin We have not received it. Could you please check the outbox? Also please check if your email policies allows you to send email outside your org.

Copy link
Contributor

Choose a reason for hiding this comment

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

@gargcreation1992 Everything seems fine. Could you also check the spam folder/filters?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I see your responses in the spam folder.

Copy link
Contributor

Choose a reason for hiding this comment

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

I will check if there is anything we can do with this.

Comment on lines 7 to 9
"imp": [
{
"id": "testImpressionId",
Copy link
Contributor

Choose a reason for hiding this comment

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

should update json test where request has multiple imp

Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, c21d599

cointraffic

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:17:	Builder		100.0%
github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:24:	MakeRequests	75.0%
github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:33:	buildRequest	87.5%
github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:53:	MakeBids	93.3%
total:										(statements)	89.7%

@stsepelin
Copy link
Contributor

Hey @onkarvhanumante! Could you review this pull request again?

return []*adapters.RequestData{requestData}, nil
}

func (a *adapter) buildRequest(request *openrtb2.BidRequest) (*adapters.RequestData, error) {
Copy link
Contributor

Choose a reason for hiding this comment

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

buildRequest only builds headers and marshals request. This logic can be moved inside MakeRequests function

@onkarvhanumante
Copy link
Contributor

Hey @onkarvhanumante! Could you review this pull request again?

good to merge after addressing #3647 (comment)

Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 8872565

cointraffic

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:17:	Builder		100.0%
github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:24:	MakeRequests	88.9%
github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:46:	MakeBids	93.3%
total:										(statements)	92.3%

@bsardo bsardo assigned bsardo and unassigned gargcreation1992 Jun 27, 2024
Copy link
Collaborator

@bsardo bsardo left a comment

Choose a reason for hiding this comment

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

This is looking good; I have just two nitpicks.

@@ -80,6 +80,7 @@ var coreBidderNames []BidderName = []BidderName{
BidderCadentApertureMX,
BidderCcx,
BidderCoinzilla,
BidderCointraffic,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Super nitpick: please move this above BidderCoinzilla to maintain alphabetical order.

@@ -368,6 +369,7 @@ const (
BidderCadentApertureMX BidderName = "cadent_aperture_mx"
BidderCcx BidderName = "ccx"
BidderCoinzilla BidderName = "coinzilla"
BidderCointraffic BidderName = "cointraffic"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Super nitpick: please move this above BidderCoinzilla to maintain alphabetical order.

Copy link

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, cbf5b42

cointraffic

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:17:	Builder		100.0%
github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:24:	MakeRequests	88.9%
github.com/prebid/prebid-server/v2/adapters/cointraffic/cointraffic.go:46:	MakeBids	93.3%
total:										(statements)	92.3%

@stsepelin
Copy link
Contributor

@bsardo Thanks for the review. Those two nitpicks are fixed now :)

@onkarvhanumante onkarvhanumante self-requested a review June 28, 2024 11:30
@bsardo bsardo merged commit 057e25d into prebid:master Jun 28, 2024
5 checks passed
mefjush pushed a commit to adhese/prebid-server that referenced this pull request Jul 19, 2024
Co-authored-by: Aleksandr Štšepelin <[email protected]>
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.

6 participants