Add createSubmissionComment mutation

Fixes COMMS-1721

Test Plan:
  * Create and publish an assignment.
  * Using the assignment1 page, add some submission comments that
    include media objects and file attachments. Grab the ids of the
    media objects and file attachments for later in the rails console:

   ```
   Attachment.last.id
   MediaObject.last.media_id
   ```

  * Run a query like the following, filling in the submission id, file
    ids, media object id, and attempt:

  ```
  mutation {
    createSubmissionComment(input: {
      submissionId: "151",
      comment: "Comment from graphql",
      attempt: 1,
      fileIds: [1],
      mediaObjectId: "1"
    }) {
      submissionComment {
        _id
        comment
        attachments {
          _id
          displayName
          mimeClass
          contentType
        }
        mediaObject {
          _id
          mediaType
          title
        }
      }
    }
  }
  ```

  * Make sure the query returns the data as expected, and you can see
    any new comments made in the old assignments page, complete with
    attachments and media comments.
  * Test the various inputs that can be passed to that mutation, make
    sure you can't break them.

Change-Id: Ibf86938ec478f26a322ed96429382416a15d3c48
Reviewed-on: https://gerrit.instructure.com/185943
Tested-by: Jenkins
Reviewed-by: Steven Burnett <sburnett@instructure.com>
QA-Review: Aaron Hsu <ahsu@instructure.com>
Product-Review: Landon Gilbert-Bland <lbland@instructure.com>

app/graphql/mutations/base_mutation.rb

@@ -41,12 +41,8 @@ class Mutations::BaseMutation < GraphQL::Schema::Mutation
     context[:session]
   end
 
-  def authorized_action?(obj, perm)
-    if obj.grants_right?(current_user, session, perm)
-      true
-    else
-      raise GraphQL::ExecutionError, "not found"
-    end
+  def verify_authorized_action!(obj, perm)
+    raise GraphQL::ExecutionError, 'not found' unless obj.grants_right?(current_user, session, perm)
   end
 
   private

app/graphql/mutations/create_group_in_set.rb

@@ -27,14 +27,13 @@ class Mutations::CreateGroupInSet < Mutations::BaseMutation
   def resolve(input:)
     category_id = GraphQLHelpers.parse_relay_or_legacy_id(input[:group_set_id], "GroupSet")
     set = GroupCategory.find(category_id)
-    if authorized_action?(set.context, :manage_groups)
+    verify_authorized_action!(set.context, :manage_groups)
     group = set.groups.build(name: input[:name], context: set.context)
     if group.save
       {group: group}
     else
       errors_for(group)
     end
-    end
   rescue ActiveRecord::RecordNotFound
     raise GraphQL::ExecutionError, "not found"
   end

app/graphql/mutations/create_submission_comment.rb

@@ -0,0 +1,65 @@
+#
+# 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 Mutations::CreateSubmissionComment < Mutations::BaseMutation
+  graphql_name 'CreateSubmissionComment'
+
+  argument :submission_id, ID, required: true, prepare: GraphQLHelpers.relay_or_legacy_id_prepare_func('Submission')
+  argument :attempt, Integer, required: false
+  argument :comment, String, required: true
+  argument :file_ids, [ID], required: false, prepare: GraphQLHelpers.relay_or_legacy_ids_prepare_func('Attachment')
+  argument :media_object_id, ID, required: false
+
+  field :submission_comment, Types::SubmissionCommentType, null: true
+
+  def resolve(input:)
+    submission = Submission.find input[:submission_id]
+    verify_authorized_action!(submission, :comment)
+
+    opts = {
+      attempt: input[:attempt],
+      author: current_user,
+      comment: input[:comment]
+    }
+
+    if input[:media_object_id].present?
+      media_objects = MediaObject.by_media_id(input[:media_object_id])
+      raise GraphQL::ExecutionError, 'not found' if media_objects.empty?
+      opts[:media_comment_id] = input[:media_object_id]
+    end
+
+    file_ids = (input[:file_ids] || []).uniq
+    unless file_ids.empty?
+      attachments = Attachment.where(id: file_ids).to_a
+      raise GraphQL::ExecutionError, 'not found' unless attachments.length == file_ids.length
+      attachments.each do |a|
+        verify_authorized_action!(a, :attach_to_submission_comment)
+        a.ok_for_submission_comment = true
+      end
+      opts[:attachments] = attachments
+    end
+
+    assignment = submission.assignment
+    comment = assignment.add_submission_comment(submission.user, opts).first
+    {submission_comment: comment}
+  rescue ActiveRecord::RecordInvalid => invalid
+    errors_for(invalid.record)
+  rescue ActiveRecord::RecordNotFound
+    raise GraphQL::ExecutionError, 'not found'
+  end
+end

app/graphql/mutations/hide_assignment_grades.rb

@@ -32,7 +32,7 @@ class Mutations::HideAssignmentGrades < Mutations::BaseMutation
       raise GraphQL::ExecutionError, "not found"
     end
 
-    return nil unless authorized_action?(assignment, :grade)
+    verify_authorized_action!(assignment, :grade)
     raise GraphQL::ExecutionError, "Post Policies feature not enabled" unless course.feature_enabled?(:post_policies)
 
     unless assignment.grades_published?

app/graphql/mutations/hide_assignment_grades_for_sections.rb

@@ -35,7 +35,7 @@ class Mutations::HideAssignmentGradesForSections < Mutations::BaseMutation
       raise GraphQL::ExecutionError, "not found"
     end
 
-    return nil unless authorized_action?(assignment, :grade)
+    verify_authorized_action!(assignment, :grade)
     raise GraphQL::ExecutionError, "Post Policies feature not enabled" unless course.feature_enabled?(:post_policies)
 
     unless assignment.grades_published?

app/graphql/mutations/post_assignment_grades.rb

@@ -32,7 +32,7 @@ class Mutations::PostAssignmentGrades < Mutations::BaseMutation
       raise GraphQL::ExecutionError, "not found"
     end
 
-    return nil unless authorized_action?(assignment, :grade)
+    verify_authorized_action!(assignment, :grade)
     raise GraphQL::ExecutionError, "Post Policies feature not enabled" unless course.feature_enabled?(:post_policies)
 
     unless assignment.grades_published?

app/graphql/mutations/post_assignment_grades_for_sections.rb

@@ -35,7 +35,7 @@ class Mutations::PostAssignmentGradesForSections < Mutations::BaseMutation
       raise GraphQL::ExecutionError, "not found"
     end
 
-    return nil unless authorized_action?(assignment, :grade)
+    verify_authorized_action!(assignment, :grade)
     raise GraphQL::ExecutionError, "Post Policies feature not enabled" unless course.feature_enabled?(:post_policies)
 
     unless assignment.grades_published?

app/graphql/mutations/set_assignment_post_policy.rb

@@ -32,7 +32,7 @@ class Mutations::SetAssignmentPostPolicy < Mutations::BaseMutation
       raise GraphQL::ExecutionError, "An assignment with that id does not exist"
     end
 
-    return unless authorized_action?(course, :manage_grades)
+    verify_authorized_action!(course, :manage_grades)
 
     policy = PostPolicy.find_or_create_by(course: course, assignment: assignment)
     policy.update!(post_manually: input[:post_manually])

app/graphql/mutations/set_course_post_policy.rb

@@ -37,7 +37,7 @@ class Mutations::SetCoursePostPolicy < Mutations::BaseMutation
       raise GraphQL::ExecutionError, "A course with that id does not exist"
     end
 
-    return unless authorized_action?(course, :manage_grades)
+    verify_authorized_action!(course, :manage_grades)
 
     policy = PostPolicy.find_or_create_by(course: course, assignment_id: nil)
     policy.update!(post_manually: input[:post_manually])

app/graphql/mutations/set_override_score.rb

@@ -45,8 +45,8 @@ class Mutations::SetOverrideScore < Mutations::BaseMutation
     current_enrollments.each do |enrollment|
       score = enrollment.find_score(score_params)
       raise ActiveRecord::RecordNotFound if score.blank?
+      verify_authorized_action!(score.course, :manage_grades)
 
-      if authorized_action?(score.course, :manage_grades)
       score.update(override_score: input[:override_score])
       next unless enrollment == requested_enrollment
 
@@ -56,7 +56,6 @@ class Mutations::SetOverrideScore < Mutations::BaseMutation
         errors_for(score)
       end
     end
-    end
 
     return_value
   rescue ActiveRecord::RecordNotFound

app/graphql/types/mutation_type.rb

@@ -53,4 +53,5 @@ class Types::MutationType < Types::ApplicationObjectType
     existing assignment post policies.
   DESC
   field :update_assignment, mutation: Mutations::UpdateAssignment
+  field :create_submission_comment, mutation: Mutations::CreateSubmissionComment
 end

app/graphql/types/submission_type.rb

@@ -55,6 +55,11 @@ module Types
     end
     private :protect_submission_grades
 
+    field :attempt, Integer, null: false
+    def attempt
+      submission.attempt || 0 # Nil in database, make it 0 here for easier api
+    end
+
     field :comments_connection, SubmissionCommentType.connection_type, null: true do
       argument :filter, Types::SubmissionCommentFilterInputType, required: false
     end

app/models/assignment.rb

@@ -1906,13 +1906,15 @@ class Assignment < ActiveRecord::Base
     end
   end
 
-  # Update at this point is solely used for commenting on the submission
-  def update_submission(original_student, opts={})
+  def update_submission_runner(original_student, opts={})
     raise "Student Required" unless original_student
 
     group, students = group_students(original_student)
     opts[:author] ||= opts[:commenter] || opts[:user_id].present? && User.find_by(id: opts[:user_id])
-    res = []
+    res = {
+      comments: [],
+      submissions: []
+    }
 
     # Only teachers (those who can manage grades) can have hidden comments
     opts[:hidden] = muted? && self.context.grants_right?(opts[:author], :manage_grades) unless opts.key?(:hidden)
@@ -1927,23 +1929,37 @@ class Assignment < ActiveRecord::Base
     ensure_grader_can_adjudicate(grader: opts[:author], provisional: opts[:provisional], occupy_slot: true) do
       if opts[:comment] && Canvas::Plugin.value_to_boolean(opts[:group_comment])
         uuid = CanvasSlug.generate_securish_uuid
-        res = find_or_create_submissions(students) do |submission|
-          save_comment_to_submission(submission, group, opts, uuid)
+        find_or_create_submissions(students) do |submission|
+          res[:comments] << save_comment_to_submission(submission, group, opts, uuid)
+          res[:submissions] << submission
         end
       else
         submission = find_or_create_submission(original_student)
-        res << save_comment_to_submission(submission, group, opts)
+        res[:comments] << save_comment_to_submission(submission, group, opts)
+        res[:submissions] << submission
       end
     end
     res
   end
+  private :update_submission_runner
+
+  def add_submission_comment(original_student, opts={})
+    comments = update_submission_runner(original_student, opts)[:comments]
+    comments.compact # Possible no comments were added depending on opts
+  end
+
+  # Update at this point is solely used for commenting on the submission
+  def update_submission(original_student, opts={})
+    update_submission_runner(original_student, opts)[:submissions]
+  end
 
   def save_comment_to_submission(submission, group, opts, uuid = nil)
     submission.group = group
     submission.save! if submission.changed?
     opts[:group_comment_id] = uuid if group && uuid
-    submission.add_comment(opts)
+    comment = submission.add_comment(opts)
     submission.reload
+    comment
   end
   private :save_comment_to_submission
 

app/models/submission_comment.rb

@@ -50,7 +50,7 @@ class SubmissionComment < ActiveRecord::Base
   validates_each :attempt do |record, attr, value|
     next if value.nil?
     if record.submission.attempt.nil? || value > record.submission.attempt
-      record.errors.add(attr, 'attempt must not be larger then number of submission attempts')
+      record.errors.add(attr, 'attempt must not be larger than number of submission attempts')
     end
   end
 

schema.graphql

@@ -786,6 +786,25 @@ type CreateGroupInSetPayload {
   group: Group
 }
 
+"""
+Autogenerated input type of CreateSubmissionComment
+"""
+input CreateSubmissionCommentInput {
+  attempt: Int
+  comment: String!
+  fileIds: [ID!]
+  mediaObjectId: ID
+  submissionId: ID!
+}
+
+"""
+Autogenerated return type of CreateSubmissionComment
+"""
+type CreateSubmissionCommentPayload {
+  errors: [ValidationError!]
+  submissionComment: SubmissionComment
+}
+
 """
 an ISO8601 formatted time string
 """
@@ -1388,6 +1407,7 @@ interface ModuleItemInterface {
 
 type Mutation {
   createGroupInSet(input: CreateGroupInSetInput!): CreateGroupInSetPayload
+  createSubmissionComment(input: CreateSubmissionCommentInput!): CreateSubmissionCommentPayload
   hideAssignmentGrades(input: HideAssignmentGradesInput!): HideAssignmentGradesPayload
   hideAssignmentGradesForSections(input: HideAssignmentGradesForSectionsInput!): HideAssignmentGradesForSectionsPayload
   postAssignmentGrades(input: PostAssignmentGradesInput!): PostAssignmentGradesPayload
@@ -1873,6 +1893,7 @@ type Submission implements Node & Timestamped {
   """
   _id: ID!
   assignment: Assignment
+  attempt: Int!
   commentsConnection(
     """
     Returns the elements in the list that come after the specified cursor.

spec/factories/attachment_factory.rb

@@ -109,10 +109,12 @@ module Factories
   end
 
   def create_attachment_for_file_upload_submission!(submission, opts={})
-    submission.attachments.create! opts.merge({
+    defaults = {
       :filename => "doc.doc",
-      :display_name => "doc.doc", :user => @user,
-      :uploaded_data => dummy_io
-    })
+      :display_name => "doc.doc",
+      :user => @user,
+      :uploaded_data => dummy_io,
+    }
+    submission.attachments.create! defaults.merge(opts)
   end
 end

spec/graphql/mutations/create_submission_comment_spec.rb

@@ -0,0 +1,187 @@
+#
+# 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/>.
+#
+
+require File.expand_path(File.dirname(__FILE__) + '/../../spec_helper')
+
+describe Mutations::CreateSubmissionComment do
+  before(:once) do
+    @account = Account.create!
+    @course = @account.courses.create!
+    @teacher = @course.enroll_teacher(User.create!, enrollment_state: 'active').user
+    @student = @course.enroll_student(User.create!, enrollment_state: 'active').user
+    @assignment = @course.assignments.create!(title: 'Example Assignment')
+    @submission = @assignment.submit_homework(
+      @student,
+      submission_type: 'online_text_entry',
+      body: 'body'
+    )
+  end
+
+  def value_or_null(value, stringify=true)
+    return 'null' if value.nil?
+    stringify ? "\"#{value}\"" : value
+  end
+
+  def mutation_str(submission_id: nil, attempt: nil, comment: 'hello', file_ids: [], media_object_id: nil)
+    <<~GQL
+      mutation {
+        createSubmissionComment(input: {
+          attempt: #{value_or_null(attempt, false)}
+          comment: #{value_or_null(comment)}
+          fileIds: #{file_ids}
+          mediaObjectId: #{value_or_null(media_object_id)}
+          submissionId: #{value_or_null(submission_id || @submission.id)}
+        }) {
+          submissionComment {
+            _id
+            attempt
+            comment
+            attachments {
+              _id
+            }
+            mediaObject {
+              _id
+            }
+          }
+          errors {
+            attribute
+            message
+          }
+        }
+      }
+    GQL
+  end
+
+  def run_mutation(opts = {}, current_user = @teacher)
+    result = CanvasSchema.execute(mutation_str(opts), context: {current_user: current_user})
+    result.to_h.with_indifferent_access
+  end
+
+  it 'creates a new submission comment' do
+    result = run_mutation
+    expect(
+      result.dig(:data, :createSubmissionComment, :submissionComment, :_id)
+    ).to eq SubmissionComment.last.id.to_s
+  end
+
+  it 'requires permission to comment on the submission' do
+    @student2 = @course.enroll_student(User.create!, enrollment_state: 'active').user
+    result = run_mutation({}, @student2)
+    expect(result[:errors].length).to eq 1
+    expect(result[:errors][0][:message]).to eq 'not found'
+  end
+
+  describe 'submission_id argument' do
+    it 'is gracefully handled when the submission is not found' do
+      result = run_mutation(submission_id: 12345)
+      expect(result[:errors].length).to eq 1
+      expect(result[:errors][0][:message]).to eq 'not found'
+    end
+  end
+
+  describe 'comment argument' do
+    it 'is properly saved to the comment text' do
+      result = run_mutation(comment: 'dogs and cats')
+      expect(
+        result.dig(:data, :createSubmissionComment, :submissionComment, :comment)
+      ).to eq 'dogs and cats'
+    end
+  end
+
+  describe 'attempt argument' do
+    it 'is 0 if unused' do
+      expect(
+        run_mutation.dig(:data, :createSubmissionComment, :submissionComment, :attempt)
+      ).to eq 0
+    end
+
+    it 'can be used to set the submission comment attempt' do
+      @submission.update!(attempt: 3)
+      result = run_mutation(attempt: 2)
+      expect(
+        result.dig(:data, :createSubmissionComment, :submissionComment, :attempt)
+      ).to eq 2
+    end
+
+    it 'is gracefully handled when invalid' do
+      @submission.update!(attempt: 3)
+      result = run_mutation(attempt: 4)
+      errors = result.dig(:data, :createSubmissionComment, :errors)
+      expect(errors.length).to eq 1
+      expect(errors[0][:attribute]).to eq 'attempt'
+      expect(errors[0][:message]).to eq 'attempt must not be larger than number of submission attempts'
+      expect(result.dig(:data, :createSubmissionComment, :submissionComment)).to be_nil
+    end
+  end
+
+  describe 'file_ids argument' do
+    before(:once) do
+      opts = {user: @teacher, context: @assignment}
+      @attachment1 = create_attachment_for_file_upload_submission!(@submission, opts)
+      @attachment2 = create_attachment_for_file_upload_submission!(@submission, opts)
+    end
+
+    it 'lets you attach files to this submission comment' do
+      result = run_mutation(file_ids: [@attachment1.id.to_s])
+      attachments = result.dig(:data, :createSubmissionComment, :submissionComment, :attachments)
+      expect(attachments.length).to eq 1
+      expect(attachments[0][:_id]).to eq @attachment1.id.to_s
+    end
+
+    it 'can attach more then one file' do
+      result = run_mutation(file_ids: [@attachment1.id.to_s, @attachment2.id.to_s])
+      attachments = result.dig(:data, :createSubmissionComment, :submissionComment, :attachments)
+      expect(attachments.length).to eq 2
+      expect(attachments[0][:_id]).to eq @attachment1.id.to_s
+      expect(attachments[1][:_id]).to eq @attachment2.id.to_s
+    end
+
+    it 'requires permissions for all files' do
+      opts = {user: @student, context: @assignment}
+      student_attachment = create_attachment_for_file_upload_submission!(@submission, opts)
+      result = run_mutation({file_ids: [@attachment1.id.to_s, student_attachment.id.to_s]})
+      expect(result[:errors].length).to eq 1
+      expect(result[:errors][0][:message]).to eq 'not found'
+    end
+
+    it 'gracefully handles an attachment not being found' do
+      result = run_mutation(file_ids: ['12345'])
+      expect(result[:errors].length).to eq 1
+      expect(result[:errors][0][:message]).to eq 'not found'
+    end
+  end
+
+  describe 'media_object_id argument' do
+    before(:once) do
+      @media_object = media_object
+    end
+
+    it 'lets you attach a media comment to the submission comment' do
+      result = run_mutation(media_object_id: @media_object.media_id)
+      expect(
+        result.dig(:data, :createSubmissionComment, :submissionComment, :mediaObject, :_id)
+      ).to eq @media_object.media_id
+    end
+
+    it 'gracefully handles the media object not being found' do
+      result = run_mutation(media_object_id: 'm-2pRR7YQkQAR9mBzBdwmT1EZbYfUpzkMY')
+      expect(result[:errors].length).to eq 1
+      expect(result[:errors][0][:message]).to eq 'not found'
+    end
+  end
+end

spec/graphql/types/submission_type_spec.rb

@@ -116,6 +116,18 @@ describe Types::SubmissionType do
     end
   end
 
+  describe '#attempt' do
+    it 'should show the attempt' do
+      @submission.update_column(:attempt, 1) # bypass infer_values callback
+      expect(submission_type.resolve('attempt')).to eq 1
+    end
+
+    it 'should translate nil in the database to 0 in graphql' do
+      @submission.update_column(:attempt, nil) # bypass infer_values callback
+      expect(submission_type.resolve('attempt')).to eq 0
+    end
+  end
+
   describe "submission comments" do
     before(:once) do
       student_in_course(active_all: true)

spec/models/assignment_spec.rb

@@ -5917,6 +5917,76 @@ describe Assignment do
     end
   end
 
+  describe '#add_submission_comment' do
+    let(:assignment) { assignment_model(course: @course) }
+
+    it 'raises an error if original_student is nil' do
+      expect {
+        assignment.add_submission_comment(nil)
+      }.to raise_error 'Student Required'
+    end
+
+    context 'when the student is not in a group' do
+      let!(:associate_student_and_submission) {
+        assignment.submissions.find_by user: @student
+      }
+      let(:update_submission_response) {
+        assignment.add_submission_comment(@student, comment: 'WAT?')
+      }
+
+      it 'returns an Array' do
+        expect(update_submission_response.class).to eq Array
+      end
+
+      it 'returns a collection of submission comments' do
+        expect(update_submission_response.first.class).to eq SubmissionComment
+      end
+    end
+
+    context 'when the student is in a group' do
+      let!(:create_a_group_with_a_submitted_assignment) {
+        setup_assignment_with_group
+        @assignment.submit_homework(
+          @u1,
+          submission_type: 'online_text_entry',
+          body: 'Some text for you'
+        )
+      }
+
+      context 'when a comment is submitted' do
+        let(:update_assignment_with_comment) {
+          @assignment.add_submission_comment(
+            @u2,
+            comment: 'WAT?',
+            group_comment: true,
+            user_id: @course.teachers.first.id
+          )
+        }
+
+        it 'returns an Array' do
+          expect(update_assignment_with_comment).to be_an_instance_of Array
+        end
+
+        it 'creates a comment for each student in the group' do
+          expect {
+            update_assignment_with_comment
+          }.to change{ SubmissionComment.count }.by(@u1.groups.first.users.count)
+        end
+
+        it 'creates comments with the same group_comment_id' do
+          comments = update_assignment_with_comment
+          expect(comments.first.group_comment_id).to eq comments.last.group_comment_id
+        end
+      end
+
+      context 'when a comment is not submitted' do
+        it 'returns an Array' do
+          expect(@assignment.add_submission_comment(@u2).class).to eq Array
+        end
+      end
+    end
+  end
+
   describe "#update_submission" do
     let(:assignment) { assignment_model(course: @course) }