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

CONSENT-309: Ensure consent data is copied for Segment Connections + Profiles #2070

Merged
merged 35 commits into from
Jun 13, 2024

Conversation

chenxzhang
Copy link
Contributor

@chenxzhang chenxzhang commented Jun 3, 2024

While working on consent in RETL, we realized that the event's context.consent.categoryPreferences data isn't copied over. This PR adds the data to the track events for Segment Connections + Profiles.

Testing

Include any additional information about the testing you have completed to
ensure your changes behave as expected. For a speedy review, please check
any of the tasks you completed below during your testing.

  • Added unit tests for new functionality
  • Tested end-to-end using the local server
  • [Segmenters] Tested in the staging environment

Tested the change in stage and verified the events in Segment Connections + Profiles have the consent data.

Segment Connections:
Screenshot 2024-06-10 at 5 03 12 PM
Segment Profiles:
Screenshot 2024-06-10 at 5 03 49 PM

@chenxzhang chenxzhang changed the title CONSENT-309: Ensure consent data is copied for Segment Connections CONSENT-309: Ensure consent data is copied for Segment Connections + Profiles Jun 6, 2024
@chenxzhang chenxzhang marked this pull request as ready for review June 6, 2024 02:59
@chenxzhang chenxzhang requested a review from a team as a code owner June 6, 2024 02:59
Copy link
Contributor

@joe-ayoub-segment joe-ayoub-segment left a comment

Choose a reason for hiding this comment

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

hi @chenxzhang thanks for raising this PR.

Here's some feedback and questions.

  1. I can't see why the yarn.lock file was edited. Was this a mistake? If so can you please rectify?

  2. Is your intent to grab andinclude the entrire object from the context.consent location? If so that's all good. Is there a partticular schema that this data should look like? Should we validate it in any way? If so, I can show you how to do this.

  3. Could you please attach evidence of testing howing what the context.consent object looks like please?

@chenxzhang
Copy link
Contributor Author

hi @chenxzhang thanks for raising this PR.

Here's some feedback and questions.

  1. I can't see why the yarn.lock file was edited. Was this a mistake? If so can you please rectify?
  2. Is your intent to grab andinclude the entrire object from the context.consent location? If so that's all good. Is there a partticular schema that this data should look like? Should we validate it in any way? If so, I can show you how to do this.
  3. Could you please attach evidence of testing howing what the context.consent object looks like please?

Thanks for the review!

  1. I was getting CI error and had to do yarn install https://github.com/segmentio/action-destinations/actions/runs/9393961071/job/25870870707

  2. Yes. I want to grab context.consent.categoryPreferences. categoryPreferences should be an object/map of string: boolean.
    For example:

"context": {
    "consent": 
      "categoryPreferences": {
         "Advertising": true,
         "Analytics": false,
         "Functional": true,
         "C003": false
      }
    }
  }

  1. I'll add them to the PR description

@joe-ayoub-segment
Copy link
Contributor

Thanks @chenxzhang . I still think the yarn.lock file shouldn't change. Any chance you could rebase from main? it might fix that issue.

Copy link
Contributor

@varadarajan-tw varadarajan-tw left a comment

Choose a reason for hiding this comment

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

The changes look good to me. Left few minor comments.

description: 'Segment event consent category preferences.',
type: 'object',
default: { '@path': '$.context.consent' },
additionalProperties: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think addtionalProperties is necessary here would help considering the field is hidden.

description: 'Segment event consent category preferences.',
type: 'object',
default: { '@path': '$.context.consent' },
additionalProperties: true,
Copy link
Contributor

Choose a reason for hiding this comment

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

Same comment as above. additionalProperties are not required for this use case.

@@ -7,3 +7,7 @@ export const MissingUserOrAnonymousIdThrowableError = new PayloadValidationError
export const InvalidEndpointSelectedThrowableError = new PayloadValidationError(
'A valid endpoint must be selected. Please check your Segment settings.'
)

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 error being used anywhere?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleting it

@varadarajan-tw varadarajan-tw dismissed joe-ayoub-segment’s stale review June 13, 2024 16:42

Joe is out of office and the team needs this PR shipped. So, I am approving on behalf of Joe

@chenxzhang chenxzhang merged commit b6cf8b4 into main Jun 13, 2024
11 checks passed
@chenxzhang chenxzhang deleted the CONSENT-309/segment branch June 13, 2024 16:47
@joe-ayoub-segment
Copy link
Contributor

hi @varadarajan-tw has this been deployed?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants