Refactor MessagesController

app/controllers/messages_controller.rb

@@ -3,18 +3,18 @@
 #   the COPYRIGHT file.
 
 class MessagesController < ApplicationController
-  include ApplicationHelper
   before_filter :authenticate_user!
 
   respond_to :html, :mobile
   respond_to :json, :only => :show
 
   def create
-    cnv = Conversation.joins(:conversation_visibilities).where(:id => params[:conversation_id],
+    conversation = Conversation.find(params[:conversation_id])
-                              :conversation_visibilities => {:person_id => current_user.person_id}).first
 
-    if cnv
+    message = conversation.messages.build(
-      message = Message.new(:conversation_id => cnv.id, :text => params[:message][:text], :author => current_user.person)
+      :text   => params[:message][:text],
+      :author => current_user.person
+    )
 
     if message.save
       Rails.logger.info("event=create type=comment user=#{current_user.diaspora_handle} status=success message=#{message.id} chars=#{params[:message][:text].length}")
@@ -22,10 +22,6 @@ class MessagesController < ApplicationController
     else
       flash[:error] = I18n.t('conversations.new_message.fail')
     end
-      redirect_to conversations_path(:conversation_id => cnv.id)
+    redirect_to conversations_path(:conversation_id => conversation.id)
-    else
-      render :nothing => true, :status => 422
   end
-  end
-
 end

spec/controllers/messages_controller_spec.rb

@@ -6,98 +6,113 @@ require 'spec_helper'
 
 describe MessagesController do
   before do
-    @user1 = alice
+    sign_in :user, alice
-    @user2 = bob
-
-    @aspect1 = @user1.aspects.first
-    @aspect2 = @user2.aspects.first
-
-    sign_in :user, @user1
   end
 
   describe '#create' do
     before do
-      @create_hash = {
+      @conversation_params = {
-        :author => @user1.person,
+        :author              => alice.person,
-        :participant_ids => [@user1.contacts.first.person.id, @user1.person.id],
+        :participant_ids     => [alice.contacts.first.person.id, alice.person.id],
         :subject             => 'cool stuff',
-        :messages_attributes => [ {:author => @user1.person, :text => 'stuff'} ]
+        :messages_attributes => [ {:author => alice.person, :text => 'stuff'} ]
       }
     end
 
     context "on my own post" do
       before do
-        @cnv = Conversation.create(@create_hash)
+        @conversation = Conversation.create!(@conversation_params)
       end
 
       context "with a valid message" do
         before do
-          @message_hash = {:conversation_id => @cnv.id, :message => {:text => "here is something else"}}
+          @message_params = {
+            :conversation_id => @conversation.id,
+            :message         => { :text => "here is something else" }
+          }
         end
 
         it 'redirects to conversation' do
-          lambda{
+          lambda {
-            post :create, @message_hash
+            post :create, @message_params
           }.should change(Message, :count).by(1)
-          response.code.should == '302'
+          response.status.should == 302
-          response.should redirect_to(conversations_path(:conversation_id => @cnv))
+          response.should redirect_to(conversations_path(:conversation_id => @conversation))
         end
       end
 
       context "with an empty message" do
         before do
-          @message_hash = {:conversation_id => @cnv.id, :message => {:text => " "}}
+          @message_params = {
+            :conversation_id => @conversation.id,
+            :message         => { :text => " " }
+          }
         end
 
-        it 'redirects to conversation' do
+        it 'does not create the message' do
-          lambda{
+          lambda {
-            post :create, @message_hash
+            post :create, @message_params
-          }.should_not change(Message, :count).by(1)
+          }.should_not change(Message, :count)
-          response.code.should == '302'
+          flash[:error].should be_present
-          response.should redirect_to(conversations_path(:conversation_id => @cnv))
         end
       end
     end
 
     context "on a post from a contact" do
       before do
-        @create_hash[:author] = @user2.person
+        @conversation_params[:author] = bob.person
-        @cnv = Conversation.create(@create_hash)
+        @conversation = Conversation.create!(@conversation_params)
-        @message_hash = {:conversation_id => @cnv.id, :message => {:text => "here is something else"}}
+        @message_params = {
+          :conversation_id => @conversation.id,
+          :message         => { :text => "here is something else" }
+        }
       end
 
       it 'comments' do
-        post :create, @message_hash
+        post :create, @message_params
-        response.code.should == '302'
+        response.status.should == 302
-        response.should redirect_to(conversations_path(:conversation_id => @cnv))
+        response.should redirect_to(conversations_path(:conversation_id => @conversation))
       end
 
       it "doesn't overwrite author_id" do
         new_user = FactoryGirl.create(:user)
-        @message_hash[:author_id] = new_user.person.id.to_s
+        @message_params[:author_id] = new_user.person.id.to_s
-        post :create, @message_hash
+
-        Message.find_by_text(@message_hash[:message][:text]).author_id.should == @user1.person.id
+        post :create, @message_params
+        created_message = Message.find_by_text(@message_params[:message][:text])
+        created_message.author.should == alice.person
       end
 
       it "doesn't overwrite id" do
-        old_message = Message.create(:text => "hello", :author_id => @user1.person.id, :conversation_id => @cnv.id)
+        old_message = Message.create!(
-        @message_hash[:id] = old_message.id
+          :text            => "hello",
-        post :create, @message_hash
+          :author_id       => alice.person.id,
+          :conversation_id => @conversation.id
+        )
+        @message_params[:id] = old_message.id
+
+        post :create, @message_params
         old_message.reload.text.should == 'hello'
       end
     end
 
     context 'on a post from a stranger' do
       before do
-        @create_hash[:author] = eve.person
+        conversation = Conversation.create!(
-        @create_hash[:participant_ids] = [eve.person.id, bob.person.id]
+          :author          => eve.person,
-        @cnv = Conversation.create(@create_hash)
+          :participant_ids => [eve.person.id, bob.person.id]
-        @message_hash = {:conversation_id => @cnv.id, :message => {:text => "here is something else"}}
+        )
+        @message_params = {
+          :conversation_id => conversation.id,
+          :message         => { :text => "here is something else" }
+        }
       end
 
-      it 'posts no comment' do
+      it 'does not create the message' do
-        post :create, @message_hash
+        lambda {
-        response.code.should == '422'
+          post :create, @message_params
+        }.should_not change(Message, :count)
+        flash[:error].should be_present
       end
     end
   end