Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Pagination Support #52

Merged
merged 3 commits into from

3 participants

@myronmarston

This provides pagination support as I asked for in #50.

I tried to add generic pagination support that can be used for lots of pages. There are probably pages I missed.

The failed page was a bit weird, as the order things came back in was reversed from the normal order--is that a bug or expected?

I also found that the 26th job would show up on page 1 (as the last job) and page 2 (as the first job), so there's an off-by-one error in there but I think it might be in the qless-core scripts.

Lastly, it'd be nice to provide more than next/prev (i.e. numbers to click) but that requires knowledge of how many jobs there are, and the object returned by the qless client don't provide that sort of rich metadata. I think what we have here is sufficient given that the UI is for developers--we can always edit the URL manually to go to a later page.

Please review!

/cc @dlecocq @proby

@proby proby commented on the diff
lib/qless/server.rb
((12 lines not shown))
- when 'scheduled'
- jobs = queue.jobs.scheduled
- when 'stalled'
- jobs = queue.jobs.stalled
- when 'depends'
- jobs = queue.jobs.depends
- when 'recurring'
- jobs = queue.jobs.recurring
- end
- jobs = jobs.map { |jid| Server.client.jobs[jid] }
- if tab == 'waiting'
- jobs = queue.peek(20)
+ tab = params.fetch('tab', 'stats')
+
+ jobs = if tab == 'waiting'
+ queue.peek(20)
@proby
proby added a note

Why only 20 here? I know it was 20 before but why isn't it 25 like everything else? I guess this is a question more for @dlecocq

@myronmarston Owner

Great question...

@dlecocq Owner
dlecocq added a note

Off hand, I can't recall. I don't see any reason it shouldn't be 25 like you guys pointed out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@myronmarston

@dlecocq -- is this good to merge? Also, do you have any ideas about these questions/issues I mentioned above?

The failed page was a bit weird, as the order things came back in was reversed from the normal order--is that a bug or expected?

I also found that the 26th job would show up on page 1 (as the last job) and page 2 (as the first job), so there's an off-by-one error in there but I think it might be in the qless-core scripts.

@dlecocq
Owner

Sorry, I've been sick all week -- just getting back into things.

The failed core script is a bit wonky. By default it takes 0 - 25 inclusive, so I'll make the fix and then tack it on to the end of the pull request. Sorry about that!

@dlecocq dlecocq merged commit 609952f into master

1 check failed

Details default The Travis build is in progress
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 12, 2012
  1. @myronmarston
  2. @myronmarston
Commits on Oct 19, 2012
  1. Fixing an off-by-one

    Dan Lecocq authored
This page is out of date. Refresh to see the latest.
2  lib/qless/qless-core
@@ -1 +1 @@
-Subproject commit 27c7e101d83e883416fe7b77bb8c50dc863bc05c
+Subproject commit 555c318a5baf8065d8a4daca44188cd7642604a2
View
70 lib/qless/server.rb
@@ -36,6 +36,45 @@ def path_prefix
request.env['SCRIPT_NAME']
end
+ def url_with_modified_query
+ url = URI(request.url)
+ existing_query = Rack::Utils.parse_query(url.query)
+ url.query = Rack::Utils.build_query(yield existing_query)
+ url.to_s
+ end
+
+ def page_url(offset)
+ url_with_modified_query do |query|
+ query.merge('page' => current_page + offset)
+ end
+ end
+
+ def next_page_url
+ page_url 1
+ end
+
+ def prev_page_url
+ page_url -1
+ end
+
+ def current_page
+ @current_page ||= begin
+ Integer(params[:page])
+ rescue
+ 1
+ end
+ end
+
+ PAGE_SIZE = 25
+ def pagination_values
+ start = (current_page - 1) * PAGE_SIZE
+ [start, start + PAGE_SIZE]
+ end
+
+ def paginated(qless_object, method, *args)
+ qless_object.send(method, *(args + pagination_values))
+ end
+
def tabs
return [
{:name => 'Queues' , :path => '/queues' },
@@ -132,26 +171,19 @@ def strftime(t)
json(Server.client.queues[params[:name]].counts)
end
+ filtered_tabs = %w[ running scheduled stalled depends recurring ].to_set
get '/queues/:name/?:tab?' do
queue = Server.client.queues[params[:name]]
- tab = params.fetch('tab', 'stats')
- jobs = []
- case tab
- when 'running'
- jobs = queue.jobs.running
- when 'scheduled'
- jobs = queue.jobs.scheduled
- when 'stalled'
- jobs = queue.jobs.stalled
- when 'depends'
- jobs = queue.jobs.depends
- when 'recurring'
- jobs = queue.jobs.recurring
- end
- jobs = jobs.map { |jid| Server.client.jobs[jid] }
- if tab == 'waiting'
- jobs = queue.peek(20)
+ tab = params.fetch('tab', 'stats')
+
+ jobs = if tab == 'waiting'
+ queue.peek(20)
@proby
proby added a note

Why only 20 here? I know it was 20 before but why isn't it 25 like everything else? I guess this is a question more for @dlecocq

@myronmarston Owner

Great question...

@dlecocq Owner
dlecocq added a note

Off hand, I can't recall. I don't see any reason it shouldn't be 25 like you guys pointed out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ elsif filtered_tabs.include?(tab)
+ paginated(queue.jobs, tab).map { |jid| Server.client.jobs[jid] }
+ else
+ []
end
+
erb :queue, :layout => true, :locals => {
:title => "Queue #{params[:name]}",
:tab => tab,
@@ -179,7 +211,7 @@ def strftime(t)
erb :failed_type, :layout => true, :locals => {
:title => 'Failed | ' + params[:type],
:type => params[:type],
- :failed => Server.client.jobs.failed(params[:type])
+ :failed => paginated(Server.client.jobs, :failed, params[:type])
}
end
@@ -215,7 +247,7 @@ def strftime(t)
end
get '/tag/?' do
- jobs = Server.client.jobs.tagged(params[:tag])
+ jobs = paginated(Server.client.jobs, :tagged, params[:tag])
erb :tag, :layout => true, :locals => {
:title => "Tag | #{params[:tag]}",
:tag => params[:tag],
View
8 lib/qless/server/views/_job_list.erb
@@ -0,0 +1,8 @@
+<%= erb :_pagination, :layout => false %>
+
+<% jobs.each do |job| %>
+<%= erb :_job, :layout => false, :locals => { :job => job, :queues => queues } %>
+<% end %>
+
+<%= erb :_pagination, :layout => false %>
+
View
7 lib/qless/server/views/_pagination.erb
@@ -0,0 +1,7 @@
+<div class="pagination">
+ <ul>
+ <li><a href="<%= prev_page_url %>">Prev</a></li>
+ <li><a href="<%= next_page_url %>">Next</a></li>
+ </ul>
+</div>
+
View
5 lib/qless/server/views/failed_type.erb
@@ -14,6 +14,5 @@
</div>
</div>
-<% failed['jobs'].each do |job| %>
-<%= erb :_job, :layout => false, :locals => { :job => job, :queues => queues } %>
-<% end %>
+<%= erb :_job_list, :locals => { :jobs => failed.fetch('jobs'), :queues => queues } %>
+
View
4 lib/qless/server/views/queue.erb
@@ -75,9 +75,7 @@
<% if ['running', 'waiting', 'scheduled', 'stalled', 'depends'].include?(tab) %>
<hr/>
- <% jobs.each do |job| %>
- <%= erb :_job, :layout => false, :locals => { :job => job, :queues => queues } %>
- <% end %>
+ <%= erb :_job_list, :locals => { :jobs => jobs, :queues => queues } %>
<% else %>
<div class="row" style="margin-top: 15px">
<div class="span6">
View
5 lib/qless/server/views/tag.erb
@@ -2,6 +2,5 @@
<h2>Jobs tagged '<%= tag %>'</h2>
</div>
-<% jobs.each do |job| %>
-<%= erb :_job, :layout => false, :locals => { :job => job, :queues => queues } %>
-<% end %>
+<%= erb :_job_list, :locals => { :jobs => jobs, :queues => queues } %>
+
View
65 spec/integration/server_spec.rb
@@ -42,6 +42,71 @@ def first(selector, options = {})
end
end
+ def build_paginated_objects
+ # build 30 since our page size is 25 so we have at least 2 pages
+ 30.times do |i|
+ yield "jid-#{i + 1}"
+ end
+ end
+
+ # We put periods on the end of these jids so that
+ # an assertion about "jid-1" will not pass if "jid-11"
+ # is on the page. The jids are formatted as "#{jid}..."
+ def assert_page(present_jid_num, absent_jid_num)
+ page.should have_content("jid-#{present_jid_num}.")
+ page.should_not have_content("jid-#{absent_jid_num}.")
+ end
+
+ def click_pagination_link(text)
+ within ".pagination" do
+ click_link text
+ end
+ end
+
+ def test_pagination(page_1_jid = 1, page_2_jid = 27)
+ assert_page page_1_jid, page_2_jid
+
+ click_pagination_link "Next"
+ assert_page page_2_jid, page_1_jid
+
+ click_pagination_link "Prev"
+ assert_page page_1_jid, page_2_jid
+ end
+
+ it 'can paginate a group of tagged jobs' do
+ build_paginated_objects do |jid|
+ q.put(Qless::Job, {}, :tags => ['foo'], :jid => jid)
+ end
+
+ visit '/tag?tag=foo'
+
+ test_pagination
+ end
+
+ it 'can paginate the failed jobs page' do
+ build_paginated_objects do |jid|
+ q.put(Qless::Job, {}, :jid => jid)
+ q.pop.fail("group", "msg")
+ end
+
+ visit '/failed/group'
+
+ # The failed jobs page shows the jobs in reverse order, for some reason.
+ test_pagination 20, 1
+ end
+
+ it 'can paginate jobs on a state filter tab' do
+ q.put(Qless::Job, {}, :jid => "parent-job")
+
+ build_paginated_objects do |jid|
+ q.put(Qless::Job, {}, :jid => jid, :depends => ["parent-job"])
+ end
+
+ visit "/queues/#{q.name}/depends"
+
+ test_pagination
+ end
+
it 'can see the root-level summary' do
visit '/'
Something went wrong with that request. Please try again.