Merge pull request #4534 from tomhughes/list-headers

Add standard List-XXX headers to some notification mails

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

app/controllers/api/changesets_controller.rb

@@ -44,7 +44,7 @@ module Api
       cs.save_with_tags!
 
       # Subscribe user to changeset comments
-      cs.subscribers << current_user
+      cs.subscribe(current_user)
 
       render :plain => cs.id.to_s
     end
@@ -233,10 +233,10 @@ module Api
 
       # Find the changeset and check it is valid
       changeset = Changeset.find(id)
-      raise OSM::APIChangesetAlreadySubscribedError, changeset if changeset.subscribers.exists?(current_user.id)
+      raise OSM::APIChangesetAlreadySubscribedError, changeset if changeset.subscribed?(current_user)
 
       # Add the subscriber
-      changeset.subscribers << current_user
+      changeset.subscribe(current_user)
 
       # Return a copy of the updated changeset
       @changeset = changeset
@@ -259,10 +259,10 @@ module Api
 
       # Find the changeset and check it is valid
       changeset = Changeset.find(id)
-      raise OSM::APIChangesetNotSubscribedError, changeset unless changeset.subscribers.exists?(current_user.id)
+      raise OSM::APIChangesetNotSubscribedError, changeset unless changeset.subscribed?(current_user)
 
       # Remove the subscriber
-      changeset.subscribers.delete(current_user)
+      changeset.unsubscribe(current_user)
 
       # Return a copy of the updated changeset
       @changeset = changeset

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

app/mailers/user_mailer.rb

@@ -105,6 +105,14 @@ class UserMailer < ApplicationMailer
 
       set_references("diary", comment.diary_entry)
 
+      set_list_headers(
+        "#{comment.diary_entry.id}.diary.www.openstreetmap.org",
+        t(".description", :id => comment.diary_entry.id),
+        :archive => @readurl,
+        :subscribe => diary_entry_subscribe_url(comment.diary_entry.user, comment.diary_entry),
+        :unsubscribe => @unsubscribeurl
+      )
+
       mail :from => from_address(comment.user.display_name, "c", comment.id, comment.notification_token(recipient.id), recipient.id),
            :to => recipient.email,
            :subject => t(".subject", :user => comment.user.display_name)
@@ -143,6 +151,12 @@ class UserMailer < ApplicationMailer
 
       set_references("note", comment.note)
 
+      set_list_headers(
+        "#{comment.note.id}.note.www.openstreetmap.org",
+        t(".description", :id => comment.note.id),
+        :archive => @noteurl
+      )
+
       subject = if @owner
                   t(".#{@event}.subject_own", :commenter => @commenter)
                 else
@@ -163,6 +177,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
@@ -175,6 +190,14 @@ class UserMailer < ApplicationMailer
 
       set_references("changeset", comment.changeset)
 
+      set_list_headers(
+        "#{comment.changeset.id}.changeset.www.openstreetmap.org",
+        t(".description", :id => comment.changeset.id),
+        :subscribe => changeset_subscribe_url(comment.changeset),
+        :unsubscribe => @unsubscribe_url,
+        :archive => @changeset_url
+      )
+
       mail :to => recipient.email, :subject => subject
     end
   end
@@ -247,4 +270,11 @@ class UserMailer < ApplicationMailer
     headers["In-Reply-To"] = ref
     headers["References"] = ref
   end
+
+  def set_list_headers(id, description, options = {})
+    headers["List-ID"] = "#{description} <#{id}>"
+    headers["List-Archive"] = "<#{options[:archive]}>" if options[:archive]
+    headers["List-Subscribe"] = "<#{options[:subscribe]}>" if options[:subscribe]
+    headers["List-Unsubscribe"] = "<#{options[:unsubscribe]}>" if options[:unsubscribe]
+  end
 end

app/models/changeset.rb

@@ -213,4 +213,16 @@ class Changeset < ApplicationRecord
 
     save_with_tags!
   end
+
+  def subscribe(user)
+    subscribers << user
+  end
+
+  def unsubscribe(user)
+    subscribers.delete(user)
+  end
+
+  def subscribed?(user)
+    subscribers.exists?(user.id)
+  end
 end

app/views/browse/changeset.html.erb

@@ -18,9 +18,9 @@
     <% if current_user %>
       <div class="col-auto">
         <% if @changeset.subscribers.exists?(current_user.id) %>
-          <button class="btn btn-sm btn-primary" name="unsubscribe" data-method="POST" data-url="<%= changeset_unsubscribe_url(@changeset) %>"><%= t("javascripts.changesets.show.unsubscribe") %></button>
+          <button class="btn btn-sm btn-primary" name="unsubscribe" data-method="POST" data-url="<%= api_changeset_unsubscribe_url(@changeset) %>"><%= t("javascripts.changesets.show.unsubscribe") %></button>
         <% else %>
-          <button class="btn btn-sm btn-primary" name="subscribe" data-method="POST" data-url="<%= changeset_subscribe_url(@changeset) %>"><%= t("javascripts.changesets.show.subscribe") %></button>
+          <button class="btn btn-sm btn-primary" name="subscribe" data-method="POST" data-url="<%= api_changeset_subscribe_url(@changeset) %>"><%= t("javascripts.changesets.show.subscribe") %></button>
         <% end %>
       </div>
     <% end %>

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>

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>

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

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

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

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

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:
@@ -1571,6 +1584,7 @@ en:
     more: More
   user_mailer:
     diary_comment_notification:
+      description: "OpenStreetMap Diary Entry #%{id}"
       subject: "[OpenStreetMap] %{user} commented on a diary entry"
       hi: "Hi %{to_user},"
       header: "%{from_user} has commented on the OpenStreetMap diary entry with the subject %{subject}:"
@@ -1627,6 +1641,7 @@ en:
       hopefully_you: "Someone (possibly you) has asked for the password to be reset on this email address's openstreetmap.org account."
       click_the_link: "If this is you, please click the link below to reset your password."
     note_comment_notification:
+      description: "OpenStreetMap Note #%{id}"
       anonymous: An anonymous user
       greeting: "Hi,"
       commented:
@@ -1653,6 +1668,7 @@ en:
       details: "More details about the note can be found at %{url}."
       details_html: "More details about the note can be found at %{url}."
     changeset_comment_notification:
+      description: "OpenStreetMap Changeset #%{id}"
       hi: "Hi %{to_user},"
       greeting: "Hi,"
       commented:
@@ -1667,8 +1683,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!

config/routes.rb

@@ -21,8 +21,8 @@ OpenStreetMap::Application.routes.draw do
     post "changeset/:id/upload" => "api/changesets#upload", :as => :changeset_upload, :id => /\d+/
     get "changeset/:id/download" => "api/changesets#download", :as => :changeset_download, :id => /\d+/
     get "changeset/:id" => "api/changesets#show", :as => :changeset_show, :id => /\d+/
-    post "changeset/:id/subscribe" => "api/changesets#subscribe", :as => :changeset_subscribe, :id => /\d+/
-    post "changeset/:id/unsubscribe" => "api/changesets#unsubscribe", :as => :changeset_unsubscribe, :id => /\d+/
+    post "changeset/:id/subscribe" => "api/changesets#subscribe", :as => :api_changeset_subscribe, :id => /\d+/
+    post "changeset/:id/unsubscribe" => "api/changesets#unsubscribe", :as => :api_changeset_unsubscribe, :id => /\d+/
     put "changeset/:id" => "api/changesets#update", :id => /\d+/
     put "changeset/:id/close" => "api/changesets#close", :as => :changeset_close, :id => /\d+/
     get "changesets" => "api/changesets#query"
@@ -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")

test/controllers/api/changesets_controller_test.rb

@@ -2370,14 +2370,14 @@ module Api
       changeset = create(:changeset, :closed)
 
       assert_difference "changeset.subscribers.count", 1 do
-        post changeset_subscribe_path(changeset), :headers => auth_header
+        post api_changeset_subscribe_path(changeset), :headers => auth_header
       end
       assert_response :success
 
       # not closed changeset
       changeset = create(:changeset)
       assert_difference "changeset.subscribers.count", 1 do
-        post changeset_subscribe_path(changeset), :headers => auth_header
+        post api_changeset_subscribe_path(changeset), :headers => auth_header
       end
       assert_response :success
     end
@@ -2390,7 +2390,7 @@ module Api
       # unauthorized
       changeset = create(:changeset, :closed)
       assert_no_difference "changeset.subscribers.count" do
-        post changeset_subscribe_path(changeset)
+        post api_changeset_subscribe_path(changeset)
       end
       assert_response :unauthorized
 
@@ -2398,7 +2398,7 @@ module Api
 
       # bad changeset id
       assert_no_difference "changeset.subscribers.count" do
-        post changeset_subscribe_path(:id => 999111), :headers => auth_header
+        post api_changeset_subscribe_path(:id => 999111), :headers => auth_header
       end
       assert_response :not_found
 
@@ -2406,7 +2406,7 @@ module Api
       changeset = create(:changeset, :closed)
       changeset.subscribers.push(user)
       assert_no_difference "changeset.subscribers.count" do
-        post changeset_subscribe_path(changeset), :headers => auth_header
+        post api_changeset_subscribe_path(changeset), :headers => auth_header
       end
       assert_response :conflict
     end
@@ -2420,7 +2420,7 @@ module Api
       changeset.subscribers.push(user)
 
       assert_difference "changeset.subscribers.count", -1 do
-        post changeset_unsubscribe_path(changeset), :headers => auth_header
+        post api_changeset_unsubscribe_path(changeset), :headers => auth_header
       end
       assert_response :success
 
@@ -2429,7 +2429,7 @@ module Api
       changeset.subscribers.push(user)
 
       assert_difference "changeset.subscribers.count", -1 do
-        post changeset_unsubscribe_path(changeset), :headers => auth_header
+        post api_changeset_unsubscribe_path(changeset), :headers => auth_header
       end
       assert_response :success
     end
@@ -2440,7 +2440,7 @@ module Api
       # unauthorized
       changeset = create(:changeset, :closed)
       assert_no_difference "changeset.subscribers.count" do
-        post changeset_unsubscribe_path(changeset)
+        post api_changeset_unsubscribe_path(changeset)
       end
       assert_response :unauthorized
 
@@ -2448,14 +2448,14 @@ module Api
 
       # bad changeset id
       assert_no_difference "changeset.subscribers.count" do
-        post changeset_unsubscribe_path(:id => 999111), :headers => auth_header
+        post api_changeset_unsubscribe_path(:id => 999111), :headers => auth_header
       end
       assert_response :not_found
 
       # trying to unsubscribe when not subscribed
       changeset = create(:changeset, :closed)
       assert_no_difference "changeset.subscribers.count" do
-        post changeset_unsubscribe_path(changeset), :headers => auth_header
+        post api_changeset_unsubscribe_path(changeset), :headers => auth_header
       end
       assert_response :not_found
     end

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

test/models/changeset_test.rb

@@ -71,4 +71,17 @@ class ChangesetTest < ActiveSupport::TestCase
       Changeset.from_xml(xml, :create => true)
     end
   end
+
+  def test_subscription
+    changeset = create(:changeset)
+    user = create(:user)
+
+    assert_not changeset.subscribed?(user)
+
+    changeset.subscribe(user)
+    assert changeset.subscribed?(user)
+
+    changeset.unsubscribe(changeset.subscribers.first)
+    assert_not changeset.subscribed?(user)
+  end
 end