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: Flipp #2759

Merged
merged 14 commits into from
May 17, 2023
Merged

New adapter: Flipp #2759

merged 14 commits into from
May 17, 2023

Conversation

hasan-kanjee
Copy link
Contributor

@hasan-kanjee hasan-kanjee commented May 9, 2023

New Flipp Adapter
Link to doc: prebid/prebid.github.io#4551

hasan-kanjee and others added 5 commits April 13, 2023 10:31
…client side adapter (#5)

* OFF-552 fix IMP id being set incorrectly

* use impression id as the request id

* check startCompact

* make bid.h zero

* remove unused const

---------

Co-authored-by: Hasan Kanjee <[email protected]>
const BannerType = "banner"
const InlineDivName = "inline"
const FlippBidder = "flipp"
const DefaultCurrency = "USD"
Copy link
Contributor

@Sonali-More-Xandr Sonali-More-Xandr May 10, 2023

Choose a reason for hiding this comment

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

nit:- all these constants can be grouped in one const block for better readability like this :-

const (
	BannerType      = "banner"
	InlineDivName   = "inline"
	FlippBidder     = "flipp"
	DefaultCurrency = "USD"
)


var Count = int64(1)
var AdTypes = []int64{4309, 641}
var DtxTypes = []int64{5061}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:- These variable declarations can be grouped in a var block for better readability :-

var (
	Count    = int64(1)
	AdTypes  = []int64{4309, 641}
	DtxTypes = []int64{5061}
)


"github.com/buger/jsonparser"
"github.com/gofrs/uuid"
"github.com/prebid/openrtb/v17/openrtb2"
Copy link
Contributor

Choose a reason for hiding this comment

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

We have upgraded the version of the openrtb library from v17 to v19. Please update your import statements to use
github.com/prebid/openrtb/v19/openrtb2

Comment on lines 164 to 166
if responseData.StatusCode == http.StatusNoContent {
return nil, nil
}
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 correct, but you can also make use of existing util functions :-

if httputil.IsResponseStatusCodeNoContent(responseData) {
		return nil, nil
	}

Comment on lines 168 to 180
if responseData.StatusCode == http.StatusBadRequest {
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),
}
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.

As mentioned above, you can make use of existing util functions for these checks :-

if err := httputil.CheckResponseStatusCodeForErrors(responseData); err != nil {
		return nil, []error{err}
	}

SiteID int64 `json:"siteId"`
ZoneIds []int64 `json:"zoneIds,omitempty"`
UserKey string `json:"userKey,omitempty"`
Options *map[string]interface{} `json:"options,omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add this parameter in json schema defined in static/bidder-params/flipp.json as well

@hasan-kanjee
Copy link
Contributor Author

@onkarvhanumante @Sonali-More-Xandr thank you for the review. Addressed the comments and confirmed email verification.

Copy link
Contributor

@onkarvhanumante onkarvhanumante left a comment

Choose a reason for hiding this comment

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

@hasan-kanjee Thanks for keeping patience with code reviews. Added some suggestions to some json tests and checks

@hasan-kanjee
Copy link
Contributor Author

@onkarvhanumante thanks again for the review. I made the code formatting changes as you suggested and added some additional test cases. For the UID generation code path, I am unable to mock the uuid.NewV4() because making changes to Builder() parameters is not allowed. Do you have any suggestions?

@onkarvhanumante
Copy link
Contributor

@onkarvhanumante thanks again for the review. I made the code formatting changes as you suggested and added some additional test cases. For the UID generation code path, I am unable to mock the uuid.NewV4() because making changes to Builder() parameters is not allowed. Do you have any suggestions?

Good callout/ We can skip uuid.NewV4() for now.

"type": "string"
},
"options": {
"type": "object"
Copy link
Contributor

@Sonali-More-Xandr Sonali-More-Xandr May 16, 2023

Choose a reason for hiding this comment

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

can you please add detailed schema over here? Based on your test jsons here is what it may look like. Please add any other fields that you are expecting as part of this object:-

"options": {
      "type": "object",
      "properties": {
        "startCompact": {
          "type": "boolean"
        }
      }
    }

@Sonali-More-Xandr
Copy link
Contributor

Thank you @hasan-kanjee for addressing the comments. I have left one more suggestion on the PR. Please let me know your thoughts on this. Ready to approve it once it's resolved.

@hasan-kanjee
Copy link
Contributor Author

@Sonali-More-Xandr @onkarvhanumante added updated options object parameters. Added some additional code so that one of the new parameters is sent to bidding server.

@hasan-kanjee
Copy link
Contributor Author

@onkarvhanumante @Sonali-More-Xandr thank you for merging this PR. Can we also merge the documentation PR?

prebid/prebid.github.io#4551

@onkarvhanumante
Copy link
Contributor

@onkarvhanumante @Sonali-More-Xandr thank you for merging this PR. Can we also merge the documentation PR?

prebid/prebid.github.io#4551

documentation PR is being reviewed. This PR had a feedback from reviewer prebid/prebid.github.io#4551 (review)

blueseasx pushed a commit to blueseasx/prebid-server that referenced this pull request Jun 2, 2023
Co-authored-by: Jairo <[email protected]>
Co-authored-by: Mike Lei <[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.

5 participants