Fix submission of form containing both existing and new translations

8 files changed, 140 insertions, 121 deletions

diff --git a/app/helpers/admin_public_body_helper.rb b/app/helpers/admin_public_body_helper.rb
index 5139bd49f..97f656ddb 100644
--- a/app/helpers/admin_public_body_helper.rb
+++ b/app/helpers/admin_public_body_helper.rb
@@ -7,11 +7,11 @@ module AdminPublicBodyHelper
             object = public_body
         else
             # ...but additional locales go "on the side"
-            prefix = 'public_body[translated_versions][]'
+            prefix = :translations
             object = if public_body.new_record?
                          PublicBody::Translation.new
                      else
-                        public_body.find_translation_by_locale(locale.to_s)
+                         public_body.find_translation_by_locale(locale.to_s)
                      end
             object ||= PublicBody::Translation.new
         end
diff --git a/app/models/public_body.rb b/app/models/public_body.rb
index a434cce35..d3544e13c 100644
--- a/app/models/public_body.rb
+++ b/app/models/public_body.rb
@@ -64,6 +64,7 @@ class PublicBody < ActiveRecord::Base
     }
 
     translates :name, :short_name, :request_email, :url_name, :notes, :first_letter, :publication_scheme
+    accepts_nested_attributes_for :translations
 
     # Default fields available for importing from CSV, in the format
     # [field_name, 'short description of field (basic html allowed)']
@@ -151,12 +152,11 @@ class PublicBody < ActiveRecord::Base
         translations
     end
 
-    def translated_versions=(translation_attrs)
+    def translations_attributes=(translation_attrs)
         def empty_translation?(attrs)
             attrs_with_values = attrs.select{ |key, value| value != '' and key.to_s != 'locale' }
             attrs_with_values.empty?
         end
-
         if translation_attrs.respond_to? :each_value    # Hash => updating
             translation_attrs.each_value do |attrs|
                 next if empty_translation?(attrs)
diff --git a/app/views/admin_public_body/_form.html.erb b/app/views/admin_public_body/_form.html.erb
index 6b573ac9e..177873984 100644
--- a/app/views/admin_public_body/_form.html.erb
+++ b/app/views/admin_public_body/_form.html.erb
@@ -9,53 +9,22 @@
   <% end %>
   </ul>
   <div class="tab-content">
-<% I18n.available_locales.each do |locale| %>
-    <% context = public_body_form_object(@public_body, locale) %>
-
-    <%= fields_for context[:prefix], context[:object] do |t| %>
-    <div class="tab-pane" id="div-locale-<%=locale.to_s%>">
-      <div class="control-group">
-        <%= t.hidden_field :locale, :value => locale.to_s %>
-        <label for="<%= form_tag_id(t.object_name, :name, locale) %>" class="control-label">Name</label>
-        <div class="controls">
-          <%= t.text_field :name, :id => form_tag_id(t.object_name, :name, locale), :class => "span4" %>
-        </div>
-      </div>
-      <div class="control-group">
-        <label for="<%= form_tag_id(t.object_name, :short_name, locale) %>", class="control-label"><%=_("Short name")%></label>
-        <div class="controls">
-          <%= t.text_field :short_name, :id => form_tag_id(t.object_name, :short_name, locale), :class => "span2"  %>
-          <p class="help-block"><%=_("Only put in abbreviations which are really used, otherwise leave blank. Short or long name is used in the URL – don't worry about breaking URLs through renaming, as the history is used to redirect")%></p>
-        </div>
-      </div>
-      <div class="control-group">
-        <label for="<%= form_tag_id(t.object_name, :request_email, locale) %>" class="control-label"><%=_("Request email")%></label>
-        <div class="controls">
-          <%= t.text_field :request_email, :id => form_tag_id(t.object_name, :request_email, locale), :class => "span3" %>
-          <p class="help-block"><%=_("set to <strong>blank</strong> (empty string) if can't find an address; these emails are <strong>public</strong> as anyone can view with a CAPTCHA")%></p>
-        </div>
-      </div>
-      <div class="control-group">
-        <label for="<%= form_tag_id(t.object_name, :publication_scheme, locale) %>" class="control-label"><%=_("Publication scheme URL")%></label>
-        <div class="controls">
-          <%= t.text_field :publication_scheme, :size => 60, :id => form_tag_id(t.object_name, :publication_scheme, locale), :class => "span3" %>
-        </div>
-      </div>
-      <div class="control-group">
-        <label for="<%= form_tag_id(t.object_name, :notes, locale) %>" class="control-label"><%=_("Public notes")%></label>
-        <div class="controls">
-          <%= t.text_area :notes, :rows => 3, :id => form_tag_id(t.object_name, :notes, locale), :class => "span6" %>
-          <p class="help-block">
-            HTML, for users to consider when making FOI requests to the authority
-          </p>
-        </div>
-      </div>
-    </div>
+  <% I18n.available_locales.each do |locale| %>
+      <% context = public_body_form_object(@public_body, locale) %>
+      <% if locale.to_s == I18n.default_locale.to_s %>
+        <%= fields_for('public_body', context[:object]) do |t| %>
+          <%= render :partial => 'locale_fields', :locals => {:t => t, :locale => locale}%>
+        <% end %>
+      <% else %>
+        <%= f.fields_for(context[:prefix], context[:object], :child_index => locale) do |t| %>
+          <%= render :partial => 'locale_fields' , :locals => {:t => t, :locale => locale}%>
+        <% end %>
+      <% end %>
   <% end %>
-<% end %>
   </div>
 </div>
 
+
 <h3>Common Fields</h3>
 <div class="control-group">
   <label for="public_body_tag_string" class="control-label"><%=_("Tags")%></label>
diff --git a/app/views/admin_public_body/_locale_fields.html.erb b/app/views/admin_public_body/_locale_fields.html.erb
new file mode 100644
index 000000000..cbe008fe2
--- /dev/null
+++ b/app/views/admin_public_body/_locale_fields.html.erb
@@ -0,0 +1,49 @@
+
+<div class="tab-pane" id="div-locale-<%=locale.to_s%>">
+  <div class="control-group">
+    <%= t.hidden_field :locale, :value => locale.to_s %>
+    <label for="<%= form_tag_id(t.object_name, :name, locale) %>" class="control-label">Name</label>
+    <div class="controls">
+      <%= t.text_field :name, :id => form_tag_id(t.object_name, :name, locale), :class => "span4" %>
+    </div>
+  </div>
+
+  <div class="control-group">
+    <label for="<%= form_tag_id(t.object_name, :short_name, locale) %>" class="control-label"><%=_("Short name")%></label>
+    <div class="controls">
+      <%= t.text_field :short_name, :id => form_tag_id(t.object_name, :short_name, locale), :class => "span2"  %>
+      <p class="help-block">
+        <%=_("Only put in abbreviations which are really used, otherwise leave blank. Short or long name is used in the URL – don't worry about breaking URLs through renaming, as the history is used to redirect")%>
+      </p>
+    </div>
+  </div>
+
+  <div class="control-group">
+    <label for="<%= form_tag_id(t.object_name, :request_email, locale) %>" class="control-label"><%=_("Request email")%></label>
+    <div class="controls">
+      <%= t.text_field :request_email, :id => form_tag_id(t.object_name, :request_email, locale), :class => "span3" %>
+      <p class="help-block">
+        <%=_("set to <strong>blank</strong> (empty string) if can't find an address; these emails are <strong>public</strong> as anyone can view with a CAPTCHA")%>
+      </p>
+    </div>
+  </div>
+
+  <div class="control-group">
+    <label for="<%= form_tag_id(t.object_name, :publication_scheme, locale) %>" class="control-label"><%=_("Publication scheme URL")%></label>
+    <div class="controls">
+      <%= t.text_field :publication_scheme, :size => 60, :id => form_tag_id(t.object_name, :publication_scheme, locale), :class => "span3" %>
+    </div>
+  </div>
+
+  <div class="control-group">
+    <label for="<%= form_tag_id(t.object_name, :notes, locale) %>" class="control-label"><%=_("Public notes")%></label>
+    <div class="controls">
+      <%= t.text_area :notes, :rows => 3, :id => form_tag_id(t.object_name, :notes, locale), :class => "span6" %>
+      <p class="help-block">
+        HTML, for users to consider when making FOI requests to the authority
+      </p>
+    </div>
+  </div>
+
+</div>
+
diff --git a/spec/controllers/admin_public_body_controller_spec.rb b/spec/controllers/admin_public_body_controller_spec.rb
index 8aebbc9ed..5b8ed6c55 100644
--- a/spec/controllers/admin_public_body_controller_spec.rb
+++ b/spec/controllers/admin_public_body_controller_spec.rb
@@ -88,11 +88,13 @@ describe AdminPublicBodyController, "when creating a public body" do
                               :tag_string => "blah",
                               :request_email => 'newquango@localhost',
                               :last_edit_comment => 'From test code',
-                :translated_versions => [{ :locale => "es",
-                                           :name => "Mi Nuevo Quango",
-                                           :short_name => "",
-                                           :request_email => 'newquango@localhost' }]
-                }
+                              :translations_attributes => {
+                                  'es' => { :locale => "es",
+                                            :name => "Mi Nuevo Quango",
+                                            :short_name => "",
+                                            :request_email => 'newquango@localhost' }
+                              }
+            }
         }
         PublicBody.count.should == n + 1
 
@@ -209,17 +211,19 @@ describe AdminPublicBodyController, "when updating a public body" do
                 :tag_string => "some tags",
                 :request_email => 'edited@localhost',
                 :last_edit_comment => 'From test code',
-                :translations_attributes => [
-                    { :locale => "es",
-                      :name => "El Department for Humpadinking",
-                      :short_name => "",
-                      :request_email => 'edited@localhost' }
-                ]
+                :translations_attributes => {
+                    'es' => { :locale => "es",
+                              :name => "El Department for Humpadinking",
+                              :short_name => "",
+                              :request_email => 'edited@localhost' }
+                }
             }
         }
 
         request.flash[:notice].should include('successful')
 
+        pb = PublicBody.find(public_bodies(:humpadink_public_body).id)
+
         I18n.with_locale(:es) do
            expect(pb.name).to eq('El Department for Humpadinking')
         end
@@ -238,21 +242,23 @@ describe AdminPublicBodyController, "when updating a public body" do
                 :tag_string => "some tags",
                 :request_email => 'edited@localhost',
                 :last_edit_comment => 'From test code',
-                :translations_attributes => [
-                    { :locale => "es",
-                      :name => "El Department for Humpadinking",
-                      :short_name => "",
-                      :request_email => 'edited@localhost' },
-                    { :locale => "fr",
-                      :name => "Le Department for Humpadinking",
-                      :short_name => "",
-                      :request_email => 'edited@localhost' }
-                ]
+                :translations_attributes => {
+                    'es' => { :locale => "es",
+                              :name => "El Department for Humpadinking",
+                              :short_name => "",
+                              :request_email => 'edited@localhost' },
+                    'fr' => { :locale => "fr",
+                              :name => "Le Department for Humpadinking",
+                              :short_name => "",
+                              :request_email => 'edited@localhost' }
+                }
             }
         }
 
         request.flash[:notice].should include('successful')
 
+        pb = PublicBody.find(public_bodies(:humpadink_public_body).id)
+
         I18n.with_locale(:es) do
            expect(pb.name).to eq('El Department for Humpadinking')
         end
@@ -265,7 +271,7 @@ describe AdminPublicBodyController, "when updating a public body" do
     it 'updates an existing translation and adds a third translation' do
         pb = public_bodies(:humpadink_public_body)
 
-        post :update, {
+        put :update, {
             :id => pb.id,
             :public_body => {
                 :name => "Department for Humpadinking",
@@ -273,25 +279,25 @@ describe AdminPublicBodyController, "when updating a public body" do
                 :tag_string => "some tags",
                 :request_email => 'edited@localhost',
                 :last_edit_comment => 'From test code',
-                :translations_attributes => [
+                :translations_attributes => {
                     # Update existing translation
-                    { :locale => "es",
-                      :name => "Renamed Department for Humpadinking",
-                      :short_name => "",
-                      :request_email => 'edited@localhost' },
+                    'es' => { :locale => "es",
+                              :name => "Renamed Department for Humpadinking",
+                              :short_name => "",
+                              :request_email => 'edited@localhost' },
                     # Add new translation
-                    { :locale => "fr",
-                      :name => "Le Department for Humpadinking",
-                      :short_name => "",
-                      :request_email => 'edited@localhost' }
-                ]
+                    'fr' => { :locale => "fr",
+                              :name => "Le Department for Humpadinking",
+                              :short_name => "",
+                              :request_email => 'edited@localhost' }
+                }
             }
         }
 
-        pb.reload
-
         request.flash[:notice].should include('successful')
 
+        pb = PublicBody.find(public_bodies(:humpadink_public_body).id)
+
         I18n.with_locale(:es) do
            expect(pb.name).to eq('Renamed Department for Humpadinking')
         end
@@ -299,6 +305,7 @@ describe AdminPublicBodyController, "when updating a public body" do
         I18n.with_locale(:fr) do
            expect(pb.name).to eq('Le Department for Humpadinking')
         end
+
     end
 
     it "saves edits to a public body in another locale" do
@@ -313,11 +320,11 @@ describe AdminPublicBodyController, "when updating a public body" do
                     :tag_string => "some tags",
                     :request_email => 'edited@localhost',
                     :last_edit_comment => 'From test code',
-                    :translated_versions => {
-                        3 => {:locale => "es",
-                              :name => "Renamed",
-                              :short_name => "",
-                              :request_email => 'edited@localhost'}
+                    :translations_attributes => {
+                        'es' => { :locale => "es",
+                                  :name => "Renamed",
+                                  :short_name => "",
+                                  :request_email => 'edited@localhost' }
                         }
                     }
                 }
@@ -325,11 +332,13 @@ describe AdminPublicBodyController, "when updating a public body" do
         end
 
         pb = PublicBody.find(public_bodies(:humpadink_public_body).id)
+
         I18n.with_locale(:es) do
-           pb.name.should == "Renamed"
+           expect(pb.name).to eq('Renamed')
         end
+
         I18n.with_locale(:en) do
-           pb.name.should == "Department for Humpadinking"
+           expect(pb.name).to eq('Department for Humpadinking')
         end
 
     end
diff --git a/spec/helpers/admin_public_body_helper_spec.rb b/spec/helpers/admin_public_body_helper_spec.rb
index defb1b359..c135ef348 100644
--- a/spec/helpers/admin_public_body_helper_spec.rb
+++ b/spec/helpers/admin_public_body_helper_spec.rb
@@ -27,11 +27,11 @@ describe AdminPublicBodyHelper do
 
         context 'in an alternative locale' do
 
-            it 'provides the prefix public_body[translated_versions][]' do
+            it 'provides the prefix :translations' do
                 public_body = FactoryGirl.build(:public_body)
                 locale = :es
                 prefix = public_body_form_object(public_body, locale)[:prefix]
-                expect(prefix).to eq('public_body[translated_versions][]')
+                expect(prefix).to eq(:translations)
             end
 
             context 'when the PublicBody is new' do
diff --git a/spec/integration/admin_public_body_edit_spec.rb b/spec/integration/admin_public_body_edit_spec.rb
index 83174fa80..d75ded6fe 100644
--- a/spec/integration/admin_public_body_edit_spec.rb
+++ b/spec/integration/admin_public_body_edit_spec.rb
@@ -19,9 +19,9 @@ describe 'Editing a Public Body' do
 
     it 'can add a translation for a single locale' do
         expect(@body.find_translation_by_locale('fr')).to be_nil
-        
+
         @admin.visit admin_body_edit_path(@body)
-        @admin.fill_in 'public_body_translated_versions__name__fr', :with => 'New Quango FR'
+        @admin.fill_in 'public_body_translations_attributes_fr_name__fr', :with => 'New Quango FR'
         @admin.click_button 'Save'
 
         pb = PublicBody.find_by_name('New Quango')
@@ -30,17 +30,17 @@ describe 'Editing a Public Body' do
         end
     end
 
-    it 'can add a translation for multiple locales' do
+    it 'can add a translation for multiple locales', :focus => true do
         # Add FR translation
         expect(@body.find_translation_by_locale('fr')).to be_nil
         @admin.visit admin_body_edit_path(@body)
-        @admin.fill_in 'public_body_translated_versions__name__fr', :with => 'New Quango FR'
+        @admin.fill_in 'public_body_translations_attributes_fr_name__fr', :with => 'New Quango FR'
         @admin.click_button 'Save'
 
         # Add ES translation
         expect(@body.find_translation_by_locale('es')).to be_nil
-        @admin.visit admin_body_edit_path(@body)        
-        @admin.fill_in 'public_body_translated_versions__name__es', :with => 'New Quango ES'
+        @admin.visit admin_body_edit_path(@body)
+        @admin.fill_in 'public_body_translations_attributes_es_name__es', :with => 'New Quango ES'
         @admin.click_button 'Save'
 
         pb = PublicBody.find_by_name('New Quango')
diff --git a/spec/models/public_body_spec.rb b/spec/models/public_body_spec.rb
index bdfaab6bf..f12582f21 100644
--- a/spec/models/public_body_spec.rb
+++ b/spec/models/public_body_spec.rb
@@ -30,23 +30,23 @@ require File.expand_path(File.dirname(__FILE__) + '/../spec_helper')
 
 describe PublicBody do
 
-    describe :translated_versions= do
+    describe :translations_attributes= do
 
         context 'translation_attrs is a Hash' do
 
             it 'takes the correct code path for a Hash' do
                 attrs = {}
                 attrs.should_receive(:each_value)
-                PublicBody.new().translated_versions = attrs
+                PublicBody.new().translations_attributes = attrs
             end
 
             it 'updates an existing translation' do
                 body = public_bodies(:geraldine_public_body)
                 translation = body.translation_for(:es)
-                params = { translation.id.to_sym => { :locale => 'es',
-                                                      :name => 'Renamed' } }
+                params = { 'es' => { :locale => 'es',
+                                     :name => 'Renamed' } }
 
-                body.translated_versions = params
+                body.translations_attributes = params
                 I18n.with_locale(:es) { expect(body.name).to eq('Renamed') }
             end
 
@@ -56,15 +56,11 @@ describe PublicBody do
 
                 expect(body.translations.size).to eq(2)
 
-                body.translated_versions = {
-                    translation.id.to_sym => {
-                        :locale => 'es',
-                        :name => 'Renamed'
-                    },
-                    :new_translation => {
-                        :locale => 'fr',
-                        :name => 'Le Geraldine Quango'
-                    }
+                body.translations_attributes = {
+                    'es' => { :locale => 'es',
+                              :name => 'Renamed' },
+                    'fr' => { :locale => 'fr',
+                              :name => 'Le Geraldine Quango' }
                 }
 
                 expect(body.translations.size).to eq(3)
@@ -78,14 +74,10 @@ describe PublicBody do
 
                 expect(body.translations.size).to eq(2)
 
-                body.translated_versions = {
-                    translation.id.to_sym => {
-                        :locale => 'es',
-                        :name => 'Renamed'
-                    },
-                    :empty_translation => {
-                        :locale => 'es'
-                    }
+                body.translations_attributes = {
+                    'es' => { :locale => 'es',
+                              :name => 'Renamed' },
+                    'fr' => { :locale => 'fr' }
                 }
 
                 expect(body.translations.size).to eq(2)
@@ -98,7 +90,7 @@ describe PublicBody do
             it 'takes the correct code path for an Array' do
                 attrs = []
                 attrs.should_receive(:each)
-                PublicBody.new().translated_versions = attrs
+                PublicBody.new().translations_attributes = attrs
             end
 
             it 'creates a new translation' do
@@ -108,7 +100,7 @@ describe PublicBody do
 
                 expect(body.translations.size).to eq(1)
 
-                body.translated_versions = [ {
+                body.translations_attributes = [ {
                         :locale => 'es',
                         :name => 'Renamed'
                     }
@@ -125,7 +117,7 @@ describe PublicBody do
 
                 expect(body.translations.size).to eq(1)
 
-                body.translated_versions = [
+                body.translations_attributes = [
                     { :locale => 'empty' }
                 ]