remove concluded enrollments from course roster table

fixes VICE-3051
flag=react_people_page

Test Plan:
- Tests pass
- With FF turned off, visit a course people page as a user
  with the teacher role
- For an existing user, add a new enrollment (role and/or
  section) that is distinct from its existing enrollments
- Visit the user's user detail page and conclude the
  enrollment that you just added
- Turn on 'People Page Upgrade' FF
- Visit the course people page and verify that the concluded
  enrollment is not visible in the roster table

Change-Id: I0f9ce2f7d2c065f47215d2c0187d5b8856e58c6e
Reviewed-on: https://gerrit.instructure.com/c/canvas-lms/+/298420
Tested-by: Service Cloud Jenkins <svc.cloudjenkins@instructure.com>
Reviewed-by: Omar Soto-Fortuño <omar.soto@instructure.com>
Product-Review: Omar Soto-Fortuño <omar.soto@instructure.com>
QA-Review: Jason Gillett <jason.gillett@instructure.com>

app/graphql/types/user_type.rb

@@ -112,6 +112,9 @@ module Types
       argument :order_by, [String],
                "The fields to order the results by",
                required: false
+      argument :exclude_concluded, Boolean,
+               "Whether or not to exclude `completed` enrollments",
+               required: false
     end
 
     field :login_id, String, null: true
@@ -132,12 +135,13 @@ module Types
       pseudonym.unique_id
     end
 
-    def enrollments(course_id: nil, current_only: false, order_by: [])
+    def enrollments(course_id: nil, current_only: false, order_by: [], exclude_concluded: false)
       course_ids = [course_id].compact
       Loaders::UserCourseEnrollmentLoader.for(
         course_ids: course_ids,
         order_by: order_by,
-        current_only: current_only
+        current_only: current_only,
+        exclude_concluded: exclude_concluded
       ).load(object.id).then do |enrollments|
         (enrollments || []).select do |enrollment|
           object == context[:current_user] ||
@@ -434,7 +438,7 @@ end
 
 module Loaders
   class UserCourseEnrollmentLoader < Loaders::ForeignKeyLoader
-    def initialize(course_ids:, order_by: [], current_only: false)
+    def initialize(course_ids:, order_by: [], current_only: false, exclude_concluded: false)
       scope = Enrollment.joins(:course)
 
       scope = if current_only
@@ -446,6 +450,8 @@ module Loaders
 
       scope = scope.where(course_id: course_ids) if course_ids.present?
 
+      scope = scope.where.not(enrollments: { workflow_state: "completed" }) if exclude_concluded
+
       order_by.each { |o| scope = scope.order(o) }
 
       super(scope, :user_id)

spec/graphql/types/user_type_spec.rb

@@ -283,6 +283,12 @@ describe Types::UserType do
         user_type.resolve(%|enrollments(courseId: "#{@course2.id}") { _id }|)
       ).to eq []
     end
+
+    it "excludes concluded enrollments when excludeConcluded is true" do
+      expect(user_type.resolve("enrollments(excludeConcluded: true) { _id }").length).to eq 1
+      @student.enrollments.update_all workflow_state: "completed"
+      expect(user_type.resolve("enrollments(excludeConcluded: true) { _id }")).to eq []
+    end
   end
 
   context "email" do

ui/features/course_people/graphql/Mocks.js

@@ -35,37 +35,34 @@ export const mockUser = ({
   sectionName = 'Section 1',
   additionalEnrollments = []
 } = {}) => ({
-  node: {
+  name,
-    name,
+  _id,
-    _id,
+  id: Buffer.from(`User-${_id}`).toString('base64'),
-    id: Buffer.from(`User-${_id}`).toString('base64'),
+  sisId,
-    sisId,
+  avatarUrl,
-    avatarUrl,
+  pronouns,
-    pronouns,
+  loginId,
-    loginId,
+  __typename: 'user',
-    __typename: 'user',
+  enrollments: [
-    enrollments: [
+    {
-      {
+      id: Buffer.from(`Enrollment-${_id}`).toString('base64'),
-        id: Buffer.from(`Enrollment-${_id}`).toString('base64'),
+      type: enrollmentType,
-        type: enrollmentType,
+      state: enrollmentStatus,
-        state: enrollmentStatus,
+      lastActivityAt,
-        lastActivityAt,
+      htmlUrl: `http://test.host/courses/${courseID}/users/${_id}`,
-        htmlUrl: `http://test.host/courses/${courseID}/users/${_id}`,
+      totalActivityTime,
-        totalActivityTime,
+      canBeRemoved,
-        canBeRemoved,
+      associatedUser: null, // always null for user's own enrollment
-        associatedUser: null, // always null for user's own enrollment
+      __typename: 'enrollment',
-        __typename: 'enrollment',
+      section: {
-        section: {
+        _id: sectionID,
-          _id: sectionID,
+        id: Buffer.from(`Section-${sectionID}`).toString('base64'),
-          id: Buffer.from(`Section-${sectionID}`).toString('base64'),
+        name: sectionName,
-          name: sectionName,
+        __typename: 'section'
-          __typename: 'section'
+      }
-        }
+    },
-      },
+    ...additionalEnrollments
-      ...additionalEnrollments
+  ]
-    ]
-  },
-  __typename: 'usersConnectionEdge'
 })
 
 export const mockEnrollment = ({
@@ -115,7 +112,7 @@ export const getRosterQueryMock = ({mockUsers = [], courseID = '1', shouldError
       data: {
         course: {
           usersConnection: {
-            edges: [...mockUsers],
+            nodes: [...mockUsers],
             __typename: 'usersConnection'
           },
           __typename: 'course'

ui/features/course_people/graphql/Queries.js

@@ -22,33 +22,31 @@ export const ROSTER_QUERY = gql`
   query getRosterQuery($courseID: ID!) {
     course(id: $courseID) {
       usersConnection {
-        edges {
+        nodes {
-          node {
+          name
-            name
+          _id
-            _id
+          id
+          sisId
+          avatarUrl
+          pronouns
+          loginId
+          enrollments(courseId: $courseID, excludeConcluded: true) {
             id
-            sisId
+            type
-            avatarUrl
+            state
-            pronouns
+            lastActivityAt
-            loginId
+            htmlUrl
-            enrollments(courseId: $courseID) {
+            totalActivityTime
+            canBeRemoved
+            associatedUser {
+              _id
               id
-              type
+              name
-              state
+            }
-              lastActivityAt
+            section {
-              htmlUrl
+              _id
-              totalActivityTime
+              id
-              canBeRemoved
+              name
-              associatedUser {
-                _id
-                id
-                name
-              }
-              section {
-                _id
-                id
-                name
-              }
             }
           }
         }

ui/features/course_people/react/containers/RosterTable/RosterTable.js

@@ -51,8 +51,8 @@ const RosterTable = () => {
 
   const {view_user_logins, read_sis} = ENV?.permissions || {}
 
-  const tableRows = data.course.usersConnection.edges.map(edge => {
+  const tableRows = data.course.usersConnection.nodes.map(node => {
-    const {name, _id, sisId, enrollments, loginId, avatarUrl, pronouns} = edge.node
+    const {name, _id, sisId, enrollments, loginId, avatarUrl, pronouns} = node
     const {totalActivityTime, htmlUrl, state} = enrollments[0]
 
     const sectionNames = enrollments.map(enrollment => {

ui/features/course_people/react/containers/RosterTable/__tests__/RosterTable.test.js

@@ -169,7 +169,7 @@ describe('RosterTable', () => {
   it('should wrap the name of each user in a button', async () => {
     const container = setup(getRosterQueryMock({mockUsers}))
     const cells = await container.findAllByTestId('roster-table-name-cell')
-    const names = mockUsers.map(user => user.node.name)
+    const names = mockUsers.map(user => user.name)
     cells.forEach((cell, index) => {
       const nameMatch = new RegExp(names[index])
       const button = within(cell).getByRole('button', {name: nameMatch})
@@ -186,13 +186,13 @@ describe('RosterTable', () => {
     const container = setup(getRosterQueryMock({mockUsers: [mockSelf]}))
     const cells = await container.findByTestId('roster-table-name-cell')
     const link = within(cells).getByRole('link', {name: nameMatch})
-    expect(link).toHaveAttribute('href', mockSelf.node.enrollments.htmlUrl)
+    expect(link).toHaveAttribute('href', mockSelf.enrollments.htmlUrl)
   })
 
   it('should not link the current_user to the user detail page when clicking a name that is not their own', async () => {
     const container = setup(getRosterQueryMock({mockUsers}))
     const cells = await container.findAllByTestId('roster-table-name-cell')
-    const names = mockUsers.map(user => user.node.name)
+    const names = mockUsers.map(user => user.name)
     cells.forEach((cell, index) => {
       const nameMatch = new RegExp(names[index])
       const button = within(cell).getByRole('button', {name: nameMatch})
@@ -208,9 +208,9 @@ describe('RosterTable', () => {
     const container = setup(getRosterQueryMock({mockUsers}))
     const rows = await container.findAllByTestId('roster-table-data-row')
     const lastActivityByUser = mockUsers.map(user => {
-      return user.node.enrollments[0].type === 'ObserverEnrollment'
+      return user.enrollments[0].type === 'ObserverEnrollment'
         ? null
-        : user.node.enrollments[0].lastActivityAt
+        : user.enrollments[0].lastActivityAt
     })
     rows.forEach((row, index) => {
       const lastActivity = queryAllByText(row, datetimePattern)
@@ -221,7 +221,7 @@ describe('RosterTable', () => {
   it('should display users total activity time only if total time is greater than zero', async () => {
     const container = setup(getRosterQueryMock({mockUsers}))
     const rows = await container.findAllByTestId('roster-table-data-row')
-    const totalActivityByUser = mockUsers.map(user => user.node.enrollments[0].totalActivityTime)
+    const totalActivityByUser = mockUsers.map(user => user.enrollments[0].totalActivityTime)
     rows.forEach((row, index) => {
       const totalActivity = queryAllByText(row, /^[0-9]+(:[0-5][0-9]){1,2}$/) // 00:00 or 00:00:00
       expect(totalActivity).toHaveLength(totalActivityByUser[index] ? 1 : 0)
@@ -231,7 +231,7 @@ describe('RosterTable', () => {
   it('should list the user pronouns if available', async () => {
     const container = setup(getRosterQueryMock({mockUsers}))
     const cells = await container.findAllByTestId('roster-table-name-cell')
-    const userPronouns = mockUsers.map(user => user.node.pronouns)
+    const userPronouns = mockUsers.map(user => user.pronouns)
     cells.forEach((cell, index) => {
       if (userPronouns[index]) {
         const pronounMatch = new RegExp(userPronouns[index], 'i')
@@ -243,7 +243,7 @@ describe('RosterTable', () => {
   it('should list the user status if not active', async () => {
     const container = setup(getRosterQueryMock({mockUsers}))
     const cells = await container.findAllByTestId('roster-table-name-cell')
-    const userStatus = mockUsers.map(user => user.node.enrollments[0].state)
+    const userStatus = mockUsers.map(user => user.enrollments[0].state)
     cells.forEach((cell, index) => {
       if (userStatus[index] !== ACTIVE_STATE) {
         const status = PILL_MAP[userStatus[index]].text
@@ -261,7 +261,7 @@ describe('RosterTable', () => {
     expect(container.queryAllByTestId('colheader-login-id')).toHaveLength(0)
 
     // Check there is no login id data
-    const loginIdByUser = mockUsers.map(user => user.node.enrollments[0].loginId)
+    const loginIdByUser = mockUsers.map(user => user.enrollments[0].loginId)
     rows.forEach((row, index) => {
       loginIdByUser[index] && expect(queryAllByText(row, loginIdByUser[index])).toHaveLength(0)
     })
@@ -271,7 +271,7 @@ describe('RosterTable', () => {
     window.ENV.permissions.read_sis = false
     const container = setup(getRosterQueryMock({mockUsers}))
     const rows = await container.findAllByTestId('roster-table-data-row')
-    const sisIdByUser = mockUsers.map(user => user.node.enrollments[0].sisId)
+    const sisIdByUser = mockUsers.map(user => user.enrollments[0].sisId)
 
     // Check there is no column header
     expect(container.queryAllByTestId('colheader-sis-id')).toHaveLength(0)