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

feat: Capture Integration Ids and assign to events #926

Merged

Conversation

alexs-mparticle
Copy link
Collaborator

Instructions

  1. PR target branch should be against development
  2. PR title name should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-title-check.yml
  3. PR branch prefix should follow this format: https://github.com/mParticle/mparticle-workflows/blob/main/.github/workflows/pr-branch-check-name.yml

Summary

Testing Plan

  • Was this tested locally? If not, explain why.
  • Using a sample app, pass in Facebook Click IDs via a query parameter, or Facebook Pixel ID as a cookie value and verify they appear in the Custom Flags for Events

Reference Issue (For mParticle employees only. Ignore if you are an outside contributor)

src/mp-instance.js Outdated Show resolved Hide resolved
src/serverModel.ts Outdated Show resolved Hide resolved
src/serverModel.ts Outdated Show resolved Hide resolved
Copy link

@brewdente brewdente left a comment

Choose a reason for hiding this comment

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

I did a read through with @alexs-mparticle and everything looks clean and pretty straightforward. The tests are comprehensive and verified in his environment that it works.

src/integrationCapture.ts Outdated Show resolved Hide resolved
src/utils.ts Outdated Show resolved Hide resolved
src/integrationCapture.ts Show resolved Hide resolved
src/integrationCapture.ts Outdated Show resolved Hide resolved
src/utils.ts Show resolved Hide resolved
src/utils.ts Show resolved Hide resolved

describe('Integration Capture', () => {
describe('constructor', () => {
it('should initialize with clickIds as undefined', () => {
Copy link
Member

Choose a reason for hiding this comment

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

I go back and forth with saying this should be undefined vs it being defined with a default value of {}. But not a hill i'm going to die on :)

Copy link
Member

@rmi22186 rmi22186 left a comment

Choose a reason for hiding this comment

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

very minor nits. looks great!

test/jest/utils.spec.ts Outdated Show resolved Hide resolved
test/jest/utils.spec.ts Outdated Show resolved Hide resolved
test/src/tests-feature-flags.ts Outdated Show resolved Hide resolved
src/integrationCapture.ts Outdated Show resolved Hide resolved
src/integrationCapture.ts Outdated Show resolved Hide resolved
src/integrationCapture.ts Show resolved Hide resolved
test/jest/integration-capture.spec.ts Outdated Show resolved Hide resolved
src/integrationCapture.ts Outdated Show resolved Hide resolved
src/integrationCapture.ts Outdated Show resolved Hide resolved
src/integrationCapture.ts Outdated Show resolved Hide resolved
src/integrationCapture.ts Outdated Show resolved Hide resolved
src/mp-instance.js Show resolved Hide resolved
@alexs-mparticle alexs-mparticle force-pushed the feat/SQDSDKS-6806-capture-integration-ids branch from 97eeb5d to 05fc756 Compare October 7, 2024 19:22
src/integrationCapture.ts Outdated Show resolved Hide resolved
Copy link
Member

@rmi22186 rmi22186 left a comment

Choose a reason for hiding this comment

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

tiny nits

src/integrationCapture.ts Outdated Show resolved Hide resolved
src/serverModel.ts Outdated Show resolved Hide resolved
src/utils.ts Show resolved Hide resolved
Copy link

sonarcloud bot commented Oct 8, 2024

Quality Gate Failed Quality Gate failed

Failed conditions
11.1% Duplication on New Code (required ≤ 3%)

See analysis details on SonarCloud

@alexs-mparticle alexs-mparticle merged commit a916262 into development Oct 8, 2024
24 of 28 checks passed
github-actions bot pushed a commit that referenced this pull request Oct 8, 2024
# [2.29.0](v2.28.3...v2.29.0) (2024-10-08)

### Bug Fixes

* Fix typo in identity interface ([#923](#923)) ([f1e0ec9](f1e0ec9))

### Features

* Capture Integration Ids and assign to events ([#926](#926)) ([a916262](a916262))
@mparticle-automation
Copy link
Collaborator

🎉 This PR is included in version 2.29.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

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

Successfully merging this pull request may close these issues.

6 participants