Skip to content

Commit 5ca224a

Browse files
authored
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.
1 parent 59b30f0 commit 5ca224a

File tree

16 files changed

+493
-45
lines changed

16 files changed

+493
-45
lines changed

models/migrations/migrations.go

+2
Original file line numberDiff line numberDiff line change
@@ -385,6 +385,8 @@ var migrations = []Migration{
385385
NewMigration("Add allow edits from maintainers to PullRequest table", addAllowMaintainerEdit),
386386
// v214 -> v215
387387
NewMigration("Add auto merge table", addAutoMergeTable),
388+
// v215 -> v216
389+
NewMigration("allow to view files in PRs", addReviewViewedFiles),
388390
}
389391

390392
// GetCurrentDBVersion returns the current db version

models/migrations/v215.go

+25
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,25 @@
1+
// Copyright 2022 The Gitea Authors. All rights reserved.
2+
// Use of this source code is governed by a MIT-style
3+
// license that can be found in the LICENSE file.
4+
5+
package migrations
6+
7+
import (
8+
"code.gitea.io/gitea/models/pull"
9+
"code.gitea.io/gitea/modules/timeutil"
10+
11+
"xorm.io/xorm"
12+
)
13+
14+
func addReviewViewedFiles(x *xorm.Engine) error {
15+
type ReviewState struct {
16+
ID int64 `xorm:"pk autoincr"`
17+
UserID int64 `xorm:"NOT NULL UNIQUE(pull_commit_user)"`
18+
PullID int64 `xorm:"NOT NULL UNIQUE(pull_commit_user) DEFAULT 0"`
19+
CommitSHA string `xorm:"NOT NULL VARCHAR(40) UNIQUE(pull_commit_user)"`
20+
UpdatedFiles map[string]pull.ViewedState `xorm:"NOT NULL LONGTEXT JSON"`
21+
UpdatedUnix timeutil.TimeStamp `xorm:"updated"`
22+
}
23+
24+
return x.Sync2(new(ReviewState))
25+
}

models/pull/review_state.go

+139
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,139 @@
1+
// Copyright 2022 The Gitea Authors. All rights reserved.
2+
// Use of this source code is governed by a MIT-style
3+
// license that can be found in the LICENSE file.
4+
package pull
5+
6+
import (
7+
"context"
8+
"fmt"
9+
10+
"code.gitea.io/gitea/models/db"
11+
"code.gitea.io/gitea/modules/log"
12+
"code.gitea.io/gitea/modules/timeutil"
13+
)
14+
15+
// ViewedState stores for a file in which state it is currently viewed
16+
type ViewedState uint8
17+
18+
const (
19+
Unviewed ViewedState = iota
20+
HasChanged // cannot be set from the UI/ API, only internally
21+
Viewed
22+
)
23+
24+
func (viewedState ViewedState) String() string {
25+
switch viewedState {
26+
case Unviewed:
27+
return "unviewed"
28+
case HasChanged:
29+
return "has-changed"
30+
case Viewed:
31+
return "viewed"
32+
default:
33+
return fmt.Sprintf("unknown(value=%d)", viewedState)
34+
}
35+
}
36+
37+
// ReviewState stores for a user-PR-commit combination which files the user has already viewed
38+
type ReviewState struct {
39+
ID int64 `xorm:"pk autoincr"`
40+
UserID int64 `xorm:"NOT NULL UNIQUE(pull_commit_user)"`
41+
PullID int64 `xorm:"NOT NULL UNIQUE(pull_commit_user) DEFAULT 0"` // Which PR was the review on?
42+
CommitSHA string `xorm:"NOT NULL VARCHAR(40) UNIQUE(pull_commit_user)"` // Which commit was the head commit for the review?
43+
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
44+
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
45+
}
46+
47+
func init() {
48+
db.RegisterModel(new(ReviewState))
49+
}
50+
51+
// GetReviewState returns the ReviewState with all given values prefilled, whether or not it exists in the database.
52+
// If the review didn't exist before in the database, it won't afterwards either.
53+
// The returned boolean shows whether the review exists in the database
54+
func GetReviewState(ctx context.Context, userID, pullID int64, commitSHA string) (*ReviewState, bool, error) {
55+
review := &ReviewState{UserID: userID, PullID: pullID, CommitSHA: commitSHA}
56+
has, err := db.GetEngine(ctx).Get(review)
57+
return review, has, err
58+
}
59+
60+
// UpdateReviewState updates the given review inside the database, regardless of whether it existed before or not
61+
// The given map of files with their viewed state will be merged with the previous review, if present
62+
func UpdateReviewState(ctx context.Context, userID, pullID int64, commitSHA string, updatedFiles map[string]ViewedState) error {
63+
log.Trace("Updating review for user %d, repo %d, commit %s with the updated files %v.", userID, pullID, commitSHA, updatedFiles)
64+
65+
review, exists, err := GetReviewState(ctx, userID, pullID, commitSHA)
66+
if err != nil {
67+
return err
68+
}
69+
70+
if exists {
71+
review.UpdatedFiles = mergeFiles(review.UpdatedFiles, updatedFiles)
72+
} else if previousReview, err := getNewestReviewStateApartFrom(ctx, userID, pullID, commitSHA); err != nil {
73+
return err
74+
75+
// Overwrite the viewed files of the previous review if present
76+
} else if previousReview != nil {
77+
review.UpdatedFiles = mergeFiles(previousReview.UpdatedFiles, updatedFiles)
78+
} else {
79+
review.UpdatedFiles = updatedFiles
80+
}
81+
82+
// Insert or Update review
83+
engine := db.GetEngine(ctx)
84+
if !exists {
85+
log.Trace("Inserting new review for user %d, repo %d, commit %s with the updated files %v.", userID, pullID, commitSHA, review.UpdatedFiles)
86+
_, err := engine.Insert(review)
87+
return err
88+
}
89+
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)
90+
_, err = engine.ID(review.ID).Update(&ReviewState{UpdatedFiles: review.UpdatedFiles})
91+
return err
92+
}
93+
94+
// mergeFiles merges the given maps of files with their viewing state into one map.
95+
// Values from oldFiles will be overridden with values from newFiles
96+
func mergeFiles(oldFiles, newFiles map[string]ViewedState) map[string]ViewedState {
97+
if oldFiles == nil {
98+
return newFiles
99+
} else if newFiles == nil {
100+
return oldFiles
101+
}
102+
103+
for file, viewed := range newFiles {
104+
oldFiles[file] = viewed
105+
}
106+
return oldFiles
107+
}
108+
109+
// GetNewestReviewState gets the newest review of the current user in the current PR.
110+
// The returned PR Review will be nil if the user has not yet reviewed this PR.
111+
func GetNewestReviewState(ctx context.Context, userID, pullID int64) (*ReviewState, error) {
112+
var review ReviewState
113+
has, err := db.GetEngine(ctx).Where("user_id = ?", userID).And("pull_id = ?", pullID).OrderBy("updated_unix DESC").Get(&review)
114+
if err != nil || !has {
115+
return nil, err
116+
}
117+
return &review, err
118+
}
119+
120+
// getNewestReviewStateApartFrom is like GetNewestReview, except that the second newest review will be returned if the newest review points at the given commit.
121+
// The returned PR Review will be nil if the user has not yet reviewed this PR.
122+
func getNewestReviewStateApartFrom(ctx context.Context, userID, pullID int64, commitSHA string) (*ReviewState, error) {
123+
var reviews []ReviewState
124+
err := db.GetEngine(ctx).Where("user_id = ?", userID).And("pull_id = ?", pullID).OrderBy("updated_unix DESC").Limit(2).Find(&reviews)
125+
// It would also be possible to use ".And("commit_sha != ?", commitSHA)" instead of the error handling below
126+
// However, benchmarks show drastically improved performance by not doing that
127+
128+
// Error cases in which no review should be returned
129+
if err != nil || len(reviews) == 0 || (len(reviews) == 1 && reviews[0].CommitSHA == commitSHA) {
130+
return nil, err
131+
132+
// The first review points at the commit to exclude, hence skip to the second review
133+
} else if len(reviews) >= 2 && reviews[0].CommitSHA == commitSHA {
134+
return &reviews[1], nil
135+
}
136+
137+
// As we have no error cases left, the result must be the first element in the list
138+
return &reviews[0], nil
139+
}

modules/git/repo_compare.go

+9
Original file line numberDiff line numberDiff line change
@@ -286,6 +286,15 @@ func (repo *Repository) GetPatch(base, head string, w io.Writer) error {
286286
return err
287287
}
288288

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

options/locale/locale_en-US.ini

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

routers/web/repo/pull.go

+25-12
Original file line numberDiff line numberDiff line change
@@ -685,22 +685,35 @@ func ViewPullFiles(ctx *context.Context) {
685685
if fileOnly && (len(files) == 2 || len(files) == 1) {
686686
maxLines, maxFiles = -1, -1
687687
}
688-
689-
diff, err := gitdiff.GetDiff(gitRepo,
690-
&gitdiff.DiffOptions{
691-
BeforeCommitID: startCommitID,
692-
AfterCommitID: endCommitID,
693-
SkipTo: ctx.FormString("skip-to"),
694-
MaxLines: maxLines,
695-
MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters,
696-
MaxFiles: maxFiles,
697-
WhitespaceBehavior: gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)),
698-
}, ctx.FormStrings("files")...)
688+
diffOptions := &gitdiff.DiffOptions{
689+
BeforeCommitID: startCommitID,
690+
AfterCommitID: endCommitID,
691+
SkipTo: ctx.FormString("skip-to"),
692+
MaxLines: maxLines,
693+
MaxLineCharacters: setting.Git.MaxGitDiffLineCharacters,
694+
MaxFiles: maxFiles,
695+
WhitespaceBehavior: gitdiff.GetWhitespaceFlag(ctx.Data["WhitespaceBehavior"].(string)),
696+
}
697+
698+
var methodWithError string
699+
var diff *gitdiff.Diff
700+
if !ctx.IsSigned {
701+
diff, err = gitdiff.GetDiff(gitRepo, diffOptions, files...)
702+
methodWithError = "GetDiff"
703+
} else {
704+
diff, err = gitdiff.SyncAndGetUserSpecificDiff(ctx, ctx.Doer.ID, pull, gitRepo, diffOptions, files...)
705+
methodWithError = "SyncAndGetUserSpecificDiff"
706+
}
699707
if err != nil {
700-
ctx.ServerError("GetDiffRangeWithWhitespaceBehavior", err)
708+
ctx.ServerError(methodWithError, err)
701709
return
702710
}
703711

712+
ctx.PageData["prReview"] = map[string]interface{}{
713+
"numberOfFiles": diff.NumFiles,
714+
"numberOfViewedFiles": diff.NumViewedFiles,
715+
}
716+
704717
if err = diff.LoadComments(ctx, issue, ctx.Doer); err != nil {
705718
ctx.ServerError("LoadComments", err)
706719
return

routers/web/repo/pull_review.go

+46
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,10 @@ import (
99
"net/http"
1010

1111
"code.gitea.io/gitea/models"
12+
pull_model "code.gitea.io/gitea/models/pull"
1213
"code.gitea.io/gitea/modules/base"
1314
"code.gitea.io/gitea/modules/context"
15+
"code.gitea.io/gitea/modules/json"
1416
"code.gitea.io/gitea/modules/log"
1517
"code.gitea.io/gitea/modules/setting"
1618
"code.gitea.io/gitea/modules/web"
@@ -242,3 +244,47 @@ func DismissReview(ctx *context.Context) {
242244

243245
ctx.Redirect(fmt.Sprintf("%s/pulls/%d#%s", ctx.Repo.RepoLink, comm.Issue.Index, comm.HashTag()))
244246
}
247+
248+
// viewedFilesUpdate Struct to parse the body of a request to update the reviewed files of a PR
249+
// If you want to implement an API to update the review, simply move this struct into modules.
250+
type viewedFilesUpdate struct {
251+
Files map[string]bool `json:"files"`
252+
HeadCommitSHA string `json:"headCommitSHA"`
253+
}
254+
255+
func UpdateViewedFiles(ctx *context.Context) {
256+
// Find corresponding PR
257+
issue := checkPullInfo(ctx)
258+
if ctx.Written() {
259+
return
260+
}
261+
pull := issue.PullRequest
262+
263+
var data *viewedFilesUpdate
264+
err := json.NewDecoder(ctx.Req.Body).Decode(&data)
265+
if err != nil {
266+
log.Warn("Attempted to update a review but could not parse request body: %v", err)
267+
ctx.Resp.WriteHeader(http.StatusBadRequest)
268+
return
269+
}
270+
271+
// Expect the review to have been now if no head commit was supplied
272+
if data.HeadCommitSHA == "" {
273+
data.HeadCommitSHA = pull.HeadCommitID
274+
}
275+
276+
updatedFiles := make(map[string]pull_model.ViewedState, len(data.Files))
277+
for file, viewed := range data.Files {
278+
279+
// Only unviewed and viewed are possible, has-changed can not be set from the outside
280+
state := pull_model.Unviewed
281+
if viewed {
282+
state = pull_model.Viewed
283+
}
284+
updatedFiles[file] = state
285+
}
286+
287+
if err := pull_model.UpdateReviewState(ctx, ctx.Doer.ID, pull.ID, data.HeadCommitSHA, updatedFiles); err != nil {
288+
ctx.ServerError("UpdateReview", err)
289+
}
290+
}

routers/web/web.go

+1
Original file line numberDiff line numberDiff line change
@@ -849,6 +849,7 @@ func RegisterRoutes(m *web.Route) {
849849
m.Post("/deadline", bindIgnErr(structs.EditDeadlineOption{}), repo.UpdateIssueDeadline)
850850
m.Post("/watch", repo.IssueWatch)
851851
m.Post("/ref", repo.UpdateIssueRef)
852+
m.Post("/viewed-files", repo.UpdateViewedFiles)
852853
m.Group("/dependency", func() {
853854
m.Post("/add", repo.AddDependency)
854855
m.Post("/delete", repo.RemoveDependency)

0 commit comments

Comments
 (0)