-
Notifications
You must be signed in to change notification settings - Fork 732
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
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
|
return nil, []error{&errortypes.BadInput{ | ||
Message: fmt.Sprintf("Error extracting Ext: %s", err.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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
LegalName
as typestring
and change&ad.Advertiser.LegalName != nil
toad.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
LegalName
to type*string
and change&ad.Advertiser.LegalName != nil
toad.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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please remove copy
from the name of this file.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nitpick: you can get rid of the else
and just have return nil, 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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.