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: liftoff #2968

Merged
merged 11 commits into from
Aug 11, 2023
Merged

New Adapter: liftoff #2968

merged 11 commits into from
Aug 11, 2023

Conversation

Vungle-GordonTian
Copy link
Contributor

}

// Check if imp comes with bid floor amount defined in a foreign currency
if imp.BidFloor > 0 && imp.BidFloorCur != "" && strings.ToUpper(imp.BidFloorCur) != "USD" {

Choose a reason for hiding this comment

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

make "USD" as a const for multiple place using?

// Convert to US dollars
convertedValue, err := requestInfo.ConvertCurrency(imp.BidFloor, imp.BidFloorCur, "USD")
if err != nil {
return nil, []error{err}

Choose a reason for hiding this comment

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

if need to append to errs and return?

continue
}

if len(bidderImpExt.PubAppStoreID) > 0 && len(bidderImpExt.PlacementRefID) > 0 && len(bidderImpExt.BidToken) > 0 {

Choose a reason for hiding this comment

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

use bidderImpExt.PubAppStoreID != ""?

Endpoint: config.Endpoint,
}

return bidder, nil

Choose a reason for hiding this comment

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

return directly? such as:
return &adapter{
Endpoint: config.Endpoint,
}, nil

PS: looks like the bidderName & server is unused?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it's unused. But the interface must provide the parameters.

Copy link
Contributor

@SyntaxNode SyntaxNode left a comment

Choose a reason for hiding this comment

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

Great first pass. Thank you for properly handling all errors.

adapters/liftoff/liftoff.go Outdated Show resolved Hide resolved

type liftoffImpressionExt struct {
*adapters.ExtImpBidder
// Ext represents the vungle extension.
Copy link
Contributor

Choose a reason for hiding this comment

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

What's vungle?

Copy link
Contributor Author

@Vungle-GordonTian Vungle-GordonTian Aug 2, 2023

Choose a reason for hiding this comment

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

Vungle is our company name before merging with liftoff. In our server code it's a field name which holds our external partners' token and other info.

adapters/liftoff/liftoff.go Outdated Show resolved Hide resolved
continue
}

if bidderImpExt.PubAppStoreID != "" && bidderImpExt.PlacementRefID != "" && bidderImpExt.BidToken != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move these length constraints to the bidder params json file? Then you don't need to worry about error handling here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.

Copy link
Contributor Author

@Vungle-GordonTian Vungle-GordonTian Aug 2, 2023

Choose a reason for hiding this comment

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

@SyntaxNode I tried moving it into yaml with "minLength": 1, but when I run the same UT test, got the different reuslt from before. It produces request for case missing_appid_or_placementid.json. Is it right?

adapters/liftoff/liftoff.go Outdated Show resolved Hide resolved
@Vungle-GordonTian
Copy link
Contributor Author

@SyntaxNode PTAL, I think I've fix your comments.

go.mod Outdated Show resolved Hide resolved
@@ -0,0 +1,10 @@
endpoint: "https://rtb.ads.vungle.com/bid/t/c770f32"
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 email to this email address for verification.

Copy link
Contributor

Choose a reason for hiding this comment

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

@Vungle-GordonTian email is bounced. Could you please re-check if this email ID is working.

Copy link
Contributor Author

@Vungle-GordonTian Vungle-GordonTian Aug 4, 2023

Choose a reason for hiding this comment

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

@gargcreation1992 I'm sure this is the valid and correct email address. liftoff.io is blocked only in China, otherwise it should be ok. Can u provide your email info such as country/region, address and send server?

Copy link
Contributor

Choose a reason for hiding this comment

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

Here is the reply that we have received -

Hello [[email protected]](mailto:[email protected]),

We're writing to let you know that the group you tried to contact (platform-ssp) may not exist, or you may not have permission to post messages to the group. A few more details on why you weren't able to post:

 * You might have spelled or formatted the group name incorrectly.
 * The owner of the group may have removed this group.
 * You may need to join the group before receiving permission to post.
 * This group may not be open to posting.

Copy link
Contributor Author

@Vungle-GordonTian Vungle-GordonTian Aug 4, 2023

Choose a reason for hiding this comment

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

@gargcreation1992 Many thanks for the detailed info. I'll reach out to our IT to find out the root cause.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@gargcreation1992 I've reach out to our IT and they have now adjusted the permissions for [email protected] to accept incoming emails from external senders.

Could you try sending an email again and it should all be good.

Copy link
Contributor

Choose a reason for hiding this comment

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

sent again. Please reply with received.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 4 to 6
BidToken string `json:"bid_token,omitempty"`
PubAppStoreID string `json:"app_store_id,omitempty"`
PlacementRefID string `json:"placement_reference_id,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 remove the omitempty tag as all the parameters are defined as required in the bidder-params/liftoff.json

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

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.

this is correct but you might also wanna use built-in methods -

if adapters.IsResponseStatusCodeNoContent(response) {
		return nil, nil
	}

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

Comment on lines 136 to 137
for _, temp := range seatBid.Bid {
bid := temp // avoid taking address of a for loop variable
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 might wanna consider the following code to avoid creating a new temp variable -

for i := range seatBid.Bid {
    b := &adapters.TypedBid{
				Bid:     &seatBid.Bid[i],
				BidType: openrtb_ext.BidTypeVideo,
			}
       // more code

}

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 optimizing

@Vungle-GordonTian
Copy link
Contributor Author

Vungle-GordonTian commented Aug 11, 2023

@gargcreation1992 I'm wondering what's the next move since this PR and user documentation PR are both approved? And I've done local end to end test.

@gargcreation1992
Copy link
Contributor

@gargcreation1992 I'm wondering what's the next move since this PR and user documentation PR are both approved? And I've done local end to end test.

@Vungle-GordonTian thanks for being patient on this one. There are two required approvals before we merge this one in. Someone from the team will be taking a look at this PR soon.

@Vungle-GordonTian
Copy link
Contributor Author

@gargcreation1992 Per as the documentation said, you will verify the adapter works correctly on a technical level during the code review, now since the code has been merged, I want to confirm if the technical level verification(I would reckon that as e2e test, right?) has been done.

@gargcreation1992
Copy link
Contributor

@gargcreation1992 Per as the documentation said, you will verify the adapter works correctly on a technical level during the code review, now since the code has been merged, I want to confirm if the technical level verification(I would reckon that as e2e test, right?) has been done.

Please refer to this part of documentation for e2e test - https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html#manual-end-to-end-tests

The general guideline is to run prebid-server in your local and test your adapter using a sample request as mentioned in the docs above.

@Vungle-GordonTian
Copy link
Contributor Author

Vungle-GordonTian commented Sep 1, 2023

@gargcreation1992 Per as the documentation said, you will verify the adapter works correctly on a technical level during the code review, now since the code has been merged, I want to confirm if the technical level verification(I would reckon that as e2e test, right?) has been done.

Please refer to this part of documentation for e2e test - https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html#manual-end-to-end-tests

The general guideline is to run prebid-server in your local and test your adapter using a sample request as mentioned in the docs above.

@gargcreation1992 Yeah, I've done the local e2e test already.

@Vungle-GordonTian
Copy link
Contributor Author

@gargcreation1992 Per as the documentation said, you will verify the adapter works correctly on a technical level during the code review, now since the code has been merged, I want to confirm if the technical level verification(I would reckon that as e2e test, right?) has been done.

Please refer to this part of documentation for e2e test - https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html#manual-end-to-end-tests
The general guideline is to run prebid-server in your local and test your adapter using a sample request as mentioned in the docs above.

@gargcreation1992 Yeah, I've done the local e2e test already.

@gargcreation1992 Hi, I am wondering when will our adapter be on production on Xandr side? Will we be informed of it through the group email?

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