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

Nextmillennium: add extra_info support #3351

Merged
merged 3 commits into from
Dec 21, 2023
Merged

Nextmillennium: add extra_info support #3351

merged 3 commits into from
Dec 21, 2023

Conversation

JacobKlein26
Copy link
Contributor

Added the ability for users to pass custom flags using extra_info,

@SyntaxNode SyntaxNode self-requested a review December 13, 2023 19:03
@SyntaxNode SyntaxNode self-assigned this Dec 13, 2023
@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, 09e3b37

nextmillennium

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:35:	MakeRequests		90.0%
github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:53:	getImpressionsInfo	100.0%
github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:67:	getImpressionExt	85.7%
github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:84:	buildAdapterRequest	88.9%
github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:104:	createBidRequest	95.8%
github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:142:	MakeBids		88.2%
github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:177:	Builder			60.0%
total:											(statements)		89.9%

SyntaxNode
SyntaxNode reviewed Dec 13, 2023
View reviewed changes
Copy link
Contributor

@SyntaxNode SyntaxNode left a comment

Choose a reason for hiding this comment

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

Looking good. Please add unit test coverage for passing in extraInfo and verifying it's included in the request to your servers. Something similar to huaweiads_test.go would be great.

adapters/nextmillennium/nextmillenniumtest/exemplary/banner-empty-group-id.json
@@ -33,6 +33,7 @@
"body":{
"id": "testid",
"ext": {
"nextMillennium": {},
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it desired to send an empty object if the list is empty? If you add an omitempty flag to the json attribute you could skip creating an empty object (unless you need it on your side). ie:

NextMillennium nmExtNMM    `json:"nextMillennium"`

to

NextMillennium nmExtNMM    `json:"nextMillennium,omitempty"`

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 a test case for extra_info,
When I add omitempty it doesn't actually change anything... I always believed omitempty only works on keys (meaning not objects) (or if it's a pointer...),
Either way, it doesn't bother us, so if it's ok with you, I would just leave it...

Copy link
Contributor

Choose a reason for hiding this comment

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

@JacobKlein26

omitempty was added on NmmFlags. But recommendation was to add omitempty on nextMillennium

type nmExtNMM struct {
- 	NmmFlags []string `json:"nmmFlags,omitempty"`
+ 	NmmFlags []string `json:"nmmFlags"`
}
type nextMillJsonExt struct {
	Prebid nmExtPrebid `json:"prebid"`
	Prebid         nmExtPrebid `json:"prebid"`
-	NextMillennium nmExtNMM    `json:"nextMillennium"`
+ 	NextMillennium nmExtNMM    `json:"nextMillennium,omitempty"`

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @onkarvhanumante.
Thanks for clarifying.
I pushed another commit to add it. However, as far as I'm aware, adding omitempty on a type of struct makes no difference (the object itself will be empty but the nextMillennium key will still exist as an empty object).
The only way (from a quick google search) to remove it is to convert it to a pointer.
It is not important for us to remove it, but if for whatever reason you think I should do it, let me know and I'll do it.

Thank you!

@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, ed25481

nextmillennium

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:35:	MakeRequests		90.0%
github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:53:	getImpressionsInfo	100.0%
github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:67:	getImpressionExt	85.7%
github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:84:	buildAdapterRequest	88.9%
github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:104:	createBidRequest	95.8%
github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:142:	MakeBids		88.2%
github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:177:	Builder			80.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, 4d454e9

nextmillennium

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:35:	MakeRequests		90.0%
github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:53:	getImpressionsInfo	100.0%
github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:67:	getImpressionExt	85.7%
github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:84:	buildAdapterRequest	88.9%
github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:104:	createBidRequest	95.8%
github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:142:	MakeBids		88.2%
github.com/prebid/prebid-server/v2/adapters/nextmillennium/nextmillennium.go:177:	Builder			80.0%
total:											(statements)		91.1%

@gargcreation1992 gargcreation1992 merged commit c773b0f into prebid:master Dec 21, 2023
5 checks passed
@cross-repo-issue-creation cross-repo-issue-creation bot mentioned this pull request Dec 21, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SyntaxNode SyntaxNode SyntaxNode left review comments

@onkarvhanumante onkarvhanumante onkarvhanumante approved these changes

@gargcreation1992 gargcreation1992 gargcreation1992 approved these changes

Assignees

@SyntaxNode SyntaxNode

@gargcreation1992 gargcreation1992

@onkarvhanumante onkarvhanumante

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@JacobKlein26 @SyntaxNode @gargcreation1992 @onkarvhanumante