Skip to content

Pull request review/approval and comment on code #3748

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 104 commits into from
Aug 6, 2018
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
104 commits
Select commit Hold shift + click to select a range
85e1ad5
Initial ui components for pull request review
lafriks Apr 2, 2018
623a9f6
Add Review
jonasfranz May 6, 2018
aeb0577
Replace ReviewComment with Content
jonasfranz May 6, 2018
2c18552
Add load functions
jonasfranz May 10, 2018
9544c46
Add create review comment implementation
jonasfranz May 10, 2018
0f772d1
Simplified create and find functions for review
jonasfranz May 10, 2018
4ad563d
Moved "Pending" to first position
jonasfranz May 10, 2018
17af2d1
Add GetCurrentReview to simplify fetching current review
jonasfranz May 10, 2018
75b7d9b
Preview for listing comments
jonasfranz May 11, 2018
e252d3b
Move new comment form to its own file
jonasfranz May 11, 2018
3e5f3c3
Implement Review form
jonasfranz May 11, 2018
61cc134
Add support for single comments
jonasfranz May 11, 2018
36d6631
Add pending tag to pending review comments
jonasfranz May 11, 2018
9c6bb4b
Add unit tests for Review
jonasfranz May 11, 2018
bc93592
Fetch all review ids at once
jonasfranz May 11, 2018
58fb672
gofmt
jonasfranz May 12, 2018
066086c
Improved comment rendering in "Files" view by adding Comments to Diff…
jonasfranz May 12, 2018
7986d6e
Add support for invalidating comments
jonasfranz May 13, 2018
cbdd8c9
Switched back to code.gitea.io/git
jonasfranz May 13, 2018
2f46613
Merge pull request #2 from JonasFranzDEV/feat/approval
lafriks May 13, 2018
6d00c1a
Merge branch 'master' into feat/approval-new
jonasfranz May 13, 2018
7723e15
Moved review migration from v64 to v65
jonasfranz May 13, 2018
90d9dda
Rebuild css
jonasfranz May 13, 2018
5f55ede
gofmt
jonasfranz May 13, 2018
d2b4347
Improve translations
jonasfranz May 13, 2018
ed695c1
Fix unit tests by updating fixtures and updating outdated test
jonasfranz May 14, 2018
de7081c
Comments will be shown at the right place now
jonasfranz May 14, 2018
7c4bf56
Add support for deleting CodeComments
jonasfranz May 14, 2018
6ae32b2
Fix problems caused by files in subdirectories
jonasfranz May 14, 2018
4ea74e5
Add support for showing code comments of reviews in conversation
jonasfranz May 15, 2018
7c1edf9
Merge branch 'master' into feat/approval
jonasfranz May 15, 2018
8bb5113
Add support for "Show/Hide outdated"
jonasfranz May 15, 2018
5c2171e
Update code.gitea.io/git
jonasfranz May 17, 2018
0f64dad
Merge branch 'master' into feat/approval-new
jonasfranz May 17, 2018
6e55557
Add support for new webhooks
jonasfranz May 17, 2018
05df5a7
Update comparison
jonasfranz May 19, 2018
e5bde14
Resolve conflicts
jonasfranz May 19, 2018
a8dc699
Merge branch 'master' into feat/approval-new
jonasfranz May 19, 2018
8ea8209
Minor UI improvements
lafriks May 19, 2018
a550052
Merge branch 'master' into feat/approval
jonasfranz May 21, 2018
0f88cb8
update code.gitea.io/git
jonasfranz May 21, 2018
e60b3f6
Merge branch 'master' into feat/approval
jonasfranz May 22, 2018
a05d052
Merge branch 'master' into feat/approval
jonasfranz May 23, 2018
27c488e
Merge branch 'master' into feat/approval
jonasfranz May 23, 2018
2b6001b
Fix ui bug reported by @lunny causing wrong position of add button
jonasfranz May 24, 2018
d25df5b
Prepare solving conflicts
jonasfranz May 24, 2018
7592f5b
Merge branch 'master' into feat/approval-new
jonasfranz May 24, 2018
4cb3a60
Show add button only if no comments already exist for the line
jonasfranz May 24, 2018
f64f8e0
Add missing vendor files
jonasfranz May 24, 2018
f07b4e1
Merge branch 'master' into feat/approval
jonasfranz May 24, 2018
4ad72de
Check if reviewer is nil
jonasfranz May 25, 2018
e2f60f4
Merge branch 'master' into feat/approval-new
jonasfranz May 25, 2018
229129d
Merge remote-tracking branch 'lafriks/feat/approval' into feat/approv…
jonasfranz May 25, 2018
c083682
Show forms only to users who are logged in
jonasfranz May 25, 2018
c7dffe6
Revert "Show forms only to users who are logged in"
jonasfranz May 29, 2018
4d0abce
Save patch in comment
jonasfranz May 31, 2018
c79e5a1
Merge branch 'master' into feat/approval-new
jonasfranz May 31, 2018
39fcc99
Add link to comment in code
jonasfranz May 31, 2018
9a0c394
Add reply form to comment list
jonasfranz May 31, 2018
c8c1e70
Add 'Reply' as translatable
jonasfranz May 31, 2018
853ed7d
Merge branch 'master' into feat/approval
jonasfranz May 31, 2018
b6d8aea
gofmt
jonasfranz May 31, 2018
e3f87a9
Merge remote-tracking branch 'lafriks/feat/approval' into feat/approv…
jonasfranz May 31, 2018
3f48c7c
Fix problems introduced by checking for singed in user
jonasfranz May 31, 2018
b4e43d6
Merge branch 'master' of https://github.com/go-gitea/gitea into feat/…
jonasfranz Jul 5, 2018
45dabaf
Add v70
jonasfranz Jul 5, 2018
b553556
Update generated stylesheet
jonasfranz Jul 5, 2018
73b325c
Fix preview
jonasfranz Jul 5, 2018
3cd5ee4
Add new algo to generate diff for line range
jonasfranz Jul 6, 2018
7f0eb69
Merge branch 'master' of https://github.com/go-gitea/gitea into feat/…
jonasfranz Jul 6, 2018
8a84f04
Add documentation and example for CutDiffAroundLine
jonasfranz Jul 6, 2018
fbaeb02
Fix example of CutDiffAroundLine
jonasfranz Jul 6, 2018
5dd39d3
Fix some comment UI rendering bugs
jonasfranz Jul 6, 2018
aee593b
Add code comment edit mode
jonasfranz Jul 6, 2018
8f77329
Send notifications / actions to users until review gets published
jonasfranz Jul 6, 2018
d8ddade
Fix vet errors
jonasfranz Jul 6, 2018
dbc7aee
Send notifications also for single comments
jonasfranz Jul 6, 2018
a0d9afd
Fix some notification bugs, fix link
jonasfranz Jul 6, 2018
b2092fe
Fix: add comment icon is only shown on code lines
jonasfranz Jul 6, 2018
3013c0c
Add lint comment
jonasfranz Jul 6, 2018
858345d
Add unit tests for git diff
jonasfranz Jul 7, 2018
e340181
Merge branch 'master' into feat/approval
jonasfranz Jul 7, 2018
3f39e23
Add more error messages
jonasfranz Jul 9, 2018
6de1f38
Merge remote-tracking branch 'lafriks/feat/approval' into feat/approv…
jonasfranz Jul 9, 2018
65d4318
Merge branch 'master' of https://github.com/go-gitea/gitea into feat/…
jonasfranz Jul 20, 2018
e7b2b61
Regenerated css
jonasfranz Jul 20, 2018
8a6e6dc
fmt
jonasfranz Jul 20, 2018
5554ad2
Regenerated CSS with latest less version
jonasfranz Jul 20, 2018
b29e722
Merge branch 'master' of https://github.com/go-gitea/gitea into feat/…
jonasfranz Jul 21, 2018
cb29fdb
Fix test by updating comment type to new ID
jonasfranz Jul 21, 2018
021f028
Introducing CodeComments as type for map[string]map[int64][]*Comment
jonasfranz Jul 24, 2018
5539c96
Fix data-tab issues
jonasfranz Jul 24, 2018
7eec104
Merge branch 'master' of https://github.com/go-gitea/gitea into feat/…
jonasfranz Jul 24, 2018
ce07867
Remove unnecessary change
jonasfranz Jul 24, 2018
dc4a27d
refactored checkForInvalidation
jonasfranz Jul 24, 2018
4c76cf5
Append comments instead of setting
jonasfranz Jul 24, 2018
77caec7
Use HeadRepo instead of BaseRepo
jonasfranz Jul 25, 2018
f1a3e6f
Merge branch 'master' into feat/approval
jonasfranz Jul 25, 2018
a116913
Merge branch 'master' of https://github.com/go-gitea/gitea into feat/…
jonasfranz Jul 27, 2018
64269ef
Update migration
jonasfranz Jul 27, 2018
7888318
Regenerated CSS
jonasfranz Jul 27, 2018
638ea14
Add copyright
jonasfranz Jul 27, 2018
c503a70
Merge branch 'master' of https://github.com/go-gitea/gitea into feat/…
jonasfranz Aug 5, 2018
5f8c9a2
Update index.css
jonasfranz Aug 5, 2018
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Add support for invalidating comments
Signed-off-by: Jonas Franz <[email protected]>
  • Loading branch information
jonasfranz committed May 13, 2018
commit 7986d6ed22f7b08d1937f7d0a21a304b2db26d30
2 changes: 1 addition & 1 deletion models/git_diff_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -48,7 +48,7 @@ func TestDiff_LoadComments(t *testing.T) {
{
Lines: []*DiffLine{
{
LeftIdx: 4,
LeftIdx: 4,
RightIdx: 4,
},
},
Expand Down
84 changes: 75 additions & 9 deletions models/issue_comment.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,10 @@ package models

import (
"fmt"
"path"
"strings"

"code.gitea.io/git"
"code.gitea.io/gitea/modules/markup/markdown"
"github.com/Unknwon/com"
"github.com/go-xorm/builder"
Expand Down Expand Up @@ -122,8 +124,9 @@ type Comment struct {
// For view issue page.
ShowTag CommentTag `xorm:"-"`

Review *Review `xorm:"-"`
ReviewID int64
Review *Review `xorm:"-"`
ReviewID int64
Invalidated bool
}

// AfterLoad is invoked from XORM after setting the values of all fields of this object.
Expand Down Expand Up @@ -339,6 +342,40 @@ func (c *Comment) LoadReview() error {
return c.loadReview(x)
}

func (c *Comment) getPathAndFile(repoPath string) (string, string) {
p := path.Dir(c.TreePath)
if p == "." {
p = ""
}
p = fmt.Sprintf("%s/%s", repoPath, p)
file := path.Base(c.TreePath)
return p, file
}

func (c *Comment) checkInvalidation(e Engine, repo *git.Repository, branch string) error {
p, file := c.getPathAndFile(repo.Path)
// FIXME differentiate between previous and proposed line
var line = c.Line
if line < 0 {
line *= -1
}
commit, err := repo.LineBlame(branch, p, file, uint(line))
if err != nil {
return err
}
if c.CommitSHA != commit.ID.String() {
c.Invalidated = true
return UpdateComment(c)
}
return nil
}

// CheckInvalidation checks if the line of code comment got changed by another commit.
// If the line got changed the comment is going to be invalidated.
func (c *Comment) CheckInvalidation(repo *git.Repository, branch string) error {
return c.checkInvalidation(x, repo, branch)
}

func createComment(e *xorm.Session, opts *CreateCommentOptions) (_ *Comment, err error) {
var LabelID int64
if opts.Label != nil {
Expand Down Expand Up @@ -622,7 +659,29 @@ func CreateIssueComment(doer *User, repo *Repository, issue *Issue, content stri
}

// CreateCodeComment creates a plain code comment at the specified line / path
func CreateCodeComment(doer *User, repo *Repository, issue *Issue, commitSHA, content, treePath string, line, reviewID int64) (*Comment, error) {
func CreateCodeComment(doer *User, repo *Repository, issue *Issue, content, treePath string, line, reviewID int64) (*Comment, error) {
pr, err := GetPullRequestByIssueID(issue.ID)
if err != nil {
return nil, fmt.Errorf("GetPullRequestByIssueID: %v", err)
}
if err := pr.GetHeadRepo(); err != nil {
return nil, fmt.Errorf("GetHeadRepo: %v", err)
}
gitRepo, err := git.OpenRepository(pr.HeadRepo.RepoPath())
if err != nil {
return nil, fmt.Errorf("OpenRepository: %v", err)
}
dummyComment := &Comment{Line: line, TreePath: treePath}
p, file := dummyComment.getPathAndFile(gitRepo.Path)
// FIXME differentiate between previous and proposed line
var gitLine = line
if gitLine < 0 {
gitLine *= -1
}
commit, err := gitRepo.LineBlame(pr.HeadBranch, p, file, uint(gitLine))
if err != nil {
return nil, err
}
return CreateComment(&CreateCommentOptions{
Type: CommentTypeCode,
Doer: doer,
Expand All @@ -631,7 +690,7 @@ func CreateCodeComment(doer *User, repo *Repository, issue *Issue, commitSHA, co
Content: content,
LineNum: line,
TreePath: treePath,
CommitSHA: commitSHA,
CommitSHA: commit.ID.String(),
ReviewID: reviewID,
})
}
Expand Down Expand Up @@ -792,14 +851,21 @@ func DeleteComment(comment *Comment) error {

func fetchCodeComments(e Engine, issue *Issue, currentUser *User) (map[string]map[int64][]*Comment, error) {
Copy link
Contributor

@cryptix cryptix Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

might be more approachable to make this map a concrete type and explain it with a godoc comment?

I guess its file > line > coments, right?

pathToLineToComment := make(map[string]map[int64][]*Comment)
comments, err := findComments(e, FindCommentsOptions{

//Find comments
opts := FindCommentsOptions{
Type: CommentTypeCode,
IssueID: issue.ID,
})
if err != nil {
}
var comments []*Comment
if err := e.Where(opts.toConds().And(builder.Eq{"invalidated": false})).
Asc("comment.created_unix").
Asc("comment.id").
Find(&comments); err != nil {
return nil, err
}
if err = issue.loadRepo(e); err != nil {

if err := issue.loadRepo(e); err != nil {
return nil, err
}
// Find all reviews by ReviewID
Expand All @@ -810,7 +876,7 @@ func fetchCodeComments(e Engine, issue *Issue, currentUser *User) (map[string]ma
ids = append(ids, comment.ReviewID)
}
}
if err = e.In("id", ids).Find(&reviews); err != nil {
if err := e.In("id", ids).Find(&reviews); err != nil {
return nil, err
}
for _, comment := range comments {
Expand Down
59 changes: 54 additions & 5 deletions models/pull.go
Original file line number Diff line number Diff line change
Expand Up @@ -1063,10 +1063,7 @@ func (prs PullRequestList) loadAttributes(e Engine) error {
}

// Load issues.
issueIDs := make([]int64, 0, len(prs))
for i := range prs {
issueIDs = append(issueIDs, prs[i].IssueID)
}
issueIDs := prs.getIssueIDs()
issues := make([]*Issue, 0, len(issueIDs))
if err := e.
Where("id > 0").
Expand All @@ -1085,11 +1082,44 @@ func (prs PullRequestList) loadAttributes(e Engine) error {
return nil
}

func (prs PullRequestList) getIssueIDs() []int64 {
issueIDs := make([]int64, 0, len(prs))
for i := range prs {
issueIDs = append(issueIDs, prs[i].IssueID)
}
return issueIDs
}

// LoadAttributes load all the prs attributes
func (prs PullRequestList) LoadAttributes() error {
return prs.loadAttributes(x)
}

func (prs PullRequestList) invalidateCodeComments(e Engine, repo *git.Repository, branch string) error {
if len(prs) == 0 {
return nil
}
issueIDs := prs.getIssueIDs()
var codeComments []*Comment
if err := e.
Where("type = ? and invalidated = ?", CommentTypeCode, false).
In("issue_id", issueIDs).
Find(&codeComments); err != nil {
return fmt.Errorf("find code comments: %v", err)
}
for _, comment := range codeComments {
if err := comment.CheckInvalidation(repo, branch); err != nil {
return err
}
}
return nil
}

// InvalidateCodeComments will lookup the prs for code comments which got invalidated by change
func (prs PullRequestList) InvalidateCodeComments(repo *git.Repository, branch string) error {
return prs.invalidateCodeComments(x, repo, branch)
}

func addHeadRepoTasks(prs []*PullRequest) {
for _, pr := range prs {
log.Trace("addHeadRepoTasks[%d]: composing new test task", pr.ID)
Expand All @@ -1116,10 +1146,29 @@ func AddTestPullRequestTask(doer *User, repoID int64, branch string, isSync bool
}

if isSync {
if err = PullRequestList(prs).LoadAttributes(); err != nil {
requests := PullRequestList(prs)
if err = requests.LoadAttributes(); err != nil {
log.Error(4, "PullRequestList.LoadAttributes: %v", err)
}
var gitRepo *git.Repository
repo, err := GetRepositoryByID(repoID)
if err != nil {
log.Error(4, "GetRepositoryByID: %v", err)
goto REQUIRED_PROCEDURE
}
gitRepo, err = git.OpenRepository(repo.RepoPath())
if err != nil {
log.Error(4, "git.OpenRepository: %v", err)
goto REQUIRED_PROCEDURE
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not sure how much goto is used in this codebase but couldn't you put the next goroutine in the } else { of this if err != nil {} ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

goto just jumps to REQUIRED_PROCEDURE and using an else on every err != nil check would result in massive code duplication.

Copy link
Contributor

@cryptix cryptix Jul 24, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I understand your duplication concern. Yet the line after the goto tag checks if err == nil, which it won't be, since that triggered the jump. This entanglement warrants a comment, I think.

If you want to run that code in all three cases, I'd put it in a func var like this and call it in all three places.

var queHooks = func() {
 // iterate, preapare and add to que...
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I re-factored the complete procedure. Is it now better?

}
go func() {
err := requests.InvalidateCodeComments(gitRepo, branch)
if err != nil {
log.Error(4, "PullRequestList.InvalidateCodeComments: %v", err)
}
}()

REQUIRED_PROCEDURE:
if err == nil {
for _, pr := range prs {
pr.Issue.PullRequest = pr
Expand Down
11 changes: 5 additions & 6 deletions modules/auth/repo_form.go
Original file line number Diff line number Diff line change
Expand Up @@ -358,12 +358,11 @@ func (f *MergePullRequestForm) Validate(ctx *macaron.Context, errs binding.Error

// CodeCommentForm form for adding code comments for PRs
type CodeCommentForm struct {
Content string `binding:"Required"`
Side string `binding:"Required;In(previous,proposed)"`
Line int64
TreePath string `form:"path" binding:"Required"`
CommitSHA string `form:"commit_id" binding:"Required"`
IsReview bool `form:"is_review"`
Content string `binding:"Required"`
Side string `binding:"Required;In(previous,proposed)"`
Line int64
TreePath string `form:"path" binding:"Required"`
IsReview bool `form:"is_review"`
}

// Validate validates the fields
Expand Down
5 changes: 1 addition & 4 deletions routers/repo/pull_review.go
Original file line number Diff line number Diff line change
Expand Up @@ -63,14 +63,11 @@ func CreateCodeComment(ctx *context.Context, form auth.CodeCommentForm) {
}
}
}

//FIXME check if line, commit and treepath exist
var err error
comment, err = models.CreateCodeComment(
comment, err := models.CreateCodeComment(
ctx.User,
issue.Repo,
issue,
form.CommitSHA,
form.Content,
form.TreePath,
signedLine,
Expand Down
14 changes: 14 additions & 0 deletions vendor/code.gitea.io/git/repo_blame.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

7 changes: 4 additions & 3 deletions vendor/vendor.json
Original file line number Diff line number Diff line change
Expand Up @@ -3,10 +3,11 @@
"ignore": "test appengine",
"package": [
{
"checksumSHA1": "BfL4Z7P1alyUUNspKJu7Q4GPCNs=",
"checksumSHA1": "jkAY8qJRd3N2isGPpoCMoq+QkBc=",
"origin": "github.com/JonasFranzDEV/git",
"path": "code.gitea.io/git",
"revision": "f1ecc138bebcffed32be1a574ed0c2701b33733f",
"revisionTime": "2018-04-21T01:08:19Z"
"revision": "575c3983fb275c7e87906a781ace9d97e8f4071d",
"revisionTime": "2018-05-13T11:02:42Z"
},
{
"checksumSHA1": "WMD6+Qh2+5hd9uiq910pF/Ihylw=",
Expand Down