-
Notifications
You must be signed in to change notification settings - Fork 720
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: Missena #3761
base: master
Are you sure you want to change the base?
New Adapter: Missena #3761
Conversation
Code coverage summaryNote:
missenaRefer here for heat map coverage report
|
static/bidder-info/missena.yaml
Outdated
@@ -0,0 +1,16 @@ | |||
endpoint: https://bid.missena.io/ | |||
maintainer: | |||
email: yboussafa@missena.com |
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.
this looks like a private email, please enter maintenance email
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.
Done ✅
static/bidder-params/missena.json
Outdated
"properties": { | ||
"apiKey": { | ||
"type": "string", | ||
"description": "API Key" |
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.
You have to add "minimum": 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.
You should only add this if you don't want to accept an apiKey
of zero length.
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.
Done ✅
adapters/missena/params_test.go
Outdated
} | ||
|
||
var validParams = []string{ | ||
`{"apiKey": ""}`, |
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.
when You add "minimum": 1 to missena.json add this example to ivalidParams
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.
Done ✅
if request.Device.IP != "" { | ||
headers.Add("X-Forwarded-For", request.Device.IP) | ||
} else if request.Device.IPv6 != "" { | ||
headers.Add("X-Forwarded-For", request.Device.IPv6) |
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.
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.
Done ✅
static/bidder-params/missena.json
Outdated
"properties": { | ||
"apiKey": { | ||
"type": "string", | ||
"description": "API Key" |
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.
You should only add this if you don't want to accept an apiKey
of zero length.
@@ -0,0 +1,16 @@ | |||
endpoint: https://bid.missena.io/ |
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.
Endpoint is reachable
curl -i --location --request POST https://bid.missena.io/
HTTP/1.1 403 Forbidden
Date: Fri, 30 Aug 2024 20:00:07 GMT
Content-Type: text/plain; charset=utf-8
Content-Length: 9
Connection: keep-alive
Vary: Origin
Forbidden
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.
@bsardo The endpoint is only accessible by calling it with a valid API Key. Example:
curl --location 'https://bid.staging.missena.xyz/?t=PA-34761163' \
--header 'Accept: */*' \
--header 'Accept-Language: en-GB,en-US;q=0.9,en;q=0.8' \
--header 'Cache-Control: no-cache' \
--header 'Connection: keep-alive' \
--header 'Origin: https://publisher.staging.missena.click' \
--header 'Pragma: no-cache' \
--header 'Referer: https://publisher.staging.missena.click/' \
--header 'Sec-Browsing-Topics: ();p=P0000000000000000000000000000000' \
--header 'Sec-Fetch-Dest: empty' \
--header 'Sec-Fetch-Mode: cors' \
--header 'Sec-Fetch-Site: cross-site' \
--header 'User-Agent: Mozilla/5.0 (iPhone; CPU iPhone OS 16_6 like Mac OS X) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/16.6 Mobile/15E148 Safari/604.1' \
--header 'content-type: text/plain' \
--data '{
"adunit": "msna-sticky-dd",
"request_id": "53dfce8e0aad03",
"test": "smart-banner",
"placement": "sticky"
}'
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.
All good @ysfbsf. All I'm doing here is proving that I can reach your endpoint and sharing that proof with other reviewers. No action required.
endpoint: https://bid.missena.io/ | ||
maintainer: | ||
email: yboussafa@missena.com | ||
gvlVendorID: 687 |
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.
verified GVL ID:
curl https://vendor-list.consensu.org/v3/vendor-list.json | jq '.vendors."687"'
% Total % Received % Xferd Average Speed Time Time Time Current
Dload Upload Total Spent Left Speed
100 628k 100 628k 0 0 3635k 0 --:--:-- --:--:-- --:--:-- 3655k
{
"id": 687,
"name": "MISSENA",
"purposes": [
1,
2,
3,
4,
5,
6,
7
],
"legIntPurposes": [],
"flexiblePurposes": [],
"specialPurposes": [
2
],
"features": [
2,
3
],
"specialFeatures": [],
"cookieMaxAgeSeconds": 31104000,
"usesCookies": true,
"cookieRefresh": true,
"usesNonCookieAccess": true,
"dataRetention": {
"stdRetention": 360,
"purposes": {},
"specialPurposes": {}
},
"urls": [
{
"langId": "en",
"privacy": "https://missena.com/privacy",
"legIntClaim": "https://missena.com/privacy"
}
],
"dataDeclaration": [
1,
2,
3,
4,
5,
6,
7,
8,
10
],
"deviceStorageDisclosureUrl": "https://ad.missena.io/iab.json"
}
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.
@bsardo Do you mean that no need to provide the GVL ID since it's a verified one?
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.
Sorry for the confusion @ysfbsf. No action is required here; you should continue to specify the GVL ID. My comment is just proof to other reviewers that I verified the GVL ID you specified is associated with your bidder.
adapters/missena/params_test.go
Outdated
var invalidParams = []string{ | ||
`{"apiKey": 42}`, | ||
`{"placement": 111}`, | ||
} |
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.
I suggest adding the following test cases:
placement
is valid butapiKey
is omitted since it is requiredapiKey
is valid butplacement
is not
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.
Done ✅
adapters/missena/missena_test.go
Outdated
|
||
func TestJsonSamples(t *testing.T) { | ||
bidder, buildErr := Builder(openrtb_ext.BidderMissena, config.Adapter{ | ||
Endpoint: "https://bid.missena.io"}, |
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.
I recommend using a fake URL for your endpoint to minimize maintenance should you ever change your production endpoint.
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.
Done ✅
adapters/missena/missena.go
Outdated
var tempErrors []error | ||
gdprApplies, consentString := readGDPR(request) | ||
|
||
var missenaInternalParams MissenaInternalParams |
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.
Nitpick: you could combine this with the gdpr field assignments on lines 144-145 as:
missenaInternalParams := MissenaInternalParams{
GDPR: gdprApplies,
GDPRConsent: consentString,
}
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.
Done ✅
adapters/missena/missena.go
Outdated
} else if newHttpRequest != nil { | ||
httpRequests = append(httpRequests, newHttpRequest) | ||
} | ||
|
||
return httpRequests, finalErrors |
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.
Do you need the else if
? You could probably just do:
httpRequests = append(httpRequests, newHttpRequest)
return httpRequests, errors
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.
Done ✅
adapters/missena/missena.go
Outdated
var errors []error | ||
return bidResponse, errors |
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.
Simplify to return bidResponse, nil
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.
Done ✅
|
||
var missenaInternalParams MissenaInternalParams | ||
|
||
for _, imp := range request.Imp { |
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.
It appears you are prepared to handle multiple imps but really you only care that at least one of them is valid as you're just preparing a single request without specifying the impression ID, though you are grabbing the params from the last imp you process. Is this what you want or do you want to grab the info from the first imp?
I suggest adding a test where there are multiple imps with different params, specifically placement
. I also think you should add a test where one imp results in an error and the other is well formed.
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.
Done ✅
adapters/missena/missena.go
Outdated
Uri: url, | ||
Headers: headers, | ||
Body: body, | ||
ImpIDs: openrtb_ext.GetImpIDs(request.Imp), |
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.
If you're only sending a bid request to your server for one imp, that imp is the one that should be specified here rather than specifying all of the imps.
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.
Done ✅
Code coverage summaryNote:
missenaRefer here for heat map coverage report
|
@@ -0,0 +1,16 @@ | |||
endpoint: https://bid.missena.io/ | |||
maintainer: | |||
email: prebid@missena.com |
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.
We've sent a test email to prebid@missena.com. Please respond to the email with a message of "received".
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.
@bsardo Can you send the email again?
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.
@ysfbsf Sent
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.
Confirmed
userSync: | ||
redirect: | ||
url: https://sync.missena.io/iframe?gdpr={{.GDPR}}&consent={{.GDPRConsent}}&us_privacy={{.USPrivacy}}&redirect={{.RedirectURL}} | ||
userMacro: $UID |
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.
Is this working correctly? I didn't see the redirect to the PBS setuid endpoint with the missena user ID so that it can be set on the PBS cookie.
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.
This does not appear to be working. Two things:
- I see
iframe
in your URL which leads me to believe your user sync is of type iframe instead of redirect. If that's the case, you should indicate as such:
userSync:
iframe:
url: ...
userMacro: $UID
- When I initiate a user sync I do not see the redirect to the PBS setuid endpoint, which will be specified in the redirect portion of your user sync query string (
redirect={{.RedirectURL}}
). This is where your user ID is set on the PBS cookie.
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.
Please move this test under the supplemental folder or rename to simple-banner-ipv6.json
.
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.
You might want to consider adding a third imp to this test so that two imps are valid and one is invalid. That would make the intended behavior clear for when there are multiple valid imps.
adapters/missena/missena.go
Outdated
if len(httpRequests) == 0 && len(errors) == 0 { | ||
errors = append(errors, &errortypes.BadInput{ | ||
Message: "No valid impressions found", | ||
}) | ||
} |
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.
I think you can delete this if
block. The only time this condition would be true is if the bid request does not contain any imps but the code upstream will validate that there is at least one impression before calling your adapter.
adapters/missena/missena.go
Outdated
return &missenaRequest | ||
} | ||
|
||
func (a *adapter) makeRequest(missenaParams MissenaInternalParams, reqInfo *adapters.ExtraRequestInfo, imp *openrtb2.Imp, request *openrtb2.BidRequest) (*adapters.RequestData, error) { |
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.
Nitpick: instead of passing a pointer to the imp, I suggest just passing the imp ID as a string to keep things simple since that's the only field you're using on the impression.
adapters/missena/missena.go
Outdated
func (a *adapter) makeRequest(missenaParams MissenaInternalParams, reqInfo *adapters.ExtraRequestInfo, imp *openrtb2.Imp, request *openrtb2.BidRequest) (*adapters.RequestData, error) { | ||
|
||
url := a.endpoint + "?t=" + missenaParams.ApiKey | ||
parameter := a.makeParameter(missenaParams, request) |
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.
You could simplify your adapter a bit eliminating a function call by deleting the makeParameter
function and moving that function's contents in line so that you construct the MissenaAdRequest
instance here.
adapters/missena/missena.go
Outdated
if request.Device != nil { | ||
headers.Add("User-Agent", request.Device.UA) | ||
} | ||
|
||
if request.Device != nil { |
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.
Nitpick: you can simplify your logic by getting rid of the first device nil check and instead moving headers.Add("User-Agent", request.Device.UA)
into the second device nil check.
adapters/missena/missena.go
Outdated
} | ||
|
||
func (a *adapter) MakeRequests(request *openrtb2.BidRequest, requestInfo *adapters.ExtraRequestInfo) ([]*adapters.RequestData, []error) { | ||
// print the request |
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.
Delete comment
adapters/missena/missena.go
Outdated
consentString = extUser.Consent | ||
} | ||
} | ||
gdprApplies := true |
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.
Just checking, are you sure you always want to default to gdpr applies if no gdpr signal has been provided? Perhaps a supplemental test should be added for when gdpr is not set.
Code coverage summaryNote:
missenaRefer here for heat map coverage report
|
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.
LGTM except for the user sync.
userSync: | ||
redirect: | ||
url: https://sync.missena.io/iframe?gdpr={{.GDPR}}&consent={{.GDPRConsent}}&us_privacy={{.USPrivacy}}&redirect={{.RedirectURL}} | ||
userMacro: $UID |
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.
This does not appear to be working. Two things:
- I see
iframe
in your URL which leads me to believe your user sync is of type iframe instead of redirect. If that's the case, you should indicate as such:
userSync:
iframe:
url: ...
userMacro: $UID
- When I initiate a user sync I do not see the redirect to the PBS setuid endpoint, which will be specified in the redirect portion of your user sync query string (
redirect={{.RedirectURL}}
). This is where your user ID is set on the PBS cookie.
Related Documentation: prebid/prebid.github.io#5437