chore: Remove unused UserData.role
#37381
Conversation
WalkthroughThe pull request introduces several changes across multiple files, primarily focusing on the removal of the Changes
Possibly related PRs
Suggested labels
Suggested reviewers
📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (11)
💤 Files with no reviewable changes (5)
🔇 Additional comments (9)app/server/appsmith-server/src/main/java/com/appsmith/server/dtos/ce/UserUpdateCE_DTO.java (2)
The simplified
Let's verify that API consumers are not relying on the role field in user updates. app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCE.java (2)
The removal of the
This is a breaking change in the interface contract. Let's verify all implementations and callers have been updated. ✅ Verification successfulBased on the results, I can generate the final response as we have found all the necessary information. The search results show:
All implementations and callers are properly aligned with the interface changes The method signature changes have been consistently applied across the codebase. The implementation in 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Find implementations of AnalyticsServiceCE
ast-grep --pattern 'class $_ implements $$$AnalyticsServiceCE$$$'
# Find calls to identifyInstance
rg 'identifyInstance\(' -A 2
Length of output: 1466 app/client/src/ce/sagas/userSagas.tsx (1)
The removal of the ✅ Verification successfulRemoval of Based on the codebase analysis:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check if the API endpoint handles the absence of the role field gracefully
# by looking for any validation or required field checks in the API implementation
# Search for API validation related to user role
ast-grep --pattern 'class $_ implements $_ {
$$$
updateUser($payload) {
$$$
role
$$$
}
$$$
}'
# Search for any TypeScript interfaces that might still require the role field
rg -g '*.ts' -g '*.tsx' 'interface.*User.*\{[\s\S]*?role.*?\}'
Length of output: 180 Script: #!/bin/bash
# Let's try a different approach to find API and interface definitions
# Search for updateUser API implementation
rg -g '*.ts' -g '*.tsx' 'updateUser.*\(' -A 5
# Search for User-related interfaces and types
rg -g '*.ts' -g '*.tsx' 'type.*User|interface.*User' -A 5
# Search for role-related validations
rg -g '*.ts' -g '*.tsx' 'role.*required|required.*role'
Length of output: 42066 app/server/appsmith-server/src/main/java/com/appsmith/server/services/ce/AnalyticsServiceCEImpl.java (2)
Setting role to empty string maintains analytics payload structure while removing dependency on UserData.role.
Removing the role parameter and setting role to empty string maintains consistency with identifyUser method. Also applies to: 164-164 app/server/appsmith-server/src/main/java/com/appsmith/server/solutions/ce/UserSignupCEImpl.java (1)
Setting role to empty string in analytics properties aligns with the PR objective while preserving analytics pipeline stability. Let's verify that this is the only place where role is used in analytics events: ✅ Verification successfulThe previous search might have been too specific. These new searches will help us:
Verified: Role field is only used once in analytics properties The search results confirm that in the analytics context, the role field is only used once in 🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Search for other analytics-related role usages
rg -l "analyticsProps.*role|role.*analyticsProps" --type java
Length of output: 63 Script: #!/bin/bash
# Let's try a broader search for analytics properties and role
rg -i "role.*analytics|analytics.*role" --type java -A 2 -B 2
# Also search for any role-related analytics constants
rg "ROLE.*=|=.*ROLE" --type java -A 1 -B 1
# Search for analytics event related code with role
ast-grep --pattern 'analyticsService.sendEvent($$$role$$$)'
Length of output: 9229 app/server/appsmith-server/src/test/java/com/appsmith/server/services/UserServiceTest.java (1)Line range hint The test method has been correctly renamed and implementation properly focuses on name and use case updates. Let's verify that all role-related code has been removed: ✅ Verification successfulThe previous search showed role-related methods but we need to specifically verify the UserData class to ensure complete removal of the role field and related code. Role field removal is complete and test changes are correct The verification confirms that:
🏁 Scripts executedThe following scripts were executed for the analysis: Script: #!/bin/bash
# Description: Check for any remaining references to user role in the codebase
# Expect: No occurrences except in migration scripts or documentation
# Search for role-related method names and variables in Java files
rg -t java "updateNameRoleAndUseCase|getUserRole|setRole|getRole"
# Search for role field references in UserData related files
rg -t java "UserData.*role|role.*UserData"
Length of output: 6350 Script: #!/bin/bash
# Let's search for any remaining references to role in UserData class and related files
ast-grep --pattern 'class UserData {
$$$
$role
$$$
}'
# Also check for any role-related methods in UserData
ast-grep --pattern 'class UserData {
$$$
$role($$$) {
$$$
}
$$$
}'
# Search for UserData references in test files
rg -t java "class.*UserData|UserData.*class" -A 5
Length of output: 9823 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
Documentation and Community
|
sharat87
changed the title
UserData.roleUserData.role
github-actions
bot
added
the
skip-changelog
Failed server tests
|
sharat87
requested review from
mohanarpit,
nidhi-nair,
trishaanand and
abhvsn
The `role` field is not sent by the client and is not used by the server anywhere. We're not removing the `role` field in analytics payloads, since changes like that in the past have broken other analytics pipelines (where a field value was `null` instead of `""` started to throw NPEs in some internal workflows). We can solve that as a separate problem later. ## Automation /test sanity authentication ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11830985060> > Commit: 986fc89 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11830985060&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity, @tag.Authentication` > Spec: > <hr>Thu, 14 Nov 2024 05:59:04 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Simplified user update requests by removing the `role` field, streamlining user data management. - **Bug Fixes** - Adjusted analytics tracking to exclude user role information, ensuring compliance with updated data handling practices. - **Documentation** - Updated test cases to reflect changes in user role management, focusing on name and use case updates instead. These changes enhance the user experience by simplifying user management and improving data privacy in analytics. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
The `role` field is not sent by the client and is not used by the server anywhere. We're not removing the `role` field in analytics payloads, since changes like that in the past have broken other analytics pipelines (where a field value was `null` instead of `""` started to throw NPEs in some internal workflows). We can solve that as a separate problem later. ## Automation /test sanity authentication ### 🔍 Cypress test results <!-- This is an auto-generated comment: Cypress test results --> > [!TIP] > 🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉 > Workflow run: <https://github.com/appsmithorg/appsmith/actions/runs/11830985060> > Commit: 986fc89 > <a href="https://internal.appsmith.com/app/cypress-dashboard/rundetails-65890b3c81d7400d08fa9ee5?branch=master&workflowId=11830985060&attempt=2" target="_blank">Cypress dashboard</a>. > Tags: `@tag.Sanity, @tag.Authentication` > Spec: > <hr>Thu, 14 Nov 2024 05:59:04 UTC <!-- end of auto-generated comment: Cypress test results --> ## Communication Should the DevRel and Marketing teams inform users about this change? - [ ] Yes - [x] No <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit ## Release Notes - **New Features** - Simplified user update requests by removing the `role` field, streamlining user data management. - **Bug Fixes** - Adjusted analytics tracking to exclude user role information, ensuring compliance with updated data handling practices. - **Documentation** - Updated test cases to reflect changes in user role management, focusing on name and use case updates instead. These changes enhance the user experience by simplifying user management and improving data privacy in analytics. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
The
rolefield is not sent by the client and is not used by the server anywhere.We're not removing the
rolefield in analytics payloads, since changes like that in the past have broken other analytics pipelines (where a field value wasnullinstead of""started to throw NPEs in some internal workflows). We can solve that as a separate problem later.Automation
/test sanity authentication
🔍 Cypress test results
Tip
🟢 🟢 🟢 All cypress tests have passed! 🎉 🎉 🎉
Workflow run: https://github.com/appsmithorg/appsmith/actions/runs/11830985060
Commit: 986fc89
Cypress dashboard.
Tags:
@tag.Sanity, @tag.AuthenticationSpec:
Thu, 14 Nov 2024 05:59:04 UTC
Communication
Should the DevRel and Marketing teams inform users about this change?
Summary by CodeRabbit
Release Notes
New Features
rolefield, streamlining user data management.Bug Fixes
Documentation
These changes enhance the user experience by simplifying user management and improving data privacy in analytics.