Show commit status icon in commits table #1688
Conversation
lunny
added
the
type/feature
|
|
tboerger
added
the
lgtm/need 2
|
Fixed also bug in d066d05 that it was failing render commits table when creating PR with signed commits |
|
Great enough and LGTM. Maybe add an integration test for the commit table page is better! |
|
Let L-G-T-M work |
tboerger
added
lgtm/need 1
|
Added integration tests for commit table and all commit statuses |
|
In master getting latest status using API does not work except for sqlite. Fixed this here in latest commit |
integrations/repo_commits_test.go
Outdated
| "path" | ||
| "testing" | ||
|
|
||
| //"fmt" |
models/status.go
Outdated
| fullStatuses := make([]*CommitStatus, 0, len(statuses)) | ||
| for _, status := range statuses { | ||
| cs := make([]*CommitStatus, 0, 1) | ||
| err = x.Where("repo_id = ?", repo.ID).And("sha = ?", sha). |
There was a problem hiding this comment.
Because there is no way to get newest row grouped by context in one select that would work across all supported databases
There was a problem hiding this comment.
I suggest
var ids []int64
err := x.Limit(10, page*10).
Where("repo_id = ?", repo.ID).And("sha = ?", sha).
Select("id").
GroupBy("context").OrderBy("max(created_unix) desc").Find(&ids)
if len(ids) > 0 {
x.In("id", ids).Find(&statuses)
}There was a problem hiding this comment.
You can't select columns that are outside of group by expressions
|
Testing with harness/harness#2017 PR. It is working for me.
|
|
Ok, I have rewritten latest commit status selects. Only drawback that it does not selects it by latest created_at column but by largest id. But that is sacrifice that can be taken for performance reasons |
|
LGTM |
|
make LGTM work |
tboerger
added
lgtm/done
|
The next step maybe PR's commit status or PR's status? |
|
I think PR commit table is same as repository commit table, so there already should be commit status. Next step would probably be PR status. Is it just PR latest commits status? |
|
Yes. I think PR status == PR latest commit status |
|
Now harness/harness#2017 has been merged, is the PR's status the next step? |
This would be the first step to display status icons in commit table
Screenshot: