From 0af0131062ef2d518ca8f36b0de65bc0a4d579f1 Mon Sep 17 00:00:00 2001 From: Harold Wanyama Date: Thu, 4 Jul 2024 13:55:27 +0300 Subject: [PATCH] [#4244] Feature/GitLab MR Comments - Handle author and assocaited commits Signed-off-by: Harold Wanyama --- cla-backend-go/gitlab_api/client.go | 49 ++++++- .../gitlab_api/mocks/mock_client.go | 79 +++++++++++ cla-backend-go/gitlab_api/mr.go | 49 ++++--- cla-backend-go/gitlab_api/mr_test.go | 124 ++++++++++++++++++ .../v2/gitlab-activity/service_test.go | 109 +++++++++++++++ cla-backend-go/v2/signatures/service_test.go | 2 +- 6 files changed, 390 insertions(+), 22 deletions(-) create mode 100644 cla-backend-go/gitlab_api/mocks/mock_client.go create mode 100644 cla-backend-go/gitlab_api/mr_test.go diff --git a/cla-backend-go/gitlab_api/client.go b/cla-backend-go/gitlab_api/client.go index 9566aefeb..110c00784 100644 --- a/cla-backend-go/gitlab_api/client.go +++ b/cla-backend-go/gitlab_api/client.go @@ -19,6 +19,17 @@ import ( goGitLab "github.com/xanzy/go-gitlab" ) +type GitLabClient interface { + GetMergeRequestCommits(projectID int, mergeID int, opts *goGitLab.GetMergeRequestCommitsOptions) ([]*goGitLab.Commit, error) + ListUsers(opts *goGitLab.ListUsersOptions) ([]*goGitLab.User, error) + SetCommitStatus(projectID int, commitSHA string, opts *goGitLab.SetCommitStatusOptions) error +} + +// Client is the gitlab client +type GitLabClientWrapper struct { + gitlabClient *goGitLab.Client +} + // OauthSuccessResponse is success response from Gitlab type OauthSuccessResponse struct { AccessToken string `json:"access_token"` @@ -29,7 +40,7 @@ type OauthSuccessResponse struct { } // NewGitlabOauthClient creates a new gitlab client from the given oauth info, authInfo is encrypted -func NewGitlabOauthClient(authInfo string, gitLabApp *App) (*goGitLab.Client, error) { +func NewGitlabOauthClient(authInfo string, gitLabApp *App) (GitLabClient, error) { if authInfo == "" { return nil, errors.New("unable to decrypt auth info - authentication info input is nil") } @@ -47,7 +58,14 @@ func NewGitlabOauthClient(authInfo string, gitLabApp *App) (*goGitLab.Client, er } log.Infof("creating oauth client with access token : %s", oauthResp.AccessToken) - return goGitLab.NewOAuthClient(oauthResp.AccessToken) + client, err := goGitLab.NewOAuthClient(oauthResp.AccessToken) + if err != nil { + return nil, err + } + + return &GitLabClientWrapper{ + gitlabClient: client, + }, nil } // NewGitlabOauthClientFromAccessToken creates a new gitlab client from the given access token @@ -154,3 +172,30 @@ func decrypt(key, cipherText []byte) ([]byte, error) { return cipherText, nil } + +// GetMergeRequestCommits returns the commits for the given merge request +func (c *GitLabClientWrapper) GetMergeRequestCommits(projectID int, mergeID int, opts *goGitLab.GetMergeRequestCommitsOptions) ([]*goGitLab.Commit, error) { + commits, _, err := c.gitlabClient.MergeRequests.GetMergeRequestCommits(projectID, mergeID, opts) + if err != nil { + return nil, err + } + return commits, nil +} + +// ListUsers returns the list of users +func (c *GitLabClientWrapper) ListUsers(opts *goGitLab.ListUsersOptions) ([]*goGitLab.User, error) { + users, _, err := c.gitlabClient.Users.ListUsers(opts) + if err != nil { + return nil, err + } + return users, nil +} + +// SetCommitStatus sets the commit status +func (c *GitLabClientWrapper) SetCommitStatus(projectID int, commitSHA string, opts *goGitLab.SetCommitStatusOptions) error { + _, _, err := c.gitlabClient.Commits.SetCommitStatus(projectID, commitSHA, opts) + if err != nil { + return err + } + return nil +} diff --git a/cla-backend-go/gitlab_api/mocks/mock_client.go b/cla-backend-go/gitlab_api/mocks/mock_client.go new file mode 100644 index 000000000..63ca32fcf --- /dev/null +++ b/cla-backend-go/gitlab_api/mocks/mock_client.go @@ -0,0 +1,79 @@ +// Code generated by MockGen. DO NOT EDIT. +// Source: client.go + +// Package mock_gitlab is a generated GoMock package. +package mocks + +import ( + reflect "reflect" + + gomock "github.com/golang/mock/gomock" + gitlab "github.com/xanzy/go-gitlab" +) + +// MockGitLabClient is a mock of GitLabClient interface. +type MockGitLabClient struct { + ctrl *gomock.Controller + recorder *MockGitLabClientMockRecorder +} + +// MockGitLabClientMockRecorder is the mock recorder for MockGitLabClient. +type MockGitLabClientMockRecorder struct { + mock *MockGitLabClient +} + +// NewMockGitLabClient creates a new mock instance. +func NewMockGitLabClient(ctrl *gomock.Controller) *MockGitLabClient { + mock := &MockGitLabClient{ctrl: ctrl} + mock.recorder = &MockGitLabClientMockRecorder{mock} + return mock +} + +// EXPECT returns an object that allows the caller to indicate expected use. +func (m *MockGitLabClient) EXPECT() *MockGitLabClientMockRecorder { + return m.recorder +} + +// GetMergeRequestCommits mocks base method. +func (m *MockGitLabClient) GetMergeRequestCommits(projectID, mergeID int, opts *gitlab.GetMergeRequestCommitsOptions) ([]*gitlab.Commit, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "GetMergeRequestCommits", projectID, mergeID, opts) + ret0, _ := ret[0].([]*gitlab.Commit) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// GetMergeRequestCommits indicates an expected call of GetMergeRequestCommits. +func (mr *MockGitLabClientMockRecorder) GetMergeRequestCommits(projectID, mergeID, opts interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "GetMergeRequestCommits", reflect.TypeOf((*MockGitLabClient)(nil).GetMergeRequestCommits), projectID, mergeID, opts) +} + +// ListUsers mocks base method. +func (m *MockGitLabClient) ListUsers(opts *gitlab.ListUsersOptions) ([]*gitlab.User, error) { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "ListUsers", opts) + ret0, _ := ret[0].([]*gitlab.User) + ret1, _ := ret[1].(error) + return ret0, ret1 +} + +// ListUsers indicates an expected call of ListUsers. +func (mr *MockGitLabClientMockRecorder) ListUsers(opts interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "ListUsers", reflect.TypeOf((*MockGitLabClient)(nil).ListUsers), opts) +} + +// SetCommitStatus mocks base method. +func (m *MockGitLabClient) SetCommitStatus(projectID int, commitSHA string, opts *gitlab.SetCommitStatusOptions) error { + m.ctrl.T.Helper() + ret := m.ctrl.Call(m, "SetCommitStatus", projectID, commitSHA, opts) + ret0, _ := ret[0].(error) + return ret0 +} + +// SetCommitStatus indicates an expected call of SetCommitStatus. +func (mr *MockGitLabClientMockRecorder) SetCommitStatus(projectID, commitSHA, opts interface{}) *gomock.Call { + mr.mock.ctrl.T.Helper() + return mr.mock.ctrl.RecordCallWithMethodType(mr.mock, "SetCommitStatus", reflect.TypeOf((*MockGitLabClient)(nil).SetCommitStatus), projectID, commitSHA, opts) +} diff --git a/cla-backend-go/gitlab_api/mr.go b/cla-backend-go/gitlab_api/mr.go index a0db1d259..523df6d01 100644 --- a/cla-backend-go/gitlab_api/mr.go +++ b/cla-backend-go/gitlab_api/mr.go @@ -14,6 +14,16 @@ import ( "github.com/xanzy/go-gitlab" ) +type UserCommitSummary struct { + AuthorID int + AuthorUsername string + CommitSha string + AuthorName string + AuthorEmail string + Authorized bool + Affiliated bool +} + // FetchMrInfo is responsible for fetching the MR info for given project func FetchMrInfo(client *gitlab.Client, projectID int, mergeID int) (*gitlab.MergeRequest, error) { m, _, err := client.MergeRequests.GetMergeRequest(projectID, mergeID, &gitlab.GetMergeRequestsOptions{}) @@ -24,7 +34,7 @@ func FetchMrInfo(client *gitlab.Client, projectID int, mergeID int) (*gitlab.Mer return m, nil } -func GetLatestCommit(client *gitlab.Client, projectID int, mergeID int) (*gitlab.Commit, error) { +func GetLatestCommit(client GitLabClient, projectID int, mergeID int) (*gitlab.Commit, error) { f := logrus.Fields{ "functionName": "gitlab_api.GetLatestCommit", "projectID": projectID, @@ -32,7 +42,7 @@ func GetLatestCommit(client *gitlab.Client, projectID int, mergeID int) (*gitlab } log.WithFields(f).Debug("fetching latest commit...") - commits, _, err := client.MergeRequests.GetMergeRequestCommits(projectID, mergeID, &gitlab.GetMergeRequestCommitsOptions{}) + commits, err := client.GetMergeRequestCommits(projectID, mergeID, &gitlab.GetMergeRequestCommitsOptions{}) if err != nil { return nil, fmt.Errorf("fetching merge request commits : %d for project : %v failed : %v", mergeID, projectID, err) } @@ -45,28 +55,26 @@ func GetLatestCommit(client *gitlab.Client, projectID int, mergeID int) (*gitlab } // FetchMrParticipants is responsible to get unique mr participants -func FetchMrParticipants(client *gitlab.Client, projectID int, mergeID int) ([]*gitlab.User, error) { +func FetchMrParticipants(client GitLabClient, projectID int, mergeID int) ([]*UserCommitSummary, error) { f := logrus.Fields{ "functionName": "gitlab_api.FetchMrParticipants", "projectID": projectID, "mergeID": mergeID, } + + results := make([]*UserCommitSummary, 0) + log.WithFields(f).Debug("fetching mr participants...") - commits, response, err := client.MergeRequests.GetMergeRequestCommits(projectID, mergeID, &gitlab.GetMergeRequestCommitsOptions{}) + commits, err := client.GetMergeRequestCommits(projectID, mergeID, &gitlab.GetMergeRequestCommitsOptions{}) if err != nil { return nil, fmt.Errorf("fetching gitlab participants for project : %d and merge id : %d, failed : %v", projectID, mergeID, err) } - if response.StatusCode != 200 { - return nil, fmt.Errorf("fetching gitlab participants for project : %d and merge id : %d, failed with status code : %d", projectID, mergeID, response.StatusCode) - } if len(commits) == 0 { log.WithFields(f).Debugf("no commits found for project : %d and merge id : %d", projectID, mergeID) - return nil, nil + return results, nil } - var results []*gitlab.User - for _, commit := range commits { log.WithFields(f).Debugf("commit information: %v", commit) // The author is the person who originally wrote the code. The committer, on the other hand, is assumed to be @@ -82,6 +90,8 @@ func FetchMrParticipants(client *gitlab.Client, projectID int, mergeID int) ([]* return nil, getUserErr } + user.CommitSha = commit.ShortID + results = append(results, user) } @@ -89,7 +99,7 @@ func FetchMrParticipants(client *gitlab.Client, projectID int, mergeID int) ([]* } // SetCommitStatus is responsible for setting the MR status for commit sha -func SetCommitStatus(client *gitlab.Client, projectID int, commitSha string, state gitlab.BuildStateValue, message string, targetURL string) error { +func SetCommitStatus(client GitLabClient, projectID int, commitSha string, state gitlab.BuildStateValue, message string, targetURL string) error { f := logrus.Fields{ "functionName": "gitlab_api.SetCommitStatus", "projectID": projectID, @@ -110,7 +120,7 @@ func SetCommitStatus(client *gitlab.Client, projectID int, commitSha string, sta options.TargetURL = gitlab.String(targetURL) } - _, _, err := client.Commits.SetCommitStatus(projectID, commitSha, options) + err := client.SetCommitStatus(projectID, commitSha, options) if err != nil { return fmt.Errorf("setting commit status for the sha : %s and project id : %d failed : %v", commitSha, projectID, err) } @@ -161,23 +171,24 @@ func SetMrComment(client *gitlab.Client, projectID int, mergeID int, message str } // getUser is responsible for fetching the user info for given user email -func getUser(client *gitlab.Client, email, name *string) (*gitlab.User, error) { +func getUser(client GitLabClient, email, name *string) (*UserCommitSummary, error) { f := logrus.Fields{ "functionName": "gitlab_api.getUser", "email": *email, "name": *name, } - user := &gitlab.User{ - Email: *email, - Name: *name, + user := &UserCommitSummary{ + AuthorEmail: *email, + AuthorName: *name, } - users, _, err := client.Users.ListUsers(&gitlab.ListUsersOptions{ + users, err := client.ListUsers(&gitlab.ListUsersOptions{ Active: utils.Bool(true), Blocked: utils.Bool(false), Search: email, }) + if err != nil { log.WithFields(f).Warnf("unable to find user for email : %s, error : %v", utils.StringValue(email), err) return nil, err @@ -193,8 +204,8 @@ func getUser(client *gitlab.Client, email, name *string) (*gitlab.User, error) { for _, found := range users { if strings.EqualFold(found.Name, *name) { log.WithFields(f).Debugf("found matching user : %+v - updating GitLab username and ID", found) - user.Username = found.Username - user.ID = found.ID + user.AuthorID = found.ID + user.AuthorUsername = found.Username break } } diff --git a/cla-backend-go/gitlab_api/mr_test.go b/cla-backend-go/gitlab_api/mr_test.go new file mode 100644 index 000000000..2ea9dbde3 --- /dev/null +++ b/cla-backend-go/gitlab_api/mr_test.go @@ -0,0 +1,124 @@ +package gitlab + +import ( + "fmt" + "testing" + + gitlab_api "github.com/communitybridge/easycla/cla-backend-go/gitlab_api" + "github.com/communitybridge/easycla/cla-backend-go/gitlab_api/mocks" + "github.com/communitybridge/easycla/cla-backend-go/utils" + "github.com/golang/mock/gomock" + "github.com/stretchr/testify/assert" + goGitLab "github.com/xanzy/go-gitlab" +) + +func TestFetchMrParticipants(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + mockGitLabClient := mocks.NewMockGitLabClient(ctrl) + + projectID := 123 + mergeID := 456 + + commits := []*goGitLab.Commit{ + { + ID: "commit1", + AuthorEmail: "author1@example.com", + AuthorName: "Author 1", + ShortID: "shortID1", + }, + { + ID: "commit2", + AuthorEmail: "author2@example.com", + AuthorName: "Author 2", + ShortID: "shortID2", + }, + } + + users := []*goGitLab.User{ + { + ID: 1, + Username: "author_username1", + Email: "author1@example.com", + Name: "Author 1", + }, + { + ID: 2, + Username: "author_username2", + Email: "author2@example.com", + Name: "Author 2", + }, + } + + testCases := []struct { + name string + mockSetup func() + expectedResults []*gitlab_api.UserCommitSummary + expectedError error + }{ + { + name: "Successful fetch with participants", + mockSetup: func() { + mockGitLabClient.EXPECT().GetMergeRequestCommits(projectID, mergeID, &goGitLab.GetMergeRequestCommitsOptions{}).Return(commits, nil) + for index, commit := range commits { + mockGitLabClient.EXPECT().ListUsers(&goGitLab.ListUsersOptions{Active: utils.Bool(true), Blocked: utils.Bool(false), Search: &commit.AuthorEmail}).Return([]*goGitLab.User{users[index]}, nil) + } + }, + expectedResults: []*gitlab_api.UserCommitSummary{ + { + CommitSha: "shortID1", + AuthorName: "Author 1", + AuthorEmail: "author1@example.com", + Authorized: false, + Affiliated: false, + AuthorID: 1, + AuthorUsername: "author_username1", + }, + { + CommitSha: "shortID2", + AuthorName: "Author 2", + AuthorEmail: "author2@example.com", + Authorized: false, + Affiliated: false, + AuthorID: 2, + AuthorUsername: "author_username2", + }, + }, + expectedError: nil, + }, + { + name: "No commits found", + mockSetup: func() { + mockGitLabClient.EXPECT().GetMergeRequestCommits(projectID, mergeID, &goGitLab.GetMergeRequestCommitsOptions{}).Return([]*goGitLab.Commit{}, nil) + }, + expectedResults: []*gitlab_api.UserCommitSummary{}, + expectedError: nil, + }, + { + name: "Error fetching commits", + mockSetup: func() { + mergeRequestError := fmt.Errorf("error fetching merge request commits") + mockGitLabClient.EXPECT().GetMergeRequestCommits(projectID, mergeID, &goGitLab.GetMergeRequestCommitsOptions{}).Return(nil, mergeRequestError) + }, + expectedResults: nil, + expectedError: fmt.Errorf("fetching gitlab participants for project : %d and merge id : %d, failed : %v", projectID, mergeID, fmt.Errorf("error fetching merge request commits")), + }, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + tc.mockSetup() + + results, err := gitlab_api.FetchMrParticipants(mockGitLabClient, projectID, mergeID) + + if tc.expectedError != nil { + assert.EqualError(t, err, tc.expectedError.Error()) + } else { + assert.NoError(t, err) + } + assert.Equal(t, tc.expectedResults, results) + }) + } +} + diff --git a/cla-backend-go/v2/gitlab-activity/service_test.go b/cla-backend-go/v2/gitlab-activity/service_test.go index 761840bf1..a64ff8c3f 100644 --- a/cla-backend-go/v2/gitlab-activity/service_test.go +++ b/cla-backend-go/v2/gitlab-activity/service_test.go @@ -13,6 +13,7 @@ import ( "github.com/sirupsen/logrus" "github.com/stretchr/testify/assert" "github.com/xanzy/go-gitlab" + ) const enabled = false //nolint @@ -201,3 +202,111 @@ func TestPrepareMrCommentContent(t *testing.T) { } } + +func TestService_ProcessMergeActivity(t *testing.T) { + type testCase struct { + name string + secretToken string + input *ProcessMergeActivityInput + expectedError error + expectedCommitSha string + expectedMissing []*gitlab_api.UserCommitSummary + expectedSigned []*gitlab_api.UserCommitSummary + expectedCommitMsg string + expectedCommitURL string + expectedCommitStat gitlab.CommitStatusValue + } + + cases := []testCase{ + { + name: "Valid Merge Activity", + secretToken: "secret", + input: &ProcessMergeActivityInput{ + ProjectName: "project", + ProjectPath: "path", + ProjectNamespace: "namespace", + ProjectID: "projectID", + MergeID: "mergeID", + RepositoryPath: "repositoryPath", + LastCommitSha: "lastCommitSha", + }, + expectedError: nil, + expectedCommitSha: "lastCommitSha", + expectedMissing: []*gitlab_api.UserCommitSummary{}, + expectedSigned: []*gitlab_api.UserCommitSummary{}, + expectedCommitMsg: "EasyCLA check passed. You are authorized to contribute.", + expectedCommitURL: "", + expectedCommitStat: gitlab.Success, + }, + { + name: "Invalid Merge Activity", + secretToken: "secret", + input: &ProcessMergeActivityInput{ + ProjectName: "project", + ProjectPath: "path", + ProjectNamespace: "namespace", + ProjectID: "projectID", + MergeID: "mergeID", + RepositoryPath: "repositoryPath", + LastCommitSha: "lastCommitSha", + }, + expectedError: fmt.Errorf("fetching internal gitlab org for following path : repositoryPath failed : error"), + expectedCommitSha: "", + expectedMissing: nil, + expectedSigned: nil, + expectedCommitMsg: "", + expectedCommitURL: "", + expectedCommitStat: "", + }, + } + + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + ctrl := gomock.NewController(t) + defer ctrl.Finish() + + service := &service{ + gitlabOrgService: mock_gitlab.NewMockOrgService(ctrl), + gitLabApp: mock_gitlab.NewMockApp(ctrl), + } + + ctx := context.Background() + + // Mock getGitlabOrganizationFromProjectPath + service.gitlabOrgService.EXPECT().GetGitlabOrganizationFromProjectPath(ctx, tc.input.ProjectPath, tc.input.ProjectNamespace).Return(&gitlab_api.GitlabOrganization{}, tc.expectedError) + + // Mock RefreshGitLabOrganizationAuth + service.gitlabOrgService.EXPECT().RefreshGitLabOrganizationAuth(ctx, gomock.Any()).Return(&gitlab_api.OauthResponse{}, tc.expectedError) + + // Mock NewGitlabOauthClient + service.gitLabApp.EXPECT().NewGitlabOauthClient(gomock.Any(), gomock.Any()).Return(nil, tc.expectedError) + + // Mock GetLatestCommit + mockGitlabClient := mock_gitlab.NewMockClient(ctrl) + mockGitlabClient.EXPECT().GetLatestCommit(gomock.Any(), tc.input.ProjectID, tc.input.MergeID).Return(&gitlab_api.Commit{}, tc.expectedError) + service.gitLabApp.EXPECT().GetGitlabClient().Return(mockGitlabClient, tc.expectedError) + + // Mock FetchMrInfo + mockGitlabClient.EXPECT().FetchMrInfo(gomock.Any(), tc.input.ProjectID, tc.input.MergeID).Return(nil, tc.expectedError) + + // Mock getGitlabRepoByName + service.getGitlabRepoByName = func(ctx context.Context, repositoryPath string) (*gitlab_api.GitlabRepository, error) { + return &gitlab_api.GitlabRepository{}, tc.expectedError + } + + // Mock hasUserSigned + service.hasUserSigned = func(ctx context.Context, claGroupID string, user *gitlab_api.UserCommitSummary) (bool, error) { + return false, tc.expectedError + } + + // Mock SetCommitStatus + mockGitlabClient.EXPECT().SetCommitStatus(gomock.Any(), tc.input.ProjectID, tc.expectedCommitSha, tc.expectedCommitStat, tc.expectedCommitMsg, tc.expectedCommitURL).Return(tc.expectedError) + + // Mock SetMrComment + mockGitlabClient.EXPECT().SetMrComment(gomock.Any(), tc.input.ProjectID, tc.input.MergeID, gomock.Any()).Return(tc.expectedError) + + err := service.ProcessMergeActivity(ctx, tc.secretToken, tc.input) + assert.Equal(t, tc.expectedError, err) + }) + } +} diff --git a/cla-backend-go/v2/signatures/service_test.go b/cla-backend-go/v2/signatures/service_test.go index 54c42ad23..5ac572e5f 100644 --- a/cla-backend-go/v2/signatures/service_test.go +++ b/cla-backend-go/v2/signatures/service_test.go @@ -227,4 +227,4 @@ func TestService_IsUserAuthorized(t *testing.T) { assert.Equal(t, tc.expectedCompanyAffiliation, result.CompanyAffiliation) }) } -} +} \ No newline at end of file