-
Notifications
You must be signed in to change notification settings - Fork 770
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
Attekmi: Remove partnerName param requirement #4208
base: master
Are you sure you want to change the base?
Attekmi: Remove partnerName param requirement #4208
Conversation
Code coverage summaryNote:
smarthubRefer here for heat map coverage report
|
@SyntaxNode @bsardo Please review it. |
@@ -35,11 +35,9 @@ var invalidParams = []string{ | |||
`{"partnerName":"partnertest"}`, | |||
`{"seat":"9Q20EdGxzgWdfPYShScl"}`, | |||
`{"token":"Y9Evrh40ejsrCR4EtidUt1cSxhJsz8X1"}`, | |||
`{"seat":"9Q20EdGxzgWdfPYShScl", "token":"alNYtemWggraDVbhJrsOs9pXc3Eld32E"}`, |
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.
Since none of your params are now considered required, please add the following test cases to validParams
to ensure that the absence of each param is valid, the empty string is allowed for partnerId
and a minimum length of 1 is valid for seat
and token
:
{"partnerName":""}
{"partnerName":"1"}
{"seat":"1"},
{"token":"1"},
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.
ok I added this test cases and all tests was passed
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.
Since none of your params are now considered required
@bsardo - The seat and token params are still required in this schema.
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.
Hmm this cases I added to invalidParams because we using parnterName+seat+token OR seat+token in aliases, this means that the pair seat+token will always be required.
For example:
endpoint: "https://prebid.attekmi.com/pbserver?partnerName={{.Host}}&seat={{.AccountID}}&token={{.SourceId}}" |
endpoint: "https://vimayx-prebid.attekmi.com/pbserver/?seat={{.AccountID}}&token={{.SourceId}}" |
Code coverage summaryNote:
smarthubRefer here for heat map coverage report
|
AccountID: params.Seat, | ||
SourceId: params.Token, | ||
Host: params.PartnerName, |
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.
Might be better to delete the Host template param entirely. This is only used for endpoint macros and neither smarthub nor its aliases use this macro.
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.
Currently, we use both methods of redirecting traffic, both through partnerName and using aliases.We also need to use param partnerName to temporarily connect partners.
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.
Ah, I see. The aliases do not use this macro, but the base smarthub
adapter does. Ok.
Attekmi: add Addigi adapter prebid.github.io#5836
Attekmi: add new adapter Adinify prebid.github.io#5822