Beachfront - Currency conversion #2078

muncha · 2021-11-10T13:38:58Z

This handles currency conversion on imp.bidfloor, corrects and expands on the logic of selecting between imp.bidfloor and ext.bidfloor, and when to apply the minimum. Testing is based on the Rubicon TestOpenRTBRequestWithDifferentBidFloorAttributes() method.

…it, etc. Gets the structure of that process ready to plug in currency conversion.

…turns are being tested.

…ly good.

AlexBVolcy

The code updates and the test coverage from these updates looks good! I'm going to wait for the review of @guscarreon to see if I missed anything.

AlexBVolcy · 2021-11-11T16:54:48Z

adapters/beachfront/beachfront.go

@@ -88,8 +88,8 @@ type beachfrontSlot struct {
 }

 type beachfrontSize struct {
-	W uint64 `json:"w"`
-	H uint64 `json:"h"`
+	W int `json:"w"`


I'm curious about why this was updated from uint64 to int?

I was getting a weird error from the test. It was somehow getting converted to hex. Very weird. When I changed it to int the error went away. I'm sure it was not the actual problem, but just got around it somehow, but it appears to be something that got actually fixed along the way. I can not reproduce the error. Thanks for pointing it out. I just changed it back, as it should be unsigned.

Got it thanks for updating! I'll look over this again today.

…le while messing with pbs.yaml, so deleted that.

guscarreon

Looks good to me @muncha. Just one more ask: Now that pull request #2071 has been merged, JSON tests with currency conversion are now supported. Can you convert TestRequestWithDifferentBidFloorAttributes to JSON tests?
Currency conversion rates get read from the request.ext.prebid.currency.rates field as shown below:

  "mockBidRequest": {
    "id": "test-request-id",
       .
       .
    "imp": [
       .
       .
    ],
    "ext": {
      "prebid": {
        "currency": {
          "rates": {
            "MXN": {
              "USD": 0.05,
              "EUR": 0.03
            }
            "EUR": {
              "USD": 1.12
            }
              .
              .
          },
          "usepbsrates": false
        }
      }
    }
  }

muncha · 2021-11-22T23:10:15Z

@guscarreon FYI - I'm working on switching it to JSON, I'm hitting a panic on the currency conversion and trying to get to the bottom of that.

guscarreon · 2021-11-23T04:00:14Z

@guscarreon FYI - I'm working on switching it to JSON, I'm hitting a panic on the currency conversion and trying to get to the bottom of that.

Thank's @muncha. Currency tests will panic if the recent currency conversion test functionality recently merged into master is not in place. Maybe updating your local branch will latest master could probably solve it.

muncha · 2021-11-23T09:55:40Z

That was the problem. I thought I had that already merged to beachfront's master. I had not.

guscarreon

Looks pretty good, just have a couple of questions:

guscarreon · 2021-11-23T17:40:05Z

adapters/beachfront/beachfront.go

+	var err error
+
+	if imp.BidFloorCur != "" && strings.ToUpper(imp.BidFloorCur) != "USD" {
+		imp.BidFloor, err = reqInfo.ConvertCurrency(imp.BidFloor, imp.BidFloorCur, "USD")


What if imp.BidFloor = 0 and imp.BidFloorCur == "foreignCurrency"? Should we also check that imp.BidFloor != 0 before making the call to localReqInfo.ConvertCurrency(imp.BidFloor, imp.BidFloorCur, "USD")?

guscarreon · 2021-11-23T17:41:48Z

adapters/beachfront/beachfront.go

+		}
+	}
+
+	if imp.BidFloor > ext.BidFloor {


Is there a possibility that both imp.BidFloor and ext.BidFloor come in a non-USD currency? If so, imp.BidFloor has already been converted from a foreign currency at this point, should we convert ext.BidFloor if needed?

guscarreon · 2021-11-23T17:49:58Z

adapters/beachfront/beachfront.go

+	if floor > minBidFloor {
+		imp.BidFloorCur = "USD"
+	} else {
+		imp.BidFloorCur = ""


if we are positive ext.BidFloor won't ever come in a foreign currency, maybe we could get rid of this if/else statement entirely if we slightly modify the if/else on line 599 above:

598 + imp.BidFloorCur = "USD" 599 if imp.BidFloor > ext.BidFloor { 600 floor = imp.BidFloor 601 } else if ext.BidFloor > 0 { 602 floor = ext.BidFloor 603 } else { 604 floor = 0 + imp.BidFloorCur = "" 605 } 606 607 - if floor > minBidFloor { 608 - imp.BidFloorCur = "USD" 609 - } else { 610 - imp.BidFloorCur = "" 611 - floor = 0 612 - } 613 - 614 imp.BidFloor = floor 615 return false, nil 616 } adapters/beachfront/beachfront.go

…rrency conversion, but it is below the minimum

guscarreon · 2021-11-29T16:26:05Z

adapters/beachfront/beachfront.go

 		}
 	}

-	if floor <= minBidFloor {
-		floor = 0
+	if imp.BidFloor < ext.BidFloor {


When execution gets here, there's a possibility that imp.BidFloor has already been converted from a foreign currency to "USD". Are we positive that ext.BidFloor comes in "USD" and not the same foreign currency imp.BidFloor was?

For instance: say a given request comes with both imp.BidFloor and ext.BidFloor set to "EUR". The comparisson here would be misleading because we could be comparing imp.BidFloor representing a price in US dollars versus ext.BidFloor representing Euros. Is this a feasible possibility? Can ext.BidFloor come in a non "USD" currency?

@guscarreon
ext.bidfloor is always is USD. So in the case where imp.bidfloor has been converted to USD from EUR, the ext.bidfloor is not converted (it is never converted). Note the comparison at 577. This also assumes that ext.bidfloor is USD, as is minBidFloor (which beachfront may one day decide should be more than $0.01)

If imp.bidfloorcur = INR, imp.bidfloor= 375.23, and ext.bidfloor = 310.21 (erroneously assumed to be in INR) , then ext.bidfloor will be used, as the converted value of imp.bidfloor = 5.00 USD is less than ext.bidfloor = 310.21 USD, which was entered in INR due to user error, and someone's not getting any ads. I verified this with beachfront, and I agree, it's a bit counter intuitive, but it is correct.

I need to update the docs as they still say the converter doesn't work, but this bit is in there as of my previous PR. I'll push that in a couple minutes.

Thanks for clarifying @muncha

AlexBVolcy

This looks good to me! Thanks for being so responsive to feedback!

guscarreon

LGTM. @muncha thanks for clarifying and addressing our feedback

* 'master' of https://github.com/wwwyyy/prebid-server: (23 commits) Add GPID Reserved Bidder Name (prebid#2107) Marsmedia adapter - param zoneId can get string and int (prebid#2105) Algorix: add Server Region Support (prebid#2096) New Adapter: Bizzclick (prebid#2056) Collect Prometheus Go process metrics (prebid#2101) Added channel support (prebid#2037) Rubicon: update fpd fields resolving (prebid#2091) Beachfront - Currency conversion (prebid#2078) New Adapter: Groupm (Alias Of PubMatic) (prebid#2042) Adform adapter lacked gross/net parameter support (prebid#2084) add iframe sync for openx (prebid#2094) changes to the correct user sync url (prebid#2092) Smaato: Add instl to tests (prebid#2089) Correct MakeRequests() output assertion in adapter JSON tests (prebid#2087) Adview: adapter currency fix. (prebid#2060) New Adapter: Coinzilla (prebid#2074) Syncer Level ExternalURL Override (prebid#2072) 33Across: Enable Support for SRA requests (prebid#2079) Currency conversion on adapter JSON tests (prebid#2071) New Adapter: VideoByte (prebid#2058) ...

Jim Naumann added 8 commits November 8, 2021 19:01

this gets the logic straight for which bidfloor to use, when to zero …

2133bb0

…it, etc. Gets the structure of that process ready to plug in currency conversion.

this gets a banner test working for currency conversion

66f7b59

got a video test working

461b468

combined tests

8d7af83

this gets 3 significant test run with both banner and video. error re…

6354b1c

…turns are being tested.

removed redundant JSON tests for bidfloor and a bit of clean up. most…

ba6c21b

…ly good.

calling it 1.0.0

adaf581

and then correct the tests

652ead5

mansinahar requested review from AlexBVolcy and guscarreon November 10, 2021 18:59

mansinahar assigned guscarreon and AlexBVolcy Nov 10, 2021

removed a comment block

e67371b

AlexBVolcy reviewed Nov 11, 2021

View reviewed changes

Jim Naumann added 2 commits November 11, 2021 17:03

removed a redundant test. also I seem to have created an analytics fi…

c4724cd

…le while messing with pbs.yaml, so deleted that.

changed beachfrontsize back to uint64.

b63bd90

AlexBVolcy previously approved these changes Nov 16, 2021

View reviewed changes

guscarreon reviewed Nov 18, 2021

View reviewed changes

Merge remote-tracking branch 'origin' into currency-conversion

514da41

Merge remote-tracking branch 'upstream/master' into currency-conversion

bef756e

Jim Naumann added 2 commits November 23, 2021 10:30

working JSON currency test.

375bdfa

currency conversion tests for error conditions added

2c50b0d

muncha dismissed AlexBVolcy’s stale review via 2c50b0d November 23, 2021 10:39

guscarreon reviewed Nov 23, 2021

View reviewed changes

Jim Naumann added 2 commits November 28, 2021 02:31

simplified the conditionals for bidfloors

347d6e4

added a test to cover the case where there is a fallback for a bad cu…

fd14c20

…rrency conversion, but it is below the minimum

guscarreon reviewed Nov 29, 2021

View reviewed changes

updated documentation

078bade

AlexBVolcy approved these changes Nov 30, 2021

View reviewed changes

guscarreon approved these changes Nov 30, 2021

View reviewed changes

mansinahar merged commit cd4319b into prebid:master Nov 30, 2021

muncha deleted the currency-conversion branch December 1, 2021 09:38

yevhenii-viktorov mentioned this pull request Jan 12, 2022

adds currency conversion and tests to Beachfront Bidder prebid/prebid-server-java#1678

Closed

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

Beachfront - Currency conversion (prebid#2078)

55cd5a4

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

Beachfront - Currency conversion (prebid#2078)

74e8583

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Beachfront - Currency conversion #2078

Beachfront - Currency conversion #2078

muncha commented Nov 10, 2021

AlexBVolcy left a comment

AlexBVolcy Nov 11, 2021

muncha Nov 11, 2021 •

edited

AlexBVolcy Nov 16, 2021

guscarreon left a comment

muncha commented Nov 22, 2021

guscarreon commented Nov 23, 2021

muncha commented Nov 23, 2021 •

edited

guscarreon left a comment

guscarreon Nov 23, 2021

guscarreon Nov 23, 2021

guscarreon Nov 23, 2021

guscarreon Nov 29, 2021

muncha Nov 30, 2021 •

edited

guscarreon Nov 30, 2021

AlexBVolcy left a comment

guscarreon left a comment

Beachfront - Currency conversion #2078

Beachfront - Currency conversion #2078

Conversation

muncha commented Nov 10, 2021

AlexBVolcy left a comment

Choose a reason for hiding this comment

AlexBVolcy Nov 11, 2021

Choose a reason for hiding this comment

muncha Nov 11, 2021 • edited

Choose a reason for hiding this comment

AlexBVolcy Nov 16, 2021

Choose a reason for hiding this comment

guscarreon left a comment

Choose a reason for hiding this comment

muncha commented Nov 22, 2021

guscarreon commented Nov 23, 2021

muncha commented Nov 23, 2021 • edited

guscarreon left a comment

Choose a reason for hiding this comment

guscarreon Nov 23, 2021

Choose a reason for hiding this comment

guscarreon Nov 23, 2021

Choose a reason for hiding this comment

guscarreon Nov 23, 2021

Choose a reason for hiding this comment

guscarreon Nov 29, 2021

Choose a reason for hiding this comment

muncha Nov 30, 2021 • edited

Choose a reason for hiding this comment

guscarreon Nov 30, 2021

Choose a reason for hiding this comment

AlexBVolcy left a comment

Choose a reason for hiding this comment

guscarreon left a comment

Choose a reason for hiding this comment

muncha Nov 11, 2021 •

edited

muncha commented Nov 23, 2021 •

edited

muncha Nov 30, 2021 •

edited