Improve notifications for WIP draft PR's (#14663)
* #14559 Reduce amount of email notifications for WIP draft PR's don't notify repo watchers of WIP draft PR's * #13190 Notification when WIP Pull Request is ready for review * Send email notification to repo watchers when WIP PR is created * Send ui notification to repo watchers when WIP PR is created * send specific email notification when PR is marked ready for review instead of reusing the CreatePullRequest action * Fix lint error Co-authored-by: techknowlogick <techknowlogick@gitea.io>
This commit is contained in:
		
							parent
							
								
									66f8da538a
								
							
						
					
					
						commit
						17030ced75
					
				
					 8 changed files with 97 additions and 43 deletions
				
			
		|  | @ -51,6 +51,7 @@ const ( | ||||||
| 	ActionCommentPull                                     // 23
 | 	ActionCommentPull                                     // 23
 | ||||||
| 	ActionPublishRelease                                  // 24
 | 	ActionPublishRelease                                  // 24
 | ||||||
| 	ActionPullReviewDismissed                             // 25
 | 	ActionPullReviewDismissed                             // 25
 | ||||||
|  | 	ActionPullRequestReadyForReview                       // 26
 | ||||||
| ) | ) | ||||||
| 
 | 
 | ||||||
| // Action represents user operation type and other information to
 | // Action represents user operation type and other information to
 | ||||||
|  |  | ||||||
|  | @ -207,7 +207,7 @@ func createOrUpdateIssueNotifications(e Engine, issueID, commentID, notification | ||||||
| 		for _, id := range issueWatches { | 		for _, id := range issueWatches { | ||||||
| 			toNotify[id] = struct{}{} | 			toNotify[id] = struct{}{} | ||||||
| 		} | 		} | ||||||
| 
 | 		if !(issue.IsPull && HasWorkInProgressPrefix(issue.Title)) { | ||||||
| 			repoWatches, err := getRepoWatchersIDs(e, issue.RepoID) | 			repoWatches, err := getRepoWatchersIDs(e, issue.RepoID) | ||||||
| 			if err != nil { | 			if err != nil { | ||||||
| 				return err | 				return err | ||||||
|  | @ -215,6 +215,7 @@ func createOrUpdateIssueNotifications(e Engine, issueID, commentID, notification | ||||||
| 			for _, id := range repoWatches { | 			for _, id := range repoWatches { | ||||||
| 				toNotify[id] = struct{}{} | 				toNotify[id] = struct{}{} | ||||||
| 			} | 			} | ||||||
|  | 		} | ||||||
| 		issueParticipants, err := issue.getParticipantIDsByIssue(e) | 		issueParticipants, err := issue.getParticipantIDsByIssue(e) | ||||||
| 		if err != nil { | 		if err != nil { | ||||||
| 			return err | 			return err | ||||||
|  |  | ||||||
|  | @ -595,9 +595,13 @@ func (pr *PullRequest) IsWorkInProgress() bool { | ||||||
| 		log.Error("LoadIssue: %v", err) | 		log.Error("LoadIssue: %v", err) | ||||||
| 		return false | 		return false | ||||||
| 	} | 	} | ||||||
|  | 	return HasWorkInProgressPrefix(pr.Issue.Title) | ||||||
|  | } | ||||||
| 
 | 
 | ||||||
|  | // HasWorkInProgressPrefix determines if the given PR title has a Work In Progress prefix
 | ||||||
|  | func HasWorkInProgressPrefix(title string) bool { | ||||||
| 	for _, prefix := range setting.Repository.PullRequest.WorkInProgressPrefixes { | 	for _, prefix := range setting.Repository.PullRequest.WorkInProgressPrefixes { | ||||||
| 		if strings.HasPrefix(strings.ToUpper(pr.Issue.Title), prefix) { | 		if strings.HasPrefix(strings.ToUpper(title), prefix) { | ||||||
| 			return true | 			return true | ||||||
| 		} | 		} | ||||||
| 	} | 	} | ||||||
|  |  | ||||||
|  | @ -73,6 +73,18 @@ func (m *mailNotifier) NotifyIssueChangeStatus(doer *models.User, issue *models. | ||||||
| 	} | 	} | ||||||
| } | } | ||||||
| 
 | 
 | ||||||
|  | func (m *mailNotifier) NotifyIssueChangeTitle(doer *models.User, issue *models.Issue, oldTitle string) { | ||||||
|  | 	if err := issue.LoadPullRequest(); err != nil { | ||||||
|  | 		log.Error("issue.LoadPullRequest: %v", err) | ||||||
|  | 		return | ||||||
|  | 	} | ||||||
|  | 	if issue.IsPull && models.HasWorkInProgressPrefix(oldTitle) && !issue.PullRequest.IsWorkInProgress() { | ||||||
|  | 		if err := mailer.MailParticipants(issue, doer, models.ActionPullRequestReadyForReview, nil); err != nil { | ||||||
|  | 			log.Error("MailParticipants: %v", err) | ||||||
|  | 		} | ||||||
|  | 	} | ||||||
|  | } | ||||||
|  | 
 | ||||||
| func (m *mailNotifier) NotifyNewPullRequest(pr *models.PullRequest, mentions []*models.User) { | func (m *mailNotifier) NotifyNewPullRequest(pr *models.PullRequest, mentions []*models.User) { | ||||||
| 	if err := mailer.MailParticipants(pr.Issue, pr.Issue.Poster, models.ActionCreatePullRequest, mentions); err != nil { | 	if err := mailer.MailParticipants(pr.Issue, pr.Issue.Poster, models.ActionCreatePullRequest, mentions); err != nil { | ||||||
| 		log.Error("MailParticipants: %v", err) | 		log.Error("MailParticipants: %v", err) | ||||||
|  |  | ||||||
|  | @ -94,6 +94,19 @@ func (ns *notificationService) NotifyIssueChangeStatus(doer *models.User, issue | ||||||
| 	}) | 	}) | ||||||
| } | } | ||||||
| 
 | 
 | ||||||
|  | func (ns *notificationService) NotifyIssueChangeTitle(doer *models.User, issue *models.Issue, oldTitle string) { | ||||||
|  | 	if err := issue.LoadPullRequest(); err != nil { | ||||||
|  | 		log.Error("issue.LoadPullRequest: %v", err) | ||||||
|  | 		return | ||||||
|  | 	} | ||||||
|  | 	if issue.IsPull && models.HasWorkInProgressPrefix(oldTitle) && !issue.PullRequest.IsWorkInProgress() { | ||||||
|  | 		_ = ns.issueQueue.Push(issueNotificationOpts{ | ||||||
|  | 			IssueID:              issue.ID, | ||||||
|  | 			NotificationAuthorID: doer.ID, | ||||||
|  | 		}) | ||||||
|  | 	} | ||||||
|  | } | ||||||
|  | 
 | ||||||
| func (ns *notificationService) NotifyMergePullRequest(pr *models.PullRequest, doer *models.User) { | func (ns *notificationService) NotifyMergePullRequest(pr *models.PullRequest, doer *models.User) { | ||||||
| 	_ = ns.issueQueue.Push(issueNotificationOpts{ | 	_ = ns.issueQueue.Push(issueNotificationOpts{ | ||||||
| 		IssueID:              pr.Issue.ID, | 		IssueID:              pr.Issue.ID, | ||||||
|  | @ -106,15 +119,32 @@ func (ns *notificationService) NotifyNewPullRequest(pr *models.PullRequest, ment | ||||||
| 		log.Error("Unable to load issue: %d for pr: %d: Error: %v", pr.IssueID, pr.ID, err) | 		log.Error("Unable to load issue: %d for pr: %d: Error: %v", pr.IssueID, pr.ID, err) | ||||||
| 		return | 		return | ||||||
| 	} | 	} | ||||||
| 	_ = ns.issueQueue.Push(issueNotificationOpts{ | 	toNotify := make(map[int64]struct{}, 32) | ||||||
| 		IssueID:              pr.Issue.ID, | 	repoWatchers, err := models.GetRepoWatchersIDs(pr.Issue.RepoID) | ||||||
| 		NotificationAuthorID: pr.Issue.PosterID, | 	if err != nil { | ||||||
| 	}) | 		log.Error("GetRepoWatchersIDs: %v", err) | ||||||
|  | 		return | ||||||
|  | 	} | ||||||
|  | 	for _, id := range repoWatchers { | ||||||
|  | 		toNotify[id] = struct{}{} | ||||||
|  | 	} | ||||||
|  | 	issueParticipants, err := models.GetParticipantsIDsByIssueID(pr.IssueID) | ||||||
|  | 	if err != nil { | ||||||
|  | 		log.Error("GetParticipantsIDsByIssueID: %v", err) | ||||||
|  | 		return | ||||||
|  | 	} | ||||||
|  | 	for _, id := range issueParticipants { | ||||||
|  | 		toNotify[id] = struct{}{} | ||||||
|  | 	} | ||||||
|  | 	delete(toNotify, pr.Issue.PosterID) | ||||||
| 	for _, mention := range mentions { | 	for _, mention := range mentions { | ||||||
|  | 		toNotify[mention.ID] = struct{}{} | ||||||
|  | 	} | ||||||
|  | 	for receiverID := range toNotify { | ||||||
| 		_ = ns.issueQueue.Push(issueNotificationOpts{ | 		_ = ns.issueQueue.Push(issueNotificationOpts{ | ||||||
| 			IssueID:              pr.Issue.ID, | 			IssueID:              pr.Issue.ID, | ||||||
| 			NotificationAuthorID: pr.Issue.PosterID, | 			NotificationAuthorID: pr.Issue.PosterID, | ||||||
| 			ReceiverID:           mention.ID, | 			ReceiverID:           receiverID, | ||||||
| 		}) | 		}) | ||||||
| 	} | 	} | ||||||
| } | } | ||||||
|  |  | ||||||
|  | @ -377,6 +377,8 @@ func actionToTemplate(issue *models.Issue, actionType models.ActionType, | ||||||
| 		name = "merge" | 		name = "merge" | ||||||
| 	case models.ActionPullReviewDismissed: | 	case models.ActionPullReviewDismissed: | ||||||
| 		name = "review_dismissed" | 		name = "review_dismissed" | ||||||
|  | 	case models.ActionPullRequestReadyForReview: | ||||||
|  | 		name = "ready_for_review" | ||||||
| 	default: | 	default: | ||||||
| 		switch commentType { | 		switch commentType { | ||||||
| 		case models.CommentTypeReview: | 		case models.CommentTypeReview: | ||||||
|  |  | ||||||
|  | @ -30,7 +30,7 @@ const ( | ||||||
| 
 | 
 | ||||||
| // mailIssueCommentToParticipants can be used for both new issue creation and comment.
 | // mailIssueCommentToParticipants can be used for both new issue creation and comment.
 | ||||||
| // This function sends two list of emails:
 | // This function sends two list of emails:
 | ||||||
| // 1. Repository watchers and users who are participated in comments.
 | // 1. Repository watchers (except for WIP pull requests) and users who are participated in comments.
 | ||||||
| // 2. Users who are not in 1. but get mentioned in current issue/comment.
 | // 2. Users who are not in 1. but get mentioned in current issue/comment.
 | ||||||
| func mailIssueCommentToParticipants(ctx *mailCommentContext, mentions []*models.User) error { | func mailIssueCommentToParticipants(ctx *mailCommentContext, mentions []*models.User) error { | ||||||
| 
 | 
 | ||||||
|  | @ -74,11 +74,13 @@ func mailIssueCommentToParticipants(ctx *mailCommentContext, mentions []*models. | ||||||
| 
 | 
 | ||||||
| 	// =========== Repo watchers ===========
 | 	// =========== Repo watchers ===========
 | ||||||
| 	// Make repo watchers last, since it's likely the list with the most users
 | 	// Make repo watchers last, since it's likely the list with the most users
 | ||||||
|  | 	if !(ctx.Issue.IsPull && ctx.Issue.PullRequest.IsWorkInProgress() && ctx.ActionType != models.ActionCreatePullRequest) { | ||||||
| 		ids, err = models.GetRepoWatchersIDs(ctx.Issue.RepoID) | 		ids, err = models.GetRepoWatchersIDs(ctx.Issue.RepoID) | ||||||
| 		if err != nil { | 		if err != nil { | ||||||
| 			return fmt.Errorf("GetRepoWatchersIDs(%d): %v", ctx.Issue.RepoID, err) | 			return fmt.Errorf("GetRepoWatchersIDs(%d): %v", ctx.Issue.RepoID, err) | ||||||
| 		} | 		} | ||||||
| 		unfiltered = append(ids, unfiltered...) | 		unfiltered = append(ids, unfiltered...) | ||||||
|  | 	} | ||||||
| 
 | 
 | ||||||
| 	visited := make(map[int64]bool, len(unfiltered)+len(mentions)+1) | 	visited := make(map[int64]bool, len(unfiltered)+len(mentions)+1) | ||||||
| 
 | 
 | ||||||
|  |  | ||||||
|  | @ -51,6 +51,8 @@ | ||||||
| 			<b>@{{.Doer.Name}}</b> commented on this pull request. | 			<b>@{{.Doer.Name}}</b> commented on this pull request. | ||||||
| 		{{else if eq .ActionName "review_dismissed"}} | 		{{else if eq .ActionName "review_dismissed"}} | ||||||
| 			<b>@{{.Doer.Name}}</b> dismissed last review from {{.Comment.Review.Reviewer.Name}} for this pull request. | 			<b>@{{.Doer.Name}}</b> dismissed last review from {{.Comment.Review.Reviewer.Name}} for this pull request. | ||||||
|  | 		{{else if eq .ActionName "ready_for_review"}} | ||||||
|  | 			<b>@{{.Doer.Name}}</b> marked this pull request ready for review. | ||||||
| 		{{end}} | 		{{end}} | ||||||
| 
 | 
 | ||||||
| 		{{- if eq .Body ""}} | 		{{- if eq .Body ""}} | ||||||
|  |  | ||||||
		Loading…
	
		Reference in a new issue