Merge remote-tracking branch 'upstream/pull/5535'

app/abilities/ability.rb

@@ -32,7 +32,7 @@ def initialize(user)
       can :read, [:deletion, :account_terms]
 
       if Settings.status != "database_offline"
-        can [:subscribe, :unsubscribe], Changeset
+        can [:read, :create, :destroy], :changeset_subscription
         can [:read, :create, :update, :destroy], :oauth2_application
         can [:read, :destroy], :oauth2_authorized_application
         can [:read, :create, :destroy], :oauth2_authorization

app/controllers/changeset_subscriptions_controller.rb

@@ -0,0 +1,38 @@
+class ChangesetSubscriptionsController < ApplicationController
+  layout "site"
+
+  before_action :authorize_web
+  before_action :set_locale
+  before_action :check_database_writable
+
+  authorize_resource :class => :changeset_subscription
+
+  around_action :web_timeout
+
+  def show
+    @changeset = Changeset.find(params[:changeset_id])
+    @subscribed = @changeset.subscribed?(current_user)
+  rescue ActiveRecord::RecordNotFound
+    render :action => "no_such_entry", :status => :not_found
+  end
+
+  def create
+    @changeset = Changeset.find(params[:changeset_id])
+
+    @changeset.subscribe(current_user) unless @changeset.subscribed?(current_user)
+
+    redirect_to changeset_path(@changeset)
+  rescue ActiveRecord::RecordNotFound
+    render :action => "no_such_entry", :status => :not_found
+  end
+
+  def destroy
+    @changeset = Changeset.find(params[:changeset_id])
+
+    @changeset.unsubscribe(current_user)
+
+    redirect_to changeset_path(@changeset)
+  rescue ActiveRecord::RecordNotFound
+    render :action => "no_such_entry", :status => :not_found
+  end
+end

app/controllers/changesets_controller.rb

@@ -7,9 +7,8 @@ class ChangesetsController < ApplicationController
 
   before_action :authorize_web
   before_action :set_locale
-  before_action -> { check_database_readable(:need_api => true) }, :only => [:index, :feed, :show]
+  before_action -> { check_database_readable(:need_api => true) }
   before_action :require_oauth, :only => :show
-  before_action :check_database_writable, :only => [:subscribe, :unsubscribe]
 
   authorize_resource
 
@@ -107,34 +106,6 @@ def show
     render :template => "browse/not_found", :status => :not_found, :layout => map_layout
   end
 
-  ##
-  # subscribe to a changeset
-  def subscribe
-    @changeset = Changeset.find(params[:id])
-
-    if request.post?
-      @changeset.subscribe(current_user) unless @changeset.subscribed?(current_user)
-
-      redirect_to changeset_path(@changeset)
-    end
-  rescue ActiveRecord::RecordNotFound
-    render :action => "no_such_entry", :status => :not_found
-  end
-
-  ##
-  # unsubscribe from a changeset
-  def unsubscribe
-    @changeset = Changeset.find(params[:id])
-
-    if request.post?
-      @changeset.unsubscribe(current_user)
-
-      redirect_to changeset_path(@changeset)
-    end
-  rescue ActiveRecord::RecordNotFound
-    render :action => "no_such_entry", :status => :not_found
-  end
-
   private
 
   #------------------------------------------------------------

app/mailers/user_mailer.rb

@@ -177,7 +177,7 @@ def changeset_comment_notification(comment, recipient)
       @changeset_comment = comment.changeset.tags["comment"].presence
       @time = comment.created_at
       @changeset_author = comment.changeset.user.display_name
-      @unsubscribe_url = unsubscribe_changeset_url(comment.changeset)
+      @changeset_subscription_url = changeset_subscription_url(comment.changeset)
       @author = @commenter
 
       subject = if @owner
@@ -193,8 +193,8 @@ def changeset_comment_notification(comment, recipient)
       set_list_headers(
         "#{comment.changeset.id}.changeset.www.openstreetmap.org",
         t(".description", :id => comment.changeset.id),
-        :subscribe => subscribe_changeset_url(comment.changeset),
-        :unsubscribe => @unsubscribe_url,
+        :subscribe => @changeset_subscription_url,
+        :unsubscribe => @changeset_subscription_url,
         :archive => @changeset_url
       )
 

app/views/changeset_subscriptions/no_such_entry.html.erb

@@ -0,0 +1,5 @@
+<% content_for :heading do %>
+  <h1><%= t ".heading", :id => h(params[:changeset_id]) %></h1>
+<% end %>
+
+<p><%= t ".body", :id => h(params[:changeset_id]) %></p>

app/views/changeset_subscriptions/show.html.erb

@@ -0,0 +1,12 @@
+<% content_for :heading do %>
+  <h1><%= @subscribed ? t(".unsubscribe.heading") : t(".subscribe.heading") %></h1>
+<% end %>
+
+<%= render :partial => "heading", :object => @changeset, :as => "changeset" %>
+
+<%= bootstrap_form_tag :method => (@subscribed ? :delete : :post) do |f| %>
+  <% if params[:referer] -%>
+  <%= f.hidden_field :referer, :value => params[:referer] %>
+  <% end -%>
+  <%= f.primary @subscribed ? t(".unsubscribe.button") : t(".subscribe.button") %>
+<% end %>

app/views/user_mailer/changeset_comment_notification.html.erb

@@ -24,6 +24,6 @@
 
 <% content_for :footer do %>
   <p>
-    <%= t ".unsubscribe_html", :url => link_to(@unsubscribe_url, @unsubscribe_url) %>
+    <%= t ".unsubscribe_html", :url => link_to(@changeset_subscription_url, @changeset_subscription_url) %>
   </p>
 <% end %>

app/views/user_mailer/changeset_comment_notification.text.erb

@@ -17,4 +17,4 @@
 
 <%= t '.details', :url => @changeset_url %>
 
-<%= t '.unsubscribe', :url => @unsubscribe_url %>
+<%= t '.unsubscribe', :url => @changeset_subscription_url %>

config/locales/en.yml

@@ -484,18 +484,6 @@ en:
         created: "Created"
         closed: "Closed"
         belongs_to: "Author"
-    subscribe:
-      heading: Subscribe to the following changeset discussion?
-      button: Subscribe to discussion
-    unsubscribe:
-      heading: Unsubscribe from the following changeset discussion?
-      button: Unsubscribe from discussion
-    heading:
-      title: "Changeset %{id}"
-      created_by_html: "Created by %{link_user} on %{created}."
-    no_such_entry:
-      heading: "No entry with the id: %{id}"
-      body: "Sorry, there is no changeset with the id %{id}. Please check your spelling, or maybe the link you clicked is wrong."
     show:
       title: "Changeset: %{id}"
       created: "Created: %{when}"
@@ -527,6 +515,20 @@ en:
       sorry: "Sorry, changeset #%{id} could not be found."
     timeout:
       sorry: "Sorry, the list of changesets you requested took too long to retrieve."
+  changeset_subscriptions:
+    show:
+      subscribe:
+        heading: Subscribe to the following changeset discussion?
+        button: Subscribe to discussion
+      unsubscribe:
+        heading: Unsubscribe from the following changeset discussion?
+        button: Unsubscribe from discussion
+    heading:
+      title: "Changeset %{id}"
+      created_by_html: "Created by %{link_user} on %{created}."
+    no_such_entry:
+      heading: "No entry with the id: %{id}"
+      body: "Sorry, there is no changeset with the id %{id}. Please check your spelling, or maybe the link you clicked is wrong."
   dashboards:
     contact:
       km away: "%{count}km away"

config/routes.rb

@@ -124,14 +124,16 @@
   get "/relation/:id" => "relations#show", :id => /\d+/, :as => :relation
   get "/relation/:id/history" => "old_relations#index", :id => /\d+/, :as => :relation_history
   resources :old_relations, :path => "/relation/:id/history", :id => /\d+/, :version => /\d+/, :param => :version, :only => :show
-  resources :changesets, :path => "changeset", :id => /\d+/, :only => :show do
-    match :subscribe, :on => :member, :via => [:get, :post]
-    match :unsubscribe, :on => :member, :via => [:get, :post]
 
+  resources :changesets, :path => "changeset", :id => /\d+/, :only => :show do
+    resource :subscription, :controller => :changeset_subscriptions, :only => [:show, :create, :destroy]
     namespace :changeset_comments, :as => :comments, :path => :comments do
       resource :feed, :only => :show, :defaults => { :format => "rss" }
     end
   end
+  get "/changeset/:id/subscribe", :id => /\d+/, :to => redirect(:path => "/changeset/%{id}/subscription")
+  get "/changeset/:id/unsubscribe", :id => /\d+/, :to => redirect(:path => "/changeset/%{id}/subscription")
+
   resources :notes, :path => "note", :id => /\d+/, :only => [:show, :new]
 
   get "/user/:display_name/history" => "changesets#index"

test/controllers/changeset_subscriptions_controller_test.rb

@@ -0,0 +1,150 @@
+require "test_helper"
+
+class ChangesetSubscriptionsControllerTest < ActionDispatch::IntegrationTest
+  ##
+  # test all routes which lead to this controller
+  def test_routes
+    assert_routing(
+      { :path => "/changeset/1/subscription", :method => :get },
+      { :controller => "changeset_subscriptions", :action => "show", :changeset_id => "1" }
+    )
+    assert_routing(
+      { :path => "/changeset/1/subscription", :method => :post },
+      { :controller => "changeset_subscriptions", :action => "create", :changeset_id => "1" }
+    )
+    assert_routing(
+      { :path => "/changeset/1/subscription", :method => :delete },
+      { :controller => "changeset_subscriptions", :action => "destroy", :changeset_id => "1" }
+    )
+
+    get "/changeset/1/subscribe"
+    assert_redirected_to "/changeset/1/subscription"
+
+    get "/changeset/1/unsubscribe"
+    assert_redirected_to "/changeset/1/subscription"
+  end
+
+  def test_show_as_anonymous
+    changeset = create(:changeset)
+
+    get changeset_subscription_path(changeset)
+    assert_redirected_to login_path(:referer => changeset_subscription_path(changeset))
+  end
+
+  def test_show_when_not_subscribed
+    user = create(:user)
+    other_user = create(:user)
+    changeset = create(:changeset, :user => user)
+
+    session_for(other_user)
+    get changeset_subscription_path(changeset)
+
+    assert_response :success
+    assert_dom ".content-body" do
+      assert_dom "a[href='#{changeset_path(changeset)}']", :text => "Changeset #{changeset.id}"
+      assert_dom "a[href='#{user_path(user)}']", :text => user.display_name
+      assert_dom "form" do
+        assert_dom "> @action", changeset_subscription_path(changeset)
+        assert_dom "input[type=submit]" do
+          assert_dom "> @value", "Subscribe to discussion"
+        end
+      end
+    end
+  end
+
+  def test_show_when_subscribed
+    user = create(:user)
+    other_user = create(:user)
+    changeset = create(:changeset, :user => user)
+    changeset.subscribers << other_user
+
+    session_for(other_user)
+    get changeset_subscription_path(changeset)
+
+    assert_response :success
+    assert_dom ".content-body" do
+      assert_dom "a[href='#{changeset_path(changeset)}']", :text => "Changeset #{changeset.id}"
+      assert_dom "a[href='#{user_path(user)}']", :text => user.display_name
+      assert_dom "form" do
+        assert_dom "> @action", changeset_subscription_path(changeset)
+        assert_dom "input[type=submit]" do
+          assert_dom "> @value", "Unsubscribe from discussion"
+        end
+      end
+    end
+  end
+
+  def test_create_success
+    user = create(:user)
+    other_user = create(:user)
+    changeset = create(:changeset, :user => user)
+
+    session_for(other_user)
+    assert_difference "changeset.subscribers.count", 1 do
+      post changeset_subscription_path(changeset)
+    end
+    assert_redirected_to changeset_path(changeset)
+    assert changeset.reload.subscribed?(other_user)
+  end
+
+  def test_create_fail
+    user = create(:user)
+    other_user = create(:user)
+    changeset = create(:changeset, :user => user)
+    changeset.subscribers << other_user
+
+    # not signed in
+    assert_no_difference "changeset.subscribers.count" do
+      post changeset_subscription_path(changeset)
+    end
+    assert_response :forbidden
+
+    session_for(other_user)
+
+    # bad diary id
+    post changeset_subscription_path(999111)
+    assert_response :not_found
+
+    # trying to subscribe when already subscribed
+    assert_no_difference "changeset.subscribers.count" do
+      post changeset_subscription_path(changeset)
+    end
+  end
+
+  def test_destroy_success
+    user = create(:user)
+    other_user = create(:user)
+    changeset = create(:changeset, :user => user)
+    changeset.subscribers << other_user
+
+    session_for(other_user)
+    assert_difference "changeset.subscribers.count", -1 do
+      delete changeset_subscription_path(changeset)
+    end
+    assert_redirected_to changeset_path(changeset)
+    assert_not changeset.reload.subscribed?(other_user)
+  end
+
+  def test_unsubscribe_fail
+    user = create(:user)
+    other_user = create(:user)
+    changeset = create(:changeset, :user => user)
+
+    # not signed in
+    assert_no_difference "changeset.subscribers.count" do
+      delete changeset_subscription_path(changeset)
+    end
+    assert_response :forbidden
+
+    session_for(other_user)
+
+    # bad diary id
+    delete changeset_subscription_path(999111)
+    assert_response :not_found
+
+    # trying to unsubscribe when not subscribed
+    assert_no_difference "changeset.subscribers.count" do
+      delete changeset_subscription_path(changeset)
+    end
+  end
+end