Skip to content
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

Test: suppressIBV in response correction module #3789

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

marki1an
Copy link
Collaborator

@marki1an marki1an commented Feb 26, 2025

ibv response correction

🔧 Type of changes

  • new bid adapter
  • bid adapter update
  • new feature
  • new analytics adapter
  • new module
  • module update
  • bugfix
  • documentation
  • configuration
  • dependency update
  • tech debt (test coverage, refactorings, etc.)

✨ What's the context?

What's the context for the changes?

🧠 Rationale behind the change

Why did you choose to make these changes? Were there any trade-offs you had to consider?

🔎 New Bid Adapter Checklist

  • verify email contact works
  • NO fully dynamic hostnames
  • geographic host parameters are NOT required
  • direct use of HTTP is prohibited - implement an existing Bidder interface that will do all the job
  • if the ORTB is just forwarded to the endpoint, use the generic adapter - define the new adapter as the alias of the generic adapter
  • cover an adapter configuration with an integration test

🧪 Test plan

How do you know the changes are safe to ship to production?

🏎 Quality check

  • Are your changes following our code style guidelines?
  • Are there any breaking changes in your code?
  • Does your test coverage exceed 90%?
  • Are there any erroneous console logs, debuggers or leftover code in your changes?

`ibv response correction`
@marki1an marki1an added the tests Functional or other tests label Feb 26, 2025
@marki1an marki1an self-assigned this Feb 26, 2025
Comment on lines +26 to +28

RESPONSE_REJECTED_DUE_TO_IN_BANNER_VIDEO(353),

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it is a part of response_rejection group, no need of extra space

import static org.prebid.server.functional.model.request.auction.BidRequest.getDefaultBidRequest
import static org.prebid.server.functional.model.request.auction.BidRequest.getDefaultVideoRequest
import static org.prebid.server.functional.model.request.auction.DistributionChannel.APP
import static org.prebid.server.functional.model.request.auction.DistributionChannel.DOOH
import static org.prebid.server.functional.model.request.auction.DistributionChannel.SITE
import static org.prebid.server.functional.model.response.auction.BidRejectionReason.*
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wildcard import

@@ -439,7 +443,7 @@ class ResponseCorrectionSpec extends ModuleBaseSpec {
bidder.setResponse(bidRequest.id, bidResponse)

and: "Save account with enabled response correction module"
def accountWithResponseCorrectionModule = accountConfigWithResponseCorrectionModule(bidRequest)
def accountWithResponseCorrectionModule = accountConfigWithResponseCorrectionModuleAndAppVideoHtml(bidRequest.getAccountId())
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could be:
accountConfigWithResponseCorrectionModuleAndAppVideoHtml(bidRequest.accountId)
same for others

private static Account accountConfigWithResponseCorrectionModule(BidRequest bidRequest, Boolean enabledResponseCorrection = true, Boolean enabledAppVideoHtml = true) {
def modulesConfig = new PbsModulesConfig(pbResponseCorrection: new PbResponseCorrection(
enabled: enabledResponseCorrection, appVideoHtml: new AppVideoHtml(enabled: enabledAppVideoHtml)))
def "PBS shouldn't reject auction with 353 code when enabled response correction is #enabledResponseCorrection and enabled suppress ibv is #enabledSuppressIbv"() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PBS should process request without any errors and warning when response correction is #enabledResponseCorrection and suppress ibv is #enabledSuppressIbv

false | false
}

def "PBS shouldn't reject auction with 353 code when enabled suppress ibv and bid request with imp media type is #mediaType"() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PBS should process request without any errors and warning when suppress ibv is enabled and bid request with imp media type is #mediaType

mediaType << [VIDEO, NATIVE, AUDIO]
}

def "PBS shouldn't reject auction with 353 code when enabled suppress ibv and bis response with meta media type is #mediaType"() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PBS should process request without any errors and warning when suppress ibv is enabled and bis response with meta media type is #mediaType

def "PBS shouldn't reject auction with 353 code when enabled response correction is #enabledResponseCorrection and enabled suppress ibv is #enabledSuppressIbv"() {
given: "Default bid request with banner and APP"
def bidRequest = getDefaultBidRequest(APP).tap {
ext.prebid.returnAllBidStatus = true
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we really need returnAllBidStatus?

mediaType << [BANNER, NATIVE, AUDIO]
}

def "PBS shouldn't reject auction with 353 code when requested bidder is excluded"() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PBS should process request without any errors and warning when requested bidder is excluded

[BidderName.GENERIC, BidderName.GENERIC_CAMEL_CASE],]
}

def "PBS should reject auction with 353 code when enabled suppress ibv and imp with media type Banner and bid response meta media type with Video and excluded bidder are not in case"() {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PBS should reject auction with 353 code when suppress IBV is enabled, imp media type is Banner, bid response meta media type is Video, and excluded list lacks request bidder

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and are we sure that it only for banner? Not the general mismatch with video bid response meta media type?

Comment on lines +909 to +915
private static Account accountConfigWithResponseCorrectionModuleAndAppVideoHtml(String accountId, boolean enabledResponseCorrection = true, boolean enabledAppVideoHtml = true) {
accountConfigWithResponseCorrectionModule(accountId, enabledResponseCorrection, enabledAppVideoHtml, false)
}

private static Account accountConfigWithResponseCorrectionModuleAndSuppressIbv(String accountId, boolean enabledResponseCorrection = true, boolean enabledSuppressIbv = true) {
accountConfigWithResponseCorrectionModule(accountId, enabledResponseCorrection, false, enabledSuppressIbv)
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we should set enabledResponseCorrection to true by default? It will be in this position for 90% of cases. Also, should we add excludedBidders as a parameter for accountConfigWithResponseCorrectionModuleAndSuppressIbv?

@osulzhenko
Copy link
Collaborator

update in progress

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tests Functional or other tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants