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

Smaato: Change server response type #3734

Conversation

Enigo
Copy link
Contributor

@Enigo Enigo commented Jun 6, 2024

We've simplified our server response to make it more ortb-like
Backward compatibility is guaranteed server-side

Copy link

github-actions bot commented Jun 6, 2024

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, 8906df3

smaato

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/smaato/banner.go:9:		extractAdmBanner		100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/native.go:14:	extractAdmNative		85.7%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:70:	Builder				100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:79:	MakeRequests			100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:99:	MakeBids			85.3%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:163:	makeIndividualRequests		89.5%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:196:	splitImpressionsByMediaType	100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:226:	makePodRequests			84.6%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:250:	makeRequest			85.7%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:269:	getAdMarkupType			100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:279:	getTTLFromHeaderOrDefault	100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:293:	renderAdMarkup			100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:308:	convertAdMarkupTypeToMediaType	80.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:323:	prepareCommonRequest		100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:337:	prepareIndividualRequest	100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:347:	preparePodRequest		80.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:359:	setUser				95.2%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:402:	setExt				100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:410:	setSite				100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:430:	setApp				100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:437:	setPublisherId			100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:456:	setImpForAdspace		90.9%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:476:	setImpForAdBreak		94.1%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:508:	makeImpExt			90.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:530:	groupImpressionsByPod		100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:550:	buildBidVideo			87.5%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:570:	extractBidExt			100.0%
total:									(statements)			93.1%

@@ -57,7 +57,8 @@ type bidRequestExt struct {

// bidExt defines Bid.Ext object for Smaato
type bidExt struct {
Duration int `json:"duration"`
Duration int `json:"duration"`
Curls []string `json:"curls"`
Copy link
Collaborator

Choose a reason for hiding this comment

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

What is the main purpose of adding the Curls field to the bidExt structure? Can you provide examples of situations where it will be used?

Copy link
Collaborator

Choose a reason for hiding this comment

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

is it click url?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@przemkaczmarek
yes it is click urls array
use case is to track ads clicks - both our own and potentially demand partners - and since there is no ortb specific field for this we decided to use ext

przemkaczmarek
przemkaczmarek previously approved these changes Jul 17, 2024
@bsardo bsardo self-assigned this Jul 18, 2024
Copy link
Contributor

@bsardo bsardo left a comment

Choose a reason for hiding this comment

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

@Enigo can you please merge with master (no rebase) to resolve conflicts? I also left just one minor comment. Everything else looks good.

expectedAdMarkup string
}{
{
testName: "extract banner without curls",
Copy link
Contributor

Choose a reason for hiding this comment

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

Nitpick: I suggest replacing the spaces with underscores in your test names (e.g. extract_banner_without_curls). t.Run will automatically convert them to underscores but it would be better to avoid that so it is easier to find failed tests via a search without having to consider this possibility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

renamed and fixed the merge conflicts for both PRs

@Enigo Enigo force-pushed the feature/SCORE-16207_Q_prebid_migrate_away_from_api_v6_type_of_responses branch from 8906df3 to b2188fa Compare July 23, 2024 03:59
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, b2188fa

smaato

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/smaato/banner.go:9:		extractAdmBanner		100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/native.go:14:	extractAdmNative		85.7%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:70:	Builder				100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:79:	MakeRequests			100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:99:	MakeBids			85.3%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:163:	makeIndividualRequests		89.5%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:196:	splitImpressionsByMediaType	100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:226:	makePodRequests			84.6%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:250:	makeRequest			85.7%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:269:	getAdMarkupType			100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:279:	getTTLFromHeaderOrDefault	100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:293:	renderAdMarkup			100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:308:	convertAdMarkupTypeToMediaType	80.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:323:	prepareCommonRequest		100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:337:	prepareIndividualRequest	100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:347:	preparePodRequest		80.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:359:	setUser				95.2%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:402:	setExt				100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:410:	setSite				100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:430:	setApp				100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:437:	setPublisherId			100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:456:	setImpForAdspace		90.9%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:476:	setImpForAdBreak		94.1%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:508:	makeImpExt			90.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:530:	groupImpressionsByPod		100.0%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:550:	buildBidVideo			87.5%
github.com/prebid/prebid-server/v2/adapters/smaato/smaato.go:570:	extractBidExt			100.0%
total:									(statements)			93.1%

@Enigo
Copy link
Contributor Author

Enigo commented Jul 24, 2024

@bsardo @przemkaczmarek
can we then merge it now?

@bsardo bsardo merged commit d1596c8 into prebid:master Jul 24, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

None yet

3 participants