Merge pull request #4768 from linuxfoundation/unicron-co-authors-algorithm-updates

Updates to co-author search algorithm

Author: lukaszgryglicki

CO_AUTHORS.md

@@ -155,7 +155,7 @@ If it does, the backend will process the co-authors as follows, assume trailer v
 
 - Second we check if email is in format `[email protected]`. If it is we use username part as GitHub username/login and fetch the user from GitHub API. If the user is found, we use that user as co-author.
 
-- Third we lookup for email using GitHub API. If the user is found, we use that user as co-author.
+- Third we lookup for email using GitHub API. If the user is found, we use that user as co-author. If not we lookup for this user in EasyCLA database by email (first by primary/LF email and then by other emails). If found, we use that user as co-author.
 
 - Finally we use the name part for `name <email>`. If the name matches the GitHub username pattern (alphanumeric characters or hyphens, must be 3–39 characters long, cannot start or end with a hyphen, and cannot contain consecutive hyphens), then we lookup using the GitHub API assuming that this name is a GitHub username/login (this is the case for some bots). If the user is found, we use that user as co-author.
 

cla-backend-go/github/github_repository.go

@@ -15,6 +15,7 @@ import (
 	"time"
 
 	log "github.com/linuxfoundation/easycla/cla-backend-go/logging"
+	"github.com/linuxfoundation/easycla/cla-backend-go/users"
 	"github.com/linuxfoundation/easycla/cla-backend-go/utils"
 	"github.com/sirupsen/logrus"
 
@@ -30,6 +31,31 @@ var (
 	GithubUsernameRegex         = regexp.MustCompile(`^[A-Za-z0-9-]{3,39}$`)
 )
 
+// Note: we use | and ||| as placeholders for inline and fenced code, then swap to backticks at render time.
+const MissingCoAuthorsMessage = `
+
+One or more co-authors of this pull request were not found. You must specify co-authors in commit message trailer via:
+
+|||
+Co-authored-by: name <email>
+|||
+
+Supported |Co-authored-by:| formats include:
+
+1) |Anything <[email protected]>| - it will locate your GitHub user by |id| part.
+2) |Anything <[email protected]>| - it will locate your GitHub user by |login| part.
+3) |Anything <public-email>| - it will locate your GitHub user by |public-email| part. Note that this email must be made public on Github.
+4) |Anything <other-email>| - it will locate your GitHub user by |other-email| part but only if that email was used before for any other CLA as a main commit author.
+5) |login <any-valid-email>| - it will locate your GitHub user by |login| part, note that |login| part must be at least 3 characters long.
+
+Please update your commit message(s) by doing |git commit --amend| and then |git push [--force]| and then request re-running CLA check via commenting on this pull request:
+
+|||
+/easycla
+|||
+
+`
+
 const (
 	help         = "https://help.github.com/en/github/committing-changes-to-your-project/why-are-my-commits-linked-to-the-wrong-user"
 	unknown      = "Unknown"
@@ -327,13 +353,15 @@ func GetCoAuthorsFromCommit(
 		commitMessage := commit.GetCommit().GetMessage()
 		// log.WithFields(f).Debugf("commit message: %s", commitMessage)
 
-		re := regexp.MustCompile(`(?i)co-authored-by: (.*) <(.*)>`)
+		re := regexp.MustCompile(`(?i)co-authored-by:\s*(.+?)\s*<([^<>]+)>`)
 		matches := re.FindAllStringSubmatch(commitMessage, -1)
 		for _, match := range matches {
 			name := strings.TrimSpace(match[1])
-			email := strings.TrimSpace(match[2])
-			coAuthors = append(coAuthors, [2]string{name, email})
-			log.WithFields(f).Debugf("found co-author: name: %s, email: %s", name, email)
+			email := strings.ToLower(strings.TrimSpace(match[2]))
+			if name != "" && email != "" {
+				coAuthors = append(coAuthors, [2]string{name, email})
+				log.WithFields(f).Debugf("found co-author: name: %s, email: %s", name, email)
+			}
 		}
 	}
 	return coAuthors
@@ -343,21 +371,27 @@ func GetCoAuthorsFromCommit(
 func ExpandWithCoAuthors(
 	ctx context.Context,
 	client *github.Client,
+	usersService users.Service,
 	commit *github.RepositoryCommit,
 	pr int,
 	installationID int64,
 	commitAuthors *[]*UserCommitSummary,
-) {
+) bool {
 	f := logrus.Fields{
 		"functionName": "github.github_repository.ExpandWithCoAuthors",
 		"pr":           pr,
 	}
 	coAuthors := GetCoAuthorsFromCommit(ctx, commit)
 	log.WithFields(f).Debugf("co-authors found: %s", coAuthors)
+	missing := false
 	for _, coAuthor := range coAuthors {
-		summary := GetCoAuthorCommits(ctx, client, coAuthor, commit, pr, installationID)
+		summary, found := GetCoAuthorCommits(ctx, client, usersService, coAuthor, commit, pr, installationID)
 		*commitAuthors = append(*commitAuthors, summary)
+		if !missing && !found {
+			missing = true
+		}
 	}
+	return missing
 }
 
 // IsValidGitHubUsername checks if the provided username is a valid GitHub username.
@@ -374,14 +408,16 @@ func IsValidGitHubUsername(username string) bool {
 	return true
 }
 
+//nolint:gocyclo // complexity is acceptable for now
 func GetCoAuthorCommits(
 	ctx context.Context,
 	client *github.Client,
+	usersService users.Service,
 	coAuthor [2]string,
 	commit *github.RepositoryCommit,
 	pr int,
 	installationID int64,
-) *UserCommitSummary {
+) (*UserCommitSummary, bool) {
 	f := logrus.Fields{
 		"functionName":    "github.github_repository.GetCoAuthorCommits",
 		"pr":              pr,
@@ -398,9 +434,12 @@ func GetCoAuthorCommits(
 	)
 	name = strings.TrimSpace(coAuthor[0])
 	email = strings.TrimSpace(coAuthor[1])
+	lName := strings.ToLower(name)
 
-	if cachedUser, ok := GithubUserCache.Get([2]string{name, email}); ok {
+	cacheKey := [2]string{lName, email}
+	if cachedUser, ok := GithubUserCache.Get(cacheKey); ok {
 		log.WithFields(f).Debugf("GitHub user found in cache for name/email: %s/%s: %+v", name, email, cachedUser)
+		found := false
 		var summary *UserCommitSummary
 		if cachedUser != nil {
 			summary = &UserCommitSummary{
@@ -409,6 +448,7 @@ func GetCoAuthorCommits(
 				Affiliated:   false,
 				Authorized:   false,
 			}
+			found = cachedUser.ID != nil
 		} else {
 			summary = &UserCommitSummary{
 				SHA: utils.StringValue(commit.SHA),
@@ -423,7 +463,7 @@ func GetCoAuthorCommits(
 			}
 		}
 		log.WithFields(f).Debugf("PR: %d, %+v (from cache)", pr, summary)
-		return summary
+		return summary, found
 	}
 
 	log.WithFields(f).Debugf("Getting co-author details: %+v", coAuthor)
@@ -454,19 +494,63 @@ func GetCoAuthorCommits(
 		}
 	}
 
-	// 3. Try to find user by email
+	// 3. Try to find user by email via GitHub APIs
 	if user == nil {
 		user, err = SearchGithubUserByEmail(ctx, client, email)
 		if err != nil {
-			log.WithFields(f).Debugf("Co-author GitHub user not found via email %s: %v (error: %v)", email, coAuthor, err)
+			log.WithFields(f).Debugf("Co-author GitHub user not found via github email %s: %v (error: %v)", email, coAuthor, err)
 			user = nil
 		}
 	}
 
+	//	3b. Try to find user by email in our database
+	if user == nil {
+		var githubID string
+		dbUsers, err2 := usersService.GetUsersByLFEmail(email)
+		if err2 == nil {
+			for _, dbUser := range dbUsers {
+				if dbUser.GithubID != "" {
+					githubID = dbUser.GithubID
+					// log.WithFields(f).Debugf("FOUND githubID.1 = %s", githubID)
+					break
+				}
+			}
+		} else {
+			log.WithFields(f).Debugf("Co-author GitHub user not found via lf email %s: %v (error: %v)", email, coAuthor, err2)
+		}
+		if githubID == "" {
+			dbUsers, err2 := usersService.GetUsersByEmail(email)
+			if err2 == nil {
+				for _, dbUser := range dbUsers {
+					if dbUser.GithubID != "" {
+						githubID = dbUser.GithubID
+						// log.WithFields(f).Debugf("FOUND githubID.2 = %s", githubID)
+						break
+					}
+				}
+			} else {
+				log.WithFields(f).Debugf("Co-author GitHub user not found via emails %s: %v (error: %v)", email, coAuthor, err2)
+			}
+		}
+		if githubID != "" {
+			githubIDInt, err2 := strconv.ParseInt(githubID, 10, 64)
+			if err2 != nil {
+				log.WithFields(f).Debugf("Co-author GitHub user not found via lf email %s, wrong GitHub ID: %s: %v (error: %v)", email, githubID, coAuthor, err2)
+			} else {
+				user, err = GetGithubUserByID(ctx, client, githubIDInt)
+				if err != nil {
+					log.WithFields(f).Debugf("Error fetching user by ID %d: %v", githubIDInt, err)
+					user = nil
+				}
+				// log.WithFields(f).Debugf("FOUND user = (%s, %d, %s, %s)", *user.Login, *user.ID, *user.Name, *user.Email)
+			}
+		}
+	}
+
 	// 4. Last resort - try to find by name=login
-	if user == nil && IsValidGitHubUsername(name) {
+	if user == nil && IsValidGitHubUsername(lName) {
 		// Note that Co-authored-by: name <email> is not actually a GitHub login but rather a name - but we are trying hard to find a GitHub profile
-		user, err = GetGithubUserByLogin(ctx, client, name)
+		user, err = GetGithubUserByLogin(ctx, client, lName)
 		if err != nil {
 			log.WithFields(f).Debugf("Co-author GitHub user not found via name=login=%s: %v (error: %v)", name, coAuthor, err)
 			user = nil
@@ -476,12 +560,14 @@ func GetCoAuthorCommits(
 	log.WithFields(f).Debugf("Co-author: %v, user: %+v", coAuthor, user)
 
 	var summary *UserCommitSummary
+	found := false
 	if user != nil {
 		if user.Login != nil {
 			login = *user.Login
 		}
 		if user.ID != nil {
 			githubID = *user.ID
+			found = true
 		}
 		if user.Name == nil || (user.Name != nil && strings.TrimSpace(*user.Name) == "") {
 			user.Name = &name
@@ -512,11 +598,11 @@ func GetCoAuthorCommits(
 		log.WithFields(f).Debugf("Co-author GitHub user details not found: %v", coAuthor)
 	}
 
-	GithubUserCache.Set([2]string{name, email}, user)
-	return summary
+	GithubUserCache.Set(cacheKey, user)
+	return summary, found
 }
 
-func GetPullRequestCommitAuthors(ctx context.Context, installationID int64, pullRequestID int, owner, repo string, withCoAuthors bool) ([]*UserCommitSummary, *string, error) {
+func GetPullRequestCommitAuthors(ctx context.Context, usersService users.Service, installationID int64, pullRequestID int, owner, repo string, withCoAuthors bool) ([]*UserCommitSummary, *string, bool, error) {
 	f := logrus.Fields{
 		"functionName":  "github.github_repository.GetPullRequestCommitAuthors",
 		"pullRequestID": pullRequestID,
@@ -527,21 +613,22 @@ func GetPullRequestCommitAuthors(ctx context.Context, installationID int64, pull
 	client, err := NewGithubAppClient(installationID)
 	if err != nil {
 		log.WithFields(f).WithError(err).Warn("unable to create Github client")
-		return nil, nil, err
+		return nil, nil, false, err
 	}
 
 	commits, resp, comErr := client.PullRequests.ListCommits(ctx, owner, repo, pullRequestID, &github.ListOptions{})
 	if comErr != nil {
 		log.WithFields(f).WithError(comErr).Warnf("problem listing commits for repo: %s/%s pull request: %d", owner, repo, pullRequestID)
-		return nil, nil, comErr
+		return nil, nil, false, comErr
 	}
 	if resp.StatusCode != http.StatusOK {
 		msg := fmt.Sprintf("unexpected status code: %d - expected: %d", resp.StatusCode, http.StatusOK)
 		log.WithFields(f).Warn(msg)
-		return nil, nil, errors.New(msg)
+		return nil, nil, false, errors.New(msg)
 	}
 
 	log.WithFields(f).Debugf("found %d commits for pull request: %d", len(commits), pullRequestID)
+	anyMissing := false
 	for _, commit := range commits {
 		log.WithFields(f).Debugf("loaded commit: %+v", commit)
 		commitAuthor := ""
@@ -571,7 +658,10 @@ func GetPullRequestCommitAuthors(ctx context.Context, installationID int64, pull
 			Authorized:   false,
 		})
 		if withCoAuthors {
-			ExpandWithCoAuthors(ctx, client, commit, pullRequestID, installationID, &userCommitSummary)
+			missing := ExpandWithCoAuthors(ctx, client, usersService, commit, pullRequestID, installationID, &userCommitSummary)
+			if !anyMissing && missing {
+				anyMissing = true
+			}
 		}
 	}
 
@@ -584,10 +674,10 @@ func GetPullRequestCommitAuthors(ctx context.Context, installationID int64, pull
 	//	}
 	//	log.WithFields(f).Debugf("user commit summary: %+v", *summary)
 	//}
-	return userCommitSummary, latestCommitSHA, nil
+	return userCommitSummary, latestCommitSHA, anyMissing, nil
 }
 
-func UpdatePullRequest(ctx context.Context, installationID int64, pullRequestID int, owner, repo string, repoID *int64, latestSHA string, signed []*UserCommitSummary, missing []*UserCommitSummary, CLABaseAPIURL, CLALandingPage, CLALogoURL string) error {
+func UpdatePullRequest(ctx context.Context, installationID int64, pullRequestID int, owner, repo string, repoID *int64, latestSHA string, signed []*UserCommitSummary, missing []*UserCommitSummary, anyMissing bool, CLABaseAPIURL, CLALandingPage, CLALogoURL string) error {
 	f := logrus.Fields{
 		"functionName":   "github.github_repository.UpdatePullRequest",
 		"installationID": installationID,
@@ -618,7 +708,7 @@ func UpdatePullRequest(ctx context.Context, installationID int64, pullRequestID
 		return failedErr
 	}
 
-	body := assembleCLAComment(ctx, int(installationID), pullRequestID, repoID, signed, missing, CLABaseAPIURL, CLALogoURL, CLALandingPage)
+	body := assembleCLAComment(ctx, int(installationID), pullRequestID, repoID, signed, missing, anyMissing, CLABaseAPIURL, CLALogoURL, CLALandingPage)
 
 	if len(missing) == 0 {
 		// All contributors are passing
@@ -649,7 +739,7 @@ func UpdatePullRequest(ctx context.Context, installationID int64, pullRequestID
 			// If we have previously succeeded, then we also need to update the comment (pass => fail)
 			log.WithFields(f).Debugf("Found previously succeeeded checks - updating the CLA comment in the PR : %d", pullRequestID)
 			// Generate a new comment with all the failed CLA info
-			failedComment := assembleCLAComment(ctx, int(installationID), pullRequestID, repoID, signed, missing, CLABaseAPIURL, CLALogoURL, CLALandingPage)
+			failedComment := assembleCLAComment(ctx, int(installationID), pullRequestID, repoID, signed, missing, anyMissing, CLABaseAPIURL, CLALogoURL, CLALandingPage)
 			previousSucceededComment.Body = &failedComment
 			_, _, err = client.Issues.EditComment(ctx, owner, repo, *previousSucceededComment.ID, previousSucceededComment)
 			if err != nil {
@@ -766,7 +856,7 @@ func assembleCLAStatus(authorName string, signed bool) (string, string) {
 	return authorName, "Missing CLA Authorization."
 }
 
-func assembleCLAComment(ctx context.Context, installationID, pullRequestID int, repositoryID *int64, signed, missing []*UserCommitSummary, apiBaseURL, CLALogoURL, CLALandingPage string) string {
+func assembleCLAComment(ctx context.Context, installationID, pullRequestID int, repositoryID *int64, signed, missing []*UserCommitSummary, anyMissing bool, apiBaseURL, CLALogoURL, CLALandingPage string) string {
 	f := logrus.Fields{
 		"functionName":   "github.github_repository.assembleCLAComment",
 		utils.XREQUESTID: ctx.Value(utils.XREQUESTID),
@@ -786,13 +876,13 @@ func assembleCLAComment(ctx context.Context, installationID, pullRequestID int,
 
 	log.WithFields(f).Debug("Building CLAComment body ")
 	signURL := getFullSignURL(repositoryType, strconv.Itoa(installationID), strconv.Itoa(int(*repositoryID)), strconv.Itoa(pullRequestID), apiBaseURL)
-	commentBody := getCommentBody(repositoryType, signURL, signed, missing)
+	commentBody := getCommentBody(repositoryType, signURL, signed, missing, anyMissing)
 	allSigned := len(missing) == 0
 	badge := getCommentBadge(allSigned, signURL, missingID, false, CLALandingPage, CLALogoURL)
 	return fmt.Sprintf("%s<br >%s", badge, commentBody)
 }
 
-func getCommentBody(repositoryType, signURL string, signed, missing []*UserCommitSummary) string {
+func getCommentBody(repositoryType, signURL string, signed, missing []*UserCommitSummary, anyMissing bool) string {
 	f := logrus.Fields{
 		"functionName":   "github.github_repository:getCommentBody",
 		"repositoryType": repositoryType,
@@ -864,6 +954,10 @@ func getCommentBody(repositoryType, signURL string, signed, missing []*UserCommi
 		text = "<br>The committers listed above are authorized under a signed CLA."
 	}
 
+	if anyMissing {
+		committersComment.WriteString(strings.ReplaceAll(MissingCoAuthorsMessage, "|", "`"))
+		log.WithFields(f).Debug("some co-authors are missing for this PR, added the missing co-author message")
+	}
 	return fmt.Sprintf("%s%s", committersComment.String(), text)
 }
 

cla-backend-go/signatures/service.go

@@ -1023,7 +1023,7 @@ func (s service) updateChangeRequest(ctx context.Context, ghOrg *models.GithubOr
 	// Fetch committers
 	withCoAuthors := github.IsCoAuthorsEnabledForRepo(ghOrg.EnableCoAuthors, gitHubRepoName)
 	log.WithFields(f).Debugf("fetching commit authors for PR: %d using repository owner: %s, repo: %s", pullRequestID, gitHubOrgName, gitHubRepoName)
-	authors, latestSHA, authorsErr := github.GetPullRequestCommitAuthors(ctx, ghOrg.OrganizationInstallationID, int(pullRequestID), gitHubOrgName, gitHubRepoName, withCoAuthors)
+	authors, latestSHA, anyMissing, authorsErr := github.GetPullRequestCommitAuthors(ctx, s.usersService, ghOrg.OrganizationInstallationID, int(pullRequestID), gitHubOrgName, gitHubRepoName, withCoAuthors)
 	if authorsErr != nil {
 		log.WithFields(f).WithError(authorsErr).Warnf("unable to get commit authors for %s/%s for PR: %d", gitHubOrgName, gitHubRepoName, pullRequestID)
 		return authorsErr
@@ -1124,7 +1124,7 @@ func (s service) updateChangeRequest(ctx context.Context, ghOrg *models.GithubOr
 	log.WithFields(f).Debugf("commit authors status after allowlisting bots => signed: %+v, missing: %+v, allowlisted: %+v", signed, unsigned, allowlisted)
 
 	// update pull request
-	updateErr := github.UpdatePullRequest(ctx, ghOrg.OrganizationInstallationID, int(pullRequestID), gitHubOrgName, gitHubRepoName, githubRepository.ID, *latestSHA, signed, unsigned, s.claBaseAPIURL, s.claLandingPage, s.claLogoURL)
+	updateErr := github.UpdatePullRequest(ctx, ghOrg.OrganizationInstallationID, int(pullRequestID), gitHubOrgName, gitHubRepoName, githubRepository.ID, *latestSHA, signed, unsigned, anyMissing, s.claBaseAPIURL, s.claLandingPage, s.claLogoURL)
 	if updateErr != nil {
 		log.WithFields(f).Debugf("unable to update PR: %d", pullRequestID)
 		return updateErr