33Across: Add video support in adapter #1557

curlyblueeagle · 2020-10-28T18:10:09Z

No description provided.

SyntaxNode · 2020-11-03T19:26:49Z

@curlyblueeagle Are you looking to also add support for Long Form Video / Instream Video with this PR?

guscarreon

Looks pretty good, just a question in regards of the JSON files. Also, just for consistency in respect of other adapters, can we rename adapters/33across/33across/ folder to adapters/33across/33acrosstest/ please?

guscarreon · 2020-11-04T02:49:00Z

adapters/33across/33across/exemplary/simple-video.json

+                "ext": {
+                  "ttx": {
+                    "mediaType": "video"
+                  }


Do we only expect the 33Across server to respond with an "ext" field to video requests? Because in the banner example found in adapters/33across/33across/exemplary/simple-banner.json the "mockResponse" doesn't have that. If they do, should we expect a "banner" value?

45 "mockResponse": { 46 "status": 200, 47 "body": { 48 "id": "test-request-id", 49 "seatbid": [ 50 { 51 "seat": "ttx", 52 "bid": [{ 53 "id": "8ee514f1-b2b8-4abb-89fd-084437d1e800", 54 "impid": "test-imp-id", 55 "price": 0.500000, 56 "adm": "some-test-ad", 57 "crid": "crid_10", 58 "h": 90, 59 "w": 728 + "ext": { + "ttx": { + "mediaType": "video" + } + } 60 }] 61 } 62 ], 63 "cur": "USD" 64 } 65 } 66 } 67 ], adapters/33across/33across/exemplary/simple-banner.json

Correct, the ext field if present will specify the mediaType. If not present assume banner, which is what the adapter will do in such a case.

didn't mean to approve, I meant to request changes

guscarreon

Looks pretty good, just a question in regards of the JSON files. Also, just for consistency in respect of other adapters, can we rename adapters/33across/33across/ folder to adapters/33across/33acrosstest/ please?

curlyblueeagle · 2020-11-04T19:05:12Z

@SyntaxNode Yes we are looking to support instream. The adapter will treat it the same as outstream except for validating placement and startdelay values.

SyntaxNode · 2020-11-04T21:55:37Z

@SyntaxNode Yes we are looking to support instream. The adapter will treat it the same as outstream except for validating placement and startdelay values.

Gotcha. Sounds good. The /openrtb2/video endpoint requires certain fields on the bid be populated to work properly. We have those requirements documented here: https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html#long-form-video-support

curlyblueeagle · 2020-11-04T23:12:10Z

@SyntaxNode Yes we are looking to support instream. The adapter will treat it the same as outstream except for validating placement and startdelay values.

Gotcha. Sounds good. The /openrtb2/video endpoint requires certain fields on the bid be populated to work properly. We have those requirements documented here: https://docs.prebid.org/prebid-server/developers/add-new-bidder-go.html#long-form-video-support

Oh interesting. We were assuming that Long Form was Ad Pods which is distinct from instream. Is that not the case? We are not looking to support Ad Pods. Just instream video.

SyntaxNode · 2020-11-05T17:06:17Z

Oh interesting. We were assuming that Long Form was Ad Pods which is distinct from instream. Is that not the case? We are not looking to support Ad Pods. Just instream video.

Ah, ok. You're fine. I was wrongfully assuming all instream video was ad pod based. I'll update our docs to make that clearer.

SyntaxNode

Code looks good. I commented on a few areas where we'd like to see increased test coverage.

adapters/33across/33across.go

SyntaxNode · 2020-11-05T17:12:07Z

adapters/33across/33across.go

+		videoCopy.Placement = 1
+
+		if videoCopy.StartDelay == nil {
+			videoCopy.StartDelay = openrtb.StartDelay.Ptr(0)


Please add test coverage for this default to 0 branch.

SyntaxNode · 2020-11-05T17:12:31Z

adapters/33across/33across.go

+		return openrtb_ext.BidTypeVideo
+	}
+
+	return openrtb_ext.BidTypeBanner


Please add test coverage for this assume banner if not a video mediatype branch.

curlyblueeagle · 2020-11-05T17:25:37Z

Oh interesting. We were assuming that Long Form was Ad Pods which is distinct from instream. Is that not the case? We are not looking to support Ad Pods. Just instream video.

Ah, ok. You're fine. I was wrongfully assuming all instream video was ad pod based. I'll update our docs to make that clearer.

I asked this question earlier to the Prebid team :). This was the answer I received #1487 (comment)

So yes, we will be doing on only "Short form" instream and not Ad Pod (aka Long form)

Added more unit tests

curlyblueeagle · 2020-11-06T16:22:47Z

Code looks good. I commented on a few areas where we'd like to see increased test coverage.

@SyntaxNode I have added more tests to pass the threshold of 90% (sorry only just came to know about this requirement). Please let me know if you need anything more.

guscarreon

I believe you forgot to get rid of the adapters/33across/33across and the adapters/33across/33across/supplemental directories that now seem to be empty

╰─➤  tree adapters/33across
adapters/33across
├── 33across
│   └── supplemental
├── 33across.go
├── 33across_test.go
├── 33acrosstest
│   ├── exemplary
│   │   ├── bidresponse-defaults.json
│   │   ├── instream-video-defaults.json
│   │   ├── multi-format.json
│   │   ├── optional-params.json
│   │   ├── outstream-video-defaults.json
│   │   ├── simple-banner.json
│   │   └── simple-video.json
│   ├── params
│   │   └── race
│   │       ├── banner.json
│   │       └── video.json
│   └── supplemental
│       ├── status-not-ok.json
│       └── video-validation-fail.json
├── params_test.go
├── usersync.go
└── usersync_test.go

curlyblueeagle · 2020-11-09T14:02:06Z

I believe you forgot to get rid of the adapters/33across/33across and the adapters/33across/33across/supplemental directories that now seem to be empty

╰─➤  tree adapters/33across
adapters/33across
├── 33across
│   └── supplemental
├── 33across.go
├── 33across_test.go
├── 33acrosstest
│   ├── exemplary
│   │   ├── bidresponse-defaults.json
│   │   ├── instream-video-defaults.json
│   │   ├── multi-format.json
│   │   ├── optional-params.json
│   │   ├── outstream-video-defaults.json
│   │   ├── simple-banner.json
│   │   └── simple-video.json
│   ├── params
│   │   └── race
│   │       ├── banner.json
│   │       └── video.json
│   └── supplemental
│       ├── status-not-ok.json
│       └── video-validation-fail.json
├── params_test.go
├── usersync.go
└── usersync_test.go

Hmm not sure where you are seeing this folder structure. Here's what I see in the source branch

SyntaxNode · 2020-11-09T17:53:51Z

@guscarreon I just pulled down a fresh copy and cannot see the empty directories. That might be a sync status issue on your machine.

guscarreon · 2020-11-09T18:56:38Z

Hmm not sure where you are seeing this folder structure. Here's what I see in the source branch

You are right, those must have been left-overs in my local folder

adapters/33across/33across.go

SyntaxNode · 2020-11-09T17:57:29Z

adapters/33across/33across.go

@@ -49,6 +56,14 @@ func (a *TtxAdapter) makeRequest(request *openrtb.BidRequest) (*adapters.Request
 		errs = append(errs, err)
 	}

+	if reqCopy.Imp[0].Banner == nil && reqCopy.Imp[0].Video == nil {


reqCopy.Imp[0]

It's unexpected for an adapter to only forward the first impression and throw out the rest. I recognize this is existing behavior which predates me by quite a bit. I don't wish for this to hold up this PR.

At the very least, please document this behavior here: https://github.com/prebid/prebid.github.io/blob/master/dev-docs/bidders/33across.md

As a future improvement, you may wish to consider sending multiple requests to your bidding server for each impression provided by the publisher.

Thanks for the suggestion, will incorporate this in follow up PR. I will also file a Docs PR shortly.

@SyntaxNode a follow up question about your suggestion to send multiple requests-- how would Prebid Server handle responses? Wait for all responses for each request to return or send them upstream as they come one-by-one?

adapters/33across/33across.go

add video support

cfb4cea

SyntaxNode requested review from SyntaxNode and guscarreon October 29, 2020 14:18

SyntaxNode assigned SyntaxNode and guscarreon Oct 29, 2020

guscarreon previously approved these changes Nov 4, 2020

View reviewed changes

guscarreon reviewed Nov 4, 2020

View reviewed changes

curlyblueeagle added 2 commits November 4, 2020 14:26

renamed folder for tests per code review from Prebid

b37978e

updated path to renamed test folder

e248833

SyntaxNode reviewed Nov 5, 2020

View reviewed changes

Added more unit tests

820fb30

This was referenced Nov 6, 2020

Add test coverage #1567

Closed

Added more unit tests 33Across/prebid-server#1

Merged

Merge pull request #1 from 33Across/add_test_coverage

193c42f

Added more unit tests

curlyblueeagle requested a review from SyntaxNode November 6, 2020 16:21

guscarreon requested changes Nov 7, 2020

View reviewed changes

SyntaxNode reviewed Nov 9, 2020

View reviewed changes

Added a newline between structs

67e04cd

curlyblueeagle mentioned this pull request Nov 9, 2020

Single imp support prebid/prebid.github.io#2482

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

33Across: Add video support in adapter #1557

33Across: Add video support in adapter #1557

curlyblueeagle commented Oct 28, 2020

SyntaxNode commented Nov 3, 2020

guscarreon left a comment

guscarreon Nov 4, 2020

curlyblueeagle Nov 4, 2020 •

edited

Loading

guscarreon left a comment

curlyblueeagle commented Nov 4, 2020

SyntaxNode commented Nov 4, 2020

curlyblueeagle commented Nov 4, 2020

SyntaxNode commented Nov 5, 2020

SyntaxNode left a comment

SyntaxNode Nov 5, 2020

SyntaxNode Nov 5, 2020

curlyblueeagle commented Nov 5, 2020

curlyblueeagle commented Nov 6, 2020

guscarreon left a comment

curlyblueeagle commented Nov 9, 2020

SyntaxNode commented Nov 9, 2020

guscarreon commented Nov 9, 2020 •

edited

Loading

SyntaxNode Nov 9, 2020

curlyblueeagle Nov 9, 2020

curlyblueeagle Nov 10, 2020

33Across: Add video support in adapter #1557

33Across: Add video support in adapter #1557

Conversation

curlyblueeagle commented Oct 28, 2020

SyntaxNode commented Nov 3, 2020

guscarreon left a comment

Choose a reason for hiding this comment

guscarreon Nov 4, 2020

Choose a reason for hiding this comment

curlyblueeagle Nov 4, 2020 • edited Loading

Choose a reason for hiding this comment

guscarreon left a comment

Choose a reason for hiding this comment

curlyblueeagle commented Nov 4, 2020

SyntaxNode commented Nov 4, 2020

curlyblueeagle commented Nov 4, 2020

SyntaxNode commented Nov 5, 2020

SyntaxNode left a comment

Choose a reason for hiding this comment

SyntaxNode Nov 5, 2020

Choose a reason for hiding this comment

SyntaxNode Nov 5, 2020

Choose a reason for hiding this comment

curlyblueeagle commented Nov 5, 2020

curlyblueeagle commented Nov 6, 2020

guscarreon left a comment

Choose a reason for hiding this comment

curlyblueeagle commented Nov 9, 2020

SyntaxNode commented Nov 9, 2020

guscarreon commented Nov 9, 2020 • edited Loading

SyntaxNode Nov 9, 2020

Choose a reason for hiding this comment

curlyblueeagle Nov 9, 2020

Choose a reason for hiding this comment

curlyblueeagle Nov 10, 2020

Choose a reason for hiding this comment

curlyblueeagle Nov 4, 2020 •

edited

Loading

guscarreon commented Nov 9, 2020 •

edited

Loading