Only call File.read once for import (#27)

westonganger · Feb 5, 2025 · 37c747b · 37c747b
1 parent f0ec2c6
commit 37c747b
Show file tree

Hide file tree

Showing 5 changed files with 45 additions and 33 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -1,7 +1,7 @@
 # CHANGELOG
 
 ### Unreleased - [View Diff](https://github.com/westonganger/rails_i18n_manager/compare/v1.1.1...master)
-- Nothing yet
+- [#27](https://github.com/westonganger/rails_i18n_manager/pull/27) - Only call File.read once for import
 
 ### v1.1.1 - Febuary 4, 2025 - [View Diff](https://github.com/westonganger/rails_i18n_manager/compare/v1.1.0...v1.1.1)
 - [#26](https://github.com/westonganger/rails_i18n_manager/pull/26) - Fix file not found issues with file import

diff --git a/app/controllers/rails_i18n_manager/translations_controller.rb b/app/controllers/rails_i18n_manager/translations_controller.rb
@@ -96,34 +96,29 @@ def delete_inactive_keys
     end
 
     def import
-      @form = Forms::TranslationFileForm.new(params[:import_form])
-
       if request.get?
+        @form = Forms::TranslationFileForm.new
         render
       else
-        if @form.valid?
-          if @form.file.path.end_with?(".json")
-            parsed_file_contents = JSON.parse(@form.file.read)
-          else
-            parsed_file_contents = YAML.safe_load(@form.file.read)
-          end
+        @form = Forms::TranslationFileForm.new(params[:import_form])
 
+        if @form.valid?
           begin
-            TranslationsImportJob.new.perform(
+            TranslationsImporter.import(
               translation_app_id: @form.translation_app_id,
-              parsed_file_contents: parsed_file_contents,
+              parsed_file_contents: @form.parsed_file_contents,
               overwrite_existing: @form.overwrite_existing,
               mark_inactive_translations: @form.mark_inactive_translations,
             )
-          rescue TranslationsImportJob::ImportAbortedError => e
+          rescue TranslationsImporter::ImportAbortedError => e
             flash.now.alert = e.message
             render
             return
           end
 
           redirect_to translations_path, notice: "Import Successful"
         else
-          flash.now.alert = "Import not started due to form errors."
+          flash.now.alert = "Please see form errors below"
           render
         end
       end

diff --git a/app/lib/rails_i18n_manager/forms/translation_file_form.rb b/app/lib/rails_i18n_manager/forms/translation_file_form.rb
@@ -17,36 +17,53 @@ def mark_inactive_translations=(val)
         @mark_inactive_translations = ["1", "true", "t"].include?(val.to_s.downcase)
       end
 
+      def file_extname
+        @file_extname ||= File.extname(file)
+      end
+
+      def file_contents_string
+        @file_contents_string ||= file.read
+      end
+
+      def parsed_file_contents
+        if defined?(@parsed_file_contents)
+          return @parsed_file_contents
+        end
+
+        case file_extname
+        when ".yml", ".yaml"
+          @parsed_file_contents = YAML.safe_load(file_contents_string)
+        when ".json"
+          @parsed_file_contents = JSON.parse(file_contents_string)
+        end
+      end
+
       def validate_file
         if file.blank?
           errors.add(:file, "Must upload a valid translation file.")
           return
         end
 
-        file_extname = File.extname(file)
-
         if [".yml", ".yaml", ".json"].exclude?(file_extname)
           errors.add(:file, "Invalid file format. Must be yaml or json file.")
           return
         end
 
-        file_contents = File.read(file)
-
-        if file_contents.blank?
+        if file_contents_string.blank?
           errors.add(:file, "Empty file provided.")
           return
         end
 
         case file_extname
         when ".yml", ".yaml"
-          if !YAML.safe_load(file_contents).is_a?(Hash)
+          if !parsed_file_contents.is_a?(Hash)
             errors.add(:file, "Invalid #{file_extname.sub(".","")} file.")
             return
           end
 
         when ".json"
           begin
-            JSON.parse(file_contents)
+            parsed_file_contents
           rescue JSON::ParserError
             errors.add(:file, "Invalid json file.")
             return

diff --git a/...s_i18n_manager/translations_import_job.rb → ...ils_i18n_manager/translations_importer.rb b/...s_i18n_manager/translations_import_job.rb → ...ils_i18n_manager/translations_importer.rb
@@ -1,9 +1,9 @@
 module RailsI18nManager
-  class TranslationsImportJob
+  class TranslationsImporter
 
     class ImportAbortedError < StandardError; end
 
-    def perform(translation_app_id:, parsed_file_contents:, overwrite_existing: false, mark_inactive_translations: false)
+    def self.import(translation_app_id:, parsed_file_contents:, overwrite_existing: false, mark_inactive_translations: false)
       app_record = TranslationApp.find(translation_app_id)
 
       new_locales = parsed_file_contents.keys - app_record.all_locales

diff --git a/...unit/jobs/translations_import_job_spec.rb → spec/unit/translations_importer_spec.rb b/...unit/jobs/translations_import_job_spec.rb → spec/unit/translations_importer_spec.rb
@@ -1,7 +1,7 @@
 require "spec_helper"
 
 module RailsI18nManager
-  RSpec.describe TranslationsImportJob, type: :model do
+  RSpec.describe TranslationsImporter, type: :model do
 
     let(:translation_app){ FactoryBot.create(:translation_app, default_locale: :en, additional_locales: [:fr]) }
 
@@ -23,8 +23,8 @@ module RailsI18nManager
       YAML
 
       expect do
-        TranslationsImportJob.new.perform(translation_app_id: translation_app.id, parsed_file_contents: YAML.safe_load(yaml))
-      end.to raise_error(TranslationsImportJob::ImportAbortedError)
+        TranslationsImporter.import(translation_app_id: translation_app.id, parsed_file_contents: YAML.safe_load(yaml))
+      end.to raise_error(TranslationsImporter::ImportAbortedError)
     end
 
     it "creates the correct amount of TranslationKey and TranslationValue" do
@@ -43,7 +43,7 @@ module RailsI18nManager
           fr_only_key: fr_only
       YAML
 
-      TranslationsImportJob.new.perform(translation_app_id: translation_app.id, parsed_file_contents: YAML.safe_load(yaml))
+      TranslationsImporter.import(translation_app_id: translation_app.id, parsed_file_contents: YAML.safe_load(yaml))
 
       translation_app.reload
 
@@ -69,7 +69,7 @@ module RailsI18nManager
             foo: updated
         YAML
 
-        TranslationsImportJob.new.perform(translation_app_id: translation_app.id, parsed_file_contents: YAML.safe_load(yaml), overwrite_existing: true)
+        TranslationsImporter.import(translation_app_id: translation_app.id, parsed_file_contents: YAML.safe_load(yaml), overwrite_existing: true)
 
         foo_value.reload
 
@@ -105,7 +105,7 @@ module RailsI18nManager
             bar: bar_updated
         YAML
 
-        TranslationsImportJob.new.perform(translation_app_id: translation_app.id, parsed_file_contents: YAML.safe_load(yaml), overwrite_existing: false)
+        TranslationsImporter.import(translation_app_id: translation_app.id, parsed_file_contents: YAML.safe_load(yaml), overwrite_existing: false)
 
         foo_value.reload
         blank_value.reload
@@ -124,7 +124,7 @@ module RailsI18nManager
             baz:
         YAML
 
-        TranslationsImportJob.new.perform(translation_app_id: translation_app.id, parsed_file_contents: YAML.safe_load(yaml), mark_inactive_translations: true)
+        TranslationsImporter.import(translation_app_id: translation_app.id, parsed_file_contents: YAML.safe_load(yaml), mark_inactive_translations: true)
         translation_app.translation_keys.reload
         expect(translation_app.translation_keys.select(&:active).size).to eq(3)
         expect(translation_app.translation_keys.reject(&:active).size).to eq(0)
@@ -136,7 +136,7 @@ module RailsI18nManager
             baz:
         YAML
 
-        TranslationsImportJob.new.perform(translation_app_id: translation_app.id, parsed_file_contents: YAML.safe_load(yaml), mark_inactive_translations: true)
+        TranslationsImporter.import(translation_app_id: translation_app.id, parsed_file_contents: YAML.safe_load(yaml), mark_inactive_translations: true)
         translation_app.translation_keys.reload
         expect(translation_app.translation_keys.select(&:active).size).to eq(2)
         expect(translation_app.translation_keys.reject(&:active).size).to eq(1)
@@ -148,7 +148,7 @@ module RailsI18nManager
             baz:
         YAML
 
-        TranslationsImportJob.new.perform(translation_app_id: translation_app.id, parsed_file_contents: YAML.safe_load(yaml), mark_inactive_translations: true)
+        TranslationsImporter.import(translation_app_id: translation_app.id, parsed_file_contents: YAML.safe_load(yaml), mark_inactive_translations: true)
         translation_app.translation_keys.reload
         expect(translation_app.translation_keys.select(&:active).size).to eq(3)
         expect(translation_app.translation_keys.reject(&:active).size).to eq(0)
@@ -162,7 +162,7 @@ module RailsI18nManager
             baz:
         YAML
 
-        TranslationsImportJob.new.perform(translation_app_id: translation_app.id, parsed_file_contents: YAML.safe_load(yaml), mark_inactive_translations: false)
+        TranslationsImporter.import(translation_app_id: translation_app.id, parsed_file_contents: YAML.safe_load(yaml), mark_inactive_translations: false)
         expect(translation_app.translation_keys.select(&:active).size).to eq(3)
         expect(translation_app.translation_keys.reject(&:active).size).to eq(0)
 
@@ -173,7 +173,7 @@ module RailsI18nManager
             baz:
         YAML
 
-        TranslationsImportJob.new.perform(translation_app_id: translation_app.id, parsed_file_contents: YAML.safe_load(yaml), mark_inactive_translations: false)
+        TranslationsImporter.import(translation_app_id: translation_app.id, parsed_file_contents: YAML.safe_load(yaml), mark_inactive_translations: false)
         expect(translation_app.translation_keys.select(&:active).size).to eq(3)
         expect(translation_app.translation_keys.reject(&:active).size).to eq(0)
       end