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

OpenRTB v2.6-202309 #11

Merged
merged 6 commits into from
Nov 28, 2023
Merged

OpenRTB v2.6-202309 #11

merged 6 commits into from
Nov 28, 2023

Conversation

SyntaxNode
Copy link
Contributor

Updates the object model for OpenRTB 2.6-202309 release.

openrtb2/deal.go Outdated
Comment on lines 95 to 97
// An array of DurFloors objects (Section 3.2.35) indicating the floor
// prices for video creatives of various durations that the buyer may bid with.
DurFloors []DurFloors `json:"durfloors,omitempty"`

Choose a reason for hiding this comment

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

Description in spec:

Container for floor price by duration information, to be used if a given deal is eligible for video or audio demand. An array of DurFloors objects (see Section 3.2.35).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good find. This was a copy/paste error. Fixed.

Copy link

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

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

This might be out of the scope of this PR: openrtb2.x/2.6.md, section 3.2.34 refers to this attribute as min_refresh_interval_seconds from commit a6ab486cbddb2d91031cd7b554299da7a09adaf6 on. Should we rename to MinRefreshInterval or leave it as MinInt?

23     // Attribute:
24     //   minint
25     // Type:
26     //   integer; recommended
27     // Description:
28     //   The minimum refresh interval in seconds. This applies to all
29     //   refresh types. This is the (uninterrupted) time the ad creative
30     //   will be rendered before refreshing to the next creative. If
31     //   the field is absent, the exposure time is unknown. This field
32     //   does not account for viewability or external factors such as a
33     //   user leaving a page.
34     MinInt int `json:"minint,omitempty"`
   +   MinRefreshInterval int `json:"min_refresh_int,omitempty"`
openrtb2/refsettings.go

@@ -8,7 +8,7 @@ import (

// 3.2.1 Object: BidRequest
//
// The top-level bid request object contains a globally unique bid request or auction ID.
// The top-level bid request object contains an exchange unique bid request or auction ID.

Choose a reason for hiding this comment

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

Maybe it's just me but I find the comment in line 25 below easier to understand than this one. At least upon first read. I realize this is the exact sentence found in the docs but, could we slightly reword?

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'd like to match the spec, but I can propose an update to the spec. What would sound better?

Choose a reason for hiding this comment

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

How about: "The top-level bid request object contains an exchange-assigned unique bid request ID or auction ID.
"?

@@ -8,7 +8,7 @@ import (

// 3.2.1 Object: BidRequest

Choose a reason for hiding this comment

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

Pull request Add AutoRefresh and AutoRefreshSettings to Bid Request of the openrtb2.x/2.6.md repository lists AutoRefresh and AutoRefreshSettings under the BidRequest list but, this fields, along with DurFlooors are not in the table depicted in the 3.2.1 - Object: BidRequest section. Are they getting added to the table, and eventually, as fields of the BidRequest struct eventually? Should we add them to the BidRequest object already?

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'm curious what pointed your attention to that specific commit? That was a commit to a develop branch. The location was moved to the Imp object before release.

Are they getting added to the table, and eventually, as fields of the BidRequest struct eventually?

No.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The main branch correctly places the refresh object in the desired location.

@SyntaxNode
Copy link
Contributor Author

This might be out of the scope of this PR: openrtb2.x/2.6.md, section 3.2.34 refers to this attribute as min_refresh_interval_seconds from commit a6ab486cbddb2d91031cd7b554299da7a09adaf6 on. Should we rename to MinRefreshInterval or leave it as MinInt?

23     // Attribute:
24     //   minint
25     // Type:
26     //   integer; recommended
27     // Description:
28     //   The minimum refresh interval in seconds. This applies to all
29     //   refresh types. This is the (uninterrupted) time the ad creative
30     //   will be rendered before refreshing to the next creative. If
31     //   the field is absent, the exposure time is unknown. This field
32     //   does not account for viewability or external factors such as a
33     //   user leaving a page.
34     MinInt int `json:"minint,omitempty"`
   +   MinRefreshInterval int `json:"min_refresh_int,omitempty"`
openrtb2/refsettings.go

The field was called min_refresh_int in an earlier draft. It was renamed to minint before release.

Copy link

@guscarreon guscarreon left a comment

Choose a reason for hiding this comment

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

LGTM

@bsardo bsardo merged commit 3dee7e9 into prebid:main Nov 28, 2023
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants