New Adapter: QT #3696
New Adapter: QT #3696
Conversation
Code coverage summaryNote:
qtRefer here for heat map coverage report |
Code coverage summaryNote:
qtRefer here for heat map coverage report |
|
@onkarvhanumante can you please check again? |
adapters/qt/qt.go
Outdated
| if len(adapterRequests) == 0 { | ||
| errs = append(errs, errors.New("found no valid impressions")) | ||
| return nil, errs | ||
| } | ||
|
|
||
| return adapterRequests, nil |
There was a problem hiding this comment.
These could be combined as - return adapterRequests, append(errs, errors.New("found no valid impressions"))
There was a problem hiding this comment.
@qt-io please update on the comment above.
There was a problem hiding this comment.
@qt-io Gentle reminder.
There was a problem hiding this comment.
@gargcreation1992 but this code will append error "found no valid impressions" even if len(adapterRequests) > 0
So, what's the purpose of that? Perhaps I'm missing something.
Maybe better to do something like this:
if len(adapterRequests) == 0 {
errs = append(errs, errors.New("found no valid impressions"))
}
return adapterRequests, errs
?
There was a problem hiding this comment.
^ Looks good to me. These are equivalent since adapterRequests will be nil if never appended.
Not sure what value a "found no valid impressions" error provides since there must be at least one error for this condition to be hit.
adapters/qt/qt.go
Outdated
| if len(adapterRequests) == 0 { | ||
| errs = append(errs, errors.New("found no valid impressions")) | ||
| return nil, errs | ||
| } | ||
|
|
||
| return adapterRequests, nil |
There was a problem hiding this comment.
^ Looks good to me. These are equivalent since adapterRequests will be nil if never appended.
Not sure what value a "found no valid impressions" error provides since there must be at least one error for this condition to be hit.
adapters/qt/qt_test.go
Outdated
|
|
||
| func TestJsonSamples(t *testing.T) { | ||
| bidder, buildErr := Builder(openrtb_ext.BidderQT, config.Adapter{ | ||
| Endpoint: "https://endpoint1.qt.io/pserver"}, config.Server{ExternalUrl: "http://hosturl.com", GvlID: 1, DataCenter: "2"}) |
There was a problem hiding this comment.
Nitpick: It's not necessary to use a real url for tests. Might be easier to use an obviously fake value so there's no pressure to keep it updated.
|
@SyntaxNode @gargcreation1992 @onkarvhanumante
|
Code coverage summaryNote:
qtRefer here for heat map coverage report |
Yes. This needs to be a group email address and we need to receive a reply from a test email to approve.
I see only the |
|
@SyntaxNode email changed |
| @@ -0,0 +1,18 @@ | |||
| endpoint: "https://endpoint1.qt.io/pserver" | |||
| maintainer: | |||
| email: "[email protected]" | |||
There was a problem hiding this comment.
Prebid team has sent email to verify above maintainers email. Requesting to responding back on email thread
There was a problem hiding this comment.
received response on email thread
| url: "https://cs.qt.io/pbserver?gdpr={{.GDPR}}&gdpr_consent={{.GDPRConsent}}&us_privacy={{.USPrivacy}}&gpp={{.GPP}}&gpp_sid={{.GPPSID}}&redir={{.RedirectURL}}" | ||
| userMacro: "[UID]" |
There was a problem hiding this comment.
redirect url is working
https://cs.qt.io/pbserver?gdpr=&gdpr_consent=&us_privacy=&gpp=&gpp_sid=&redir=https%3A%2F%2Fprebid.adnxs.com%2Fpbs%2Fv1%2Fsetuid%3Fbidder%3DQT%26gdpr%3D%26gdpr_consent%3D%26f%3Di%26uid%3D%22%5BUID%5D%22
There was a problem hiding this comment.
@qt-io requesting to address https://github.com/prebid/prebid-server/pull/3696/files#r1660622081
|
@onkarvhanumante The email has been responded to |
There was a problem hiding this comment.
@qt-io The code looks good. I left two comments.
Could you also please acknowledge these two outstanding comments?
#3696 (comment)
| "httpCalls": [ | ||
| { | ||
| "expectedRequest": { | ||
| "uri": "https://endpoint1.qt.io/pserver", |
There was a problem hiding this comment.
I suggest using a fake url in all of your tests instead of a real url.
| } | ||
| }, | ||
| { | ||
| "id": "test-imp-id", |
There was a problem hiding this comment.
I suggest making this imp ID different from the first imp so you can be sure the adapter is working properly.
Code coverage summaryNote:
qtRefer here for heat map coverage report |
|
@bsardo @onkarvhanumante improved multi-imp test and used fake URLs in tests. I also removed 'no valid impression' error from code. Can this PR be merged now? |
Code coverage summaryNote:
qtRefer here for heat map coverage report |
DOC - prebid/prebid.github.io#5331