Initial RhythmOne Prebid Server Adapter #704

dineshkumarjayaram · 2018-10-04T14:02:51Z

Initial RhythmOne Prebid Server Adapter support

dbemiller

Looks really solid overall.

Only one serious issue... the JSON schema should be more strict to validate incoming params better. All my other comments are just pointing out extra logic which you don't need because the Prebid Server core logic already handles it for you elsewhere.

dbemiller · 2018-10-05T17:20:11Z

adapters/rhythmone/rhythmone.go

+)
+
+type RhythmoneAdapter struct {
+	http     *adapters.HTTPAdapter


This field is unused... it can be deleted.

dbemiller · 2018-10-05T17:20:26Z

adapters/rhythmone/rhythmone.go

+	return "rhythmone"
+}
+
+func (a *RhythmoneAdapter) SkipNoCookies() bool {


Name() and SkipNoCookies() are legacy things... you don't need them.

dbemiller · 2018-10-05T17:23:08Z

adapters/rhythmone/rhythmone.go

+
+	validImpDataExists := false
+
+	for _, imp := range request.Imp {


You can delete this block. The adapters.EnforceBidderInfo() call in adapter_map makes sure your Imps have valid formats, according to your static/bidder-info/rhythmone.yaml definitions.

dbemiller · 2018-10-05T17:23:39Z

adapters/rhythmone/rhythmone.go

+		}
+	}
+
+	if !validImpDataExists {


Same here. This is guaranteed by the EnforceBidderInfo() stuff.

dbemiller · 2018-10-05T17:58:19Z

adapters/rhythmone/rhythmone.go

+		if imp.Audio != nil {
+			continue
+		}
+		if imp.Native != nil {


These two blocks can go away... audio/native aren't possible.

dbemiller · 2018-10-05T18:07:48Z

adapters/rhythmone/rhythmone.go

+	return mediaType
+}
+
+func NewRhythmoneAdapter(config *adapters.HTTPAdapterConfig, endpoint string) *RhythmoneAdapter {


This function is unused, and can be deleted.

dbemiller · 2018-10-05T18:19:17Z

exchange/adapter_map.go

@@ -61,5 +62,6 @@ func newAdapterMap(client *http.Client, cfg *config.Configuration, infos adapter
 			infos[string(openrtb_ext.BidderRubicon)]), client),
 		openrtb_ext.BidderSomoaudience: adaptBidder(adapters.EnforceBidderInfo(somoaudience.NewSomoaudienceBidder(cfg.Adapters[string(openrtb_ext.BidderSomoaudience)].Endpoint), infos[string(openrtb_ext.BidderSomoaudience)]), client),
 		openrtb_ext.BidderSovrn:        adaptBidder(adapters.EnforceBidderInfo(sovrn.NewSovrnBidder(client, cfg.Adapters[string(openrtb_ext.BidderSovrn)].Endpoint), infos[string(openrtb_ext.BidderSovrn)]), client),
+		openrtb_ext.BidderRhythmone:    adaptBidder(adapters.EnforceBidderInfo(rhythmone.NewRhythmoneBidder(client, cfg.Adapters[string(openrtb_ext.BidderRhythmone)].Endpoint), infos[string(openrtb_ext.BidderRhythmone)]), client),


Alphabetize by key please.

dbemiller · 2018-10-05T18:19:32Z

openrtb_ext/bidders.go

@@ -37,6 +37,7 @@ const (
 	BidderRubicon      BidderName = "rubicon"
 	BidderSomoaudience BidderName = "somoaudience"
 	BidderSovrn        BidderName = "sovrn"
+	BidderRhythmone    BidderName = "rhythmone"


alphabetize by key please

dbemiller · 2018-10-05T18:25:02Z

usersync/usersyncers/rhythmone_test.go

+	an := NewRhythmoneSyncer("https://sync.1rx.io/usersync2/rmphb?redir=", "localhost")
+	syncInfo := an.GetUsersyncInfo("1", "BOPVK28OVJoTBABABAENBs-AAAAhuAKAANAAoACwAGgAPAAxAB0AHgAQAAiABOADkA")
+	url := "https://sync.1rx.io/usersync2/rmphb?redir=localhost%2Fsetuid%3Fbidder%3Drhythmone%26gdpr%3D1%26gdpr_consent%3DBOPVK28OVJoTBABABAENBs-AAAAhuAKAANAAoACwAGgAPAAxAB0AHgAQAAiABOADkA%26uid%3D%5BRX_UUID%5D"
+	assertStringsMatch(t, url, syncInfo.URL)


Use testify for cleaner assertions: https://godoc.org/github.com/stretchr/testify/assert

assert.Equal() should do the trick here.

openrtb_ext/imp.go

Rajeshk08 · 2018-10-10T13:55:35Z

Updated code review comments as per dbemiller.
Please review

dbemiller

One last unused param you should remove... otherwise LGTM

dbemiller · 2018-10-10T17:24:41Z

adapters/rhythmone/rhythmone.go

+	return mediaType
+}
+
+func NewRhythmoneBidder(client *http.Client, endpoint string) *RhythmoneAdapter {


client is unused... please remove it.

…er in NewRhythmoneBidder method

dineshkumarjayaram · 2018-10-11T10:15:41Z

Review comments incorporated, please review.

hhhjort · 2018-10-11T13:50:41Z

adapters/rhythmone/rhythmone.go

+func (a *RhythmoneAdapter) preProcess(req *openrtb.BidRequest, errors []error) (*openrtb.BidRequest, string, []error) {
+	numRequests := len(req.Imp)
+
+	requestImpCopy := req.Imp


Since req.Imp is a slice, and a slice is essentially a pointer to an existing array structure, requestImpCopy := req.Imp will end up referencing the same underlying array as req.Imp So it is not a true copy.

You only seem to use reqestImpCopy for imp := requestImpCopy[i], which will make imp a copy shallow of the original imp. So there does not seem to be a reason to define reqestImpCopy.

adapters/rhythmone/rhythmone.go

adapters/rhythmone/params_test.go

dineshkumarjayaram · 2018-10-11T16:53:54Z

@hhhjort - Review comments incorporated, please review.

hhhjort · 2018-10-15T14:36:15Z

We have merge conflicts now

hhhjort · 2018-10-15T16:07:53Z

DISREGARD

Ack, looks like an older PR that just merged is what actually caused the conflicts, and the change shouldn't have merged. Not sure why git merged it that way, but we should be keeping EnforceBidderInfo Will need to git that fixed, sorry for the confusion.

hhhjort · 2018-10-15T16:20:00Z

Never mind the last comment, that change was a valid part of that PR, so it is needed. I just got a little confused.

mkendall07

LGTM once merge conflicts are resolved.

* Initial RhythmOne Prebid Server Adapter * Updated UserSync URL * Fixed gofmt issue * Code review comments updated, UserSync URL updated * Updated code based on review comments - Removed unused client parameter in NewRhythmoneBidder method * Incorporated review comments * Updated adapter with resolved conflict

Dinesh Kumar Jayaram added 3 commits October 4, 2018 19:28

Initial RhythmOne Prebid Server Adapter

fbe205b

Updated UserSync URL

e79e61b

Fixed gofmt issue

c8c33d5

dbemiller reviewed Oct 5, 2018

View reviewed changes

Code review comments updated, UserSync URL updated

e13ed9a

dbemiller reviewed Oct 10, 2018

View reviewed changes

dbemiller assigned hhhjort Oct 10, 2018

dbemiller requested a review from hhhjort October 10, 2018 17:35

Updated code based on review comments - Removed unused client paramet…

deb60d2

…er in NewRhythmoneBidder method

hhhjort reviewed Oct 11, 2018

View reviewed changes

adapters/rhythmone/rhythmone.go Show resolved Hide resolved

hhhjort reviewed Oct 11, 2018

View reviewed changes

adapters/rhythmone/params_test.go Show resolved Hide resolved

Incorporated review comments

1eadb45

dbemiller previously approved these changes Oct 12, 2018

View reviewed changes

hhhjort previously approved these changes Oct 12, 2018

View reviewed changes

Rajeshk08 previously approved these changes Oct 15, 2018

View reviewed changes

Updated adapter with resolved conflict

35beb9e

dineshkumarjayaram dismissed stale reviews from Rajeshk08, hhhjort, and dbemiller via 35beb9e October 15, 2018 15:34

hhhjort approved these changes Oct 15, 2018

View reviewed changes

hhhjort requested a review from mkendall07 October 15, 2018 16:23

hhhjort assigned mkendall07 Oct 15, 2018

mkendall07 approved these changes Oct 15, 2018

View reviewed changes

Merge branch 'master' into master

f9d02e0

hhhjort merged commit 7be7989 into prebid:master Oct 15, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial RhythmOne Prebid Server Adapter #704

Initial RhythmOne Prebid Server Adapter #704

dineshkumarjayaram commented Oct 4, 2018

dbemiller left a comment

dbemiller Oct 5, 2018

dbemiller Oct 5, 2018

dbemiller Oct 5, 2018

dbemiller Oct 5, 2018

dbemiller Oct 5, 2018

dbemiller Oct 5, 2018

dbemiller Oct 5, 2018

dbemiller Oct 5, 2018

dbemiller Oct 5, 2018

Rajeshk08 commented Oct 10, 2018

dbemiller left a comment

dbemiller Oct 10, 2018

dineshkumarjayaram commented Oct 11, 2018

hhhjort Oct 11, 2018

dineshkumarjayaram commented Oct 11, 2018

hhhjort commented Oct 15, 2018

hhhjort commented Oct 15, 2018 •

edited

Loading

hhhjort commented Oct 15, 2018

mkendall07 left a comment


		validImpDataExists := false

		for _, imp := range request.Imp {

Initial RhythmOne Prebid Server Adapter #704

Initial RhythmOne Prebid Server Adapter #704

Conversation

dineshkumarjayaram commented Oct 4, 2018

dbemiller 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

Rajeshk08 commented Oct 10, 2018

dbemiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dineshkumarjayaram commented Oct 11, 2018

Choose a reason for hiding this comment

dineshkumarjayaram commented Oct 11, 2018

hhhjort commented Oct 15, 2018

hhhjort commented Oct 15, 2018 • edited Loading

hhhjort commented Oct 15, 2018

mkendall07 left a comment

Choose a reason for hiding this comment

hhhjort commented Oct 15, 2018 •

edited

Loading