Allow to mark files in a PR as viewed

models/migrations/migrations.go

@@ -374,6 +374,11 @@ var migrations = []Migration{
 	NewMigration("Increase WebAuthentication CredentialID size to 410 - NO-OPED", increaseCredentialIDTo410),
 	// v210 -> v211
 	NewMigration("v208 was completely broken - remigrate", remigrateU2FCredentials),
+
+	// Gitea 1.16.2 ends at v211
+
+	// v211 -> v212
+	NewMigration("allow to view files in PRs", addPRReviewedFiles),
 }
 
 // GetCurrentDBVersion returns the current db version

models/migrations/v210.go

@@ -11,8 +11,8 @@ import (
 	"strings"
 
 	"code.gitea.io/gitea/modules/timeutil"
-	"github.com/tstranex/u2f"
 
+	"github.com/tstranex/u2f"
 	"xorm.io/xorm"
 	"xorm.io/xorm/schemas"
 )

models/migrations/v210_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	"code.gitea.io/gitea/modules/timeutil"
+
 	"github.com/stretchr/testify/assert"
 	"xorm.io/xorm/schemas"
 )

models/migrations/v211.go

@@ -0,0 +1,24 @@
+// 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/modules/timeutil"
+
+	"xorm.io/xorm"
+)
+
+func addPRReviewedFiles(x *xorm.Engine) error {
+	type PRReview struct {
+		ID          int64              `xorm:"pk autoincr"`
+		UserID      int64              `xorm:"NOT NULL UNIQUE(pull_commit_user)"`
+		ViewedFiles map[string]bool    `xorm:"TEXT JSON"`
+		CommitSHA   string             `xorm:"NOT NULL UNIQUE(pull_commit_user)"`
+		PullID      int64              `xorm:"NOT NULL UNIQUE(pull_commit_user)"`
+		UpdatedUnix timeutil.TimeStamp `xorm:"updated"`
+	}
+
+	return x.Sync2(new(PRReview))
+}

models/pulls/pull_review.go

@@ -0,0 +1,109 @@
+// 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 pulls
+
+import (
+	"code.gitea.io/gitea/models/db"
+	"code.gitea.io/gitea/modules/timeutil"
+)
+
+// PRReview stores for a user - PR - commit combination which files the user has already viewed
+type PRReview struct {
+	ID          int64              `xorm:"pk autoincr"`
+	UserID      int64              `xorm:"NOT NULL UNIQUE(pull_commit_user)"`
+	ViewedFiles map[string]bool    `xorm:"TEXT JSON"`                         // Stores for each of the changed files of a PR whether they have been viewed or not
+	CommitSHA   string             `xorm:"NOT NULL UNIQUE(pull_commit_user)"` // Which commit was the head commit for the review?
+	PullID      int64              `xorm:"NOT NULL UNIQUE(pull_commit_user)"` // Which PR was the review on?
+	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(PRReview))
+}
+
+// GetReview returns the PRReview 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 GetReview(userID, pullID int64, commitSHA string) (*PRReview, bool, error) {
+	review := &PRReview{UserID: userID, CommitSHA: commitSHA, PullID: pullID}
+	has, err := db.GetEngine(db.DefaultContext).Get(review)
+	return review, has, err
+}
+
+// UpdateReview updates the given review inside the database, regardless of whether it existed before or not
+// The given map of viewed files will be merged with the previous review, if present
+func UpdateReview(userID, pullID int64, commitSHA string, viewedFiles map[string]bool) error {
+	review, exists, err := GetReview(userID, pullID, commitSHA)
+	if err != nil {
+		return err
+	}
+
+	if exists {
+		review.ViewedFiles = mergeFiles(review.ViewedFiles, viewedFiles)
+	} else if previousReview, err := getNewestReviewApartFrom(commitSHA, userID, pullID); err != nil {
+		return err
+
+		// Overwrite the viewed files of the previous review if present
+	} else if previousReview != nil {
+		review.ViewedFiles = mergeFiles(previousReview.ViewedFiles, viewedFiles)
+	} else {
+		review.ViewedFiles = viewedFiles
+	}
+
+	// Insert or Update review
+	engine := db.GetEngine(db.DefaultContext)
+	if !exists {
+		_, err := engine.Insert(review)
+		return err
+	}
+	_, err = engine.ID(review.ID).Update(review)
+	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]bool) map[string]bool {
+	if oldFiles == nil {
+		return newFiles
+	} else if newFiles == nil {
+		return oldFiles
+	}
+
+	for file, viewed := range newFiles {
+		oldFiles[file] = viewed
+	}
+	return oldFiles
+}
+
+// GetNewestReview 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 GetNewestReview(userID, pullID int64) (*PRReview, error) {
+	var review PRReview
+	has, err := db.GetEngine(db.DefaultContext).Where("user_id = ?", userID).And("pull_id = ?", pullID).OrderBy("updated_unix DESC").Limit(1).Get(&review)
+	if err != nil || !has {
+		return nil, err
+	}
+	return &review, err
+}
+
+// getNewestReviewApartFrom 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 getNewestReviewApartFrom(commitSHA string, userID, pullID int64) (*PRReview, error) {
+	var reviews []PRReview
+	err := db.GetEngine(db.DefaultContext).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 a MASSIVE performance gain by not doing that: 1000 ms => <300 ms
+
+	// 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

@@ -292,6 +292,40 @@ func (repo *Repository) GetPatch(base, head string, w io.Writer) error {
 	return err
 }
 
+type lineWriter struct {
+	lines                 []string
+	hasEndedWithLineBreak bool
+}
+
+// Write accumulates all lines in the provided bytestream
+func (lineWriter *lineWriter) Write(p []byte) (int, error) {
+	buffer := string(p)
+	lines := strings.Split(buffer, "\n")
+
+	// Merge the previously last value with the first value now in case there was only a buffer end and no newline
+	if !lineWriter.hasEndedWithLineBreak && len(lineWriter.lines) > 0 {
+		lineWriter.lines[len(lineWriter.lines)-1] = lineWriter.lines[len(lineWriter.lines)-1] + lines[0]
+		lines = lines[1:]
+	}
+	lineWriter.lines = append(lineWriter.lines, lines...)
+	lineWriter.hasEndedWithLineBreak = strings.HasSuffix(buffer, "\n")
+	return len(p), nil
+}
+
+// GetFilesChangedBetween returns a list of all files that have been changed between the given commits
+func (repo *Repository) GetFilesChangedBetween(base, head string) ([]string, error) {
+	stderr := new(bytes.Buffer)
+	w := &lineWriter{}
+	err := NewCommand(repo.Ctx, "diff", "--name-only", base+".."+head).
+		RunWithContext(&RunContext{
+			Timeout: -1,
+			Dir:     repo.Path,
+			Stdout:  w,
+			Stderr:  stderr,
+		})
+	return w.lines, 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

@@ -1480,6 +1480,9 @@ pulls.new = New Pull Request
 pulls.view = View Pull Request
 pulls.compare_changes = New Pull Request
 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

@@ -683,19 +683,29 @@ 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.GetUserSpecificDiff(ctx.User.ID, checkPullInfo(ctx).PullRequest, gitRepo, diffOptions, ctx.FormStrings("files")...)
+		ctx.PageData["numberOfFiles"] = diff.NumFiles
+		ctx.PageData["numberOfViewedFiles"] = diff.NumViewedFiles
+		methodWithError = "GetUserSpecificDiff"
+	}
 	if err != nil {
-		ctx.ServerError("GetDiffRangeWithWhitespaceBehavior", err)
+		ctx.ServerError(methodWithError, err)
 		return
 	}
 

routers/web/repo/pull_review.go

@@ -7,8 +7,11 @@ package repo
 import (
 	"fmt"
 	"net/http"
+	"strconv"
+	"strings"
 
 	"code.gitea.io/gitea/models"
+	"code.gitea.io/gitea/models/pulls"
 	"code.gitea.io/gitea/modules/base"
 	"code.gitea.io/gitea/modules/context"
 	"code.gitea.io/gitea/modules/log"
@@ -242,3 +245,43 @@ func DismissReview(ctx *context.Context) {
 
 	ctx.Redirect(fmt.Sprintf("%s/pulls/%d#%s", ctx.Repo.RepoLink, comm.Issue.Index, comm.HashTag()))
 }
+
+const headCommitKey = "_headCommitSHA"
+
+func UpdateViewedFiles(ctx *context.Context) {
+	pull := checkPullInfo(ctx).PullRequest
+	if ctx.Written() {
+		return
+	}
+	updatedFiles := make(map[string]bool, len(ctx.Req.Form))
+	headCommitSHA := ""
+
+	// Collect all files and their viewed state
+	for file, values := range ctx.Req.Form {
+		for _, viewedString := range values {
+			viewed, err := strconv.ParseBool(viewedString)
+			// Ignore fields that do not parse as a boolean, i.e. the CSRF token
+			if err != nil {
+
+				// Prevent invalid reviews by specifically supplying the commit the user viewed the file under
+				if file == headCommitKey {
+					headCommitSHA = viewedString
+				}
+				continue
+			}
+			updatedFiles[strings.ReplaceAll(file, "%22", "\"")] = viewed
+			// \" is the only character that gets encoded when sent as form-encoded string.
+			// Unfortunately, this WILL break marking any file as viewed that contains an actual "%22" in its name.
+			// There is no way to prevent this, and "%22" is way more unlikely to occur in a filename than \".
+		}
+	}
+
+	// No head commit SHA was supplied - expect the review to have been now
+	if headCommitSHA == "" {
+		headCommitSHA = pull.HeadCommitID
+	}
+
+	if err := pulls.UpdateReview(ctx.User.ID, pull.ID, headCommitSHA, updatedFiles); err != nil {
+		ctx.ServerError("UpdateReview", err)
+	}
+}

routers/web/web.go

@@ -741,6 +741,7 @@ func RegisterRoutes(m *web.Route) {
 				m.Post("/content", repo.UpdateIssueContent)
 				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)