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.

Sign up for GitHub

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

Jump to bottom

Prebid 7: Freestar Bid Adapter initial release & Deprecate Sortable Bid Adapter #8263

Closed
wants to merge 4 commits into from
Closed

Prebid 7: Freestar Bid Adapter initial release & Deprecate Sortable Bid Adapter #8263

wants to merge 4 commits into from

Conversation

nicgallardo
Copy link
Contributor

Type of change

  • Bugfix
  • Feature
  • New bidder adapter
  • Code style update (formatting, local variables)
  • Refactoring (no functional changes, no api changes)
  • Build related changes
  • CI related changes
  • Does this change affect user-facing APIs or examples documented on http://prebid.org?
  • Other

Description of change

  • Removal of Sortable Bid Adapter
  • Addition of Freestar Bid Adapter
  • test parameters for validating bids
{
  "bidder": "freestar",
    "params": {
      "siteId":"example.com",
      "tagId":"example_728x90_970x90_320x50",
      "keywords":{}
    }
}

Accompanying Documentation

Other information

@bretg
Copy link
Collaborator

bretg commented Apr 5, 2022

@nicgallardo - you can remove sortable from the docs, but it's considered a breaking change to remove it from the code repo. Please restore the .js files and explain in the .md files that the sortable bidder and analytics adapter are deprecated. You can remove them formally in PBJS 7.0, which is next month!

@nicgallardo
Copy link
Contributor Author

@bretg I am attempting to merge our branch to the prebid-7 branch so that it might be in the 7.0 release. If this is not the correct manner to add to that release please point me in the correct direction to add to the 7.0 release. Thanks!

@nicgallardo nicgallardo closed this Apr 6, 2022
@nicgallardo nicgallardo reopened this Apr 6, 2022
@bretg
Copy link
Collaborator

bretg commented Apr 6, 2022

Thanks Nic. Adding @dgirardi for guidance on how to get this change into 7, and @patmmccann to consider for the release notes (remove sortable bid and analytics adapters)

@dgirardi
Copy link
Collaborator

dgirardi commented Apr 6, 2022

This PR looks correct for getting this into 7; you will need to post a separate PR if you also want it in 6 - I suggest waiting until this gets approved.

@ChrisHuie ChrisHuie changed the title Cactus 102 pbjs fs module Freestar Bid Adapter: add new bid adapter and deprecate Sortable adapter docs Apr 6, 2022
@ChrisHuie ChrisHuie self-requested a review April 6, 2022 16:32
@ChrisHuie ChrisHuie self-assigned this Apr 6, 2022
@patmmccann
Copy link
Collaborator

tagging #7796 for linkage, thanks!

@patmmccann patmmccann changed the title Freestar Bid Adapter: add new bid adapter and deprecate Sortable adapter docs Prebid 7: Freestar Bid Adapter: initial release Apr 7, 2022
@patmmccann patmmccann changed the title Prebid 7: Freestar Bid Adapter: initial release Prebid 7: Freestar Bid Adapter initial release & Deprecate Sortable Bid Adapter Apr 7, 2022
patmmccann
patmmccann reviewed Apr 7, 2022
View reviewed changes
modules/freestarBidAdapter.js
))
const isBanner = !bid.mediaTypes || bid.mediaTypes[BANNER];
const bannerSizes = isBanner ? deepAccess(bid, `mediaType.${BANNER}.sizes`) || bid.sizes : null;
return !!(bid.params.tagId && haveSiteId && validFloor && validKeywords && (!isBanner ||
Copy link
Collaborator

Choose a reason for hiding this comment

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

are you really rejecting biid requests if the keywords are invalid? why not just drop the keywords

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we are validating just as we did in the SortableBidAdapter.

Copy link
Collaborator

Choose a reason for hiding this comment

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

this validation is quite aggressive, doesnt seem best for your company or your publisher partners

Copy link
Collaborator

Choose a reason for hiding this comment

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

also why not fallback to no floor when the floor is invalid?

patmmccann
patmmccann requested changes Apr 7, 2022
View reviewed changes
modules/freestarBidAdapter.js
if (floor) {
rv.floor = floor;
}
if (bid.params.keywords) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

please support reading keywords from ortb2 object

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We use the keywords in our param a bit differently than the ortb2 documentation.

We can support keywords as the spec states in the future. However, we do not have it on our roadmap at this moment.

Copy link
Collaborator

Choose a reason for hiding this comment

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

you have to fix or delete support for keywords

@patmmccann
Copy link
Collaborator

@nicgallardo looks good, just two requested changes with respect to keywords

@patmmccann patmmccann mentioned this pull request Apr 12, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ChrisHuie ChrisHuie Awaiting requested review from ChrisHuie

@patmmccann patmmccann Awaiting requested review from patmmccann

Assignees

@ChrisHuie ChrisHuie

Labels
7.0 API Change feature major
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@nicgallardo @bretg @dgirardi @patmmccann @ChrisHuie