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

Fix release display and correct paging #2080

Merged
merged 1 commit into from
Jun 29, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
93 changes: 93 additions & 0 deletions integrations/release_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,12 +5,60 @@
package integrations

import (
"fmt"
"net/http"
"testing"

"github.com/Unknwon/i18n"
"github.com/stretchr/testify/assert"
)

func createNewRelease(t *testing.T, session *TestSession, repoURL, tag, title string, preRelease, draft bool) {
req := NewRequest(t, "GET", repoURL+"/releases/new")
resp := session.MakeRequest(t, req)
assert.EqualValues(t, http.StatusOK, resp.HeaderCode)
htmlDoc := NewHTMLParser(t, resp.Body)

link, exists := htmlDoc.doc.Find("form").Attr("action")
assert.True(t, exists, "The template has changed")

postData := map[string]string{
"_csrf": htmlDoc.GetCSRF(),
"tag_name": tag,
"tag_target": "master",
"title": title,
"content": "",
}
if preRelease {
postData["prerelease"] = "on"
}
if draft {
postData["draft"] = "Save Draft"
}
req = NewRequestWithValues(t, "POST", link, postData)

resp = session.MakeRequest(t, req)
assert.EqualValues(t, http.StatusFound, resp.HeaderCode)

redirectedURL := resp.Headers["Location"]
assert.NotEmpty(t, redirectedURL, "Redirected URL is not found")
}

func checkLatestReleaseAndCount(t *testing.T, session *TestSession, repoURL, version, label string, count int) {
req := NewRequest(t, "GET", repoURL+"/releases")
resp := session.MakeRequest(t, req)
assert.EqualValues(t, http.StatusOK, resp.HeaderCode)

htmlDoc := NewHTMLParser(t, resp.Body)
labelText := htmlDoc.doc.Find("#release-list > li .meta .label").First().Text()
assert.EqualValues(t, label, labelText)
titleText := htmlDoc.doc.Find("#release-list > li .detail h3 a").First().Text()
assert.EqualValues(t, version, titleText)

releaseList := htmlDoc.doc.Find("#release-list > li")
assert.EqualValues(t, count, releaseList.Length())
}

func TestViewReleases(t *testing.T) {
prepareTestEnv(t)

Expand All @@ -27,3 +75,48 @@ func TestViewReleasesNoLogin(t *testing.T) {
resp := MakeRequest(req)
assert.EqualValues(t, http.StatusOK, resp.HeaderCode)
}

func TestCreateRelease(t *testing.T) {
prepareTestEnv(t)

session := loginUser(t, "user2")
createNewRelease(t, session, "/user2/repo1", "v0.0.1", "v0.0.1", false, false)

checkLatestReleaseAndCount(t, session, "/user2/repo1", "v0.0.1", i18n.Tr("en", "repo.release.stable"), 1)
}

func TestCreateReleasePreRelease(t *testing.T) {
prepareTestEnv(t)

session := loginUser(t, "user2")
createNewRelease(t, session, "/user2/repo1", "v0.0.1", "v0.0.1", true, false)

checkLatestReleaseAndCount(t, session, "/user2/repo1", "v0.0.1", i18n.Tr("en", "repo.release.prerelease"), 1)
}

func TestCreateReleaseDraft(t *testing.T) {
prepareTestEnv(t)

session := loginUser(t, "user2")
createNewRelease(t, session, "/user2/repo1", "v0.0.1", "v0.0.1", false, true)

checkLatestReleaseAndCount(t, session, "/user2/repo1", "v0.0.1", i18n.Tr("en", "repo.release.draft"), 1)
}

func TestCreateReleasePaging(t *testing.T) {
prepareTestEnv(t)

session := loginUser(t, "user2")
// Create enaugh releases to have paging
for i := 0; i < 12; i++ {
version := fmt.Sprintf("v0.0.%d", i)
createNewRelease(t, session, "/user2/repo1", version, version, false, false)
}
createNewRelease(t, session, "/user2/repo1", "v0.0.12", "v0.0.12", false, true)

checkLatestReleaseAndCount(t, session, "/user2/repo1", "v0.0.12", i18n.Tr("en", "repo.release.draft"), 10)

// Check that user3 does not see draft and still see 10 latest releases
session2 := loginUser(t, "user3")
checkLatestReleaseAndCount(t, session2, "/user2/repo1", "v0.0.11", i18n.Tr("en", "repo.release.stable"), 10)
}
1 change: 1 addition & 0 deletions models/fixtures/release.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[] # empty
48 changes: 26 additions & 22 deletions models/release.go
Original file line number Diff line number Diff line change
Expand Up @@ -233,40 +233,44 @@ func GetReleaseByID(id int64) (*Release, error) {
return rel, nil
}

// GetReleasesByRepoID returns a list of releases of repository.
func GetReleasesByRepoID(repoID int64, page, pageSize int) (rels []*Release, err error) {
if page <= 0 {
page = 1
}
err = x.
Desc("created_unix").
Limit(pageSize, (page-1)*pageSize).
Find(&rels, Release{RepoID: repoID})
return rels, err
// FindReleasesOptions describes the conditions to Find releases
type FindReleasesOptions struct {
IncludeDrafts bool
TagNames []string
}

// GetReleaseCountByRepoID returns the count of releases of repository
func GetReleaseCountByRepoID(repoID int64, includeDrafts bool) (int64, error) {
func (opts *FindReleasesOptions) toConds(repoID int64) builder.Cond {
Copy link
Member

Choose a reason for hiding this comment

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

Why not let repoID as a field of FindReleasesOptions?

Copy link
Member Author

Choose a reason for hiding this comment

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

At first I did that but than it looked strange as func name is GetReleasesByRepoID. And also repoID is mandatory condition

Copy link
Member

Choose a reason for hiding this comment

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

I think FindReleases and CountReleases maybe a new name?

Copy link
Member Author

Choose a reason for hiding this comment

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

But repoID is still allways required field and that will not easy be understandable by function signature

Copy link
Member

Choose a reason for hiding this comment

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

seems right.

var cond = builder.NewCond()
cond = cond.And(builder.Eq{"repo_id": repoID})

if includeDrafts {
return x.Where(cond).Count(&Release{})
if !opts.IncludeDrafts {
cond = cond.And(builder.Eq{"is_draft": false})
}

cond = cond.And(builder.Eq{"is_draft": false})
return x.Where(cond).Count(&Release{})
if len(opts.TagNames) > 0 {
cond = cond.And(builder.In("tag_name", opts.TagNames))
}
return cond
}

// GetReleasesByRepoIDAndNames returns a list of releases of repository according repoID and tagNames.
func GetReleasesByRepoIDAndNames(repoID int64, tagNames []string) (rels []*Release, err error) {
// GetReleasesByRepoID returns a list of releases of repository.
func GetReleasesByRepoID(repoID int64, opts FindReleasesOptions, page, pageSize int) (rels []*Release, err error) {
Copy link
Member

Choose a reason for hiding this comment

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

As above and page and pageSize is also could be fields of FindReleasesOptions

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 it's not really related to conditions and toConds can't use them anyway

if page <= 0 {
page = 1
}

err = x.
Desc("created_unix").
In("tag_name", tagNames).
Find(&rels, Release{RepoID: repoID})
Desc("created_unix", "id").
Limit(pageSize, (page-1)*pageSize).
Where(opts.toConds(repoID)).
Find(&rels)
return rels, err
}

// GetReleaseCountByRepoID returns the count of releases of repository
func GetReleaseCountByRepoID(repoID int64, opts FindReleasesOptions) (int64, error) {
return x.Where(opts.toConds(repoID)).Count(&Release{})
}

type releaseMetaSearch struct {
ID []int64
Rel []*Release
Expand Down
17 changes: 8 additions & 9 deletions routers/api/v1/repo/release.go
Original file line number Diff line number Diff line change
Expand Up @@ -34,22 +34,21 @@ func GetRelease(ctx *context.APIContext) {

// ListReleases list a repository's releases
func ListReleases(ctx *context.APIContext) {
releases, err := models.GetReleasesByRepoID(ctx.Repo.Repository.ID, 1, 2147483647)
access, err := models.AccessLevel(ctx.User.ID, ctx.Repo.Repository)
if err != nil {
ctx.Error(500, "GetReleasesByRepoID", err)
ctx.Error(500, "AccessLevel", err)
return
}
rels := make([]*api.Release, len(releases))
access, err := models.AccessLevel(ctx.User.ID, ctx.Repo.Repository)

releases, err := models.GetReleasesByRepoID(ctx.Repo.Repository.ID, models.FindReleasesOptions{
IncludeDrafts: access >= models.AccessModeWrite,
}, 1, 2147483647)
if err != nil {
ctx.Error(500, "AccessLevel", err)
ctx.Error(500, "GetReleasesByRepoID", err)
return
}
rels := make([]*api.Release, len(releases))
for i, release := range releases {
if release.IsDraft && access < models.AccessModeWrite {
// hide drafts from users without push access
continue
}
if err := release.LoadAttributes(); err != nil {
ctx.Error(500, "LoadAttributes", err)
return
Expand Down
15 changes: 7 additions & 8 deletions routers/repo/release.go
Original file line number Diff line number Diff line change
Expand Up @@ -65,13 +65,17 @@ func Releases(ctx *context.Context) {
limit = 10
}

releases, err := models.GetReleasesByRepoID(ctx.Repo.Repository.ID, page, limit)
opts := models.FindReleasesOptions{
IncludeDrafts: ctx.Repo.IsWriter(),
}

releases, err := models.GetReleasesByRepoID(ctx.Repo.Repository.ID, opts, page, limit)
if err != nil {
ctx.Handle(500, "GetReleasesByRepoID", err)
return
}

count, err := models.GetReleaseCountByRepoID(ctx.Repo.Repository.ID, ctx.Repo.IsOwner())
count, err := models.GetReleaseCountByRepoID(ctx.Repo.Repository.ID, opts)
if err != nil {
ctx.Handle(500, "GetReleaseCountByRepoID", err)
return
Expand All @@ -91,11 +95,7 @@ func Releases(ctx *context.Context) {
}
var ok bool

releasesToDisplay := make([]*models.Release, 0, len(releases))
for _, r := range releases {
if r.IsDraft && !ctx.Repo.IsOwner() {
continue
}
if r.Publisher, ok = cacheUsers[r.PublisherID]; !ok {
r.Publisher, err = models.GetUserByID(r.PublisherID)
if err != nil {
Expand All @@ -113,12 +113,11 @@ func Releases(ctx *context.Context) {
return
}
r.Note = markdown.RenderString(r.Note, ctx.Repo.RepoLink, ctx.Repo.Repository.ComposeMetas())
releasesToDisplay = append(releasesToDisplay, r)
}

pager := paginater.New(int(count), limit, page, 5)
ctx.Data["Page"] = pager
ctx.Data["Releases"] = releasesToDisplay
ctx.Data["Releases"] = releases
ctx.HTML(200, tplReleases)
}

Expand Down