From c62a32194ec07ce465fac8cb1250e861014a22a1 Mon Sep 17 00:00:00 2001 From: Nic Nolan Date: Thu, 11 Aug 2022 14:10:05 -0700 Subject: [PATCH] remove concluded enrollments from course roster table MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 Reviewed-by: Omar Soto-Fortuño Product-Review: Omar Soto-Fortuño QA-Review: Jason Gillett --- app/graphql/types/user_type.rb | 12 +++- spec/graphql/types/user_type_spec.rb | 6 ++ ui/features/course_people/graphql/Mocks.js | 61 +++++++++---------- ui/features/course_people/graphql/Queries.js | 48 +++++++-------- .../containers/RosterTable/RosterTable.js | 4 +- .../RosterTable/__tests__/RosterTable.test.js | 20 +++--- 6 files changed, 79 insertions(+), 72 deletions(-) diff --git a/app/graphql/types/user_type.rb b/app/graphql/types/user_type.rb index 5e90f09b8a5..ac17b7abcca 100644 --- a/app/graphql/types/user_type.rb +++ b/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) diff --git a/spec/graphql/types/user_type_spec.rb b/spec/graphql/types/user_type_spec.rb index 4c146d38615..35467b94bd9 100644 --- a/spec/graphql/types/user_type_spec.rb +++ b/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 diff --git a/ui/features/course_people/graphql/Mocks.js b/ui/features/course_people/graphql/Mocks.js index 0a19b458a36..6c7a23e9497 100644 --- a/ui/features/course_people/graphql/Mocks.js +++ b/ui/features/course_people/graphql/Mocks.js @@ -35,37 +35,34 @@ export const mockUser = ({ sectionName = 'Section 1', additionalEnrollments = [] } = {}) => ({ - node: { - name, - _id, - id: Buffer.from(`User-${_id}`).toString('base64'), - sisId, - avatarUrl, - pronouns, - loginId, - __typename: 'user', - enrollments: [ - { - id: Buffer.from(`Enrollment-${_id}`).toString('base64'), - type: enrollmentType, - state: enrollmentStatus, - lastActivityAt, - htmlUrl: `http://test.host/courses/${courseID}/users/${_id}`, - totalActivityTime, - canBeRemoved, - associatedUser: null, // always null for user's own enrollment - __typename: 'enrollment', - section: { - _id: sectionID, - id: Buffer.from(`Section-${sectionID}`).toString('base64'), - name: sectionName, - __typename: 'section' - } - }, - ...additionalEnrollments - ] - }, - __typename: 'usersConnectionEdge' + name, + _id, + id: Buffer.from(`User-${_id}`).toString('base64'), + sisId, + avatarUrl, + pronouns, + loginId, + __typename: 'user', + enrollments: [ + { + id: Buffer.from(`Enrollment-${_id}`).toString('base64'), + type: enrollmentType, + state: enrollmentStatus, + lastActivityAt, + htmlUrl: `http://test.host/courses/${courseID}/users/${_id}`, + totalActivityTime, + canBeRemoved, + associatedUser: null, // always null for user's own enrollment + __typename: 'enrollment', + section: { + _id: sectionID, + id: Buffer.from(`Section-${sectionID}`).toString('base64'), + name: sectionName, + __typename: 'section' + } + }, + ...additionalEnrollments + ] }) 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' diff --git a/ui/features/course_people/graphql/Queries.js b/ui/features/course_people/graphql/Queries.js index 3d2e50e4629..6e48b51263e 100644 --- a/ui/features/course_people/graphql/Queries.js +++ b/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 { - node { - name - _id + nodes { + name + _id + id + sisId + avatarUrl + pronouns + loginId + enrollments(courseId: $courseID, excludeConcluded: true) { id - sisId - avatarUrl - pronouns - loginId - enrollments(courseId: $courseID) { + type + state + lastActivityAt + htmlUrl + totalActivityTime + canBeRemoved + associatedUser { + _id id - type - state - lastActivityAt - htmlUrl - totalActivityTime - canBeRemoved - associatedUser { - _id - id - name - } - section { - _id - id - name - } + name + } + section { + _id + id + name } } } diff --git a/ui/features/course_people/react/containers/RosterTable/RosterTable.js b/ui/features/course_people/react/containers/RosterTable/RosterTable.js index 39943a3ff9d..148293f0a86 100644 --- a/ui/features/course_people/react/containers/RosterTable/RosterTable.js +++ b/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 {name, _id, sisId, enrollments, loginId, avatarUrl, pronouns} = edge.node + const tableRows = data.course.usersConnection.nodes.map(node => { + const {name, _id, sisId, enrollments, loginId, avatarUrl, pronouns} = node const {totalActivityTime, htmlUrl, state} = enrollments[0] const sectionNames = enrollments.map(enrollment => { diff --git a/ui/features/course_people/react/containers/RosterTable/__tests__/RosterTable.test.js b/ui/features/course_people/react/containers/RosterTable/__tests__/RosterTable.test.js index 8516af01271..6e580cde1ea 100644 --- a/ui/features/course_people/react/containers/RosterTable/__tests__/RosterTable.test.js +++ b/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)