Merge pull request #94 in INTL/shuttle from yunus/update-multiple-tra…

…nslation-at-once to master

* commit '87432a373ce8d7c613f6a9d6c7921dcbadd1fbab':
  Update multiple translation at once

app/controllers/translations_controller.rb

@@ -106,21 +106,22 @@ def update
     # translators cannot modify approved copy
     return head(:forbidden) if @translation.approved? && current_user.role == 'translator'
 
-    TranslationUpdateMediator.new(@translation, current_user, params).update
+    mediator = TranslationUpdateMediator.new(@translation, current_user, params)
+    mediator.update!
 
     respond_with(@translation, location: project_key_translation_url(@project, @key, @translation)) do |format|
       format.json do
-        if @translation.valid?
+        if mediator.success?
           render json: decorate([@translation]).first
         else
-          render json: @translation.errors.full_messages, status: :unprocessable_entity
+          render json: mediator.errors, status: :unprocessable_entity
         end
       end
       format.html do
-        if @translation.errors.any?
-          redirect_to edit_project_key_translation_url(@project, @key, @translation), flash: { alert: @translation.errors.full_messages.unshift(t('controllers.translations.update.failure')) }
-        else
+        if mediator.success?
           redirect_to edit_project_key_translation_url(@project, @key, @translation), flash: { success: t('controllers.translations.update.success') }
+        else
+          redirect_to edit_project_key_translation_url(@project, @key, @translation), flash: { alert: mediator.errors.unshift(t('controllers.translations.update.failure')) }
         end
       end
     end

app/mediators/basic_mediator.rb

@@ -0,0 +1,60 @@
+# Copyright 2014 Square Inc.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License");
+#    you may not use this file except in compliance with the License.
+#    You may obtain a copy of the License at
+#
+#        http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS,
+#    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#    See the License for the specific language governing permissions and
+#    limitations under the License.
+
+# Implements basic mediator functionality such as keeping track of errors and
+# providing helper methods to interact with them.
+#
+# Fields
+# ======
+#
+# |          |                                                                                  |
+# |:---------|:---------------------------------------------------------------------------------|
+# | `errors` | an array which should keep track of errors that happened in the inheriting class |
+
+class BasicMediator
+
+  attr_reader :errors
+
+  def initialize
+    @errors = []
+  end
+
+  # @return [true, false] true if no errors have been recorded, false otherwise
+  def success?
+    @errors.blank?
+  end
+
+  # @return [true, false] true if any errors have been recorded, false otherwise
+  def failure?
+    !success?
+  end
+
+  private
+
+  # Add multiple errors at once
+  #
+  # @param [Array<String>] msgs a list of error messages in String format
+
+  def add_errors(msgs)
+    @errors.push(*msgs)
+  end
+
+  # Add a single error
+  #
+  # @param [String] msg an error message in String format
+
+  def add_error(msg)
+    @errors.push(msg)
+  end
+end

app/mediators/translation_update_mediator.rb

@@ -23,24 +23,38 @@
 # | `user`                | {User} who is making the change                                                           |
 # | `params`              | params from controller                                                                    |
 
-class TranslationUpdateMediator
+class TranslationUpdateMediator < BasicMediator
 
   # @param [Translation] primary_translation that will be updated
   # @param [User] user who is making the changes
   # @param [ActionController::Parameters] params that will be used to update the translation
 
   def initialize(primary_translation, user, params)
+    super()
     @primary_translation, @user, @params = primary_translation, user, params
   end
 
   # Updates this translation and its associated translations
-  def update
-    update_single_translation(@primary_translation, @params[:blank_string], @params.require(:translation).permit(:copy, :notes))
-    # TODO (yunus): update user specified associated translations
+  def update!
+    copy_to_translations = translations_that_should_be_multi_updated
+    return if failure?
+
+    Translation.transaction do
+      copy_to_translations.each do |translation|
+        update_single_translation!(translation)
+      end
+      update_single_translation!(@primary_translation)
+    end
+
+    @primary_translation.key.reload.recalculate_ready! # Readiness hooks were skipped in the transaction above, now we gotta run them.
+  rescue ActiveRecord::RecordInvalid => err
+    add_errors(err.record.errors.full_messages.map { |msg| "(#{err.record.rfc5646_locale}): #{msg}" })
   end
 
-  # Finds all translations that can be updated alongside the inputted {Translation} with the same copy.
-  # These translations make the keys of the returned hash. The values are the {LocaleAssociation LocaleAssociations}
+  # Finds all translations that is allowed to be updated alongside the inputted {Translation} with the same copy,
+  # based on {LocaleAssociation LocaleAssociations}. Doesn't include self, or base translation.
+  #
+  # These translations are the keys of the returned hash. The values are the {LocaleAssociation LocaleAssociations}
   # that tie the associated translations to the inputted translation.
   #
   # This should be used in 2 places:
@@ -64,24 +78,45 @@ def self.multi_updateable_translations_to_locale_associations_hash(translation)
 
   private
 
-  # @return [Array<Translation>] a list of translations that can be updated alongside the primary translation.
-  def multi_updateable_translations
-    self.class.multi_updateable_translations_to_locale_associations_hash(@primary_translation).keys
+  # Retrive all translations that should be updated alongside the primary translation, based on the `copyToLocales`
+  # field in user provided params. It maps the user provided `copyToLocales` to {Translation} objects.
+  #
+  # If one of the requested locales are not allowed to be updated, it adds an error and breaks out of the loop.
+  #
+  # The consumer of this function should check for errors before proceeding.
+  #
+  # @return [Array<Translation>] array of translations that should be updated alongside the primary translation
+
+  def translations_that_should_be_multi_updated
+    return [] unless @params[:copyToLocales]
+    translations_indexed_by_rfc5646_locale = self.class.multi_updateable_translations_to_locale_associations_hash(@primary_translation).keys.index_by(&:rfc5646_locale)
+    @params[:copyToLocales].map do |rfc5646_locale|
+      unless translations_indexed_by_rfc5646_locale.key?(rfc5646_locale)
+        add_error("Cannot update translation in locale #{rfc5646_locale}")
+        break
+      end
+      translations_indexed_by_rfc5646_locale[rfc5646_locale]
+    end
+  end
+
+  # @return [Hash<String, Translation>] a hash of rfc5646_locale to translations that is allowed to be updated alongside the primary translation.
+  def multi_updateable_translations_indexed_by_rfc5646_locale
+    @multi_updateable_translations_indexed_by_rfc5646_locale ||=
+        self.class.multi_updateable_translations_to_locale_associations_hash(@primary_translation).keys.index_by(&:rfc5646_locale)
   end
 
   # Updates a single translation.
   #
   # @param [Translation] translation that will be updated
-  # @param [true, false] is_blank_string true if the translation should be updated as an empty string, false otherwise
-  # @param [ActionController::Parameters] permitted_params that will be used to update the translation
+  # @raise [ActiveRecord::RecordInvalid] if translation is invalid
 
-  def update_single_translation(translation, is_blank_string, permitted_params)
+  def update_single_translation!(translation)
     translation.freeze_tracked_attributes
     translation.modifier = @user
-    translation.assign_attributes(permitted_params)
+    translation.assign_attributes(@params.require(:translation).permit(:copy, :notes))
 
     # un-translate translation if empty but blank_string is not specified
-    if translation.copy.blank? && !is_blank_string
+    if translation.copy.blank? && !@params[:blank_string].parse_bool
       untranslate(translation)
     else
       translation.translator = @user if translation.copy != translation.copy_was
@@ -91,7 +126,8 @@ def update_single_translation(translation, is_blank_string, permitted_params)
         translation.preserve_reviewed_status = true
       end
     end
-    translation.save
+    translation.skip_readiness_hooks = true
+    translation.save!
   end
 
   # Untranslates a translation, but doesn't call `save`.

spec/controllers/translations_controller_spec.rb

@@ -318,6 +318,47 @@
         expect(translation.translation_changes.count).to eq(1)
       end
     end
+
+    context "[multi-locale update]" do
+      before :each do
+        @user.update role: 'reviewer'
+
+        @project = FactoryGirl.create(:project, targeted_rfc5646_locales: { 'fr' => true, 'fr-CA' =>true, 'fr-FR' => true } )
+        @key = FactoryGirl.create(:key, ready: false, project:@project)
+        @fr_translation    = FactoryGirl.create(:translation, key: @key, copy: nil, translator: nil, rfc5646_locale: 'fr')
+        @fr_CA_translation = FactoryGirl.create(:translation, key: @key, copy: nil, translator: nil, rfc5646_locale: 'fr-CA')
+        @fr_FR_translation = FactoryGirl.create(:translation, key: @key, copy: nil, translator: nil, rfc5646_locale: 'fr-FR')
+        @fr_to_fr_CA = FactoryGirl.create(:locale_association, source_rfc5646_locale: 'fr', target_rfc5646_locale: 'fr-CA')
+        @fr_to_fr_FR = FactoryGirl.create(:locale_association, source_rfc5646_locale: 'fr', target_rfc5646_locale: 'fr-FR')
+        expect(@key.reload).to_not be_ready
+      end
+
+      it "updates the primary translation and 2 associated translations that user specifies" do
+        patch :update, project_id: @project.to_param, key_id: @key.to_param, id: @fr_translation.to_param, translation: { copy: "test" }, copyToLocales: %w(fr-CA fr-FR), format: 'json'
+
+        expect(@fr_translation.reload.copy).to eql("test")
+        expect(@fr_translation.translator).to eql(@user)
+        expect(@fr_CA_translation.reload.copy).to eql("test")
+        expect(@fr_CA_translation.translator).to eql(@user)
+        expect(@fr_FR_translation.reload.copy).to eql("test")
+        expect(@fr_FR_translation.translator).to eql(@user)
+        expect(@key.reload.ready).to be_true
+      end
+
+      it "doesn't update any of the requested translations because one of the translations are not linked to the primary one with a LocaleAssociation" do
+        @fr_to_fr_CA.delete
+
+        patch :update, project_id: @project.to_param, key_id: @key.to_param, id: @fr_translation.to_param, translation: { copy: "test" }, copyToLocales: %w(fr-CA fr-FR), format: 'json'
+
+        expect(@fr_translation.reload.copy).to be_nil
+        expect(@fr_translation.translator).to be_nil
+        expect(@fr_CA_translation.reload.copy).to be_nil
+        expect(@fr_CA_translation.translator).to be_nil
+        expect(@fr_FR_translation.reload.copy).to be_nil
+        expect(@fr_FR_translation.translator).to be_nil
+        expect(@key.reload.ready).to be_false
+      end
+    end
   end
 
   describe "#approve" do

spec/controllers/users_controller_spec.rb

@@ -95,14 +95,6 @@
                                                             {"name"=>"test2 user3",  "email"=>"[email protected]"},
                                                             {"name"=>"1test2 user4", "email"=>"[email protected]"}])
         end
-
-        it "returns fuzzy matched users' email addresses and names" do
-          pending # TODO (yunus): implement fuzzy matching
-          FactoryGirl.create(:user, :activated, first_name: "tast",   last_name: "user1", email: '[email protected]') # fuzzy match
-          FactoryGirl.create(:user, :activated, first_name: "attast", last_name: "user2", email: '[email protected]') # miss
-          subject
-          expect(JSON.parse(response.body)).to eql([{"name"=>"tast user1", "email"=>"[email protected]"}])
-        end
       end
 
       context "[matching by last name]" do
@@ -118,14 +110,6 @@
                                                             {"name"=>"user3 test2",  "email"=>"[email protected]"},
                                                             {"name"=>"user4 1test2", "email"=>"[email protected]"}])
           end
-
-        it "returns fuzzy matched users' email addresses and names" do
-          pending # TODO (yunus): implement fuzzy matching
-          FactoryGirl.create(:user, :activated, first_name: "user1", last_name: "tast",   email: '[email protected]') # fuzzy match
-          FactoryGirl.create(:user, :activated, first_name: "user2", last_name: "attast", email: '[email protected]') # miss
-          subject
-          expect(JSON.parse(response.body)).to eql([{"name"=>"user1 tast", "email"=>"[email protected]"}])
-        end
       end
 
 

spec/factories/users.rb

@@ -27,5 +27,17 @@
       role { 'monitor' }
       confirmed_at { Time.now }
     end
+
+    trait :translator do
+      role { 'translator' }
+    end
+
+    trait :reviewer do
+      role { 'reviewer' }
+    end
+
+    trait :admin do
+      role { 'admin' }
+    end
   end
 end

spec/mediators/basic_mediator_spec.rb

@@ -0,0 +1,59 @@
+# Copyright 2014 Square Inc.
+#
+#    Licensed under the Apache License, Version 2.0 (the "License");
+#    you may not use this file except in compliance with the License.
+#    You may obtain a copy of the License at
+#
+#        http://www.apache.org/licenses/LICENSE-2.0
+#
+#    Unless required by applicable law or agreed to in writing, software
+#    distributed under the License is distributed on an "AS IS" BASIS,
+#    WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+#    See the License for the specific language governing permissions and
+#    limitations under the License.
+
+require 'spec_helper'
+
+describe BasicMediator do
+  before :each do
+    @mediator = BasicMediator.new
+  end
+
+  describe "#success?" do
+    it "returns true if there are no errors" do
+      expect(@mediator.success?).to eql(true)
+    end
+
+    it "returns false if there are any errors" do
+      @mediator.send(:add_error, "Test")
+      expect(@mediator.success?).to eql(false)
+    end
+  end
+
+  describe "#failure?" do
+    it "returns true if there are any errors" do
+      @mediator.send(:add_error, "Test")
+      expect(@mediator.failure?).to eql(true)
+    end
+
+    it "returns false if there are no errors" do
+      expect(@mediator.failure?).to eql(false)
+    end
+  end
+
+  describe "#add_error" do
+    it "adds an error to mediator" do
+      @mediator.send(:add_error, "Test1")
+      @mediator.send(:add_error, "Test2")
+      expect(@mediator.errors).to eql(["Test1", "Test2"])
+    end
+  end
+
+  describe "#add_errors" do
+    it "adds errors in bulk to mediator" do
+      @mediator.send(:add_errors, ["Test1", "Test2"])
+      @mediator.send(:add_errors, ["Test3", "Test4"])
+      expect(@mediator.errors).to eql(["Test1", "Test2", "Test3", "Test4"])
+    end
+  end
+end