From: Tom Hughes <tom@compton.nu>
Date: Thu, 22 Mar 2012 22:58:57 +0000 (+0000)
Subject: Add functional tests for messages
X-Git-Tag: live~5629
X-Git-Url: https://git.openstreetmap.org/rails.git/commitdiff_plain/eaad3611b8560966b8126abbae3828a15d768533

Add functional tests for messages

Also fixes various issues in the code discovered while writing
the tests, and adds some named routes for messages.
---

diff --git a/app/controllers/message_controller.rb b/app/controllers/message_controller.rb
index e34aad37d..94bee5d35 100644
--- a/app/controllers/message_controller.rb
+++ b/app/controllers/message_controller.rb
@@ -8,7 +8,7 @@ class MessageController < ApplicationController
   before_filter :check_database_readable
   before_filter :check_database_writable, :only => [:new, :reply, :mark]
 
-  # Allow the user to write a new message to another user. This action also 
+  # Allow the user to write a new message to another user. This action also
   # deals with the sending of that message to the other user when the user
   # clicks send.
   # The display_name param is the display name of the user that the message is being sent to.
@@ -29,14 +29,7 @@ class MessageController < ApplicationController
         end
       end
     else
-      if params[:title]
-        # ?title= is set when someone reponds to this user's diary
-        # entry. Then we pre-fill out the subject and the <title>
-        @title = @subject = params[:title]
-      else
-        # The default /message/new/$user view
-        @title = t 'message.new.title'
-      end
+      @title = t 'message.new.title'
     end
   end
 
@@ -45,7 +38,7 @@ class MessageController < ApplicationController
     message = Message.find(params[:message_id])
 
     if message.to_user_id == @user.id then
-      @body = "On #{message.sent_on} #{message.sender.display_name} wrote:\n\n#{message.body.gsub(/^/, '> ')}" 
+      @body = "On #{message.sent_on} #{message.sender.display_name} wrote:\n\n#{message.body.gsub(/^/, '> ')}"
       @title = @subject = "Re: #{message.title.sub(/^Re:\s*/, '')}"
       @this_user = User.find(message.from_user_id)
 
@@ -96,22 +89,19 @@ class MessageController < ApplicationController
 
   # Set the message as being read or unread.
   def mark
-    if params[:message_id]
-      id = params[:message_id]
-      @message = Message.where(:id => id).where("to_user_id = ? OR from_user_id = ?", @user.id, @user.id).first
-      if params[:mark] == 'unread'
-        message_read = false 
-        notice = t 'message.mark.as_unread'
-      else
-        message_read = true
-        notice = t 'message.mark.as_read'
-      end
-      @message.message_read = message_read
-      if @message.save
-        if not request.xhr?
-          flash[:notice] = notice
-          redirect_to :controller => 'message', :action => 'inbox', :display_name => @user.display_name
-        end
+    @message = Message.where("to_user_id = ? OR from_user_id = ?", @user.id, @user.id).find(params[:message_id])
+    if params[:mark] == 'unread'
+      message_read = false
+      notice = t 'message.mark.as_unread'
+    else
+      message_read = true
+      notice = t 'message.mark.as_read'
+    end
+    @message.message_read = message_read
+    if @message.save
+      if not request.xhr?
+        flash[:notice] = notice
+        redirect_to :controller => 'message', :action => 'inbox', :display_name => @user.display_name
       end
     end
   rescue ActiveRecord::RecordNotFound
@@ -121,19 +111,16 @@ class MessageController < ApplicationController
 
   # Delete the message.
   def delete
-    if params[:message_id]
-      id = params[:message_id]
-      message = Message.where(:id => id).where("to_user_id = ? OR from_user_id = ?", @user.id, @user.id).first
-      message.from_user_visible = false if message.sender == @user
-      message.to_user_visible = false if message.recipient == @user
-      if message.save
-        flash[:notice] = t 'message.delete.deleted'
+    message = Message.where("to_user_id = ? OR from_user_id = ?", @user.id, @user.id).find(params[:message_id])
+    message.from_user_visible = false if message.sender == @user
+    message.to_user_visible = false if message.recipient == @user
+    if message.save
+      flash[:notice] = t 'message.delete.deleted'
 
-        if params[:referer]
-          redirect_to params[:referer]
-        else
-          redirect_to :controller => 'message', :action => 'inbox', :display_name => @user.display_name
-        end
+      if params[:referer]
+        redirect_to params[:referer]
+      else
+        redirect_to :controller => 'message', :action => 'inbox', :display_name => @user.display_name
       end
     end
   rescue ActiveRecord::RecordNotFound
diff --git a/app/views/message/outbox.html.erb b/app/views/message/outbox.html.erb
index ece514289..6f6823c1b 100644
--- a/app/views/message/outbox.html.erb
+++ b/app/views/message/outbox.html.erb
@@ -1,6 +1,6 @@
 <h2><%= raw(t'message.outbox.my_inbox', :inbox_link => link_to(t('message.outbox.inbox'), inbox_path(@user.display_name))) %>/<%= t'message.outbox.outbox' %></h2>
 
-<p><%= t'message.outbox.messages', :count => @user.sent_messages.size %>
+<p><%= t'message.outbox.messages', :count => @user.sent_messages.size %></p>
 
 <% if @user.sent_messages.size > 0 %>
   <div id="messages">
diff --git a/config/locales/en.yml b/config/locales/en.yml
index e9ee39812..70b95fb65 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -1284,7 +1284,7 @@ en:
       reading_your_sent_messages: "Reading your sent messages"
       to: "To"
       back_to_outbox: "Back to outbox"
-      wrong_user: "You are logged in as `%{user}' but the message you have asked to read to was not sent by or to that user. Please login as the correct user in order to read it."
+      wrong_user: "You are logged in as `%{user}' but the message you have asked to read was not sent by or to that user. Please login as the correct user in order to read it."
     sent_message_summary:
       delete_button: "Delete"
     mark:
diff --git a/config/routes.rb b/config/routes.rb
index 9550c84ff..779f9b2a7 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -201,11 +201,11 @@ OpenStreetMap::Application.routes.draw do
   # messages
   match '/user/:display_name/inbox' => 'message#inbox', :via => :get, :as => "inbox"
   match '/user/:display_name/outbox' => 'message#outbox', :via => :get, :as => "outbox"
-  match '/message/new/:display_name' => 'message#new', :via => [:get, :post]
-  match '/message/read/:message_id' => 'message#read', :via => :get
-  match '/message/mark/:message_id' => 'message#mark', :via => :post
-  match '/message/reply/:message_id' => 'message#reply', :via => [:get, :post]
-  match '/message/delete/:message_id' => 'message#delete', :via => :post
+  match '/message/new/:display_name' => 'message#new', :via => [:get, :post], :as => "new_message"
+  match '/message/read/:message_id' => 'message#read', :via => :get, :as => "read_message"
+  match '/message/mark/:message_id' => 'message#mark', :via => :post, :as => "mark_message"
+  match '/message/reply/:message_id' => 'message#reply', :via => [:get, :post], :as => "reply_message"
+  match '/message/delete/:message_id' => 'message#delete', :via => :post, :as => "delete_message"
 
   # oauth admin pages (i.e: for setting up new clients, etc...)
   scope "/user/:display_name" do
diff --git a/test/fixtures/messages.yml b/test/fixtures/messages.yml
index 22fab1863..0a54cce87 100644
--- a/test/fixtures/messages.yml
+++ b/test/fixtures/messages.yml
@@ -1,16 +1,21 @@
-# Read about fixtures at http://ar.rubyonrails.org/classes/Fixtures.html
-one:
+unread_message:
+  id: 1
   from_user_id: 1
   title: test message 1
   body: some body text
   sent_on: "2008-05-01 12:34:56"
   message_read: false
   to_user_id: 2
+  from_user_visible: true
+  to_user_visible: true
   
-two:
+read_message:
+  id: 2
   from_user_id: 2
   title: test message 2
   body: some body test
   sent_on: "2008-05-02 12:45:23"
   message_read: true
   to_user_id: 1
+  from_user_visible: true
+  to_user_visible: true
diff --git a/test/functional/message_controller_test.rb b/test/functional/message_controller_test.rb
index a2bb1f805..d673a8126 100644
--- a/test/functional/message_controller_test.rb
+++ b/test/functional/message_controller_test.rb
@@ -1,7 +1,8 @@
 require File.dirname(__FILE__) + '/../test_helper'
-require 'message_controller'
 
 class MessageControllerTest < ActionController::TestCase
+  fixtures :users, :messages
+
   ##
   # test all routes which lead to this controller
   def test_routes
@@ -42,4 +43,302 @@ class MessageControllerTest < ActionController::TestCase
       { :controller => "message", :action => "delete", :message_id => "1" }
     )
   end
+
+  ##
+  # test the new action
+  def test_new
+    # Check that the new message page requires us to login
+    get :new, :display_name => users(:public_user).display_name
+    assert_redirected_to login_path(:referer => new_message_path(:display_name => users(:public_user).display_name))
+
+    # Login as a normal user
+    session[:user] = users(:normal_user).id
+    cookies["_osm_username"] = users(:normal_user).display_name
+
+    # Check that the new message page loads
+    get :new, :display_name => users(:public_user).display_name
+    assert_response :success
+    assert_template "new"
+    assert_select "title", "OpenStreetMap | Send message"
+    assert_select "form[action='#{new_message_path(:display_name => users(:public_user).display_name)}']", :count => 1 do
+      assert_select "input#message_title", :count => 1
+      assert_select "textarea#message_body", :count => 1
+      assert_select "input[type='submit'][value='Send']", :count => 1
+    end
+
+    # Check that sending a message works
+    assert_difference "ActionMailer::Base.deliveries.size", 1 do
+      assert_difference "Message.count", 1 do
+        post :new,
+          :display_name => users(:public_user).display_name,
+          :message => { :title => "Test Message", :body => "Test message body" }
+      end
+    end
+    assert_redirected_to inbox_path(:display_name => users(:normal_user).display_name)
+    assert_equal "Message sent", flash[:notice]
+    e = ActionMailer::Base.deliveries.first
+    assert_equal [ users(:public_user).email ], e.to
+    assert_equal "[OpenStreetMap] Test Message", e.subject
+    assert_match /Test message body/, e.text_part.decoded
+    assert_match /Test message body/, e.html_part.decoded
+    ActionMailer::Base.deliveries.clear
+    m = Message.find(3)
+    assert_equal users(:normal_user).id, m.from_user_id
+    assert_equal users(:public_user).id, m.to_user_id
+    assert_in_delta Time.now, m.sent_on, 1
+    assert_equal "Test Message", m.title
+    assert_equal "Test message body", m.body
+    assert_equal "markdown", m.body_format
+
+    # Asking to send a message with no user name should fail
+    get :new
+    assert_response :not_found
+    assert_template "user/no_such_user"
+    assert_select "h2", "The user  does not exist"
+
+    # Asking to send a message with a bogus user name should fail
+    get :new, :display_name => "non_existent_user"
+    assert_response :not_found
+    assert_template "user/no_such_user"
+    assert_select "h2", "The user non_existent_user does not exist"
+  end
+
+  ##
+  # test the reply action
+  def test_reply
+    # Check that the message reply page requires us to login
+    get :reply, :message_id => messages(:read_message).id
+    assert_redirected_to login_path(:referer => reply_message_path(:message_id => messages(:read_message).id))
+
+    # Login as the wrong user
+    session[:user] = users(:second_public_user).id
+    cookies["_osm_username"] = users(:second_public_user).display_name
+
+    # Check that we can't reply to somebody else's message
+    get :reply, :message_id => messages(:read_message).id
+    assert_redirected_to login_path(:referer => reply_message_path(:message_id => messages(:read_message).id))
+    assert_equal "You are logged in as `pulibc_test2' but the message you have asked to reply to was not sent to that user. Please login as the correct user in order to reply.", flash[:notice]
+
+    # Login as the right user
+    session[:user] = users(:normal_user).id
+    cookies["_osm_username"] = users(:normal_user).display_name
+
+    # Check that the message reply page loads
+    get :reply, :message_id => messages(:read_message).id
+    assert_response :success
+    assert_template "new"
+    assert_select "title", "OpenStreetMap | Re: test message 2"
+    assert_select "form[action='#{new_message_path(:display_name => users(:public_user).display_name)}']", :count => 1 do
+      assert_select "input#message_title[value='Re: test message 2']", :count => 1
+      assert_select "textarea#message_body", :count => 1
+      assert_select "input[type='submit'][value='Send']", :count => 1
+    end
+
+    # Asking to reply to a message with no ID should fail
+    get :reply
+    assert_response :not_found
+    assert_template "no_such_message"
+
+    # Asking to reply to a message with a bogus ID should fail
+    get :reply, :message_id => 99999
+    assert_response :not_found
+    assert_template "no_such_message"
+  end
+
+  ##
+  # test the read action
+  def test_read
+    # Check that the read message page requires us to login
+    get :read, :message_id => messages(:unread_message).id
+    assert_redirected_to login_path(:referer => read_message_path(:message_id => messages(:unread_message).id))
+
+    # Login as the wrong user
+    session[:user] = users(:second_public_user).id
+    cookies["_osm_username"] = users(:second_public_user).display_name
+
+    # Check that we can't read the message
+    get :read, :message_id => messages(:unread_message).id
+    assert_redirected_to login_path(:referer => read_message_path(:message_id => messages(:unread_message).id))
+    assert_equal "You are logged in as `pulibc_test2' but the message you have asked to read was not sent by or to that user. Please login as the correct user in order to read it.", flash[:notice]
+
+    # Login as the message sender
+    session[:user] = users(:normal_user).id
+    cookies["_osm_username"] = users(:normal_user).display_name
+
+    # Check that the message sender can read the message
+    get :read, :message_id => messages(:unread_message).id
+    assert_response :success
+    assert_template "read"
+    assert_equal false, Message.find(messages(:unread_message).id).message_read
+
+    # Login as the message recipient
+    session[:user] = users(:public_user).id
+    cookies["_osm_username"] = users(:public_user).display_name
+
+    # Check that the message recipient can read the message
+    get :read, :message_id => messages(:unread_message).id
+    assert_response :success
+    assert_template "read"
+    assert_equal true, Message.find(messages(:unread_message).id).message_read
+
+    # Asking to read a message with no ID should fail
+    get :read
+    assert_response :not_found
+    assert_template "no_such_message"
+
+    # Asking to read a message with a bogus ID should fail
+    get :read, :message_id => 99999
+    assert_response :not_found
+    assert_template "no_such_message"
+  end
+
+  ##
+  # test the inbox action
+  def test_inbox
+    # Check that the inbox page requires us to login
+    get :inbox, :display_name => users(:normal_user).display_name
+    assert_redirected_to login_path(:referer => inbox_path(:display_name => users(:normal_user).display_name))
+
+    # Login
+    session[:user] = users(:normal_user).id
+    cookies["_osm_username"] = users(:normal_user).display_name
+
+    # Check that we can view our inbox when logged in
+    get :inbox, :display_name => users(:normal_user).display_name
+    assert_response :success
+    assert_template "inbox"
+    assert_select "table.messages", :count => 1 do
+      assert_select "tr", :count => 2
+      assert_select "tr#inbox-#{messages(:read_message).id}.inbox-row", :count => 1
+    end
+
+    # Check that we can't view somebody else's inbox when logged in
+    get :inbox, :display_name => users(:public_user).display_name
+    assert_redirected_to inbox_path(:display_name => users(:normal_user).display_name)
+  end
+
+  ##
+  # test the outbox action
+  def test_outbox
+    # Check that the outbox page requires us to login
+    get :outbox, :display_name => users(:normal_user).display_name
+    assert_redirected_to login_path(:referer => outbox_path(:display_name => users(:normal_user).display_name))
+
+    # Login
+    session[:user] = users(:normal_user).id
+    cookies["_osm_username"] = users(:normal_user).display_name
+
+    # Check that we can view our outbox when logged in
+    get :outbox, :display_name => users(:normal_user).display_name
+    assert_response :success
+    assert_template "outbox"
+    assert_select "table.messages", :count => 1 do
+      assert_select "tr", :count => 2
+      assert_select "tr.inbox-row", :count => 1
+    end
+
+    # Check that we can't view somebody else's outbox when logged in
+    get :outbox, :display_name => users(:public_user).display_name
+    assert_redirected_to outbox_path(:display_name => users(:normal_user).display_name)
+  end
+
+  ##
+  # test the mark action
+  def test_mark
+    # Check that the marking a message requires us to login
+    post :mark, :message_id => messages(:unread_message).id
+    assert_response :forbidden
+
+    # Login as a user with no messages
+    session[:user] = users(:second_public_user).id
+    cookies["_osm_username"] = users(:second_public_user).display_name
+
+    # Check that marking a message we didn't send or receive fails
+    post :mark, :message_id => messages(:read_message).id
+    assert_response :not_found
+    assert_template "no_such_message"
+
+    # Login as the message recipient
+    session[:user] = users(:public_user).id
+    cookies["_osm_username"] = users(:public_user).display_name
+
+    # Check that the marking a message read works
+    post :mark, :message_id => messages(:unread_message).id, :mark => "read"
+    assert_redirected_to inbox_path(:display_name => users(:public_user).display_name)
+    assert_equal true, Message.find(messages(:unread_message).id).message_read
+
+    # Check that the marking a message unread works
+    post :mark, :message_id => messages(:unread_message).id, :mark => "unread"
+    assert_redirected_to inbox_path(:display_name => users(:public_user).display_name)
+    assert_equal false, Message.find(messages(:unread_message).id).message_read
+
+    # Check that the marking a message read via XHR works
+    xhr :post, :mark, :message_id => messages(:unread_message).id, :mark => "read"
+    assert_response :success
+    assert_template "mark"
+    assert_equal true, Message.find(messages(:unread_message).id).message_read
+
+    # Check that the marking a message unread via XHR works
+    xhr :post, :mark, :message_id => messages(:unread_message).id, :mark => "unread"
+    assert_response :success
+    assert_template "mark"
+    assert_equal false, Message.find(messages(:unread_message).id).message_read
+
+    # Asking to mark a message with no ID should fail
+    post :mark
+    assert_response :not_found
+    assert_template "no_such_message"
+
+    # Asking to mark a message with a bogus ID should fail
+    post :mark, :message_id => 99999
+    assert_response :not_found
+    assert_template "no_such_message"
+  end
+
+  ##
+  # test the delete action
+  def test_delete
+    # Check that the deleting a message requires us to login
+    post :delete, :message_id => messages(:read_message).id
+    assert_response :forbidden
+
+    # Login as a user with no messages
+    session[:user] = users(:second_public_user).id
+    cookies["_osm_username"] = users(:second_public_user).display_name
+
+    # Check that deleting a message we didn't send or receive fails
+    post :delete, :message_id => messages(:read_message).id
+    assert_response :not_found
+    assert_template "no_such_message"
+
+    # Login as the message recipient
+    session[:user] = users(:normal_user).id
+    cookies["_osm_username"] = users(:normal_user).display_name
+
+    # Check that the deleting a received message works
+    post :delete, :message_id => messages(:read_message).id
+    assert_redirected_to inbox_path(:display_name => users(:normal_user).display_name)
+    assert_equal "Message deleted", flash[:notice]
+    m = Message.find(messages(:read_message).id)
+    assert_equal true, m.from_user_visible
+    assert_equal false, m.to_user_visible
+
+    # Check that the deleting a sent message works
+    post :delete, :message_id => messages(:unread_message).id
+    assert_redirected_to inbox_path(:display_name => users(:normal_user).display_name)
+    assert_equal "Message deleted", flash[:notice]
+    m = Message.find(messages(:unread_message).id)
+    assert_equal false, m.from_user_visible
+    assert_equal true, m.to_user_visible
+
+    # Asking to delete a message with no ID should fail
+    post :delete
+    assert_response :not_found
+    assert_template "no_such_message"
+
+    # Asking to delete a message with a bogus ID should fail
+    post :delete, :message_id => 99999
+    assert_response :not_found
+    assert_template "no_such_message"
+  end
 end
diff --git a/test/unit/message_test.rb b/test/unit/message_test.rb
index 863b1a4f2..260852aea 100644
--- a/test/unit/message_test.rb
+++ b/test/unit/message_test.rb
@@ -23,14 +23,14 @@ class MessageTest < ActiveSupport::TestCase
   end
   
   def test_validating_msgs
-    message = messages(:one)
+    message = messages(:unread_message)
     assert message.valid?
-    massage = messages(:two)
+    massage = messages(:read_message)
     assert message.valid?
   end
   
   def test_invalid_send_recipient
-    message = messages(:one)
+    message = messages(:unread_message)
     message.sender = nil
     message.recipient = nil
     assert !message.valid?
@@ -184,7 +184,7 @@ class MessageTest < ActiveSupport::TestCase
 private
 
   def make_message(char, count)
-    message = messages(:one)
+    message = messages(:unread_message)
     message.title = char * count
     return message
   end