Allow to mark files in a PR as viewed (#19007)

Users can now mark files in PRs as viewed, resulting in them not being shown again by default when they reopen the PR again.

models/migrations/migrations.go

@@ -385,6 +385,8 @@ var migrations = []Migration{
 	NewMigration("Add allow edits from maintainers to PullRequest table", addAllowMaintainerEdit),
 	// v214 -> v215
 	NewMigration("Add auto merge table", addAutoMergeTable),
+	// v215 -> v216
+	NewMigration("allow to view files in PRs", addReviewViewedFiles),
 }
 
 // GetCurrentDBVersion returns the current db version

models/migrations/v215.go

@@ -0,0 +1,25 @@
+// Copyright 2022 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+
+package migrations
+
+import (
+	"code.gitea.io/gitea/models/pull"
+	"code.gitea.io/gitea/modules/timeutil"
+
+	"xorm.io/xorm"
+)
+
+func addReviewViewedFiles(x *xorm.Engine) error {
+	type ReviewState struct {
+		ID           int64                       `xorm:"pk autoincr"`
+		UserID       int64                       `xorm:"NOT NULL UNIQUE(pull_commit_user)"`
+		PullID       int64                       `xorm:"NOT NULL UNIQUE(pull_commit_user) DEFAULT 0"`
+		CommitSHA    string                      `xorm:"NOT NULL VARCHAR(40) UNIQUE(pull_commit_user)"`
+		UpdatedFiles map[string]pull.ViewedState `xorm:"NOT NULL LONGTEXT JSON"`
+		UpdatedUnix  timeutil.TimeStamp          `xorm:"updated"`
+	}
+
+	return x.Sync2(new(ReviewState))
+}

models/pull/review_state.go

@@ -0,0 +1,139 @@
+// Copyright 2022 The Gitea Authors. All rights reserved.
+// Use of this source code is governed by a MIT-style
+// license that can be found in the LICENSE file.
+package pull
+
+import (
+	"context"
+	"fmt"
+
+	"code.gitea.io/gitea/models/db"
+	"code.gitea.io/gitea/modules/log"
+	"code.gitea.io/gitea/modules/timeutil"
+)
+
+// ViewedState stores for a file in which state it is currently viewed
+type ViewedState uint8
+
+const (
+	Unviewed   ViewedState = iota
+	HasChanged             // cannot be set from the UI/ API, only internally
+	Viewed
+)
+
+func (viewedState ViewedState) String() string {
+	switch viewedState {
+	case Unviewed:
+		return "unviewed"
+	case HasChanged:
+		return "has-changed"
+	case Viewed:
+		return "viewed"
+	default:
+		return fmt.Sprintf("unknown(value=%d)", viewedState)
+	}
+}
+
+// ReviewState stores for a user-PR-commit combination which files the user has already viewed
+type ReviewState struct {
+	ID           int64                  `xorm:"pk autoincr"`
+	UserID       int64                  `xorm:"NOT NULL UNIQUE(pull_commit_user)"`
+	PullID       int64                  `xorm:"NOT NULL UNIQUE(pull_commit_user) DEFAULT 0"`   // Which PR was the review on?
+	CommitSHA    string                 `xorm:"NOT NULL VARCHAR(40) UNIQUE(pull_commit_user)"` // Which commit was the head commit for the review?
+	UpdatedFiles map[string]ViewedState `xorm:"NOT NULL LONGTEXT JSON"`                        // Stores for each of the changed files of a PR whether they have been viewed, changed since last viewed, or not viewed
+	UpdatedUnix  timeutil.TimeStamp     `xorm:"updated"`                                       // Is an accurate indicator of the order of commits as we do not expect it to be possible to make reviews on previous commits
+}
+
+func init() {
+	db.RegisterModel(new(ReviewState))
+}
+
+// GetReviewState returns the ReviewState with all given values prefilled, whether or not it exists in the database.
+// If the review didn't exist before in the database, it won't afterwards either.
+// The returned boolean shows whether the review exists in the database
+func GetReviewState(ctx context.Context, userID, pullID int64, commitSHA string) (*ReviewState, bool, error) {
+	review := &ReviewState{UserID: userID, PullID: pullID, CommitSHA: commitSHA}
+	has, err := db.GetEngine(ctx).Get(review)
+	return review, has, err
+}
+
+// UpdateReviewState updates the given review inside the database, regardless of whether it existed before or not
+// The given map of files with their viewed state will be merged with the previous review, if present
+func UpdateReviewState(ctx context.Context, userID, pullID int64, commitSHA string, updatedFiles map[string]ViewedState) error {
+	log.Trace("Updating review for user %d, repo %d, commit %s with the updated files %v.", userID, pullID, commitSHA, updatedFiles)
+
+	review, exists, err := GetReviewState(ctx, userID, pullID, commitSHA)
+	if err != nil {
+		return err
+	}
+
+	if exists {
+		review.UpdatedFiles = mergeFiles(review.UpdatedFiles, updatedFiles)
+	} else if previousReview, err := getNewestReviewStateApartFrom(ctx, userID, pullID, commitSHA); err != nil {
+		return err
+
+		// Overwrite the viewed files of the previous review if present
+	} else if previousReview != nil {
+		review.UpdatedFiles = mergeFiles(previousReview.UpdatedFiles, updatedFiles)
+	} else {
+		review.UpdatedFiles = updatedFiles
+	}
+
+	// Insert or Update review
+	engine := db.GetEngine(ctx)
+	if !exists {
+		log.Trace("Inserting new review for user %d, repo %d, commit %s with the updated files %v.", userID, pullID, commitSHA, review.UpdatedFiles)
+		_, err := engine.Insert(review)
+		return err
+	}
+	log.Trace("Updating already existing review with ID %d (user %d, repo %d, commit %s) with the updated files %v.", review.ID, userID, pullID, commitSHA, review.UpdatedFiles)
+	_, err = engine.ID(review.ID).Update(&ReviewState{UpdatedFiles: review.UpdatedFiles})
+	return err
+}
+
+// mergeFiles merges the given maps of files with their viewing state into one map.
+// Values from oldFiles will be overridden with values from newFiles
+func mergeFiles(oldFiles, newFiles map[string]ViewedState) map[string]ViewedState {
+	if oldFiles == nil {
+		return newFiles
+	} else if newFiles == nil {
+		return oldFiles
+	}
+
+	for file, viewed := range newFiles {
+		oldFiles[file] = viewed
+	}
+	return oldFiles
+}
+
+// GetNewestReviewState gets the newest review of the current user in the current PR.
+// The returned PR Review will be nil if the user has not yet reviewed this PR.
+func GetNewestReviewState(ctx context.Context, userID, pullID int64) (*ReviewState, error) {
+	var review ReviewState
+	has, err := db.GetEngine(ctx).Where("user_id = ?", userID).And("pull_id = ?", pullID).OrderBy("updated_unix DESC").Get(&review)
+	if err != nil || !has {
+		return nil, err
+	}
+	return &review, err
+}
+
+// getNewestReviewStateApartFrom is like GetNewestReview, except that the second newest review will be returned if the newest review points at the given commit.
+// The returned PR Review will be nil if the user has not yet reviewed this PR.
+func getNewestReviewStateApartFrom(ctx context.Context, userID, pullID int64, commitSHA string) (*ReviewState, error) {
+	var reviews []ReviewState
+	err := db.GetEngine(ctx).Where("user_id = ?", userID).And("pull_id = ?", pullID).OrderBy("updated_unix DESC").Limit(2).Find(&reviews)
+	// It would also be possible to use ".And("commit_sha != ?", commitSHA)" instead of the error handling below
+	// However, benchmarks show drastically improved performance by not doing that
+
+	// Error cases in which no review should be returned
+	if err != nil || len(reviews) == 0 || (len(reviews) == 1 && reviews[0].CommitSHA == commitSHA) {
+		return nil, err
+
+		// The first review points at the commit to exclude, hence skip to the second review
+	} else if len(reviews) >= 2 && reviews[0].CommitSHA == commitSHA {
+		return &reviews[1], nil
+	}
+
+	// As we have no error cases left, the result must be the first element in the list
+	return &reviews[0], nil
+}

modules/git/repo_compare.go

@@ -286,6 +286,15 @@ func (repo *Repository) GetPatch(base, head string, w io.Writer) error {
 	return err
 }
 
+// GetFilesChangedBetween returns a list of all files that have been changed between the given commits
+func (repo *Repository) GetFilesChangedBetween(base, head string) ([]string, error) {
+	stdout, _, err := NewCommand(repo.Ctx, "diff", "--name-only", base+".."+head).RunStdString(&RunOpts{Dir: repo.Path})
+	if err != nil {
+		return nil, err
+	}
+	return strings.Split(stdout, "\n"), err
+}
+
 // GetDiffFromMergeBase generates and return patch data from merge base to head
 func (repo *Repository) GetDiffFromMergeBase(base, head string, w io.Writer) error {
 	stderr := new(bytes.Buffer)

options/locale/locale_en-US.ini

@@ -1493,6 +1493,9 @@ pulls.allow_edits_from_maintainers = Allow edits from maintainers
 pulls.allow_edits_from_maintainers_desc = Users with write access to the base branch can also push to this branch
 pulls.allow_edits_from_maintainers_err = Updating failed
 pulls.compare_changes_desc = Select the branch to merge into and the branch to pull from.
+pulls.has_viewed_file = Viewed
+pulls.has_changed_since_last_review = Changed since your last review
+pulls.viewed_files_label = %[1]d / %[2]d files viewed
 pulls.compare_base = merge into
 pulls.compare_compare = pull from
 pulls.switch_comparison_type = Switch comparison type

routers/web/repo/pull.go

@@ -685,22 +685,35 @@ func ViewPullFiles(ctx *context.Context) {
 	if fileOnly && (len(files) == 2 || len(files) == 1) {
 		maxLines, maxFiles = -1, -1
 	}
-
-	diff, err := gitdiff.GetDiff(gitRepo,
-		&gitdiff.DiffOptions{
-			BeforeCommitID:     startCommitID,
-			AfterCommitID:      endCommitID,
-			SkipTo:             ctx.FormString("skip-to"),
-			MaxLines:           maxLines,
-			MaxLineCharacters:  setting.Git.MaxGitDiffLineCharacters,
-			MaxFiles:           maxFiles,
-			WhitespaceBehavior: gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)),
-		}, ctx.FormStrings("files")...)
+	diffOptions := &gitdiff.DiffOptions{
+		BeforeCommitID:     startCommitID,
+		AfterCommitID:      endCommitID,
+		SkipTo:             ctx.FormString("skip-to"),
+		MaxLines:           maxLines,
+		MaxLineCharacters:  setting.Git.MaxGitDiffLineCharacters,
+		MaxFiles:           maxFiles,
+		WhitespaceBehavior: gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)),
+	}
+
+	var methodWithError string
+	var diff *gitdiff.Diff
+	if !ctx.IsSigned {
+		diff, err = gitdiff.GetDiff(gitRepo, diffOptions, files...)
+		methodWithError = "GetDiff"
+	} else {
+		diff, err = gitdiff.SyncAndGetUserSpecificDiff(ctx, ctx.Doer.ID, pull, gitRepo, diffOptions, files...)
+		methodWithError = "SyncAndGetUserSpecificDiff"
+	}
 	if err != nil {
-		ctx.ServerError("GetDiffRangeWithWhitespaceBehavior", err)
+		ctx.ServerError(methodWithError, err)
 		return
 	}
 
+	ctx.PageData["prReview"] = map[string]interface{}{
+		"numberOfFiles":       diff.NumFiles,
+		"numberOfViewedFiles": diff.NumViewedFiles,
+	}
+
 	if err = diff.LoadComments(ctx, issue, ctx.Doer); err != nil {
 		ctx.ServerError("LoadComments", err)
 		return

routers/web/repo/pull_review.go

@@ -9,8 +9,10 @@ import (
 	"net/http"
 
 	"code.gitea.io/gitea/models"
+	pull_model "code.gitea.io/gitea/models/pull"
 	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/context"
+	"code.gitea.io/gitea/modules/json"
 	"code.gitea.io/gitea/modules/log"
 	"code.gitea.io/gitea/modules/setting"
 	"code.gitea.io/gitea/modules/web"
@@ -242,3 +244,47 @@ func DismissReview(ctx *context.Context) {
 
 	ctx.Redirect(fmt.Sprintf("%s/pulls/%d#%s", ctx.Repo.RepoLink, comm.Issue.Index, comm.HashTag()))
 }
+
+// viewedFilesUpdate Struct to parse the body of a request to update the reviewed files of a PR
+// If you want to implement an API to update the review, simply move this struct into modules.
+type viewedFilesUpdate struct {
+	Files         map[string]bool `json:"files"`
+	HeadCommitSHA string          `json:"headCommitSHA"`
+}
+
+func UpdateViewedFiles(ctx *context.Context) {
+	// Find corresponding PR
+	issue := checkPullInfo(ctx)
+	if ctx.Written() {
+		return
+	}
+	pull := issue.PullRequest
+
+	var data *viewedFilesUpdate
+	err := json.NewDecoder(ctx.Req.Body).Decode(&data)
+	if err != nil {
+		log.Warn("Attempted to update a review but could not parse request body: %v", err)
+		ctx.Resp.WriteHeader(http.StatusBadRequest)
+		return
+	}
+
+	// Expect the review to have been now if no head commit was supplied
+	if data.HeadCommitSHA == "" {
+		data.HeadCommitSHA = pull.HeadCommitID
+	}
+
+	updatedFiles := make(map[string]pull_model.ViewedState, len(data.Files))
+	for file, viewed := range data.Files {
+
+		// Only unviewed and viewed are possible, has-changed can not be set from the outside
+		state := pull_model.Unviewed
+		if viewed {
+			state = pull_model.Viewed
+		}
+		updatedFiles[file] = state
+	}
+
+	if err := pull_model.UpdateReviewState(ctx, ctx.Doer.ID, pull.ID, data.HeadCommitSHA, updatedFiles); err != nil {
+		ctx.ServerError("UpdateReview", err)
+	}
+}

routers/web/web.go

@@ -849,6 +849,7 @@ func RegisterRoutes(m *web.Route) {
 				m.Post("/deadline", bindIgnErr(structs.EditDeadlineOption{}), repo.UpdateIssueDeadline)
 				m.Post("/watch", repo.IssueWatch)
 				m.Post("/ref", repo.UpdateIssueRef)
+				m.Post("/viewed-files", repo.UpdateViewedFiles)
 				m.Group("/dependency", func() {
 					m.Post("/add", repo.AddDependency)
 					m.Post("/delete", repo.RemoveDependency)