New Adapter: Smoot #4148

leamarty · 2025-01-13T16:08:01Z

github-actions · 2025-01-13T16:08:45Z

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, 7c0f3cf

smoot

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:29:	Builder		100.0%
github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:36:	MakeRequests	74.2%
github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:86:	makeRequest	85.7%
github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:104:	MakeBids	100.0%
github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:141:	getBidType	100.0%
total:								(statements)	85.9%

ccorbo · 2025-01-17T19:03:49Z

Confirmed endpoint:

leamarty · 2025-01-20T11:04:17Z

Thank you @ccorbo ! I imagine we'll need a couple approvals here, right?

Docs were already approved btw: prebid/prebid.github.io#5811

ccorbo · 2025-01-22T16:00:34Z

Validated user sync endpoint and cookie being set

Cookie set:

Email sent to maintainer email adress for verification

ccorbo · 2025-01-22T16:01:14Z

Thank you @ccorbo ! I imagine we'll need a couple approvals here, right?

Docs were already approved btw: prebid/prebid.github.io#5811

Hello! - Yes you will need other approvals as well, it is in the queue.

leamarty · 2025-01-22T16:10:23Z

Thank you @ccorbo ! I'll stay tuned then. By the way:

Email sent to maintainer email adress for verification

Did you send the email to [email protected]? We didn't receive anything yet

ccorbo · 2025-01-22T16:06:22Z

adapters/smoot/smoottest/exemplary/simple-banner.json

@@ -0,0 +1,136 @@
+{


nit: rename to simple-app-banner.json to easily differentiate between the simple-web-banner.json

ccorbo · 2025-01-22T16:09:33Z

adapters/smoot/smoot.go

+		var bidderExt adapters.ExtImpBidder
+		var smootExt openrtb_ext.ImpExtSmoot
+
+		if err := jsonutil.Unmarshal(imp.Ext, &bidderExt); err != nil {


Can you add test coverage on these unmarshal errors

Yes, sorry for that. Done ✅

ccorbo · 2025-01-22T16:15:43Z

adapters/smoot/smoot.go

+	headers.Add("Content-Type", "application/json;charset=utf-8")
+	headers.Add("Accept", "application/json")
+	return &adapters.RequestData{
+		Method:  "POST",


suggestion: use http.MethodPost

Awesome, thanks! Done ✅

ccorbo · 2025-01-22T16:17:26Z

Thank you @ccorbo ! I'll stay tuned then. By the way:

Email sent to maintainer email adress for verification

Did you send the email to [email protected]? We didn't receive anything yet

Sorry about that - missed hitting the send button before I posted that. Just sent it over

ccorbo · 2025-01-22T16:25:02Z

Verified maintainer email address

github-actions · 2025-01-23T08:34:40Z

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, 4fefda1

smoot

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:29:	Builder		100.0%
github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:36:	MakeRequests	87.1%
github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:86:	makeRequest	85.7%
github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:104:	MakeBids	100.0%
github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:141:	getBidType	100.0%
total:								(statements)	92.2%

leamarty · 2025-01-27T09:05:20Z

Hi @ccorbo @bsardo

Just wanted to know if there's any update on this. I'll appreciate a review & merge since we are waiting on it 🙏🏻

Thanks!

ccorbo

LGTM from my POV - will need another approval from another reviewer however

leamarty · 2025-01-29T08:44:30Z

Awesome, thank you @ccorbo . Who should I reach to get the final approval?

bsardo · 2025-01-29T20:54:28Z

Hi @leamarty, I'll review tomorrow and get back to you. Thanks for your patience.

leamarty · 2025-01-30T18:33:33Z

Awesome, thank you @bsardo ! Any news?

bsardo

Looks good, just a few minor comments.

bsardo · 2025-01-31T00:53:58Z

adapters/smoot/params_test.go

+	`{"placementId": 42}`,
+	`{"endpointId": 42}`,


Can you add the following test cases to test the minimum length:

`{"placementId": ""}` `{"endpointId": ""}`

Adding these two separate test cases instead of combining them into one test case like you have ({"placementId": "", "endpointId": ""} ensures that there is a min length constraint on each field instead of on at least one of the fields. I suggest making this change.

bsardo · 2025-01-31T01:20:09Z

adapters/smoot/smoot.go

+		if smootExt.PlacementID != "" {
+			impExt.SmootBidderExt.PlacementID = smootExt.PlacementID
+			impExt.SmootBidderExt.Type = "publisher"
+		} else if smootExt.EndpointID != "" {


This conditional can be deleted with the else if just becoming an else since you've guaranteed that your bidder will only be called if placementId or endpointId is set based on how you've defined your bidder parameters.

bsardo · 2025-01-31T01:41:12Z

adapters/smoot/smoot.go

+			bid := seatBid.Bid[i]
+			bidType, err := getBidType(bid)
+			if err != nil {
+				return nil, []error{err}
+			}
+
+			b := &adapters.TypedBid{
+				Bid:     &seatBid.Bid[i],


What you have works. I'll leave it up to you but you can optimize this with a couple of simple tweaks so you don't ever copy the bid (which you're doing twice) but rather copy the bid address and pass that reference around to read the mtype and to include in the typed bid:

bid := &seatBid.Bid[i] bidType, err := getBidType(bid) if err != nil { return nil, []error{err} } b := &adapters.TypedBid{ Bid: bid, ....

Decided not to do it

bsardo · 2025-01-31T02:20:02Z

adapters/smoot/smoottest/supplemental/bad_media_type.json

Use underscores in the file name: bad-media-type.json

bsardo · 2025-01-31T02:20:27Z

adapters/smoot/smoottest/supplemental/bad_response.json

Use underscores in the file name: bad-response.json

bsardo · 2025-01-31T02:30:23Z

adapters/smoot/smoottest/exemplary/multi-format.json

+      "mockResponse": {
+        "status": 204
+      }
+    }
+  ],
+  "expectedBidResponses": []


This is debatable and I know some adapters have done this but for an exemplary test like this, I would expect to see a happy path 200 with at least one bid, especially when we have a supplemental 204 test case defined.

bsardo · 2025-01-31T02:39:16Z

adapters/smoot/smoottest/supplemental/invalid-bidder.json

+  "mockBidRequest": {
+    "id": "test-request-id",
+    "imp": [
+      {
+        "id": "test-imp-id",
+        "ext": {
+          "bidder": 999
+        }
+      }
+    ]
+  },


Nitpick: this gets the job done as far as forcing the error path. I suggest making this a stripped down valid mockBidRequest with just the imp[0].ext.bidder being invalid:

"mockBidRequest": { "id": "test-request-id", "imp": [ { "id": "test-imp-id", "banner": { "format": [ { "w": 300, "h": 250 } ] }, "ext": { "bidder": { "placementId": 999 } } } ], "app": { "id": "1", "bundle": "com.wls.testwlsapplication" } },

bsardo · 2025-01-31T14:21:04Z

adapters/smoot/smoottest/supplemental/invalid-ext.json

Nitpick: same suggestion as on invalid-bidder.json regarding minimum valid request with just the ext being invalid.

bsardo · 2025-01-31T14:26:43Z

Also can you please merge with master? One of the validate jobs is failing because your branch is off of a version of master that contains a PR that has since been reverted. The validate job should then start passing.

github-actions · 2025-02-03T06:27:35Z

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, e61e169

smoot

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:29:	Builder		100.0%
github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:36:	MakeRequests	86.7%
github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:86:	makeRequest	85.7%
github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:104:	MakeBids	100.0%
github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:141:	getBidType	100.0%
total:								(statements)	92.1%

leamarty · 2025-02-03T06:43:44Z

Thank you so much for your comments @bsardo ! We already went through them.

Please I'd ask you to have some speed on the remaining approval, since we've been waiting for 3 weeks already and we may get another "out of date" situation with the master branch.

Thanks!

bsardo

We're close. I have just two comments stemming from previous ones.

bsardo · 2025-02-05T19:34:26Z

adapters/smoot/smoottest/supplemental/invalid-ext.json

+  "mockBidRequest": {
+    "id": "test-request-id",
+    "imp": [
+      {
+        "id": "test-imp-id",
+        "banner": {
+          "format": [
+            {
+              "w": 300,
+              "h": 250
+            }
+          ]
+        },
+        "ext": 123
+      }
+    ]
+  },


Please add site or app to make this a valid request. This should suffice:

"app": { "id": "1", "bundle": "com.wls.testwlsapplication" }

Perfect, done ✅

bsardo · 2025-02-05T19:38:37Z

adapters/smoot/params_test.go

+	`{"placementId": 42}`,
+	`{"endpointId": 42}`,


Adding these two separate test cases instead of combining them into one test case like you have ({"placementId": "", "endpointId": ""} ensures that there is a min length constraint on each field instead of on at least one of the fields. I suggest making this change.

github-actions · 2025-02-06T15:12:54Z

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

smoot

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:29:	Builder		100.0%
github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:36:	MakeRequests	86.7%
github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:86:	makeRequest	85.7%
github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:104:	MakeBids	100.0%
github.com/prebid/prebid-server/v3/adapters/smoot/smoot.go:141:	getBidType	100.0%
total:								(statements)	92.1%

leamarty · 2025-02-06T15:56:33Z

Failed task says "Operation was cancelled". I guess we'll need a "retry"

leamarty · 2025-02-06T16:37:22Z

🥳 Thank you @bsardo @ccorbo !

New Adapter: Smoot

7c0f3cf

leamarty mentioned this pull request Jan 13, 2025

Smoot Adapter: pbs set to true prebid/prebid.github.io#5811

Open

8 tasks

bsardo added the adapter label Jan 14, 2025

ccorbo reviewed Jan 22, 2025

View reviewed changes

bsardo self-assigned this Jan 22, 2025

add test coverage for json unmarshal

4fefda1

leamarty requested a review from ccorbo January 23, 2025 08:35

ccorbo approved these changes Jan 27, 2025

View reviewed changes

bsardo reviewed Jan 31, 2025

View reviewed changes

improve testing coverage and update upstream

e61e169

leamarty requested a review from bsardo February 5, 2025 10:24

bsardo assigned guscarreon Feb 5, 2025

bsardo reviewed Feb 5, 2025

View reviewed changes

testing coverage placementId and endpointId

6ac536d

leamarty requested a review from bsardo February 6, 2025 15:12

bsardo approved these changes Feb 6, 2025

View reviewed changes

New Adapter: Smoot #4148

Are you sure you want to change the base?

New Adapter: Smoot #4148

Conversation

leamarty commented Jan 13, 2025

github-actions bot commented Jan 13, 2025

Code coverage summary

smoot

ccorbo commented Jan 17, 2025

leamarty commented Jan 20, 2025

ccorbo commented Jan 22, 2025

ccorbo commented Jan 22, 2025

leamarty commented Jan 22, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ccorbo commented Jan 22, 2025

ccorbo commented Jan 22, 2025

github-actions bot commented Jan 23, 2025

Code coverage summary

smoot

leamarty commented Jan 27, 2025

ccorbo left a comment

Choose a reason for hiding this comment

leamarty commented Jan 29, 2025

bsardo commented Jan 29, 2025

leamarty commented Jan 30, 2025

bsardo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsardo commented Jan 31, 2025

github-actions bot commented Feb 3, 2025

Code coverage summary

smoot

leamarty commented Feb 3, 2025

bsardo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Feb 6, 2025

Code coverage summary

smoot

leamarty commented Feb 6, 2025

leamarty commented Feb 6, 2025