Improvements of releases list and tags list #25859

Zettat123 · 2023-07-13T02:46:19Z

This PR introduces the following improvements:

We do not need to call GetTags to get tags because tags have been loaded by RepoAssignment

Lines 663 to 668 in ef90fdb

    
           tags, err := repo_model.GetTagNamesByRepoID(ctx, ctx.Repo.Repository.ID) 
        
           if err != nil { 
        
           	ctx.ServerError("GetTagNamesByRepoID", err) 
        
           	return cancel 
        
           } 
        
           ctx.Data["Tags"] = tags

Similarly, the number of tags and releases also have been loaded by RepoAssignment, so the related code has been removed from the handlers. The query condition of GetReleaseCountByRepoID in RepoAssignment has been changed to include draft releases.

gitea/modules/context/repo.go

Lines 538 to 551 in ef90fdb

    
           ctx.Data["NumTags"], err = repo_model.GetReleaseCountByRepoID(ctx, ctx.Repo.Repository.ID, repo_model.FindReleasesOptions{ 
        
           	IncludeDrafts: true, 
        
           	IncludeTags:   true, 
        
           	HasSha1:       util.OptionalBoolTrue, // only draft releases which are created with existing tags 
        
           }) 
        
           if err != nil { 
        
           	ctx.ServerError("GetReleaseCountByRepoID", err) 
        
           	return nil 
        
           } 
        
           ctx.Data["NumReleases"], err = repo_model.GetReleaseCountByRepoID(ctx, ctx.Repo.Repository.ID, repo_model.FindReleasesOptions{}) 
        
           if err != nil { 
        
           	ctx.ServerError("GetReleaseCountByRepoID", err) 
        
           	return nil 
        
           }

releasesOrTags function has been removed. The code for rendering releases list and tags list moved to Releases and TagList respectively.

wxiaoguang · 2023-08-23T01:51:13Z

routers/web/repo/release_test.go

+	var err error
+	ctx.Data["NumReleases"], err = repo_model.GetReleaseCountByRepoID(ctx, ctx.Repo.Repository.ID, repo_model.FindReleasesOptions{
+		IncludeDrafts: ctx.Repo.CanWrite(unit_model.TypeReleases),
+	})
+	assert.NoError(t, err)


Is there an assertion for ctx.Data["NumReleases"]?

Hmm .... I think the test code is not ideal. Calling Releases directly makes the middlewares never executed.

If it doesn't test NumReleases logic, it's better to remove this code block.

Sorry for replying so late. I agree with your opinion that the test code is not ideal. Right now this unit test cannot work without the RepoAssignment middleware. I tried moving it to integration tests, then I found there was already a TestViewReleaseListNoLogin which tests the release list.

gitea/tests/integration/release_test.go

Lines 134 to 174 in 5600504

func TestViewReleaseListNoLogin(t *testing.T) {

defer tests.PrepareTestEnv(t)()

repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 57, OwnerName: "user2", LowerName: "repo-release"})

link := repo.Link() + "/releases"

req := NewRequest(t, "GET", link)

rsp := MakeRequest(t, req, http.StatusOK)

htmlDoc := NewHTMLParser(t, rsp.Body)

releases := htmlDoc.Find("#release-list li.ui.grid")

assert.Equal(t, 5, releases.Length())

links := make([]string, 0, 5)

commitsToMain := make([]string, 0, 5)

releases.Each(func(i int, s *goquery.Selection) {

link, exist := s.Find(".release-list-title a").Attr("href")

if !exist {

return

}

links = append(links, link)

commitsToMain = append(commitsToMain, s.Find(".ahead > a").Text())

})

assert.EqualValues(t, []string{

"/user2/repo-release/releases/tag/empty-target-branch",

"/user2/repo-release/releases/tag/non-existing-target-branch",

"/user2/repo-release/releases/tag/v2.0",

"/user2/repo-release/releases/tag/v1.1",

"/user2/repo-release/releases/tag/v1.0",

}, links)

assert.EqualValues(t, []string{

"1 commits", // like v1.1

"1 commits", // like v1.1

"0 commits",

"1 commits", // should be 3 commits ahead and 2 commits behind, but not implemented yet

"3 commits",

}, commitsToMain)

}

Maybe we can remove TestNewReleasesList from unit tests?

wxiaoguang

Why TestNewReleasesList is removed? Could it be kept?

Zettat123 · 2023-09-28T04:09:37Z

Why TestNewReleasesList is removed? Could it be kept?

Because ctx.Data["NumReleases"] is required in TestNewReleasesList. However, NumReleases is assigned in the RepoAssignment middleware and this test calls the handler(Release) directly without using the middleware. So TestNewReleasesList cannot work now.

In addition, as my previous comment said, I think the TestViewReleaseListNoLogin integration test is able to test the logic of the handler.

wxiaoguang · 2023-09-28T04:16:42Z

The test code and TargetBehind was introduced by #24470

If I understand correctly, only the removed code tested the TargetBehind ?

Zettat123 · 2023-09-28T06:02:13Z

The test code and TargetBehind was introduced by #24470

If I understand correctly, only the removed code tested the TargetBehind ?

To test TargetBehind, I added TestNewReleasesList back and renamed it to TestCalReleaseNumCommitsBehind in a8050b7

This fixes a regression from #25859 If a tag has no Release, Gitea will show a Link to create a Release for the Tag if the User has the Permission to do this, but the variable to indicate that is no longer set. Used here: https://github.com/go-gitea/gitea/blob/1bfcdeef4cca0f5509476358e5931c13d37ed1ca/templates/repo/tag/list.tmpl#L39-L41

This fixes a regression from go-gitea#25859 If a tag has no Release, Gitea will show a Link to create a Release for the Tag if the User has the Permission to do this, but the variable to indicate that is no longer set. Used here: https://github.com/go-gitea/gitea/blob/1bfcdeef4cca0f5509476358e5931c13d37ed1ca/templates/repo/tag/list.tmpl#L39-L41

Zettat123 added 2 commits July 12, 2023 18:21

NumReleases include drafts if CanWrite releases

d5d06b0

remove releasesOrTags

86890ad

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jul 13, 2023

pull-request-size bot added the size/L Denotes a PR that changes 100-499 lines, ignoring generated files. label Jul 13, 2023

Zettat123 added the type/enhancement An improvement of existing functionality label Jul 13, 2023

fix test

59e0ddd

Zettat123 marked this pull request as draft July 13, 2023 04:53

Zettat123 marked this pull request as ready for review July 27, 2023 01:04

lunny added this to the 1.21.0 milestone Aug 22, 2023

lunny requested review from lunny, wolfogre and delvh August 22, 2023 06:04

wolfogre approved these changes Aug 23, 2023

View reviewed changes

GiteaBot 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 Aug 23, 2023

wxiaoguang reviewed Aug 23, 2023

View reviewed changes

lunny modified the milestones: 1.21.0, 1.22.0 Sep 21, 2023

Zettat123 added 3 commits September 22, 2023 10:43

Merge branch 'main' into improve-tags-and-releases

a1229db

Merge branch 'main' into improve-tags-and-releases

99f3ec2

remove TestNewReleasesList

6ead82c

Zettat123 requested a review from wxiaoguang September 28, 2023 03:26

wxiaoguang reviewed Sep 28, 2023

View reviewed changes

add TestCalReleaseNumCommitsBehind

a8050b7

wxiaoguang approved these changes Sep 28, 2023

View reviewed changes

GiteaBot 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 Sep 28, 2023

lunny added the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 28, 2023

lunny enabled auto-merge (squash) September 28, 2023 12:56

Merge branch 'main' into improve-tags-and-releases

24f25e9

lunny merged commit 3fcad58 into go-gitea:main Sep 28, 2023
25 checks passed

GiteaBot removed the reviewed/wait-merge This pull request is part of the merge queue. It will be merged soon. label Sep 28, 2023

JakobDev mentioned this pull request Nov 30, 2023

Add missing variable in tag list #28305

Merged

go-gitea locked as resolved and limited conversation to collaborators Dec 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improvements of releases list and tags list #25859

Improvements of releases list and tags list #25859

Zettat123 commented Jul 13, 2023 •

edited

Loading

wxiaoguang Aug 23, 2023

wxiaoguang Aug 23, 2023 •

edited

Loading

Zettat123 Sep 27, 2023

wxiaoguang left a comment

Zettat123 commented Sep 28, 2023

wxiaoguang commented Sep 28, 2023

Zettat123 commented Sep 28, 2023

	tags, err := repo_model.GetTagNamesByRepoID(ctx, ctx.Repo.Repository.ID)
	if err != nil {
	ctx.ServerError("GetTagNamesByRepoID", err)
	return cancel
	}
	ctx.Data["Tags"] = tags

	ctx.Data["NumTags"], err = repo_model.GetReleaseCountByRepoID(ctx, ctx.Repo.Repository.ID, repo_model.FindReleasesOptions{
	IncludeDrafts: true,
	IncludeTags: true,
	HasSha1: util.OptionalBoolTrue, // only draft releases which are created with existing tags
	})
	if err != nil {
	ctx.ServerError("GetReleaseCountByRepoID", err)
	return nil
	}
	ctx.Data["NumReleases"], err = repo_model.GetReleaseCountByRepoID(ctx, ctx.Repo.Repository.ID, repo_model.FindReleasesOptions{})
	if err != nil {
	ctx.ServerError("GetReleaseCountByRepoID", err)
	return nil
	}

	func TestViewReleaseListNoLogin(t *testing.T) {
	defer tests.PrepareTestEnv(t)()

	repo := unittest.AssertExistsAndLoadBean(t, &repo_model.Repository{ID: 57, OwnerName: "user2", LowerName: "repo-release"})

	link := repo.Link() + "/releases"

	req := NewRequest(t, "GET", link)
	rsp := MakeRequest(t, req, http.StatusOK)

	htmlDoc := NewHTMLParser(t, rsp.Body)
	releases := htmlDoc.Find("#release-list li.ui.grid")
	assert.Equal(t, 5, releases.Length())

	links := make([]string, 0, 5)
	commitsToMain := make([]string, 0, 5)
	releases.Each(func(i int, s *goquery.Selection) {
	link, exist := s.Find(".release-list-title a").Attr("href")
	if !exist {
	return
	}
	links = append(links, link)

	commitsToMain = append(commitsToMain, s.Find(".ahead > a").Text())
	})

	assert.EqualValues(t, []string{
	"/user2/repo-release/releases/tag/empty-target-branch",
	"/user2/repo-release/releases/tag/non-existing-target-branch",
	"/user2/repo-release/releases/tag/v2.0",
	"/user2/repo-release/releases/tag/v1.1",
	"/user2/repo-release/releases/tag/v1.0",
	}, links)
	assert.EqualValues(t, []string{
	"1 commits", // like v1.1
	"1 commits", // like v1.1
	"0 commits",
	"1 commits", // should be 3 commits ahead and 2 commits behind, but not implemented yet
	"3 commits",
	}, commitsToMain)
	}

Improvements of releases list and tags list #25859

Improvements of releases list and tags list #25859

Conversation

Zettat123 commented Jul 13, 2023 • edited Loading

wxiaoguang Aug 23, 2023

Choose a reason for hiding this comment

wxiaoguang Aug 23, 2023 • edited Loading

Choose a reason for hiding this comment

Zettat123 Sep 27, 2023

Choose a reason for hiding this comment

wxiaoguang left a comment

Choose a reason for hiding this comment

Zettat123 commented Sep 28, 2023

wxiaoguang commented Sep 28, 2023

Zettat123 commented Sep 28, 2023

Zettat123 commented Jul 13, 2023 •

edited

Loading

wxiaoguang Aug 23, 2023 •

edited

Loading