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

Core: Add new OwnAdx bidder #2868

Merged
merged 11 commits into from
Aug 13, 2024
Merged

Core: Add new OwnAdx bidder #2868

merged 11 commits into from
Aug 13, 2024

Conversation

marki1an
Copy link
Collaborator

No description provided.

@marki1an marki1an linked an issue Dec 28, 2023 that may be closed by this pull request
assertThat(result.getErrors()).isEmpty();
assertThat(result.getValue())
.flatExtracting(HttpRequest::getImpIds)
.containsExactlyInAnyOrder(firstId, secondId, firstId, secondId);
Copy link
Collaborator

Choose a reason for hiding this comment

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

so what is the difference between the bid requests?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@marki1an please add .hasSize(2)

@Test
public void openrtb2AuctionShouldRespondWithBidsFromOwnAdx() throws IOException, JSONException {
// given
WIRE_MOCK_RULE.stubFor(post(urlPathEqualTo("/ownadx-exchange/bid//"))
Copy link
Collaborator

Choose a reason for hiding this comment

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

@SerhiiNahornyi isn't that weird URL? I don't understand why the json requires only one of those params while the URL needs all three

I also don't see any example on the GO side when they cover the case of now having at least one of those params

Copy link
Collaborator Author

@marki1an marki1an Jan 9, 2024

Choose a reason for hiding this comment

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

I thinking about it, I suppose owned.json should contain those params in one required block.

Apart that GO has differend naming between ownadx.json(inside json either, I hope it's typo) and imp_ownadx.go.

Copy link
Collaborator

Choose a reason for hiding this comment

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

https://github.com/prebid/prebid.github.io/pull/4687/files describes all the fields as mandatory... I suppose it's a mistake on the GO side, but it's worth discussing with @SerhiiNahornyi

Copy link
Collaborator

Choose a reason for hiding this comment

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

Please point this question to @bretg, json schema definitely needs to be updated
So we need to reach OwnAdx

Copy link
Collaborator

@AntoxaAntoxic AntoxaAntoxic Apr 2, 2024

Choose a reason for hiding this comment

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

@marki1an any updates here?

anyway I suggest following the requirements, where all the URL parts are mandatory

@SerhiiNahornyi SerhiiNahornyi added the do not merge Not the time for merging yet label Jan 23, 2024
return endpointUrl
.replace(ACCOUNT_ID_MACROS_ENDPOINT, ownAdx.map(ExtImpOwnAdx::getSspId).orElse(""))
.replace(ZONE_ID_MACROS_ENDPOINT, ownAdx.map(ExtImpOwnAdx::getSeatId).orElse(""))
.replace(SOURCE_ID_MACROS_ENDPOINT, ownAdx.map(ExtImpOwnAdx::getTokenId).orElse(""));
Copy link
Collaborator

Choose a reason for hiding this comment

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

StringUtils.EMPTY

@SerhiiNahornyi
Copy link
Collaborator

Depends on prebid/prebid-server#3774

@SerhiiNahornyi
Copy link
Collaborator

We can also add updates from this issue #3260

@SerhiiNahornyi
Copy link
Collaborator

Depends on prebid/prebid-server#3813

@marki1an marki1an linked an issue Aug 5, 2024 that may be closed by this pull request
@SerhiiNahornyi SerhiiNahornyi linked an issue Aug 13, 2024 that may be closed by this pull request
@SerhiiNahornyi SerhiiNahornyi removed the do not merge Not the time for merging yet label Aug 13, 2024
@SerhiiNahornyi SerhiiNahornyi merged commit f486ca5 into master Aug 13, 2024
4 of 5 checks passed
@SerhiiNahornyi SerhiiNahornyi deleted the add-new-ownAdX-adapter branch August 13, 2024 13:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants