Nextmillennium: New fields and adapter version update #4246
Conversation
| NextMillenniumExtBidder.of( | ||
| nmmFlags, extImp.getAdSlots(), extImp.getAllowedAds(), null, null)); |
There was a problem hiding this comment.
If I understand the changes in Go correctly adslots and allowedads shouldn't be added to the imp.ext
There was a problem hiding this comment.
and please, create a separate static constructor of with the only nmmFlags parameter
There was a problem hiding this comment.
according to the json's samples:
"mockBidRequest": {
"id": "testid",
"imp": [
{
"id": "123654",
"banner": {
"w": 320,
"h": 250
},
"ext": {
"bidder": {
"group_id": "7819",
"allowedAds":["allowed_ad_1, allowed_ad_2"]
}
}
}
],
"app": {
"domain": "www.example.com"
}
},
"httpCalls": [
{
"expectedRequest": {
"uri": "https://pbs.nextmillmedia.com/openrtb2/auction",
"body":{
"id": "testid",
"app": {
"domain": "www.example.com"
},
"ext": {
"nextMillennium": {
"allowedAds":["allowed_ad_1, allowed_ad_2"],
"nm_version":"v1.0.1"
},
```
----------------------------------------------------------
```
type nmExtNMM struct {
NmmFlags []string `json:"nmmFlags,omitempty"`
AdSlots []string `json:"adSlots,omitempty"`
AllowedAds []string `json:"allowedAds,omitempty"`
ServerVersion string `json:"server_version,omitempty"`
AdapterVersion string `json:"nm_version,omitempty"`
}
AdSlots and AllowedAds are parts of nmmExt:
There was a problem hiding this comment.
it should be included in the ext.nextMillennium, but not in the imp[].ext.nextMillennium (compare the payload you provided and the payload of the integration tests you've added)
| nmmFlags, extImp.getAdSlots(), extImp.getAllowedAds(), NM_ADAPTER_VERSION, | ||
| versionProvider.getNameVersionRecord())); |
There was a problem hiding this comment.
please chop the whole parameter list down
There was a problem hiding this comment.
Do you mean listing them one per line or removing them?
There was a problem hiding this comment.
one per line, please
| request -> request.app(App.builder().domain("appDomain").build()), | ||
| givenImpWithExt(identity(), ExtImpNextMillennium.of(null, "group1")), | ||
| givenImpWithExt(identity(), ExtImpNextMillennium.of(null, "group2"))); | ||
| givenImpWithExt(identity(), ExtImpNextMillennium.of(null, "group1", null, null)), |
There was a problem hiding this comment.
I'd prefer extracting ExtImpNextMillennium.of(null, "group1", null, null) and ExtImpNextMillennium.of("placement1", "group1", null, null) into separate methods in order not to pass nulls everywhere
| final ObjectNode requestExtNode = mapper.valueToTree(actualRequest.getExt()); | ||
| assertThat(requestExtNode.at("/nextMillennium/adSlots/0").asText()).isEqualTo("slot1"); | ||
| assertThat(requestExtNode.at("/nextMillennium/adSlots/1").asText()).isEqualTo("slot2"); | ||
| assertThat(requestExtNode.at("/nextMillennium/allowedAds/0").asText()).isEqualTo("ad1"); | ||
| assertThat(requestExtNode.at("/nextMillennium/allowedAds/1").asText()).isEqualTo("ad2"); | ||
|
|
||
| final ObjectNode impExtNode = (ObjectNode) actualRequest.getImp().getFirst().getExt(); | ||
| assertThat(impExtNode.at("/nextMillennium/adSlots/0").asText()).isEqualTo("slot1"); | ||
| assertThat(impExtNode.at("/nextMillennium/adSlots/1").asText()).isEqualTo("slot2"); | ||
| assertThat(impExtNode.at("/nextMillennium/allowedAds/0").asText()).isEqualTo("ad1"); | ||
| assertThat(impExtNode.at("/nextMillennium/allowedAds/1").asText()).isEqualTo("ad2"); |
There was a problem hiding this comment.
I'd prefer using extracting approach like everywhere else in the test
|
|
||
| String serverVersion; | ||
|
|
||
| public static NextMillenniumExtBidder ofNmmFlags(List<String> nmmFlags) { |
|
|
||
| return bidRequest.toBuilder() | ||
| .imp(modifyFirstImp(bidRequest.getImp(), soredRequestId, extImp)) | ||
| .imp(modifyFirstImp(bidRequest.getImp(), soredRequestId)) |
There was a problem hiding this comment.
a typo in soredRequestId
| nmmFlags, extImp.getAdSlots(), extImp.getAllowedAds(), NM_ADAPTER_VERSION, | ||
| versionProvider.getNameVersionRecord())); |
CTMBNara
deleted the
Nextmillennium-New-fields-and-adapter-version-update
branch
🔧 Type of changes
✨ What's the context?
#4234