feat: added properties for executeAction mixpanel event (#35291)#35645
Conversation
WalkthroughThe recent changes enhance the application’s action execution capabilities by introducing new constants for managing parameters and refining the analytics event logging process. A new Changes
Assessment against linked issues
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 as PR comments)
Additionally, you can add CodeRabbit Configuration File (
|
github-actions
bot
added
Integrations Pod General
NilanshBansal
requested review from
nidhi-nair and
sondermanish
as code owners
![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
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java (4 hunks)
Additional context used
Learnings (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java (1)
Learnt from: abhvsn PR: appsmithorg/appsmith#33894 File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java:256-258 Timestamp: 2024-05-31T13:06:01.938Z Learning: Different values for `EMAIL_DOMAIN_HASH` and `ADMIN_EMAIL_DOMAIN_HASH` are used in events to facilitate granular filtering on third-party services.
Additional comments not posted (5)
app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java (1)
154-156: Great job adding new constants!The new constants
ACTION_EXECUTION_REQUEST_PARAMS_VALUE_MAP,ACTION_EXECUTION_INVERT_PARAMETER_MAP, andACTION_EXECUTION_RAW_QUERYare well-named and consistent with the existing naming conventions in the class. These additions will help improve the clarity and maintainability of the code. Ensure these constants are used appropriately in the application to avoid any unused code.app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java (1)
210-212: Well done on enhancing data privacy!The conditional check to remove
ACTION_EXECUTION_REQUEST_PARAMS_VALUE_MAPfromanalyticsPropertiesfor self-hosted instances is a thoughtful addition. This change aligns with best practices for data privacy and ensures that sensitive data is not unnecessarily exposed.app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java (3)
792-792: Good addition of therawQueryparameter!Including the
rawQueryin the analytics data provides valuable insights into user queries and helps in understanding user behavior better. Ensure that this parameter is correctly populated and used throughout the application to maximize its utility.
1123-1129: Nice work on enriching analytics data!The inclusion of
ACTION_EXECUTION_REQUEST_PARAMS_VALUE_MAP,ACTION_EXECUTION_INVERT_PARAMETER_MAP, andACTION_EXECUTION_RAW_QUERYin the analytics data enriches the context available for analysis. This will help in tracking user interactions more effectively.
828-833: Ensure consistency in method signatures.The addition of the
rawQueryparameter to thesendExecuteAnalyticsEventmethod is consistent with the goal of enhancing analytics data. Verify that all calls to this method are updated to include the new parameter to avoid any runtime issues.Verification successful
Consistency in Method Signatures Verified
The
sendExecuteAnalyticsEventmethod has been updated to include therawQueryparameter, and all calls to this method within the codebase are consistent with this change. There are no other calls outside theActionExecutionSolutionCEImpl.javafile.
- The method signature and its usage are consistent with the addition of the
rawQueryparameter.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that all calls to `sendExecuteAnalyticsEvent` include the new `rawQuery` parameter. # Test: Search for the method usage. Expect: All occurrences include the `rawQuery` parameter. rg --type java -A 2 $'sendExecuteAnalyticsEvent('Length of output: 153
Script:
#!/bin/bash # Correcting the regex pattern to search for the method usage and verify all occurrences include the `rawQuery` parameter. rg --type java -A 5 'sendExecuteAnalyticsEvent\('Length of output: 2021
Script:
#!/bin/bash # Search for the method signature of `sendExecuteAnalyticsEvent` to verify the presence of the `rawQuery` parameter. rg --type java -A 10 'sendExecuteAnalyticsEvent\('Length of output: 3410
Script:
#!/bin/bash # Search for all occurrences of the `sendExecuteAnalyticsEvent` method across the codebase to ensure consistency. rg --type java 'sendExecuteAnalyticsEvent\('Length of output: 398
| if (shouldHashUserId(event, userId, hashUserId, commonConfig.isCloudHosting())) { | ||
| final String hashedUserId = hash(userId); | ||
| analyticsProperties.remove("request"); | ||
| // Remove params map key property if it's self-hosted |
There was a problem hiding this comment.
@NilanshBansal Why are we removing this for self hosted? Can we please let the comment also reflect why we are doing this?
...er/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java
Outdated
Show resolved
Hide resolved
...ith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (4)
- app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ActionExecutionSolutionImpl.java (3 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java (11 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImplTest.java (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/constants/ce/FieldNameCE.java
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java
Additional comments not posted (4)
app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ActionExecutionSolutionImpl.java (2)
4-4: The import statement forCommonConfiglooks good!The addition of this import is necessary for the new configuration dependency.
42-43: Constructor changes look good!The inclusion of
CommonConfigenhances the class's ability to manage configurations effectively.However, ensure that this change is reflected in all places where this class is instantiated.
Also applies to: 62-63
app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImplTest.java (2)
132-134: The addition of@MockBeanforCommonConfigis well done!This allows for effective testing without relying on the actual configuration implementation.
164-165: ThebeforeEachmethod update looks good!Including
commonConfigensures that the mock is utilized during test execution.Consider verifying that all relevant tests cover the use of
CommonConfig.Verification successful
Test Coverage for
CommonConfigis AdequateThe
commonConfigis utilized in multiple test files, indicating that its functionality is well-covered across different scenarios. This ensures that the mock configuration is effectively tested.
- Files with coverage:
ProductAlertServiceCEImplTestUserIdentifierServiceCEImplTestUserServiceTestThe update to include
commonConfigin thebeforeEachmethod is well-supported by these tests.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify test coverage for `CommonConfig`. # Test: Search for test methods using `commonConfig`. Expect: Relevant tests cover its usage. rg --type java '@Test' -A 5 | rg 'commonConfig'Length of output: 2372
...ith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java
Show resolved
Hide resolved
Failed server tests
|
Failed server tests
|
1 similar comment
Failed server tests
|
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
Files selected for processing (3)
- app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java (1 hunks)
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java (11 hunks)
- app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImplTest.java (3 hunks)
Files skipped from review as they are similar to previous changes (2)
- app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImpl.java
- app/server/appsmith-server/src/test/java/com/appsmith/server/solutions/ce/ActionExecutionSolutionCEImplTest.java
Additional context used
Learnings (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java (1)
Learnt from: abhvsn PR: appsmithorg/appsmith#33894 File: app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java:256-258 Timestamp: 2024-05-31T13:06:01.938Z Learning: Different values for `EMAIL_DOMAIN_HASH` and `ADMIN_EMAIL_DOMAIN_HASH` are used in events to facilitate granular filtering on third-party services.
Additional comments not posted (1)
app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java (1)
209-209: Ensure proper handling of sensitive data.The removal of the
requestkey fromanalyticsPropertiesfor self-hosted instances is a good step towards enhancing privacy. Ensure that this aligns with your overall data privacy strategy and that no other sensitive data is inadvertently exposed.Verification successful
Ensure sensitive data is handled appropriately in analytics properties.
The code effectively removes potentially sensitive information, such as the
requestkey, fromanalyticsPropertiesfor self-hosted instances. Additionally, sensitive fields like email domains are hashed, which is a good practice. Ensure that all sensitive data is either removed or properly anonymized before being sent in analytics events.
- Verify that no other sensitive data is included without proper anonymization or removal.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that no sensitive data is included in `analyticsProperties` for self-hosted instances. # Test: Search for any sensitive data handling in `sendEvent` method. Expect: No sensitive data should be present. rg --type java 'analyticsProperties.put' -A 3Length of output: 30976
NilanshBansal
deleted the
feat/issue-35291/added-mixpanel-properties-executeAction
branch
2 participants
Description
This PR adds new properties to the
Execute Actionmixpanel event so that we can measure the following:New Properties added are:
Fixes #35291
Automation
/ok-to-test tags="@tag.Sanity"
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/10372829069
Commit: e8d8a02
Cypress dashboard.
Tags:
@tag.SanitySpec:
Tue, 13 Aug 2024 16:11:44 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
New Features
Improvements