Revert "update submission attachment copy to always create new attachment"

This reverts commit 61c75bae54.

fixes ADMIN-2981

Change-Id: Ic782bcc79741272555706823f788b9510a0ed98f
Reviewed-on: https://gerrit.instructure.com/212933
Tested-by: Jenkins
Reviewed-by: Carl Kibler <ckibler@instructure.com>
QA-Review: Carl Kibler <ckibler@instructure.com>
Product-Review: Mysti Lilla <mysti@instructure.com>

app/controllers/submissions_controller.rb

@@ -256,7 +256,7 @@ class SubmissionsController < SubmissionsBaseController
       :attachment_ids => []
     )
     submission_params[:group_comment] = value_to_boolean(submission_params[:group_comment])
-    submission_params[:attachments] = Attachment.copy_attachments_to_submissions_folder(@context, params[:submission][:attachments].compact.uniq)
+    submission_params[:attachments] = self.class.copy_attachments_to_submissions_folder(@context, params[:submission][:attachments].compact.uniq)
 
     begin
       @submission = @assignment.submit_homework(@submission_user, submission_params)
@@ -377,6 +377,18 @@ class SubmissionsController < SubmissionsBaseController
   end
   private :lookup_existing_attachments
 
+  def self.copy_attachments_to_submissions_folder(assignment_context, attachments)
+    attachments.map do |attachment|
+      if attachment.folder && attachment.folder.for_submissions?
+        attachment # already in a submissions folder
+      elsif attachment.context.respond_to?(:submissions_folder)
+        attachment.copy_to_folder!(attachment.context.submissions_folder(assignment_context))
+      else
+        attachment # in a weird context; leave it alone
+      end
+    end
+  end
+
   def is_media_recording?
     return params[:submission][:submission_type] == 'media_recording'
   end

app/graphql/mutations/create_submission.rb

@@ -78,7 +78,7 @@ class Mutations::CreateSubmission < Mutations::BaseMutation
       upload_errors = validate_online_upload(assignment, attachments)
       return upload_errors if upload_errors
 
-      submission_params[:attachments] = Attachment.copy_attachments_to_submissions_folder(context, attachments)
+      submission_params[:attachments] = copy_attachments_to_submissions_folder(context, attachments)
     when 'online_url'
       submission_params[:url] = input[:url]
     end
@@ -122,4 +122,16 @@ class Mutations::CreateSubmission < Mutations::BaseMutation
 
     true
   end
+
+  def copy_attachments_to_submissions_folder(assignment_context, attachments)
+    attachments.map do |attachment|
+      if attachment&.folder&.for_submissions?
+        attachment # already in a submissions folder
+      elsif attachment.context.respond_to?(:submissions_folder)
+        attachment.copy_to_folder!(attachment.context.submissions_folder(assignment_context))
+      else
+        attachment # in a weird context; leave it alone
+      end
+    end
+  end
 end

app/models/attachment.rb

@@ -1962,17 +1962,6 @@ class Attachment < ActiveRecord::Base
     false
   end
 
-  def self.copy_attachments_to_submissions_folder(assignment_context, attachments)
-    attachments.map do |attachment|
-      if attachment.context.respond_to?(:submissions_folder)
-        # if its already in the submission folder, then we need to make a copy of it.
-        attachment.copy_to_folder!(attachment.context.submissions_folder(assignment_context))
-      else
-        attachment # in a weird context; leave it alone
-      end
-    end
-  end
-
   def set_publish_state_for_usage_rights
     if self.context &&
        (!self.folder || !self.folder.for_submissions?) &&

spec/apis/v1/submissions_api_spec.rb

@@ -3927,15 +3927,17 @@ describe 'Submissions API', type: :request do
         })
       end
 
-      it "always copies files to the submissions folder even if already there" do
+      it "copys files to the submissions folder if they're not there already" do
         @assignment.update_attributes(:submission_types => 'online_upload')
         a1 = attachment_model(:context => @user, :folder => @user.submissions_folder)
         a2 = attachment_model(:context => @user)
         json = do_submit(:submission_type => 'online_upload', :file_ids => [a1.id, a2.id])
         submission_attachment_ids = json['attachments'].map { |a| a['id'] }
-        checks = Attachment.find(submission_attachment_ids)
-        expect(checks.map{ |attachment| attachment.root_attachment.id }).to contain_exactly(a1.id, a2.id)
-        checks.each { |attachment| expect(attachment.folder).to eq @user.submissions_folder(@course) }
+        expect(submission_attachment_ids.size).to eq 2
+        expect(submission_attachment_ids.delete(a1.id)).not_to be_nil
+        copy = Attachment.find(submission_attachment_ids.last)
+        expect(copy.folder).to eq @user.submissions_folder(@course)
+        expect(copy.root_attachment).to eq a2
       end
     end
 

spec/controllers/submissions_controller_spec.rb

@@ -1107,4 +1107,42 @@ describe SubmissionsController do
       end
     end
   end
+
+  describe "copy_attachments_to_submissions_folder" do
+    before(:once) do
+      course_with_student
+      @course.account.enable_service(:avatars)
+      attachment_model(context: @student)
+    end
+
+    it "copies a user attachment into the user's submissions folder" do
+      atts = SubmissionsController.copy_attachments_to_submissions_folder(@course, [@attachment])
+      expect(atts.length).to eq 1
+      expect(atts[0]).not_to eq @attachment
+      expect(atts[0].folder).to eq @student.submissions_folder(@course)
+    end
+
+    it "leaves files already in submissions folders alone" do
+      @attachment.folder = @student.submissions_folder(@course)
+      @attachment.save!
+      atts = SubmissionsController.copy_attachments_to_submissions_folder(@course, [@attachment])
+      expect(atts).to eq [@attachment]
+    end
+
+    it "copies a group attachment into the group submission folder" do
+      group_model(context: @course)
+      attachment_model(context: @group)
+      atts = SubmissionsController.copy_attachments_to_submissions_folder(@course, [@attachment])
+      expect(atts.length).to eq 1
+      expect(atts[0]).not_to eq @attachment
+      expect(atts[0].folder).to eq @group.submissions_folder
+    end
+
+    it "leaves files in non user/group context alone" do
+      assignment_model(context: @course)
+      weird_file = @assignment.attachments.create! display_name: 'blah', uploaded_data: default_uploaded_data
+      atts = SubmissionsController.copy_attachments_to_submissions_folder(@course, [weird_file])
+      expect(atts).to eq [weird_file]
+    end
+  end
 end

spec/models/attachment_spec.rb

@@ -2213,43 +2213,4 @@ describe Attachment do
       expect(dup.display_name).not_to eq 'test.txt'
     end
   end
-
-  describe "copy_attachments_to_submissions_folder" do
-    before(:once) do
-      course_with_student
-      @course.account.enable_service(:avatars)
-      attachment_model(context: @student)
-    end
-
-    it "copies a user attachment into the user's submissions folder" do
-      atts = Attachment.copy_attachments_to_submissions_folder(@course, [@attachment])
-      expect(atts.length).to eq 1
-      expect(atts[0]).not_to eq @attachment
-      expect(atts[0].folder).to eq @student.submissions_folder(@course)
-    end
-
-    it "copies files already in submissions folders" do
-      @attachment.folder = @student.submissions_folder(@course)
-      @attachment.save!
-      atts = Attachment.copy_attachments_to_submissions_folder(@course, [@attachment])
-      expect(atts).not_to include @attachment
-      expect(atts[0].root_attachment).to eq @attachment
-    end
-
-    it "copies a group attachment into the group submission folder" do
-      group_model(context: @course)
-      attachment_model(context: @group)
-      atts = Attachment.copy_attachments_to_submissions_folder(@course, [@attachment])
-      expect(atts.length).to eq 1
-      expect(atts[0]).not_to eq @attachment
-      expect(atts[0].folder).to eq @group.submissions_folder
-    end
-
-    it "leaves files in non user/group context alone" do
-      assignment_model(context: @course)
-      weird_file = @assignment.attachments.create! display_name: 'blah', uploaded_data: default_uploaded_data
-      atts = Attachment.copy_attachments_to_submissions_folder(@course, [weird_file])
-      expect(atts).to eq [weird_file]
-    end
-  end
 end