test5

Timeline events will contain Reviews and ReviewRequests and
ReviewDismissed events. We need to handle this at event parsing
time and not to punt this to the query functions later on.

If the last event is an actual review, we use this.
If no review, check if last event associated with the reviewer
is Dismissed or Requested Review but not if a dismissed Review
preceeds it.

common/pr.go

@@ -468,7 +468,7 @@ func (rs *PRSet) IsApproved(gitea GiteaPRChecker, maintainers MaintainershipData
 				LogError("Cannot fetch gita reaviews for PR:", err)
 				return false
 			}
-			r.RequestedReviewers = reviewers
+			r.SetRequiredReviewers(reviewers)
 			prjgit.Reviews = r
 			if prjgit.Reviews.IsManualMergeOK() {
 				is_manually_reviewed_ok = true
@@ -489,7 +489,7 @@ func (rs *PRSet) IsApproved(gitea GiteaPRChecker, maintainers MaintainershipData
 					LogError("Cannot fetch gita reaviews for PR:", err)
 					return false
 				}
-				r.RequestedReviewers = reviewers
+				r.SetRequiredReviewers(reviewers)
 				pr.Reviews = r
 				if !pr.Reviews.IsManualMergeOK() {
 					LogInfo("Not approved manual merge. PR:", pr.PR.URL)
@@ -530,7 +530,7 @@ func (rs *PRSet) IsApproved(gitea GiteaPRChecker, maintainers MaintainershipData
 			LogError("Cannot fetch gitea reaviews for PR:", err)
 			return false
 		}
-		r.RequestedReviewers = reviewers
+		r.SetRequiredReviewers(reviewers)
 
 		is_manually_reviewed_ok = r.IsApproved()
 		LogDebug("PR to", pr.PR.Base.Repo.Name, "reviewed?", is_manually_reviewed_ok)

common/pr_test.go

@@ -807,9 +807,8 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
 							Base: &models.PRBranchInfo{Name: "main", Repo: &models.Repository{Name: "pkg", Owner: &models.User{UserName: "org"}}},
 						},
 						Reviews: &common.PRReviews{
-							Reviews:            []*models.PullReview{{State: common.ReviewStateRequestReview, User: &models.User{UserName: "m1"}}},
-							RequestedReviewers: []string{"m1"},
-							FullTimeline: []*models.TimelineComment{
+							Reviews: []*models.PullReview{{State: common.ReviewStateRequestReview, User: &models.User{UserName: "m1"}}},
+							RequestedReviewers: []*models.TimelineComment{
 								{User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "m1"}, Type: common.TimelineCommentType_ReviewRequested},
 							},
 						},
@@ -919,8 +918,7 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
 						},
 						Reviews: &common.PRReviews{
 							Reviews:            []*models.PullReview{{State: common.ReviewStateRequestReview, User: &models.User{UserName: "reviewer"}}},
-							RequestedReviewers: []string{"reviewer"},
-							FullTimeline:       []*models.TimelineComment{{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "reviewer"}}},
+							RequestedReviewers: []*models.TimelineComment{{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "reviewer"}}},
 						},
 					},
 					{
@@ -930,8 +928,7 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
 						},
 						Reviews: &common.PRReviews{
 							Reviews:            []*models.PullReview{{State: common.ReviewStateRequestReview, User: &models.User{UserName: "reviewer"}}},
-							RequestedReviewers: []string{"reviewer"},
-							FullTimeline:       []*models.TimelineComment{{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "reviewer"}}},
+							RequestedReviewers: []*models.TimelineComment{{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "reviewer"}}},
 						},
 					},
 				},
@@ -966,8 +963,7 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
 								{State: common.ReviewStateApproved, User: &models.User{UserName: "pkgmaintainer"}},
 								{State: common.ReviewStatePending, User: &models.User{UserName: "prjmaintainer"}},
 							},
-							RequestedReviewers: []string{"user2", "pkgmaintainer", "prjmaintainer"},
-							FullTimeline: []*models.TimelineComment{
+							RequestedReviewers: []*models.TimelineComment{
 								{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "user2"}},
 								{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "pkgmaintainer"}},
 								{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "prjmaintainer"}},
@@ -985,8 +981,7 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
 								{State: common.ReviewStateRequestChanges, User: &models.User{UserName: "user1"}},
 								{State: common.ReviewStateRequestReview, User: &models.User{UserName: "autogits_obs_staging_bot"}},
 							},
-							RequestedReviewers: []string{"user1", "autogits_obs_staging_bot"},
-							FullTimeline: []*models.TimelineComment{
+							RequestedReviewers: []*models.TimelineComment{
 								{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "user1"}},
 								{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "autogits_obs_staging_bot"}},
 							},
@@ -1026,8 +1021,7 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
 								{State: common.ReviewStatePending, User: &models.User{UserName: "prj2"}},
 								{State: common.ReviewStatePending, User: &models.User{UserName: "someother"}},
 							},
-							RequestedReviewers: []string{"user2", "pkgmaintainer", "prjmaintainer", "pkgm1", "pkgm2", "someother", "prj1", "prj2"},
-							FullTimeline: []*models.TimelineComment{
+							RequestedReviewers: []*models.TimelineComment{
 								{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "pkgmaintainer"}},
 								{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "prjmaintainer"}},
 								{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "prj1"}},
@@ -1050,8 +1044,7 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
 								{State: common.ReviewStatePending, User: &models.User{UserName: "prj1"}},
 								{State: common.ReviewStatePending, User: &models.User{UserName: "prj2"}},
 							},
-							RequestedReviewers: []string{"user1", "autogits_obs_staging_bot", "prj1", "prj2"},
-							FullTimeline: []*models.TimelineComment{
+							RequestedReviewers: []*models.TimelineComment{
 								{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "autogits_obs_staging_bot"}},
 								{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "prj1"}},
 								{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "prj2"}},
@@ -1090,8 +1083,7 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
 								{State: common.ReviewStateRequestReview, User: &models.User{UserName: "prj1"}},
 								{State: common.ReviewStateRequestReview, User: &models.User{UserName: "someother"}},
 							},
-							RequestedReviewers: []string{"user2", "pkgmaintainer", "prjmaintainer", "pkgm1", "someother", "prj1"},
-							FullTimeline: []*models.TimelineComment{
+							RequestedReviewers: []*models.TimelineComment{
 								{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "pkgm1"}},
 								{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "pkgmaintainer"}},
 								{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "prjmaintainer"}},
@@ -1112,8 +1104,7 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
 								{State: common.ReviewStateRequestReview, User: &models.User{UserName: "autogits_obs_staging_bot"}},
 								{State: common.ReviewStateRequestReview, User: &models.User{UserName: "prj1"}},
 							},
-							RequestedReviewers: []string{"user1", "autogits_obs_staging_bot", "prj1"},
-							FullTimeline: []*models.TimelineComment{
+							RequestedReviewers: []*models.TimelineComment{
 								{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "autogits_obs_staging_bot"}},
 								{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "bot"}, Assignee: &models.User{UserName: "prj1"}},
 								{Type: common.TimelineCommentType_ReviewRequested, User: &models.User{UserName: "!bot"}, Assignee: &models.User{UserName: "user1"}},
@@ -1199,6 +1190,9 @@ func TestFindMissingAndExtraReviewers(t *testing.T) {
 		t.Run(test.name, func(t *testing.T) {
 			test.prset.HasAutoStaging = !test.noAutoStaging
 			for idx, pr := range test.prset.PRs {
+				if pr.Reviews != nil {
+					pr.Reviews.SetRequiredReviewers(test.prset.Config.Reviewers)
+				}
 				missing, extra := test.prset.FindMissingAndExtraReviewers(test.maintainers, idx)
 
 				// avoid nil dereference below, by adding empty array elements

common/reviews.go

@@ -8,32 +8,47 @@ import (
 	"src.opensuse.org/autogits/common/gitea-generated/models"
 )
 
+type ReviewInterface interface {
+	IsManualMergeOK() bool
+	IsApproved() bool
+	MisingReviews() []string
+	FindReviewRequester(reviewer string) *models.TimelineComment
+	HasPendingReviewBy(reviewer string) bool
+	IsReviewedBy(reviewer string) bool
+	IsReviewedByOneOf(reviewers ...string) bool
+
+	SetRequiredReviewers(reviewers []string)
+}
+
 type PRReviews struct {
 	Reviews            []*models.PullReview
-	RequestedReviewers []string
+	RequestedReviewers []*models.TimelineComment
 	Comments           []*models.TimelineComment
 
-	FullTimeline []*models.TimelineComment
+	RequiredReviewers []string
+}
+
+func (r *PRReviews) SetRequiredReviewers(reviewers []string) {
+	r.RequiredReviewers = reviewers
 }
 
 func FetchGiteaReviews(rf GiteaReviewTimelineFetcher, org, repo string, no int64) (*PRReviews, error) {
-	rawReviews, err := rf.GetPullRequestReviews(org, repo, no)
-	if err != nil {
-		return nil, err
-	}
-
 	timeline, err := rf.GetTimeline(org, repo, no)
 	if err != nil {
 		return nil, err
 	}
 
+	rawReviews, err := rf.GetPullRequestReviews(org, repo, no)
+	if err != nil {
+		return nil, err
+	}
 
 	reviews := make([]*models.PullReview, 0, 10)
-	needNewReviews := []string{}
 	var comments []*models.TimelineComment
 
+	var foundUsers []string
 	alreadyHaveUserReview := func(user string) bool {
-		if slices.Contains(needNewReviews, user) {
+		if slices.Contains(foundUsers, user) {
 			return true
 		}
 		for _, r := range reviews {
@@ -49,20 +64,24 @@ func FetchGiteaReviews(rf GiteaReviewTimelineFetcher, org, repo string, no int64
 	LogDebug("Number of items in timeline:", len(timeline))
 
 	cutOffIdx := len(timeline)
+	var PendingRequestedReviews []*models.TimelineComment
 	for idx, item := range timeline {
-		if item.Type == TimelineCommentType_Review || item.Type == TimelineCommentType_ReviewRequested {
+		if item.Type == TimelineCommentType_Review {
 			for _, r := range rawReviews {
-				if r.ID == item.ReviewID {
+				if r.ID == item.ReviewID && r.User != nil {
 					if !alreadyHaveUserReview(r.User.UserName) {
-						if item.Type == TimelineCommentType_Review && idx > cutOffIdx {
-							needNewReviews = append(needNewReviews, r.User.UserName)
-						} else {
+						if idx < cutOffIdx {
 							reviews = append(reviews, r)
 						}
+						foundUsers = append(foundUsers, r.User.UserName)
 					}
 					break
 				}
 			}
+		} else if item.Type == TimelineCommentType_ReviewRequested && item.Assignee != nil && !alreadyHaveUserReview(item.Assignee.UserName) {
+			PendingRequestedReviews = append(PendingRequestedReviews, item)
+		} else if item.Type == TimelineCommentType_DismissReview && item.Assignee != nil && !alreadyHaveUserReview(item.Assignee.UserName) {
+			foundUsers = append(foundUsers, item.Assignee.UserName)
 		} else if item.Type == TimelineCommentType_Comment && cutOffIdx > idx {
 			comments = append(comments, item)
 		} else if item.Type == TimelineCommentType_PushPull && cutOffIdx == len(timeline) {
@@ -75,9 +94,9 @@ func FetchGiteaReviews(rf GiteaReviewTimelineFetcher, org, repo string, no int64
 	LogDebug("num comments:", len(comments), "timeline:", len(reviews))
 
 	return &PRReviews{
-		Reviews:      reviews,
-		Comments:     comments,
-		FullTimeline: timeline,
+		Reviews:            reviews,
+		Comments:           comments,
+		RequestedReviewers: PendingRequestedReviews,
 	}, nil
 }
 
@@ -105,7 +124,7 @@ func (r *PRReviews) IsManualMergeOK() bool {
 			continue
 		}
 		LogDebug("comment:", c.User.UserName, c.Body)
-		if slices.Contains(r.RequestedReviewers, c.User.UserName) {
+		if slices.Contains(r.RequiredReviewers, c.User.UserName) {
 			if bodyCommandManualMergeOK(c.Body) {
 				return true
 			}
@@ -116,7 +135,7 @@ func (r *PRReviews) IsManualMergeOK() bool {
 		if c.Updated != c.Submitted {
 			continue
 		}
-		if slices.Contains(r.RequestedReviewers, c.User.UserName) {
+		if slices.Contains(r.RequiredReviewers, c.User.UserName) {
 			if bodyCommandManualMergeOK(c.Body) {
 				return true
 			}
@@ -132,7 +151,7 @@ func (r *PRReviews) IsApproved() bool {
 	}
 	goodReview := true
 
-	for _, reviewer := range r.RequestedReviewers {
+	for _, reviewer := range r.RequiredReviewers {
 		goodReview = false
 		for _, review := range r.Reviews {
 			if review.User.UserName == reviewer && review.State == ReviewStateApproved && !review.Stale && !review.Dismissed {
@@ -156,7 +175,7 @@ func (r *PRReviews) MissingReviews() []string {
 		return missing
 	}
 
-	for _, reviewer := range r.RequestedReviewers {
+	for _, reviewer := range r.RequiredReviewers {
 		if !r.IsReviewedBy(reviewer) {
 			missing = append(missing, reviewer)
 		}
@@ -169,12 +188,11 @@ func (r *PRReviews) FindReviewRequester(reviewer string) *models.TimelineComment
 		return nil
 	}
 
-	for _, r := range r.FullTimeline {
-		if r.Type == TimelineCommentType_ReviewRequested && r.Assignee.UserName == reviewer {
-			return r
+	for _, t := range r.RequestedReviewers {
+		if t.Assignee.UserName == reviewer {
+			return t
 		}
 	}
-
 	return nil
 }
 
@@ -188,10 +206,19 @@ func (r *PRReviews) HasPendingReviewBy(reviewer string) bool {
 			switch r.State {
 			case ReviewStateRequestReview, ReviewStatePending:
 				return true
+			default:
+				return false
 			}
 		}
 	}
 
+	// at this point, we do not have actual review by user. Check if we have a pending review
+	for _, t := range r.RequestedReviewers {
+		if t.Assignee != nil && t.Assignee.UserName == reviewer {
+			return true
+		}
+	}
+
 	return false
 }
 
@@ -200,20 +227,18 @@ func (r *PRReviews) IsReviewedBy(reviewer string) bool {
 		return false
 	}
 
-	res := false
-
-	for _, i := range r.Reviews {
-		if i.User.UserName == reviewer && !i.Stale {
-			switch i.State {
+	for _, r := range r.Reviews {
+		if r.User.UserName == reviewer && !r.Stale {
+			switch r.State {
 			case ReviewStateApproved, ReviewStateRequestChanges:
-				res = true
-			case ReviewStateRequestReview, ReviewStatePending:
+				return true
+			default:
 				return false
 			}
 		}
 	}
 
-	return res
+	return false
 }
 
 func (r *PRReviews) IsReviewedByOneOf(reviewers ...string) bool {

common/reviews_test.go

@@ -137,6 +137,61 @@ func TestReviews(t *testing.T) {
 			isApproved:        false,
 			isReviewedByTest1: true,
 		},
+		{
+			name: "Ghost user review",
+			reviews: []*models.PullReview{
+				{State: common.ReviewStateApproved, User: nil},
+			},
+			reviewers:  []string{"user1"},
+			isApproved: false,
+		},
+		{
+			name:    "ReviewRequested predates PushPull should be seen as pending",
+			reviews: []*models.PullReview{},
+			timeline: []*models.TimelineComment{
+				{Type: common.TimelineCommentType_PushPull},
+				{Type: common.TimelineCommentType_ReviewRequested, Assignee: &models.User{UserName: "user1"}},
+			},
+			reviewers:        []string{"user1"},
+			isPendingByTest1: true,
+		},
+		{
+			name:    "ReviewRequested postdates PushPull but blocked by older dismiss",
+			reviews: []*models.PullReview{},
+			timeline: []*models.TimelineComment{
+				{Type: common.TimelineCommentType_ReviewRequested, Assignee: &models.User{UserName: "user1"}},
+				{Type: common.TimelineCommentType_PushPull},
+				{Type: common.TimelineCommentType_ReviewDismissed, Assignee: &models.User{UserName: "user1"}},
+			},
+			reviewers:        []string{"user1"},
+			isPendingByTest1: true,
+		},
+		{
+			name: "ReviewRequested predates PushPull should be seen as pending",
+			reviews: []*models.PullReview{
+				{ID: 101, State: common.ReviewStateRequestReview, User: &models.User{UserName: "user1"}},
+			},
+			timeline: []*models.TimelineComment{
+				{Type: common.TimelineCommentType_PushPull},
+				{Type: common.TimelineCommentType_ReviewRequested, Assignee: &models.User{UserName: "user1"}},
+			},
+			reviewers:        []string{"user1"},
+			isPendingByTest1: true,
+		},
+		{
+			name: "Review requested, review, then push needs re-requesting",
+			reviews: []*models.PullReview{
+				{ID: 100, State: common.ReviewStateRequestChanges, User: &models.User{UserName: "user1"}},
+			},
+			timeline: []*models.TimelineComment{
+				{Type: common.TimelineCommentType_PushPull},
+				{Type: common.TimelineCommentType_Review, ReviewID: 100},
+				{Type: common.TimelineCommentType_ReviewRequested, Assignee: &models.User{UserName: "user1"}},
+			},
+			reviewers:         []string{"user1"},
+			isReviewedByTest1: false, // Should be stale
+			isPendingByTest1:  false, // Should be stale
+		},
 	}
 
 	for _, test := range tests {
@@ -158,7 +213,7 @@ func TestReviews(t *testing.T) {
 				}
 				return
 			}
-			reviews.RequestedReviewers = test.reviewers
+			reviews.SetRequiredReviewers(test.reviewers)
 
 			if r := reviews.IsApproved(); r != test.isApproved {
 				t.Fatal("Unexpected IsReviewed():", r, "vs. expected", test.isApproved)

common/timeline.go

@@ -8,6 +8,7 @@ import (
 )
 
 const (
+	TimelineCommentType_ReviewDismissed = "dismiss_review"
 	TimelineCommentType_ReviewRequested = "review_request"
 	TimelineCommentType_Review          = "review"
 	TimelineCommentType_PushPull        = "pull_push"

integration/podman-compose.yml

@@ -8,6 +8,7 @@ services:
   gitea:
     build: ./gitea
     container_name: gitea-test
+    init: true
     environment:
       - GITEA_WORK_DIR=/var/lib/gitea
     networks:
@@ -27,6 +28,7 @@ services:
   rabbitmq:
     image: rabbitmq:3.13.7-management
     container_name: rabbitmq-test
+    init: true
     healthcheck:
       test: ["CMD", "rabbitmq-diagnostics", "check_running", "-q"]
       interval: 30s
@@ -55,6 +57,7 @@ services:
       context: ..
       dockerfile: integration/gitea-events-rabbitmq-publisher/Dockerfile${GIWTF_IMAGE_SUFFIX}
     container_name: gitea-publisher
+    init: true
     networks:
       - gitea-network
     depends_on:
@@ -75,6 +78,7 @@ services:
       context: ..
       dockerfile: integration/workflow-pr/Dockerfile${GIWTF_IMAGE_SUFFIX}
     container_name: workflow-pr
+    init: true
     networks:
       - gitea-network
     depends_on:
@@ -103,6 +107,7 @@ services:
   mock-obs:
     build: ./mock-obs
     container_name: mock-obs
+    init: true
     networks:
       - gitea-network
     ports:
@@ -116,6 +121,7 @@ services:
       context: ..
       dockerfile: integration/obs-staging-bot/Dockerfile${GIWTF_IMAGE_SUFFIX}
     container_name: obs-staging-bot
+    init: true
     networks:
       - gitea-network
     depends_on:

workflow-pr/dummy

@@ -0,0 +1 @@
+5