-
Notifications
You must be signed in to change notification settings - Fork 139
Added support of the consent flag parameter cs_ucfr, updated beacon.j… #623
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
Conversation
kdaswani
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great @longstoryshort! I really appreciate all the detail in the description and test results. I've left a couple of comments - one for Pooya (who we will also request review from) and one for you 😄
@pooyaj given Elena's thorough test results and only a few customers using Comscore (Fox, Nexstar), is pinning to a specific source necessary or could we go straight to adding this to a release train?
…alytics.js-integrations into comscore_cs_ucfr_flag
pooyaj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Other than the auto-imported options the rest looks good to me.
|
@kdaswani As discussed with Fox - Changes / updates:
Unit tests: Local tests: Local tests completed successfully
If nested attribute is not found in the
4.1 3rd character = "N" => 4.2 3rd character = "Y" => 4.3 3rd character = "-" =>
|
kdaswani
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a couple of notes on two of the unit tests, but otherwise this looks great! Once we update those unit tests, this is all good from my end. @pooyaj can you do another review and let us know if there's a standard for whether if conditions should be single or multi-line? We had to make some updates as per Fox's request in terms of the mappings (i.e. can map from the context object, can map using the US Privacy string values). Thanks so much!
| } | ||
| }); | ||
|
|
||
| if (this.options.consentFlag) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pooyaj do we have a preference/standard on if we should put some of these conditions on a single line or if multiple lines are clearer?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the current format is fine
pooyaj
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me 👍
| } | ||
| }); | ||
|
|
||
| if (this.options.consentFlag) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, the current format is fine










What does this PR do?
This PR does the following:
https://sb.scorecardresearch.com/cs/<c2 client Id value>/beacon.js. Requested changes document (see 1st rquest).cs_ucfr: allows a customer to map page() event property to the_cs_ucfr_label (comScore user consent flag) in the Destination Settings; converts property value into "1"/"0" as expected by comScore. PRD document.Are there breaking changes in this PR?
No.
Testing
Testing completed successfully: both unit tests and local testing.
Unit tests completed successfully including new (highlighted in orange) and changed (highlighted in red).
2.1 Consent flag is not mapped in the Destination Settings

Result:

cs_ucfris not passed with beacon.js2.2 Consent flag is mapped in the Destination Settings, page() event does not contain specified property

Result:

cs_ucfris not passed with beacon.js2.3 Consent flag is mapped in the Destination Settings, page() event property is other then 1/0, true/false
Result:

cs_ucfris passed with beacon.js,_cs_ucfr=""2.4 Consent flag is mapped in the Destination Settings, page() event property is Null
Result:

cs_ucfris passed with beacon.js,cs_ucfr=""2.5 Consent flag is mapped in the Destination Settings, page() event property is true (bool)
Result:

cs_ucfris passed with beacon.js,cs_ucfr="1"2.6 Consent flag is mapped in the Destination Settings, page() event property is "true" (string)
Result:

cs_ucfris passed with beacon.js,cs_ucfr="1"2.7 Consent flag is mapped in the Destination Settings, page() event property is 1 (int)
Result:

cs_ucfris passed with beacon.js,cs_ucfr="1"2.8 Consent flag is mapped in the Destination Settings, page() event property is "1" (string)
Result:

cs_ucfris passed with beacon.js,cs_ucfr="1"2.9 Consent flag is mapped in the Destination Settings, page() event property is false (bool)
Result:

cs_ucfris passed with beacon.js,cs_ucfr="0"2.10 Consent flag is mapped in the Destination Settings, page() event property is "false" (string)
Result:

cs_ucfris passed with beacon.js,cs_ucfr="0"2.11 Consent flag is mapped in the Destination Settings, page() event property is 0 (int)
Result:

cs_ucfris passed with beacon.js,cs_ucfr="0"2.12 Consent flag is mapped in the Destination Settings, page() event property is "0" (string)
Result:

cs_ucfris passed with beacon.js,cs_ucfr="0"Any background context you want to provide?
Both changes requested by Fox.
cs_ucfrflag was introduced by comScore last year and required for tracking user consent. This implementation follows PRD document here.Is there parity with the server-side/android/iOS integration components (if applicable)?
Planned, but not ready yet.
iOS and Android implementation of the
cs_ucfrflag is planned in the upcoming weeks.Does this require a new integration setting? If so, please explain how the new setting works
Yes.
This change requires a new setting
consentFlag. The setting is a string field, not required. The setting is live in the Staging environment.The setting allows a customer to specify page() event property name which should be mapped to the
cs_ucfrbeacon.js label.If the setting is not populated, or mapped property is missing in the page() call - the integration omits
cs_ucfrlabel. If mapped property value is 1 or true - the integration setscs_ucfr="1". If mapped property value is 0 or false - the integration setscs_ucfr="0". If mapped property value is any value other than 1/0/true/false, or if it is Null - the integration setscs_ucfr="".Links to helpful docs and other external resources
PRD document: link