Merge pull request #3660 from diaspora/privatize_some_controller_methods

Privatize some controller methods

app/controllers/admins_controller.rb

@@ -83,6 +83,7 @@ class AdminsController < ApplicationController
   end
 
   private
+
   def percent_change(today, yesterday)
     sprintf( "%0.02f", ((today-yesterday) / yesterday.to_f)*100).to_f
   end

app/controllers/application_controller.rb

@@ -22,6 +22,8 @@ class ApplicationController < ActionController::Base
                 :tags,
                 :open_publisher
 
+  private
+
   def ensure_http_referer_is_set
     request.env['HTTP_REFERER'] ||= '/'
   end
@@ -129,8 +131,6 @@ class ApplicationController < ActionController::Base
     params[:max_time] ? Time.at(params[:max_time].to_i) : Time.now + 1
   end
 
-  private
-
   def current_user_redirect_path
     current_user.getting_started? ? getting_started_path : root_path
   end

app/controllers/aspects_controller.rb

@@ -34,16 +34,6 @@ class AspectsController < ApplicationController
     end
   end
 
-  #person_id, user, @aspect
-  def connect_person_to_aspect(aspecting_person_id)
-    @person = Person.find(aspecting_person_id)
-    if @contact = current_user.contact_for(@person)
-      @contact.aspects << @aspect
-    else
-      @contact = current_user.share_with(@person, @aspect)
-    end
-  end
-
   def new
     @aspect = Aspect.new
     @person_id = params[:person_id]
@@ -120,4 +110,15 @@ class AspectsController < ApplicationController
     end
     @aspect.save
   end
+
+  private
+
+  def connect_person_to_aspect(aspecting_person_id)
+    @person = Person.find(aspecting_person_id)
+    if @contact = current_user.contact_for(@person)
+      @contact.aspects << @aspect
+    else
+      @contact = current_user.share_with(@person, @aspect)
+    end
+  end
 end

app/controllers/blocks_controller.rb

@@ -32,7 +32,7 @@ class BlocksController < ApplicationController
     end
   end
 
-  protected
+  private
 
   def disconnect_if_contact(person)
     if contact = current_user.contact_for(person)

app/controllers/comments_controller.rb

@@ -61,7 +61,7 @@ class CommentsController < ApplicationController
     end
   end
 
-  protected
+  private
 
   def find_post
     if user_signed_in?

app/controllers/contacts_controller.rb

@@ -33,7 +33,7 @@ class ContactsController < ApplicationController
     @people = Person.community_spotlight
   end
 
-  protected
+  private
 
   def set_up_contacts
     @contacts = case params[:set]
@@ -51,5 +51,4 @@ class ContactsController < ApplicationController
     end
     @contacts = @contacts.for_a_stream.paginate(:page => params[:page], :per_page => 25)
   end
-
 end

app/controllers/invitation_codes_controller.rb

@@ -10,6 +10,8 @@ class InvitationCodesController < ApplicationController
     redirect_to new_user_registration_path(:invite => {:token => params[:id]})
   end
 
+  private
+
   def ensure_valid_invite_code
     InvitationCode.find_by_token!(params[:id])
   end

app/controllers/likes_controller.rb

@@ -46,7 +46,7 @@ class LikesController < ApplicationController
     end
   end
 
-  protected
+  private
 
   def target
     @target ||= if params[:post_id]

app/controllers/people_controller.rb

@@ -162,14 +162,6 @@ class PeopleController < ApplicationController
     end
   end
 
-  def diaspora_id?(query)
-    !query.try(:match, /^(\w)*@([a-zA-Z0-9]|[-]|[.]|[:])*$/).nil?
-  end
-
-  def search_query
-    @search_query ||= params[:q] || params[:term] || ''
-  end
-
   def redirect_if_tag_search
     if search_query.starts_with?('#')
       if search_query.length > 1
@@ -181,7 +173,15 @@ class PeopleController < ApplicationController
     end
   end
 
-  protected
+  private
+
+  def search_query
+    @search_query ||= params[:q] || params[:term] || ''
+  end
+
+  def diaspora_id?(query)
+    !query.try(:match, /^(\w)*@([a-zA-Z0-9]|[-]|[.]|[:])*$/).nil?
+  end
 
   def remote_profile_with_no_user_session?
     @person.try(:remote?) && !user_signed_in?

app/controllers/profiles_controller.rb

@@ -64,7 +64,7 @@ class ProfilesController < ApplicationController
     end
   end
 
-  protected
+  private
 
   def munge_tag_string
     unless @profile_attrs[:tag_string].nil? || @profile_attrs[:tag_string] == I18n.t('profiles.edit.your_tags_placeholder')

app/controllers/publics_controller.rb

@@ -74,7 +74,6 @@ class PublicsController < ApplicationController
     render :nothing => true, :status => 202
   end
 
-
   private
 
   def check_for_xml

app/controllers/registrations_controller.rb

@@ -30,6 +30,7 @@ class RegistrationsController < Devise::RegistrationsController
   end
 
   private
+
   def check_valid_invite!
     return true if AppConfig.settings.enable_registrations? #this sucks
     return true if invite && invite.can_be_used?

app/controllers/services_controller.rb

@@ -63,6 +63,5 @@ class ServicesController < ApplicationController
     @service.destroy
     flash[:notice] = I18n.t 'services.destroy.success'
     redirect_to services_url
-    end
+  end
-
 end

app/controllers/share_visibilities_controller.rb

@@ -15,7 +15,7 @@ class ShareVisibilitiesController < ApplicationController
     render :nothing => true, :status => 200
   end
 
-  protected
+  private
 
   def accessible_post
     @post ||= params[:shareable_type].constantize.where(:id => params[:post_id]).select("id, guid, author_id, created_at").first

app/controllers/status_messages_controller.rb

@@ -12,16 +12,6 @@ class StatusMessagesController < ApplicationController
              :json
 
   layout :bookmarklet_layout, :only => :bookmarklet
-  
-  # Define bookmarklet layout depending on whether
-  # user is in mobile or desktop mode
-  def bookmarklet_layout
-    if request.format == :mobile
-      'application'
-    else
-      'blank'
-    end
-  end
 
   # Called when a user clicks "Mention" on a profile page
   # @param person_id [Integer] The id of the person to be mentioned
@@ -88,6 +78,8 @@ class StatusMessagesController < ApplicationController
     end
   end
 
+  private
+
   def destination_aspect_ids
     if params[:status_message][:public] || params[:status_message][:aspect_ids].first == "all_aspects"
       current_user.aspect_ids
@@ -116,4 +108,14 @@ class StatusMessagesController < ApplicationController
   def remove_getting_started
     current_user.disable_getting_started
   end
+
+  # Define bookmarklet layout depending on whether
+  # user is in mobile or desktop mode
+  def bookmarklet_layout
+    if request.format == :mobile
+      'application'
+    else
+      'blank'
+    end
+  end
 end

app/controllers/tags_controller.rb

@@ -39,23 +39,26 @@ class TagsController < ApplicationController
     end
   end
 
- def tag_followed?
+  private
-   TagFollowing.user_is_following?(current_user, params[:name])
+
- end
+  def tag_followed?
+    TagFollowing.user_is_following?(current_user, params[:name])
+  end
 
   def prep_tags_for_javascript
-    @tags.map! do |obj|
+    @tags.map! do |tag|
-        { :name => ("#"+obj.name),
+      {
-          :value => ("#"+obj.name),
+        :name  => ("#" + tag.name),
-          :url => tag_path(obj.name)
+        :value => ("#" + tag.name),
-        }
+        :url   => tag_path(tag.name)
-      end
-
-      @tags << {
-        :name => ('#' + params[:q]),
-        :value => ("#" + params[:q]),
-        :url => tag_path(params[:q].downcase)
       }
-      @tags.uniq!
+    end
+
+    @tags << {
+      :name  => ('#' + params[:q]),
+      :value => ("#" + params[:q]),
+      :url   => tag_path(params[:q].downcase)
+    }
+    @tags.uniq!
   end
 end

spec/controllers/application_controller_spec.rb

@@ -6,19 +6,15 @@ require 'spec_helper'
 
 describe ApplicationController do
   controller do
-    def user_signed_in?
-      nil
-    end
-
-    def current_user
-      nil
-    end
-
     def index
-      render :nothing => true
+      head :ok
     end
   end
 
+  before do
+    sign_in alice
+  end
+
   describe '#set_diaspora_headers' do
     it 'sets the version header' do
       get :index
@@ -66,4 +62,33 @@ describe ApplicationController do
       request.format.xml?.should be_true
     end
   end
+
+  describe '#tags' do
+    before do
+      @tag = ActsAsTaggableOn::Tag.create!(:name => "partytimeexcellent")
+      TagFollowing.create!(:tag => @tag, :user => alice)
+    end
+
+    it 'queries current_users tag if there are tag_followings' do
+      @controller.send(:tags).should == [@tag]
+    end
+
+    it 'does not query twice' do
+      User.any_instance.should_receive(:followed_tags).once.and_return([@tag])
+      @controller.send(:tags)
+      @controller.send(:tags)
+    end
+  end
+
+  describe "#after_sign_in_path_for" do
+    context 'getting started true on user' do
+      before do
+        alice.update_attribute(:getting_started, true)
+      end
+
+      it "redirects to getting started if the user has getting started set to true" do
+        @controller.send(:after_sign_in_path_for, alice).should == getting_started_path
+      end
+    end
+  end
 end

spec/controllers/aspects_controller_spec.rb

@@ -174,23 +174,4 @@ describe AspectsController do
       @alices_aspect_1.reload.contacts_visible.should be_false
     end
   end
-
-  context 'helper methods' do
-    before do
-      @tag = ActsAsTaggableOn::Tag.create!(:name => "partytimeexcellent")
-      TagFollowing.create!(:tag => @tag, :user => alice)
-      alice.should_receive(:followed_tags).once.and_return([42])
-    end
-
-    describe 'tags' do
-      it 'queries current_users tag if there are tag_followings' do
-        @controller.tags.should == [42]
-      end
-
-      it 'does not query twice' do
-        @controller.tags.should == [42]
-        @controller.tags.should == [42]
-      end
-    end
-  end
 end

spec/controllers/people_controller_spec.rb

@@ -413,35 +413,35 @@ describe PeopleController do
 
   describe '#diaspora_id?' do
     it 'returns true for pods on urls' do
-      @controller.diaspora_id?("ilya_123@pod.geraspora.de").should be_true
+      @controller.send(:diaspora_id?, "ilya_123@pod.geraspora.de").should be_true
     end
 
     it 'returns true for pods on urls with port' do
-      @controller.diaspora_id?("ilya_123@pod.geraspora.de:12314").should be_true
+      @controller.send(:diaspora_id?, "ilya_123@pod.geraspora.de:12314").should be_true
     end
 
     it 'returns true for pods on localhost' do
-      @controller.diaspora_id?("ilya_123@localhost").should be_true
+      @controller.send(:diaspora_id?, "ilya_123@localhost").should be_true
     end
 
     it 'returns true for pods on localhost and port' do
-      @controller.diaspora_id?("ilya_123@localhost:1234").should be_true
+      @controller.send(:diaspora_id?, "ilya_123@localhost:1234").should be_true
     end
 
     it 'returns true for pods on ip' do
-      @controller.diaspora_id?("ilya_123@1.1.1.1").should be_true
+      @controller.send(:diaspora_id?, "ilya_123@1.1.1.1").should be_true
     end
 
     it 'returns true for pods on ip and port' do
-      @controller.diaspora_id?("ilya_123@1.2.3.4:1234").should be_true
+      @controller.send(:diaspora_id?, "ilya_123@1.2.3.4:1234").should be_true
     end
 
     it 'returns false for pods on with invalid url characters' do
-      @controller.diaspora_id?("ilya_123@join_diaspora.com").should be_false
+      @controller.send(:diaspora_id?, "ilya_123@join_diaspora.com").should be_false
     end
 
     it 'returns false for invalid usernames' do
-      @controller.diaspora_id?("ilya_2%3@joindiaspora.com").should be_false
+      @controller.send(:diaspora_id?, "ilya_2%3@joindiaspora.com").should be_false
     end
   end
 end

spec/controllers/status_messages_controller_spec.rb

@@ -224,17 +224,17 @@ describe StatusMessagesController do
     it 'removes the getting started flag from new users' do
       alice.getting_started = true
       alice.save
-      expect{
+      expect {
-        @controller.remove_getting_started
+        @controller.send(:remove_getting_started)
-      }.to change{
+      }.to change {
         alice.reload.getting_started
       }.from(true).to(false)
     end
 
     it 'does nothing for returning users' do
-      expect{
+      expect {
-        @controller.remove_getting_started
+        @controller.send(:remove_getting_started)
-      }.to_not change{
+      }.to_not change {
         alice.reload.getting_started
       }
     end

spec/controllers/tags_controller_spec.rb

@@ -81,11 +81,11 @@ describe TagsController do
 
       it 'returns true if the following already exists and should be case insensitive' do
         TagFollowing.create!(:tag => @tag, :user => bob )
-        @controller.tag_followed?.should be_true
+        @controller.send(:tag_followed?).should be_true
       end
 
       it 'returns false if the following does not already exist' do
-        @controller.tag_followed?.should be_false
+        @controller.send(:tag_followed?).should be_false
       end
     end
   end

spec/controllers/users_controller_spec.rb

@@ -251,22 +251,5 @@ describe UsersController do
       response.should be_success
     end
   end
-
-  # This logic lives in application controller
-  describe "#after_sign_in_path_for" do
-    before do
-      @controller.stub(:current_user).and_return(eve)
-    end
-
-    context 'getting started true on user' do
-      before do
-        eve.update_attribute(:getting_started, true)
-      end
-
-      it "redirects to getting started if the user has getting started set to true" do
-        @controller.after_sign_in_path_for(eve).should == getting_started_path
-      end
-    end
-  end
 end