From b60101b9ad472534d727815693ae684c8f66eb99 Mon Sep 17 00:00:00 2001 From: Sarah Mei Date: Sun, 28 Aug 2011 16:08:57 -0700 Subject: [PATCH] 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 | 6 ++--- app/views/aspects/_aspect_listings.haml | 2 +- app/views/aspects/_new.haml | 27 --------------------- app/views/aspects/new.haml | 23 +++++++++++++++++- app/views/layouts/_header.html.haml | 5 ---- app/views/shared/_aspect_dropdown.html.haml | 2 +- spec/controllers/aspects_controller_spec.rb | 18 ++++++++++++++ 7 files changed, 45 insertions(+), 38 deletions(-) delete mode 100644 app/views/aspects/_new.haml diff --git a/app/controllers/aspects_controller.rb b/app/controllers/aspects_controller.rb index 21c8b7372..5093a1dbc 100644 --- a/app/controllers/aspects_controller.rb +++ b/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 '_new' } + format.html { render :layout => false } end end diff --git a/app/views/aspects/_aspect_listings.haml b/app/views/aspects/_aspect_listings.haml index 560a7888e..f012862aa 100644 --- a/app/views/aspects/_aspect_listings.haml +++ b/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")} diff --git a/app/views/aspects/_new.haml b/app/views/aspects/_new.haml deleted file mode 100644 index 01e2ebb3b..000000000 --- a/app/views/aspects/_new.haml +++ /dev/null @@ -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' - diff --git a/app/views/aspects/new.haml b/app/views/aspects/new.haml index 0747f9658..81656487b 100644 --- a/app/views/aspects/new.haml +++ b/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' diff --git a/app/views/layouts/_header.html.haml b/app/views/layouts/_header.html.haml index b4a6510f7..115d6e61d 100644 --- a/app/views/layouts/_header.html.haml +++ b/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" diff --git a/app/views/shared/_aspect_dropdown.html.haml b/app/views/shared/_aspect_dropdown.html.haml index 33c9ae811..16c2ff457 100644 --- a/app/views/shared/_aspect_dropdown.html.haml +++ b/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' diff --git a/spec/controllers/aspects_controller_spec.rb b/spec/controllers/aspects_controller_spec.rb index a69357aea..0569498a9 100644 --- a/spec/controllers/aspects_controller_spec.rb +++ b/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