diff --git a/lib/lti/membership_service/collator_base.rb b/lib/lti/membership_service/collator_base.rb new file mode 100644 index 00000000000..a9fb1df000b --- /dev/null +++ b/lib/lti/membership_service/collator_base.rb @@ -0,0 +1,57 @@ +# +# Copyright (C) 2018 - present Instructure, Inc. +# +# This file is part of Canvas. +# +# Canvas is free software: you can redistribute it and/or modify it under +# the terms of the GNU Affero General Public License as published by the Free +# Software Foundation, version 3 of the License. +# +# Canvas is distributed in the hope that it will be useful, but WITHOUT ANY +# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR +# A PARTICULAR PURPOSE. See the GNU Affero General Public License for more +# details. +# +# You should have received a copy of the GNU Affero General Public License along +# with this program. If not, see . +# + +module Lti::MembershipService + class CollatorBase + attr_reader :next_page + + def initialize + @next_page = true + end + + def next_page? + @next_page.present? + end + + def memberships + raise 'Abstract Method' + end + + protected + + def scope + raise 'Abstract Method' + end + + def membership_type + raise 'Abstract Method' + end + + def bookmarked_collection + @_bookmarked_collection ||= begin + bookmarker = BookmarkedCollection::SimpleBookmarker.new(membership_type, :id) + BookmarkedCollection.build(bookmarker) do |pager| + bookmarker_scope = bookmarker.restrict_scope(scope, pager) + page = bookmarker_scope.paginate(page: @page, per_page: pager.per_page) + @next_page = page.next_page + page + end + end + end + end +end diff --git a/lib/lti/membership_service/course_group_collator.rb b/lib/lti/membership_service/course_group_collator.rb index 582338926f5..f9932197c97 100644 --- a/lib/lti/membership_service/course_group_collator.rb +++ b/lib/lti/membership_service/course_group_collator.rb @@ -18,23 +18,27 @@ module Lti module MembershipService - class CourseGroupCollator - attr_reader :role, :per_page, :page, :context, :user, :memberships + class CourseGroupCollator < CollatorBase + attr_reader :role, :per_page, :page, :context, :user def initialize(context, opts={}) + super() @role = opts[:role] @per_page = [[opts[:per_page].to_i, Api.per_page].max, Api.max_per_page].min - @page = [opts[:page].to_i - 1, 0].max + @page = [opts[:page].to_i, 1].max @context = context - @memberships = collate_memberships end - def next_page? - groups.length > @per_page + def memberships + @_memberships ||= collate_memberships end private + def membership_type + Group + end + def collate_memberships groups.to_a.slice(0, @per_page).map do |user| generate_membership(user) @@ -42,10 +46,11 @@ module Lti end def groups - @groups ||= @context.groups.active. - order(:id). - offset(@page * @per_page). - limit(@per_page + 1) + @groups ||= bookmarked_collection.paginate(per_page: @per_page) + end + + def scope + @context.groups.active end def generate_member(group) diff --git a/lib/lti/membership_service/course_lis_person_collator.rb b/lib/lti/membership_service/course_lis_person_collator.rb index 568faef3b18..7200c76bb2a 100644 --- a/lib/lti/membership_service/course_lis_person_collator.rb +++ b/lib/lti/membership_service/course_lis_person_collator.rb @@ -30,13 +30,10 @@ module Lti private def users - @users ||= user_scope. - preload(:communication_channels, :not_ended_enrollments). - offset(@page * @per_page). - limit(@per_page + 1) + @users ||= bookmarked_collection.paginate(page: @page,per_page: @per_page) end - def user_scope + def scope options = { enrollment_type: ['teacher', 'ta', 'designer', 'observer', 'student'], include_inactive_enrollments: false diff --git a/lib/lti/membership_service/group_lis_person_collator.rb b/lib/lti/membership_service/group_lis_person_collator.rb index b846e684e88..0014cbc0e5d 100644 --- a/lib/lti/membership_service/group_lis_person_collator.rb +++ b/lib/lti/membership_service/group_lis_person_collator.rb @@ -29,14 +29,7 @@ module Lti private - def users - @users ||= user_scope. - preload(:communication_channels). - offset(@page * @per_page). - limit(@per_page + 1) - end - - def user_scope + def scope @user_scope ||= @user.nil? ? @context.participating_users : UserSearch.scope_for(@context, @user) end diff --git a/lib/lti/membership_service/lis_person_collator_base.rb b/lib/lti/membership_service/lis_person_collator_base.rb index a453071db3a..c07c76cbd4b 100644 --- a/lib/lti/membership_service/lis_person_collator_base.rb +++ b/lib/lti/membership_service/lis_person_collator_base.rb @@ -18,17 +18,14 @@ module Lti module MembershipService - class LisPersonCollatorBase + class LisPersonCollatorBase < CollatorBase attr_reader :role, :per_page, :page def initialize(opts={}) + super() @role = opts[:role] @per_page = [[opts[:per_page].to_i, Api.per_page].max, Api.max_per_page].min - @page = [opts[:page].to_i - 1, 0].max - end - - def next_page? - users.length > @per_page + @page = [opts[:page].to_i, 1].max end def memberships @@ -39,8 +36,12 @@ module Lti private + def membership_type + User.preload(:communication_channels, :not_ended_enrollments) + end + def users - [] + @users ||= bookmarked_collection.paginate(per_page: @per_page) end def generate_member(user) diff --git a/lib/lti/membership_service/page_presenter.rb b/lib/lti/membership_service/page_presenter.rb index dd087b34fcc..145364f6645 100644 --- a/lib/lti/membership_service/page_presenter.rb +++ b/lib/lti/membership_service/page_presenter.rb @@ -45,7 +45,7 @@ module Lti def next_page_query_params query = {} query[:role] = @membership_collator.role if @membership_collator.role - query[:page] = @membership_collator.page + 2 + query[:page] = @membership_collator.next_page query[:per_page] = @membership_collator.per_page query end diff --git a/spec/lib/lti/membership_service/course_group_collator_spec.rb b/spec/lib/lti/membership_service/course_group_collator_spec.rb index a0a6a47719d..36f288fa507 100644 --- a/spec/lib/lti/membership_service/course_group_collator_spec.rb +++ b/spec/lib/lti/membership_service/course_group_collator_spec.rb @@ -37,7 +37,7 @@ module Lti::MembershipService # expect(collator.role).to eq(IMS::LIS::ContextType::URNs::Group) expect(collator.per_page).to eq(Api.per_page) - expect(collator.page).to eq(0) + expect(collator.page).to eq(1) end it 'handles negative values for :page option' do @@ -46,7 +46,7 @@ module Lti::MembershipService } collator = CourseGroupCollator.new(@course, opts) - expect(collator.page).to eq(0) + expect(collator.page).to eq(1) end it 'handles negative values for :per_page option' do @@ -110,6 +110,7 @@ module Lti::MembershipService it 'returns false when there are no more pages' do collator = CourseGroupCollator.new(@course, page: 11) + collator.memberships expect(collator.next_page?).to eq(false) end end diff --git a/spec/lib/lti/membership_service/course_lis_person_collator_spec.rb b/spec/lib/lti/membership_service/course_lis_person_collator_spec.rb index ba592deb5ce..4debba6827e 100644 --- a/spec/lib/lti/membership_service/course_lis_person_collator_spec.rb +++ b/spec/lib/lti/membership_service/course_lis_person_collator_spec.rb @@ -32,7 +32,7 @@ module Lti::MembershipService expect(collator.role).to be_nil expect(collator.per_page).to eq(Api.per_page) - expect(collator.page).to eq(0) + expect(collator.page).to eq(1) end it 'handles negative values for :page option' do @@ -41,7 +41,7 @@ module Lti::MembershipService } collator = CourseLisPersonCollator.new(@course, @teacher, opts) - expect(collator.page).to eq(0) + expect(collator.page).to eq(1) end it 'handles negative values for :per_page option' do @@ -190,11 +190,31 @@ module Lti::MembershipService @course.enroll_user(@student, 'StudentEnrollment', enrollment_state: 'active') @observer = user_model @course.enroll_user(@observer, 'ObserverEnrollment', enrollment_state: 'active') + allow(Api).to receive(:per_page).and_return(1) + end + + context 'OAuth 1' do + subject do + collator_one.memberships.map(&:member).map(&:user_id) + + collator_two.memberships.map(&:member).map(&:user_id) + + collator_three.memberships.map(&:member).map(&:user_id) + end + + let(:collator_one) { CourseLisPersonCollator.new(@course, @teacher, per_page: 2, page: 1) } + let(:collator_two) { CourseLisPersonCollator.new(@course, @teacher, per_page: 2, page: 2) } + let(:collator_three) { CourseLisPersonCollator.new(@course, @teacher, per_page: 2, page: 3) } + + it 'does not render duplicate items when paginating' do + expect(subject.length).to eq subject.uniq.length + end + + it 'paginates the entire collection' do + expect(subject.length).to eq @course.current_users.length + end end describe '#memberships' do it 'returns the number of memberships specified by the per_page params' do - allow(Api).to receive(:per_page).and_return(1) collator = CourseLisPersonCollator.new(@course, @teacher, per_page: 1, page: 1) expect(collator.memberships.size).to eq(1) @@ -205,7 +225,6 @@ module Lti::MembershipService end it 'returns the right page of memberships based on the page param' do - allow(Api).to receive(:per_page).and_return(1) collator1 = CourseLisPersonCollator.new(@course, @teacher, per_page: 1, page: 1) collator2 = CourseLisPersonCollator.new(@course, @teacher, per_page: 1, page: 2) collator3 = CourseLisPersonCollator.new(@course, @teacher, per_page: 1, page: 3) @@ -225,16 +244,13 @@ module Lti::MembershipService describe '#next_page?' do it 'returns true when there is an additional page of results' do - allow(Api).to receive(:per_page).and_return(1) collator = CourseLisPersonCollator.new(@course, @teacher, per_page: 1, page: 1) - expect(collator.next_page?).to eq(true) end it 'returns false when there are no more pages' do - allow(Api).to receive(:per_page).and_return(1) collator = CourseLisPersonCollator.new(@course, @teacher, per_page: 1, page: 5) - + collator.memberships expect(collator.next_page?).to eq(false) end end diff --git a/spec/lib/lti/membership_service/group_lis_person_collator_spec.rb b/spec/lib/lti/membership_service/group_lis_person_collator_spec.rb index a3d8a3e2e11..cd3e56d1dec 100644 --- a/spec/lib/lti/membership_service/group_lis_person_collator_spec.rb +++ b/spec/lib/lti/membership_service/group_lis_person_collator_spec.rb @@ -113,14 +113,13 @@ module Lti::MembershipService it 'returns true when there is an additional page of results' do allow(Api).to receive(:per_page).and_return(1) collator = GroupLisPersonCollator.new(@group, @student1, per_page: 1, page: 1) - expect(collator.next_page?).to eq(true) end it 'returns false when there are no more pages' do allow(Api).to receive(:per_page).and_return(1) - collator = GroupLisPersonCollator.new(@group, @student1, per_page: 1, page: 5) - + collator = GroupLisPersonCollator.new(@group, @student1, per_page: 1, page: 3) + collator.memberships expect(collator.next_page?).to eq(false) end end