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: Xeworks #2752

Merged
merged 9 commits into from
May 29, 2023
Merged

Conversation

dimashirokov
Copy link
Contributor

No description provided.

endpoint, err := a.buildEndpointFromRequest(openRTBRequest)
if err != nil {
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.

Since you are accessing imp[0] in buildEndpointFromRequest method but please be aware that there could be multiple impression in the request. Read more here.
You probably would need a for loop here to handle multiple impressions in the bidRequest.

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.

requesting few changes.

@Sonali-More-Xandr
Copy link
Contributor

Sonali-More-Xandr commented May 9, 2023

@dimashirokov
Copy link
Contributor Author

Hi guys, thank you for the review

@dimashirokov
Copy link
Contributor Author

Hello @gargcreation1992 and @Sonali-More-Xandr, could you please review the provided updates? Thank you!

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.

Requesting few more changes to improve readability and use hostname as query param instead of uri.

@gargcreation1992
Copy link
Contributor

@dimashirokov Requesting you to please look at the open comments.

@dimashirokov
Copy link
Contributor Author

@gargcreation1992 will check

@dimashirokov
Copy link
Contributor Author

@gargcreation1992 could you please check recent update

@gargcreation1992
Copy link
Contributor

@dimashirokov PR checks are failing. Could you please check?

}

bidResponse.Bids = append(bidResponse.Bids, &adapters.TypedBid{
Bid: &bid,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is incorrect. After the end of for loop, &bid will have the last element of seatBid.Bid slice. I would recommend to change &bid to &seatBid.Bid[bidId].


for seatId, seatBid := range seats {
for bidId, bid := range seatBid.Bid {
var bidExt bidExt
Copy link
Contributor

Choose a reason for hiding this comment

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

I know you are using seatId in the below error messaging but IMHO this is just be iterator values like 0, 1, 2 which won't be helpful in logging, I think. You might wanna remove it.


if len(errs) > 0 {
return nil, errs
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would suggest to simply return - return bidResponse, errs instead of a check here.

@@ -1,4 +1,4 @@
endpoint: "http://prebid-{{.Host}}.xe.works/?pid={{.SourceId}}"
endpoint: "http://prebid-srv.xe.works/?pid={{.SourceId}}&host={{.Host}}"
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

@@ -1,4 +1,4 @@
endpoint: "http://prebid-{{.Host}}.xe.works/?pid={{.SourceId}}"
endpoint: "http://prebid-srv.xe.works/?pid={{.SourceId}}&host={{.Host}}"
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.

sent an email to this address for verification. Please expect an email from [email protected] and reply with received. thanks!

Copy link
Contributor

Choose a reason for hiding this comment

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

email was replied. thanks!

@dimashirokov
Copy link
Contributor Author

Done

@Sonali-More-Xandr Sonali-More-Xandr merged commit f390f79 into prebid:master May 29, 2023
blueseasx pushed a commit to blueseasx/prebid-server that referenced this pull request Jun 2, 2023
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