New Adapter: Adnuntius #2014

mikael-lundin · 2021-09-24T13:29:58Z

Hi, this is a pull request for a new adapter that will be for adnuntius adserver. This is my first time using go, please be gentle and if there's anything you need just tell me and I'll provide. :)

Gdpr Added to adapter.

bsardo · 2021-09-28T18:21:07Z

Hi @mikael-lundin. Thanks for your submission. We'll get to this shortly but could you open up a docs PR here in the meantime?

mikael-lundin · 2021-10-01T09:07:44Z

I've added information about server side here. :)
prebid/prebid.github.io#3312

bsardo

@mikael-lundin Nice work; I left you some comments. Let me know if you have any questions.

static/bidder-info/adnuntius.yaml

adapters/adnuntius/params_test.go

adapters/adnuntius/adnuntius.go

adapters/adnuntius/adnuntiustest/exemplary/simple-banner.json

adapters/adnuntius/adnuntius.go

bsardo · 2021-10-04T16:06:49Z

adapters/adnuntius/adnuntius.go

+		if len(adunit.Ads) > 0 {
+			ad := adunit.Ads[0]
+
+			currency = ad.Bid.Currency


I'm curious about what you're doing with currency. Currency is set on the bidResponse. It looks like you iterate over all ad units and only process the first ad for an adunit. You're then setting the currency to that ad's bid currency. That means that the currency you select will be the currency of the last ad you process since you're assigning currency to bidResponse.currency on line 298 after you're done iterating over all adunits. This should be fine if it is guaranteed all ad bids will have currency set.

yeah, this is just me getting around how our ad server works. there will always be a currency set in the ad unit response.

adapters/adnuntius/adnuntiustest/exemplary/simple-banner.json

bsardo · 2021-10-04T16:19:58Z

adapters/adnuntius/adnuntius.go

+				domainArray := strings.Split(url, "/")
+				domain := strings.Replace(domainArray[2], "www.", "", -1)
+				adDomain = append(adDomain, domain)


Are the destination URLs always going to start with http(s)://?
Nitpick: maybe split on // and grab domainArray[1] instead? IMO that seems a little more straightforward.

Yeah, that seems like a good idea but I got the impression that i was only allowed to send the top level domain in that field? :D If not I can just stop what that stuff is doing and just pass the entire url. And yes, we force the url to have http(s)://

mikael-lundin · 2021-10-05T19:41:56Z

As a general note, I think I've managed to fix the issues mentioned in this pull request, please let me know if I'm missing anything. :)

VeronikaSolovei9 · 2021-10-12T06:27:01Z

adapters/adnuntius/adnuntius.go

+			errors = append(errors, &errortypes.BadInput{
+				Message: fmt.Sprintf("Error unmarshalling ExtImpBmtm: %s", err.Error()),
+			})
+		}


In case of error occurs, lines 128, 134 and 141 - should you just return an error or skip to next imp?
Edit: Similar to what you do in MakeBids

Hopefully I've done it as you meant it in my coming commit.

Thank you for the update.
Please check it one more time. You probably don't want to collect all errors. Instead return after first error occurrence. If so please modify return []error to just error and return it immediately.
Edit: Please check same thing in other functions

adapters/adnuntius/adnuntius.go

bsardo · 2021-10-11T14:58:00Z

static/bidder-info/adnuntius.yaml

@@ -1,9 +1,10 @@
 maintainer:


Please change the indentation in this file to 2 spaces instead of 4 to meet the recommended yaml spacing.

bsardo · 2021-10-11T15:03:24Z

adapters/adnuntius/adnuntius.go

 		}

-		var network string
+		network := "default"


Nitpick: How about making this a constant?

This is because it might change in the lines below, so making it a constant is not correct, I think? :) but then again my first go at golang, (pun intended) so i might be mistaking.

Sorry I mean define a constant at the top of the file (e.g. const defaultNetwork = "default")and assign it to network here.

bsardo · 2021-10-11T15:03:29Z

adapters/adnuntius/adnuntius.go

@@ -153,11 +156,9 @@ func (a *adapter) generateRequests(ortbRequest openrtb2.BidRequest) ([]*adapters
 			})
 	}

-	var site string
+	site := "unknown"


Nitpick: How about making this a constant?

Same as above, it might change further down.

bsardo · 2021-10-12T13:36:03Z

adapters/adnuntius/adnuntius.go

+		return "", []error{fmt.Errorf("failed to parse Adnuntius endpoint: %v", err)}
+	}
+
+	_, offset := a.tzo.Now().UTC().Local().Zone()


Thanks for injecting the clock when instantiating the adapter. This line though is still causing a problem when running the tests locally because of the call to Local() which will consult the TZ environment variable to determine the timezone.
Regardless, I think we can simplify this line to _, offset := a.tzo.Now().Zone() which will also fix this issue.
Now() returns the local timezone so whatever zone you specify when defining your fake time for testing purposes will be the local time.

Thanks, have changed this and really hope it works for you now! :D

bsardo · 2021-10-12T15:37:07Z

adapters/adnuntius/timezone.go

@@ -0,0 +1,23 @@
+package adnuntius


I think you can delete this file; I apologize for asking you to create it. Instead, would you please use what's defined in util/timeutil/time.go?
In the Builder function you should be able to instantiate like this:

bidder := &adapter{ time: &timeutil.RealTime{}, endpoint: config.Endpoint, }

And then in your test you can do this:

// FakeTime implements the Time interface type FakeTime struct { time time.Time } func (ft *FakeTime) Now() time.Time { return ft.time } func replaceRealTimeWithKnownTime(bidder adapters.Bidder) { bidderAdnuntius, _ := bidder.(*adapter) bidderAdnuntius.time = &FakeTime { time: time.Date(2016, 1, 1, 12, 30, 15, 0, time.UTC), } }

Please delete this file since it's not used anymore.

bsardo · 2021-10-12T15:42:08Z

adapters/adnuntius/adnuntius.go

+
+type QueryString map[string]string
+type adapter struct {
+	tzo      timezone


Nitpick: I suggest calling this something else like time or clock perhaps since it isn't a timezone but rather a time.

bsardo · 2021-10-12T15:45:28Z

adapters/adnuntius/adnuntius.go

+	}
+
+	_, offset := a.tzo.Now().UTC().Local().Zone()
+	tzo := -offset / 3600 * 60


Nitpick: how about making the denominator a constant?

bsardo · 2021-10-12T16:20:07Z

adapters/adnuntius/adnuntius.go

-		}}
-	}
-
-	if response.StatusCode != http.StatusOK {


I suggest keeping an error code catch-all in case something goes wrong on your server so PBS can handle it gracefully.

bsardo · 2021-10-12T16:45:59Z

adapters/adnuntius/adnuntiustest/supplemental/width-error.json

+                                        "currency": "NOK"
+                                    },
+                                    "adId": "adn-id-1559784094",
+                                    "creativeWidth": 980,


This is a valid scenario but you're not getting test coverage of lines if Werr != nil { and if len(adsErr) > 0 {. If you change this to a string that doesn't parse to an int (e.g. "ABC"), the call to ParseInt will return the error as Werr giving you the code coverage.

bsardo · 2021-10-12T16:46:33Z

adapters/adnuntius/adnuntiustest/supplemental/height-error.json

+                                    },
+                                    "adId": "adn-id-1559784094",
+                                    "creativeWidth": "980",
+                                    "creativeHeight": 240,


This is a valid scenario but you're not getting test coverage of lines if Herr != nil { and if len(adsErr) > 0 {. If you change this to a string that doesn't parse to an int (e.g. "ABC"), the call to ParseInt will return the error as Herr giving you the code coverage.

mikael-lundin · 2021-10-14T14:36:30Z

Done and done! :D

…

On Thu, Oct 14, 2021 at 3:46 PM Brian Sardo ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In adapters/adnuntius/adnuntius.go <#2014 (comment)>: > } - var network string + network := "default" Sorry I mean define a constant at the top of the file (e.g. const defaultNetwork = "default")and assign it to network here. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2014 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH64BIKELNYQYCN4I2RQJ3UG3NMZANCNFSM5EV7ACNA> .

mikael-lundin · 2021-10-14T16:48:34Z

I think it was me doing something crazy when rebasing on your master. :) that's why you see double stuff, since that deleted file made it back again. Anyways, Hopefully I've done all the asked for things. Please do feel free to scold me if I missunderstood the last comment.

bsardo · 2021-10-14T17:49:48Z

The code looks good. We just need to clean up the commits here before merging. One way to do this is to create a new branch locally with a tip at PBS master:head and then cherry pick your commits from this branch onto it so your commits are sitting on top of master. Then force push your new branch to this PR effectively rewriting the commit history here. The PR should then only show your ~11 commits (we shouldn't need the merge commit mixed in there). Let me know if you have any questions.

mikael-lundin · 2021-10-15T10:04:02Z

Let's hope I managed to do what you wrote! :D

bsardo

Thanks for cleaning up the commits. I have just one last small comment.

bsardo · 2021-10-15T16:12:26Z

adapters/adnuntius/adnuntius.go

+	}
+
+	if response.StatusCode != http.StatusOK {
+		return nil, []error{&errortypes.BadInput{


Can you change this one to errortypes.BadServerResponse please? BadInput is typically reserved for invalid input but this catch all should typically handle other types of unexpected errors.

Done and done!

VeronikaSolovei9 · 2021-10-17T03:09:33Z

Thank you for the updates, it looks good to me. I have a minor comment about errors return.

mikael-lundin · 2021-10-19T14:14:29Z

yeah, I think I changed them earlier to do something more in the lines of this: if response.StatusCode == http.StatusBadRequest { return nil, []error{&errortypes.BadInput{ Message: fmt.Sprintf("Status code: %d, Request malformed", response. StatusCode), }} } if response.StatusCode != http.StatusOK { return nil, []error{&errortypes.BadServerResponse{ Message: fmt.Sprintf("Status code: %d, Something went wrong with your request", response.StatusCode), }} }

…

On Sun, Oct 17, 2021 at 5:09 AM Veronika Solovei ***@***.***> wrote: Thank you for the updates, it looks good to me. I have a minor comment about errors return. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2014 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAH64BOTK6UYSJTYDDK4I5TUHI47RANCNFSM5EV7ACNA> .

bsardo

LGTM

VeronikaSolovei9 · 2021-10-20T16:39:46Z

adapters/adnuntius/adnuntius.go

+	if err != nil {
+		errs = append(errs, err...)
+	}
+	return extRequests, errs


This entire function can look like:

return a.generateRequests(*request)

aah! much better! :)

Thank you for addressing the comments :)

VeronikaSolovei9 · 2021-10-20T16:43:09Z

adapters/adnuntius/adnuntius.go

+
+	endpoint, err := makeEndpointUrl(ortbRequest, a)
+	if err != nil {
+		errors = append(errors, fmt.Errorf("failed to parse URL: %v", err))


Should it be return here instead?

Yes, it should, thanks! :D

VeronikaSolovei9 · 2021-10-20T16:46:58Z

adapters/adnuntius/adnuntius.go

+
+		if len(errors) > 0 {
+			return nil, errors
+		}


Looks like it will be never more than one error in this list. Please consider to remove var errors []error and return error right after the occurrence.
Lines 189-191 can be removed, use Line 190 instead of 186

Thanks for that, it makes much more sense

VeronikaSolovei9 · 2021-10-20T16:48:49Z

adapters/adnuntius/adnuntius.go

+
+			creativeHeight, heightErr := strconv.ParseInt(ad.CreativeHeight, 10, 64)
+			if heightErr != nil {
+				adsErr = append(adsErr, heightErr)


Do you need to collect errors here? (Lines 270 and 275)

no, I can just return them as I did above.

VeronikaSolovei9 · 2021-10-20T16:50:04Z

adapters/adnuntius/adnuntius.go

+				adDomain = append(adDomain, domain)
+			}
+
+			if len(adsErr) > 0 {


Nitpick: Should you move it to Line 277?

mansinahar · 2021-10-25T17:56:35Z

@mikael-lundin Just a friendly reminder about the comments above

mikael-lundin · 2021-10-25T18:14:35Z

Thanks!! Will adress these

mikael-lundin · 2021-10-28T10:57:27Z

Now, all the comments above should be fixed, sorry for missing them in the first place. :(

bsardo

Looks good, just one last minor comment.

bsardo · 2021-10-28T14:46:39Z

adapters/adnuntius/adnuntius.go

+		return nil, []error{&errortypes.BadServerResponse{
+			Message: fmt.Sprintf("failed to parse URL: %s", err),
+		}}


I think it would make more sense if this was BadInput since generating the URL endpoint involves unmarshaling various parts of the request (body or query string) and validating it.

bsardo

LGTM

VeronikaSolovei9

LGTM

mikael-lundin and others added 7 commits September 13, 2021 13:55

Making adnuntius bidder working.

90adf68

Testing that container works.

8b5bb30

Tests work but fail.

2ef05d3

Tzo added to the request.

57f824e

Tests for adnuntius adapter.

ef6d3b7

Gdpr Added to adapter.

c21586a

Merge pull request #1 from Adnuntius/consent

b5d7833

Gdpr Added to adapter.

mansinahar requested review from bsardo and VeronikaSolovei9 September 27, 2021 17:38

mansinahar assigned bsardo and VeronikaSolovei9 Sep 27, 2021

bsardo reviewed Oct 4, 2021

View reviewed changes

Fixing commented issues in pull request.

2825270

VeronikaSolovei9 reviewed Oct 12, 2021

View reviewed changes

adapters/adnuntius/adnuntius.go Outdated Show resolved Hide resolved

bsardo reviewed Oct 12, 2021

View reviewed changes

mikael-lundin added 10 commits October 14, 2021 12:30

Making adnuntius bidder working.

9444bff

Testing that container works.

82ddf79

Tests work but fail.

524d0b3

Tzo added to the request.

41bf3ff

Tests for adnuntius adapter.

cbddd69

Gdpr Added to adapter.

25fefef

Fixing commented issues in pull request.

a651cce

Changes made in response to comments in pull request.

6c5fe81

Merge branch 'master' of github.com:Adnuntius/prebid-server

176ef4b

Adding constants.

e4a5700

Adding handling of 400 error, removing timezone file.

2f3b0b7

mikael-lundin force-pushed the master branch 2 times, most recently from b21dd42 to 2f3b0b7 Compare October 15, 2021 10:03

bsardo reviewed Oct 15, 2021

View reviewed changes

Changed from errortypes.BadInput to errortypes.BadServerResponse.

f1682e1

bsardo previously approved these changes Oct 19, 2021

View reviewed changes

VeronikaSolovei9 reviewed Oct 20, 2021

View reviewed changes

mikael-lundin added 2 commits October 28, 2021 10:22

Merge remote-tracking branch 'upstream/master'

0426a7e

Made changes based on comments

1aedd36

mikael-lundin dismissed bsardo’s stale review via 1aedd36 October 28, 2021 10:56

bsardo reviewed Oct 28, 2021

View reviewed changes

Changed error type to Bad input for url-parsing.

5838ca3

bsardo approved these changes Oct 29, 2021

View reviewed changes

VeronikaSolovei9 approved these changes Oct 29, 2021

View reviewed changes

bsardo merged commit 56e11d0 into prebid:master Oct 29, 2021

mansinahar pushed a commit to mansinahar/prebid-server that referenced this pull request Jan 18, 2022

New Adapter: Adnuntius (prebid#2014)

59e309e

jorgeluisrocha pushed a commit to jwplayer/prebid-server that referenced this pull request Sep 28, 2022

New Adapter: Adnuntius (prebid#2014)

7ce34fc

shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022

New Adapter: Adnuntius (prebid#2014)

23a40e1

New Adapter: Adnuntius #2014

New Adapter: Adnuntius #2014

Conversation

mikael-lundin commented Sep 24, 2021

bsardo commented Sep 28, 2021

mikael-lundin commented Oct 1, 2021

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

mikael-lundin commented Oct 5, 2021

VeronikaSolovei9 Oct 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

VeronikaSolovei9 Oct 17, 2021 • edited Loading

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

mikael-lundin commented Oct 14, 2021 via email

mikael-lundin commented Oct 14, 2021

bsardo commented Oct 14, 2021

mikael-lundin commented Oct 15, 2021

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

VeronikaSolovei9 commented Oct 17, 2021

mikael-lundin commented Oct 19, 2021 via email

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

mansinahar commented Oct 25, 2021

mikael-lundin commented Oct 25, 2021

mikael-lundin commented Oct 28, 2021

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

bsardo left a comment

Choose a reason for hiding this comment

VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

VeronikaSolovei9 Oct 12, 2021 •

edited

Loading

VeronikaSolovei9 Oct 17, 2021 •

edited

Loading