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

AMX Adapter: support native format #2220

Merged
merged 2 commits into from
Apr 25, 2022

Conversation

nickjacob
Copy link
Contributor

This PR adds support for native ads & removes unnecessary legacy code that was used for interpolating pixels into ad markup:

  • add support for native requests, checking bid type based on bid.ext
  • update AMX adapter internal version to 1.2
  • add test of bid.ext / bid type logic in MakeBids

@nickjacob nickjacob changed the title AMX Adapter: support native form AMX Adapter: support native format Apr 14, 2022
@VeronikaSolovei9
Copy link
Contributor

VeronikaSolovei9 commented Apr 19, 2022

These changes looks good to me. However while I was reviewing and testing this PR I noticed you didn't specify required parameters for imp.ext.amx. In amx.json file there are 2 fields imp.ext.amx can have: tagId and adUnitId. This corresponds to ExtImpAMX.
Without specifying required fields I can run request with empty imp.ext.amx and it doesn't throw error, which I think is not a valid bidder config. I may be wrong, I don't know how your system works inside. If empty imp.ext.amx is a valid case, please ignore this comment. If not - specify required fields. Please refer to PBS documentation and examples: https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html#bidder-parameters

@nickjacob
Copy link
Contributor Author

Thanks Veronika!

We do support empty ext.amx. We allow passing the tagId parameter via a custom endpoint that we provide publishers, and adUnitId is not required--that's why we don't require or test for imp.ext.amx.

}

assert.Len(t, bids.Bids, 1)
assert.Equal(t, bids.Bids[0].BidType, test.bidType)
Copy link
Contributor

Choose a reason for hiding this comment

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

The order for this needs to be the other way round like:

assert.Equal(t, test.bidType, bids.Bids[0].BidType)

The assert function signature requires the expected value to be specified first.
Reference: https://pkg.go.dev/github.com/stretchr/testify/assert#Equal

@mansinahar
Copy link
Contributor

LGTM except for the above minor comment. Also, please update the docs here: https://github.com/prebid/prebid.github.io/blob/master/dev-docs/bidders/amx.md to include native as a supported media type

@nickjacob
Copy link
Contributor Author

Thanks @mansinahar -- I made the change to the test & opened this PR: prebid/prebid.github.io#3714

Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

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

Thank you for your answers. LGTM

@mansinahar
Copy link
Contributor

Thanks @mansinahar -- I made the change to the test & opened this PR: prebid/prebid.github.io#3714

Thanks @nickjacob. Looks good to me

@mansinahar mansinahar merged commit 6efb55d into prebid:master Apr 25, 2022
jorgeluisrocha pushed a commit to jwplayer/prebid-server that referenced this pull request Sep 28, 2022
shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022
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