Adnuntius: Return DSA in bid response #3777
Conversation
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report |
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report |
bsardo
changed the title
| return nil, []error{&errortypes.BadInput{ | ||
| Message: fmt.Sprintf("Error extracting Ext: %s", err.Error()), | ||
| }} | ||
| } |
There was a problem hiding this comment.
please add test for this. Not covered
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report |
|
hi, has anyone looked at my comment? |
Hi @mikael-lundin, I don't see any comments from you nor do I see any updates other than merging with master. Please see the two comments from @przemkaczmarek. I'll give this another look once you've addressed those. Thanks! |
|
Strange. :) my question was if I needed to test the error or the function? Because the function should be covered. :) |
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report |
|
@mikael-lundin You added a master to this PR. This is incorrect because we cannot track the changes. please undo this comit |
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report |
|
Sorry about that, should be cleaned up now. |
There was a problem hiding this comment.
It appears you added this test to verify an error occurs during MakeBids when looking for the DSA object in the response, however, this test is actually forcing an error in MakeRequests when looking for the regs.ext.gdpr signal. I think this test is worth keeping; I would just change its name to something like invalid-regs-ext.json or regs-ext-error.json.
I don't think you'll be able to test the new MakeBids regs.ext unmarshal error code paths because that would involve setting regs.ext to something invalid but doing that will cause an error in MakeRequests before the MakeBids logic runs since regs.ext is unmarshaled in both functions and MakeRequests runs first.
| }, | ||
| "expectedMakeRequestsErrors": [ | ||
| { | ||
| "value": "failed to parse URL: [failed to parse Adnuntius endpoint: failed to parse ExtRegs in Adnuntius GDPR check: json: cannot unmarshal string into Go value of type openrtb_ext.ExtRegs]", |
There was a problem hiding this comment.
I know this is not new logic but I just want to point out that this error is a little misleading since the error does not appear to be due to parsing a URL or the endpoint but rather due to a failure parsing the ExtRegs while attempting to generate the URL. Perhaps you want to address this in a future PR?
adapters/adnuntius/adnuntius.go
Outdated
| } | ||
| } | ||
|
|
||
| if requestRegsExt != nil && requestRegsExt.DSA != nil && requestRegsExt.DSA.PubRender != &adRender { |
There was a problem hiding this comment.
The comparison requestRegsExt.DSA.PubRender != &adRender will always be not equal since you're comparing memory addresses and the memory address of requestRegsExt.DSA.PubRender will never be the same as adRender.
I suggest adding another JSON test called check-dsa-pubrender-zero.json and renaming your test check-dsa.json to check-dsa-pubrender-non-zero.json.
Then I think you want to change this conditional to the following so that you are comparing the values instead of the memory addresses:
if requestRegsExt != nil && requestRegsExt.DSA != nil && requestRegsExt.DSA.PubRender != nil && *requestRegsExt.DSA.PubRender != adRender
Also, what happens if regs.ext.dsa.pubrender is nil or if regs.ext.dsa.pubrender is 0? You don't want the dsa object included in the bid response in these cases?
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report |
adapters/adnuntius/adnuntius.go
Outdated
| } | ||
| } | ||
|
|
||
| if requestRegsExt != nil && requestRegsExt.DSA != nil && &ad.Advertiser != nil && &ad.Advertiser.LegalName != nil { |
There was a problem hiding this comment.
It looks like you're now trying to detect presence of LegalName as part of this conditional, in which case either of these would be considered a preferred approach:
- if you don't care to differentiate between presence and an empty string meaning you want to create a DSA if legal name doesn't exist as well as when legal name has been provided but is an empty string, keep
LegalNameas typestringand change&ad.Advertiser.LegalName != niltoad.Advertiser.LegalName != "" - if you do care to differentiate between presence and an empty string where you want to create a DSA only if legal name exists including the empty string case, change
LegalNameto type*stringand change&ad.Advertiser.LegalName != niltoad.Advertiser.LegalName != nil
adapters/adnuntius/adnuntius.go
Outdated
| } | ||
| } | ||
|
|
||
| if requestRegsExt != nil && requestRegsExt.DSA != nil && &ad.Advertiser != nil && &ad.Advertiser.LegalName != nil { |
There was a problem hiding this comment.
You shouldn't need to check if ad.Advertiser is present via && &ad.Advertiser != nil since Advertiser will always exist on the Ad struct since it isn't defined as a pointer:
type Ad struct {
...
DestinationUrls map[string]string
Advertiser adnAdvertiser
}
In the case where it isn't defined in the response from your server, when it is mapped to your structs, the Advertiser field will exist as a zero-value struct.
There was a problem hiding this comment.
Please remove copy from the name of this file.
There was a problem hiding this comment.
Since you've changed the behavior that determines whether to create a DSA from being dependent on the value of pubrender to being dependent on the value of Advertiser.LegalName, I don't think these check-dsa-pubrender tests are serving much of a purpose. Instead you should probably add the following tests:
- ad.advertiser is not provided
- ad.advertiser is provided with legal name missing
- ad.advertiser is provided with legal name as empty string
- ad.advertiser is provided with legal name not empty
There was a problem hiding this comment.
Please see above comment regarding tests. Your code isn't doing anything with pubrender anymore other than hardcoding the AdRender field to zero. Consider getting rid of the multiple pubrender tests and instead adding the test cases I outlined above so that you're exercising the various advertiser name and legal name scenarios.
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report |
|
Hi has anyone had time to look at my changes? |
adapters/adnuntius/adnuntius.go
Outdated
| } else { | ||
| return nil, nil | ||
| } |
There was a problem hiding this comment.
Nitpick: you can get rid of the else and just have return nil, nil
There was a problem hiding this comment.
Please see above comment regarding tests. Your code isn't doing anything with pubrender anymore other than hardcoding the AdRender field to zero. Consider getting rid of the multiple pubrender tests and instead adding the test cases I outlined above so that you're exercising the various advertiser name and legal name scenarios.
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report |
|
Hi, so now I added some tests for advertiser instead, to check for the three things that can happen in our ad server. Thank you all for your guidance and great help. :) |
There was a problem hiding this comment.
Nitpick: remove copy from the file name renaming it to check-dsa-advertiser-legalName.json
| @@ -387,7 +387,7 @@ func generateReturnExt(ad Ad, request *openrtb2.BidRequest) (json.RawMessage, er | |||
| } | |||
| } | |||
|
|
|||
| if requestRegsExt != nil && requestRegsExt.DSA != nil { | |||
| if ad.Advertiser.Name != "" && requestRegsExt != nil && requestRegsExt.DSA != nil { | |||
There was a problem hiding this comment.
Just want to confirm that Name is required to add the DSA object such that if LegalName is present but Name is not, you don't want to add the DSA object. Is that correct?
Code coverage summaryNote:
adnuntiusRefer here for heat map coverage report |
This enables DSA for our adapter. it reads the ORTB, see if DSA is applicable and then return the DSA information. we start with advertiser name and move onto more information if needed from our clients.