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

fix ordering on a few conversions to ShardedBookmarkedCollection 

didn't realize that helper method would return the raw relation as
an optimization, which if the relation doesn't have an order by on
it would be bad.

also fix the enrollments_api_spec to actually test bookmarks
and non-bookmarks, and simplify some of the logic in
EnrollmentsApiController#index both to clean up the code, and to
allow the specs to easily test both cases

Change-Id: Ifbaf588f787de7b54f3a26c6412cca81e2f34c4a
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/270186
Reviewed-by: Simon Williams <simon@instructure.com>
Reviewed-by: Rob Orton <rob@instructure.com>
QA-Review: Simon Williams <simon@instructure.com>
Product-Review: Simon Williams <simon@instructure.com>
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
This commit is contained in:
Cody Cutrer 2021-07-28 13:33:11 -06:00
parent 3176418d6b
commit 679e8d9201
4 changed files with 39 additions and 32 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/controllers/accounts_controller.rb
View File

@ -371,7 +371,7 @@ class AccountsController < ApplicationController
@current_user.enrollments.admin.shard(@current_user).except(:select, :joins)
).select("accounts.id").distinct.pluck(:id).map{|id| Shard.global_id_for(id)}
end
course_accounts = ShardedBookmarkedCollection.build(Account::Bookmarker, Account.where(id: account_ids))
course_accounts = ShardedBookmarkedCollection.build(Account::Bookmarker, Account.where(id: account_ids), always_use_bookmarks: true)
@accounts = Api.paginate(course_accounts, self, api_v1_course_accounts_url)
else
@accounts = []

27
app/controllers/enrollments_api_controller.rb
View File

@ -425,14 +425,7 @@ class EnrollmentsApiController < ApplicationController
course_index_enrollments :
user_index_enrollments
# a few specific developer keys temporarily need bookmarking disabled, see INTEROP-5326
pagination_override_key_list = Setting.get("pagination_override_key_list", "").split(',').map(&:to_i)
use_numeric_pagination_override = pagination_override_key_list.include?(@access_token&.global_developer_key_id)
use_bookmarking = !use_numeric_pagination_override
enrollments = use_bookmarking ?
enrollments.joins(:user).select("enrollments.*, users.sortable_name AS sortable_name") :
enrollments.joins(:user).select("enrollments.*").
order(:type, User.sortable_name_order_by_clause("users"), :id)
enrollments = enrollments.joins(:user).select("enrollments.*")
has_courses = enrollments.where_clause.instance_variable_get(:@predicates).
any? { |cond| cond.is_a?(String) && cond =~ /courses\./ }
@ -485,14 +478,14 @@ class EnrollmentsApiController < ApplicationController
end
end
enrollments = enrollments.joins(:user).order(User.sortable_name_order_by_clause)
collection =
if use_bookmarking
if use_bookmarking?
enrollments = enrollments.select("users.sortable_name AS sortable_name")
bookmarker = BookmarkedCollection::SimpleBookmarker.new(Enrollment,
{:type => {:skip_collation => true}, :sortable_name => {:type => :string, :null => false}}, :id)
ShardedBookmarkedCollection.build(bookmarker, enrollments)
ShardedBookmarkedCollection.build(bookmarker, enrollments, always_use_bookmarks: true)
else
enrollments
enrollments.order(:type, User.sortable_name_order_by_clause("users"), :id)
end
enrollments = Api.paginate(
collection,
@ -1012,4 +1005,14 @@ class EnrollmentsApiController < ApplicationController
def render_create_errors(errors)
render json: {message: errors.join(', ')}, status: :bad_request
end
def use_bookmarking?
unless instance_variable_defined?(:@use_bookmarking)
# a few specific developer keys temporarily need bookmarking disabled, see INTEROP-5326
pagination_override_key_list = Setting.get("pagination_override_key_list", "").split(',').map(&:to_i)
use_numeric_pagination_override = pagination_override_key_list.include?(@access_token&.global_developer_key_id)
@use_bookmarking = !use_numeric_pagination_override
end
@use_bookmarking
end
end

6
app/models/sharded_bookmarked_collection.rb
View File

@ -35,7 +35,7 @@ module ShardedBookmarkedCollection
# scope.active
# end
#
def self.build(bookmarker, relation)
def self.build(bookmarker, relation, always_use_bookmarks: false)
# automatically make associations multi-shard, since that's definitely what you want if you're
# using this
if (owner = (relation.respond_to?(:proxy_association) && relation.proxy_association&.owner))
@ -50,6 +50,7 @@ module ShardedBookmarkedCollection
sharded_relation = yield sharded_relation if block_given?
# if they returned nil, there was nothing pertinent on this shard anyway, so completely skip it
next if sharded_relation.nil?
last_relation = sharded_relation
collections << [Shard.current.id, BookmarkedCollection.wrap(bookmarker, sharded_relation)]
nil
@ -57,7 +58,8 @@ module ShardedBookmarkedCollection
# optimization if there ended up being none
return relation.none if collections.empty?
# optimization if there only ended up being one
return last_relation if collections.size == 1
return always_use_bookmarks ? collections.last.last : last_relation if collections.size == 1
BookmarkedCollection.merge(*collections)
end
end

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

@ -849,6 +849,7 @@ describe EnrollmentsApiController, type: :request do
end
it "should deterministically order enrollments for pagination" do
allow_any_instance_of(EnrollmentsApiController).to receive(:use_bookmarking?).and_return(true)
enrollment_num = 10
enrollment_num.times do
u = user_with_pseudonym(name: "John Smith", sortable_name: "Smith, John")
@ -857,12 +858,12 @@ describe EnrollmentsApiController, type: :request do
found_enrollment_ids = []
enrollment_num.times do |i|
if i == 0
json = api_call(:get, "/api/v1/courses/#{@course.id}/enrollments?per_page=1",
json = if i == 0
api_call(:get, "/api/v1/courses/#{@course.id}/enrollments?per_page=1",
:controller => "enrollments_api", :action => "index", :format => "json",
:course_id => @course.id.to_s, :per_page => 1)
else
json = follow_pagination_link('next', {:controller => 'enrollments_api',
follow_pagination_link('next', {:controller => 'enrollments_api',
:action => 'index', :format => 'json', :course_id => @course.id.to_s})
end
id = json[0]["id"]
@ -873,6 +874,7 @@ describe EnrollmentsApiController, type: :request do
end
it "should deterministically order enrollments for pagination with bookmarking not enabled" do
allow_any_instance_of(EnrollmentsApiController).to receive(:use_bookmarking?).and_return(false)
enrollment_num = 10
enrollment_num.times do
u = user_with_pseudonym(name: "John Smith", sortable_name: "Smith, John")
@ -881,12 +883,12 @@ describe EnrollmentsApiController, type: :request do
found_enrollment_ids = []
enrollment_num.times do |i|
if i == 0
json = api_call(:get, "/api/v1/courses/#{@course.id}/enrollments?per_page=1",
json = if i == 0
api_call(:get, "/api/v1/courses/#{@course.id}/enrollments?per_page=1",
:controller => "enrollments_api", :action => "index", :format => "json",
:course_id => @course.id.to_s, :per_page => 1)
else
json = follow_pagination_link('next', {:controller => 'enrollments_api',
follow_pagination_link('next', {:controller => 'enrollments_api',
:action => 'index', :format => 'json', :course_id => @course.id.to_s})
end
id = json[0]["id"]
@ -1309,7 +1311,7 @@ describe EnrollmentsApiController, type: :request do
@path = "/api/v1/courses/#{@course.id}/enrollments"
@params = { :controller => "enrollments_api", :action => "index", :course_id => @course.id.to_param, :format => "json", :include => ["group_ids"] }
enrollments_json = api_call(:get, @path, @params)
expect(enrollments_json [1]["user"]["group_ids"]).to eq([@group.id])
expect(enrollments_json[0]["user"]["group_ids"]).to eq([@group.id])
end
it "should not include a users deleted memberships" do
@ -1333,8 +1335,8 @@ describe EnrollmentsApiController, type: :request do
@params = { :controller => "enrollments_api", :action => "index", :course_id => @course.id.to_param, :format => "json", :include => ["group_ids"] }
enrollments_json = api_call(:get, @path, @params)
expect(enrollments_json[1]["user"]["group_ids"]).to include(@group.id)
expect(enrollments_json[1]["user"]["group_ids"]).not_to include(group2.id)
expect(enrollments_json[0]["user"]["group_ids"]).to include(@group.id)
expect(enrollments_json[0]["user"]["group_ids"]).not_to include(group2.id)
end
end
@ -1931,7 +1933,7 @@ describe EnrollmentsApiController, type: :request do
enrollments = %w{observer student ta teacher}.inject([]) do |res, type|
res + @course.send("#{type}_enrollments").preload(:user)
end
enrollments = enrollments.sort_by {|e| e.user.sortable_name}
enrollments = enrollments.sort_by {|e| [e.type, e.user.sortable_name] }
expect(json).to eq(enrollments.map do |e|
user_json = {
'name' => e.user.name,
@ -2364,26 +2366,26 @@ describe EnrollmentsApiController, type: :request do
'current_grade' => nil,
} if e.student?
h.merge!(
'last_activity_at' => e.last_activity_at.xmlschema,
'last_activity_at' => nil,
'last_attended_at' => nil,
'total_activity_time' => 0
) if e.user == @user
h
end
link_header = response.headers['Link'].split(',')
expect(link_header[0]).to match /page=1&per_page=1/ # current page
expect(link_header[0]).to match /page=.*&per_page=1/ # current page
md = link_header[1].match(/page=(.*)&per_page=1/) # next page
bookmark = md[1]
expect(bookmark).to be_present
expect(link_header[2]).to match /page=1&per_page=1/ # first page
expect(json).to eql [enrollments[1]]
expect(link_header[2]).to match /page=.*&per_page=1/ # first page
expect(json).to eql [enrollments[0]]
json = api_call(:get, "#{@path}?page=#{bookmark}&per_page=1", @params.merge(:page => bookmark, :per_page => 1.to_param))
link_header = response.headers['Link'].split(',')
expect(link_header[0]).to match /page=#{bookmark}&per_page=1/ # current page
expect(link_header[1]).to match /page=1&per_page=1/ # first page
expect(link_header[1]).to match /page=.*&per_page=1/ # first page
expect(link_header[2]).to match /page=.*&per_page=1/ # last page
expect(json).to eql [enrollments[0]]
expect(json).to eql [enrollments[1]]
end
end
@ -2726,7 +2728,7 @@ describe EnrollmentsApiController, type: :request do
@course.enroll_user(@new_user, 'ObserverEnrollment', :enrollment_state => 'active')
@user = request_user
json = api_call(:get, "#{@path}?type[]=StudentEnrollment&type[]=TeacherEnrollment", @params.merge(:type => %w{StudentEnrollment TeacherEnrollment}))
enrollments = (@course.student_enrollments + @course.teacher_enrollments).sort_by {|e| e.user.sortable_name}
enrollments = (@course.student_enrollments + @course.teacher_enrollments).sort_by {|e| [e.type, e.user.sortable_name] }
expect(json).to eq(enrollments.map { |e|
h = {