Skip to content

Ogury: Enable in app traffic#4314

Merged
bsardo merged 5 commits into
prebid:masterfrom
Ogury:ADV-26330-enable-in-app-traffic
Jun 5, 2025
Merged

Ogury: Enable in app traffic#4314
bsardo merged 5 commits into
prebid:masterfrom
Ogury:ADV-26330-enable-in-app-traffic

Conversation

@krdzo
Copy link
Copy Markdown
Contributor

@krdzo krdzo commented Apr 24, 2025

No description provided.

bsardo
bsardo reviewed Apr 24, 2025
View reviewed changes
Comment thread static/bidder-info/ogury.yaml
Comment on lines +13 to +15
app:
mediaTypes:
- banner
Copy link
Copy Markdown
Collaborator

@bsardo bsardo Apr 24, 2025

Choose a reason for hiding this comment

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

Please add an exemplary JSON test with the app field present in mockBidRequest.

Copy link
Copy Markdown
Contributor Author

@krdzo krdzo Apr 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Added the app JSON test.
Also converted this PR to draft, some unresolved questions have arisen internally so until they are resolved It would be better not to merge this PR.
Sorry for the inconvenience

@bsardo bsardo self-assigned this Apr 24, 2025
@bsardo bsardo changed the title Ogury adapter: enable in app traffic Ogury: Enable in app traffic Apr 24, 2025
@bsardo bsardo added the adapter label Apr 25, 2025
@krdzo krdzo marked this pull request as draft April 28, 2025 09:27
@github-actions
Copy link
Copy Markdown

github-actions Bot commented Apr 28, 2025

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, ca9be5d

ogury

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/ogury/ogury.go:22:	Builder			100.0%
github.com/prebid/prebid-server/v3/adapters/ogury/ogury.go:26:	MakeRequests		75.7%
github.com/prebid/prebid-server/v3/adapters/ogury/ogury.go:114:	buildHeaders		50.0%
github.com/prebid/prebid-server/v3/adapters/ogury/ogury.go:127:	getMediaTypeForBid	33.3%
github.com/prebid/prebid-server/v3/adapters/ogury/ogury.go:144:	MakeBids		70.0%
total:								(statements)		68.1%

@krdzo krdzo marked this pull request as ready for review May 15, 2025 07:19
@krdzo
Copy link
Copy Markdown
Contributor Author

krdzo commented May 15, 2025

Hello @pm-isha-bharti the PR is ready for review, please take a look

pm-isha-bharti
pm-isha-bharti reviewed May 16, 2025
View reviewed changes
Comment thread adapters/ogury/ogury.go Outdated
}

if len(impsWithOguryParams) == 0 && (request.Site == nil || request.Site.Publisher.ID == "") {
if len(impsWithOguryParams) == 0 && (request.Site == nil || request.Site.Publisher == nil || request.Site.Publisher.ID == "") {
Copy link
Copy Markdown
Contributor

@pm-isha-bharti pm-isha-bharti May 16, 2025

Choose a reason for hiding this comment

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

Is this check valid for app requests as well?

Copy link
Copy Markdown
Contributor

@pm-isha-bharti pm-isha-bharti May 16, 2025

Choose a reason for hiding this comment

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

What should be the expected behavior when the request is for an app, but assetKey and adUnitId are missing from imp.ext?

Copy link
Copy Markdown
Contributor

@pm-isha-bharti pm-isha-bharti May 16, 2025

Choose a reason for hiding this comment

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

Also can you include test case for this scenario?

Copy link
Copy Markdown
Contributor Author

@krdzo krdzo May 16, 2025

Choose a reason for hiding this comment

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

added logic for when the request is from app but the request doesn't have assetKey/adUnitId values, also added a json test case for it

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 16, 2025

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 1712f23

ogury

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/ogury/ogury.go:22:	Builder			100.0%
github.com/prebid/prebid-server/v3/adapters/ogury/ogury.go:26:	MakeRequests		77.5%
github.com/prebid/prebid-server/v3/adapters/ogury/ogury.go:122:	buildHeaders		50.0%
github.com/prebid/prebid-server/v3/adapters/ogury/ogury.go:135:	getMediaTypeForBid	33.3%
github.com/prebid/prebid-server/v3/adapters/ogury/ogury.go:152:	MakeBids		70.0%
total:								(statements)		69.3%

@pm-isha-bharti
Copy link
Copy Markdown
Contributor

pm-isha-bharti commented May 17, 2025

LGTM. Can you also update the documentation to add support for app by adding: pbs_app_supported: true

@krdzo krdzo mentioned this pull request May 19, 2025
Merged
1 task
@krdzo
Copy link
Copy Markdown
Contributor Author

krdzo commented May 19, 2025

@pm-isha-bharti
Here is the PR for the change of the documentation

pm-isha-bharti
pm-isha-bharti previously approved these changes May 20, 2025
View reviewed changes
@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 26, 2025

Code coverage summary

Note:

  • Prebid team doesn't anticipate tests covering code paths that might result in marshal and unmarshal errors
  • Coverage summary encompasses all commits leading up to the latest one, 39a8d55

ogury

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/ogury/ogury.go:22:	Builder			100.0%
github.com/prebid/prebid-server/v3/adapters/ogury/ogury.go:26:	MakeRequests		77.5%
github.com/prebid/prebid-server/v3/adapters/ogury/ogury.go:122:	buildHeaders		50.0%
github.com/prebid/prebid-server/v3/adapters/ogury/ogury.go:135:	getMediaTypeForBid	33.3%
github.com/prebid/prebid-server/v3/adapters/ogury/ogury.go:152:	MakeBids		70.0%
total:								(statements)		69.3%

@krdzo
Copy link
Copy Markdown
Contributor Author

krdzo commented May 26, 2025

Hello @pm-isha-bharti
I noticed that the new error message was incomplete so I pushed a change to fix it. Can you look again and approve the PR.

Also when do you expect we can get a second review for this PR so we can merge it?

@pm-isha-bharti
Copy link
Copy Markdown
Contributor

pm-isha-bharti commented May 27, 2025

@bsardo Could someone please review this PR?

@krdzo krdzo mentioned this pull request May 27, 2025
Merged
5 tasks
@bsardo bsardo merged commit 8c93fa2 into prebid:master Jun 5, 2025
6 checks passed
@cross-repo-issue-creation cross-repo-issue-creation Bot mentioned this pull request Jun 5, 2025
Closed
shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Jun 13, 2025
@krdzo @shunj-nb
Ogury: Enable in app traffic (prebid#4314)
ff9321d
prnvgupta pushed a commit to automatad/prebid-server that referenced this pull request Dec 3, 2025
@krdzo @prnvgupta
Ogury: Enable in app traffic (prebid#4314)
aee26fd
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bsardo bsardo bsardo left review comments

@pm-isha-bharti pm-isha-bharti pm-isha-bharti approved these changes

@przemkaczmarek przemkaczmarek przemkaczmarek approved these changes

Assignees

@bsardo bsardo

@pm-isha-bharti pm-isha-bharti

Labels

adapter

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@krdzo @pm-isha-bharti @bsardo @przemkaczmarek