From 3172e45ccd927cb4eceabf8d21f6915b3dd78574 Mon Sep 17 00:00:00 2001
From: Cody Cutrer <cody@instructure.com>
Date: Mon, 30 Apr 2012 12:08:55 -0600
Subject: [PATCH] use display name for files in downloaded zips fixes #6731

test plan:
 * upload a file
 * rename it via the ui
 * download the folder as a zip
 * the file should be named what it is in the UI

Change-Id: I61c29d18d5384d24303e6ce57423fb055884e5ec
Reviewed-on: https://gerrit.instructure.com/10418
Reviewed-by: Simon Williams <simon@instructure.com>
Tested-by: Jenkins <jenkins@instructure.com>
---
 lib/content_zipper.rb           |  2 +-
 spec/lib/content_zipper_spec.rb | 32 ++++++++++++++++++++++++--------
 2 files changed, 25 insertions(+), 9 deletions(-)

diff --git a/lib/content_zipper.rb b/lib/content_zipper.rb
index 421a81830db..d84a8493837 100644
--- a/lib/content_zipper.rb
+++ b/lib/content_zipper.rb
@@ -306,7 +306,7 @@ class ContentZipper
       callback.call(attachment, folder_names) if callback
       @context = folder.context
       @logger.debug("  found attachment: #{attachment.unencoded_filename}")
-      path = folder_names.empty? ? attachment.filename : File.join(folder_names, attachment.unencoded_filename)
+      path = folder_names.empty? ? attachment.display_name : File.join(folder_names, attachment.display_name)
       @files_added = false unless add_attachment_to_zip(attachment, zipfile, path)
     end
     folder.active_sub_folders.select{|f| !@check_user || f.grants_right?(@user, nil, :read_contents)}.each do |sub_folder|
diff --git a/spec/lib/content_zipper_spec.rb b/spec/lib/content_zipper_spec.rb
index 772470f3774..cbfa5164629 100644
--- a/spec/lib/content_zipper_spec.rb
+++ b/spec/lib/content_zipper_spec.rb
@@ -105,16 +105,16 @@ describe ContentZipper do
         names = []
         @attachment.reload
         Zip::ZipFile.foreach(@attachment.full_filename) {|f| names << f.name if f.file? }
-        names
+        names.sort
       end
 
       context "in a private course" do
         it "should give logged in students some files" do
-          zipped_files_for_user(@user).should == ['visible.png', 'visible/sub-vis.png']
+          zipped_files_for_user(@user).should == ['visible.png', 'visible/sub-vis.png'].sort
         end
 
         it "should give logged in teachers all files" do
-          zipped_files_for_user(@teacher).should == ["locked/sub-locked-vis.png", "hidden/sub-hidden.png", "hidden.png", "visible.png", "visible/sub-locked.png", "visible/sub-vis.png", "locked.png"]
+          zipped_files_for_user(@teacher).should == ["locked/sub-locked-vis.png", "hidden/sub-hidden.png", "hidden.png", "visible.png", "visible/sub-locked.png", "visible/sub-vis.png", "locked.png"].sort
         end
 
         it "should give logged out people no files" do
@@ -122,7 +122,7 @@ describe ContentZipper do
         end
         
         it "should give all files if check_user=false" do
-          zipped_files_for_user(nil, false).should == ["locked/sub-locked-vis.png", "hidden/sub-hidden.png", "hidden.png", "visible.png", "visible/sub-locked.png", "visible/sub-vis.png", "locked.png"]
+          zipped_files_for_user(nil, false).should == ["locked/sub-locked-vis.png", "hidden/sub-hidden.png", "hidden.png", "visible.png", "visible/sub-locked.png", "visible/sub-vis.png", "locked.png"].sort
         end
       end
 
@@ -133,19 +133,19 @@ describe ContentZipper do
         end
 
         it "should give logged in students some files" do
-          zipped_files_for_user(@user).should == ['visible.png', 'visible/sub-vis.png']
+          zipped_files_for_user(@user).should == ['visible.png', 'visible/sub-vis.png'].sort
         end
 
         it "should give logged in teachers all files" do
-          zipped_files_for_user(@teacher).should == ["locked/sub-locked-vis.png", "hidden/sub-hidden.png", "hidden.png", "visible.png", "visible/sub-locked.png", "visible/sub-vis.png", "locked.png"]
+          zipped_files_for_user(@teacher).should == ["locked/sub-locked-vis.png", "hidden/sub-hidden.png", "hidden.png", "visible.png", "visible/sub-locked.png", "visible/sub-vis.png", "locked.png"].sort
         end
 
         it "should give logged out people the same thing as students" do
-          zipped_files_for_user(nil).should == ['visible.png', 'visible/sub-vis.png']
+          zipped_files_for_user(nil).should == ['visible.png', 'visible/sub-vis.png'].sort
         end
 
         it "should give all files if check_user=false" do
-          zipped_files_for_user(nil, false).should == ["locked/sub-locked-vis.png", "hidden/sub-hidden.png", "hidden.png", "visible.png", "visible/sub-locked.png", "visible/sub-vis.png", "locked.png"]
+          zipped_files_for_user(nil, false).should == ["locked/sub-locked-vis.png", "hidden/sub-hidden.png", "hidden.png", "visible.png", "visible/sub-locked.png", "visible/sub-vis.png", "locked.png"].sort
         end
       end
     end
@@ -161,5 +161,21 @@ describe ContentZipper do
       ContentZipper.process_attachment(attachment, @user)
       attachment.workflow_state.should == 'zipped'
     end
+
+    it "should use the display name" do
+      course_with_student(:active_all => true)
+      folder = Folder.root_folders(@course).first
+      attachment_model(:uploaded_data => stub_png_data('hidden.png'), :content_type => 'image/png', :folder => folder, :display_name => 'otherfile.png')
+      attachment = Attachment.new(:display_name => 'my_download.zip')
+      attachment.user_id = @user.id
+      attachment.workflow_state = 'to_be_zipped'
+      attachment.context = folder
+      attachment.save!
+      ContentZipper.process_attachment(attachment, @user)
+      attachment.reload
+      names = []
+      Zip::ZipFile.foreach(attachment.full_filename) {|f| names << f.name if f.file? }
+      names.should == ['otherfile.png']
+    end
   end
 end