Fix duplicate users in membership service pagination

Closes PLAT-3607

Test Plan:
- Create a course with 20ish students
- Install a tool that uses the membership service via oauth 1
- Use the tool to hit the membership service endpoint:
  /api/lti/courses/17/membership_service?page=1&per_page=4
- Use the "nextPage" values to paginate through the entire
  collection
- Verify there are no duplicate students
- Verify all students are included
- Verfy the following membership services continue to
  work as before:
  * /api/lti/groups/:id/membership_service?per_page=10&page=1
  * /api/lti/courses/17/membership_service?page=1&per_page=4&
      role=urn%3Alti%3Acontext-type%3Aims%2Flis%2FGroupcollate_memberships

Change-Id: I3750a82d0f90ab3031862294d4d7016c67574108
Reviewed-on: https://gerrit.instructure.com/161761
Tested-by: Jenkins
Reviewed-by: Nathan Mills <nathanm@instructure.com>
QA-Review: Nathan Mills <nathanm@instructure.com>
Product-Review: Weston Dransfield <wdransfield@instructure.com>

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 <http://www.gnu.org/licenses/>.
+#
+
+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

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)

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

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
 

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)

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

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

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

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