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

Rubicon adapter: accept accountId, siteId, zoneId as strings #2110

Merged
merged 5 commits into from
Jan 10, 2022
Merged

Rubicon adapter: accept accountId, siteId, zoneId as strings #2110

merged 5 commits into from
Jan 10, 2022

Conversation

And1sS
Copy link
Member

@And1sS And1sS commented Dec 14, 2021

ZoneId int `json:"zoneId"`
AccountId json.RawMessage `json:"accountId"`
SiteId json.RawMessage `json:"siteId"`
ZoneId json.RawMessage `json:"zoneId"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be easier to use json.Number instead? Please refer to this PR: https://github.com/prebid/prebid-server/pull/2105/files

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, thanks for suggestion!

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for changing the code!
May I ask you to add json test where imp.ext.{accountId/siteId/zoneId} have string values? Similar to what you already have in exemplary, but with strings instead?

Copy link
Contributor

@VeronikaSolovei9 VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

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

LGTM

siteId, err := rubiconExt.SiteId.Int64()
if err != nil {
errs = append(errs, err)
continue
Copy link
Contributor

Choose a reason for hiding this comment

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

This is just for my own understanding, but is there a chance that the error we can get back is bad enough that we wouldn't want to continue execution?

I see this style of error handling in a lot of places in this file, so I was just wondering.

Copy link
Member Author

Choose a reason for hiding this comment

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

In most cases we would like to continue execution.

@VeronikaSolovei9
Copy link
Contributor

Hi guys, hope you had a good holidays!
Please add this change to documentation repository: https://github.com/prebid/prebid.github.io/blob/master/dev-docs/bidders/rubicon.md

@And1sS
Copy link
Member Author

And1sS commented Jan 10, 2022

Documentation updates related pr: prebid/prebid.github.io#3496
@VeronikaSolovei9

@mansinahar mansinahar merged commit 6677af1 into prebid:master Jan 10, 2022
wwwyyy pushed a commit to wwwyyy/prebid-server that referenced this pull request Jan 18, 2022
* 'master' of https://github.com/wwwyyy/prebid-server:
  GDPR: pass geo based on special feature 1 opt-in (prebid#2122)
  Adyoulike: Fix adapter errors if no content (prebid#2134)
  Update adyoulike.yaml (prebid#2131)
  Add ORTB 2.4 schain support (prebid#2108)
  Stored response fetcher (with new func for stored resp) (prebid#2099)
  Adot: Add OpenRTB macros resolution (prebid#2118)
  Rubicon adapter: accept accountId, siteId, zoneId as strings (prebid#2110)
  Operaads: support multiformat request (prebid#2121)
  Update go-gdpr to v1.11.0 (prebid#2125)
  Adgeneration: Update request to include device and app related info in headers and query params (prebid#2109)
  Adnuntius: Read device information from ortb Request (prebid#2112)
  New Adapter: apacdex and Remove Adapter: valueimpression (prebid#2097)
  New Adapter: Compass (prebid#2102)
  Orbidder: bidfloor currency handling (prebid#2093)
ramyferjaniadot pushed a commit to adotmob/prebid-server that referenced this pull request Feb 2, 2022
jorgeluisrocha pushed a commit to jwplayer/prebid-server that referenced this pull request Sep 28, 2022
shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022
mobfxoHB pushed a commit to mobfxoHB/prebid-server that referenced this pull request Aug 22, 2023
…solution added in addition to a deprecation warning (prebid#2110)
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.

4 participants