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

Permutive RTD Module: merge external submodule params #8881

Merged
merged 2 commits into from
Aug 29, 2022

Conversation

AntonioGargaro
Copy link
Contributor

@AntonioGargaro AntonioGargaro commented Aug 22, 2022

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

Cohort activation within Permutive's RTD module is failing to be applied with option 1, which let publishers set acBidders through Permutive's platform. We require this fix where the RTD module attempts to retrieve this config from LS or our Permutive SDK. This lets publishers modify their Permutive <-> Prebid integration without code changes.

We read from LS on initialisation in the event that the Permutive SDK has not initialised before the config is required. Every call to retrieve our config will attempt to interact with Permutive's prebid addon which will have this config available in memory, otherwise fallback to cache.

Config Priority

  1. customModuleConfig <- set by publisher with pbjs.setConfig
  2. permutiveRtdConfig <- set by the publisher using the Permutive platform
  3. defaultConfig

The configuration module is deeply merged in reverse priority order. This means that the highest priority config (customModuleConfig) defined by .setConfig within Prebid will be merged last and overwrite other configurations.

Other information

N/A

Copy link
Collaborator

@patmmccann patmmccann left a comment

Choose a reason for hiding this comment

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

j

const customModuleConfig = { params: { acBidders: ['789'], maxSegs: 499 } }
const config = getModuleConfig(customModuleConfig)

const configMergedInPriorityOrder = mergeDeep(defaultConfig, liftToParams(cachedConfig), liftToParams(permutiveRtdConfig), customModuleConfig)
Copy link
Collaborator

Choose a reason for hiding this comment

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

you want the configuration coming from window.permutive to override how the publisher configures the module in prebid?

Copy link
Contributor Author

@AntonioGargaro AntonioGargaro Aug 23, 2022

Choose a reason for hiding this comment

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

The name is misleading here, the configs are merged in reverse priority order as configs are merged left to right. The priority is,

  1. customModuleConfig <- set by publisher with pbjs.setConfig
  2. permutiveRtdConfig <- set by the publisher using Permutive.
  3. defaultConfig

Therefore, anything set by the publisher will take priority over what is set within Permutive. I'll add some comments explaining this.

  • add docs

Copy link
Collaborator

Choose a reason for hiding this comment

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

thanks!

@AntonioGargaro AntonioGargaro force-pushed the fix/permutive-rtd-config branch from 2958a6c to 5bbb8b7 Compare August 23, 2022 09:59
@AntonioGargaro AntonioGargaro marked this pull request as ready for review August 23, 2022 12:58
@ChrisHuie
Copy link
Collaborator

ChrisHuie commented Aug 23, 2022

@AntonioGargaro the integration tests don't appear to have ran on this pr. This is usually because the submitter is following a fork of prebid inside of CircleCI instead of the main Prebid project. You should be able to unfollow your fork in CircleCI and that should kick off testing.

@AntonioGargaro AntonioGargaro force-pushed the fix/permutive-rtd-config branch from 24e6e61 to c3890f8 Compare August 23, 2022 15:13
@AntonioGargaro
Copy link
Contributor Author

@ChrisHuie Should be running now 👍

@ChrisHuie ChrisHuie changed the title fix(permutive-rtd): merge external submodule params Permutive RTD Module: merge external submodule params Aug 23, 2022
@patmmccann patmmccann merged commit ece711e into prebid:master Aug 29, 2022
@AntonioGargaro AntonioGargaro deleted the fix/permutive-rtd-config branch August 30, 2022 08:27
JacobKlein26 pushed a commit to nextmillenniummedia/Prebid.js that referenced this pull request Feb 9, 2023
* fix(permutive-rtd): merge external submodule params

* fix(permutive-rtd): use ls storage wrapper
jorgeluisrocha pushed a commit to jwplayer/Prebid.js that referenced this pull request May 23, 2023
* fix(permutive-rtd): merge external submodule params

* fix(permutive-rtd): use ls storage wrapper
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants