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: Add support for Google Click Ids #957

Merged
merged 1 commit into from
Dec 11, 2024
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

  • Add support for Google Click Ids

Testing Plan

  • Was this tested locally? If not, explain why.
  • Run through automated tests. Manually configure sample app to use Integration Capture setting in backend and add relevant gclid, gbraid, wbraid values to query parameters.

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

Copy link

sonarqubecloud bot commented Dec 5, 2024

Quality Gate Failed Quality Gate failed

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

See analysis details on SonarQube Cloud

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.

LGTM! Making it modular from the beginning really simplifies adding new url parameter click ids.

@rmi22186 rmi22186 requested a review from samdozor December 6, 2024 18:21
Copy link
Member

@samdozor samdozor left a comment

Choose a reason for hiding this comment

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

looks good.

one higher level concern: we're parsing essentially random data within queryStringParser - who knows what crazy URLs there are out there. I see that we rolled our own parser for when URLSearchParams isn't there (seems like maybe 1-3% of users?). Have you considered having a try/catch to make sure we don't crash if given a totally garbage url?

I looked up the tests for node's internal url.js implementation and there is some wacky stuff it handles, which we do not.

@alexs-mparticle
Copy link
Collaborator Author

alexs-mparticle commented Dec 11, 2024

@samdozor This is a great callout. I went ahead and created SQDSDKS-6989 so we can work on this.

@alexs-mparticle alexs-mparticle merged commit f03c5ed into development Dec 11, 2024
23 of 30 checks passed
github-actions bot pushed a commit that referenced this pull request Dec 11, 2024
# [2.31.0](v2.30.4...v2.31.0) (2024-12-11)

### Features

* Add support for Google Click Ids ([#955](#955)) ([#957](#957)) ([3df8d84](3df8d84))
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.

3 participants