ylqjgm
/
canvas-lms
mirror of https://github.com/instructure/canvas-lms.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

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: <submissionID>) {
      ... 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 <ckibler@instructure.com>
Reviewed-by: Ryan Norton <rnorton@instructure.com>
Reviewed-by: Steven Burnett <sburnett@instructure.com>
Tested-by: Jenkins
QA-Review: Landon Gilbert-Bland <lbland@instructure.com>
Product-Review: Landon Gilbert-Bland <lbland@instructure.com>
This commit is contained in:
Landon Gilbert-Bland 2019-06-06 09:53:05 -06:00
parent c331a1a5dd
commit a3790f177c
11 changed files with 111 additions and 121 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

2
app/graphql/canvas_schema.rb
View File

@ -16,6 +16,8 @@
# with this program. If not, see <http://www.gnu.org/licenses/>.
#
require 'graphql_custom_connections'
class CanvasSchema < GraphQL::Schema
use GraphQL::Execution::Interpreter

39
app/graphql/graphql_custom_connections.rb Normal file
View File

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

64
app/graphql/types/submission_type.rb
View File

@ -16,33 +16,6 @@
# with this program. If not, see <http://www.gnu.org/licenses/>.
#
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

2
app/jsx/assignments_2/student/StudentView.js
View File

@ -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}) => {

10
app/jsx/assignments_2/student/assignmentData.js
View File

@ -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()}
}
}

8
app/jsx/assignments_2/student/components/StudentContent.js
View File

@ -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 {

16
app/jsx/assignments_2/student/components/__tests__/StudentContent.test.js
View File

@ -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(<StudentContent {...props} />)
@ -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(<StudentContent {...props} />)
@ -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(<StudentContent {...props} />)
@ -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(<StudentContent {...props} />)

4
app/jsx/assignments_2/student/test-utils.js
View File

@ -241,8 +241,8 @@ export function mockSubmissionHistoriesConnection() {
__typename: 'SubmissionHistoryConnection',
pageInfo: {
__typename: 'PageInfo',
hasNextPage: false,
endCursor: btoa('1')
hasPreviousPage: false,
startCursor: btoa('1')
},
edges: [
{

16
app/jsx/assignments_2/teacher/components/StudentsTable.js
View File

@ -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 (
<View as="div" margin="0 0 x-small" key={attempt.attempt}>
@ -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 (
<View as="div" margin="0 0 x-small" key={attempt.attempt}>
<FriendlyDatetime dateTime={attempt.submittedAt} format={I18n.t('#date.formats.full')} />

6
schema.graphql
View File

@ -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.
"""

65
spec/graphql/types/submission_type_spec.rb
View File

@ -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