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 mabidder #3080

Merged
merged 27 commits into from
Sep 15, 2023
Merged

New Adapter mabidder #3080

merged 27 commits into from
Sep 15, 2023

Conversation

ecdrsvc
Copy link
Contributor

@ecdrsvc ecdrsvc commented Sep 6, 2023

We have an existing Prebid.js adapter and would like to add this new Prebid Server adapter.

@ecdrsvc
Copy link
Contributor Author

ecdrsvc commented Sep 6, 2023

Documentation PR: prebid/prebid.github.io#4848

@onkarvhanumante
Copy link
Contributor

@ecdrsvc feature branch ecdrsvc:master is 22 commits ahead of, 204 commits behind

should sync feature master with latest prebid:master updates.

@github-actions
Copy link

github-actions bot commented Sep 7, 2023

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, 183345e

mabidder

Refer here for heat map coverage report

github.com/prebid/prebid-server/adapters/mabidder/mabidder.go:44:	Builder		100.0%
github.com/prebid/prebid-server/adapters/mabidder/mabidder.go:51:	MakeRequests	80.0%
github.com/prebid/prebid-server/adapters/mabidder/mabidder.go:66:	MakeBids	94.4%
total:									(statements)	92.0%

@ecdrsvc
Copy link
Contributor Author

ecdrsvc commented Sep 7, 2023

@onkarvhanumante, it's synced.

Comment on lines 13 to 14
Endpoint: "https://mabidder-test"},
config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "2"})
Copy link
Contributor

Choose a reason for hiding this comment

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

should use https://prebid.ecdrsvc.com/pbs url as mentioned in bidder-info param

endpoint: "https://prebid.ecdrsvc.com/pbs"

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

if responseData.StatusCode == http.StatusBadRequest {
err := &errortypes.BadInput{
Message: fmt.Sprintf("Unexpected status code: %d. Bad request from publisher.", responseData.StatusCode),
}
return nil, []error{err}
}

if responseData.StatusCode != http.StatusOK {
err := &errortypes.BadServerResponse{
Message: fmt.Sprintf("Unexpected status code: %d.", 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.

just a suggestion, could make use of utils mentioned below

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 fmt.Errorf("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 15 to 38
type maServerResponse struct {
Responses []maBidResponse
PrivateIdStatus string `json:"-"`
}

type maBidResponse struct {
RequestID string `json:"requestId"`
Currency string `json:"currency"`
Width int32 `json:"width"`
Height int32 `json:"height"`
PlacementId string `json:"creativeId"`
Deal string `json:"dealId,omitempty"`
NetRevenue bool `json:"netRevenue"`
TimeToLiveSeconds int32 `json:"ttl"`
AdTag string `json:"ad"`
MediaType string `json:"mediaType"`
Meta maMeta `json:"meta"`
CPM float32 `json:"cpm"`
}

type maMeta struct {
AdDomain []string `json:"advertiserDomains"`
}

Copy link
Contributor

Choose a reason for hiding this comment

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

scope of these struct is within mabidder package. so could ma prefix from above structs

- type maServerResponse struct {
+ type serverResponse struct {

- type maBidResponse struct {
+ type bidResponse struct {

-  type maMeta struct {
+  type meta struct {

Comment on lines 107 to 109
if bidResponse.Currency == "" {
bidResponse.Currency = maBidResp.Currency
}
Copy link
Contributor

@onkarvhanumante onkarvhanumante Sep 8, 2023

Choose a reason for hiding this comment

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

bidResponse is initialized by NewBidderResponseWithBidsCapacity() on line 90

// By default, currency is USD but this behavior might be subject to change.
func NewBidderResponseWithBidsCapacity(bidsCapacity int) *BidderResponse {
return &BidderResponse{
Currency: "USD",
Bids: make([]*TypedBid, 0, bidsCapacity),
}
}

Based on code setup, bidResponse.Currency will always be USD. Therefore, if check on line 107 will always be false and maBidResp.Currency won't be used. Perhaps you would like to make following change.

	if maBidResp.Currency != "" {
		bidResponse.Currency = maBidResp.Currency
	}

@@ -0,0 +1,11 @@
endpoint: "https://prebid.ecdrsvc.com/pbs"
Copy link
Contributor

Choose a reason for hiding this comment

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

endpoint is reachable

@@ -0,0 +1,11 @@
endpoint: "https://prebid.ecdrsvc.com/pbs"
maintainer:
email: lmprebidadapter@loblaw.ca
Copy link
Contributor

@onkarvhanumante onkarvhanumante Sep 8, 2023

Choose a reason for hiding this comment

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

@ecdrsvc , PBS team attempted to verify above maintainer email. However was not able to reach you out on above specified email address.

Note that an working maintainer email is needed to proceed with this PR

Copy link
Contributor

Choose a reason for hiding this comment

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

sent an email again. Please reply with "received"

Copy link
Contributor

@gargcreation1992 gargcreation1992 left a comment

Choose a reason for hiding this comment

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

g2g after addressing Onkar's comment.

@github-actions
Copy link

github-actions bot commented Sep 8, 2023

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, f12bd30

mabidder

Refer here for heat map coverage report

github.com/prebid/prebid-server/adapters/mabidder/mabidder.go:41:	Builder		100.0%
github.com/prebid/prebid-server/adapters/mabidder/mabidder.go:48:	MakeRequests	80.0%
github.com/prebid/prebid-server/adapters/mabidder/mabidder.go:63:	MakeBids	100.0%
total:									(statements)	95.2%

@ecdrsvc
Copy link
Contributor Author

ecdrsvc commented Sep 8, 2023

@onkarvhanumante the code has been updated with all your suggestions.
Regarding the email, there seems to be an issue with it. I'm waiting to hear back from our ops team and will update here when it's resolved.

Copy link
Contributor

@gargcreation1992 gargcreation1992 left a comment

Choose a reason for hiding this comment

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

g2g after email verification.

@onkarvhanumante
Copy link
Contributor

@onkarvhanumante the code has been updated with all your suggestions. Regarding the email, there seems to be an issue with it. I'm waiting to hear back from our ops team and will update here when it's resolved.

adding block label on PR until we get update on working email

@ecdrsvc
Copy link
Contributor Author

ecdrsvc commented Sep 13, 2023

@onkarvhanumante the code has been updated with all your suggestions. Regarding the email, there seems to be an issue with it. I'm waiting to hear back from our ops team and will update here when it's resolved.

adding block label on PR until we get update on working email

The email issue has been fixed. Please send a verification message and we will reply. Thanks.

@onkarvhanumante
Copy link
Contributor

@onkarvhanumante the code has been updated with all your suggestions. Regarding the email, there seems to be an issue with it. I'm waiting to hear back from our ops team and will update here when it's resolved.

adding block label on PR until we get update on working email

The email issue has been fixed. Please send a verification message and we will reply. Thanks.

Verification email is being sent to lmprebidadapter@loblaw.ca. Waiting for reply

Copy link
Contributor

@gargcreation1992 gargcreation1992 left a comment

Choose a reason for hiding this comment

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

g2g after email verification.

@ecdrsvc
Copy link
Contributor Author

ecdrsvc commented Sep 14, 2023

Just confirming here that we got the verification email and replied with 'received'. Let me know if any issues.

@onkarvhanumante
Copy link
Contributor

Just confirming here that we got the verification email and replied with 'received'. Let me know if any issues.

received reply from mabidder

@gargcreation1992 gargcreation1992 merged commit e8632b0 into prebid:master Sep 15, 2023
5 checks passed
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.

3 participants