Skip to content
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

Show commit status icon in commits table #1688

Merged
merged 8 commits into from
May 7, 2017

Conversation

lafriks
Copy link
Member

@lafriks lafriks commented May 6, 2017

This would be the first step to display status icons in commit table

Screenshot:

attels

@lunny lunny added this to the 1.2.0 milestone May 6, 2017
@lunny lunny added the type/feature Completely new functionality. Can only be merged if feature freeze is not active. label May 6, 2017
@lunny
Copy link
Member

lunny commented May 6, 2017

TargetURL could be a link opened on a new tab. And it seems CommitStatusState need one more value CommitStatusNone if there is no status for a commit.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 6, 2017
@lunny
Copy link
Member

lunny commented May 6, 2017

@bkcsoft

@lafriks
Copy link
Member Author

lafriks commented May 6, 2017

Fixed also bug in d066d05 that it was failing render commits table when creating PR with signed commits

@lunny
Copy link
Member

lunny commented May 6, 2017

Great enough and LGTM. Maybe add an integration test for the commit table page is better!

@lunny
Copy link
Member

lunny commented May 6, 2017

Let L-G-T-M work

@tboerger tboerger 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 May 6, 2017
@lafriks
Copy link
Member Author

lafriks commented May 6, 2017

Added integration tests for commit table and all commit statuses

@lafriks
Copy link
Member Author

lafriks commented May 6, 2017

In master getting latest status using API does not work except for sqlite. Fixed this here in latest commit

"path"
"testing"

//"fmt"
Copy link
Member

Choose a reason for hiding this comment

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

unused comment

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).
Copy link
Member

Choose a reason for hiding this comment

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

Why need query it twice?

Copy link
Member Author

Choose a reason for hiding this comment

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

Because there is no way to get newest row grouped by context in one select that would work across all supported databases

Copy link
Member

Choose a reason for hiding this comment

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

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)
}

Copy link
Member Author

Choose a reason for hiding this comment

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

You can't select columns that are outside of group by expressions

@appleboy
Copy link
Member

appleboy commented May 7, 2017

Testing with harness/harness#2017 PR. It is working for me.

screen shot 2017-05-07 at 10 54 28 am

@lafriks
Copy link
Member Author

lafriks commented May 7, 2017

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

@appleboy
Copy link
Member

appleboy commented May 7, 2017

LGTM

@appleboy
Copy link
Member

appleboy commented May 7, 2017

make LGTM work

@tboerger tboerger added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels May 7, 2017
@appleboy appleboy merged commit 7949404 into go-gitea:master May 7, 2017
@lafriks lafriks deleted the status_display_commit_log branch May 7, 2017 16:41
@lunny
Copy link
Member

lunny commented May 8, 2017

The next step maybe PR's commit status or PR's status?

@lafriks
Copy link
Member Author

lafriks commented May 8, 2017

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?

@lunny
Copy link
Member

lunny commented May 8, 2017

Yes. I think PR status == PR latest commit status

@tonivj5
Copy link
Contributor

tonivj5 commented May 24, 2017

Now harness/harness#2017 has been merged, is the PR's status the next step?

@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/feature Completely new functionality. Can only be merged if feature freeze is not active.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants