Pubcommon id and shared id submodules: merge into one submodule #6808

NothingIsMatter · 2021-05-21T15:48:01Z

Type of change

[ x] Feature

Description of change

Merge pubcid and sharedid

Other information

Issue - #6640

lgtm-com · 2021-05-21T16:02:04Z

This pull request fixes 1 alert when merging 2ee857b into 22ad110 - view on LGTM.com

fixed alerts:

1 for Useless conditional

patmmccann · 2021-05-21T19:35:02Z

@NothingIsMatter prebid 5 was updated just now, can you repull the branch / merge conflicts?

jdwieland8282 · 2021-05-21T20:05:21Z

@patmmccann, @NothingIsMatter is in the EU, is this something that needs to be addressed today, or can it wait until Monday?

patmmccann · 2021-05-21T20:59:56Z

It can definitely wait, the 5.0 release is two weeks out

lgtm-com · 2021-05-24T15:57:59Z

This pull request fixes 1 alert when merging 9e1bfee into 6957925 - view on LGTM.com

fixed alerts:

1 for Useless conditional

mmoschovas

Few things to take a look at here. Also it seems as though perhaps some of the files were overlooked in this update. The previous change consisted of 31 files whereas this update has only modified 25. Just doing a quick comparison, I can see that openx and ix adapters no longer have the update to remove the sharedId references. I would suggest going over this again to make sure all necessary updates are included in this version as well

mmoschovas · 2021-05-25T14:33:28Z

modules/sharedIdSystem.js

+   * @param {SubmoduleParams} [config]
+   * @param {ConsentData|undefined} consentData
+   * @param {Object} storedId existing id
+   * @returns {IdResponse|undefined}


Looks like the extendId function comments are included but the pubCommonId function itself was not placed in here. Was this intentional? The domainOverride function was also not included as it was in the last PR

No, that wasn`t intentional, will remove it

modules/sharedIdSystem.js

mmoschovas · 2021-05-25T14:44:54Z

test/spec/modules/sharedIdSystem_spec.js

@@ -1,77 +0,0 @@
-import {


Why is this module test being removed?

According to #6640:

remove the following sharedid files

modules/sharedIdSystem.js
test/spec/modules/sharedIdSystem_spec.js
modules/sharedIdSystem.md

NothingIsMatter · 2021-05-25T16:39:52Z

@mmoschovas added comments, updated the branch

lgtm-com · 2021-05-25T16:50:43Z

This pull request fixes 1 alert when merging e30ad8a into 6957925 - view on LGTM.com

fixed alerts:

1 for Useless conditional

lgtm-com · 2021-05-25T17:12:30Z

This pull request fixes 1 alert when merging 62d6193 into 6957925 - view on LGTM.com

fixed alerts:

1 for Useless conditional

lgtm-com · 2021-05-25T17:54:14Z

This pull request fixes 1 alert when merging ec41a92 into 6957925 - view on LGTM.com

fixed alerts:

1 for Useless conditional

mmoschovas · 2021-05-26T16:25:41Z

@NothingIsMatter I think there has been some confusion here most likely due to the nature of the ticket but quite possibly because of my comments. I just want to make sure there is a shared understanding here:

There are two pubCommon files - pubCommonId and pubCommonIdSystem. My understanding as per Prebid docs is that pubCommonIdSystem is currently used and pubCommonId is deprecated. Meaning that the purpose of this PR is to basically transfer code to sharedIdSystem and its corresponding unit tests from the pubCommonIdSystem. It seems the test here have been taken from the pubCommonId file. It looks as though perhaps there was never a designated test for the pubCommonIdSystem, so these tests will need to be created under sharedIdSystem_spec since I dont see one existing currently
In terms of the comments and references, I am not seeing this having been fully updated. For clarification, I am referring to areas of the code such as the following. Effectively the pubCommonId module has been renamed to sharedId and this should be reflected in these areas as to be consistent; otherwise, this can become very confusing.

* This module adds PubCommonId to the User ID module 
* The {@link module:modules/userId} module is required 
* @module modules/pubCommonIdSystem 
* @requires module:modules/userId

...
export const pubCommonIdSubmodule = {
...

...
submodule('userId', pubCommonIdSubmodule);

I just wanted to make sure you saw my previous note regarding some adapters that were not included in this PR that were in the previous PR. i.e. ix and openx (looks like there are a few more though). Just want to confirm that these adapters are being updated if their 5.0 branch code still includes the sharedId reference

NothingIsMatter · 2021-05-26T16:47:46Z

@NothingIsMatter I think there has been some confusion here most likely due to the nature of the ticket but quite possibly because of my comments. I just want to make sure there is a shared understanding here:

There are two pubCommon files - pubCommonId and pubCommonIdSystem. My understanding as per Prebid docs is that pubCommonIdSystem is currently used and pubCommonId is deprecated. Meaning that the purpose of this PR is to basically transfer code to sharedIdSystem and its corresponding unit tests from the pubCommonIdSystem. It seems the test here have been taken from the pubCommonId file. It looks as though perhaps there was never a designated test for the pubCommonIdSystem, so these tests will need to be created under sharedIdSystem_spec since I dont see one existing currently

In terms of the comments and references, I am not seeing this having been fully updated. For clarification, I am referring to areas of the code such as the following. Effectively the pubCommonId module has been renamed to sharedId and this should be reflected in these areas as to be consistent; otherwise, this can become very confusing.
* This module adds PubCommonId to the User ID module 
* The {@link module:modules/userId} module is required 
* @module modules/pubCommonIdSystem 
* @requires module:modules/userId
...
export const pubCommonIdSubmodule = {
...
...
submodule('userId', pubCommonIdSubmodule);
I just wanted to make sure you saw my previous note regarding some adapters that were not included in this PR that were in the previous PR. i.e. ix and openx (looks like there are a few more though). Just want to confirm that these adapters are being updated if their 5.0 branch code still includes the sharedId reference

I did PR according to Merge Pubcommon and SharedId user id sub adapters. #6640, it doesn`t contains any info regarding pubCommonId.js, hence i did not change it at all, also according to issue, I renamed PubcommonIdSystem_spec to sharedIdSystem_spec without changing it, but as you said its relates to pubCommonId, not to pubCommonIdSystem, probably need some input here from @jdwieland8282
Ok, I will change it
Yes, probably I missed some of the changes, will fix it too.

lgtm-com · 2021-05-26T17:18:21Z

This pull request fixes 1 alert when merging 6d4096d into 6957925 - view on LGTM.com

fixed alerts:

1 for Useless conditional

jdwieland8282 · 2021-06-01T17:42:00Z

@mmoschovas mind re reviewing we've made the requested changes.

mmoschovas · 2021-06-01T18:55:08Z

@jdwieland8282 are the unit tests going to be updated as well? It still looks like the spec file is testing the deprecated pubCommonId file

SKOCHERI · 2021-06-01T20:13:26Z

test/spec/modules/userId_spec.js

@@ -25,7 +25,6 @@ import {
 import {server} from 'test/mocks/xhr.js';
 import find from 'core-js-pure/features/array/find.js';
 import {unifiedIdSubmodule} from 'modules/unifiedIdSystem.js';
-import {pubCommonIdSubmodule} from 'modules/pubCommonIdSystem.js';


@mmoschovas pubCommonIdSystem.js is removed from the tests.

@SKOCHERI I'm referring to test/spec/modules/sharedIdSystem_spec.js which is using the pubCommonId.js functions not sharedIdSystem.js

@mmoschovas The test/spec/modules/sharedIdSystem_spec.js is updated. Can you review it?

… 6640-5.0

lgtm-com · 2021-06-03T17:26:05Z

This pull request fixes 1 alert when merging 8ca32a1 into fe175cd - view on LGTM.com

fixed alerts:

1 for Useless conditional

mmoschovas

LGTM

patmmccann · 2021-06-04T20:54:14Z

@NothingIsMatter merge conflict showed up

smenzer

LGTM

lgtm-com · 2021-06-07T17:27:50Z

This pull request fixes 1 alert when merging 0c7e5f6 into 61c494a - view on LGTM.com

fixed alerts:

1 for Useless conditional

merge pubcid and sharedid

2ee857b

patmmccann added 5.0 API Change feature major labels May 21, 2021

patmmccann requested a review from mmoschovas May 21, 2021 16:48

patmmccann requested a review from smenzer May 21, 2021 19:35

smenzer assigned smenzer and mmoschovas May 24, 2021

resolve conflicts

9e1bfee

patmmccann changed the title ~~merge pubcid and sharedid~~ Pubcommon id and shared id submodules: merge into one submodule May 25, 2021

mmoschovas requested changes May 25, 2021

View reviewed changes

remove useless comments

e30ad8a

rename/remove useless

62d6193

obernadin added 2 commits May 25, 2021 20:23

return coppa

7bef850

fix circleci

ec41a92

rename + fix missed files

6d4096d

SKOCHERI reviewed Jun 1, 2021

View reviewed changes

obernadin added 2 commits June 3, 2021 17:02

Merge branch 'prebid-5.0' of https://github.com/prebid/Prebid.js into…

3b63dd2

… 6640-5.0

revert sharedIdtest removal,merging reworked

8ca32a1

mmoschovas approved these changes Jun 4, 2021

View reviewed changes

mmoschovas added the needs 2nd review Core module updates require two approvals from the core team label Jun 4, 2021

jlukas79 approved these changes Jun 4, 2021

View reviewed changes

smenzer approved these changes Jun 7, 2021

View reviewed changes

pull + resolve conflicts

0c7e5f6

patmmccann merged commit 013698f into prebid:prebid-5.0 Jun 7, 2021

philipwatson mentioned this pull request Jun 25, 2021

sharedIdSystem: return pubcid instead of sharedId #7099

Merged

1 task

patmmccann mentioned this pull request Aug 2, 2021

BetweenBidAdapter sharedid support was accidently(?) removed in Prebid 5.0? #7221

Closed

cosminser mentioned this pull request Jan 19, 2022

Mny 471 Connatix/Prebid.js#3

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pubcommon id and shared id submodules: merge into one submodule #6808

Pubcommon id and shared id submodules: merge into one submodule #6808

NothingIsMatter commented May 21, 2021

lgtm-com bot commented May 21, 2021

patmmccann commented May 21, 2021 •

edited

Loading

jdwieland8282 commented May 21, 2021

patmmccann commented May 21, 2021

lgtm-com bot commented May 24, 2021

mmoschovas left a comment

mmoschovas May 25, 2021

NothingIsMatter May 25, 2021

NothingIsMatter May 25, 2021

mmoschovas May 25, 2021

NothingIsMatter May 25, 2021

NothingIsMatter May 25, 2021

NothingIsMatter commented May 25, 2021

lgtm-com bot commented May 25, 2021

lgtm-com bot commented May 25, 2021

lgtm-com bot commented May 25, 2021

mmoschovas commented May 26, 2021

NothingIsMatter commented May 26, 2021

lgtm-com bot commented May 26, 2021

jdwieland8282 commented Jun 1, 2021

mmoschovas commented Jun 1, 2021

SKOCHERI Jun 1, 2021

mmoschovas Jun 1, 2021

SKOCHERI Jun 3, 2021

lgtm-com bot commented Jun 3, 2021

mmoschovas left a comment

patmmccann commented Jun 4, 2021

smenzer left a comment

lgtm-com bot commented Jun 7, 2021

Pubcommon id and shared id submodules: merge into one submodule #6808

Pubcommon id and shared id submodules: merge into one submodule #6808

Conversation

NothingIsMatter commented May 21, 2021

Type of change

Description of change

Other information

lgtm-com bot commented May 21, 2021

patmmccann commented May 21, 2021 • edited Loading

jdwieland8282 commented May 21, 2021

patmmccann commented May 21, 2021

lgtm-com bot commented May 24, 2021

mmoschovas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NothingIsMatter commented May 25, 2021

lgtm-com bot commented May 25, 2021

lgtm-com bot commented May 25, 2021

lgtm-com bot commented May 25, 2021

mmoschovas commented May 26, 2021

NothingIsMatter commented May 26, 2021

lgtm-com bot commented May 26, 2021

jdwieland8282 commented Jun 1, 2021

mmoschovas commented Jun 1, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 3, 2021

mmoschovas left a comment

Choose a reason for hiding this comment

patmmccann commented Jun 4, 2021

smenzer left a comment

Choose a reason for hiding this comment

lgtm-com bot commented Jun 7, 2021

patmmccann commented May 21, 2021 •

edited

Loading