Refactor fixes to #142 and #143, new implementation much simpler to understand

5 files changed, 83 insertions, 78 deletions

diff --git a/app/controllers/admin_public_body_controller.rb b/app/controllers/admin_public_body_controller.rb
index 61e4aec54..bd85f6eed 100644
--- a/app/controllers/admin_public_body_controller.rb
+++ b/app/controllers/admin_public_body_controller.rb
@@ -82,77 +82,37 @@ class AdminPublicBodyController < AdminController
     end
 
     def new
-        @locale = self.locale_from_params()
-        PublicBody.with_locale(@locale) do 
-            @public_body = PublicBody.new
-            render
-        end
-    end
-
-    def update_localized_attributes(public_body, params)
-        I18n.available_locales.each do |locale|
-            PublicBody.with_locale(locale) do
-                unless (attr_values = params["public_body_#{locale}"]).nil?
-                    # 'publication_scheme' can't be null in the current DB schema
-                    attr_values[:publication_scheme] = attr_values[:publication_scheme] || ''
-                    public_body.attributes = attr_values
-                    public_body.save!
-                end
-            end
-        end
+        @public_body = PublicBody.new
+        render
     end
     
     def create
-        # Start creating the public body in the default locale
         PublicBody.with_locale(I18n.default_locale) do
-            begin
-                ActiveRecord::Base.transaction do
-                    params[:public_body][:last_edit_editor] = admin_http_auth_user()
-                    @public_body = PublicBody.new(params[:public_body])
-                    @public_body.save!
-
-                    # Next, save the translations in the additional locales
-                    update_localized_attributes(@public_body, params)
-                    
-                    flash[:notice] = 'PublicBody was successfully created.'
-                    redirect_to admin_url('body/show/' + @public_body.id.to_s)
-                end
-            rescue ActiveRecord::RecordInvalid
+            params[:public_body][:last_edit_editor] = admin_http_auth_user()
+            @public_body = PublicBody.new(params[:public_body])
+            if @public_body.save
+                flash[:notice] = 'PublicBody was successfully created.'
+                redirect_to admin_url('body/show/' + @public_body.id.to_s)
+            else
                 render :action => 'new'
             end
         end
     end
 
     def edit
-        PublicBody.with_locale(I18n.default_locale) do
-            @public_body = PublicBody.find(params[:id])
-            @public_body.last_edit_comment = ""
-        end
-
-        # Additional locales (could remove the default, but won't make a difference)
-        I18n.available_locales.each do |locale|
-            instance_variable_set("@public_body_#{locale}", @public_body.translations.select{|t| t.locale==locale}.first)
-        end
-        
+        @public_body = PublicBody.find(params[:id])
+        @public_body.last_edit_comment = ""        
         render
     end
 
     def update
-        # Start updating the public body in the default locale
         PublicBody.with_locale(I18n.default_locale) do
-            begin
-                ActiveRecord::Base.transaction do
-                    params[:public_body][:last_edit_editor] = admin_http_auth_user()
-                    @public_body = PublicBody.find(params[:id])
-                    @public_body.update_attributes!(params[:public_body])
-
-                    # Next, update the translations in the additional locales
-                    update_localized_attributes(@public_body, params)
-                    
-                    flash[:notice] = 'PublicBody was successfully updated.'
-                    redirect_to admin_url('body/show/' + @public_body.id.to_s)
-                end
-            rescue ActiveRecord::RecordInvalid
+            params[:public_body][:last_edit_editor] = admin_http_auth_user()
+            @public_body = PublicBody.find(params[:id])
+            if @public_body.update_attributes(params[:public_body])
+                flash[:notice] = 'PublicBody was successfully updated.'
+                redirect_to admin_url('body/show/' + @public_body.id.to_s)
+            else
                 render :action => 'edit'
             end
         end
diff --git a/app/models/public_body.rb b/app/models/public_body.rb
index 2c7f80240..a476db8c0 100644
--- a/app/models/public_body.rb
+++ b/app/models/public_body.rb
@@ -38,7 +38,7 @@ class PublicBody < ActiveRecord::Base
 
     validates_uniqueness_of :short_name, :message => N_("Short name is already taken"), :if => Proc.new { |pb| pb.short_name != "" }
     validates_uniqueness_of :name, :message => N_("Name is already taken")
-    
+
     has_many :info_requests, :order => 'created_at desc'
     has_many :track_things, :order => 'created_at desc'
 
@@ -46,6 +46,40 @@ class PublicBody < ActiveRecord::Base
 
     translates :name, :short_name, :request_email, :url_name, :notes, :first_letter, :publication_scheme
 
+    # Convenience methods for creating/editing translations via forms
+    def translation(locale)
+        self.translations.find_by_locale(locale)
+    end
+
+    # XXX - Don't like repeating this!
+    def calculate_cached_fields(t)
+        t.first_letter = t.name.scan(/^./mu)[0].upcase
+        short_long_name = t.name
+        short_long_name = t.short_name if t.short_name and !t.short_name.empty?
+        t.url_name = MySociety::Format.simplify_url_part(short_long_name, 'body')
+    end
+    
+    def translated_versions
+        translations
+    end
+    
+    def translated_versions=(translation_attrs)
+        if translation_attrs.respond_to? :each_value    # Hash => updating
+            translation_attrs.each_value do |attrs|
+                t = translation(attrs[:locale]) || PublicBody::Translation.new
+                t.attributes = attrs
+                calculate_cached_fields(t)
+                t.save!
+            end
+        else                                            # Array => creating
+            translation_attrs.each do |attrs|
+                new_translation = PublicBody::Translation.new(attrs)
+                calculate_cached_fields(new_translation)
+                translations << new_translation
+            end
+        end
+    end
+    
     # Make sure publication_scheme gets the correct default value.
     # (This would work automatically, were publication_scheme not a translated attribute)
     def after_initialize
@@ -191,7 +225,6 @@ class PublicBody < ActiveRecord::Base
 
     # When name or short name is changed, also change the url name
     def short_name=(short_name)
-
         globalize.write(self.class.locale || I18n.locale, :short_name, short_name)
         self[:short_name] = short_name
         self.update_url_name
@@ -204,15 +237,15 @@ class PublicBody < ActiveRecord::Base
     end
 
     def update_url_name
-        url_name = MySociety::Format.simplify_url_part(self.short_or_long_name, 'body')
-        self.url_name = url_name
+        self.url_name = MySociety::Format.simplify_url_part(self.short_or_long_name, 'body')
     end
+    
     # Return the short name if present, or else long name
     def short_or_long_name
-        if self.short_name.nil? # can happen during construction
+        if self.short_name.nil? || self.short_name.empty?   # 'nil' can happen during construction
             self.name
         else
-            self.short_name.empty? ? self.name : self.short_name
+            self.short_name
         end
     end
 
diff --git a/app/views/admin_public_body/_form.rhtml b/app/views/admin_public_body/_form.rhtml
index 1fb573994..98749154c 100644
--- a/app/views/admin_public_body/_form.rhtml
+++ b/app/views/admin_public_body/_form.rhtml
@@ -25,40 +25,52 @@
 
 <% 
     for locale in I18n.available_locales do
-        object_name = locale==:en ? 'public_body' : "public_body_#{locale.to_s}"
+        if locale==I18n.default_locale  # The default locale is submitted as part of the bigger object...
+            prefix = 'public_body'
+            object = @public_body
+        else                            # ...but additional locales go "on the side"
+            prefix = "public_body[translated_versions][]"
+            object = @public_body.new_record? ? 
+                        PublicBody::Translation.new : 
+                        @public_body.translation(locale.to_s) || PublicBody::Translation.new
+        end
+
+        fields_for prefix, object do |t|
 %>
 <div>
     <p>Locale: <%= locale_name(locale.to_s) %></p>
-
+    <%= t.hidden_field :locale, :value => locale.to_s %>
+    
     <p><label for="public_body_name">Name</label><br/>
-    <%= text_field object_name, 'name', :size => 60  %></p>
+    <%= t.text_field :name, :size => 60  %></p>
 
     <p><label for="public_body_short_name">Short name <small>(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)</small></label><br/>
-    <%= text_field object_name, 'short_name', :size => 60  %></p>
+    <%= t.text_field :short_name, :size => 60  %></p>
 
     <p><label for="public_body_request_email">Request email <small>(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)</small></label><br/>
-    <%= text_field object_name, 'request_email', :size => 40  %></p>
+    <%= t.text_field :request_email, :size => 40  %></p>
 
     <p><label for="public_body_publication_scheme">Publication scheme URL</label><br/>
-    <%= text_field object_name, 'publication_scheme', :size => 60  %></p>
+    <%= t.text_field :publication_scheme, :size => 60  %></p>
 
     <p><label for="public_body_notes">Public notes</label> <small>(HTML, for users to consider when making FOI requests to the authority)</small><br/>
-    <%= text_area object_name, 'notes', :rows => 3, :cols => 60  %></p>
+    <%= t.text_area :notes, :rows => 3, :cols => 60  %></p>
 </div>
 <% 
+        end
     end 
 %>
 
 <h3>Common Fields</h3>
 
 <p><label for="public_body_tag_string">Tags <small>(space separated; see list of tags on the right; also <strong>not_apply</strong> if FOI and EIR no longer apply to authority, <strong>eir_only</strong> if EIR but not FOI applies to authority, <strong>defunct</strong> if the authority no longer exists; charity:NUMBER if a registered charity)</small></label><br/>
-<%= text_field 'public_body', 'tag_string', :size => 60  %></p>
+<%= f.text_field :tag_string, :size => 60  %></p>
 
 <p><label for="public_body_home_page">Home page <small>(of whole authority, not just their FOI page; set to <strong>blank</strong> (empty string) to guess it from the email)</small></label><br/>
-<%= text_field 'public_body', 'home_page', :size => 60  %></p>
+<%= f.text_field :home_page, :size => 60  %></p>
 
 <p><label for="public_body_last_edit_comment"><strong>Comment</strong> for this edit</label> <small>(put URL or other source of new info)</small><br/>
-<%= text_area 'public_body', 'last_edit_comment', :rows => 3, :cols => 60  %></p>
+<%= f.text_area :last_edit_comment, :rows => 3, :cols => 60  %></p>
 <!--[eoform:public_body]-->
 
 
diff --git a/app/views/admin_public_body/edit.rhtml b/app/views/admin_public_body/edit.rhtml
index 005ec93ce..ae4066dc7 100644
--- a/app/views/admin_public_body/edit.rhtml
+++ b/app/views/admin_public_body/edit.rhtml
@@ -2,9 +2,9 @@
 
 <h1><%=@title%></h1>
 
-<% form_tag '../update/' + @public_body.id.to_s do %>
-  <%= render :partial => 'form' %>
-  <p><%= submit_tag 'Save', :accesskey => 's' %></p>
+<% form_for @public_body, :url => {:action => 'update'} do |f| %>
+  <%= render :partial => 'form', :locals => {:f => f} %>
+  <p><%= f.submit 'Save', :accesskey => 's' %></p>
 <% end %>
 
 <p>
diff --git a/app/views/admin_public_body/new.rhtml b/app/views/admin_public_body/new.rhtml
index 95208b5b3..fe0b5b670 100644
--- a/app/views/admin_public_body/new.rhtml
+++ b/app/views/admin_public_body/new.rhtml
@@ -2,9 +2,9 @@
 
 <h1><%=@title%></h1>
 
-<% form_tag 'create' do %>
-  <%= render :partial => 'form' %>
-  <p><%= submit_tag "Create" %></p>
+<% form_for :public_body, @public_body, :url => {:action => "create"} do |f| %>
+  <%= render :partial => 'form', :locals => {:f => f} %>
+  <p><%= f.submit "Create" %></p>
 <% end %>
 
 <p>