Skip to content

Commit 877c38b

Browse files
zeripathzeripath
committed
Simplifications
1 parent ef394de commit 877c38b

File tree

2 files changed

+75
-82
lines changed

2 files changed

+75
-82
lines changed

‎models/review.go

Lines changed: 58 additions & 68 deletions
Original file line numberDiff line numberDiff line change
@@ -219,12 +219,12 @@ type CreateReviewOptions struct {
219219
Stale bool
220220
}
221221

222-
// IsOfficialReviewer check if reviewer can make official reviews in issue (counts towards required approvals)
223-
func IsOfficialReviewer(issue *Issue, reviewer *User) (bool, error) {
224-
return isOfficialReviewer(x, issue, reviewer)
222+
// IsOfficialReviewer check if at least one of the provided reviewers can make official reviews in issue (counts towards required approvals)
223+
func IsOfficialReviewer(issue *Issue, reviewers ...*User) (bool, error) {
224+
return isOfficialReviewer(x, issue, reviewers...)
225225
}
226226

227-
func isOfficialReviewer(e Engine, issue *Issue, reviewer *User) (bool, error) {
227+
func isOfficialReviewer(e Engine, issue *Issue, reviewers ...*User) (bool, error) {
228228
pr, err := getPullRequestByIssueID(e, issue.ID)
229229
if err != nil {
230230
return false, err
@@ -236,7 +236,14 @@ func isOfficialReviewer(e Engine, issue *Issue, reviewer *User) (bool, error) {
236236
return false, nil
237237
}
238238

239-
return pr.ProtectedBranch.isUserOfficialReviewer(e, reviewer)
239+
for _, reviewer := range reviewers {
240+
official, err := pr.ProtectedBranch.isUserOfficialReviewer(e, reviewer)
241+
if official || err != nil {
242+
return official, err
243+
}
244+
}
245+
246+
return false, nil
240247
}
241248

242249
// IsOfficialReviewerTeam check if reviewer in this team can make official reviews in issue (counts towards required approvals)
@@ -426,22 +433,20 @@ func SubmitReview(doer *User, issue *Issue, reviewType ReviewType, content, comm
426433
// try to remove team review request if need
427434
if issue.Repo.Owner.IsOrganization() && (reviewType == ReviewTypeApprove || reviewType == ReviewTypeReject) {
428435
teamReviewRequests := make([]*Review, 0, 10)
429-
if err = sess.SQL("SELECT * FROM review WHERE reviewer_team_id > 0 AND type = ?", ReviewTypeRequest).Find(&teamReviewRequests); err != nil {
436+
if err := sess.SQL("SELECT * FROM review WHERE reviewer_team_id > 0 AND type = ?", ReviewTypeRequest).Find(&teamReviewRequests); err != nil {
430437
return nil, nil, err
431438
}
432439

433-
if len(teamReviewRequests) > 0 {
434-
for _, teamReviewRequest := range teamReviewRequests {
435-
ok := false
436-
if ok, err = isTeamMember(sess, issue.Repo.OwnerID, teamReviewRequest.ReviewerTeamID, doer.ID); err != nil {
437-
return nil, nil, err
438-
}
439-
440-
if ok {
441-
if _, err := sess.Delete(teamReviewRequest); err != nil {
442-
return nil, nil, err
443-
}
444-
}
440+
for _, teamReviewRequest := range teamReviewRequests {
441+
ok, err := isTeamMember(sess, issue.Repo.OwnerID, teamReviewRequest.ReviewerTeamID, doer.ID)
442+
if err != nil {
443+
return nil, nil, err
444+
} else if !ok {
445+
continue
446+
}
447+
448+
if _, err := sess.Delete(teamReviewRequest); err != nil {
449+
return nil, nil, err
445450
}
446451
}
447452
}
@@ -456,6 +461,9 @@ func GetReviewersByIssueID(issueID int64) ([]*Review, error) {
456461

457462
sess := x.NewSession()
458463
defer sess.Close()
464+
if err := sess.Begin(); err != nil {
465+
return nil, err
466+
}
459467

460468
// Get latest review of each reviwer, sorted in order they were made
461469
if err := sess.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_team_id = 0 AND type in (?, ?, ?) AND original_author_id = 0 GROUP BY issue_id, reviewer_id) ORDER BY review.updated_unix ASC",
@@ -479,20 +487,20 @@ func GetReviewersByIssueID(issueID int64) ([]*Review, error) {
479487
}
480488

481489
// GetReviewerByIssueIDAndUserID get the latest review of reviewer for a pull request
482-
func GetReviewerByIssueIDAndUserID(issueID, userID int64) (review *Review, err error) {
483-
return getReviewerByIssueIDAndUserID(x, issueID, userID)
490+
func GetReviewerByIssueIDAndUserID(issueID, userID int64) (*Review, error) {
491+
return getReviewByIssueIDAndUserID(x, issueID, userID)
484492
}
485493

486-
func getReviewerByIssueIDAndUserID(e Engine, issueID, userID int64) (review *Review, err error) {
487-
review = new(Review)
494+
func getReviewByIssueIDAndUserID(e Engine, issueID, userID int64) (*Review, error) {
495+
var review *Review
488496

489497
if _, err := e.SQL("SELECT * FROM review WHERE id IN (SELECT max(id) as id FROM review WHERE issue_id = ? AND reviewer_id = ? AND original_author_id = 0 AND type in (?, ?, ?))",
490498
issueID, userID, ReviewTypeApprove, ReviewTypeReject, ReviewTypeRequest).
491499
Get(review); err != nil {
492500
return nil, err
493501
}
494502

495-
return
503+
return review, nil
496504
}
497505

498506
// GetTeamReviewerByIssueIDAndTeamID get the latest review requst of reviewer team for a pull request
@@ -581,7 +589,7 @@ func AddReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
581589
return nil, err
582590
}
583591

584-
review, err := getReviewerByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
592+
review, err := getReviewByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
585593
if err != nil {
586594
return nil, err
587595
}
@@ -591,18 +599,10 @@ func AddReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
591599
return nil, nil
592600
}
593601

594-
var official bool
595-
if official, err = isOfficialReviewer(sess, issue, reviewer); err != nil {
602+
official, err := isOfficialReviewer(sess, issue, reviewer, doer)
603+
if err != nil {
596604
return nil, err
597-
}
598-
599-
if !official {
600-
if official, err = isOfficialReviewer(sess, issue, doer); err != nil {
601-
return nil, err
602-
}
603-
}
604-
605-
if official {
605+
} else if official {
606606
if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_id=?", false, issue.ID, reviewer.ID); err != nil {
607607
return nil, err
608608
}
@@ -618,15 +618,15 @@ func AddReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
618618
return nil, err
619619
}
620620

621-
var comment *Comment
622-
if comment, err = createComment(sess, &CreateCommentOptions{
621+
comment, err := createComment(sess, &CreateCommentOptions{
623622
Type: CommentTypeReviewRequest,
624623
Doer: doer,
625624
Repo: issue.Repo,
626625
Issue: issue,
627626
RemovedAssignee: false, // Use RemovedAssignee as !isRequest
628627
AssigneeID: reviewer.ID, // Use AssigneeID as reviewer ID
629-
}); err != nil {
628+
})
629+
if err != nil {
630630
return nil, err
631631
}
632632

@@ -641,7 +641,7 @@ func RemoveReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
641641
return nil, err
642642
}
643643

644-
review, err := getReviewerByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
644+
review, err := getReviewByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
645645
if err != nil {
646646
return nil, err
647647
}
@@ -654,16 +654,12 @@ func RemoveReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
654654
return nil, err
655655
}
656656

657-
var official bool
658-
if official, err = isOfficialReviewer(sess, issue, reviewer); err != nil {
657+
official, err := isOfficialReviewer(sess, issue, reviewer)
658+
if err != nil {
659659
return nil, err
660-
}
661-
662-
if official {
663-
// recalculate which is the latest official review from that user
664-
var review *Review
665-
666-
review, err = getReviewerByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
660+
} else if official {
661+
// recalculate the latest official review for reviewer
662+
review, err := getReviewByIssueIDAndUserID(sess, issue.ID, reviewer.ID)
667663
if err != nil {
668664
return nil, err
669665
}
@@ -675,16 +671,14 @@ func RemoveReviewRequest(issue *Issue, reviewer, doer *User) (*Comment, error) {
675671
}
676672
}
677673

678-
var comment *Comment
679-
comment, err = createComment(sess, &CreateCommentOptions{
674+
comment, err := createComment(sess, &CreateCommentOptions{
680675
Type: CommentTypeReviewRequest,
681676
Doer: doer,
682677
Repo: issue.Repo,
683678
Issue: issue,
684679
RemovedAssignee: true, // Use RemovedAssignee as !isRequest
685680
AssigneeID: reviewer.ID, // Use AssigneeID as reviewer ID
686681
})
687-
688682
if err != nil {
689683
return nil, err
690684
}
@@ -710,12 +704,10 @@ func AddTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (*Comment, e
710704
return nil, nil
711705
}
712706

713-
var official bool
714-
if official, err = isOfficialReviewerTeam(sess, issue, reviewer); err != nil {
707+
official, err := isOfficialReviewerTeam(sess, issue, reviewer)
708+
if err != nil {
715709
return nil, fmt.Errorf("isOfficialReviewerTeam(): %v", err)
716-
}
717-
718-
if !official {
710+
} else if !official {
719711
if official, err = isOfficialReviewer(sess, issue, doer); err != nil {
720712
return nil, fmt.Errorf("isOfficialReviewer(): %v", err)
721713
}
@@ -732,20 +724,20 @@ func AddTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (*Comment, e
732724
}
733725

734726
if official {
735-
if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_team_id = ?", false, issue.ID, reviewer.ID); err != nil {
727+
if _, err := sess.Exec("UPDATE `review` SET official=? WHERE issue_id=? AND reviewer_team_id=?", false, issue.ID, reviewer.ID); err != nil {
736728
return nil, err
737729
}
738730
}
739731

740-
var comment *Comment
741-
if comment, err = createComment(sess, &CreateCommentOptions{
732+
comment, err := createComment(sess, &CreateCommentOptions{
742733
Type: CommentTypeReviewRequest,
743734
Doer: doer,
744735
Repo: issue.Repo,
745736
Issue: issue,
746737
RemovedAssignee: false, // Use RemovedAssignee as !isRequest
747738
AssigneeTeamID: reviewer.ID, // Use AssigneeTeamID as reviewer team ID
748-
}); err != nil {
739+
})
740+
if err != nil {
749741
return nil, fmt.Errorf("createComment(): %v", err)
750742
}
751743

@@ -773,16 +765,14 @@ func RemoveTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (*Comment
773765
return nil, err
774766
}
775767

776-
var official bool
777-
if official, err = isOfficialReviewerTeam(sess, issue, reviewer); err != nil {
768+
official, err := isOfficialReviewerTeam(sess, issue, reviewer)
769+
if err != nil {
778770
return nil, fmt.Errorf("isOfficialReviewerTeam(): %v", err)
779771
}
780772

781773
if official {
782774
// recalculate which is the latest official review from that team
783-
var review *Review
784-
785-
review, err = getReviewerByIssueIDAndUserID(sess, issue.ID, -reviewer.ID)
775+
review, err := getReviewByIssueIDAndUserID(sess, issue.ID, -reviewer.ID)
786776
if err != nil {
787777
return nil, err
788778
}
@@ -798,15 +788,15 @@ func RemoveTeamReviewRequest(issue *Issue, reviewer *Team, doer *User) (*Comment
798788
return nil, sess.Commit()
799789
}
800790

801-
var comment *Comment
802-
if comment, err = createComment(sess, &CreateCommentOptions{
791+
comment, err := createComment(sess, &CreateCommentOptions{
803792
Type: CommentTypeReviewRequest,
804793
Doer: doer,
805794
Repo: issue.Repo,
806795
Issue: issue,
807796
RemovedAssignee: true, // Use RemovedAssignee as !isRequest
808797
AssigneeTeamID: reviewer.ID, // Use AssigneeTeamID as reviewer team ID
809-
}); err != nil {
798+
})
799+
if err != nil {
810800
return nil, fmt.Errorf("createComment(): %v", err)
811801
}
812802

‎services/issue/assignee.go

Lines changed: 17 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -85,22 +85,25 @@ func TeamReviewRequest(issue *models.Issue, doer *models.User, reviewer *models.
8585
return
8686
}
8787

88-
if comment != nil && isAdd {
89-
// notify all user in this team
90-
if err = comment.LoadIssue(); err != nil {
91-
return
92-
}
93-
if err = reviewer.GetMembers(&models.SearchMembersOptions{}); err != nil {
94-
return
95-
}
88+
if comment == nil || !isAdd {
89+
return
90+
}
9691

97-
for _, member := range reviewer.Members {
98-
if member.ID == comment.Issue.PosterID {
99-
continue
100-
}
101-
comment.AssigneeID = member.ID
102-
notification.NotifyPullReviewRequest(doer, issue, member, isAdd, comment)
92+
// notify all user in this team
93+
if err = comment.LoadIssue(); err != nil {
94+
return
95+
}
96+
97+
if err = reviewer.GetMembers(&models.SearchMembersOptions{}); err != nil {
98+
return
99+
}
100+
101+
for _, member := range reviewer.Members {
102+
if member.ID == comment.Issue.PosterID {
103+
continue
103104
}
105+
comment.AssigneeID = member.ID
106+
notification.NotifyPullReviewRequest(doer, issue, member, isAdd, comment)
104107
}
105108

106109
return nil

0 commit comments

Comments
 (0)