PR#1810: Fix odd pagination error - pagure

		`@@ -436,7 +436,7 @@`
		`flask.g.repo_admin = is_repo_admin(flask.g.repo)`
		`flask.g.branches = sorted(flask.g.repo_obj.listall_branches())`

		`- items_per_page = 100`
		`+ items_per_page = APP.config['ITEM_PER_PAGE']`
		`flask.g.offset = 0`
		`flask.g.page = 1`
		`flask.g.limit = items_per_page`

pagure/templates/issues.html

file modified

+1 -1

		`@@ -16,7 +16,7 @@`
		`{% elif status\|lower not in ['open', 'true', 'all', 'none'] %}`
		`{{ issues\|count }} Closed Issues (of {{ issues_cnt }})`
		`{% else %}`
		`- {{ issues\|count }} Issues`
		`+ {{ issues\|count }} Issues (of {{ issues_cnt }})`
		`{% endif %}`
		`<span class="btn-group btn-group-sm pull-xs-right" role="group">`
		`{% if repo.milestones %}`

pagure/ui/issues.py

file modified

+8 -4

		`@@ -669,9 +669,9 @@`
		`author=author,`
		`private=private,`
		`priority=priority,`
		`- count=True,`
		`search_pattern=search_pattern,`
		`custom_search=custom_search,`
		`+ count=True,`
		`)`
		`oth_issues_cnt = total_issues_cnt - issues_cnt`
		`else:`
		`@@ -682,12 +682,16 @@`
		`search_pattern=search_pattern,`
		`custom_search=custom_search,`
		`)`
		`- issues_cnt = total_issues_cnt`
		`-`
		`+ issues_cnt = pagure.lib.search_issues(`
		`+ SESSION, repo, tags=tags, assignee=assignee,`
		`+ author=author, private=private, priority=priority,`
		`+ search_pattern=search_pattern,`
		`+ custom_search=custom_search,`
		`+ count=True,`
		`+ )`
		`tag_list = pagure.lib.get_tags_of_project(SESSION, repo)`

		`total_page = int(ceil(issues_cnt / float(flask.g.limit)) if issues_cnt > 0 else 1)`
		`-`
		`return flask.render_template(`
		`'issues.html',`
		`select='issues',`

tests/test_pagure_flask_ui_issues.py

file modified

+22 -1

		`@@ -37,7 +37,7 @@`
		`""" Tests for flask issues controller of pagure """`

		`def setUp(self):`
		`- """ Set up the environnment, ran before every tests. """`
		`+ """ Set up the environnment, run before every tests. """`
		`super(PagureFlaskIssuestests, self).setUp()`

		`pagure.APP.config['TESTING'] = True`
		`@@ -353,11 +353,32 @@`

		`# All tickets`
		`output = self.app.get('/test/issues?status=all')`
		`+`
		`self.assertEqual(output.status_code, 200)`
		`self.assertIn('<title>Issues - test - Pagure</title>', output.data)`
		`self.assertTrue(`
		`'<h2>\n 2 Issues' in output.data)`

		`+ # All tickets - different pagination`
		`+ before = pagure.APP.config['ITEM_PER_PAGE']`
		`+ pagure.APP.config['ITEM_PER_PAGE'] = 1`
		`+ output = self.app.get('/test/issues?status=all')`
		`+ self.assertEqual(output.status_code, 200)`
		`+ self.assertIn('<title>Issues - test - Pagure</title>', output.data)`
		`+ self.assertIn('<h2>\n 1 Issues (of 2)', output.data)`
		`+ self.assertIn(`
		`+ '<li class="active">page 1 of 2</li>', output.data)`
		`+`
		`+ # All tickets - filtered for 1 - checking the pagination`
		`+ output = self.app.get(`
		`+ '/test/issues?status=all&search_pattern=invalid')`
		`+ self.assertEqual(output.status_code, 200)`
		`+ self.assertIn('<title>Issues - test - Pagure</title>', output.data)`
		`+ self.assertIn('<h2>\n 1 Issues (of 1)', output.data)`
		`+ self.assertIn(`
		`+ '<li class="active">page 1 of 1</li>', output.data)`
		`+ pagure.APP.config['ITEM_PER_PAGE'] = before`
		`+`
		`# New issue button is shown`
		`user = tests.FakeUser()`
		`with tests.user_set(pagure.APP, user):`

vibhcool commented 7 years ago

Fixes: #1790 Odd pagination on issue search

vibhcool commented 7 years ago

Tested and open for review :)

1 new commit added

fix pep8 error

7 years ago

vibhcool commented on line 19 of pagure/ui/issues.py 7 years ago

pagure.lib.search_issue() can return either a issue object, list of issues or number of issues. It can also be modified to return only list of issues whether there is 1 issue or more than 1 . Number of issues can be returned using len() method. This way shall be faster and simpler, isn't it?

pingou commented on line 19 of pagure/ui/issues.py 7 years ago

How would, retrieving potentially a large list of items from the database and then going over it to know its size be faster than retrieving a single integer from the database? :)

pingou commented on line 8 of pagure/ui/issues.py 7 years ago

That indentation shouldn't be changed actually, pep8 won't like it :)

pingou commented 7 years ago

I'd like to test this, but from a quick look it looks good :)

cep commented on line 19 of pagure/ui/issues.py 7 years ago

IMHO it might, in fact, be easier to do issues_cnt = len(issues) here since we have already performed the DB query for getting the issues.

The same holds true for similar instances of issues_cnt above.

Is that correct?

cep commented 7 years ago

Excpet the one comment, rest looks good to me. Well done! :thumbsup:

1 new commit added

adjust indentation

7 years ago

vibhcool commented 7 years ago

made the changes, up for review :)

vibhcool commented 7 years ago

@pingou retrieving single integer from database shall be faster but in the function, pagure.ui.issues.view_issue() method, to get issues and to get no. of issues for issues and issues_cnt, oth_issues and oth_issues_cnt , total_issues and total_issues_cnt , pagure.lib.search_issue() is used for them all. Instead issues_cnt and oth_issues_cnt can be found using len(issues) and len(oth_issues) respectivly.

vibhcool commented 7 years ago