Add commit status on repo and user pull request lists #2519

lunny · 2017-09-15T08:53:01Z

And something confusing me is should I use issue.PullRequest.MergeBase as the last commit of a pull request? Anyway, see the screenshots below:

lafriks · 2017-09-15T08:55:40Z

Integration test would be nice for this

lunny · 2017-09-15T08:58:22Z

@lafriks It seems some difficult since our integration tests didn't run a drone instance?

codecov-io · 2017-09-15T09:09:51Z

Codecov Report

Merging #2519 into master will increase coverage by 6.06%.
The diff coverage is 4.49%.

@@            Coverage Diff             @@
##           master    #2519      +/-   ##
==========================================
+ Coverage   20.62%   26.68%   +6.06%     
==========================================
  Files         166       90      -76     
  Lines       32261    17893   -14368     
==========================================
- Hits         6654     4775    -1879     
+ Misses      24628    12438   -12190     
+ Partials      979      680     -299

Impacted Files	Coverage Δ
models/pull.go	`20.38% <ø> (+3.64%)`	⬆️
models/status.go	`2.81% <0%> (+1.08%)`	⬆️
routers/user/home.go	`20.93% <7.69%> (-4%)`	⬇️
models/repo_issue.go	`0% <0%> (-52.64%)`	⬇️
models/wiki.go	`10.76% <0%> (-51.74%)`	⬇️
models/graph.go	`0% <0%> (-39.66%)`	⬇️
models/repo_unit.go	`8% <0%> (-29.94%)`	⬇️
modules/markup/html.go	`51.43% <0%> (-28.47%)`	⬇️
models/git_diff.go	`6.74% <0%> (-21.3%)`	⬇️
models/issue_indexer.go	`7.79% <0%> (-10.6%)`	⬇️
... and 133 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 00bd627...09ec95b. Read the comment docs.

lafriks · 2017-09-15T12:23:39Z

@lunny you can just call status api to set status for commit you dont need real CI

Morlinest · 2017-09-15T14:27:13Z

routers/user/home.go

+
+		if issue.IsPull {
+			issue.LoadAttributes()
+			statuses, err := models.GetLatestCommitStatus(issue.Repo, issue.PullRequest.MergeBase, 0)


Can you extract this block to new function? I've found this (almost) same code block 3 times in existing codebase.

I think they are different for ... range.

Maybe we should have a issue.GetLatestCommitStatus that does what this if-block does?

so instead of

if issue.IsPull { ... append(...) }

it would be

issueStatuses = append(issueStatuses, models.CalcCommitStatus(issue.GetLatestCommitStatus()))

It's same, the only difference is you are adding result of models.CalcCommitStatus(statuses) to slice. Instead of calling 2 functions and 1 err check, you can call just 1 function (err check can be inside too, it changes nothing). Best thing would be to rename GetLatestCommitStatus to GetCommitStatuses (as it returns array, not just 1 status) and create/use GetLatestCommitStatus to return just one status (for now, this few lines of code can be used, but I want to change it to pure SQL select).

ok, GetCommitStatuses is here also, I don't understand GetLatestCommitStatus implementation... As I understand, it should return only 1 status in slice.

bkcsoft · 2017-09-15T14:56:50Z

routers/user/home.go

+
+		if issue.IsPull {
+			issue.LoadAttributes()
+			statuses, err := models.GetLatestCommitStatus(issue.Repo, issue.PullRequest.MergeBase, 0)


Maybe we should have a issue.GetLatestCommitStatus that does what this if-block does?

bkcsoft · 2017-09-15T14:58:24Z

routers/user/home.go

+
+		if issue.IsPull {
+			issue.LoadAttributes()
+			statuses, err := models.GetLatestCommitStatus(issue.Repo, issue.PullRequest.MergeBase, 0)


so instead of

if issue.IsPull { ... append(...) }

it would be

issueStatuses = append(issueStatuses, models.CalcCommitStatus(issue.GetLatestCommitStatus()))

bkcsoft · 2017-09-15T15:12:16Z

templates/repo/issue/list.tmpl

+					<div class="ui {{if $issue.IsRead}}black{{else}}green{{end}} label">#{{$issue.Index}}</div>
+					<a class="title has-emoji" href="{{$.Link}}/{{$issue.Index}}">{{$issue.Title}}</a>
+					{{if $issue.IsPull}}
+					{{template "repo/commit_status" (index $.IssuesStates $index)}}


indentation

lunny · 2017-09-15T15:32:28Z

In fact I prefer to implement GetLatestCommitStatusByIDs

bkcsoft · 2017-09-15T15:59:26Z

@lunny ByIDs ? What ID?

lunny · 2017-09-16T00:17:16Z

@bkcsoft GetLatestCommitStatusByRepoIDsAndCommitIDs(repoIDs, commitIDs)

ethantkoenig · 2017-09-16T02:56:09Z

routers/repo/issue.go

 		// Check read status
 		if !ctx.IsSigned {
 			issues[i].IsRead = true
 		} else if err = issues[i].GetIsRead(ctx.User.ID); err != nil {
 			ctx.Handle(500, "GetIsRead", err)
 			return
 		}
+
+		if issue.IsPull {
+			issue.LoadAttributes()


Let's not ignore the error returned here

ethantkoenig · 2017-09-16T02:58:27Z

routers/user/home.go

 	for _, issue := range issues {
 		issue.Repo = showReposMap[issue.RepoID]
+
+		if issue.IsPull {
+			issue.LoadAttributes()


Let's not ignore the error returned here

bkcsoft · 2017-09-16T16:24:57Z

@lunny That function name is waaaay to long 😂 I say that (repoID, commitID) is enough documentation to just call it GetLatestCommitStatuses ;)

Morlinest · 2017-09-17T10:28:45Z

routers/repo/issue.go

@@ -207,17 +207,44 @@ func Issues(ctx *context.Context) {
 		}
 	}

+	var repoIDs = make([]int64, 0, len(issues))
+	var shas = make([]string, 0, len(issues))
+	var pullIDs = make([]int64, 0, len(issues))


Can you use 1 slice of structs instead?

type pull struct{ repoID int64 sha string pullID int64 } var pulls = make([]pull , 0, len(issues)) ... pulls = append(pulls, pull{ctx.Repo.Repository.ID, issue.PullRequest.MergeBase, issue.ID})

Morlinest · 2017-09-17T10:59:18Z

routers/repo/issue.go

 		// Check read status
 		if !ctx.IsSigned {
 			issues[i].IsRead = true
 		} else if err = issues[i].GetIsRead(ctx.User.ID); err != nil {
 			ctx.Handle(500, "GetIsRead", err)
 			return
 		}
+
+		if issue.IsPull {
+			if err := issue.LoadAttributes(); err != nil {


I think you don't need to run issue.LoadAttributes() again, models.Issues() has already did it.

Morlinest · 2017-09-17T11:14:02Z

models/status.go

+	var results = make([]struct {
+		ID     int64
+		RepoID int64
+	}, 0, 10*len(repoIDs))


Why 10*len(repoIDs)?

Morlinest · 2017-09-17T11:41:15Z

routers/repo/issue.go

+	var issuesStates = make(map[int64]*models.CommitStatus, len(issues))
+	for i, statuses := range commitStatuses {
+		issuesStates[pullIDs[i]] = models.CalcCommitStatus(statuses)
+	}


Is there way to reduce all of this lines above to just 1 function call? I am thinking about doing just 1 sql select to get what you need. There are too many for loops and also nested loop.

I think we can add something like "StateLevel" or "StateCode" to CommitStatus table (there are only strings - CommitStatus.State- right now). Then we can do simple select to get "worst" status. For "latests" status there is CommitStatus.Index or max CommitStatus.ID.

Morlinest · 2017-09-17T11:51:42Z

routers/user/home.go

@@ -307,10 +307,37 @@ func Issues(ctx *context.Context) {
 		return
 	}

+	var repoIDs = make([]int64, 0, len(issues))
+	var shas = make([]string, 0, len(issues))
+	var pullIDs = make([]int64, 0, len(issues))


Same comment as for "routers/repo/issue.go".

Morlinest · 2017-09-17T12:39:06Z

models/status.go

+	err := x.Table(&CommitStatus{}).
+		Where(cond).
+		Select("max( id ) as id, repo_id").
+		GroupBy("repo_id, sha, context").OrderBy("max( id ) desc").Find(&results)


Is not this select everything you need to get latest commit for each repo commit (I was just writing about adding columns to "group by" when you updated it)? You can use this as subselect (just remove repo_id from select) for mainy "where" clause.

lunny · 2017-09-18T14:03:48Z

@lafriks integration tests added

lunny · 2017-09-18T15:01:37Z

@Morlinest I think the best way is add a field LastCommitID on PullRequest and fill in it when push commit to the headbranch. So I will do it on another PR and keep this one simple.

Morlinest · 2017-09-18T14:14:53Z

routers/repo/issue.go

+	} else {
+		var repoIDs = make([]int64, 0, len(issues))
+		var shas = make([]string, 0, len(issues))
+		var pullIDs = make([]int64, 0, len(issues))


You can use just 1 slice for repoIDs + shas and 1 for pullIDs (see comment for GetLatestCommitStatuses function). Or you can use composition and create just 1 slice.

// can be used as arg for GetLatestCommitStatuses type CommitOption struct { RepoID int64 Sha string } type results struct { CommitOption pullID int64 }

Yes, you are right. But I think store the last commit id in pullrequest table is the best way. I will send another PR to do that.

If you do, call it RepoSHA instead of CommitOption since that better explains the purpose of the struct. Though in general I dislike FunctionOptions structs since they hide the arguments from the docs.

@bkcsoft But 2 slices is not solution to that as you can't be sure about the order nor dependency between values on same key. I don't think that you are hiding something with structs in go.

Morlinest · 2017-09-18T14:24:25Z

models/status.go

@@ -170,6 +172,60 @@ func GetLatestCommitStatus(repo *Repository, sha string, page int) ([]*CommitSta
 	return statuses, x.In("id", ids).Find(&statuses)
 }

+// GetLatestCommitStatuses returns all statuses with given repoIDs and shas
+func GetLatestCommitStatuses(repoIDs []int64, shas []string) ([][]*CommitStatus, error) {


Instead of 2 slices it should be 1 slice of structs with 2 values. You can't be sure, both slices are in same order and values on concrete index belongs each other.

Morlinest · 2017-09-18T14:33:27Z

models/status.go

+	var repoIDsMap = make(map[string][]int64, len(repoIDs))
+	for _, res := range results {
+		ids = append(ids, res.ID)
+		repoIDsMap[fmt.Sprintf("%d-%s", res.RepoID, res.SHA)] = append(repoIDsMap[fmt.Sprintf("%d-%s", res.RepoID, res.SHA)], res.ID)


Save fmt.Sprintf("%d-%s", res.RepoID, res.SHA) to variable and use it as key.

Morlinest · 2017-09-18T14:42:42Z

models/status.go

+	var repoIDsMap = make(map[string][]int64, len(repoIDs))
+	for _, res := range results {
+		ids = append(ids, res.ID)
+		repoIDsMap[fmt.Sprintf("%d-%s", res.RepoID, res.SHA)] = append(repoIDsMap[fmt.Sprintf("%d-%s", res.RepoID, res.SHA)], res.ID)


This will overwrite value of repoIDsMap if you have 2 different context for same commit (res.RepoID and res.SHA is same). What is the point here?

Morlinest · 2017-09-18T14:59:51Z

routers/user/home.go

+		var repoIDs = make([]int64, 0, len(issues))
+		var shas = make([]string, 0, len(issues))
+		var pullIDs = make([]int64, 0, len(issues))
+		var repoCache = make(map[int64]*git.Repository)


Same comment as for "routers/repo/issue.go". Also looks like duplicated code (one if/else check less).

Morlinest · 2017-09-18T18:16:54Z

@lunny OK, I'll wait for new PR and updated version because I have more comments to this PR :)

lunny · 2017-09-19T04:23:21Z

@Morlinest ~~I will update this after #2548 merged.~~

ethantkoenig · 2017-09-27T03:52:34Z

integrations/editor_test.go

+
+	req = NewRequestWithValues(t, "POST", url,
+		map[string]string{
+			"_csrf": htmlDoc.GetCSRF(),


nit: you can use GetCSRF(..), so you don't have to manually make GET request just for the csrf token.

ethantkoenig · 2017-09-27T03:59:18Z

templates/repo/commit_status.tmpl

-{{end}}
-{{if eq .State "warning"}}
-    <a href="{{.TargetURL}}" target=_blank><i class="commit-status warning sign icon yellow"></i></a>
+{{if .}}


This file should use tabs instead of spaces

ethantkoenig · 2017-09-27T04:02:08Z

routers/user/home.go

+
+			var rep *git.Repository
+			var ok bool
+			if rep, ok = repoCache[issue.PullRequest.HeadRepoID]; !ok {


nit: could we call the variable gitRepo, or something more descriptive? Without context, it's not obvious (at least not to me) that rep stands for repo.

ethantkoenig · 2017-09-27T04:04:20Z

templates/repo/issue/list.tmpl

@@ -162,36 +162,38 @@
 		</div>

 		<div class="issue list">
-			{{range .Issues}}
-				{{ $timeStr:= TimeSince .Created $.Lang }}
+			{{range $index, $issue := .Issues}}


Is $index ever used? Similarly for the other template.

Seems no needed after my second push.

lunny · 2017-10-28T15:36:04Z

will resolve #996

…b operations

lunny · 2019-03-25T12:53:19Z

@SijmenSchoon if you want to work on this, just take it and create another PR. I think this PR needs to fix tests. I'm now busy on other PRs.

stale · 2019-05-24T13:35:01Z

This pull request has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs during the next 2 months. Thank you for your contributions.

lunny · 2019-05-24T14:22:46Z

Since #6465 merged, I will close this one.

lunny added type/enhancement An improvement of existing functionality topic/ui Change the appearance of the Gitea UI labels Sep 15, 2017

lunny added this to the 1.3.0 milestone Sep 15, 2017

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

Morlinest suggested changes Sep 15, 2017

View reviewed changes

bkcsoft reviewed Sep 15, 2017

View reviewed changes

ethantkoenig suggested changes Sep 16, 2017

View reviewed changes

lunny force-pushed the lunny/add_commit_status_pr branch from d9892f8 to f285da9 Compare September 17, 2017 04:37

Morlinest suggested changes Sep 17, 2017

View reviewed changes

lunny force-pushed the lunny/add_commit_status_pr branch from 75f273b to 9b7d379 Compare September 18, 2017 14:02

Morlinest suggested changes Sep 18, 2017

View reviewed changes

lunny mentioned this pull request Sep 23, 2017

Add commit comment when push to pull request branch #2548

Closed

ethantkoenig suggested changes Sep 27, 2017

View reviewed changes

MorphBonehunter mentioned this pull request Oct 28, 2017

Better CI Integration #996

Closed

4 tasks

lunny modified the milestones: 1.3.0, 1.4.0 Nov 10, 2017

lunny force-pushed the lunny/add_commit_status_pr branch from 9b7d379 to 03649d9 Compare November 26, 2017 05:25

lunny added 13 commits January 6, 2019 10:14

add commit status on repo and user pull request lists

deb88a4

use GetLatestCommitStatuses instead GetLatestCommitStatus to reduce d…

f3a43b4

…b operations

fix GetLatestCommitStatuses

efe0220

add tests for pulls commit status and some fixes

3e0a87c

refactor GetLatestCommitStatuses and some other improvements

4824f72

remove unused variable on template

15e49d6

remove removed column on tests fixtures

8f54636

fix tests

53c4f6f

fix tests

bc4470a

improve GetIssuesLatestCommitStatuses

b2ba574

remove trace

008c6bc

fix tests

809f6d8

fix template variables

207fccd

lunny force-pushed the lunny/add_commit_status_pr branch from 315fc0c to 207fccd Compare January 6, 2019 02:31

Merge branch 'master' into lunny/add_commit_status_pr

09aac6f

techknowlogick modified the milestones: 1.8.0, 1.x.x Feb 12, 2019

lafriks modified the milestones: 1.x.x, 1.9.0 Mar 25, 2019

This was referenced Mar 28, 2019

Show last commit status in pull request lists #6459

Closed

Show last commit status in pull request lists #6465

Merged

LukeOwlclaw mentioned this pull request May 23, 2019

Question: Does Gitea have CI Build Status Area? #2912

Closed

stale bot added the issue/stale label May 24, 2019

lunny closed this May 24, 2019

lunny removed this from the 1.9.0 milestone Sep 18, 2019

lunny deleted the lunny/add_commit_status_pr branch September 18, 2019 06:00

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

Add commit status on repo and user pull request lists #2519

Add commit status on repo and user pull request lists #2519

Conversation

lunny commented Sep 15, 2017

lafriks commented Sep 15, 2017

lunny commented Sep 15, 2017

codecov-io commented Sep 15, 2017 • edited Loading

Codecov Report

lafriks commented Sep 15, 2017

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 Sep 15, 2017

bkcsoft commented Sep 15, 2017

lunny commented Sep 16, 2017

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

bkcsoft commented Sep 16, 2017

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 Sep 18, 2017

lunny commented Sep 18, 2017

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

Morlinest commented Sep 18, 2017

lunny commented Sep 19, 2017 • edited Loading

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 Oct 28, 2017

lunny commented Mar 25, 2019

stale bot commented May 24, 2019

lunny commented May 24, 2019

codecov-io commented Sep 15, 2017 •

edited

Loading

lunny commented Sep 19, 2017 •

edited

Loading