From 745eee126af5b4bf1a26b1d35ca0ddccabd840f0 Mon Sep 17 00:00:00 2001 From: Guillaume Lazzara Date: Wed, 18 May 2016 11:43:32 +0200 Subject: [PATCH] Code review - Remote storage --- app/models/cerfa.rb | 8 ++++---- app/models/piece_justificative.rb | 8 ++++---- app/uploaders/cerfa_uploader.rb | 20 ++++++++----------- app/uploaders/piece_justificative_uploader.rb | 18 +++++++---------- app/uploaders/procedure_logo_uploader.rb | 14 +++++-------- spec/models/cerfa_spec.rb | 2 ++ spec/models/piece_justificative_spec.rb | 2 ++ spec/models/procedure_spec.rb | 1 + 8 files changed, 33 insertions(+), 40 deletions(-) diff --git a/app/models/cerfa.rb b/app/models/cerfa.rb index 277496d42..a106e6084 100644 --- a/app/models/cerfa.rb +++ b/app/models/cerfa.rb @@ -10,10 +10,10 @@ class Cerfa < ActiveRecord::Base end def content_url - if Features.remote_storage and !content.url.nil? - (RemoteDownloader.new content.filename).url - else - unless content.url.nil? + unless content.url.nil? + if Features.remote_storage + (RemoteDownloader.new content.filename).url + else (LocalDownloader.new content, 'CERFA').url end end diff --git a/app/models/piece_justificative.rb b/app/models/piece_justificative.rb index 18763e234..0ba0f6531 100644 --- a/app/models/piece_justificative.rb +++ b/app/models/piece_justificative.rb @@ -18,10 +18,10 @@ class PieceJustificative < ActiveRecord::Base end def content_url - if Features.remote_storage and !content.url.nil? - (RemoteDownloader.new content.filename).url - else - unless content.url.nil? + unless content.url.nil? + if Features.remote_storage + (RemoteDownloader.new content.filename).url + else (LocalDownloader.new content, (type_de_piece_justificative.nil? ? content.original_filename : type_de_piece_justificative.libelle)).url end diff --git a/app/uploaders/cerfa_uploader.rb b/app/uploaders/cerfa_uploader.rb index fe1b62342..8040d94ff 100644 --- a/app/uploaders/cerfa_uploader.rb +++ b/app/uploaders/cerfa_uploader.rb @@ -1,7 +1,7 @@ # encoding: utf-8 class CerfaUploader < CarrierWave::Uploader::Base - before :cache, :save_original_filename + before :cache, :set_original_filename # Choose what kind of storage to use for this uploader: if Features.remote_storage @@ -13,9 +13,7 @@ class CerfaUploader < CarrierWave::Uploader::Base # Override the directory where uploaded files will be stored. # This is a sensible default for uploaders that are meant to be mounted: def store_dir - if Features.remote_storage - nil - else + unless Features.remote_storage "../uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}" end end @@ -31,16 +29,14 @@ class CerfaUploader < CarrierWave::Uploader::Base end def filename - if original_filename || model.content_secure_token + if original_filename.present? || model.content_secure_token if Features.remote_storage - @filename = "#{model.class.to_s.underscore}-#{secure_token}.pdf" - else original_filename - @filename = "#{model.class.to_s.underscore}.pdf" + filename = "#{model.class.to_s.underscore}-#{secure_token}.pdf" + else + filename = "#{model.class.to_s.underscore}.pdf" end - else - @filename = nil end - @filename + filename end private @@ -53,7 +49,7 @@ class CerfaUploader < CarrierWave::Uploader::Base SecureRandom.uuid end - def save_original_filename(file) + def set_original_filename(file) model.original_filename ||= file.original_filename if file.respond_to?(:original_filename) end end diff --git a/app/uploaders/piece_justificative_uploader.rb b/app/uploaders/piece_justificative_uploader.rb index 2c754f01d..3fbc4b32f 100644 --- a/app/uploaders/piece_justificative_uploader.rb +++ b/app/uploaders/piece_justificative_uploader.rb @@ -1,7 +1,7 @@ # encoding: utf-8 class PieceJustificativeUploader < CarrierWave::Uploader::Base - before :cache, :save_original_filename + before :cache, :set_original_filename # Choose what kind of storage to use for this uploader: if Features.remote_storage @@ -13,9 +13,7 @@ class PieceJustificativeUploader < CarrierWave::Uploader::Base # Override the directory where uploaded files will be stored. # This is a sensible default for uploaders that are meant to be mounted: def store_dir - if Features.remote_storage - nil - else + unless Features.remote_storage "../uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}" end end @@ -31,16 +29,14 @@ class PieceJustificativeUploader < CarrierWave::Uploader::Base end def filename - if original_filename || model.content_secure_token + if original_filename.present? || model.content_secure_token if Features.remote_storage - @filename = "#{model.class.to_s.underscore}-#{secure_token}.pdf" + filename = "#{model.class.to_s.underscore}-#{secure_token}.pdf" else original_filename - @filename = "#{model.class.to_s.underscore}.pdf" + filename = "#{model.class.to_s.underscore}.pdf" end - else - @filename = nil end - @filename + filename end def original_filename @@ -57,7 +53,7 @@ class PieceJustificativeUploader < CarrierWave::Uploader::Base SecureRandom.uuid end - def save_original_filename(file) + def set_original_filename(file) model.original_filename ||= file.original_filename if file.respond_to?(:original_filename) end end diff --git a/app/uploaders/procedure_logo_uploader.rb b/app/uploaders/procedure_logo_uploader.rb index 8c4585132..6bd9b0690 100644 --- a/app/uploaders/procedure_logo_uploader.rb +++ b/app/uploaders/procedure_logo_uploader.rb @@ -12,9 +12,7 @@ class ProcedureLogoUploader < CarrierWave::Uploader::Base # Override the directory where uploaded files will be stored. # This is a sensible default for uploaders that are meant to be mounted: def store_dir - if Features.remote_storage - nil - else + unless Features.remote_storage "uploads/#{model.class.to_s.underscore}/#{mounted_as}/#{model.id}" end end @@ -30,16 +28,14 @@ class ProcedureLogoUploader < CarrierWave::Uploader::Base end def filename - if original_filename || model.logo_secure_token + if original_filename.present? || model.logo_secure_token if Features.remote_storage - @filename = "#{model.class.to_s.underscore}-#{secure_token}.pdf" + filename = "#{model.class.to_s.underscore}-#{secure_token}.pdf" else original_filename - @filename = "#{model.class.to_s.underscore}.pdf" + filename = "#{model.class.to_s.underscore}.pdf" end - else - @filename = nil end - @filename + filename end private diff --git a/spec/models/cerfa_spec.rb b/spec/models/cerfa_spec.rb index f2b00fddf..4ea8dd675 100644 --- a/spec/models/cerfa_spec.rb +++ b/spec/models/cerfa_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe Cerfa do describe 'database columns' do it { is_expected.to have_db_column(:content) } + it { is_expected.to have_db_column(:original_filename) } + it { is_expected.to have_db_column(:content_secure_token) } it { is_expected.to have_db_column(:created_at) } end diff --git a/spec/models/piece_justificative_spec.rb b/spec/models/piece_justificative_spec.rb index 3c66eaaaa..70abcb299 100644 --- a/spec/models/piece_justificative_spec.rb +++ b/spec/models/piece_justificative_spec.rb @@ -3,6 +3,8 @@ require 'spec_helper' describe PieceJustificative do describe 'database columns' do it { is_expected.to have_db_column(:content) } + it { is_expected.to have_db_column(:original_filename) } + it { is_expected.to have_db_column(:content_secure_token) } it { is_expected.to have_db_column(:created_at) } end diff --git a/spec/models/procedure_spec.rb b/spec/models/procedure_spec.rb index 066034086..6b6118104 100644 --- a/spec/models/procedure_spec.rb +++ b/spec/models/procedure_spec.rb @@ -17,6 +17,7 @@ describe Procedure do it { is_expected.to have_db_column(:test) } it { is_expected.to have_db_column(:euro_flag) } it { is_expected.to have_db_column(:logo) } + it { is_expected.to have_db_column(:logo_secure_token) } it { is_expected.to have_db_column(:cerfa_flag) } end