From 573991e5a9997d5ea14e430ae58a40ef0eaecd34 Mon Sep 17 00:00:00 2001
From: Tom Hughes <tom@compton.nu>
Date: Wed, 15 Aug 2012 19:48:06 +0100
Subject: [PATCH] Fixup make_friend and remove_friend properly

Requests using POST are now actioned immediately, while requests
using GET present a confirmation page.
---
 app/controllers/user_controller.rb      | 70 ++++++++++++++-----------
 app/views/user/_contact.html.erb        |  4 +-
 app/views/user/make_friend.html.erb     |  7 +++
 app/views/user/remove_friend.html.erb   |  7 +++
 app/views/user/view.html.erb            |  4 +-
 config/locales/en.yml                   |  4 ++
 config/routes.rb                        |  4 +-
 test/functional/user_controller_test.rb |  8 +++
 8 files changed, 71 insertions(+), 37 deletions(-)
 create mode 100644 app/views/user/make_friend.html.erb
 create mode 100644 app/views/user/remove_friend.html.erb

diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb
index 443c433a5..620ab5090 100644
--- a/app/controllers/user_controller.rb
+++ b/app/controllers/user_controller.rb
@@ -397,47 +397,55 @@ class UserController < ApplicationController
   end
 
   def make_friend
-    if params[:display_name]
-      name = params[:display_name]
-      new_friend = User.active.where(:display_name => name).first
-      friend = Friend.new
-      friend.user_id = @user.id
-      friend.friend_user_id = new_friend.id
-      unless @user.is_friends_with?(new_friend)
-        if friend.save
-          flash[:notice] = t 'user.make_friend.success', :name => name
-          Notifier.friend_notification(friend).deliver
+    @new_friend = User.find_by_display_name(params[:display_name])
+
+    if @new_friend
+      if request.post?
+        friend = Friend.new
+        friend.user_id = @user.id
+        friend.friend_user_id = @new_friend.id
+        unless @user.is_friends_with?(@new_friend)
+          if friend.save
+            flash[:notice] = t 'user.make_friend.success', :name => @new_friend.display_name
+            Notifier.friend_notification(friend).deliver
+          else
+            friend.add_error(t('user.make_friend.failed', :name => @new_friend.display_name))
+          end
         else
-          friend.add_error(t('user.make_friend.failed', :name => name))
+          flash[:warning] = t 'user.make_friend.already_a_friend', :name => @new_friend.display_name
         end
-      else
-        flash[:warning] = t 'user.make_friend.already_a_friend', :name => name
-      end
 
-      if params[:referer]
-        redirect_to params[:referer]
-      else
-        redirect_to :controller => 'user', :action => 'view'
+        if params[:referer]
+          redirect_to params[:referer]
+        else
+          redirect_to :controller => 'user', :action => 'view'
+        end
       end
+    else
+      render_unknown_user params[:display_name]
     end
   end
 
   def remove_friend
-    if params[:display_name]
-      name = params[:display_name]
-      friend = User.active.where(:display_name => name).first
-      if @user.is_friends_with?(friend)
-        Friend.delete_all "user_id = #{@user.id} AND friend_user_id = #{friend.id}"
-        flash[:notice] = t 'user.remove_friend.success', :name => friend.display_name
-      else
-        flash[:error] = t 'user.remove_friend.not_a_friend', :name => friend.display_name
-      end
+    @friend = User.find_by_display_name(params[:display_name])
 
-      if params[:referer]
-        redirect_to params[:referer]
-      else
-        redirect_to :controller => 'user', :action => 'view'
+    if @friend
+      if request.post?
+        if @user.is_friends_with?(@friend)
+          Friend.delete_all "user_id = #{@user.id} AND friend_user_id = #{@friend.id}"
+          flash[:notice] = t 'user.remove_friend.success', :name => @friend.display_name
+        else
+          flash[:error] = t 'user.remove_friend.not_a_friend', :name => @friend.display_name
+        end
+
+        if params[:referer]
+          redirect_to params[:referer]
+        else
+          redirect_to :controller => 'user', :action => 'view'
+        end
       end
+    else
+      render_unknown_user params[:display_name]
     end
   end
 
diff --git a/app/views/user/_contact.html.erb b/app/views/user/_contact.html.erb
index 2418ed24c..c0bf160fc 100644
--- a/app/views/user/_contact.html.erb
+++ b/app/views/user/_contact.html.erb
@@ -33,9 +33,9 @@
     <%= link_to t('user.view.send message'), :controller => 'message', :action => 'new', :display_name => contact.display_name %>
     |
     <% if @user.is_friends_with?(contact) %>
-      <%= link_to t('user.view.remove as friend'), :controller => 'user', :action => 'remove_friend', :display_name => contact.display_name, :referer => request.fullpath, :method => :post %>
+      <%= link_to t('user.view.remove as friend'), remove_friend_path(:display_name => contact.display_name, :referer => request.fullpath), :method => :post %>
     <% else %>
-      <%= link_to t('user.view.add as friend'), :controller => 'user', :action => 'make_friend', :display_name => contact.display_name, :referer => request.fullpath, :method => :post %>
+      <%= link_to t('user.view.add as friend'), make_friend_path(:display_name => contact.display_name, :referer => request.fullpath), :method => :post %>
     <% end %>
   </td>
 </tr>
diff --git a/app/views/user/make_friend.html.erb b/app/views/user/make_friend.html.erb
new file mode 100644
index 000000000..abae0d201
--- /dev/null
+++ b/app/views/user/make_friend.html.erb
@@ -0,0 +1,7 @@
+<h1><%= t "user.make_friend.heading", :user => @new_friend.display_name %></h1>
+<%= form_tag do %>
+  <% if params[:referer] -%>
+  <%= hidden_field_tag("referer", params[:referer]) %>
+  <% end -%>
+  <%= submit_tag t("user.make_friend.button") %>
+<% end %>
diff --git a/app/views/user/remove_friend.html.erb b/app/views/user/remove_friend.html.erb
new file mode 100644
index 000000000..0919bc95a
--- /dev/null
+++ b/app/views/user/remove_friend.html.erb
@@ -0,0 +1,7 @@
+<h1><%= t "user.remove_friend.heading", :user => @friend.display_name %></h1>
+<%= form_tag do %>
+  <% if params[:referer] -%>
+  <%= hidden_field_tag("referer", params[:referer]) %>
+  <% end -%>
+  <%= submit_tag t("user.remove_friend.button") %>
+<% end %>
diff --git a/app/views/user/view.html.erb b/app/views/user/view.html.erb
index 8ba4c0725..da5804304 100644
--- a/app/views/user/view.html.erb
+++ b/app/views/user/view.html.erb
@@ -41,9 +41,9 @@
     <%= link_to t('user.view.comments'), :controller => 'diary_entry', :action => 'comments', :display_name => @this_user.display_name %>
     |
     <% if @user and @user.is_friends_with?(@this_user) %>
-      <%= link_to t('user.view.remove as friend'), :controller => 'user', :action => 'remove_friend', :display_name => @this_user.display_name, :method => :post %>
+      <%= link_to t('user.view.remove as friend'), remove_friend_path(:display_name => @this_user.display_name), :method => :post %>
     <% else %>
-      <%= link_to t('user.view.add as friend'), :controller => 'user', :action => 'make_friend', :display_name => @this_user.display_name, :method => :post %>
+      <%= link_to t('user.view.add as friend'), make_friend_path(:display_name => @this_user.display_name), :method => :post %>
     <% end %>
     <% if @this_user.blocks.exists? %>
       |
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 7ecb5c2e9..34104a7d1 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -1831,10 +1831,14 @@ en:
     go_public:
       flash success: "All your edits are now public, and you are now allowed to edit."
     make_friend:
+      heading: "Add %{user} as a friend?"
+      button: "Add as friend"
       success: "%{name} is now your friend."
       failed: "Sorry, failed to add %{name} as a friend."
       already_a_friend: "You are already friends with %{name}."
     remove_friend:
+      heading: "Remove %{user} as a friend?"
+      button: "Remove as friend"
       success: "%{name} was removed from your friends."
       not_a_friend: "%{name} is not one of your friends."
     filter:
diff --git a/config/routes.rb b/config/routes.rb
index c5119b945..1ef0c31af 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -174,8 +174,8 @@ OpenStreetMap::Application.routes.draw do
 
   # user pages
   match '/user/:display_name' => 'user#view', :via => :get, :as => "user"
-  match '/user/:display_name/make_friend' => 'user#make_friend', :via => :post
-  match '/user/:display_name/remove_friend' => 'user#remove_friend', :via => :post
+  match '/user/:display_name/make_friend' => 'user#make_friend', :via => [:get, :post], :as => "make_friend"
+  match '/user/:display_name/remove_friend' => 'user#remove_friend', :via => [:get, :post], :as => "remove_friend"
   match '/user/:display_name/account' => 'user#account', :via => [:get, :post]
   match '/user/:display_name/set_status' => 'user#set_status', :via => :get, :as => :set_status_user
   match '/user/:display_name/delete' => 'user#delete', :via => :get, :as => :delete_user
diff --git a/test/functional/user_controller_test.rb b/test/functional/user_controller_test.rb
index e85308183..fbceb3c05 100644
--- a/test/functional/user_controller_test.rb
+++ b/test/functional/user_controller_test.rb
@@ -139,10 +139,18 @@ class UserControllerTest < ActionController::TestCase
       { :controller => "user", :action => "account", :display_name => "username" }
     )
 
+    assert_routing(
+      { :path => "/user/username/make_friend", :method => :get },
+      { :controller => "user", :action => "make_friend", :display_name => "username" }
+    )
     assert_routing(
       { :path => "/user/username/make_friend", :method => :post },
       { :controller => "user", :action => "make_friend", :display_name => "username" }
     )
+    assert_routing(
+      { :path => "/user/username/remove_friend", :method => :get },
+      { :controller => "user", :action => "remove_friend", :display_name => "username" }
+    )
     assert_routing(
       { :path => "/user/username/remove_friend", :method => :post },
       { :controller => "user", :action => "remove_friend", :display_name => "username" }
-- 
2.39.5