Refactor.

22 files changed, 163 insertions, 193 deletions

diff --git a/.gitignore b/.gitignore
index 7a7b5c93936..89fe301ee8f 100644
--- a/.gitignore
+++ b/.gitignore
@@ -36,6 +36,7 @@ nohup.out
 public/assets/
 public/uploads.*
 public/uploads/
+uploads/
 rails_best_practices_output.html
 tags
 tmp/
diff --git a/Gemfile b/Gemfile
index c3d8299e944..3f0eae8ef42 100644
--- a/Gemfile
+++ b/Gemfile
@@ -205,6 +205,7 @@ group :development do
   gem "letter_opener"
   gem 'quiet_assets', '~> 1.0.1'
   gem 'rack-mini-profiler', require: false
+  gem "byebug"
 
   # Better errors handler
   gem 'better_errors'
diff --git a/Gemfile.lock b/Gemfile.lock
index 3283da40f8d..2f5ebf80b12 100644
--- a/Gemfile.lock
+++ b/Gemfile.lock
@@ -61,6 +61,9 @@ GEM
       sass (~> 3.2)
     browser (0.7.2)
     builder (3.2.2)
+    byebug (3.2.0)
+      columnize (~> 0.8)
+      debugger-linecache (~> 1.2)
     cal-heatmap-rails (0.0.1)
     capybara (2.2.1)
       mime-types (>= 1.16)
@@ -88,6 +91,7 @@ GEM
     coffee-script-source (1.6.3)
     colored (1.2)
     colorize (0.5.8)
+    columnize (0.9.0)
     connection_pool (2.1.0)
     coveralls (0.7.0)
       multi_json (~> 1.3)
@@ -103,6 +107,7 @@ GEM
     daemons (1.1.9)
     database_cleaner (1.3.0)
     debug_inspector (0.0.2)
+    debugger-linecache (1.2.0)
     default_value_for (3.0.0)
       activerecord (>= 3.2.0, < 5.0)
     descendants_tracker (0.0.3)
@@ -646,6 +651,7 @@ DEPENDENCIES
   binding_of_caller
   bootstrap-sass (~> 3.0)
   browser
+  byebug
   cal-heatmap-rails (~> 0.0.1)
   capybara (~> 2.2.1)
   carrierwave
diff --git a/app/assets/javascripts/dropzone_input.js.coffee b/app/assets/javascripts/dropzone_input.js.coffee
index bed8471b39a..2d9b496b132 100644
--- a/app/assets/javascripts/dropzone_input.js.coffee
+++ b/app/assets/javascripts/dropzone_input.js.coffee
@@ -9,7 +9,7 @@ class @DropzoneInput
     iconPicture = "<i class=\"fa fa-picture-o div-dropzone-icon\"></i>"
     iconSpinner = "<i class=\"fa fa-spinner fa-spin div-dropzone-icon\"></i>"
     btnAlert = "<button type=\"button\"" + alertAttr + ">&times;</button>"
-    project_file_path_upload = window.project_file_path_upload or null
+    project_uploads_path = window.project_uploads_path or null
 
     form_textarea = $(form).find("textarea.markdown-area")
     form_textarea.wrap "<div class=\"div-dropzone\"></div>"
@@ -72,10 +72,10 @@ class @DropzoneInput
       form.find(".md-preview-holder").hide()
 
     dropzone = form_dropzone.dropzone(
-      url: project_file_path_upload
+      url: project_uploads_path
       dictDefaultMessage: ""
       clickable: true
-      paramName: "markdown_file"
+      paramName: "file"
       maxFilesize: 10
       uploadMultiple: false
       headers:
@@ -131,10 +131,9 @@ class @DropzoneInput
 
     child = $(dropzone[0]).children("textarea")
 
-    formatLink = (str) ->
-      text = "[" + str.alt + "](" + str.url + ")"
-      if str.is_image is true
-        text = "!" + text
+    formatLink = (link) ->
+      text = "[#{link.alt}](#{link.url})"
+      text = "!#{text}" if link.is_image
       text
 
     handlePaste = (event) ->
@@ -179,9 +178,9 @@ class @DropzoneInput
 
     uploadFile = (item, filename) ->
       formData = new FormData()
-      formData.append "markdown_file", item, filename
+      formData.append "file", item, filename
       $.ajax
-        url: project_file_path_upload
+        url: project_uploads_path
         type: "POST"
         data: formData
         dataType: "json"
@@ -235,8 +234,7 @@ class @DropzoneInput
       $(@).closest('.gfm-form').find('.div-dropzone').click()
       return
 
-  formatLink: (str) ->
-    text = "[" + str.alt + "](" + str.url + ")"
-    if str.is_image is true
-      text = "!" + text
-    text
+  formatLink: (link) ->
+    text = "[#{link.alt}](#{link.url})"
+    text = "!#{text}" if link.is_image
+    text
\ No newline at end of file
diff --git a/app/controllers/files_controller.rb b/app/controllers/files_controller.rb
index 15523cbc2e7..267239b7b84 100644
--- a/app/controllers/files_controller.rb
+++ b/app/controllers/files_controller.rb
@@ -3,18 +3,21 @@ class FilesController < ApplicationController
     note = Note.find(params[:id])
     uploader = note.attachment
 
-    if uploader.file_storage?
-      if can?(current_user, :read_project, note.project)
-        # Replace old notes location in /public with the new one in / and send the file
+    if can?(current_user, :read_project, note.project)
+      if uploader.file_storage?
         path = uploader.file.path.gsub("#{Rails.root}/public", Rails.root.to_s)
 
-        disposition = uploader.image? ? 'inline' : 'attachment'
-        send_file path, disposition: disposition
+        if File.exist?(path)
+          disposition = uploader.image? ? 'inline' : 'attachment'
+          send_file path, disposition: disposition
+        else
+          not_found!
+        end
       else
-        not_found!
+        redirect_to uploader.url
       end
     else
-      redirect_to uploader.url
+      not_found!
     end
   end
 end
diff --git a/app/controllers/projects/uploads_controller.rb b/app/controllers/projects/uploads_controller.rb
index 1c9fb1c86fb..355163ac879 100644
--- a/app/controllers/projects/uploads_controller.rb
+++ b/app/controllers/projects/uploads_controller.rb
@@ -3,14 +3,37 @@ class Projects::UploadsController < Projects::ApplicationController
 
   before_filter :project
 
+  def create
+    link_to_file = ::Projects::UploadService.new(repository, params[:file]).
+      execute
+
+    respond_to do |format|
+      if link_to_file
+        format.json do
+          render json: { link: link_to_file }
+        end
+      else
+        format.json do
+          render json: 'Invalid file.', status: :unprocessable_entity
+        end
+      end
+    end
+  end
+
   def show
-    folder_id = params[:folder_id]
-    filename = params[:filename]
-    
-    uploader = FileUploader.new("#{Rails.root}/uploads","#{@project.path_with_namespace}/#{folder_id}")
-    uploader.retrieve_from_store!(filename)
+    uploader = FileUploader.new(project, params[:secret])
+
+    if uploader.file_storage?
+      uploader.retrieve_from_store!(params[:filename])
 
-    disposition = uploader.image? ? 'inline' : 'attachment'
-    send_file uploader.file.path, disposition: disposition
+      if uploader.file.exists?
+        disposition = uploader.image? ? 'inline' : 'attachment'
+        send_file uploader.file.path, disposition: disposition
+      else
+        not_found!
+      end
+    else
+      redirect_to uploader.url
+    end
   end
 end
diff --git a/app/controllers/projects_controller.rb b/app/controllers/projects_controller.rb
index b430278903a..9be66b6b9f9 100644
--- a/app/controllers/projects_controller.rb
+++ b/app/controllers/projects_controller.rb
@@ -134,19 +134,6 @@ class ProjectsController < ApplicationController
     end
   end
 
-  def upload_file
-    link_to_file = ::Projects::FileService.new(repository, params, root_url).
-      execute
-
-    respond_to do |format|
-      if link_to_file
-        format.json { render json: { link: link_to_file } }
-      else
-        format.json { render json: 'Invalid file.', status: :unprocessable_entity }
-      end
-    end
-  end
-
   def toggle_star
     current_user.toggle_star(@project)
     @project.reload
@@ -159,10 +146,6 @@ class ProjectsController < ApplicationController
 
   private
 
-  def invalid_file(error)
-    render json: { message: error.message }, status: :internal_server_error
-  end
-
   def set_title
     @title = 'New Project'
   end
diff --git a/app/services/projects/file_service.rb b/app/services/projects/file_service.rb
deleted file mode 100644
index 8c149bf53a1..00000000000
--- a/app/services/projects/file_service.rb
+++ /dev/null
@@ -1,55 +0,0 @@
-module Projects
-  class FileService < BaseService
-    include Rails.application.routes.url_helpers
-    def initialize(repository, params, root_url)
-      @repository, @params, @root_url = repository, params.dup, root_url
-    end
-
-    def execute
-      uploader = FileUploader.new("#{Rails.root}/uploads", upload_path, accepted_files)
-      file = @params['markdown_file']
-
-      if file
-        alt = file.original_filename
-        uploader.store!(file)
-        filename = nil
-        if image?(file)
-          filename=File.basename(alt, '.*')
-        else
-          filename=File.basename(alt)
-        end
-        link = {
-          'alt' => filename,
-          'url' => uploader.secure_url,
-          'is_image' => image?(file)
-        }
-      else
-        link = nil
-      end
-    end
-
-    protected
-
-    def accepted_files
-      # insert accepted mime types here (e.g %w(jpg jpeg gif png))
-      nil
-    end
-
-    def accepted_images
-      %w(jpg jpeg gif png)
-    end
-
-    def image?(file)
-      accepted_images.map { |format| file.content_type.include? format }.any?
-    end
-
-    def upload_path
-      base_dir = FileUploader.generate_dir
-      File.join(@repository.path_with_namespace, base_dir)
-    end
-
-    def correct_mime_type?(file)
-      accepted_files.map { |format| image.content_type.include? format }.any?
-    end
-  end
-end
diff --git a/app/services/projects/upload_service.rb b/app/services/projects/upload_service.rb
new file mode 100644
index 00000000000..a186c97628f
--- /dev/null
+++ b/app/services/projects/upload_service.rb
@@ -0,0 +1,22 @@
+module Projects
+  class UploadService < BaseService
+    def initialize(project, file)
+      @project, @file = project, file
+    end
+
+    def execute
+      return nil unless @file
+
+      uploader = FileUploader.new(@project)
+      uploader.store!(@file)
+
+      filename = uploader.image? ? uploader.file.basename : uploader.file.filename
+
+      {
+        'alt'       => filename,
+        'url'       => uploader.secure_url,
+        'is_image'  => uploader.image?
+      }
+    end
+  end
+end
diff --git a/app/uploaders/attachment_uploader.rb b/app/uploaders/attachment_uploader.rb
index 22742d287a4..58dc6e90c1e 100644
--- a/app/uploaders/attachment_uploader.rb
+++ b/app/uploaders/attachment_uploader.rb
@@ -21,7 +21,7 @@ class AttachmentUploader < CarrierWave::Uploader::Base
   end
 
   def secure_url
-    Gitlab.config.gitlab.relative_url_root + "/files/#{model.class.to_s.underscore}/#{model.id}/#{file.filename}"
+    File.join(Gitlab.config.gitlab.relative_url_root, "files", model.class.to_s.underscore, model.id.to_s, file.filename)
   end
 
   def file_storage?
diff --git a/app/uploaders/file_uploader.rb b/app/uploaders/file_uploader.rb
index 51ae8040e52..c040f6bbe92 100644
--- a/app/uploaders/file_uploader.rb
+++ b/app/uploaders/file_uploader.rb
@@ -2,47 +2,33 @@
 class FileUploader < CarrierWave::Uploader::Base
   storage :file
 
-  def initialize(base_dir, path = '', allowed_extensions = nil)
-    @base_dir = base_dir
-    @path = path
-    @allowed_extensions = allowed_extensions
+  def initialize(project, secret = self.class.generate_secret)
+    @project = project
+    @secret = secret
   end
 
   def base_dir
-    @base_dir
+    "#{Rails.root}/uploads"
   end
 
   def store_dir
-    File.join(@base_dir, @path)
+    File.join(base_dir, @project.path_with_namespace, @secret)
   end
 
   def cache_dir
-    File.join(@base_dir, 'tmp', @path)
+    File.join(base_dir, 'tmp', @project.path_with_namespace, @secret)
   end
 
-  def extension_white_list
-    @allowed_extensions || super
-  end
-
-  def store!(file)
-    @filename = self.class.generate_filename(file)
-    super
-  end
-
-  def self.generate_filename(file)
-    original_filename = File.basename(file.original_filename, '.*')
-    extension = File.extname(file.original_filename)
-    new_filename = Digest::MD5.hexdigest(original_filename) + extension
-  end
-
-  def self.generate_dir
+  def self.generate_secret
     SecureRandom.hex(5)
   end
 
   def secure_url
-    path_array = @path.split('/')
-    path = File.join(path_array[0],path_array[1],'uploads',path_array[2])
-    Gitlab.config.gitlab.relative_url_root + "/#{path}/#{@filename}"
+    File.join(Gitlab.config.gitlab.relative_url_root, @project.path_with_namespace, "uploads", @secret, file.filename)
+  end
+
+  def file_storage?
+    self.class.storage == CarrierWave::Storage::File
   end
 
   def image?
diff --git a/app/views/projects/issues/_form.html.haml b/app/views/projects/issues/_form.html.haml
index 975980bd6bd..afeeed6edf4 100644
--- a/app/views/projects/issues/_form.html.haml
+++ b/app/views/projects/issues/_form.html.haml
@@ -11,4 +11,4 @@
     e.preventDefault();
   });
 
-  window.project_file_path_upload = "#{upload_file_project_path @project}";
+  window.project_uploads_path = "#{project_uploads_path @project}";
diff --git a/app/views/projects/merge_requests/_form.html.haml b/app/views/projects/merge_requests/_form.html.haml
index 28c4734e14f..c1a05e4586f 100644
--- a/app/views/projects/merge_requests/_form.html.haml
+++ b/app/views/projects/merge_requests/_form.html.haml
@@ -9,4 +9,4 @@
     e.preventDefault();
   });
 
-  window.project_file_path_upload = "#{upload_file_project_path @project}";
+  window.project_uploads_path = "#{project_uploads_path @project}";
diff --git a/app/views/projects/merge_requests/_new_submit.html.haml b/app/views/projects/merge_requests/_new_submit.html.haml
index 0653b30fcca..4cf2a05b1a3 100644
--- a/app/views/projects/merge_requests/_new_submit.html.haml
+++ b/app/views/projects/merge_requests/_new_submit.html.haml
@@ -113,7 +113,7 @@
     e.preventDefault();
   });
 
-  window.project_file_path_upload = "#{upload_file_project_path @project}";
+  window.project_uploads_path = "#{project_uploads_path @project}";
 
 :javascript
   var merge_request
diff --git a/app/views/projects/milestones/_form.html.haml b/app/views/projects/milestones/_form.html.haml
index 5fbb668570c..dbcd23eee05 100644
--- a/app/views/projects/milestones/_form.html.haml
+++ b/app/views/projects/milestones/_form.html.haml
@@ -51,4 +51,4 @@
     onSelect: function(dateText, inst) { $("#milestone_due_date").val(dateText) }
   }).datepicker("setDate", $.datepicker.parseDate('yy-mm-dd', $('#milestone_due_date').val()));
 
-  window.project_file_path_upload = "#{upload_file_project_path @project}";
+  window.project_uploads_path = "#{project_uploads_path @project}";
diff --git a/app/views/projects/notes/_form.html.haml b/app/views/projects/notes/_form.html.haml
index fe3dab569f4..9f9efc782d5 100644
--- a/app/views/projects/notes/_form.html.haml
+++ b/app/views/projects/notes/_form.html.haml
@@ -29,4 +29,4 @@
       = f.file_field :attachment, class: "js-note-attachment-input hidden"
 
 :javascript
-  window.project_file_path_upload = "#{upload_file_project_path @project}";
+  window.project_uploads_path = "#{project_uploads_path @project}";
diff --git a/app/views/projects/wikis/_form.html.haml b/app/views/projects/wikis/_form.html.haml
index 0afee138c8b..b1579878ed1 100644
--- a/app/views/projects/wikis/_form.html.haml
+++ b/app/views/projects/wikis/_form.html.haml
@@ -43,6 +43,6 @@
       = link_to "Cancel", project_wiki_path(@project, :home), class: "btn btn-cancel"
 
 :javascript
-  window.project_file_path_upload = "#{upload_file_project_path @project}";
+  window.project_uploads_path = "#{project_uploads_path @project}";
 
 
diff --git a/config/routes.rb b/config/routes.rb
index d29ad8db639..f0a7cf1e8a6 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -220,7 +220,6 @@ Gitlab::Application.routes.draw do
       put :transfer
       post :archive
       post :unarchive
-      post :upload_file
       post :toggle_star
       post :markdown_preview
       get :autocomplete_sources
@@ -256,7 +255,11 @@ Gitlab::Application.routes.draw do
         end
       end
 
-      get '/uploads/:folder_id/:filename' => 'uploads#show', constraints: { filename:  /.+/ }
+      resources :uploads, only: [:create] do
+        collection do
+          get ":secret/:filename", action: :show, constraints: { filename: /.+/ }
+        end
+      end
 
       get '/compare/:from...:to' => 'compare#show', :as => 'compare',
           :constraints => { from: /.+/, to: /.+/ }
diff --git a/db/schema.rb b/db/schema.rb
index e11a068c9c5..be3d35a431d 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -26,7 +26,6 @@ ActiveRecord::Schema.define(version: 20150213121042) do
     t.datetime "updated_at"
     t.string   "home_page_url"
     t.integer  "default_branch_protection", default: 2
-    t.boolean  "twitter_sharing_enabled",   default: true
   end
 
   create_table "broadcast_messages", force: true do |t|
diff --git a/spec/controllers/projects/uploads_controller_spec.rb b/spec/controllers/projects/uploads_controller_spec.rb
new file mode 100644
index 00000000000..8c99b5ca528
--- /dev/null
+++ b/spec/controllers/projects/uploads_controller_spec.rb
@@ -0,0 +1,49 @@
+require('spec_helper')
+
+describe Projects::UploadsController do
+  let(:project) { create(:project) }
+  let(:user)    { create(:user) }
+  let(:jpg)     { fixture_file_upload(Rails.root + 'spec/fixtures/rails_sample.jpg', 'image/jpg') }
+  let(:txt)     { fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain') }
+
+  describe 'POST #create' do
+    before do
+      sign_in(user)
+      project.team << [user, :developer]
+    end
+
+    context "without params['file']" do
+      it 'returns an error' do
+        post :create, project_id: project.to_param, format: :json
+        expect(response.status).to eq(422)
+      end
+    end
+
+    context 'with valid image' do
+      before do
+        post :create,
+          project_id: project.to_param,
+          file: jpg,
+          format: :json
+      end
+
+      it 'returns a content with original filename, new link, and correct type.' do
+        expect(response.body).to match '\"alt\":\"rails_sample\"'
+        expect(response.body).to match "\"url\":\"/#{project.path_with_namespace}/uploads"
+        expect(response.body).to match '\"is_image\":true'
+      end
+    end
+
+    context 'with valid non-image file' do
+      before do
+        post :create, project_id: project.to_param, file: txt, format: :json
+      end
+
+      it 'returns a content with original filename, new link, and correct type.' do
+        expect(response.body).to match '\"alt\":\"doc_sample.txt\"'
+        expect(response.body).to match "\"url\":\"/#{project.path_with_namespace}/uploads"
+        expect(response.body).to match '\"is_image\":false'
+      end
+    end
+  end
+end
diff --git a/spec/controllers/projects_controller_spec.rb b/spec/controllers/projects_controller_spec.rb
index 2d52e3fd913..9be4c2e505c 100644
--- a/spec/controllers/projects_controller_spec.rb
+++ b/spec/controllers/projects_controller_spec.rb
@@ -4,50 +4,7 @@ describe ProjectsController do
   let(:project) { create(:project) }
   let(:public_project) { create(:project, :public) }
   let(:user)    { create(:user) }
-  let(:jpg)     { fixture_file_upload(Rails.root + 'spec/fixtures/rails_sample.jpg', 'image/jpg') }
-  let(:txt)     { fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain') }
-
-  describe 'POST #upload_file' do
-    before do
-      sign_in(user)
-      project.team << [user, :developer]
-    end
-
-    context "without params['markdown_file']" do
-      it 'returns an error' do
-        post :upload_file, id: project.to_param, format: :json
-        expect(response.status).to eq(422)
-      end
-    end
-
-    context 'with valid image' do
-      before do
-        post :upload_file,
-          id: project.to_param,
-          markdown_file: jpg,
-          format: :json
-      end
-
-      it 'returns a content with original filename, new link, and correct type.' do
-        expect(response.body).to match '\"alt\":\"rails_sample\"'
-        expect(response.body).to match "\"url\":\"/#{project.path_with_namespace}/uploads"
-        expect(response.body).to match '\"is_image\":true'
-      end
-    end
-
-    context 'with valid non-image file' do
-      before do
-        post :upload_file, id: project.to_param, markdown_file: txt, format: :json
-      end
-
-      it 'returns a content with original filename, new link, and correct type.' do
-        expect(response.body).to match '\"alt\":\"doc_sample.txt\"'
-        expect(response.body).to match "\"url\":\"/#{project.path_with_namespace}/uploads"
-        expect(response.body).to match '\"is_image\":false'
-      end
-    end
-  end
-
+  
   describe 'POST #toggle_star' do
     it 'toggles star if user is signed in' do
       sign_in(user)
diff --git a/spec/services/projects/file_service_spec.rb b/spec/services/projects/upload_service_spec.rb
index 7bbe5b575c9..fc34b456482 100644
--- a/spec/services/projects/file_service_spec.rb
+++ b/spec/services/projects/upload_service_spec.rb
@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Projects::FileService do
+describe Projects::UploadService do
   describe 'File service' do
     before do
       @user = create :user
@@ -10,9 +10,7 @@ describe Projects::FileService do
     context 'for valid gif file' do
       before do
         gif = fixture_file_upload(Rails.root + 'spec/fixtures/banana_sample.gif', 'image/gif')
-        @link_to_file = upload_file(@project.repository,
-          { 'markdown_file' => gif },
-          'http://test.example/')
+        @link_to_file = upload_file(@project.repository, gif)
       end
 
       it { expect(@link_to_file).to have_key('alt') }
@@ -28,9 +26,7 @@ describe Projects::FileService do
       before do
         png = fixture_file_upload(Rails.root + 'spec/fixtures/dk.png',
           'image/png')
-        @link_to_file = upload_file(@project.repository,
-          { 'markdown_file' => png },
-          'http://test.example/')
+        @link_to_file = upload_file(@project.repository, png)
       end
 
       it { expect(@link_to_file).to have_key('alt') }
@@ -45,7 +41,7 @@ describe Projects::FileService do
      context 'for valid jpg file' do
       before do
         jpg = fixture_file_upload(Rails.root + 'spec/fixtures/rails_sample.jpg', 'image/jpg')
-        @link_to_file = upload_file(@project.repository, { 'markdown_file' => jpg }, 'http://test.example/')
+        @link_to_file = upload_file(@project.repository, jpg)
       end
 
       it { expect(@link_to_file).to have_key('alt') }
@@ -60,9 +56,7 @@ describe Projects::FileService do
     context 'for txt file' do
       before do
         txt = fixture_file_upload(Rails.root + 'spec/fixtures/doc_sample.txt', 'text/plain')
-        @link_to_file = upload_file(@project.repository,
-          { 'markdown_file' => txt },
-          'http://test.example/')
+        @link_to_file = upload_file(@project.repository, txt)
       end
 
       it { expect(@link_to_file).to have_key('alt') }
@@ -75,7 +69,7 @@ describe Projects::FileService do
     end
   end
 
-  def upload_file(repository, params, root_url)
-    Projects::FileService.new(repository, params, root_url).execute
+  def upload_file(repository, file)
+    Projects::UploadService.new(repository, file).execute
   end
 end