-
Notifications
You must be signed in to change notification settings - Fork 750
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
Formalizing support for site.ext.amp
#711
Conversation
site.ext.amp
site.ext.amp
endpoints/openrtb2/auction.go
Outdated
} | ||
} | ||
|
||
func setAmpExt(site *openrtb.Site, value string) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any particular reason you put this in auction.go rather than amp_auction.go?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No reason... but that makes sense. I'll move it.
It's used in both files... started out named something else, but with "amp" in the name I can definitely see how it fits better in the amp_auction
file.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me, just an odd home for a function, but not important enough to deny the PR.
* Started adding some code to validate the amp ext, and some unit tests which dont pass yet. * Fixed some bugs. Added more tests. * Updated the docs. * Fixed some tests... but not all. * Fixed last remaining bug. * Moved setAmpExt to the amp_auction file.
* Started adding some code to validate the amp ext, and some unit tests which dont pass yet. * Fixed some bugs. Added more tests. * Updated the docs. * Fixed some tests... but not all. * Fixed last remaining bug. * Moved setAmpExt to the amp_auction file.
* Started adding some code to validate the amp ext, and some unit tests which dont pass yet. * Fixed some bugs. Added more tests. * Updated the docs. * Fixed some tests... but not all. * Fixed last remaining bug. * Moved setAmpExt to the amp_auction file.
This fixes #708
Technically the
ext
was already going to the bidders... so this just adds input validation & documents it.Along the way, I found that our AMP tests were testing lots of payloads with
request.app
defined. Since AMP impliessite
, these tests don't make sense. I added an explicit check forrequest.app
in the code.This seems like a breaking change... but I don't think it is, because a real AMP page would send the
curl
query param, and there was already code which definedsite
in that case. If the Stored Request already definedapp
, then it would cause both to be defined, which would make the request invalid anyway.