Code review - Remote storage

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

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

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

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

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

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
 

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
 

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