Use explicit parameters instead of request format to determine whether we render a remote or non-remote form in aspects#new.

Facebox needs to get back a text/html response. We were using text/javascript to indicate that we wanted a remote form and text/html to indicate we wanted a non-remote form. The trouble is, if we request with text/javascript then that is the format we get back. It doesn't really make sense to use formats that way anyway, so I changed it to an explicit parameter. This had the nice side effect of simplifying our new-aspect views.

app/controllers/aspects_controller.rb

@@ -61,7 +61,7 @@ class AspectsController < ApplicationController
         redirect_to :back
       elsif request.env['HTTP_REFERER'].include?("contacts")
         redirect_to :back
-      elsif params[:aspect][:person_id]
+      elsif params[:aspect][:person_id].present?
         @person = Person.where(:id => params[:aspect][:person_id]).first
 
         if @contact = current_user.contact_for(@person)
@@ -86,9 +86,9 @@ class AspectsController < ApplicationController
   def new
     @aspect = Aspect.new
     @person_id = params[:person_id]
+    @remote = params[:remote] == "true"
     respond_to do |format|
-      format.js { render :layout => false }
+      format.html { render :layout => false }
-      format.html { render '_new' }
     end
   end
 

app/views/aspects/_aspect_listings.haml

@@ -21,7 +21,7 @@
             = aspect
 
       %li
-        = link_to t('.add_an_aspect'), "#add_aspect_pane", :class => "new_aspect", :rel => "facebox"
+        = link_to t('.add_an_aspect'), new_aspect_path, :class => "new_aspect", :rel => "facebox"
 
   %li.all_contacts{:class => ("active" if params["set"] == "all" || params["set"] == "only_sharing")}
     %a.aspect_selector{:href => contacts_path(:set => "all"), :class => ("sub_selected" if params["set"] == "only_sharing")}

app/views/aspects/_new.haml

@@ -1,27 +0,0 @@
--#   Copyright (c) 2010, Diaspora Inc.  This file is
--#   licensed under the Affero General Public License version 3 or later.  See
--#   the COPYRIGHT file.
-
-.span-12.last
-  #facebox_header
-    %h3
-      = t('contacts.index.add_a_new_aspect')
-
-  = form_for(Aspect.new, :remote => (defined?(remote) && remote) ) do |aspect|
-    = aspect.error_messages
-    - if defined?(person_id)
-      = aspect.hidden_field :person_id, :value => person_id
-    %p
-      = aspect.label :name , t('.name')
-      = aspect.text_field :name, :maxlength => 20
-
-    %p.checkbox_select
-      = aspect.label :contacts_visible, t('aspects.edit.make_aspect_list_visible')
-      = aspect.check_box :contacts_visible, :default => true
-
-    %br
-
-    .bottom_submit_section
-      = submit_tag t('cancel'), :class => 'button', :type => :reset, :rel => "close"
-      = aspect.submit t('.create'), :class => 'button creation'
-

app/views/aspects/new.haml

@@ -2,4 +2,25 @@
 -#   licensed under the Affero General Public License version 3 or later.  See
 -#   the COPYRIGHT file.
 
-= render 'new', :remote => true, :person_id => @person_id
+.span-12.last
+  #facebox_header
+    %h3
+      = t('contacts.index.add_a_new_aspect')
+
+  = form_for(Aspect.new, :remote => @remote) do |aspect|
+    = aspect.error_messages
+    - if @person_id
+      = aspect.hidden_field :person_id, :value => @person_id
+    %p
+      = aspect.label :name , t('.name')
+      = aspect.text_field :name, :maxlength => 20
+
+    %p.checkbox_select
+      = aspect.label :contacts_visible, t('aspects.edit.make_aspect_list_visible')
+      = aspect.check_box :contacts_visible, :default => true
+
+    %br
+
+    .bottom_submit_section
+      = submit_tag t('cancel'), :class => 'button', :type => :reset, :rel => "close"
+      = aspect.submit t('.create'), :class => 'button creation'

app/views/layouts/_header.html.haml

@@ -84,8 +84,3 @@
       -if current_user.admin?
         %li= link_to t('.admin'), user_search_path
       %li= link_to t('.logout'), destroy_user_session_path
-
-    -unless @landing_page
-      .facebox_content
-        #add_aspect_pane
-          = render "aspects/new"

app/views/shared/_aspect_dropdown.html.haml

@@ -18,4 +18,4 @@
       - if (dropdown_may_create_new_aspect && defined?(person) && person)
         %li.newItem
           .add_aspect
-            = link_to t('contacts.index.add_a_new_aspect'), new_aspect_path(:person_id => person.id), :rel => 'facebox'
+            = link_to t('contacts.index.add_a_new_aspect'), new_aspect_path(:person_id => person.id, :remote => true), :rel => 'facebox'

spec/controllers/aspects_controller_spec.rb

@@ -47,6 +47,24 @@ describe AspectsController do
     it_should_behave_like "it overrides the logs on redirect"
   end
 
+  describe "#new" do
+    it "renders a remote form if remote is true" do
+      get :new, "remote" => "true"
+      response.should be_success
+      response.body.should =~ /#{Regexp.escape('data-remote="true"')}/
+    end
+    it "renders a non-remote form if remote is false" do
+      get :new, "remote" => "false"
+      response.should be_success
+      response.body.should_not =~ /#{Regexp.escape('data-remote="true"')}/
+    end
+    it "renders a non-remote form if remote is missing" do
+      get :new
+      response.should be_success
+      response.body.should_not =~ /#{Regexp.escape('data-remote="true"')}/
+    end
+  end
+
   describe "#index" do
     it "generates a jasmine fixture", :fixture => true do
       get :index