Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9c2695fb99
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
src/targeting.ts
Outdated
| const adUnitIsEligible = adUnitCodes.includes(bid.adUnitCode); | ||
| const cpmAllowed = bidderSettings.get(bid.bidderCode, 'allowZeroCpmBids') === true ? bid.cpm >= 0 : bid.cpm > 0; | ||
| const isPreferredDeal = alwaysIncludeDeals && bid.dealId; | ||
| const notExcludedByConfig = bidTargetingExclusion?.(bid) !== false; |
There was a problem hiding this comment.
Validate bidTargetingExclusion is callable before invoking
bidTargetingExclusion?.(bid) still throws when the config value is non-nullish but not a function (for example true or {}), because optional-call only short-circuits null/undefined. Since setConfig accepts arbitrary values, a malformed publisher config can now raise a TypeError inside targeting generation and prevent ad-server targeting from being produced for that page; add a typeof bidTargetingExclusion === 'function' guard (or equivalent config validation) before calling it.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
lets add the type check and also run the three filters and pass the remaining bids as a second argumernt to the exclusion function so the function can do comparisons of various bids
Pull Request Test Coverage Report for Build 22059275916Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
Type of change
Description of change
Bid targeting exclusion (
bidTargetingExclusion)Summary
Adds a new config option
bidTargetingExclusionthat lets you control which bids are included in ad server targeting (e.g.setTargetingForGPT). You can exclude specific bids from targeting (e.g. ineligible or unwanted bids).Configuration
Other information
#12399