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

Data Controller Module: initial release #8484

Merged
merged 10 commits into from
Jul 25, 2022
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
90 changes: 90 additions & 0 deletions modules/dataControllerModule/index.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,90 @@
/**
* This module sets filters the data(userEIDs and SDA) based on the configuration.
* @module modules/dataController
*/

/**
* @interface dataControllerModule
*/

/**
* @function?
* @summary return real time data
* @name dataControllerModule#filterEIDs
* @param {Object} bidRequest
*/

/**
* @function?
* @summary return real time data
* @name dataControllerModule#filterSDA
* @param {Object} bidRequest
*/

import {config} from '../../src/config.js';
import {module, getHook} from '../../src/hook.js';
import {processBidderRequests} from '../../src/adapters/bidderFactory.js';

let submodules = [];
Copy link
Collaborator

Choose a reason for hiding this comment

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

why is this planning for submodules? I'm not sure I understand the scope of the feature - going by this proposal it seems self-contained to me.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dgirardi The proposal mentions about Publisher having ability to define the rules to filter EID and SDA hence providing submodule.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't understand - publishers do not have access to the module system and they wouldn't be able to add a submodule (they would have to fork the repo).

I would expect them to define the rules with setConfig, and I don't see the need multiple submodules to read that config unless it makes sense to want only part of that proposal - but I don't see anything in it that suggests that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will take a look

const MODULE_NAME = 'dataController';
let dataControllerConfig;

/**
* enable submodule in data Controller Module
* @param {dataController} submodule
*/
export function registerSubmodules(submodule) {
submodules.push(submodule);
}

export function init(bidderRequests) {
submodules.forEach(submodule => {
filterData(submodule, bidderRequests)
});
}

export function filterData(submodule, bidRequest) {
dataControllerConfig = config.getConfig(MODULE_NAME);

if (!dataControllerConfig) {
processBidderRequests.getHooks({hook: processBidderRequestHook}).remove();
Copy link
Collaborator

@dgirardi dgirardi Jun 15, 2022

Choose a reason for hiding this comment

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

I'd prefer to keep configuration and business logic separate - this will get confusing fast as complexity increases. (IMO it makes more sense to turn the module off from the same place you turn it on).

return;
}

if (dataControllerConfig.filterEIDwhenSDA && dataControllerConfig.filterSADwhenEID) {
processBidderRequests.getHooks({hook: processBidderRequestHook}).remove();
return;
}
if (dataControllerConfig.filterEIDwhenSDA) {
let filterEIDs = submodule.filterEIDs(bidRequest)
if(filterEIDs) {
config.setConfig({'dcUsersAsEids': filterEIDs});
}
} else if (dataControllerConfig.filterSADwhenEID) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@patmmccann do we want to have the param as "supressSDAwhenEID"/"supressEIDwhenSDA"? Filter would be more generic rather than suppress.

Copy link
Collaborator

Choose a reason for hiding this comment

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

All references to SAD should be changed to SDA

Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated.

let bidderConfig = submodule.filterSDA(bidRequest);
if (bidderConfig) {
config.setBidderConfig(bidderConfig);
}
}
processBidderRequests.getHooks({hook: processBidderRequestHook}).remove();
}

/**
* BidderRequests hook to intiate module and reset data object.
*/
function processBidderRequestHook(fn, specDetails, bidRequest) {
init(bidRequest);
// Removes hook after run
processBidderRequests.getHooks({hook: processBidderRequestHook}).remove();
}

/**
* Sets bidderRequests hook
*/
function setupHook() {
getHook('processBidderRequests').before(processBidderRequestHook);
}

setupHook();

module(MODULE_NAME, registerSubmodules);
30 changes: 30 additions & 0 deletions modules/dataControllerModule/index.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,30 @@
# Overview

```
Module Name: Data Controller Module
```

# Description

This module will filter EIDs and SDA based on the configurations.
The filtered EIDs are stored in 'dcUsersAsEids' configuration and filtered SDA are updated in bidder configuration.
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this detail no longer relevant? I am not seeing any of this stored any longer, so if that is the case, can this be removed


Sub module object with the following keys:

| param name | type | Scope | Description | Params |
| :------------ | :------------ | :------ | :------ | :------ |
| filterEIDwhenSDA | function | optional | Filters user EIDs based on SDA | bidrequest |
| filterSADwhenEID | function | optional | Filters SDA based on configured EIDs | bidrequest |
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo


# Module Control Configuration

```

pbjs.setConfig({
dataController: {
filterEIDwhenSDA: ['*']
filterSADwhenEID: ['id5-sync.com']
Copy link
Collaborator

Choose a reason for hiding this comment

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

typo

}
});

```
8 changes: 8 additions & 0 deletions src/adapters/bidderFactory.js
Original file line number Diff line number Diff line change
Expand Up @@ -296,6 +296,14 @@ export function newBidder(spec) {
* @param onCompletion {function()} invoked once when all bid requests have been processed
*/
export const processBidderRequests = hook('sync', function (spec, bids, bidderRequest, ajax, wrapCallback, {onRequest, onResponse, onError, onBid, onCompletion}) {

let updatedUserAsEids = config.getConfig('dcUsersAsEids');
Copy link
Collaborator

Choose a reason for hiding this comment

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

This looks like it's using config as a global variable, plus it's adding dead code for those who are not using the data controller module.

Why not do this directly from the hook added by the module?
Also, IMO the makeBidRequests hook is a better place for this - I don't know if it makes a difference now, but my instinct is that the earlier they have this data the better.

Copy link
Contributor Author

@SKOCHERI SKOCHERI Jun 24, 2022

Choose a reason for hiding this comment

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

@dgirardi I have update this module to be self contained.
The SDA were not available if this module was called with makeBidRequests as the *RTBProviders are invoked until then

if (updatedUserAsEids && updatedUserAsEids[bidderRequest.bidderCode]) {
bids.forEach(bid => {
bid.userIdAsEids = updatedUserAsEids[bidderRequest.bidderCode];
})
};

let requests = spec.buildRequests(bids, bidderRequest);
if (!requests || requests.length === 0) {
onCompletion();
Expand Down
Loading