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.

Sign up for GitHub

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

Jump to bottom

New adapter: MinuteMedia #3399

Merged
merged 3 commits into from
Jan 22, 2024
Merged

New adapter: MinuteMedia #3399

merged 3 commits into from
Jan 22, 2024

Conversation

zkosanovic
Copy link
Contributor

No description provided.

@github-actions GitHub Actions
Copy link

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, 3cb4299

minutemedia

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:22:	Builder			100.0%
github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:29:	MakeRequests		81.8%
github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:54:	MakeBids		88.9%
github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:91:	extractOrg		100.0%
github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:112:	getMediaTypeForBid	100.0%
total:										(statements)		91.1%

@noamtzu
Copy link

noamtzu commented Jan 16, 2024

Hi guys, can we assign someone to review this PR? Thanks 🙏🏻
@bretg @bsardo

SyntaxNode
SyntaxNode reviewed Jan 16, 2024
View reviewed changes
adapters/minutemedia/minutemedia.go Outdated Show resolved Hide resolved
adapters/minutemedia/minutemedia.go Outdated Show resolved Hide resolved
adapters/minutemedia/minutemediatest/supplemental/missing-org.json Outdated Show resolved Hide resolved
adapters/minutemedia/minutemediatest/supplemental/missing-mtype.json Outdated Show resolved Hide resolved
adapters/minutemedia/minutemedia.go Outdated Show resolved Hide resolved
adapters/minutemedia/minutemedia.go Show resolved Hide resolved
static/bidder-info/minutemedia.yaml Show resolved Hide resolved
adapters/minutemedia/minutemedia_test.go Show resolved Hide resolved
adapters/minutemedia/minutemediatest/exemplary/banner-and-video-app.json Show resolved Hide resolved
@github-actions GitHub Actions
Copy link

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, 6b9e29b

minutemedia

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:24:	Builder			100.0%
github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:31:	MakeRequests		81.8%
github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:56:	MakeBids		88.9%
github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:93:	extractOrg		100.0%
github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:114:	getMediaTypeForBid	100.0%
total:										(statements)		91.1%

@github-actions GitHub Actions
Copy link

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, 64c4013

minutemedia

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:24:	Builder			100.0%
github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:31:	MakeRequests		81.8%
github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:56:	MakeBids		88.9%
github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:93:	extractOrg		90.0%
github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:112:	getMediaTypeForBid	100.0%
total:										(statements)		88.6%

@zkosanovic zkosanovic requested a review from SyntaxNode January 17, 2024 08:43
gargcreation1992
gargcreation1992 reviewed Jan 17, 2024
View reviewed changes
adapters/minutemedia/minutemedia.go

func extractOrg(openRTBRequest *openrtb2.BidRequest) (string, error) {
var err error
for _, imp := range openRTBRequest.Imp {
Copy link
Contributor

Choose a reason for hiding this comment

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

Please confirm that if there are multiple impressions than are you extracting org from the first imp ext that has an org. Or do you intent to validate that org should be same for all the imps in a request?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added minLength of 1 to the JSON schema as suggested by @SyntaxNode, so now all imps will have to have the org, right?

Either way, we expect that the publishers will send their ID in the org parameter and that it will be the same for all imps, no need to validate that IMO, we'll just use the first one.

Copy link
Contributor

Choose a reason for hiding this comment

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

so now all imps will have to have the org, right?

Correct.

we expect that the publishers will send their ID in the org parameter and that it will be the same for all imps

There is no guarantee publishers will make it the same for all impressions. At the very least, please make this expectation clear in your separate bidder docs PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@SyntaxNode I think we're good, this expectation is defined and clear in the docs: https://github.com/prebid/prebid.github.io/blob/master/dev-docs/bidders/minutemedia.md

gargcreation1992
gargcreation1992 reviewed Jan 17, 2024
View reviewed changes
Copy link
Contributor

@gargcreation1992 gargcreation1992 left a comment

Choose a reason for hiding this comment

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

Requesting few minor changes and verifications. Rest looks good to me!

@github-actions GitHub Actions
Copy link

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, 93c7e41

minutemedia

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:24:	Builder			100.0%
github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:31:	MakeRequests		88.9%
github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:54:	MakeBids		88.9%
github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:91:	extractOrg		90.0%
github.com/prebid/prebid-server/v2/adapters/minutemedia/minutemedia.go:110:	getMediaTypeForBid	100.0%
total:										(statements)		90.5%

@Sonali-More-Xandr Sonali-More-Xandr merged commit daebdcc into prebid:master Jan 22, 2024
5 checks passed
@cross-repo-issue-creation cross-repo-issue-creation bot mentioned this pull request Jan 22, 2024
Closed
@zkosanovic zkosanovic mentioned this pull request Jan 22, 2024
Merged
8 tasks
@zkosanovic zkosanovic deleted the minutemedia-adapter branch April 23, 2024 12:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Sonali-More-Xandr Sonali-More-Xandr Sonali-More-Xandr approved these changes

@gargcreation1992 gargcreation1992 gargcreation1992 approved these changes

@SyntaxNode SyntaxNode Awaiting requested review from SyntaxNode

Assignees

@gargcreation1992 gargcreation1992

@Sonali-More-Xandr Sonali-More-Xandr

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@zkosanovic @noamtzu @SyntaxNode @gargcreation1992 @Sonali-More-Xandr