First implementation of git appraise #733

sokolovstas · 2017-01-23T14:29:12Z

Issue #189

This is first implementation of appraise review. I opened gitea code only this morning and to end of day I completed showing review list.

Dear maintainers please see the code and tell me if I started implementation properly. I can continue to implement this tomorrow.

Issue go-gitea#189

lunny

Almost it's great!

lunny · 2017-01-23T14:51:16Z

routers/repo/reviews.go

@@ -0,0 +1,103 @@
+// Copyright 2014 The Gogs Authors. All rights reserved.


2017 The Gitea

lunny · 2017-01-23T14:53:10Z

routers/repo/reviews.go

+}
+
+// Review struct to Issue representation
+type Review struct {


Why not based on Issue like Pull, so that we can list all issues/pulls/reviews of one milestone.

I tried but I need that index field will be string, because appraise doesn't have unique in index

index's unique could be changed to index I think.

Yes but it is int. In appraise that all we have is string
Revision: b9f1a3d0a1081a5f159ed90a3d413320890e0986

Added IsReview flag on Issue

So you can ignore index field and add revision in Reivew struct.

lunny · 2017-01-23T14:53:35Z

routers/repo/reviews.go

+	UpdatedUnix int64
+	IsClosed    bool
+	IsRead      bool
+	IsPull      bool


IsPull needed?

Yes this may be removed, thank you

ethantkoenig · 2017-01-23T15:09:22Z

routers/repo/reviews.go

+	"strings"
+	"time"
+
+	"github.com/google/git-appraise/repository"


Local imports (e.g. code.gitea.io/gitea/foo/bar) should come before non-stdlib external packages (e.g. github.com/foo/bar).

import ( "strconv" "strings" "time" "code.gitea.io/gitea/models" "code.gitea.io/gitea/modules/base" "code.gitea.io/gitea/modules/context" "github.com/google/git-appraise/repository" "github.com/google/git-appraise/review" )

Reworked template for using IsReview

makhov · 2017-01-23T20:50:57Z

routers/repo/reviews.go

+
+	ctx.Data["Title"] = ctx.Tr("repo.reviews")
+	ctx.Data["PageIsReviewList"] = true
+	// ctx.Data["Page"] = 0


Is it really needed to add the commented code to repo?

I upload PR to other contributors and maintainers check my code for right implementation to exist code base. I will clear all commented lines on last commit.

lunny · 2017-01-25T03:42:09Z

Maybe needs to insert or update one record to issue table when a review request pushed to git?

Add commits list Add comments list Add GetIndex to models.Issue to return Revision/Index

ojarjur · 2017-01-25T21:08:35Z

routers/repo/reviews.go

+	ctx.Data["Username"] = ctx.Repo.Owner.Name
+	ctx.Data["Reponame"] = ctx.Repo.Repository.Name
+
+	reviewCommits, err := repo.ListCommitsBetween(reviewDetails.Request.TargetRef, reviewDetails.Request.ReviewRef)


The logic for getting the commits in a review is a little more complicated than this because there are multiple states a review can be in (open, submitted, abandoned, etc).

I have a PR to git-appraise to make this a first-class part of the review API: google/git-appraise#70

@ojarjur thank you, I change usage of this function as soon as it was merged

@sokolovstas That change has been merged into git-appraise now.

strk · 2017-01-26T13:15:37Z

How can I test this ? I've a repository in which git appraise list shows there is a pending review for commit 09d5b03a69f3, which I've pushed to gitea-hosted git repo with git push gitea-remote --all. Am I supposed to see the comments inline now, on the commit page ? Because I don't see it

rikvdh · 2017-01-26T18:37:39Z

I've just tested this out and it is a good start. Since we don't have line-annotations (#124) (like github does) perhaps just display line/file of a comment when it is present. Also rejection/acceptation is nice to have. Now it is just depicted as a regular comment.

Good work already, good to see the appraise comments in gitea already!

strk · 2017-01-28T09:51:23Z

@rikvdh as I could not see those comments instead, could you upload a screenshot ?

ojarjur · 2017-01-28T17:50:24Z

@strk, running 'git push --all' only pushes the branches, not the notes. To push just the notes with review information, run 'git appraise push gitea-remote' To mirror the entire repo, run 'git push --mirror'

rikvdh · 2017-01-29T18:32:04Z

Here are some screenshots including the latest commit.

The request uses the same template as an issue/pr, the review-request should probably get its own template and have <name> requested review <time ago> instead of commented, also all edit and delete buttons don't work and should be removed.

All comments seem to get a empty code-snippet at the top, also issues and pull-request comments. When there is no line-file information the code-snippet box should be removed i guess.

When having a file/line reference perhaps add the name of the file, it is missing now and on big-reviews this can become very confusing.

The close and comment on the bottom seem to affect my pull-request 1 which is the only one in my test-repo.

Also abandoned reviews (git appraise abandon) should probably close the review, which are now unclose-able. The mutations like accept, reject and abandon are not clear.

Sorry for being so critical, I would really love this feature in Gitea, having a full reviewing system integrated is golden! 👍 Keep up the good work!

bkcsoft · 2017-01-29T21:34:15Z

How would this work if a read-only user wants to use git-appraise instead of ?

Can they still push their review? Or can only users with write-access do that?
If they can, how do you handle authorization? Adding that kind of complexity (checking for notes/dev-refs) sounds like inviting security issues 🙁

Note: I'm very much against implementing something that only some people can use, if we go with git-appraise it should still work just like using the WebUI. In other words, if you can review in the WebUI you can also use git-appraise

My 2 cents 😉

bkcsoft · 2017-01-29T22:48:10Z

<name> requested review <time ago> instead of commented

I would say <name> reviewed <timeAgo> 🙂

ojarjur · 2017-01-30T23:40:19Z

@bkcsoft

How would this work if a read-only user wants to use git-appraise instead of ?

I assume you mean how does this work if someone who does not have write access to a repo wants to use the git-appraise command line tool rather than the gitea UI.

The answer is that user would be able to pull reviews from the repository, but not push them to it. They could always push the review to their fork, but not to the upstream repository.

The git-appraise command-line tool uses the same auth mechanisms as git itself, so a user who cannot push directly to the repo also cannot directly push reviews.

One of the reasons for this design is that tools like gitea already define a way for outside contributors to authenticate and participate in a review, so git-appraise doesn't try to invent its own. Instead, whatever tool is responsible for auth (e.g. gitea) is also responsible for writing the review metadata to the repository.

So what I would expect is that the outside contributor creates their reviews and comments on them via the gitea UI, and the gitea server then writes them to the git-appraise formats.

if we go with git-appraise it should still work just like using the WebUI. In other words, if you can review in the WebUI you can also use git-appraise

The important thing about git-appraise isn't the command-line tool but rather the storage formats.

Even if no one performs code reviews via the command line, having the review metadata stored directly in git is valuable.

strk · 2017-01-31T11:12:59Z

@ojarjur ok now I see the "Reviews" tab (I just didn't notice before).
Should these "Reviews" be associated to commits/pr rather than having their own tab ?

roblabla · 2017-02-02T12:10:26Z

@ojarjur would it not be possible to accept push to the git appraise reference by anyone that has read access to the repo ?

lunny · 2017-02-02T14:58:26Z

conflicted

ojarjur · 2017-02-02T18:38:18Z

would it not be possible to accept push to the git appraise reference by anyone that has read access to the repo

@roblabla It should be possible to achieve what you want, but I would recommend a different implementation than that.

Reviews in git-appraise are just git objects the same way that commits are, and there is a well-established way for read-only contributors to push their own git objects. Specifically, they create a fork of the repository and then push to that fork. Those objects can then be mirrored into the upstream repository in a controlled way (e.g. like GitHub's 'refs/pull/...' refs).

The earlier question from @bkcsoft caused me to start thinking that we could build in a corresponding workflow for git-appraise, and I've created an issue for that here. Please add comments to that issue if the workflow described there doesn't match what you are looking for.

strk · 2017-03-12T13:39:15Z

I'd love to see this completed too @sokolovstas -- any chance ?
Or anyone else would like to continue where he left ?

lunny · 2017-03-20T14:43:19Z

@sokolovstas if you have no time for this, maybe you can allow maintainers or others to continue the work so that you can check Allow edits from maintainers. on this page right bar bottom.

sokolovstas · 2017-03-20T14:45:34Z

@lunny This check box is already checked. For now I have very busy on main project and don't have free time. But will be very glad if someone can continue this work!

strk · 2017-03-20T16:15:57Z

@lunny are you going to give this a try ?

lunny · 2017-03-21T00:53:41Z

Maybe but I have to finish my PR first.

Rushmead · 2017-04-01T13:35:27Z

I've read through the comments and would love to help out with this and get it ready to be merged. Could someone make a list of exact things that need to be changed / added?

strk · 2017-04-01T13:45:23Z

Stuart I think you'll need to have an active role in this as there's no great consensus about what the design should be. Personally I agree about the UI using git repository itself for storage, rather than database (as it makes federation much easier). I guess your best first step would be using the proper UnitType and rebasing / fixing conflicts.

KangoV · 2017-05-19T11:14:31Z

Any movement on this one? I don't know GO, but I could help with testing and feedback.

lunny · 2017-05-19T14:56:50Z

@KangoV it's not finished and no one continue with this now.

lafriks · 2017-05-19T18:00:28Z

I could probably work on this as I need it myself :)

lunny · 2017-05-20T03:48:17Z

@lafriks great news, please go ahead.

strk · 2017-05-28T04:53:46Z

@lafriks you are very welcome !

strk · 2017-07-13T22:05:31Z

This PR is in the wrong milestone, should be 1.3.0 (or 1.4.0, depending on @lafriks estimate)

strk · 2017-07-13T22:07:00Z

please disregard my previous comment, 1.x.x is clearly undefined but in the 1.x branch...

lafriks · 2017-07-14T01:19:21Z

@strk It's on my todo list just after more advanced branch protection and PR approval system :)

jonasfranz · 2017-08-31T19:35:04Z

@lafriks Is theire any further news about this PR or is it replaced by another PR-approval system in future?

lafriks · 2017-08-31T20:43:11Z

I started to work on this but I did not like it how it is currently implemented. I think it should be rewritten to be integrated with PRs

lunny · 2017-09-01T00:42:45Z

@lafriks I think rewrite is a better idea.

techknowlogick · 2018-10-29T02:38:44Z

Closing this due to PR approval/comment/review system in place via other PR.

First implementation of git appraise

5f6fc3d

Issue go-gitea#189

sokolovstas force-pushed the appraise branch from 3f7375c to 5f6fc3d Compare January 23, 2017 14:35

lunny added this to the 1.1.0 milestone Jan 23, 2017

lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label Jan 23, 2017

lunny requested changes Jan 23, 2017

View reviewed changes

ethantkoenig suggested changes Jan 23, 2017

View reviewed changes

sokolovstas added 2 commits January 23, 2017 21:44

Fixed review notes

be138a1

Added IsReview flag and Revision string field to models.Issue

f6191d7

Reworked template for using IsReview

makhov reviewed Jan 23, 2017

View reviewed changes

lunny mentioned this pull request Jan 24, 2017

Add units concept for modulable functions of a repository #742

Merged

tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 25, 2017

sokolovstas added 2 commits January 26, 2017 00:30

Add files diff

99847b5

Add commits list Add comments list Add GetIndex to models.Issue to return Revision/Index

Fix active highlight on review files list

dd9dc93

ojarjur reviewed Jan 25, 2017

View reviewed changes

strk mentioned this pull request Jan 26, 2017

Commits and pull files #124

Closed

2 tasks

lunny approved these changes Jan 27, 2017

View reviewed changes

Added showing code lines in comments

48f08ab

lunny modified the milestones: 1.x.x, 1.2.0 Apr 19, 2017

monnier mentioned this pull request Jun 12, 2017

Offline access to Issues #1953

Open

7 tasks

strk mentioned this pull request Sep 6, 2017

Code review tool git-appraise intergration #189

Closed

sluedecke mentioned this pull request Oct 4, 2017

Code review tool git-appraise intergration gogs/gogs#2210

Open

sapk mentioned this pull request Apr 3, 2018

Pull request review/approval and comment on code #3748

Merged

23 tasks

techknowlogick closed this Oct 29, 2018

bkcsoft added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Oct 29, 2018

lunny removed this from the 1.x.x milestone Oct 29, 2018

go-gitea locked and limited conversation to collaborators Nov 24, 2020

		@@ -0,0 +1,103 @@
		// Copyright 2014 The Gogs Authors. All rights reserved.

First implementation of git appraise #733

First implementation of git appraise #733

Conversation

sokolovstas commented Jan 23, 2017

lunny left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lunny commented Jan 25, 2017

ojarjur Jan 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

strk commented Jan 26, 2017

rikvdh commented Jan 26, 2017

strk commented Jan 28, 2017

ojarjur commented Jan 28, 2017 via email • edited Loading

rikvdh commented Jan 29, 2017

bkcsoft commented Jan 29, 2017

bkcsoft commented Jan 29, 2017

ojarjur commented Jan 30, 2017

strk commented Jan 31, 2017

roblabla commented Feb 2, 2017

lunny commented Feb 2, 2017

ojarjur commented Feb 2, 2017

strk commented Mar 12, 2017

lunny commented Mar 20, 2017

sokolovstas commented Mar 20, 2017

strk commented Mar 20, 2017 via email

lunny commented Mar 21, 2017

Rushmead commented Apr 1, 2017

strk commented Apr 1, 2017 via email

KangoV commented May 19, 2017

lunny commented May 19, 2017

lafriks commented May 19, 2017

lunny commented May 20, 2017

strk commented May 28, 2017

strk commented Jul 13, 2017

strk commented Jul 13, 2017

lafriks commented Jul 14, 2017

jonasfranz commented Aug 31, 2017

lafriks commented Aug 31, 2017

lunny commented Sep 1, 2017

techknowlogick commented Oct 29, 2018

ojarjur Jan 25, 2017 •

edited

Loading

ojarjur commented Jan 28, 2017 via email •

edited

Loading