New Adapter: Intertech #3718
Open
New Adapter: Intertech #3718
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
SyntaxNode
reviewed
Jun 5, 2024
|
You also need to register the intertech name and adapter in a few other places. Please see this section in the docs or look at this other PR for an example. |
Code coverage summaryNote:
intertechRefer here for heat map coverage report |
bsardo
reviewed
Jun 13, 2024
Code coverage summaryNote:
intertechRefer here for heat map coverage report |
Code coverage summaryNote:
intertechRefer here for heat map coverage report |
|
@przemkaczmarek, can you resolve conflicts? Thanks! |
Code coverage summaryNote:
intertechRefer here for heat map coverage report |
Code coverage summaryNote:
intertechRefer here for heat map coverage report |
Code coverage summaryNote:
intertechRefer here for heat map coverage report |
SyntaxNode
reviewed
Nov 15, 2024
SyntaxNode
reviewed
Nov 15, 2024
Code coverage summaryNote:
intertechRefer here for heat map coverage report |
Code coverage summaryNote:
intertechRefer here for heat map coverage report |
bsardo
reviewed
Dec 9, 2024
static/bidder-info/intertech.yaml
Outdated
| - native | ||
| userSync: | ||
| redirect: | ||
| url: https://prebid.intertechsrvcs.com/mapuid/intertech/?ssp-id=10500&gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}} |
There was a problem hiding this comment.
The Java version has &location= at the end of the URL. We'll need to figure out if that's needed.
We'll also need to verify that this redirect works. It wasn't working for me.
adapters/intertech/intertech.go
Outdated
Comment on lines
108
to
109
| headers.Add("Content-Type", "application/json;charset=utf-8") | ||
| headers.Add("Accept", "application/json") |
There was a problem hiding this comment.
Since these don't involve request.Device, I suggest moving these outside of the conditional so that they are always set.
adapters/intertech/intertech.go
Outdated
Comment on lines
319
to
325
| if imp.Banner != nil { | ||
| return openrtb_ext.BidTypeBanner, nil | ||
| } | ||
|
|
||
| return "", &errortypes.BadInput{ | ||
| Message: fmt.Sprintf("Processing an invalid impression; cannot resolve impression type for imp #%s", imp.ID), | ||
| } |
There was a problem hiding this comment.
Given that this adapter only supports banner and native, the bid type is derived from the impression and PBS core will only call the adapter if an impression is one of the supported formats, this error case should not be possible. This function could be simplified as follows:
if imp.Native != nil {
return openrtb_ext.BidTypeNative, nil
}
return openrtb_ext.BidTypeBanner, nil
Code coverage summaryNote:
intertechRefer here for heat map coverage report |
bsardo
reviewed
Dec 11, 2024
Comment on lines
+118
to
+120
| if banner == nil { | ||
| return nil, fmt.Errorf("banner is null") | ||
| } |
There was a problem hiding this comment.
You can delete this conditional as banner will always be not nil since this function call is only made if banner is not nil.
Comment on lines
+109
to
+114
| if imp.Native != nil { | ||
| return imp, nil | ||
| } | ||
| return openrtb2.Imp{}, &errortypes.BadInput{ | ||
| Message: fmt.Sprintf("Intertech only supports banner and native types. Ignoring imp id=%s", imp.ID), | ||
| } |
There was a problem hiding this comment.
You can simplify this to just return imp, nil since you've only declared support for banner and native in the YAML file.
| headers.Add("X-Real-Ip", bidRequest.Device.IP) | ||
| } | ||
| if bidRequest.Device.Language != "" { | ||
| headers.Add("Accept-Language", bidRequest.Device.Language) |
There was a problem hiding this comment.
Can you add JSON test coverage for this line?
| return &bannerCopy, nil | ||
| } | ||
|
|
||
| func (a *adapter) modifyUrl(extImp ExtImpIntertech, referer, cur string) (string, error) { |
There was a problem hiding this comment.
You can delete the second return value error because it is only ever nil.
Comment on lines
+90
to
+92
| return ExtImpIntertech{}, &errortypes.BadInput{ | ||
| Message: fmt.Sprintf("imp #%s: unable to parse intertech ext: %s", imp.ID, err), | ||
| } |
There was a problem hiding this comment.
Please add test coverage for these lines. You can force this error by setting page_id or imp_id to an invalid type (e.g. string) which would be something other than an integer.
bsardo
reviewed
Dec 11, 2024
There was a problem hiding this comment.
Can you move this test into the supplemental directory?
There was a problem hiding this comment.
Can you move this test into the supplemental directory?
Comment on lines
+15
to
+19
| "banner": { | ||
| "format": [ | ||
| { "w": 300, "h": 250 } | ||
| ] | ||
| }, |
There was a problem hiding this comment.
Since you have simple-banner-multi-format.json which tests when format is populated, I suggest you make this test cover the case when there is no format but there are top-level dimensions:
"banner": {
"w": 300,
"h": 250
},
Comment on lines
+3
to
+14
| "id": "test-missing-dimensions-request-id", | ||
| "device": { | ||
| "ip": "123.123.123.123", | ||
| "ua": "Mozilla/5.0 (X11; Linux x86_64)" | ||
| }, | ||
| "site": { | ||
| "page": "http://bannercheck.com" | ||
| }, | ||
| "imp": [ | ||
| { | ||
| "id": "test-imp-missing-dim", | ||
| "tagid": "tag-missing-dim", |
There was a problem hiding this comment.
Nitpick: I suggest simplifying the names of the ids here. Just use request-id-1, imp-id-1, tag-id-1. The other names can be confusing. The test file name does a good job of indicating the test purpose.
| ], | ||
| "cur": ["USD"] | ||
| }, | ||
| "httpCalls": [ |
There was a problem hiding this comment.
You can verify the headers, specifically the device related ones, are set correctly given that you have a device object here by including the following:
"httpCalls": [
{
"expectedRequest": {
"headers": {
"Content-Type": [
"application/json;charset=utf-8"
],
"Accept": [
"application/json"
],
etc...
``
There was a problem hiding this comment.
You can delete this test. Only banner and native media type requests will result in calling this adapter.
There was a problem hiding this comment.
I suggest adding a new JSON test in the supplemental directory where the incoming request does not have a device object and then verify that the httpCalls[].expectedRequest.headers does not contain the device related headers.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.