engagebdr adapter #966

yannaingkyaw1986 · 2019-07-17T20:52:33Z

No description provided.

guscarreon

Can you please address the issues with the Travis build?

yannaingkyaw1986 · 2019-07-17T22:24:56Z

yes. we are working on it.

On Wed, Jul 17, 2019 at 3:24 PM guscarreon ***@***.***> wrote: ***@***.**** requested changes on this pull request. Can you please address the issues with the Travis build? — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#966?email_source=notifications&email_token=ACLJ7J7MQKIWWCFH7EYPBC3P76LYHA5CNFSM4IEUNCOKYY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOB6ZF6NA#pullrequestreview-263348020>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACLJ7J2OBNTZ4VWUCZZKVX3P76LYHANCNFSM4IEUNCOA> .

mansinahar · 2019-07-19T13:32:04Z

adapters/engagebdr/engagebdr.go

+	"encoding/json"
+	"github.com/prebid/prebid-server/openrtb_ext"
+	"net/http"
+	//"regexp"


Can you please remove this?

adapters/engagebdr/usersync_test.go

mansinahar · 2019-07-19T13:54:56Z

config/config.go

@@ -613,6 +613,7 @@ func SetupViper(v *viper.Viper, filename string) {
 	v.SetDefault("adapters.mgid.endpoint", "https://prebid.mgid.com/prebid/")
 	v.SetDefault("adapters.visx.endpoint", "https://t.visx.net/s2s_bid?wrapperType=s2s_prebid_standard")
 	v.SetDefault("adapters.tappx.endpoint", "https://{{.Host}}")
+	v.SetDefault("adapters.engagebdr.endpoint", "http://dsp.bnmla.com/hb")


You should also set a default usersync endpoint in the setDerivedDefaults function on L419

mansinahar · 2019-07-19T14:16:21Z

adapters/engagebdr/engagebdr.go

+		}
+		sspidImps[impExt.Sspid] = append(sspidImps[impExt.Sspid], imp)
+	}
+


Just a suggestion: Since you're adapter only supports banner and video ads, might be a good idea to add a check somewhere here in this function to discard imps of any other type. I am sure this must be something that you'd be already handling on your end but it's always better to check and fail early.

mansinahar · 2019-07-24T14:11:16Z

@yannaingkyaw1986 pinging to see if you have had the chance to address the above comments

yannaingkyaw1986 · 2019-07-24T15:06:04Z

yes. we will commit the update today.

…

On Wed, Jul 24, 2019 at 7:11 AM Mansi Nahar ***@***.***> wrote: @yannaingkyaw1986 <https://github.com/yannaingkyaw1986> pinging to see if you have had the chance to address the above comments — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#966?email_source=notifications&email_token=ACLJ7J7ZCVQR3WZ6I5I2X6LQBBPIPA5CNFSM4IEUNCOKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOD2WOVIY#issuecomment-514648739>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ACLJ7J55OCCWHMKVUZ6QYRLQBBPIPANCNFSM4IEUNCOA> .

mansinahar · 2019-07-24T17:45:56Z

Cool. Thank you @yannaingkyaw1986!

mansinahar · 2019-07-29T13:48:54Z

adapters/engagebdr/engagebdr.go

+	// EngageBDR uses different sspid parameters for banner and video.
+	sspidImps := make(map[string][]openrtb.Imp)
+	for _, imp := range request.Imp {
+		if imp.Native != nil {


Should this if condition be probably changed to if imp.Native != nil && imp.Site == nil && imp.App == nil in case of possible multiformat requests?

mansinahar · 2019-07-29T15:09:26Z

LGTM other than the minor comment mentioned above

hhhjort · 2019-07-30T18:07:09Z

exchange/adapter_map.go

@@ -6,7 +6,7 @@ import (
 	"strings"

 	"github.com/prebid/prebid-server/adapters"
-	ttx "github.com/prebid/prebid-server/adapters/33across"
+	"github.com/prebid/prebid-server/adapters/33across"


Need to keep the ttx part or you will break the line below that access the adapter by this alias

hhhjort · 2019-07-30T18:07:56Z

usersync/usersyncers/syncer.go

@@ -5,7 +5,7 @@ import (
 	"text/template"

 	"github.com/golang/glog"
-	ttx "github.com/prebid/prebid-server/adapters/33across"


again please don't blindly remove these aliases.

guscarreon

Reviewed the source code and left a few comments. Also, we probably should include more test cases in the adapters/engagebdr/engagebdrtest/exemplary/, adapters/engagebdr/engagebdrtest/supplemental/, and adapters/engagebdr/engagebdrtest/params/race/ directories to make percent of code coverage.

╰─➤  go test -coverprofile=c.out
PASS
coverage: 64.3% of statements
╰─➤  go tool cover -func=c.out
github.com/prebid/prebid-server/adapters/engagebdr/engagebdr.go:20:     MakeRequests            75.0%
github.com/prebid/prebid-server/adapters/engagebdr/engagebdr.go:85:     MakeBids                64.3%
github.com/prebid/prebid-server/adapters/engagebdr/engagebdr.go:120:    getMediaTypeForImp      0.0%
github.com/prebid/prebid-server/adapters/engagebdr/engagebdr.go:133:    NewEngageBDRBidder      100.0%
github.com/prebid/prebid-server/adapters/engagebdr/usersync.go:9:       NewEngageBDRSyncer      100.0%
total:                                                                  (statements)            64.3%

guscarreon · 2019-08-05T19:03:40Z

adapters/engagebdr/engagebdr.go

+		if err := json.Unmarshal(imp.Ext, &bidderExt); err != nil {
+			errors = append(errors, &errortypes.BadInput{
+				Message: fmt.Sprintf("Ignoring imp id=%s, error while decoding extImpBidder, err: %s", imp.ID, err),
+			})


Would a continue statement make sense after this line? If we have an error for some reason, I believe the following if statement found in line 39 will most likely also print an error (if not an NULL pointer de-reference in line 38) and we'll end up with a stack of errors that have one common source.

Breakpoint 1 set at 0x1626558 for github.com/prebid/prebid-server/adapters/engagebdr.(*EngageBDRAdapter).MakeRequests() ./engagebdr.go:32 (dlv) c > github.com/prebid/prebid-server/adapters/engagebdr.(*EngageBDRAdapter).MakeRequests() ./engagebdr.go:32 (hits goroutine(21):1 total:1) (PC: 0x1626558) 27: if imp.Native != nil { 28: // filter native imps from bid request 29: continue 30: } 31: var bidderExt adapters.ExtImpBidder => 32: if err := json.Unmarshal(imp.Ext, &bidderExt); err != nil { 33: errors = append(errors, &errortypes.BadInput{ 34: Message: fmt.Sprintf("Ignoring imp id=%s, error while decoding extImpBidder, err: %s", imp.ID, err), 35: }) 36: } 37: impExt := openrtb_ext.ExtImpEngageBDR{} 38: err := json.Unmarshal(bidderExt.Bidder, &impExt) 39: if err != nil { (dlv) p bidderExt.Bidder encoding/json.RawMessage len: 0, cap: 0, nil

guscarreon · 2019-08-05T19:05:52Z

adapters/engagebdr/engagebdr.go

+		if err != nil {
+			errors = append(errors, &errortypes.BadInput{
+				Message: fmt.Sprintf("Ignoring imp id=%s, error while decoding impExt, err: %s", imp.ID, err),
+			})


Same as line 35 above. Seems like a continue statement would be useful because we'll de-reference impExt in line 44 and there's a possibility that the Unmarshal didn't go well by then.

guscarreon · 2019-08-05T19:08:19Z

adapters/engagebdr/engagebdr.go

+		if impExt.Sspid == "" {
+			errors = append(errors, &errortypes.BadInput{
+				Message: fmt.Sprintf("Ignoring imp id=%s, no sspid present", imp.ID),
+			})


If we don't put a continue statement here, we'd be mapping an empty impExt.Sspid value into sspidImps

guscarreon · 2019-08-05T19:14:20Z

adapters/engagebdr/engagebdr.go

+	}
+
+	if len(adapterRequests) == 0 {
+		errors = append(errors, &errortypes.BadInput{Message: fmt.Sprintf("No imps present")})


Isn't this error message a little bit misleading? There's a possibility that Imp objects were actually present in the request.Imp array but all of them were invalid

guscarreon · 2019-08-06T01:34:04Z

adapters/engagebdr/engagebdr.go

+	mediaType := openrtb_ext.BidTypeBanner
+	for _, imp := range imps {
+		if imp.ID == impId {
+			if imp.Banner == nil && imp.Video != nil {


The Imp object also has an Audio and Native fields which are the other two data types supported by prebid-server but this particular adapter, according to static/bidder-info/engagebdr.yaml
does not support them. Therefore, the following scenarios might set the mediaType to openrtb_ext.BidTypeBanner. Is the behavior we want? Maybe we want to modify the if statement

Imp[i].Banner | Imp[i].Video | Imp[i].Audio | Imp[i].Native | mediaType --------------|--------------|--------------|----------------|------------- nil | nil | not nil | nil |openrtb_ext.BidTypeBanner nil | nil | nil | nil |openrtb_ext.BidTypeBanner

Meant to request changes, not approve. M bad

guscarreon

Sorry, meant to request changes. All the requests are listed in the comments above.

mansinahar

I'd suggest adding some additional tests with multiple imps and adding an example too, if possible.

adapters/engagebdr/engagebdr.go

adapters/engagebdr/params_test.go

config/config.go

static/bidder-info/engagebdr.yaml

adapters/engagebdr/engagebdrtest/supplemental/multi-banner-video.json

mansinahar · 2019-08-22T19:28:17Z

@yannaingkyaw1986 LGTM in general but there are couple minor comments from the last review that are still unaddressed. If you can please address them when you have a chance, we can get this merged.

stale · 2019-09-02T19:27:39Z

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

guscarreon

LGTM

Yan Naing Kyaw added 2 commits July 17, 2019 13:27

engagebdr adapter

49229ce

engagebdr-ext params

0289413

guscarreon requested changes Jul 17, 2019

View reviewed changes

added exemplary

9cd0251

mansinahar self-assigned this Jul 19, 2019

mansinahar reviewed Jul 19, 2019

View reviewed changes

engagebdr adapter updated

8f56e29

Yan Naing Kyaw added 2 commits July 24, 2019 15:43

update engagebdr adapter usersync

0b7abc7

update engagebdr usersync fix

e8aeb8d

mansinahar reviewed Jul 29, 2019

View reviewed changes

mansinahar requested a review from hhhjort July 29, 2019 15:47

mansinahar assigned hhhjort Jul 29, 2019

hhhjort reviewed Jul 30, 2019

View reviewed changes

revert ttx back

6128fef

hhhjort previously approved these changes Aug 1, 2019

View reviewed changes

update merge file

852ce0c

yannaingkyaw1986 dismissed hhhjort’s stale review via 852ce0c August 2, 2019 20:06

Merge branch 'master' into engagebdr-adapter

a79774f

guscarreon previously approved these changes Aug 6, 2019

View reviewed changes

guscarreon requested changes Aug 6, 2019

View reviewed changes

update supplemental and update on engagebdr.go

722678e

mansinahar reviewed Aug 14, 2019

View reviewed changes

multi banner multi video test

129b3ac

mansinahar reviewed Aug 22, 2019

View reviewed changes

adapters/engagebdr/engagebdrtest/supplemental/multi-banner-video.json Outdated Show resolved Hide resolved

update 08/26

fcd3a54

stale bot added stale and removed stale labels Sep 2, 2019

delete multi banner video json

146defe

mansinahar approved these changes Sep 10, 2019

View reviewed changes

hhhjort approved these changes Sep 10, 2019

View reviewed changes

hhhjort assigned guscarreon Sep 10, 2019

guscarreon approved these changes Sep 11, 2019

View reviewed changes

mansinahar merged commit 692d6de into prebid:master Sep 11, 2019

mansinahar pushed a commit to mansinahar/prebid-server that referenced this pull request Nov 1, 2019

engagebdr adapter (prebid#966)

48b0b1e

katsuo5 pushed a commit to flux-dev-team/prebid-server-1 that referenced this pull request Dec 1, 2020

engagebdr adapter (prebid#966)

80a8541

katsuo5 pushed a commit to flux-dev-team/prebid-server-1 that referenced this pull request Dec 2, 2020

engagebdr adapter (prebid#966)

b7fdad8

katsuo5 pushed a commit to flux-dev-team/prebid-server-1 that referenced this pull request Dec 4, 2020

engagebdr adapter (prebid#966)

a4d508a

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

engagebdr adapter #966

engagebdr adapter #966

yannaingkyaw1986 commented Jul 17, 2019

guscarreon left a comment

yannaingkyaw1986 commented Jul 17, 2019 via email

mansinahar Jul 19, 2019

mansinahar Jul 19, 2019

mansinahar Jul 19, 2019 •

edited

Loading

mansinahar commented Jul 24, 2019

yannaingkyaw1986 commented Jul 24, 2019 via email

mansinahar commented Jul 24, 2019

mansinahar Jul 29, 2019

mansinahar commented Jul 29, 2019

hhhjort Jul 30, 2019

hhhjort Jul 30, 2019

guscarreon left a comment

guscarreon Aug 5, 2019

guscarreon Aug 5, 2019

guscarreon Aug 5, 2019

guscarreon Aug 5, 2019

guscarreon Aug 6, 2019

guscarreon left a comment

mansinahar left a comment

mansinahar commented Aug 22, 2019

stale bot commented Sep 2, 2019

guscarreon left a comment

engagebdr adapter #966

engagebdr adapter #966

Conversation

yannaingkyaw1986 commented Jul 17, 2019

guscarreon left a comment

Choose a reason for hiding this comment

yannaingkyaw1986 commented Jul 17, 2019 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mansinahar Jul 19, 2019 • edited Loading

Choose a reason for hiding this comment

mansinahar commented Jul 24, 2019

yannaingkyaw1986 commented Jul 24, 2019 via email

mansinahar commented Jul 24, 2019

Choose a reason for hiding this comment

mansinahar commented Jul 29, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

guscarreon 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

guscarreon left a comment

Choose a reason for hiding this comment

mansinahar left a comment

Choose a reason for hiding this comment

mansinahar commented Aug 22, 2019

stale bot commented Sep 2, 2019

guscarreon left a comment

Choose a reason for hiding this comment

mansinahar Jul 19, 2019 •

edited

Loading