Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New adapter: Ads Interactive #3929

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

AdsInteractive
Copy link

Copy link

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, 77fe565

ads_interactive

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/ads_interactive/ads_interactive.go:28:	Builder		100.0%
github.com/prebid/prebid-server/v2/adapters/ads_interactive/ads_interactive.go:35:	MakeRequests	88.2%
github.com/prebid/prebid-server/v2/adapters/ads_interactive/ads_interactive.go:91:	makeRequest	85.7%
github.com/prebid/prebid-server/v2/adapters/ads_interactive/ads_interactive.go:109:	MakeBids	100.0%
github.com/prebid/prebid-server/v2/adapters/ads_interactive/ads_interactive.go:145:	getBidType	100.0%
total:											(statements)	92.4%

@@ -0,0 +1,21 @@
endpoint: "https://bntb.adsintreactive.com/"
maintainer:
email: "[email protected]"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have sent a email, please confirm

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please write back to my email. We need to confirm that this email works
image

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

confirmed:
image

@@ -0,0 +1,21 @@
endpoint: "https://bntb.adsintreactive.com/"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

endpoint is not reachable:
image

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oops, there was an error in the domain, fixed

Copy link

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

ads_interactive

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/ads_interactive/ads_interactive.go:28:	Builder		100.0%
github.com/prebid/prebid-server/v2/adapters/ads_interactive/ads_interactive.go:35:	MakeRequests	88.2%
github.com/prebid/prebid-server/v2/adapters/ads_interactive/ads_interactive.go:91:	makeRequest	85.7%
github.com/prebid/prebid-server/v2/adapters/ads_interactive/ads_interactive.go:109:	MakeBids	100.0%
github.com/prebid/prebid-server/v2/adapters/ads_interactive/ads_interactive.go:145:	getBidType	100.0%
total:											(statements)	92.4%

Copy link

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, 0cc30e3

ads_interactive

Refer here for heat map coverage report

github.com/prebid/prebid-server/v2/adapters/ads_interactive/ads_interactive.go:28:	Builder		100.0%
github.com/prebid/prebid-server/v2/adapters/ads_interactive/ads_interactive.go:35:	MakeRequests	88.2%
github.com/prebid/prebid-server/v2/adapters/ads_interactive/ads_interactive.go:91:	makeRequest	85.7%
github.com/prebid/prebid-server/v2/adapters/ads_interactive/ads_interactive.go:109:	MakeBids	100.0%
github.com/prebid/prebid-server/v2/adapters/ads_interactive/ads_interactive.go:145:	getBidType	100.0%
total:											(statements)	92.4%

@przemkaczmarek przemkaczmarek self-assigned this Oct 14, 2024
@przemkaczmarek
Copy link
Collaborator

@bsardo we have already adsInteractive adapter. They use the same vendor-id

@bsardo
Copy link
Collaborator

bsardo commented Oct 24, 2024

@bsardo we have already adsInteractive adapter. They use the same vendor-id

@AdsInteractive we noticed that there is already an adsinteractive adapter in PBS with the same GVL ID and endpoint domain adsinteractive.com. We also see that there are two adapters in prebid.js for Ads Interactive. Can you help us understand why there are two?

@AdsInteractive
Copy link
Author

@bsardo the existing adapter is used for one platform and this one is for ours. We have no relation to the existing adsinteractive

@bsardo
Copy link
Collaborator

bsardo commented Nov 4, 2024

Hi @AdsInteractive, we recently released PBS 3.0, more specifically v3.1.0, which updates Prebid Server package import references throughout the project from v2 to v3.
For example:

import (
    "github.com/prebid/prebid-server/v3/adapters"
)

As a result, please merge with master (no rebase) and then ensure all Prebid Server package import references in the files you’ve changed are v3 such that the test suite passes so we can resume reviewing. Thanks!

Copy link

github-actions bot commented Nov 5, 2024

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, 46a6d48

ads_interactive

Refer here for heat map coverage report

github.com/prebid/prebid-server/v3/adapters/ads_interactive/ads_interactive.go:28:	Builder		100.0%
github.com/prebid/prebid-server/v3/adapters/ads_interactive/ads_interactive.go:35:	MakeRequests	88.2%
github.com/prebid/prebid-server/v3/adapters/ads_interactive/ads_interactive.go:91:	makeRequest	85.7%
github.com/prebid/prebid-server/v3/adapters/ads_interactive/ads_interactive.go:109:	MakeBids	100.0%
github.com/prebid/prebid-server/v3/adapters/ads_interactive/ads_interactive.go:145:	getBidType	100.0%
total:											(statements)	92.4%

@AdsInteractive
Copy link
Author

@bsardo done

@bsardo bsardo self-assigned this Nov 5, 2024
@bretg
Copy link
Contributor

bretg commented Nov 5, 2024

@AdsInteractive - I disagree with this statement:

We have no relation to the existing adsinteractive

  1. The GVL ID is the same.
  2. The contact address is the same

We need you to help us understand the actual relationship between adsinteractive and ads_interactive. There are 3 options:

  1. ads_interactive is a new implementation and branding, and we should deprecate adsinteractive in a future major release.
  2. ads_interactive is really the same company and this PR should be updated to just replace adsinteractive.
  3. ads_interactive is truly a different company. You will provide different GVL ID and different contact address.

Based on the fact that this same confusion exists in Prebid.js, I suspect you'll choose option 1.

@AdsInteractive
Copy link
Author

@bretg thanks for the reply, new clarifications have appeared. Our case is the first option: ads_interactive is a new implementation and branding, and we should deprecate adsinteractive in a future major release

@bretg
Copy link
Contributor

bretg commented Nov 7, 2024

I've added "remove adsinteractive adapter" to the PBS 4.0 issue. Please make sure that you update the documentation to make it clear which branding is active.

@AdsInteractive
Copy link
Author

I've added "remove adsinteractive adapter" to the PBS 4.0 issue. Please make sure that you update the documentation to make it clear which branding is active.

@bretg thanks, doc PR - prebid/prebid.github.io#5732

@@ -0,0 +1,22 @@
endpoint: "https://bntb.adsinteractive.com/"
gvlVendorID: 1212
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

its correct:

"1212": {
      "id": 1212,
      "name": "Ads Interactive Ltd.",
      "purposes": [1, 2, 3, 4, 7, 10],
      "legIntPurposes": [],
      "flexiblePurposes": [],
      "specialPurposes": [1, 2],
      "features": [3],
      "specialFeatures": [],
      "overflow": {
        "httpGetLimit": 32
      },
      "cookieMaxAgeSeconds": 4838400,
      "usesCookies": true,
      "cookieRefresh": false,
      "urls": [
        {
          "langId": "en",
          "privacy": "https://adsinteractive.com/privacy-policy",
          "legIntClaim": "https://adsinteractive.com/privacy-policy"
        }
      ],
      "usesNonCookieAccess": false,
      "dataRetention": {
        "stdRetention": 60,
        "purposes": {

        },
        "specialPurposes": {

        }
      },
      "dataDeclaration": [1, 2, 8, 11],
      "deviceStorageDisclosureUrl": "https://adsinteractive.com/vendor.json"
    }
    ```

@@ -0,0 +1,22 @@
endpoint: "https://bntb.adsinteractive.com/"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image
not working

var bidderExt adapters.ExtImpBidder
var adsInteractiveExt openrtb_ext.ImpExtAdsInteractive

if err := json.Unmarshal(imp.Ext, &bidderExt); err != nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please switch this to using jsonutil? Starting in PBS 3.0 we're switching to our more optimized implementation. Thanks.

`{"placementId": "test"}`,
`{"placementId": "1"}`,
`{"endpointId": "test"}`,
`{"endpointId": "1"}`,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please handle:
{"placementId": "test", "unknownField": "value"},
{},
}

var invalidParams = []string{
`{"placementId": 42}`,
`{"endpointId": 42}`,
`{"placementId": "1", "endpointId": "1"}`,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please handel:
{"placementId": ""},
{"endpointId": ""},
{"randomField": "value"},

@bsardo bsardo assigned VeronikaSolovei9 and unassigned bsardo Dec 16, 2024
Comment on lines +39 to +41
reqCopy := *request
for _, imp := range request.Imp {
reqCopy.Imp = []openrtb2.Imp{imp}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you are creating a separate bid request per imp. Do you think it will make sense to move reqCopy := *request inside of the for loop?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants