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

Adverxo: New adapter ported from Go #3705

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

Conversation

przemkaczmarek
Copy link
Collaborator

@przemkaczmarek przemkaczmarek commented Jan 27, 2025

🔧 Type of changes

  • new bid adapter

✨ What's the context?

What's the context for the changes?
#3677 (comment)

@przemkaczmarek
Copy link
Collaborator Author

Hi @AntoxaAntoxic, can You check my adapter because I have problem with AdverxoTest

@przemkaczmarek przemkaczmarek changed the title AdverxoTest does not work New Adapter: Adverxo #3677 Jan 28, 2025
AntoxaAntoxic
AntoxaAntoxic previously approved these changes Feb 21, 2025
@osulzhenko osulzhenko linked an issue Feb 25, 2025 that may be closed by this pull request
@osulzhenko osulzhenko changed the title New Adapter: Adverxo #3677 Adverxo: New adapter ported from Go Feb 27, 2025
Comment on lines +7 to +8
@Value
@AllArgsConstructor(staticName = "of")
Copy link
Collaborator

Choose a reason for hiding this comment

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

@Value(staticConstructor = "of")

Copy link
Collaborator

Choose a reason for hiding this comment

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

^^


@JsonProperty("adUnitId")
Integer adUnitId;
@JsonProperty("auth")
Copy link
Collaborator

Choose a reason for hiding this comment

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

No need for @JsonProperty("auth")

Comment on lines 32 to 35
BidderDeps adverxoBidderDeps(BidderConfigurationProperties adverxoConfigurationProperties,
@NotBlank @Value("${external-url}") String externalUrl,
JacksonMapper mapper, CurrencyConversionService currencyConversionService) {

Copy link
Collaborator

Choose a reason for hiding this comment

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

Place each parameter on new different line

enabled: false
endpoint: https://adport.pbsadverxo.com/auction?id={{adUnitId}}&auth={{auth}}
usersync:
enabled: true
Copy link
Collaborator

Choose a reason for hiding this comment

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

enabled: false for all aliases

// given
final BidRequest bidRequest = givenBidRequest(impBuilder ->
impBuilder.ext(mapper.valueToTree(ExtPrebid.of(null, mapper.createArrayNode())))
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Place ); on previous line

impBuilder
.bidfloor(bidFloor)
.bidfloorcur(bidFloorCur)
);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same, and look for other occurrences

Comment on lines 87 to 89
final String adUnitId = String.valueOf(extImp.getAdUnitId());
return endpointUrl
.replace(ADUNIT_MACROS_ENDPOINT,
extImp.getAdUnitId() == null ? StringUtils.EMPTY : String.valueOf(extImp.getAdUnitId()))
.replace(ADUNIT_MACROS_ENDPOINT, adUnitId == null ? "0" : adUnitId)
Copy link
Collaborator

@CTMBNara CTMBNara Mar 10, 2025

Choose a reason for hiding this comment

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

 .replace(ADUNIT_MACROS_ENDPOINT, Objects.toString(extImp.getAdUnitId(), "0"))

@osulzhenko osulzhenko requested a review from CTMBNara March 12, 2025 13:53
Comment on lines +50 to +55
public AdverxoBidder(String endpointUrl, JacksonMapper mapper,
CurrencyConversionService currencyConversionService) {
this.endpointUrl = HttpUtil.validateUrl(Objects.requireNonNull(endpointUrl));
this.mapper = mapper;
this.currencyConversionService = currencyConversionService;
}
Copy link
Collaborator

@CTMBNara CTMBNara Mar 13, 2025

Choose a reason for hiding this comment

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

Last nitpick and we are good to go:

    public AdverxoBidder(String endpointUrl,
                         JacksonMapper mapper,
                         CurrencyConversionService currencyConversionService) {

         this.endpointUrl = HttpUtil.validateUrl(Objects.requireNonNull(endpointUrl));
         this.mapper = Objects.requireNonNull(mapper);
         this.currencyConversionService = Objects.requireNonNull(currencyConversionService);
     }

Comment on lines +7 to +8
@Value
@AllArgsConstructor(staticName = "of")
Copy link
Collaborator

Choose a reason for hiding this comment

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

^^

Comment on lines +12 to +13
Integer adUnitId;
String auth;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please, add empty line between fields

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

Successfully merging this pull request may close these issues.

Port PR from PBS-Go: New Adapter: Adverxo
4 participants