From: Tom Hughes <tom@compton.nu>
Date: Sat, 24 Feb 2024 11:08:53 +0000 (+0000)
Subject: Add unsubscribe link to changeset notification mails
X-Git-Tag: live~762^2~1
X-Git-Url: https://git.openstreetmap.org./rails.git/commitdiff_plain/f0764d3ecae3aa9251eadff96990f4afe530924d

Add unsubscribe link to changeset notification mails
---

diff --git a/app/abilities/ability.rb b/app/abilities/ability.rb
index 4a4eee390..5e0e835d4 100644
--- a/app/abilities/ability.rb
+++ b/app/abilities/ability.rb
@@ -36,6 +36,7 @@ class Ability
       can [:show], :deletion
 
       if Settings.status != "database_offline"
+        can [:subscribe, :unsubscribe], Changeset
         can [:index, :new, :create, :show, :edit, :update, :destroy], ClientApplication
         can [:index, :new, :create, :show, :edit, :update, :destroy], :oauth2_application
         can [:index, :destroy], :oauth2_authorized_application
diff --git a/app/controllers/changesets_controller.rb b/app/controllers/changesets_controller.rb
index 859242b60..5eb14629a 100644
--- a/app/controllers/changesets_controller.rb
+++ b/app/controllers/changesets_controller.rb
@@ -9,6 +9,7 @@ class ChangesetsController < ApplicationController
   before_action :authorize_web
   before_action :set_locale
   before_action -> { check_database_readable(:need_api => true) }, :only => [:index, :feed]
+  before_action :check_database_writable, :only => [:subscribe, :unsubscribe]
 
   authorize_resource
 
@@ -74,6 +75,34 @@ class ChangesetsController < ApplicationController
     index
   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
 
   #------------------------------------------------------------
diff --git a/app/mailers/user_mailer.rb b/app/mailers/user_mailer.rb
index 0894b972d..f4cd9d3e5 100644
--- a/app/mailers/user_mailer.rb
+++ b/app/mailers/user_mailer.rb
@@ -162,6 +162,7 @@ class UserMailer < ApplicationMailer
       @changeset_comment = comment.changeset.tags["comment"].presence
       @time = comment.created_at
       @changeset_author = comment.changeset.user.display_name
+      @unsubscribe_url = changeset_unsubscribe_url(comment.changeset)
       @author = @commenter
 
       subject = if @owner
diff --git a/app/views/changesets/_heading.html.erb b/app/views/changesets/_heading.html.erb
new file mode 100644
index 000000000..33bc71696
--- /dev/null
+++ b/app/views/changesets/_heading.html.erb
@@ -0,0 +1,15 @@
+<% title = changeset.tags["comment"].to_s.presence || t(".title", :id => changeset.id) -%>
+<div class='mb-3'>
+  <div class="row">
+    <div class="col-auto">
+      <%= user_thumbnail changeset.user %>
+    </div>
+    <div class="col">
+      <h2><%= link_to title, changeset_path(changeset) %></h2>
+    </div>
+  </div>
+
+  <small class='text-muted'>
+    <%= t(".created_by_html", :link_user => link_to(changeset.user.display_name, user_path(changeset.user)), :created => l(changeset.created_at, :format => :blog)) %>
+  </small>
+</div>
diff --git a/app/views/changesets/no_such_entry.html.erb b/app/views/changesets/no_such_entry.html.erb
new file mode 100644
index 000000000..da7d18524
--- /dev/null
+++ b/app/views/changesets/no_such_entry.html.erb
@@ -0,0 +1,5 @@
+<% content_for :heading do %>
+  <h1><%= t ".heading", :id => h(params[:id]) %></h1>
+<% end %>
+
+<p><%= t ".body", :id => h(params[:id]) %></p>
diff --git a/app/views/changesets/subscribe.html.erb b/app/views/changesets/subscribe.html.erb
new file mode 100644
index 000000000..6a65e5fec
--- /dev/null
+++ b/app/views/changesets/subscribe.html.erb
@@ -0,0 +1,12 @@
+<% content_for :heading do %>
+  <h1><%= t ".heading" %></h1>
+<% end %>
+
+<%= render :partial => "heading", :object => @changeset, :as => "changeset" %>
+
+<%= bootstrap_form_tag do |f| %>
+  <% if params[:referer] -%>
+  <%= f.hidden_field :referer, :value => params[:referer] %>
+  <% end -%>
+  <%= f.primary t(".button") %>
+<% end %>
diff --git a/app/views/changesets/unsubscribe.html.erb b/app/views/changesets/unsubscribe.html.erb
new file mode 100644
index 000000000..6a65e5fec
--- /dev/null
+++ b/app/views/changesets/unsubscribe.html.erb
@@ -0,0 +1,12 @@
+<% content_for :heading do %>
+  <h1><%= t ".heading" %></h1>
+<% end %>
+
+<%= render :partial => "heading", :object => @changeset, :as => "changeset" %>
+
+<%= bootstrap_form_tag do |f| %>
+  <% if params[:referer] -%>
+  <%= f.hidden_field :referer, :value => params[:referer] %>
+  <% end -%>
+  <%= f.primary t(".button") %>
+<% end %>
diff --git a/app/views/user_mailer/changeset_comment_notification.html.erb b/app/views/user_mailer/changeset_comment_notification.html.erb
index 95c5cdc5b..efba71eb6 100644
--- a/app/views/user_mailer/changeset_comment_notification.html.erb
+++ b/app/views/user_mailer/changeset_comment_notification.html.erb
@@ -24,6 +24,6 @@
 
 <% content_for :footer do %>
   <p>
-    <%= t ".unsubscribe_html", :url => link_to(@changeset_url, @changeset_url, :style => "color: #222") %>
+    <%= t ".unsubscribe_html", :url => link_to(@unsubscribe_url, @unsubscribe_url) %>
   </p>
 <% end %>
diff --git a/app/views/user_mailer/changeset_comment_notification.text.erb b/app/views/user_mailer/changeset_comment_notification.text.erb
index ce9c0099a..8c0727583 100644
--- a/app/views/user_mailer/changeset_comment_notification.text.erb
+++ b/app/views/user_mailer/changeset_comment_notification.text.erb
@@ -17,4 +17,4 @@
 
 <%= t '.details', :url => @changeset_url %>
 
-<%= t '.unsubscribe', :url => @changeset_url %>
+<%= t '.unsubscribe', :url => @unsubscribe_url %>
diff --git a/config/locales/en.yml b/config/locales/en.yml
index 775030638..35dcd3bad 100644
--- a/config/locales/en.yml
+++ b/config/locales/en.yml
@@ -464,6 +464,19 @@ en:
       feed:
         title: "Changeset %{id}"
         title_comment: "Changeset %{id} - %{comment}"
+    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:
+      title: "No such changeset"
+      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."
     timeout:
       sorry: "Sorry, the list of changesets you requested took too long to retrieve."
   changeset_comments:
@@ -1667,8 +1680,8 @@ en:
         partial_changeset_without_comment: "without comment"
       details: "More details about the changeset can be found at %{url}."
       details_html: "More details about the changeset can be found at %{url}."
-      unsubscribe: 'To unsubscribe from updates to this changeset, visit %{url} and click "Unsubscribe".'
-      unsubscribe_html: 'To unsubscribe from updates to this changeset, visit %{url} and click "Unsubscribe".'
+      unsubscribe: "You can unsubscribe from updates to this changeset at %{url}."
+      unsubscribe_html: "You can unsubscribe from updates to this changeset at %{url}."
   confirmations:
     confirm:
       heading: Check your email!
diff --git a/config/routes.rb b/config/routes.rb
index 6969218b7..af5730d21 100644
--- a/config/routes.rb
+++ b/config/routes.rb
@@ -127,6 +127,8 @@ OpenStreetMap::Application.routes.draw do
   get "/user/:display_name/notes" => "notes#index", :as => :user_notes
   get "/history/friends" => "changesets#index", :friends => true, :as => "friend_changesets", :defaults => { :format => :html }
   get "/history/nearby" => "changesets#index", :nearby => true, :as => "nearby_changesets", :defaults => { :format => :html }
+  match "/changeset/:id/subscribe" => "changesets#subscribe", :via => [:get, :post], :as => "changeset_subscribe"
+  match "/changeset/:id/unsubscribe" => "changesets#unsubscribe", :via => [:get, :post], :as => "changeset_unsubscribe"
 
   get "/browse/way/:id",                :to => redirect(:path => "/way/%{id}")
   get "/browse/way/:id/history",        :to => redirect(:path => "/way/%{id}/history")
diff --git a/test/controllers/changesets_controller_test.rb b/test/controllers/changesets_controller_test.rb
index dba7642c4..a0747a0cd 100644
--- a/test/controllers/changesets_controller_test.rb
+++ b/test/controllers/changesets_controller_test.rb
@@ -28,6 +28,22 @@ class ChangesetsControllerTest < ActionDispatch::IntegrationTest
       { :path => "/history/feed", :method => :get },
       { :controller => "changesets", :action => "feed", :format => :atom }
     )
+    assert_routing(
+      { :path => "/changeset/1/subscribe", :method => :get },
+      { :controller => "changesets", :action => "subscribe", :id => "1" }
+    )
+    assert_routing(
+      { :path => "/changeset/1/subscribe", :method => :post },
+      { :controller => "changesets", :action => "subscribe", :id => "1" }
+    )
+    assert_routing(
+      { :path => "/changeset/1/unsubscribe", :method => :get },
+      { :controller => "changesets", :action => "unsubscribe", :id => "1" }
+    )
+    assert_routing(
+      { :path => "/changeset/1/unsubscribe", :method => :post },
+      { :controller => "changesets", :action => "unsubscribe", :id => "1" }
+    )
   end
 
   ##
@@ -319,6 +335,123 @@ class ChangesetsControllerTest < ActionDispatch::IntegrationTest
     assert_redirected_to :action => :feed
   end
 
+  def test_subscribe_page
+    user = create(:user)
+    other_user = create(:user)
+    changeset = create(:changeset, :user => user)
+    path = changeset_subscribe_path(changeset)
+
+    get path
+    assert_response :redirect
+    assert_redirected_to login_path(:referer => path)
+
+    session_for(other_user)
+    get path
+    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
+    end
+  end
+
+  def test_subscribe_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_subscribe_path(changeset)
+    end
+    assert_response :redirect
+    assert_redirected_to changeset_path(changeset)
+    assert changeset.reload.subscribed?(other_user)
+  end
+
+  def test_subscribe_fail
+    user = create(:user)
+    other_user = create(:user)
+
+    changeset = create(:changeset, :user => user)
+
+    # not signed in
+    assert_no_difference "changeset.subscribers.count" do
+      post changeset_subscribe_path(changeset)
+    end
+    assert_response :forbidden
+
+    session_for(other_user)
+
+    # bad diary id
+    post changeset_subscribe_path(999111)
+    assert_response :not_found
+
+    # trying to subscribe when already subscribed
+    post changeset_subscribe_path(changeset)
+    assert_no_difference "changeset.subscribers.count" do
+      post changeset_subscribe_path(changeset)
+    end
+  end
+
+  def test_unsubscribe_page
+    user = create(:user)
+    other_user = create(:user)
+    changeset = create(:changeset, :user => user)
+    path = changeset_unsubscribe_path(changeset)
+
+    get path
+    assert_response :redirect
+    assert_redirected_to login_path(:referer => path)
+
+    session_for(other_user)
+    get path
+    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
+    end
+  end
+
+  def test_unsubscribe_success
+    user = create(:user)
+    other_user = create(:user)
+
+    changeset = create(:changeset, :user => user)
+    changeset.subscribers.push(other_user)
+
+    session_for(other_user)
+    assert_difference "changeset.subscribers.count", -1 do
+      post changeset_unsubscribe_path(changeset)
+    end
+    assert_response :redirect
+    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
+      post changeset_unsubscribe_path(changeset)
+    end
+    assert_response :forbidden
+
+    session_for(other_user)
+
+    # bad diary id
+    post changeset_unsubscribe_path(999111)
+    assert_response :not_found
+
+    # trying to unsubscribe when not subscribed
+    assert_no_difference "changeset.subscribers.count" do
+      post changeset_unsubscribe_path(changeset)
+    end
+  end
+
   private
 
   ##