From a3790f177c7a2b674587a86a854b88d9ceaa2899 Mon Sep 17 00:00:00 2001 From: Landon Gilbert-Bland Date: Thu, 6 Jun 2019 09:53:05 -0600 Subject: [PATCH] Fix submission histories in graphql Fixes COMMS-2090 Test Plan: - Create an assignment - Make a few submission attempts on the assignment - Grade the latest attempt as the teacher - Make a few more attempts on the assignment. - Run a graphql query like ``` node(id: ) { ... on Submission { submissionHistoriesConnection(includeCurrentSubmission: true) { pageInfo { startCursor endCursor hasPreviousPage hasNextPage } edges { cursor node { attempt submittedAt } } } } } ``` - Make sure there are no duplicate attempts - Make sure the cursors are all properly set and unique - change the includeCurrentSubmission to true in the query, and make sure that the most current submission is no longer returned from the graphql query results - Make sure the student view for assignment v2 still works Change-Id: Ic83a78dc0384b96014d1a407b1982ad8078ba58d Reviewed-on: https://gerrit.instructure.com/196763 Reviewed-by: Carl Kibler Reviewed-by: Ryan Norton Reviewed-by: Steven Burnett Tested-by: Jenkins QA-Review: Landon Gilbert-Bland Product-Review: Landon Gilbert-Bland --- app/graphql/canvas_schema.rb | 2 + app/graphql/graphql_custom_connections.rb | 39 +++++++++++ app/graphql/types/submission_type.rb | 64 ++++-------------- app/jsx/assignments_2/student/StudentView.js | 2 +- .../assignments_2/student/assignmentData.js | 10 +-- .../student/components/StudentContent.js | 8 +-- .../__tests__/StudentContent.test.js | 16 ++--- app/jsx/assignments_2/student/test-utils.js | 4 +- .../teacher/components/StudentsTable.js | 16 +---- schema.graphql | 6 ++ spec/graphql/types/submission_type_spec.rb | 65 ++++++++----------- 11 files changed, 111 insertions(+), 121 deletions(-) create mode 100644 app/graphql/graphql_custom_connections.rb diff --git a/app/graphql/canvas_schema.rb b/app/graphql/canvas_schema.rb index 66d45823546..f8d9a9d9f74 100644 --- a/app/graphql/canvas_schema.rb +++ b/app/graphql/canvas_schema.rb @@ -16,6 +16,8 @@ # with this program. If not, see . # +require 'graphql_custom_connections' + class CanvasSchema < GraphQL::Schema use GraphQL::Execution::Interpreter diff --git a/app/graphql/graphql_custom_connections.rb b/app/graphql/graphql_custom_connections.rb new file mode 100644 index 00000000000..ff4bdb887c6 --- /dev/null +++ b/app/graphql/graphql_custom_connections.rb @@ -0,0 +1,39 @@ +# +# Copyright (C) 2019 - 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 . +# + +class PatchedArrayConnection < GraphQL::Relay::ArrayConnection + # The default ArrayConnection uses `find_index(item)` which uses `==` to get + # the index. Unfortunately submission histories are saved through versionable + # which returns an array of submission histories that all share the same id, + # and active record overrides the `==` method to check for equality based on + # said id. When dealing with submissions, change the comparator to look at the + # submitted_at time (what submission#submission_history is doing) instead of + # by id so cursors don't break. + def cursor_from_submission_node(submission) + submission_idx = sliced_nodes.find_index { |i| i.submitted_at.to_i == submission.submitted_at.to_i } + idx = (after ? index_from_cursor(after) : 0) + submission_idx + 1 + encode(idx.to_s) + end + + def cursor_from_node(item) + return cursor_from_submission_node(item) if item.class.name == 'Submission' + super + end +end + +GraphQL::Relay::BaseConnection.register_connection_implementation(Array, PatchedArrayConnection) diff --git a/app/graphql/types/submission_type.rb b/app/graphql/types/submission_type.rb index e18f6ebaf5b..147f66ee7d5 100644 --- a/app/graphql/types/submission_type.rb +++ b/app/graphql/types/submission_type.rb @@ -16,33 +16,6 @@ # with this program. If not, see . # -class SubmissionHistoryEdgeType < GraphQL::Types::Relay::BaseEdge - node_type(Types::SubmissionHistoryType) - - def node - # Submission models or version ids for submission models can be handled here. - if object.node.is_a? Integer - Loaders::IDLoader.for(Version).load(object.node).then(&:model) - else - object.node - end - end -end - -class SubmissionHistoryConnection < GraphQL::Types::Relay::BaseConnection - edge_type(SubmissionHistoryEdgeType) - - def nodes - # Submission models or version ids for submission models can be handled here. - node_values = super - version_ids, submissions = node_values.partition { |n| n.is_a? Integer } - Loaders::IDLoader.for(Version).load_many(version_ids).then do |versions| - versions.map(&:model) + submissions - end - end -end - - module Types class SubmissionType < ApplicationObjectType graphql_name 'Submission' @@ -58,30 +31,21 @@ module Types # be added to interfaces/submission_interface.rb so that they work for # both submissions and submission histories - field :submission_histories_connection, SubmissionHistoryConnection, null: true, connection: true - def submission_histories_connection - # There is not a version saved for submission attempt zero, so we fake it - # here. If there are no versions, we are still on attempt zero and can use - # the current submission, otherwise we fudge it with a model that is not - # actually persisted to the database. - submission_zero = if object.version_ids.empty? - object - else - Submission.new( - id: object.id, - assignment_id: object.assignment_id, - user_id: object.user_id, - submission_type: object.submission_type, - workflow_state: 'unsubmitted', - created_at: object.created_at, - updated_at: object.created_at, # Don't use the current updated_at time - group_id: object.group_id, - attempt: 0, - context_code: object.context_code - ) + field :submission_histories_connection, SubmissionHistoryType.connection_type, null: true do + argument :include_current_submission, Boolean, <<~DESC, required: false, default_value: true + If the most current submission should be included in the submission + history results. Defaults to true. + DESC + end + def submission_histories_connection(include_current_submission: true) + Promise.all([ + load_association(:versions), + load_association(:assignment) + ]).then do + histories = object.submission_history + histories.pop unless include_current_submission + histories end - - object.version_ids + [submission_zero] end end end diff --git a/app/jsx/assignments_2/student/StudentView.js b/app/jsx/assignments_2/student/StudentView.js index 5a0d5cf45fa..2bde6b2fe28 100644 --- a/app/jsx/assignments_2/student/StudentView.js +++ b/app/jsx/assignments_2/student/StudentView.js @@ -64,7 +64,7 @@ const StudentView = props => ( return fetchMore({ query: NEXT_SUBMISSION, variables: { - cursor: submission.submissionHistoriesConnection.pageInfo.endCursor, + cursor: submission.submissionHistoriesConnection.pageInfo.startCursor, submissionID: submission.submissionHistoriesConnection.edges[0].node.rootId }, updateQuery: (previousResult, {fetchMoreResult}) => { diff --git a/app/jsx/assignments_2/student/assignmentData.js b/app/jsx/assignments_2/student/assignmentData.js index 8d5d74d03e3..b6ed9e069c8 100644 --- a/app/jsx/assignments_2/student/assignmentData.js +++ b/app/jsx/assignments_2/student/assignmentData.js @@ -65,8 +65,8 @@ function attachmentFields() { function submissionFields() { return ` pageInfo { - hasNextPage - endCursor + hasPreviousPage + startCursor } edges { cursor @@ -147,7 +147,7 @@ export const STUDENT_VIEW_QUERY = gql` filter: {states: [unsubmitted, graded, pending_review, submitted]} ) { nodes { - submissionHistoriesConnection(first: 1) { + submissionHistoriesConnection(last: 1) { ${submissionFields()} } } @@ -185,7 +185,7 @@ export const CREATE_SUBMISSION = gql` mutation CreateSubmission($id: ID!, $type: OnlineSubmissionType!, $fileIds: [ID!]) { createSubmission(input: {assignmentId: $id, submissionType: $type, fileIds: $fileIds}) { submission { - submissionHistoriesConnection(first: 1) { + submissionHistoriesConnection(last: 1) { ${submissionFields()} } } @@ -201,7 +201,7 @@ export const NEXT_SUBMISSION = gql` query NextSubmission($submissionID: ID!, $cursor: String) { legacyNode(_id: $submissionID, type: Submission) { ... on Submission { - submissionHistoriesConnection(after: $cursor, first: 1) { + submissionHistoriesConnection(before: $cursor, last: 1) { ${submissionFields()} } } diff --git a/app/jsx/assignments_2/student/components/StudentContent.js b/app/jsx/assignments_2/student/components/StudentContent.js index 0e4ece9dabe..c90387ce782 100644 --- a/app/jsx/assignments_2/student/components/StudentContent.js +++ b/app/jsx/assignments_2/student/components/StudentContent.js @@ -34,8 +34,8 @@ class StudentContent extends React.Component { assignment: AssignmentShape, onLoadMore: func, pageInfo: shape({ - hasNextPage: bool, - endCursor: string + hasPreviousPage: bool, + startCursor: string }), submissionHistoryEdges: arrayOf( shape({ @@ -91,7 +91,7 @@ class StudentContent extends React.Component { } hasPrevSubmission = () => { - if (this.props.pageInfo.hasNextPage) { + if (this.props.pageInfo.hasPreviousPage) { return true } @@ -121,7 +121,7 @@ class StudentContent extends React.Component { if (currentIndex > 0) { const prevCursor = state.orderedCursors[currentIndex - 1] return {displayedCursor: prevCursor} - } else if (props.pageInfo.hasNextPage && !state.loadingMore) { + } else if (props.pageInfo.hasPreviousPage && !state.loadingMore) { this.props.onLoadMore() return {loadingMore: true} } else { diff --git a/app/jsx/assignments_2/student/components/__tests__/StudentContent.test.js b/app/jsx/assignments_2/student/components/__tests__/StudentContent.test.js index 689a65949b9..b18b36c035c 100644 --- a/app/jsx/assignments_2/student/components/__tests__/StudentContent.test.js +++ b/app/jsx/assignments_2/student/components/__tests__/StudentContent.test.js @@ -38,14 +38,14 @@ function mockSubmissionHistoryEdges(count, opts = {}) { function mockPageInfo(options = {}) { const optsWithDefaults = { - hasNextPage: false, - endCursor: 1, + hasPreviousPage: false, + startCursor: 1, ...options } return { - hasNextPage: optsWithDefaults.hasNextPage, - endCursor: btoa(optsWithDefaults.endCursor.toString()) + hasPreviousPage: optsWithDefaults.hasPreviousPage, + startCursor: btoa(optsWithDefaults.startCursor.toString()) } } @@ -240,7 +240,7 @@ describe('Previous Button', () => { const props = { assignment: mockAssignment(), submissionHistoryEdges: mockSubmissionHistoryEdges(1), - pageInfo: mockPageInfo({hasNextPage: false}), + pageInfo: mockPageInfo({hasPreviousPage: false}), onLoadMore: () => {} } const {getByText} = render() @@ -264,7 +264,7 @@ describe('Previous Button', () => { const props = { assignment: mockAssignment(), submissionHistoryEdges: mockSubmissionHistoryEdges(1), - pageInfo: mockPageInfo({hasNextPage: true}), + pageInfo: mockPageInfo({hasPreviousPage: true}), onLoadMore: () => {} } const {getByText} = render() @@ -304,7 +304,7 @@ describe('Previous Button', () => { const props = { assignment: mockAssignment(), submissionHistoryEdges: mockSubmissionHistoryEdges(1), - pageInfo: mockPageInfo({hasNextPage: true}), + pageInfo: mockPageInfo({hasPreviousPage: true}), onLoadMore: mockedOnLoadMore } const {getByText} = render() @@ -318,7 +318,7 @@ describe('Previous Button', () => { const props = { assignment: mockAssignment(), submissionHistoryEdges: mockSubmissionHistoryEdges(1), - pageInfo: mockPageInfo({hasNextPage: true}), + pageInfo: mockPageInfo({hasPreviousPage: true}), onLoadMore: mockedOnLoadMore } const {getByText} = render() diff --git a/app/jsx/assignments_2/student/test-utils.js b/app/jsx/assignments_2/student/test-utils.js index 03d13528c3e..c51ff25f3c0 100644 --- a/app/jsx/assignments_2/student/test-utils.js +++ b/app/jsx/assignments_2/student/test-utils.js @@ -241,8 +241,8 @@ export function mockSubmissionHistoriesConnection() { __typename: 'SubmissionHistoryConnection', pageInfo: { __typename: 'PageInfo', - hasNextPage: false, - endCursor: btoa('1') + hasPreviousPage: false, + startCursor: btoa('1') }, edges: [ { diff --git a/app/jsx/assignments_2/teacher/components/StudentsTable.js b/app/jsx/assignments_2/teacher/components/StudentsTable.js index 0ca9fe90b8d..1ca7c09a4f8 100644 --- a/app/jsx/assignments_2/teacher/components/StudentsTable.js +++ b/app/jsx/assignments_2/teacher/components/StudentsTable.js @@ -92,20 +92,10 @@ export default class StudentsTable extends React.Component { ) } - // This becomes unnecessary after near-future GraphQL changes - submissionAttempts(submission) { - return ( - submission.submissionHistories && - submission.submissionHistories.nodes.filter(function(item) { - return item.attempt !== 0 - }) - ) - } - renderAttemptsColumn(student) { const assignmentLid = this.props.assignment.lid const courseLid = this.props.assignment.course.lid - return this.submissionAttempts(student.submission).map(attempt => { + return student.submission.submissionHistories.nodes.map(attempt => { const viewLink = `/courses/${courseLid}/assignments/${assignmentLid}/submissions/${ student.lid }/?submittedAt=${attempt.submittedAt}` @@ -120,7 +110,7 @@ export default class StudentsTable extends React.Component { } renderScoreColumn(student) { - return this.submissionAttempts(student.submission).map(attempt => { + return student.submission.submissionHistories.nodes.map(attempt => { const validScore = attempt.score || attempt.score === 0 return ( @@ -134,7 +124,7 @@ export default class StudentsTable extends React.Component { } renderSubmittedAtColumn(student) { - return this.submissionAttempts(student.submission).map(attempt => { + return student.submission.submissionHistories.nodes.map(attempt => { return ( diff --git a/schema.graphql b/schema.graphql index 96e94fa5335..a888ef2d636 100644 --- a/schema.graphql +++ b/schema.graphql @@ -2181,6 +2181,12 @@ type Submission implements Node & SubmissionInterface & Timestamped { """ first: Int + """ + If the most current submission should be included in the submission + history results. Defaults to true. + """ + includeCurrentSubmission: Boolean = true + """ Returns the last _n_ elements from the list. """ diff --git a/spec/graphql/types/submission_type_spec.rb b/spec/graphql/types/submission_type_spec.rb index 31c60b7d725..e6a531da077 100644 --- a/spec/graphql/types/submission_type_spec.rb +++ b/spec/graphql/types/submission_type_spec.rb @@ -165,58 +165,47 @@ describe Types::SubmissionType do describe 'submission histories connection' do before(:once) do - # In the world code path that the initial submission takes, there is a bulk - # insert directly into the database, which causes no version to be saved - # for the zero submission. Delete the version_ids here to simulate that. - @submission.update!(version_ids: []) + assignment = @course.assignments.create! name: "asdf2", points_possible: 10 + @submission1 = assignment.submit_homework(@student, body: 'Attempt 1', submitted_at: 2.hours.ago) + @submission2 = assignment.submit_homework(@student, body: 'Attempt 2', submitted_at: 1.hour.ago) + @submission3 = assignment.submit_homework(@student, body: 'Attempt 3') end - it 'works when there are no versions saved' do + let(:submission_history_type) { GraphQLTypeTester.new(@submission3, current_user: @teacher) } + + it 'returns the submission histories' do expect( - submission_type.resolve('submissionHistoriesConnection { nodes { attempt }}') - ).to eq [0] + submission_history_type.resolve('submissionHistoriesConnection { nodes { attempt }}') + ).to eq [1, 2, 3] end - it 'includes the zero submission when there are versions' do - @submission.update!(attempt: 1) + it 'properly handles cursors for submission histories' do expect( - submission_type.resolve('submissionHistoriesConnection { nodes { attempt }}') - ).to eq [1, 0] + submission_history_type.resolve('submissionHistoriesConnection { edges { cursor }}') + ).to eq ["MQ", "Mg", "Mw"] end - context 'custom pagination' do - before(:once) do - @submission.update!(attempt: 1) + context 'include_current_submission argument' do + it 'includes the current submission history by default' do + expect( + submission_history_type.resolve('submissionHistoriesConnection { nodes { attempt }}') + ).to eq [1, 2, 3] end - it 'works for nodes' do + it 'includes the current submission history when true' do expect( - submission_type.resolve('submissionHistoriesConnection(first: 1) { nodes { attempt }}') - ).to eq [1] + submission_history_type.resolve( + 'submissionHistoriesConnection(includeCurrentSubmission: true) { nodes { attempt }}' + ) + ).to eq [1, 2, 3] end - it 'works for edges node' do + it 'does not includes the current submission history when false' do expect( - submission_type.resolve('submissionHistoriesConnection(first: 1) { edges { node { attempt }}}') - ).to eq [1] - end - - it 'works for edges cursor' do - expect( - submission_type.resolve('submissionHistoriesConnection(first: 1) { edges { cursor }}') - ).to eq ["MQ"] # Base64 encoded 1, per the graphql gem impmenetation of array paginating - end - - it 'works for pageInfo endCursor' do - expect( - submission_type.resolve('submissionHistoriesConnection(first: 1) { pageInfo { endCursor }}') - ).to eq "MQ" # Base64 encoded 1, per the graphql gem impmenetation of array paginating - end - - it 'works for pageInfo hasNextPage' do - expect( - submission_type.resolve('submissionHistoriesConnection(first: 1) { pageInfo { hasNextPage }}') - ).to eq true + submission_history_type.resolve( + 'submissionHistoriesConnection(includeCurrentSubmission: false) { nodes { attempt }}' + ) + ).to eq [1, 2] end end end