From f0764d3ecae3aa9251eadff96990f4afe530924d Mon Sep 17 00:00:00 2001 From: Tom Hughes Date: Sat, 24 Feb 2024 11:08:53 +0000 Subject: [PATCH] Add unsubscribe link to changeset notification mails --- app/abilities/ability.rb | 1 + app/controllers/changesets_controller.rb | 29 ++++ app/mailers/user_mailer.rb | 1 + app/views/changesets/_heading.html.erb | 15 ++ app/views/changesets/no_such_entry.html.erb | 5 + app/views/changesets/subscribe.html.erb | 12 ++ app/views/changesets/unsubscribe.html.erb | 12 ++ .../changeset_comment_notification.html.erb | 2 +- .../changeset_comment_notification.text.erb | 2 +- config/locales/en.yml | 17 ++- config/routes.rb | 2 + .../controllers/changesets_controller_test.rb | 133 ++++++++++++++++++ 12 files changed, 227 insertions(+), 4 deletions(-) create mode 100644 app/views/changesets/_heading.html.erb create mode 100644 app/views/changesets/no_such_entry.html.erb create mode 100644 app/views/changesets/subscribe.html.erb create mode 100644 app/views/changesets/unsubscribe.html.erb 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) -%> +
+
+
+ <%= user_thumbnail changeset.user %> +
+
+

<%= link_to title, changeset_path(changeset) %>

+
+
+ + + <%= t(".created_by_html", :link_user => link_to(changeset.user.display_name, user_path(changeset.user)), :created => l(changeset.created_at, :format => :blog)) %> + +
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 %> +

<%= t ".heading", :id => h(params[:id]) %>

+<% end %> + +

<%= t ".body", :id => h(params[:id]) %>

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 %> +

<%= t ".heading" %>

+<% 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 %> +

<%= t ".heading" %>

+<% 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 %>

- <%= t ".unsubscribe_html", :url => link_to(@changeset_url, @changeset_url, :style => "color: #222") %> + <%= t ".unsubscribe_html", :url => link_to(@unsubscribe_url, @unsubscribe_url) %>

<% 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 ## -- 2.39.5