ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

change api pagination to include the current page link 

test plan:
 * exercise paginated api endpoints (including the search endpoint)
 - ensure the link headers now include current (for the current page)

refs CNVS-7508

Change-Id: Id271c3a05b726de9ce619bd0100af84db199d4f1
Reviewed-on: https://gerrit.instructure.com/23365
Tested-by: Jenkins <jenkins@instructure.com>
QA-Review: Hannah Bottalla <hannah@instructure.com>
Reviewed-by: James Williams  <jamesw@instructure.com>
Product-Review: Bracken Mosbacker <bracken@instructure.com>
This commit is contained in:
Mark Severson 2013-08-15 11:36:08 -06:00
parent 3cc0958328
commit 3603934770
10 changed files with 79 additions and 37 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
app/coffeescripts/collections/PaginatedCollection.coffee
View File

@ -26,7 +26,7 @@ define [
class PaginatedCollection extends Backbone.Collection
# Matches the name of each link: "next," "prev," "first," or "last."
# Matches the name of each link: "current", "next," "prev," "first," or "last."
nameRegex: /rel="([a-z]+)/
# Matches the full link, e.g. "/api/v1/accounts/1/users?page=1&per_page=15"
@ -34,10 +34,14 @@ define [
pageRegex: /\Wpage=(\d+)/
perPageRegex: /\per_page=(\d+)/
perPageRegex: /\Wper_page=(\d+)/
initialize: ->
super
@urls = {}
##
# options.page: 'next', 'prev', 'first', 'last', 'top', 'bottom'
# options.page: 'current', 'next', 'prev', 'first', 'last', 'top', 'bottom'
fetch: (options = {}) ->
exclusionFlag = "fetching#{capitalize options.page}Page"
@[exclusionFlag] = true
@ -75,7 +79,7 @@ define [
@_urlCache ?= []
urlIsNotCached = options.url not in @_urlCache
@_urlCache.push options.url if not urlIsNotCached
firstRequest = !@urls?
firstRequest = !@atLeastOnePageFetched
setBottom = firstRequest or (options.page in ['next', 'bottom'] and urlIsNotCached)
setTop = firstRequest or (options.page in ['prev', 'top'] and urlIsNotCached)
oldUrls = @urls

12
doc/api/pagination.md
View File

@ -8,19 +8,21 @@ check for the `Link` header.
To retrieve additional pages, the returned `Link` headers should be used. These
links should be treated as opaque. They will be absolute urls that include all
parameters necessary to retrieve the desired next, previous, first, or last
page. The one exception is that if an access_token parameter is sent for
parameters necessary to retrieve the desired current, next, previous, first, or
last page. The one exception is that if an access_token parameter is sent for
authentication, it will not be included in the returned links, and must be
re-appended.
Pagination information is provided in the [Link header](http://www.w3.org/Protocols/9707-link-header.html):
Link: <https://<canvas>/api/v1/courses/:id/discussion_topics.json?opaqueA>; rel="next",
<https://<canvas>/api/v1/courses/:id/discussion_topics.json?opaqueB>; rel="first",
<https://<canvas>/api/v1/courses/:id/discussion_topics.json?opaqueC>; rel="last"
Link: <https://<canvas>/api/v1/courses/:id/discussion_topics.json?opaqueA>; rel="current",
<https://<canvas>/api/v1/courses/:id/discussion_topics.json?opaqueB>; rel="next",
<https://<canvas>/api/v1/courses/:id/discussion_topics.json?opaqueC>; rel="first",
<https://<canvas>/api/v1/courses/:id/discussion_topics.json?opaqueD>; rel="last"
The possible `rel` values are:
* current - link to the current page of results.
* next - link to the next page of results.
* prev - link to the previous page of results.
* first - link to the first page of results.

3
lib/api.rb
View File

@ -209,6 +209,7 @@ module Api
links = build_links(base_url, {
:query_parameters => controller.request.query_parameters,
:per_page => collection.per_page,
:current => collection.current_page || first_page,
:next => collection.next_page,
:prev => collection.previous_page,
:first => first_page,
@ -225,7 +226,7 @@ module Api
qp = opts[:query_parameters] || {}
qp = qp.with_indifferent_access.except(*EXCLUDE_IN_PAGINATION_LINKS)
base_url += "#{qp.to_query}&" if qp.present?
[:next, :prev, :first, :last].each do |k|
[:current, :next, :prev, :first, :last].each do |k|
if opts[k].present?
links << "<#{base_url}page=#{opts[k]}&per_page=#{opts[:per_page]}>; rel=\"#{k}\""
end

12
spec/apis/v1/courses_api_spec.rb
View File

@ -1118,8 +1118,9 @@ describe CoursesController, :type => :integration do
json = api_call(:get, api_url, api_route, :search_term => "SSS", :limit => 1)
json.length.should == 1
link_header = response.headers['Link'].split(',')
link_header[0].should match /page=2&per_page=1/ # next page
link_header[1].should match /page=1&per_page=1/ # first page
link_header[0].should match /page=1&per_page=1/ # current page
link_header[1].should match /page=2&per_page=1/ # next page
link_header[2].should match /page=1&per_page=1/ # first page
end
it "should respect includes" do
@ -1416,9 +1417,10 @@ describe CoursesController, :type => :integration do
json.map{|x| x['id']}.uniq.length.should == 5
link_header = response.headers['Link'].split(',')
link_header[0].should match /page=2&per_page=5/ # next page
link_header[1].should match /page=1&per_page=5/ # first page
link_header[2].should match /page=2&per_page=5/ # last page
link_header[0].should match /page=1&per_page=5/ # current page
link_header[1].should match /page=2&per_page=5/ # next page
link_header[2].should match /page=1&per_page=5/ # first page
link_header[3].should match /page=2&per_page=5/ # last page
end
it "should allow jumping to a user's page based on id" do

14
spec/apis/v1/enrollments_api_spec.rb
View File

@ -832,16 +832,18 @@ describe EnrollmentsApiController, :type => :integration do
h
end
link_header = response.headers['Link'].split(',')
link_header[0].should match /page=2&per_page=1/ # next page
link_header[1].should match /page=1&per_page=1/ # first page
link_header[2].should match /page=2&per_page=1/ # last page
link_header[0].should match /page=1&per_page=1/ # current page
link_header[1].should match /page=2&per_page=1/ # next page
link_header[2].should match /page=1&per_page=1/ # first page
link_header[3].should match /page=2&per_page=1/ # last page
json.should eql [enrollments[0]]
json = api_call(:get, "#{@path}?page=2&per_page=1", @params.merge(:page => 2.to_param, :per_page => 1.to_param))
link_header = response.headers['Link'].split(',')
link_header[0].should match /page=1&per_page=1/ # prev page
link_header[1].should match /page=1&per_page=1/ # first page
link_header[2].should match /page=2&per_page=1/ # last page
link_header[0].should match /page=2&per_page=1/ # current page
link_header[1].should match /page=1&per_page=1/ # prev page
link_header[2].should match /page=1&per_page=1/ # first page
link_header[3].should match /page=2&per_page=1/ # last page
json.should eql [enrollments[1]]
end
end

7
spec/apis/v1/pseudonyms_api_spec.rb
View File

@ -64,9 +64,10 @@ describe PseudonymsController, :type => :integration do
})
json.count.should eql 1
headers = response.headers['Link'].split(',')
headers[0].should match /page=2&per_page=1/ # next page
headers[1].should match /page=1&per_page=1/ # first page
headers[2].should match /page=2&per_page=1/ # last page
headers[0].should match /page=1&per_page=1/ # current page
headers[1].should match /page=2&per_page=1/ # next page
headers[2].should match /page=1&per_page=1/ # first page
headers[3].should match /page=2&per_page=1/ # last page
end
it "should return all pseudonyms for a user" do

18
spec/apis/v1/search_api_spec.rb
View File

@ -295,7 +295,7 @@ describe SearchController, :type => :integration do
l['search'].should == 'cletus'
l['type'].should == 'user'
end
links.map{ |l| l[:rel] }.should == ['next', 'first']
links.map{ |l| l[:rel] }.should == ['current', 'next', 'first']
# get the next page
json = follow_pagination_link('next', :controller => 'search', :action => 'recipients')
@ -306,7 +306,7 @@ describe SearchController, :type => :integration do
l['search'].should == 'cletus'
l['type'].should == 'user'
end
links.map{ |l| l[:rel] }.should == ['first']
links.map{ |l| l[:rel] }.should == ['current', 'first']
end
it "should paginate contexts and return proper pagination headers" do
@ -324,7 +324,7 @@ describe SearchController, :type => :integration do
l['search'].should == 'ofcourse'
l['type'].should == 'context'
end
links.map{ |l| l[:rel] }.should == ['next', 'first']
links.map{ |l| l[:rel] }.should == ['current', 'next', 'first']
# get the next page
json = follow_pagination_link('next', :controller => 'search', :action => 'recipients')
@ -335,7 +335,7 @@ describe SearchController, :type => :integration do
l['search'].should == 'ofcourse'
l['type'].should == 'context'
end
links.map{ |l| l[:rel] }.should == ['first']
links.map{ |l| l[:rel] }.should == ['current', 'first']
end
it "should ignore invalid per_page" do
@ -350,7 +350,7 @@ describe SearchController, :type => :integration do
l['search'].should == 'cletus'
l['type'].should == 'user'
end
links.map{ |l| l[:rel] }.should == ['next', 'first']
links.map{ |l| l[:rel] }.should == ['current', 'next', 'first']
# get the next page
json = follow_pagination_link('next', :controller => 'search', :action => 'recipients')
@ -361,7 +361,7 @@ describe SearchController, :type => :integration do
l['search'].should == 'cletus'
l['type'].should == 'user'
end
links.map{ |l| l[:rel] }.should == ['first']
links.map{ |l| l[:rel] }.should == ['current', 'first']
end
it "should paginate combined context/user results" do
@ -385,7 +385,7 @@ describe SearchController, :type => :integration do
l[:uri].to_s.should match(%r{api/v1/search/recipients})
l['search'].should == 'term'
end
links.map{ |l| l[:rel] }.should == ['next', 'first']
links.map{ |l| l[:rel] }.should == ['current', 'next', 'first']
# get the next page
json = follow_pagination_link('next', :controller => 'search', :action => 'recipients')
@ -396,7 +396,7 @@ describe SearchController, :type => :integration do
l[:uri].to_s.should match(%r{api/v1/search/recipients})
l['search'].should == 'term'
end
links.map{ |l| l[:rel] }.should == ['next', 'first']
links.map{ |l| l[:rel] }.should == ['current', 'next', 'first']
# get the final page
json = follow_pagination_link('next', :controller => 'search', :action => 'recipients')
@ -407,7 +407,7 @@ describe SearchController, :type => :integration do
l[:uri].to_s.should match(%r{api/v1/search/recipients})
l['search'].should == 'term'
end
links.map{ |l| l[:rel] }.should == ['first']
links.map{ |l| l[:rel] }.should == ['current', 'first']
end
end

30
spec/coffeescripts/collections/PaginatedCollectionSpec.coffee
View File

@ -50,6 +50,26 @@ define [
@collection.fetch()
@server.sendPage page, @collection.urlWithParams()
test 'fetches current page', 10, ->
page1 = getFakePage 1
@collection.fetch success: =>
equal @collection.models.length, 2, 'added models to collection'
equal @collection.models[0].get('id'), 1, 'added model to collection'
equal @collection.models[1].get('id'), 2, 'added model to collection'
equal @collection.urls.current, page1.urls.current, 'current url matches'
@server.sendPage page1, @collection.urlWithParams()
@collection.on 'fetch:current', (self, modelData) ->
ok true, 'triggers fetch:current event'
deepEqual modelData, page1.data, 'passes data in'
@collection.fetch page: 'current', success: =>
equal @collection.models.length, 2, 'added models to collection'
equal @collection.models[0].get('id'), 1, 'passed in model to current page handler'
equal @collection.models[1].get('id'), 2, 'passed in model to current page handler'
equal @collection.urls.current, page1.urls.current, 'current url matches'
@server.sendPage page1, @collection.urls.current
test 'fetches next page', 8, ->
page1 = getFakePage 1
page2 = getFakePage 2
@ -67,7 +87,6 @@ define [
equal @collection.models[2].get('id'), 3, 'passed in model to next page handler'
equal @collection.models[3].get('id'), 4, 'passed in model to next page handler'
equal @collection.urls.next, page2.urls.next, 'next url matches'
console.log @collection.urls.next
@server.sendPage page2, @collection.urls.next
test 'fetches previous page', 8, ->
@ -92,7 +111,7 @@ define [
@server.sendPage page1, @collection.urls.prev
test 'fetches prev, next, top and bottom pages', 8, ->
test 'fetches current, prev, next, top and bottom pages', 8, ->
page1 = getFakePage 1
page2 = getFakePage 2
page3 = getFakePage 3
@ -106,6 +125,13 @@ define [
deepEqual @collection.urls, expectedUrls, 'urls are as expected for fetch'
@server.sendPage page3, @collection.urlWithParams()
@collection.fetch page: 'current', success: =>
expectedUrls = page3.urls
expectedUrls.top = page3.urls.prev
expectedUrls.bottom = page3.urls.next
deepEqual @collection.urls, expectedUrls, 'urls are as expected for fetch current'
@server.sendPage page3, @collection.urlWithParams()
@collection.fetch page: 'prev', success: =>
equal @collection.models.length, 4, 'added models to collection'
expectedUrls = page2.urls

3
spec/coffeescripts/helpers/getFakePage.coffee
View File

@ -6,9 +6,10 @@ define ->
url = (page) -> "/api/v1/context/2/resource?page=#{page}&per_page=2"
lastID = thisPage * 2
urls =
current: url thisPage
first: url 1
last: url 10
links = []
links = ['<' + urls.current + '>; rel="current"']
if thisPage < 10
urls.next = url thisPage + 1
links.push '<' + urls.next + '>; rel="next"'

5
spec/lib/api_spec.rb
View File

@ -603,6 +603,7 @@ describe Api do
it "should not build links for empty pages" do
Api.build_links("www.example.com/", {
:per_page => 10,
:current => "",
:next => "",
:prev => "",
:first => "",
@ -610,15 +611,17 @@ describe Api do
}).should be_empty
end
it "should build next, prev, first, and last links if provided" do
it "should build current, next, prev, first, and last links if provided" do
links = Api.build_links("www.example.com/", {
:per_page => 10,
:current => 8,
:next => 4,
:prev => 2,
:first => 1,
:last => 10,
})
links.all?{ |l| l =~ /www.example.com\/\?/ }.should be_true
links.find{ |l| l.match(/rel="current"/)}.should =~ /page=8&per_page=10>/
links.find{ |l| l.match(/rel="next"/)}.should =~ /page=4&per_page=10>/
links.find{ |l| l.match(/rel="prev"/)}.should =~ /page=2&per_page=10>/
links.find{ |l| l.match(/rel="first"/)}.should =~ /page=1&per_page=10>/