Pagination on releases page #2035

iszla · 2017-06-22T18:55:15Z

Reason for why the pagination did not work, was because the paginater was never given a higher count than the amount of releases to display (which is max 10)
Added GetReleaseCountByRepoID function to get count of releases so pagination will work on releases page.
Count also checks if user is owner, so if there are many prereleases and/or drafts the page count should not be off.

Tested by being logged in as owner, and not logged in.

Fixes #1863

…unaffected

lafriks

It will not work correctly anyway as GetReleasesByRepoID will return all releases not depending by condition (draft + owner). If not owner will be looking at release page he will get (limit - draft releases) in first page

iszla · 2017-06-22T19:45:30Z

That is true, and something that I noticed, so if you have 10 draft releases the first page will be empty. Difference here is that you will be able to go to page two by clicking to see releases instead of just seeing an empty screen. Also trying to understand why the tests are failing

tonivj5 · 2017-06-22T19:56:39Z

models/release.go

+		return x.Where("repo_id = ?", repoID).Count(&Release{})
+	}
+
+	return x.Where("repo_id = ? AND is_prerelease = 0 AND is_draft = 0", repoID).Count(&Release{})


I think you should use a Cond struct instead of a string, similar to this and then you can pass it to Where method

Also this probably wont work in postgres as I think xorm creates bool columns in it.

iszla · 2017-06-22T20:33:49Z

Switched to cond struct, and also removed checking if it was a prerelease as this seemed to be incorrect since you can see prereleases even if you are not the owner, as long as they are not drafts

ethantkoenig · 2017-06-22T20:57:16Z

models/release.go

@@ -244,6 +245,19 @@ func GetReleasesByRepoID(repoID int64, page, pageSize int) (rels []*Release, err
 	return rels, err
 }

+// GetReleaseCountByRepoID returns the count of releases of repository
+func GetReleaseCountByRepoID(repoID int64, isOwner bool) (total int64, err error) {


I think a variable name like includeDrafts would be more clear than isOwner.

Are return variable names necessary here? They don't seem to ever be used.

just (int64, error) would be better

ethantkoenig · 2017-06-22T21:01:07Z

routers/repo/release.go

@@ -67,7 +67,13 @@ func Releases(ctx *context.Context) {

 	releases, err := models.GetReleasesByRepoID(ctx.Repo.Repository.ID, page, limit)


We should also make sure that if we're not the owner, then this doesn't return drafts. I know that we check in the for loop below, but right now it's possible that we end up displaying fewer than limit releases on the first page, even if there are more than limit displayable releases in total.

EDIT: Oh, looks like @lafriks already pointed this out 😂

ethantkoenig

One more thing

ethantkoenig · 2017-06-22T21:05:08Z

models/release.go

+		return x.Where(cond).Count(&Release{})
+	}
+
+	cond = cond.And(builder.Eq{"is_draft": 0})


I think this should be false instead of 0.

lafriks · 2017-06-22T21:09:54Z

models/release.go

 	}

-	return x.Where("repo_id = ? AND is_prerelease = 0 AND is_draft = 0", repoID).Count(&Release{})
+	cond = cond.And(builder.Eq{"is_draft": 0})


It must be false instead of 0

Bwko · 2017-06-27T17:41:21Z

LGTM

lunny · 2017-06-28T01:21:04Z

@iszla need force push to let CI work.

lafriks · 2017-06-28T09:22:02Z

LGTM I will fix GetReleasesByRepoID in other PR so #1863 can't be closed just yet

lafriks · 2017-06-28T09:24:32Z

@ethantkoenig need your approval

ethantkoenig · 2017-06-28T13:42:48Z

@lafriks Okay, as long as GetReleasesByRepoID eventually gets fixed I'll approve

lafriks · 2017-06-28T13:44:48Z

@ethantkoenig thats on my task list for today when I get home ;)

iszla added 8 commits June 20, 2017 21:58

Added count to GetReleasesByRepoID so pagination will work

3cbc3e2

Separated it out to a new function, can then also leave the API part …

2046b2a

…unaffected

Remove extra whitespace added in untouched function

686ce04

Added comment and corrected name in error handler

58e90fb

Account for if the user is owner or not in the count

3d6fa22

Also check if repo is draft

db54407

revert back to the correct count in the ReleasesToDisplay loop

608579e

Fixed lint error regarding else with return statement

1f627f7

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

lafriks requested changes Jun 22, 2017

View reviewed changes

tonivj5 reviewed Jun 22, 2017

View reviewed changes

iszla mentioned this pull request Jun 22, 2017

Corrected wrong function name in error #2036

Closed

Use Cond struct instead of string, corrected name in error handler

cd8fc18

ethantkoenig suggested changes Jun 22, 2017

View reviewed changes

lafriks requested changes Jun 22, 2017

View reviewed changes

Removed unused return variable names

9fbeb76

lunny added this to the 1.2.0 milestone Jun 23, 2017

lunny added the type/bug label Jun 23, 2017

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 Jun 27, 2017

lafriks approved these changes Jun 28, 2017

View reviewed changes

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

ethantkoenig approved these changes Jun 28, 2017

View reviewed changes

lunny merged commit 3f90164 into go-gitea:master Jun 28, 2017

lafriks mentioned this pull request Jun 28, 2017

Fix release display and correct paging #2080

Merged

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Pagination on releases page #2035

Pagination on releases page #2035

iszla commented Jun 22, 2017

lafriks left a comment

iszla commented Jun 22, 2017

tonivj5 Jun 22, 2017

lafriks Jun 22, 2017

iszla commented Jun 22, 2017

ethantkoenig Jun 22, 2017

lafriks Jun 22, 2017

ethantkoenig Jun 22, 2017 •

edited

Loading

ethantkoenig left a comment

ethantkoenig Jun 22, 2017

lafriks Jun 22, 2017

Bwko commented Jun 27, 2017

lunny commented Jun 28, 2017

lafriks commented Jun 28, 2017 •

edited

Loading

lafriks commented Jun 28, 2017

ethantkoenig commented Jun 28, 2017 •

edited

Loading

lafriks commented Jun 28, 2017

		@@ -67,7 +67,13 @@ func Releases(ctx *context.Context) {

		releases, err := models.GetReleasesByRepoID(ctx.Repo.Repository.ID, page, limit)

Pagination on releases page #2035

Pagination on releases page #2035

Conversation

iszla commented Jun 22, 2017

lafriks left a comment

Choose a reason for hiding this comment

iszla commented Jun 22, 2017

tonivj5 Jun 22, 2017

Choose a reason for hiding this comment

lafriks Jun 22, 2017

Choose a reason for hiding this comment

iszla commented Jun 22, 2017

ethantkoenig Jun 22, 2017

Choose a reason for hiding this comment

lafriks Jun 22, 2017

Choose a reason for hiding this comment

ethantkoenig Jun 22, 2017 • edited Loading

Choose a reason for hiding this comment

ethantkoenig left a comment

Choose a reason for hiding this comment

ethantkoenig Jun 22, 2017

Choose a reason for hiding this comment

lafriks Jun 22, 2017

Choose a reason for hiding this comment

Bwko commented Jun 27, 2017

lunny commented Jun 28, 2017

lafriks commented Jun 28, 2017 • edited Loading

lafriks commented Jun 28, 2017

ethantkoenig commented Jun 28, 2017 • edited Loading

lafriks commented Jun 28, 2017

ethantkoenig Jun 22, 2017 •

edited

Loading

lafriks commented Jun 28, 2017 •

edited

Loading

ethantkoenig commented Jun 28, 2017 •

edited

Loading