fix: ga4 allow zero #1915
fix: ga4 allow zero #1915
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new utility function Changes
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1915 +/- ##
===========================================
+ Coverage 57.52% 57.54% +0.02%
===========================================
Files 485 485
Lines 16505 16506 +1
Branches 3315 3305 -10
===========================================
+ Hits 9494 9498 +4
- Misses 5732 5757 +25
+ Partials 1279 1251 -28 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/analytics-js-integrations/src/integrations/GA4/utils.js (1)
11-11: Well-structured changes with good architectural choices.The implementation:
- Makes a precise improvement to validation logic
- Maintains clean separation of concerns
- Follows existing patterns in the codebase
- Properly handles error cases with logging
As mentioned in the PR objectives, please ensure cross-browser testing in Chrome, Firefox, and IE11 to verify the behavior of this validation change.
Also applies to: 300-300
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/analytics-js-integrations/src/integrations/GA4/utils.js(2 hunks)
🔇 Additional comments (2)
packages/analytics-js-integrations/src/integrations/GA4/utils.js (2)
11-11: LGTM: Import statement updated appropriately.
The addition of isDefinedAndNotNull to the commonUtils imports is well-placed and aligns with its usage in the validation logic.
300-300: Validation logic improvement correctly handles zero values.
The change from a truthy check to isDefinedAndNotNull is a good improvement that allows zero values while maintaining proper validation for undefined/null values. This aligns with the PR objective to allow customers to pass a value of 0.
Let's verify the behavior with different edge cases:
size-limit report 📦
|
…dder-sdk-js into fix.ga4_allow_zero
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
packages/analytics-js-integrations/__tests__/integrations/GA4/utils.test.js(2 hunks)packages/analytics-js-integrations/src/integrations/GA4/utils.js(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/analytics-js-integrations/src/integrations/GA4/utils.js
🔇 Additional comments (2)
packages/analytics-js-integrations/__tests__/integrations/GA4/utils.test.js (2)
13-13: LGTM: Import statement follows existing patterns
The addition of prepareStandardEventParams to the imports is consistent with the existing import structure.
383-410: Verify cross-browser compatibility and GA4 requirements
Since this PR requires cross-browser testing and involves GA4 integration:
- Ensure the test suite runs successfully across Chrome, Firefox, and IE11
- Verify that zero values are accepted by GA4's API
packages/analytics-js-integrations/__tests__/integrations/GA4/utils.test.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
packages/analytics-js-integrations/__tests__/integrations/GA4/utils.test.js (1)
406-409: Remove redundant commentsThe inline comments are redundant as the test description and assertions already make it clear that we're testing zero values.
- total: 0, // Total is 0 but should pass without failure - value: 0, // Value is 0 but should pass without failure - revenue: 0, // Revenue is 0 but should pass without failure - price: 0, // Price is 0 but should pass without failure + total: 0, + value: 0, + revenue: 0, + price: 0,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
packages/analytics-js-integrations/__tests__/integrations/GA4/utils.test.js(2 hunks)
🔇 Additional comments (3)
packages/analytics-js-integrations/__tests__/integrations/GA4/utils.test.js (3)
13-13: LGTM!
The import statement is correctly updated to include the new utility function.
342-387: LGTM! Comprehensive test coverage for PII handling
The test cases effectively cover:
- Multiple PII fields
- Edge cases (undefined, empty object)
- Different data types
393-538: LGTM! Comprehensive test coverage
The test suite thoroughly covers various scenarios:
- Zero values handling
- Mixed data types
- Optional fields
- Negative values
- Complex scenarios with metadata
The tests are well-structured with clear setup, execution, and assertion phases.
|
PR Description
This PR allows customers to pass a value of 0, resolving the previous restriction.
Linear task (optional)
Resolves INT-2844
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
New Features
Bug Fixes
Tests
prepareStandardEventParamsfunction to ensure correct handling of various input scenarios, including zero and mixed data types.