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

Uid2 decouple cstg #2

Merged
merged 14 commits into from
Oct 10, 2023
Merged

Conversation

jingyi-gao-ttd
Copy link
Owner

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

Other information

@@ -0,0 +1,490 @@
/* eslint-disable no-console */

Choose a reason for hiding this comment

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

Don't think we need this, can't see any uses of console in this file

};

export function attachTokenGenerator(tokenGeneratorModule) {

Choose a reason for hiding this comment

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

Does a publisher who wishes to use CSTG need to call this, or is it just for tests?

Copy link
Owner Author

Choose a reason for hiding this comment

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

No, the publisher don't need to call this, this is for async submodule loading. no matter which module loads first, prebid core will call this function and pass the uid2Cstg in here.

https://github.com/prebid/Prebid.js/blob/master/src/hook.js#L36C8-L49

package.json Outdated
@@ -131,6 +131,7 @@
"dset": "3.1.2",
"express": "^4.15.4",
"fun-hooks": "^0.9.9",
"gulp-cli": "^2.3.0",

Choose a reason for hiding this comment

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

Should this be a dev dependency?

Copy link
Owner Author

Choose a reason for hiding this comment

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

good catch, I will remove it

gulpfile.js Outdated
@@ -450,7 +451,13 @@ gulp.task('build-bundle-verbose', gulp.series(makeWebpackPkg({
// public tasks (dependencies are needed for each task since they can be ran on their own)
gulp.task('test-only', test);
gulp.task('test-all-features-disabled', testTaskMaker({disableFeatures: require('./features.json'), oneBrowser: 'chrome', watch: false}));
// gulp.task('test-watch', testTaskMaker({watch: true, file: ['test/spec/modules/uid2idsystem_spec.js']}));

Choose a reason for hiding this comment

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

Do we want to keep the commented-out lines?

Choose a reason for hiding this comment

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

It looks like this example isn't specific to UID2, can we make such a big change?

Copy link
Owner Author

Choose a reason for hiding this comment

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

No, this PR is against my original PR and not pushed to Prebid repo, those changes are easy for everyone to pull this branch and run tests to see the coverage and tests. I will revert this back when I merge back to uid2-cstg-integration

Choose a reason for hiding this comment

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

Do we want to keep the commented-out lines?

Copy link
Owner Author

Choose a reason for hiding this comment

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

Same as above, with this commented-out when you spin up gulp serve-fast you will only see uid2 in the uid2_example page. Those will be removed once we merge back to uid2-cstg-integration

}

const mappedConfig = {
let mappedConfig = {

Choose a reason for hiding this comment

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

This can still be const

if (!storedTokenFromSameIdentity(storedTokens, identity)) {
_logInfo('CSTG supplied new identity - ignoring stored value.', storedTokens.originalIdentity, cstgIdentity);
// Stored token wasn't originally sourced from the provided identity - ignore the stored value. A new user has logged in?
if (FEATURES.UID2_CSTG && isCstgEnabled) {

Choose a reason for hiding this comment

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

Do we need to check FEATURES.UID2_CSTG here?

@@ -670,13 +752,9 @@ export function Uid2GetId(config, prebidStorageManager, _logInfo, _logWarn) {
originalToken: suppliedToken ?? storedTokens?.originalToken,
latestToken: newestAvailableToken,
};
if (cstgIdentity) tokens.originalIdentity = storedTokens?.originalIdentity
if (FEATURES.UID2_CSTG && isCstgEnabled) {

Choose a reason for hiding this comment

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

Do we need to check FEATURES.UID2_CSTG here?

Copy link
Owner Author

Choose a reason for hiding this comment

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

we could remove it, with FEATURES.UID2_CSTG line 709-726 will be removed from build, just reduce some bytes

@jingyi-gao-ttd jingyi-gao-ttd marked this pull request as ready for review October 10, 2023 02:27
@jingyi-gao-ttd jingyi-gao-ttd merged commit f644840 into uid2-cstg-integration Oct 10, 2023
@jingyi-gao-ttd jingyi-gao-ttd deleted the uid2-decouple-cstg branch October 10, 2023 02:27
jingyi-gao-ttd pushed a commit that referenced this pull request Oct 10, 2023
* Flipp Bid Adapter: initial release

* Added flippBidAdapter

* OFF-372 Support DTX/Hero in flippBidAdapter (#2)

* support creativeType

* OFF-422 flippBidAdapter handle AdTypes

---------

Co-authored-by: Jairo Panduro <[email protected]>

* OFF-465 Add getUserKey logic to prebid.js adapter (prebid#3)

* Support cookie sync and uid

* address pr feedback

* remove redundant check

* OFF-500 Support "startCompact" param for Prebid.JS prebid#4

* set startCompact default value (prebid#5)

* fix docs

* use client bidding endpoint

* update unit testing endpoint

---------

Co-authored-by: Jairo Panduro <[email protected]>
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.

2 participants