fix two issues related to hb_uuid and hb_cache_id targeting keys #3605
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
| @@ -386,6 +386,10 @@ export function doCallbacksIfTimedout(auctionInstance, bidResponse) { | |||
|
|
|||
| // Add a bid to the auction. | |||
| export function addBidToAuction(auctionInstance, bidResponse) { | |||
| let bidderRequests = auctionInstance.getBidRequests(); | |||
| let bidderRequest = find(bidderRequests, bidderRequest => bidderRequest.bidderCode === bidResponse.bidderCode); | |||
| setupBidTargeting(bidResponse, bidderRequest); | |||
There was a problem hiding this comment.
Why are we calling this function for all formats ? We need hb_uuid and hb_cache_id only when cache is done i.e only for video as of now. Won't it be easier if we attach after hook function to callPrebidCache. That way these two targeting keys will only be added if we do caching.
There was a problem hiding this comment.
Because it's not exclusively used by the cache related keys; it sets up all the targeting for the bid object.
jaiminpanchal27
approved these changes
Mar 1, 2019
jaiminpanchal27
added
LGTM
needs 2nd review
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.
Type of change
NOTE This is the re-submission of #3568 (which was reverted due to a last minute code conflict that came when #3573 was merged to
master).Description of change
This PR incorporates some fixes related to the
hb_uuidandhb_cache_idtargeting keys.Some issues that were fixed:
due to the order in which the code was executed, the
bid.adserverTargeting.hb_uuidandbid.adserverTargeting.hb_cache_idfields weren't properly populated with the bid'svideoCacheKeyafter it came back from Prebid Cache. This caused other functions that pulled the targeting keys (likepbjs.getAdserverTargetingForAdUnitCode()) to be missing this information when it was otherwise expected to be there.a consequence to the above issue, when the publisher tried to overwrite the values for these keys in the
pbjs.bidderSettingsfeature - the overwrites wouldn't work because thebid.videoCacheKeywasn't populated at the time thesebidderSettingsvalues were used.The changes in this PR addresses the above issues through mainly moving the code around to populate the
bid.adserverTargetingobject once we know the bid is complete (ie back from prebid cache for that use-case).Note - removed the
dfpAdServerVideocode since it's now redundant.