feat: add support for group in appcues #1877
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes introduce a new Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Appcues
participant SDK
User->>Appcues: Call group(rudderElement)
Appcues->>Appcues: Extract groupId and traits
Appcues->>SDK: Call window.Appcues.group(groupId, traits)
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 (
|
![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 (2)
packages/analytics-js-integrations/src/integrations/Appcues/browser.js (1)
87-99: LGTM: Group method implementation looks goodThe
groupmethod implementation aligns well with the PR objectives and follows the Appcues documentation. It correctly handles the case whengroupIdis not provided.Consider adding error handling for the
window.Appcues.groupcall. This can help catch and log any potential issues during the group identification process. Here's a suggested implementation:group(rudderElement) { const { groupId, traits } = rudderElement.message; if (!isDefinedAndNotNullAndNotEmpty(groupId)) { logger.error('group id is required'); return; } - window.Appcues.group( - groupId, // unique, required - traits, - ); + try { + window.Appcues.group( + groupId, // unique, required + traits, + ); + } catch (error) { + logger.error('Error calling Appcues.group:', error); + } }This change will ensure that any errors during the Appcues group call are caught and logged, improving the robustness of the integration.
packages/analytics-js-integrations/__tests__/integrations/Appcues/appcues.test.js (1)
165-186: LGTM: New test suite for Appcues group functionality added.The new test suite for the
groupmethod is well-structured and covers the basic functionality. It correctly initializes the Appcues integration, mocks the SDK, and verifies thegroupmethod call with the expected parameters.However, consider the following suggestions to enhance test coverage:
- Add a test case for when
groupIdis missing to ensure proper error handling.- Include a test with nested traits to verify correct flattening of objects.
- Consider adding tests for potential error cases or edge scenarios.
Would you like me to provide example test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- packages/analytics-js-integrations/tests/integrations/Appcues/appcues.test.js (2 hunks)
- packages/analytics-js-integrations/src/integrations/Appcues/browser.js (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
packages/analytics-js-integrations/src/integrations/Appcues/browser.js (1)
59-59: LGTM: Minor formatting improvementThe removal of the space before the condition check improves code consistency and adheres to common JavaScript formatting practices.
packages/analytics-js-integrations/__tests__/integrations/Appcues/appcues.test.js (1)
23-23: LGTM: Mock function forgroupadded.The addition of the
groupmock function tomockAppcuesSDKis appropriate and necessary for testing the new group functionality in the Appcues integration.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1877 +/- ##
===========================================
+ Coverage 56.97% 56.98% +0.01%
===========================================
Files 476 476
Lines 16244 16249 +5
Branches 3236 3249 +13
===========================================
+ Hits 9255 9260 +5
+ Misses 5771 5726 -45
- Partials 1218 1263 +45 ☔ View full report in Codecov by Sentry. |
size-limit report 📦
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
packages/analytics-js-integrations/__tests__/integrations/Appcues/appcues.test.js (2)
176-193: LGTM: Comprehensive test cases for the group method.Both test cases are well-structured and cover important scenarios:
- The first test correctly verifies the behavior when both
groupIdandtraitsare present.- The second test appropriately checks that the
groupmethod is not called whengroupIdis missing.These tests follow good practices and provide good coverage for the new
groupfunctionality.Consider adding a test case for when
groupIdis present buttraitsis empty or missing. This would further improve the test coverage.
23-23: Overall, excellent addition of group functionality tests.The changes introduce well-structured and comprehensive tests for the new
groupmethod in the Appcues integration. The additions are consistent with the existing test patterns and provide good coverage of the new functionality. The mock updates and test cases are appropriately implemented.To further enhance the test suite:
- Consider adding a test case for when
groupIdis present buttraitsis empty or missing.- If there are any specific requirements or edge cases for the
groupmethod mentioned in the Appcues documentation, consider adding tests for those scenarios as well.Also applies to: 165-193
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- packages/analytics-js-integrations/tests/integrations/Appcues/appcues.test.js (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
packages/analytics-js-integrations/__tests__/integrations/Appcues/appcues.test.js (2)
23-23: LGTM: Mock implementation forgroupmethod added.The addition of the
groupmock in themockAppcuesSDKfunction is appropriate and consistent with the new functionality being tested. This change enables isolated testing of thegroupmethod.
165-175: LGTM: Well-structured test suite setup for group tests.The new test suite for Appcues group tests is properly set up with a descriptive title. The
rudderElementobject is well-defined with the necessary properties (groupIdandtraits) for comprehensive group testing.
|
PR Description
Adding support for group call in appcues.
Appcues Docs: https://docs.appcues.com/en_US/dev-installing-appcues/installation-overview-for-developers#identifying-groups-15
Please include a summary of the change along with the relevant motivation and context.
Linear task (optional)
Resolves INT-2682
Linear task link
Cross Browser Tests
Please confirm you have tested for the following browsers:
Sanity Suite
Security
Summary by CodeRabbit
New Features
groupmethod in the Appcues integration, enabling user grouping based on provided parameters.Tests
groupmethod to ensure functionality and correct parameter usage.