#2827 web: don't use count(*) on first tasks page
Merged 2 years ago by tkopecek. Opened 2 years ago by tkopecek.
tkopecek/koji issue2482  into  master

file modified
+2 -1
@@ -612,7 +612,8 @@ 

      values['order'] = order

  

      tasks = kojiweb.util.paginateMethod(server, values, 'listTasks', kw={'opts': opts},

-                                         start=start, dataName='tasks', prefix='task', order=order)

+                                         start=start, dataName='tasks', prefix='task',

+                                         order=order, first_page_count=False)

  

      if view == 'tree':

          server.multicall = True

file modified
+10
@@ -105,6 +105,7 @@ 

      </tr>

      <tr>

        <td class="paginate" colspan="6">

+         #if $taskPages is not None

          #if $len($taskPages) > 1

          <form class="pageJump" action="">

            Page:
@@ -124,6 +125,10 @@ 

          #if $taskStart + $taskCount < $totalTasks

          <a href="tasks?start=#echo $taskStart + $taskRange#$util.passthrough_except($self)">&gt;&gt;&gt;</a>

          #end if

+         #else

+         <strong>Tasks #echo $taskStart + 1 # through #echo $taskStart + $taskCount#</strong>

+         <a href="tasks?start=$taskRange$util.passthrough_except($self)">&gt;&gt;&gt;</a>

+         #end if

        </td>

      </tr>

      <tr class="list-header">
@@ -164,6 +169,7 @@ 

      #end if

      <tr>

        <td class="paginate" colspan="6">

+         #if $taskPages is not None

          #if $len($taskPages) > 1

          <form class="pageJump" action="">

            Page:
@@ -183,6 +189,10 @@ 

          #if $taskStart + $taskCount < $totalTasks

          <a href="tasks?start=#echo $taskStart + $taskRange#$util.passthrough_except($self)">&gt;&gt;&gt;</a>

          #end if

+         #else

+         <strong>Tasks #echo $taskStart + 1 # through #echo $taskStart + $taskCount#</strong>

+         <a href="tasks?start=$taskRange$util.passthrough_except($self)">&gt;&gt;&gt;</a>

+         #end if

        </td>

      </tr>

    </table>

file modified
+26 -11
@@ -303,10 +303,15 @@ 

  

  

  def paginateMethod(server, values, methodName, args=None, kw=None,

-                    start=None, dataName=None, prefix=None, order=None, pageSize=50):

+                    start=None, dataName=None, prefix=None, order=None, pageSize=50,

+                    first_page_count=True):

      """Paginate the results of the method with the given name when called with the given args and

      kws. The method must support the queryOpts keyword parameter, and pagination is done in the

-     database."""

+     database.

+ 

+     :param bool first_page_count: If set to False, count is not returned for first page

+                                   to speedup default page.

+     """

      if args is None:

          args = []

      if kw is None:
@@ -320,8 +325,11 @@ 

      if not RE_ORDER.match(order):

          raise ValueError("Ordering is not alphanumeric: %r" % order)

  

-     kw['queryOpts'] = {'countOnly': True}

-     totalRows = getattr(server, methodName)(*args, **kw)

+     if start == 0 and not first_page_count:

+         totalRows = None

+     else:

+         kw['queryOpts'] = {'countOnly': True}

+         totalRows = getattr(server, methodName)(*args, **kw)

  

      kw['queryOpts'] = {'order': order,

                         'offset': start,
@@ -329,6 +337,10 @@ 

      data = getattr(server, methodName)(*args, **kw)

      count = len(data)

  

+     if start == 0 and count < pageSize:

+         # we've got everything on the first page

+         totalRows = count

+ 

      _populateValues(values, dataName, prefix, data, totalRows, start, count, pageSize, order)

  

      return data
@@ -370,7 +382,6 @@ 

      values[dataName] = data

      # Don't use capitalize() to title() here, they mess up

      # mixed-case name

-     values['total' + dataName[0].upper() + dataName[1:]] = totalRows

      # Possibly prepend a prefix to the numeric parameters, to avoid namespace collisions

      # when there is more than one list on the same page

      values[(prefix and prefix + 'Start' or 'start')] = start
@@ -379,12 +390,16 @@ 

      values[(prefix and prefix + 'Order' or 'order')] = order

      currentPage = start // pageSize

      values[(prefix and prefix + 'CurrentPage' or 'currentPage')] = currentPage

-     totalPages = int(totalRows // pageSize)

-     if totalRows % pageSize > 0:

-         totalPages += 1

-     pages = [page for page in range(0, totalPages)

-              if (abs(page - currentPage) < 100 or ((page + 1) % 100 == 0))]

-     values[(prefix and prefix + 'Pages') or 'pages'] = pages

+     values['total' + dataName[0].upper() + dataName[1:]] = totalRows

+     if totalRows is not None:

+         totalPages = int(totalRows // pageSize)

+         if totalRows % pageSize > 0:

+             totalPages += 1

+         pages = [page for page in range(0, totalPages)

+                  if (abs(page - currentPage) < 100 or ((page + 1) % 100 == 0))]

+         values[(prefix and prefix + 'Pages') or 'pages'] = pages

+     else:

+         values[(prefix and prefix + 'Pages') or 'pages'] = None

  

  

  def stateName(stateID):

Overall the code seems to work fine, but I'm a little concerned about the appearance.

With this change, the first page changes from the original heading of

Tasks 1 through 50 of 130 >>>

To simply

>>>

Without the additional text, the ">>>" looks a little strange and might not be as well understand by users. Perhaps we could just change it to something like "Tasks 1 through 50 of 130"?

As a result of change we don't know "130", So, something like "Tasks 1 through 50 next page"?

rebased onto f31b738

2 years ago

Sorry, I meant to edit that to remove the 130.

Perhaps just:
"Tasks 1 through 50"
or
""Tasks 1 through 50 of ???"

I guess I'd prefer the first since ??? is distracting.

Ah, that's what you have. Looks fine :thumbsup:

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

2 years ago

Metadata Update from @jobrauer:
- Pull-request tagged with: testing-done

2 years ago

Commit d6694d0 fixes this pull-request

Pull-Request has been merged by tkopecek

2 years ago