chore: Remove unused UserData.role (#37381)
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: 986fc8986a81aa212eaed455e22181cf927002f0 > <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 -->
This commit is contained in:
parent
035d315cb8
commit
7e209d2615
|
|
@ -57,7 +57,6 @@ export interface UpdateUserRequest {
|
||||||
name?: string;
|
name?: string;
|
||||||
email?: string;
|
email?: string;
|
||||||
proficiency?: string;
|
proficiency?: string;
|
||||||
role?: string;
|
|
||||||
useCase?: string;
|
useCase?: string;
|
||||||
intercomConsentGiven?: boolean;
|
intercomConsentGiven?: boolean;
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -417,14 +417,13 @@ export function* inviteUsers(
|
||||||
|
|
||||||
export function* updateUserDetailsSaga(action: ReduxAction<UpdateUserRequest>) {
|
export function* updateUserDetailsSaga(action: ReduxAction<UpdateUserRequest>) {
|
||||||
try {
|
try {
|
||||||
const { email, intercomConsentGiven, name, proficiency, role, useCase } =
|
const { email, intercomConsentGiven, name, proficiency, useCase } =
|
||||||
action.payload;
|
action.payload;
|
||||||
|
|
||||||
const response: ApiResponse = yield callAPI(UserApi.updateUser, {
|
const response: ApiResponse = yield callAPI(UserApi.updateUser, {
|
||||||
email,
|
email,
|
||||||
name,
|
name,
|
||||||
proficiency,
|
proficiency,
|
||||||
role,
|
|
||||||
useCase,
|
useCase,
|
||||||
intercomConsentGiven,
|
intercomConsentGiven,
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -33,11 +33,6 @@ public class UserData extends BaseDomain {
|
||||||
@JsonView(Views.Internal.class)
|
@JsonView(Views.Internal.class)
|
||||||
String userId;
|
String userId;
|
||||||
|
|
||||||
// Role of the user in their workspace, example, Designer, Developer, Product Lead etc.
|
|
||||||
@JsonView(Views.Public.class)
|
|
||||||
@Deprecated
|
|
||||||
private String role;
|
|
||||||
|
|
||||||
// The development proficiency of the user for example, Beginner, Novice, Intermediate, Advanced.
|
// The development proficiency of the user for example, Beginner, Novice, Intermediate, Advanced.
|
||||||
@JsonView(Views.Public.class)
|
@JsonView(Views.Public.class)
|
||||||
private String proficiency;
|
private String proficiency;
|
||||||
|
|
|
||||||
|
|
@ -19,9 +19,6 @@ public class UserSignupRequestDTO {
|
||||||
|
|
||||||
private String password;
|
private String password;
|
||||||
|
|
||||||
@Deprecated
|
|
||||||
private String role;
|
|
||||||
|
|
||||||
private String proficiency;
|
private String proficiency;
|
||||||
|
|
||||||
private String useCase;
|
private String useCase;
|
||||||
|
|
|
||||||
|
|
@ -9,8 +9,6 @@ import lombok.Data;
|
||||||
public class UserUpdateCE_DTO {
|
public class UserUpdateCE_DTO {
|
||||||
private String name;
|
private String name;
|
||||||
|
|
||||||
private String role;
|
|
||||||
|
|
||||||
private String proficiency;
|
private String proficiency;
|
||||||
|
|
||||||
private String useCase;
|
private String useCase;
|
||||||
|
|
@ -22,6 +20,6 @@ public class UserUpdateCE_DTO {
|
||||||
}
|
}
|
||||||
|
|
||||||
public boolean hasUserDataUpdates() {
|
public boolean hasUserDataUpdates() {
|
||||||
return role != null || proficiency != null || useCase != null || isIntercomConsentGiven;
|
return proficiency != null || useCase != null || isIntercomConsentGiven;
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -17,13 +17,7 @@ public interface AnalyticsServiceCE {
|
||||||
Mono<User> identifyUser(User user, UserData userData, String recentlyUsedWorkspaceId);
|
Mono<User> identifyUser(User user, UserData userData, String recentlyUsedWorkspaceId);
|
||||||
|
|
||||||
void identifyInstance(
|
void identifyInstance(
|
||||||
String instanceId,
|
String instanceId, String proficiency, String useCase, String adminEmail, String adminFullName, String ip);
|
||||||
String role,
|
|
||||||
String proficiency,
|
|
||||||
String useCase,
|
|
||||||
String adminEmail,
|
|
||||||
String adminFullName,
|
|
||||||
String ip);
|
|
||||||
|
|
||||||
Mono<Void> sendEvent(String event, String userId, Map<String, ?> properties);
|
Mono<Void> sendEvent(String event, String userId, Map<String, ?> properties);
|
||||||
|
|
||||||
|
|
|
||||||
|
|
@ -141,7 +141,7 @@ public class AnalyticsServiceCEImpl implements AnalyticsServiceCE {
|
||||||
"isSuperUser", isSuperUser,
|
"isSuperUser", isSuperUser,
|
||||||
"instanceId", instanceId,
|
"instanceId", instanceId,
|
||||||
"mostRecentlyUsedWorkspaceId", tuple.getT4(),
|
"mostRecentlyUsedWorkspaceId", tuple.getT4(),
|
||||||
"role", ObjectUtils.defaultIfNull(userData.getRole(), ""),
|
"role", "",
|
||||||
"proficiency", ObjectUtils.defaultIfNull(userData.getProficiency(), ""),
|
"proficiency", ObjectUtils.defaultIfNull(userData.getProficiency(), ""),
|
||||||
"goal", ObjectUtils.defaultIfNull(userData.getUseCase(), ""))));
|
"goal", ObjectUtils.defaultIfNull(userData.getUseCase(), ""))));
|
||||||
analytics.flush();
|
analytics.flush();
|
||||||
|
|
@ -150,13 +150,7 @@ public class AnalyticsServiceCEImpl implements AnalyticsServiceCE {
|
||||||
}
|
}
|
||||||
|
|
||||||
public void identifyInstance(
|
public void identifyInstance(
|
||||||
String instanceId,
|
String instanceId, String proficiency, String useCase, String adminEmail, String adminFullName, String ip) {
|
||||||
String role,
|
|
||||||
String proficiency,
|
|
||||||
String useCase,
|
|
||||||
String adminEmail,
|
|
||||||
String adminFullName,
|
|
||||||
String ip) {
|
|
||||||
if (!isActive()) {
|
if (!isActive()) {
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
|
|
@ -167,7 +161,7 @@ public class AnalyticsServiceCEImpl implements AnalyticsServiceCE {
|
||||||
"isInstance",
|
"isInstance",
|
||||||
true, // Is this "identify" data-point for a user or an instance?
|
true, // Is this "identify" data-point for a user or an instance?
|
||||||
ROLE,
|
ROLE,
|
||||||
ObjectUtils.defaultIfNull(role, ""),
|
"",
|
||||||
PROFICIENCY,
|
PROFICIENCY,
|
||||||
ObjectUtils.defaultIfNull(proficiency, ""),
|
ObjectUtils.defaultIfNull(proficiency, ""),
|
||||||
GOAL,
|
GOAL,
|
||||||
|
|
|
||||||
|
|
@ -618,9 +618,6 @@ public class UserServiceCEImpl extends BaseService<UserRepository, User, String>
|
||||||
|
|
||||||
if (allUpdates.hasUserDataUpdates()) {
|
if (allUpdates.hasUserDataUpdates()) {
|
||||||
final UserData updates = new UserData();
|
final UserData updates = new UserData();
|
||||||
if (StringUtils.hasLength(allUpdates.getRole())) {
|
|
||||||
updates.setRole(allUpdates.getRole());
|
|
||||||
}
|
|
||||||
if (StringUtils.hasLength(allUpdates.getProficiency())) {
|
if (StringUtils.hasLength(allUpdates.getProficiency())) {
|
||||||
updates.setProficiency(allUpdates.getProficiency());
|
updates.setProficiency(allUpdates.getProficiency());
|
||||||
}
|
}
|
||||||
|
|
|
||||||
|
|
@ -300,7 +300,6 @@ public class UserSignupCEImpl implements UserSignupCE {
|
||||||
})
|
})
|
||||||
.flatMap(user -> {
|
.flatMap(user -> {
|
||||||
final UserData userData = new UserData();
|
final UserData userData = new UserData();
|
||||||
userData.setRole(userFromRequest.getRole());
|
|
||||||
userData.setProficiency(userFromRequest.getProficiency());
|
userData.setProficiency(userFromRequest.getProficiency());
|
||||||
userData.setUseCase(userFromRequest.getUseCase());
|
userData.setUseCase(userFromRequest.getUseCase());
|
||||||
|
|
||||||
|
|
@ -380,9 +379,6 @@ public class UserSignupCEImpl implements UserSignupCE {
|
||||||
if (formData.containsKey(FieldName.NAME)) {
|
if (formData.containsKey(FieldName.NAME)) {
|
||||||
user.setName(formData.getFirst(FieldName.NAME));
|
user.setName(formData.getFirst(FieldName.NAME));
|
||||||
}
|
}
|
||||||
if (formData.containsKey("role")) {
|
|
||||||
user.setRole(formData.getFirst("role"));
|
|
||||||
}
|
|
||||||
if (formData.containsKey("proficiency")) {
|
if (formData.containsKey("proficiency")) {
|
||||||
user.setProficiency(formData.getFirst("proficiency"));
|
user.setProficiency(formData.getFirst("proficiency"));
|
||||||
}
|
}
|
||||||
|
|
@ -446,7 +442,7 @@ public class UserSignupCEImpl implements UserSignupCE {
|
||||||
analyticsProps.put(DISABLE_TELEMETRY, !userFromRequest.isAllowCollectingAnonymousData());
|
analyticsProps.put(DISABLE_TELEMETRY, !userFromRequest.isAllowCollectingAnonymousData());
|
||||||
analyticsProps.put(SUBSCRIBE_MARKETING, userFromRequest.isSignupForNewsletter());
|
analyticsProps.put(SUBSCRIBE_MARKETING, userFromRequest.isSignupForNewsletter());
|
||||||
analyticsProps.put(EMAIL, newsletterSignedUpUserEmail);
|
analyticsProps.put(EMAIL, newsletterSignedUpUserEmail);
|
||||||
analyticsProps.put(ROLE, ObjectUtils.defaultIfNull(userData.getRole(), ""));
|
analyticsProps.put(ROLE, "");
|
||||||
analyticsProps.put(PROFICIENCY, ObjectUtils.defaultIfNull(userData.getProficiency(), ""));
|
analyticsProps.put(PROFICIENCY, ObjectUtils.defaultIfNull(userData.getProficiency(), ""));
|
||||||
analyticsProps.put(GOAL, ObjectUtils.defaultIfNull(userData.getUseCase(), ""));
|
analyticsProps.put(GOAL, ObjectUtils.defaultIfNull(userData.getUseCase(), ""));
|
||||||
// ip is a reserved keyword for tracking events in Mixpanel though this is allowed in
|
// ip is a reserved keyword for tracking events in Mixpanel though this is allowed in
|
||||||
|
|
@ -460,7 +456,6 @@ public class UserSignupCEImpl implements UserSignupCE {
|
||||||
|
|
||||||
analyticsService.identifyInstance(
|
analyticsService.identifyInstance(
|
||||||
instanceId,
|
instanceId,
|
||||||
userData.getRole(),
|
|
||||||
userData.getProficiency(),
|
userData.getProficiency(),
|
||||||
userData.getUseCase(),
|
userData.getUseCase(),
|
||||||
newsletterSignedUpUserEmail,
|
newsletterSignedUpUserEmail,
|
||||||
|
|
|
||||||
|
|
@ -22,7 +22,6 @@ public class CommonConfigTest {
|
||||||
@Test
|
@Test
|
||||||
public void objectMapper_BeanCreated_WithPublicJsonViewAsDefault() throws JsonProcessingException {
|
public void objectMapper_BeanCreated_WithPublicJsonViewAsDefault() throws JsonProcessingException {
|
||||||
UserData userData = new UserData();
|
UserData userData = new UserData();
|
||||||
userData.setRole("new_role");
|
|
||||||
userData.setProficiency("abcd"); // this is public field
|
userData.setProficiency("abcd"); // this is public field
|
||||||
userData.setUserId("userId"); // this is internal field
|
userData.setUserId("userId"); // this is internal field
|
||||||
userData.setUserPermissions(null);
|
userData.setUserPermissions(null);
|
||||||
|
|
|
||||||
|
|
@ -425,21 +425,6 @@ public class UserServiceTest {
|
||||||
.verifyComplete();
|
.verifyComplete();
|
||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
|
||||||
@WithUserDetails(value = "api_user")
|
|
||||||
public void updateRoleOfUser() {
|
|
||||||
UserUpdateDTO updateUser = new UserUpdateDTO();
|
|
||||||
updateUser.setRole("New role of user");
|
|
||||||
final Mono<UserData> resultMono =
|
|
||||||
userService.updateCurrentUser(updateUser, null).then(userDataService.getForUserEmail("api_user"));
|
|
||||||
StepVerifier.create(resultMono)
|
|
||||||
.assertNext(userData -> {
|
|
||||||
assertNotNull(userData);
|
|
||||||
assertThat(userData.getRole()).isEqualTo("New role of user");
|
|
||||||
})
|
|
||||||
.verifyComplete();
|
|
||||||
}
|
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
@WithUserDetails(value = "api_user")
|
@WithUserDetails(value = "api_user")
|
||||||
public void updateIntercomConsentOfUser() {
|
public void updateIntercomConsentOfUser() {
|
||||||
|
|
@ -499,10 +484,9 @@ public class UserServiceTest {
|
||||||
|
|
||||||
@Test
|
@Test
|
||||||
@WithUserDetails(value = "api_user")
|
@WithUserDetails(value = "api_user")
|
||||||
public void updateNameRoleAndUseCaseOfUser() {
|
public void updateNameAndUseCaseOfUser() {
|
||||||
UserUpdateDTO updateUser = new UserUpdateDTO();
|
UserUpdateDTO updateUser = new UserUpdateDTO();
|
||||||
updateUser.setName("New name of user here");
|
updateUser.setName("New name of user here");
|
||||||
updateUser.setRole("New role of user");
|
|
||||||
updateUser.setUseCase("New use case");
|
updateUser.setUseCase("New use case");
|
||||||
final Mono<Tuple2<User, UserData>> resultMono = userService
|
final Mono<Tuple2<User, UserData>> resultMono = userService
|
||||||
.updateCurrentUser(updateUser, null)
|
.updateCurrentUser(updateUser, null)
|
||||||
|
|
@ -514,7 +498,6 @@ public class UserServiceTest {
|
||||||
assertNotNull(user);
|
assertNotNull(user);
|
||||||
assertNotNull(userData);
|
assertNotNull(userData);
|
||||||
assertEquals("New name of user here", user.getName());
|
assertEquals("New name of user here", user.getName());
|
||||||
assertEquals("New role of user", userData.getRole());
|
|
||||||
assertEquals("New use case", userData.getUseCase());
|
assertEquals("New use case", userData.getUseCase());
|
||||||
})
|
})
|
||||||
.verifyComplete();
|
.verifyComplete();
|
||||||
|
|
|
||||||
Loading…
Reference in New Issue
Block a user