chore: Added new analytics parameter for git auto commit (#29538)

## Description
A refactor in the analytics events for Git. Also adds
isSystemGenerated=false for regular commits.

#### PR fixes following issue(s)
Fixes #26769

#### Media
> A video or a GIF is preferred. when using Loom, don’t embed because it
looks like it’s a GIF. instead, just link to the video
>
>
#### Type of change
> Please delete options that are not relevant.
- Bug fix (non-breaking change which fixes an issue)
- New feature (non-breaking change which adds functionality)
- Breaking change (fix or feature that would cause existing
functionality to not work as expected)
- Chore (housekeeping or task changes that don't impact user perception)
- This change requires a documentation update
>
>
>
## Testing
>
#### How Has This Been Tested?
> Please describe the tests that you ran to verify your changes. Also
list any relevant details for your test configuration.
> Delete anything that is not relevant
- [ ] Manual
- [ ] JUnit
- [ ] Jest
- [ ] Cypress
>
>
#### Test Plan
> Add Testsmith test cases links that relate to this PR
>
>
#### Issues raised during DP testing
> Link issues raised during DP testing for better visiblity and tracking
(copy link from comments dropped on this PR)
>
>
>
## Checklist:
#### Dev activity
- [ ] My code follows the style guidelines of this project
- [ ] I have performed a self-review of my own code
- [ ] I have commented my code, particularly in hard-to-understand areas
- [ ] I have made corresponding changes to the documentation
- [ ] My changes generate no new warnings
- [ ] I have added tests that prove my fix is effective or that my
feature works
- [ ] New and existing unit tests pass locally with my changes
- [ ] PR is being merged under a feature flag


#### QA activity:
- [ ] [Speedbreak
features](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#speedbreakers-)
have been covered
- [ ] Test plan covers all impacted features and [areas of
interest](https://github.com/appsmithorg/TestSmith/wiki/Guidelines-for-test-plans#areas-of-interest-)
- [ ] Test plan has been peer reviewed by project stakeholders and other
QA members
- [ ] Manually tested functionality on DP
- [ ] We had an implementation alignment call with stakeholders post QA
Round 2
- [ ] Cypress test cases have been added and approved by SDET/manual QA
- [ ] Added `Test Plan Approved` label after Cypress tests were reviewed
- [ ] Added `Test Plan Approved` label after JUnit tests were reviewed


<!-- This is an auto-generated comment: release notes by coderabbit.ai
-->

## Summary by CodeRabbit

- **New Features**
- Enhanced Git integration with the inclusion of repository URLs in
auto-commit events.
- Improved analytics tracking by utilizing repository URLs for version
information.

- **Refactor**
- Standardized event naming by replacing string literals with enum
constants in Git-related operations.

- **Bug Fixes**
- Fixed an issue in the analytics service by correcting the method
signature for user ID hashing.

- **Documentation**
- Updated internal documentation to reflect changes in analytics and Git
service logic.

- **Tests**
- Expanded test coverage to account for new repository URL handling in
auto-commit events.

<!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
Nayan 2023-12-13 23:47:45 +06:00 committed by GitHub
parent e2d1ff7f91
commit d83f4e2fb1
No known key found for this signature in database
GPG Key ID: 4AEE18F83AFDEB23
6 changed files with 54 additions and 46 deletions

View File

@ -16,4 +16,5 @@ public class AutoCommitEvent {
private String repoName;
private String authorName;
private String authorEmail;
private String repoUrl;
}

View File

@ -104,6 +104,7 @@ public class GitAutoCommitHelperImpl implements GitAutoCommitHelper {
autoCommitEvent.setWorkspaceId(application.getWorkspaceId());
autoCommitEvent.setAuthorName(gitProfile.getAuthorName());
autoCommitEvent.setAuthorEmail(gitProfile.getAuthorEmail());
autoCommitEvent.setRepoUrl(gitApplicationMetadata.getRemoteUrl());
// it's a synchronous call, no need to return anything
autoCommitEventHandler.publish(autoCommitEvent);
return Boolean.TRUE;

View File

@ -371,9 +371,10 @@ public class AnalyticsServiceCEImpl implements AnalyticsServiceCE {
/**
* Tells whether to hash userId or not for events
*
* @param String event
* @param String userId
* @param Boolean hashUserId
* @param event String
* @param userId String
* @param hashUserId Boolean
* @param isCloudHosting Boolean
* @return Boolean
*/
public static Boolean shouldHashUserId(String event, String userId, boolean hashUserId, boolean isCloudHosting) {

View File

@ -528,7 +528,7 @@ public class GitServiceCEImpl implements GitServiceCE {
String errorMessage = "Unable to find git author configuration for logged-in user. You can set "
+ "up a git profile from the user profile section.";
return addAnalyticsForGitOperation(
AnalyticsEvents.GIT_COMMIT.getEventName(),
AnalyticsEvents.GIT_COMMIT,
childApplication,
AppsmithError.INVALID_GIT_CONFIGURATION.getErrorType(),
AppsmithError.INVALID_GIT_CONFIGURATION.getMessage(errorMessage),
@ -552,7 +552,7 @@ public class GitServiceCEImpl implements GitServiceCE {
return Mono.just(EMPTY_COMMIT_ERROR_MESSAGE);
}
return addAnalyticsForGitOperation(
AnalyticsEvents.GIT_COMMIT.getEventName(),
AnalyticsEvents.GIT_COMMIT,
childApplication,
error.getClass().getName(),
error.getMessage(),
@ -610,7 +610,7 @@ public class GitServiceCEImpl implements GitServiceCE {
return Mono.just(application);
})
.then(addAnalyticsForGitOperation(
AnalyticsEvents.GIT_COMMIT.getEventName(),
AnalyticsEvents.GIT_COMMIT,
childApplication,
"",
"",
@ -737,7 +737,7 @@ public class GitServiceCEImpl implements GitServiceCE {
return Mono.just(application);
}
return addAnalyticsForGitOperation(
AnalyticsEvents.GIT_PRIVATE_REPO_LIMIT_EXCEEDED.getEventName(),
AnalyticsEvents.GIT_PRIVATE_REPO_LIMIT_EXCEEDED,
application,
AppsmithError.GIT_APPLICATION_LIMIT_ERROR.getErrorType(),
AppsmithError.GIT_APPLICATION_LIMIT_ERROR.getMessage(),
@ -764,7 +764,7 @@ public class GitServiceCEImpl implements GitServiceCE {
return fileUtils
.deleteLocalRepo(repoSuffix)
.then(addAnalyticsForGitOperation(
AnalyticsEvents.GIT_CONNECT.getEventName(),
AnalyticsEvents.GIT_CONNECT,
application,
error.getClass().getName(),
error.getMessage(),
@ -817,7 +817,7 @@ public class GitServiceCEImpl implements GitServiceCE {
boolean isRepoPrivate = objects.getT2();
if (!isEmpty) {
return addAnalyticsForGitOperation(
AnalyticsEvents.GIT_CONNECT.getEventName(),
AnalyticsEvents.GIT_CONNECT,
application,
AppsmithError.INVALID_GIT_REPO.getErrorType(),
AppsmithError.INVALID_GIT_REPO.getMessage(),
@ -934,8 +934,7 @@ public class GitServiceCEImpl implements GitServiceCE {
.flatMap(isDeleted -> {
if (error instanceof TransportException) {
return addAnalyticsForGitOperation(
AnalyticsEvents.GIT_CONNECT
.getEventName(),
AnalyticsEvents.GIT_CONNECT,
application,
error.getClass()
.getName(),
@ -955,7 +954,7 @@ public class GitServiceCEImpl implements GitServiceCE {
}));
})
.then(addAnalyticsForGitOperation(
AnalyticsEvents.GIT_CONNECT.getEventName(),
AnalyticsEvents.GIT_CONNECT,
application,
application.getGitApplicationMetadata().getIsRepoPrivate()))
.map(responseUtils::updateApplicationWithDefaultResources);
@ -1041,7 +1040,7 @@ public class GitServiceCEImpl implements GitServiceCE {
gitData.getBranchName())
.zipWith(Mono.just(application)))
.onErrorResume(error -> addAnalyticsForGitOperation(
AnalyticsEvents.GIT_PUSH.getEventName(),
AnalyticsEvents.GIT_PUSH,
application,
error.getClass().getName(),
error.getMessage(),
@ -1077,7 +1076,7 @@ public class GitServiceCEImpl implements GitServiceCE {
String pushStatus = tuple.getT1();
Application application = tuple.getT2();
return addAnalyticsForGitOperation(
AnalyticsEvents.GIT_PUSH.getEventName(),
AnalyticsEvents.GIT_PUSH,
application,
application.getGitApplicationMetadata().getIsRepoPrivate())
.thenReturn(pushStatus);
@ -1101,7 +1100,7 @@ public class GitServiceCEImpl implements GitServiceCE {
if (pushResult.contains("REJECTED_NONFASTFORWARD")) {
return addAnalyticsForGitOperation(
AnalyticsEvents.GIT_PUSH.getEventName(),
AnalyticsEvents.GIT_PUSH,
application,
AppsmithError.GIT_UPSTREAM_CHANGES.getErrorType(),
AppsmithError.GIT_UPSTREAM_CHANGES.getMessage(),
@ -1229,8 +1228,7 @@ public class GitServiceCEImpl implements GitServiceCE {
})
.collectList()
.flatMapMany(actionCollectionService::saveAll))
.then(addAnalyticsForGitOperation(
AnalyticsEvents.GIT_DISCONNECT.getEventName(), application, false))
.then(addAnalyticsForGitOperation(AnalyticsEvents.GIT_DISCONNECT, application, false))
.map(responseUtils::updateApplicationWithDefaultResources));
return Mono.create(sink -> disconnectMono.subscribe(sink::success, sink::error, null, sink.currentContext()));
@ -1368,7 +1366,7 @@ public class GitServiceCEImpl implements GitServiceCE {
.flatMap(application -> releaseFileLock(
application.getGitApplicationMetadata().getDefaultApplicationId())
.then(addAnalyticsForGitOperation(
AnalyticsEvents.GIT_CREATE_BRANCH.getEventName(),
AnalyticsEvents.GIT_CREATE_BRANCH,
application,
application.getGitApplicationMetadata().getIsRepoPrivate())))
.map(responseUtils::updateApplicationWithDefaultResources);
@ -1427,7 +1425,7 @@ public class GitServiceCEImpl implements GitServiceCE {
branchName, defaultApplicationId, applicationPermission.getReadPermission());
})
.flatMap(application -> addAnalyticsForGitOperation(
AnalyticsEvents.GIT_CHECKOUT_BRANCH.getEventName(),
AnalyticsEvents.GIT_CHECKOUT_BRANCH,
application,
application.getGitApplicationMetadata().getIsRepoPrivate()))
.map(responseUtils::updateApplicationWithDefaultResources);
@ -1543,7 +1541,7 @@ public class GitServiceCEImpl implements GitServiceCE {
.importApplicationInWorkspaceFromGit(
application.getWorkspaceId(), applicationJson, application.getId(), branchName)
.flatMap(application1 -> addAnalyticsForGitOperation(
AnalyticsEvents.GIT_CHECKOUT_REMOTE_BRANCH.getEventName(),
AnalyticsEvents.GIT_CHECKOUT_REMOTE_BRANCH,
application1,
Boolean.TRUE.equals(application1
.getGitApplicationMetadata()
@ -1763,7 +1761,7 @@ public class GitServiceCEImpl implements GitServiceCE {
.flatMap(gitBranchDTOList -> Boolean.FALSE.equals(pruneBranches)
? Mono.just(gitBranchDTOList)
: addAnalyticsForGitOperation(
AnalyticsEvents.GIT_PRUNE.getEventName(),
AnalyticsEvents.GIT_PRUNE,
rootApp,
rootApp.getGitApplicationMetadata().getIsRepoPrivate())
.thenReturn(gitBranchDTOList));
@ -2129,7 +2127,7 @@ public class GitServiceCEImpl implements GitServiceCE {
gitExecutor.mergeBranch(repoSuffix, sourceBranch, destinationBranch),
Mono.just(defaultApplication))
.onErrorResume(error -> addAnalyticsForGitOperation(
AnalyticsEvents.GIT_MERGE.getEventName(),
AnalyticsEvents.GIT_MERGE,
defaultApplication,
error.getClass().getName(),
error.getMessage(),
@ -2196,7 +2194,7 @@ public class GitServiceCEImpl implements GitServiceCE {
Application application = tuple.getT2();
// Send analytics event
return releaseFileLock(defaultApplicationId).flatMap(status -> addAnalyticsForGitOperation(
AnalyticsEvents.GIT_MERGE.getEventName(),
AnalyticsEvents.GIT_MERGE,
application,
application.getGitApplicationMetadata().getIsRepoPrivate())
.thenReturn(mergeStatusDTO));
@ -2239,7 +2237,7 @@ public class GitServiceCEImpl implements GitServiceCE {
.flatMap(srcBranchStatus -> {
if (!Integer.valueOf(0).equals(srcBranchStatus.getBehindCount())) {
return addAnalyticsForGitOperation(
AnalyticsEvents.GIT_MERGE_CHECK.getEventName(),
AnalyticsEvents.GIT_MERGE_CHECK,
application,
AppsmithError.GIT_MERGE_FAILED_LOCAL_CHANGES.name(),
AppsmithError.GIT_MERGE_FAILED_LOCAL_CHANGES.getMessage(
@ -2255,7 +2253,7 @@ public class GitServiceCEImpl implements GitServiceCE {
sourceBranch))));
} else if (!CollectionUtils.isNullOrEmpty(srcBranchStatus.getModified())) {
return addAnalyticsForGitOperation(
AnalyticsEvents.GIT_MERGE_CHECK.getEventName(),
AnalyticsEvents.GIT_MERGE_CHECK,
application,
AppsmithError.GIT_MERGE_FAILED_LOCAL_CHANGES.name(),
AppsmithError.GIT_MERGE_FAILED_LOCAL_CHANGES.getMessage(
@ -2272,7 +2270,7 @@ public class GitServiceCEImpl implements GitServiceCE {
.map(destBranchStatus -> {
if (!Integer.valueOf(0).equals(destBranchStatus.getBehindCount())) {
return addAnalyticsForGitOperation(
AnalyticsEvents.GIT_MERGE_CHECK.getEventName(),
AnalyticsEvents.GIT_MERGE_CHECK,
application,
AppsmithError.GIT_MERGE_FAILED_REMOTE_CHANGES.name(),
AppsmithError.GIT_MERGE_FAILED_REMOTE_CHANGES
@ -2290,7 +2288,7 @@ public class GitServiceCEImpl implements GitServiceCE {
destinationBranch))));
} else if (!CollectionUtils.isNullOrEmpty(destBranchStatus.getModified())) {
return addAnalyticsForGitOperation(
AnalyticsEvents.GIT_MERGE_CHECK.getEventName(),
AnalyticsEvents.GIT_MERGE_CHECK,
application,
AppsmithError.GIT_MERGE_FAILED_LOCAL_CHANGES.name(),
AppsmithError.GIT_MERGE_FAILED_LOCAL_CHANGES.getMessage(
@ -2319,7 +2317,7 @@ public class GitServiceCEImpl implements GitServiceCE {
.isMergeBranch(repoSuffix, sourceBranch, destinationBranch)
.flatMap(mergeStatusDTO -> releaseFileLock(defaultApplicationId)
.flatMap(mergeStatus -> addAnalyticsForGitOperation(
AnalyticsEvents.GIT_MERGE_CHECK.getEventName(),
AnalyticsEvents.GIT_MERGE_CHECK,
application,
null,
null,
@ -2347,7 +2345,7 @@ public class GitServiceCEImpl implements GitServiceCE {
return mergeStatus;
})
.flatMap(mergeStatusDTO -> addAnalyticsForGitOperation(
AnalyticsEvents.GIT_MERGE_CHECK.getEventName(),
AnalyticsEvents.GIT_MERGE_CHECK,
application,
error.getClass().getName(),
error.getMessage(),
@ -2476,7 +2474,7 @@ public class GitServiceCEImpl implements GitServiceCE {
return Mono.just(gitAuth).zipWith(applicationMono);
}
return addAnalyticsForGitOperation(
AnalyticsEvents.GIT_IMPORT.getEventName(),
AnalyticsEvents.GIT_IMPORT,
newApplication,
AppsmithError.GIT_APPLICATION_LIMIT_ERROR.getErrorType(),
AppsmithError.GIT_APPLICATION_LIMIT_ERROR.getMessage(),
@ -2501,7 +2499,7 @@ public class GitServiceCEImpl implements GitServiceCE {
.onErrorResume(error -> {
log.error("Error while cloning the remote repo, {}", error.getMessage());
return addAnalyticsForGitOperation(
AnalyticsEvents.GIT_IMPORT.getEventName(),
AnalyticsEvents.GIT_IMPORT,
application,
error.getClass().getName(),
error.getMessage(),
@ -2626,7 +2624,7 @@ public class GitServiceCEImpl implements GitServiceCE {
.flatMap(applicationImportDTO -> {
Application application = applicationImportDTO.getApplication();
return addAnalyticsForGitOperation(
AnalyticsEvents.GIT_IMPORT.getEventName(),
AnalyticsEvents.GIT_IMPORT,
application,
application.getGitApplicationMetadata().getIsRepoPrivate())
.thenReturn(applicationImportDTO);
@ -2682,7 +2680,7 @@ public class GitServiceCEImpl implements GitServiceCE {
+ gitApplicationMetadata.getRemoteUrl() + " ",
error);
return addAnalyticsForGitOperation(
AnalyticsEvents.GIT_TEST_CONNECTION.getEventName(),
AnalyticsEvents.GIT_TEST_CONNECTION,
application,
error.getClass().getName(),
error.getMessage(),
@ -2710,7 +2708,7 @@ public class GitServiceCEImpl implements GitServiceCE {
.flatMap(objects -> {
Application application = objects.getT2();
return addAnalyticsForGitOperation(
AnalyticsEvents.GIT_TEST_CONNECTION.getEventName(),
AnalyticsEvents.GIT_TEST_CONNECTION,
application,
application.getGitApplicationMetadata().getIsRepoPrivate())
.thenReturn(objects.getT1());
@ -2779,7 +2777,7 @@ public class GitServiceCEImpl implements GitServiceCE {
.onErrorResume(throwable -> {
log.warn("Unable to find branch with name ", throwable);
return addAnalyticsForGitOperation(
AnalyticsEvents.GIT_DELETE_BRANCH.getEventName(),
AnalyticsEvents.GIT_DELETE_BRANCH,
application,
throwable.getClass().getName(),
throwable.getMessage(),
@ -2789,7 +2787,7 @@ public class GitServiceCEImpl implements GitServiceCE {
});
})
.flatMap(application -> addAnalyticsForGitOperation(
AnalyticsEvents.GIT_DELETE_BRANCH.getEventName(),
AnalyticsEvents.GIT_DELETE_BRANCH,
application,
application.getGitApplicationMetadata().getIsRepoPrivate()))
.map(responseUtils::updateApplicationWithDefaultResources);
@ -2854,8 +2852,7 @@ public class GitServiceCEImpl implements GitServiceCE {
.flatMap(application -> publishAndOrGetApplication(application.getId(), true));
})
.flatMap(application -> releaseFileLock(defaultApplicationId)
.then(this.addAnalyticsForGitOperation(
AnalyticsEvents.GIT_DISCARD_CHANGES.getEventName(), application, null)))
.then(this.addAnalyticsForGitOperation(AnalyticsEvents.GIT_DISCARD_CHANGES, application, null)))
.map(responseUtils::updateApplicationWithDefaultResources);
return Mono.create(
@ -3054,7 +3051,7 @@ public class GitServiceCEImpl implements GitServiceCE {
branchedApplication.getId(),
branchName)
.flatMap(application -> addAnalyticsForGitOperation(
AnalyticsEvents.GIT_PULL.getEventName(),
AnalyticsEvents.GIT_PULL,
application,
application
.getGitApplicationMetadata()
@ -3084,28 +3081,32 @@ public class GitServiceCEImpl implements GitServiceCE {
}
private Mono<Application> addAnalyticsForGitOperation(
String eventName, Application application, Boolean isRepoPrivate) {
AnalyticsEvents eventName, Application application, Boolean isRepoPrivate) {
return addAnalyticsForGitOperation(eventName, application, "", "", isRepoPrivate, false);
}
private Mono<Application> addAnalyticsForGitOperation(
String eventName, Application application, String errorType, String errorMessage, Boolean isRepoPrivate) {
AnalyticsEvents eventName,
Application application,
String errorType,
String errorMessage,
Boolean isRepoPrivate) {
return addAnalyticsForGitOperation(eventName, application, errorType, errorMessage, isRepoPrivate, false);
}
private Mono<Application> addAnalyticsForGitOperation(
String eventName,
AnalyticsEvents event,
Application application,
String errorType,
String errorMessage,
Boolean isRepoPrivate,
Boolean isSystemGenerated) {
return addAnalyticsForGitOperation(
eventName, application, errorType, errorMessage, isRepoPrivate, isSystemGenerated, null);
event, application, errorType, errorMessage, isRepoPrivate, isSystemGenerated, null);
}
private Mono<Application> addAnalyticsForGitOperation(
String eventName,
AnalyticsEvents event,
Application application,
String errorType,
String errorMessage,
@ -3120,6 +3121,9 @@ public class GitServiceCEImpl implements GitServiceCE {
analyticsProps.put(FieldName.BRANCH_NAME, gitData.getBranchName());
analyticsProps.put(FieldName.GIT_HOSTING_PROVIDER, GitUtils.getGitProviderName(gitData.getRemoteUrl()));
analyticsProps.put(FieldName.REPO_URL, gitData.getRemoteUrl());
if (event == AnalyticsEvents.GIT_COMMIT) {
analyticsProps.put("isAutoCommit", false);
}
}
// Do not include the error data points in the map for success states
if (!StringUtils.isEmptyOrNull(errorMessage) || !StringUtils.isEmptyOrNull(errorType)) {
@ -3145,7 +3149,7 @@ public class GitServiceCEImpl implements GitServiceCE {
Map.of(FieldName.APP_MODE, ApplicationMode.EDIT.toString(), FieldName.APPLICATION, application);
analyticsProps.put(FieldName.EVENT_DATA, eventData);
return sessionUserService.getCurrentUser().flatMap(user -> analyticsService
.sendEvent(eventName, user.getUsername(), analyticsProps)
.sendEvent(event.getEventName(), user.getUsername(), analyticsProps)
.thenReturn(application));
}

View File

@ -186,7 +186,7 @@ public class AutoCommitEventHandlerCEImpl implements AutoCommitEventHandlerCE {
analyticsProps.put("orgId", autoCommitEvent.getWorkspaceId());
analyticsProps.put("isSystemGenerated", true);
analyticsProps.put("isAutoCommit", true);
analyticsProps.put("version", projectProperties.getVersion());
analyticsProps.put("repoUrl", autoCommitEvent.getRepoUrl());
return analyticsService
.sendEvent(

View File

@ -116,6 +116,7 @@ public class AutoCommitEventHandlerImplTest {
autoCommitEvent.setAuthorName("test author");
autoCommitEvent.setAuthorEmail("testauthor@example.com");
autoCommitEvent.setWorkspaceId("test-workspace-id");
autoCommitEvent.setRepoUrl("git@example.com:exampleorg/example-repo.git");
return autoCommitEvent;
}