diff --git a/app/controllers/user_controller.rb b/app/controllers/user_controller.rb index 197b289140..0423740010 100644 --- a/app/controllers/user_controller.rb +++ b/app/controllers/user_controller.rb @@ -706,6 +706,8 @@ def update_user(user, params) user.auth_uid = nil end + user.preferred_email_format = params[:user][:preferred_email_format] + if user.save set_locale diff --git a/app/models/notifier.rb b/app/models/notifier.rb index 8f9e3e2954..725447b3fb 100644 --- a/app/models/notifier.rb +++ b/app/models/notifier.rb @@ -4,7 +4,6 @@ class Notifier < ActionMailer::Base :auto_submitted => "auto-generated" helper :application before_action :set_shared_template_vars - before_action :attach_project_logo def signup_confirm(user, token) with_recipient_locale user do @@ -13,8 +12,8 @@ def signup_confirm(user, token) :display_name => user.display_name, :confirm_string => token.token) - mail :to => user.email, - :subject => I18n.t("notifier.signup_confirm.subject") + compose_mail user, + :subject => I18n.t("notifier.signup_confirm.subject") end end @@ -25,8 +24,9 @@ def email_confirm(user, token) :controller => "user", :action => "confirm_email", :confirm_string => token.token) - mail :to => user.new_email, - :subject => I18n.t("notifier.email_confirm.subject") + compose_mail user, + :subject => I18n.t("notifier.email_confirm.subject"), + :to => user.new_email end end @@ -36,8 +36,8 @@ def lost_password(user, token) :controller => "user", :action => "reset_password", :token => token.token) - mail :to => user.email, - :subject => I18n.t("notifier.lost_password.subject") + compose_mail user, + :subject => I18n.t("notifier.lost_password.subject") end end @@ -49,8 +49,8 @@ def gpx_success(trace, possible_points) @trace_tags = trace.tags @possible_points = possible_points - mail :to => trace.user.email, - :subject => I18n.t("notifier.gpx_notification.success.subject") + compose_mail trace.user, + :subject => I18n.t("notifier.gpx_notification.success.subject") end end @@ -61,8 +61,8 @@ def gpx_failure(trace, error) @trace_tags = trace.tags @error = error - mail :to => trace.user.email, - :subject => I18n.t("notifier.gpx_notification.failure.subject") + compose_mail trace.user, + :subject => I18n.t("notifier.gpx_notification.failure.subject") end end @@ -80,11 +80,10 @@ def message_notification(message) :message_id => message.id) @author = @from_user - attach_user_avatar(message.sender) - - mail :from => from_address(message.sender.display_name, "m", message.id, message.digest), - :to => message.recipient.email, - :subject => I18n.t("notifier.message_notification.subject_header", :subject => message.title) + compose_mail message.recipient, + :from => from_address(message.sender.display_name, "m", message.id, message.digest), + :subject => I18n.t("notifier.message_notification.subject_header", :subject => message.title), + :attach_avatar => message.sender end end @@ -113,11 +112,10 @@ def diary_comment_notification(comment, recipient) :title => "Re: #{comment.diary_entry.title}") @author = @from_user - attach_user_avatar(comment.user) - - mail :from => from_address(comment.user.display_name, "c", comment.id, comment.digest, recipient.id), - :to => recipient.email, - :subject => I18n.t("notifier.diary_comment_notification.subject", :user => comment.user.display_name) + compose_mail recipient, + :from => from_address(comment.user.display_name, "c", comment.id, comment.digest, recipient.id), + :subject => I18n.t("notifier.diary_comment_notification.subject", :user => comment.user.display_name), + :attach_avatar => comment.user end end @@ -132,9 +130,9 @@ def friend_notification(friend) :display_name => @friend.befriender.display_name) @author = @friend.befriender.display_name - attach_user_avatar(@friend.befriender) - mail :to => friend.befriendee.email, - :subject => I18n.t("notifier.friend_notification.subject", :user => friend.befriender.display_name) + compose_mail friend.befriendee, + :subject => I18n.t("notifier.friend_notification.subject", :user => friend.befriender.display_name), + :attach_avatar => @friend.befriender end end @@ -145,15 +143,12 @@ def note_comment_notification(comment, recipient) @comment = comment.body @owner = recipient == comment.note.author @event = comment.event - @commenter = if comment.author comment.author.display_name else I18n.t("notifier.note_comment_notification.anonymous") end - @author = @commenter - attach_user_avatar(comment.author) subject = if @owner I18n.t("notifier.note_comment_notification.#{@event}.subject_own", :commenter => @commenter) @@ -161,7 +156,9 @@ def note_comment_notification(comment, recipient) I18n.t("notifier.note_comment_notification.#{@event}.subject_other", :commenter => @commenter) end - mail :to => recipient.email, :subject => subject + compose_mail recipient, + :subject => subject, + :attach_avatar => comment.author end end @@ -183,24 +180,42 @@ def changeset_comment_notification(comment, recipient) I18n.t("notifier.changeset_comment_notification.commented.subject_other", :commenter => @commenter) end - attach_user_avatar(comment.author) - - mail :to => recipient.email, :subject => subject + compose_mail recipient, + :subject => subject, + :attach_avatar => comment.author end end private + def compose_mail(recipient, options = {}) + needs_avatar = options.include? :attach_avatar + avatar_user = options.delete :attach_avatar + options[:to] ||= recipient.email + mail(options) do |format| + if recipient.preferred_email_format == "text_only" + format.text + else + unless recipient.preferred_email_format == "multipart" + logger.warn "Unknown email format '#{recipient.preferred_email_format}, treated as 'multipart'" + end + attach_images(needs_avatar, avatar_user) + format.text + format.html + end + end + end + def set_shared_template_vars @root_url = root_url(:host => SERVER_URL) end - def attach_project_logo + def attach_images(needs_avatar, avatar_user) + # NB we could have (needs_avatar==true && avatar_user.nil?) iff this is an anonymous comment attachments.inline["logo.png"] = File.read(Rails.root.join("app", "assets", "images", "osm_logo_30.png")) - end - - def attach_user_avatar(user) - attachments.inline["avatar.png"] = File.read(user_avatar_file_path(user)) + if needs_avatar + attachments.inline["avatar.png"] = File.read(user_avatar_file_path(avatar_user)) + end end def user_avatar_file_path(user) diff --git a/app/views/user/account.html.erb b/app/views/user/account.html.erb index 47a84e99bc..6abb432916 100644 --- a/app/views/user/account.html.erb +++ b/app/views/user/account.html.erb @@ -51,7 +51,11 @@ <%= f.select :auth_provider, Auth::PROVIDERS %> <%= f.text_field :auth_uid %> (<%= t 'user.account.openid.link text' %>) - + +
+ + <%= f.select :preferred_email_format, [[t('user.email_format.multipart'), 'multipart'], [t('user.email_format.text_only'), 'text_only']] %> +
diff --git a/config/locales/en.yml b/config/locales/en.yml index 7edeea7d0c..31c5be82a7 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -1951,6 +1951,7 @@ en: return to profile: Return to profile flash update success confirm needed: "User information updated successfully. Check your email for a note to confirm your new email address." flash update success: "User information updated successfully." + preferred_email_format: "Preferred Email Format:" confirm: heading: Check your email! introduction_1: | @@ -2029,6 +2030,9 @@ en: If you already have an account, you can login to your account using your username and password and then associate the account with your ID in your user settings. + email_format: + multipart: Mixed Text & HTML (default) + text_only: Text Only user_role: filter: not_an_administrator: "Only administrators can perform user role management, and you are not an administrator." diff --git a/db/migrate/20170320220020_add_preferred_email_format.rb b/db/migrate/20170320220020_add_preferred_email_format.rb new file mode 100644 index 0000000000..00d3a52c39 --- /dev/null +++ b/db/migrate/20170320220020_add_preferred_email_format.rb @@ -0,0 +1,8 @@ +require "migrate" + +class AddPreferredEmailFormat < ActiveRecord::Migration + def change + create_enumeration :email_format_enum, %w(text_only multipart) + add_column :users, :preferred_email_format, :email_format_enum, :null => false, :default => "multipart" + end +end diff --git a/test/controllers/changeset_controller_test.rb b/test/controllers/changeset_controller_test.rb index 9d39a8555c..f7adf57aee 100644 --- a/test/controllers/changeset_controller_test.rb +++ b/test/controllers/changeset_controller_test.rb @@ -1,6 +1,7 @@ require "test_helper" require "changeset_controller" +# rubocop:disable ClassLength class ChangesetControllerTest < ActionController::TestCase api_fixtures @@ -2032,6 +2033,29 @@ def test_create_comment_success assert_equal "[OpenStreetMap] pulibc_test2 has commented on a changeset you are interested in", email.subject ActionMailer::Base.deliveries.clear + + changeset.subscribers.clear + changeset.subscribers.push(users(:text_only_emails_user)) + + assert_difference "ActionMailer::Base.deliveries.size", 1 do + post :comment, :id => changeset.id, :text => "This is a comment" + end + assert_message_is_text_only(ActionMailer::Base.deliveries.first) do |part| + assert_match "This is a comment", part.to_s + end + + ActionMailer::Base.deliveries.clear + + changeset.subscribers.clear + changeset.subscribers.push(users(:multipart_emails_user)) + assert_difference "ActionMailer::Base.deliveries.size", 1 do + post :comment, :id => changeset.id, :text => "This is a comment" + end + assert_message_is_multipart(ActionMailer::Base.deliveries.first) do |part| + assert_match "This is a comment", part.to_s + end + + ActionMailer::Base.deliveries.clear end ## diff --git a/test/controllers/diary_entry_controller_test.rb b/test/controllers/diary_entry_controller_test.rb index 49d56c4e42..da9b888ffd 100644 --- a/test/controllers/diary_entry_controller_test.rb +++ b/test/controllers/diary_entry_controller_test.rb @@ -2,6 +2,7 @@ class DiaryEntryControllerTest < ActionController::TestCase include ActionView::Helpers::NumberHelper + api_fixtures def setup # Create the default language for diary entries @@ -405,6 +406,36 @@ def test_comment end end + def test_diary_comment_notification_email_format + commenter = create(:user) + + # User with email pref set to multipart gets multipart message + diary_author = users(:multipart_emails_user) + entry = create(:diary_entry, :user => diary_author) + post :subscribe, { :id => entry.id, :display_name => diary_author.display_name }, { :user => diary_author } + assert_difference "ActionMailer::Base.deliveries.size", entry.subscribers.count do + post :comment, { :display_name => diary_author.display_name, :id => entry.id, :diary_comment => { :body => "New comment" } }, { :user => commenter } + end + assert_message_is_multipart(ActionMailer::Base.deliveries.first) do |part| + assert_match "New comment", part.to_s + end + + ActionMailer::Base.deliveries.clear + + # User with email pref set to text-only gets text-only message + diary_author = users(:text_only_emails_user) + entry = create(:diary_entry, :user => diary_author) + post :subscribe, { :id => entry.id, :display_name => diary_author.display_name }, { :user => diary_author } + assert_difference "ActionMailer::Base.deliveries.size", entry.subscribers.count do + post :comment, { :display_name => diary_author.display_name, :id => entry.id, :diary_comment => { :body => "New comment" } }, { :user => commenter } + end + assert_message_is_text_only(ActionMailer::Base.deliveries.first) do |part| + assert_match "New comment", part.to_s + end + + ActionMailer::Base.deliveries.clear + end + def test_comment_spammy user = create(:user) other_user = create(:user) diff --git a/test/controllers/message_controller_test.rb b/test/controllers/message_controller_test.rb index ae71046c16..e0cd94d5a6 100644 --- a/test/controllers/message_controller_test.rb +++ b/test/controllers/message_controller_test.rb @@ -1,6 +1,8 @@ require "test_helper" class MessageControllerTest < ActionController::TestCase + api_fixtures + ## # test all routes which lead to this controller def test_routes @@ -194,6 +196,38 @@ def test_new_post_send assert_select "h1", "The user non_existent_user does not exist" end + def test_email_format + session[:user] = create(:user).id + message_title = "Test Message" + message_body = "Test message body" + + # User with email pref set to multipart gets multipart message + assert_difference "ActionMailer::Base.deliveries.size", 1 do + post :new, + :display_name => users(:multipart_emails_user).display_name, + :message => { :title => message_title, :body => message_body } + end + assert_message_is_multipart(ActionMailer::Base.deliveries.first) do |part| + assert_match message_title, part.to_s + assert_match message_body, part.to_s + end + + ActionMailer::Base.deliveries.clear + + # User with email pref set to text-only gets text-only message + assert_difference "ActionMailer::Base.deliveries.size", 1 do + post :new, + :display_name => users(:text_only_emails_user).display_name, + :message => { :title => message_title, :body => message_body } + end + assert_message_is_text_only(ActionMailer::Base.deliveries.first) do |part| + assert_match message_title, part.to_s + assert_match message_body, part.to_s + end + + ActionMailer::Base.deliveries.clear + end + ## # test the new action message limit def test_new_limit diff --git a/test/controllers/notes_controller_test.rb b/test/controllers/notes_controller_test.rb index 3ee283f4a7..293e1485a7 100644 --- a/test/controllers/notes_controller_test.rb +++ b/test/controllers/notes_controller_test.rb @@ -1,6 +1,8 @@ require "test_helper" class NotesControllerTest < ActionController::TestCase + api_fixtures + def setup # Stub nominatim response for note locations stub_request(:get, %r{^http://nominatim\.openstreetmap\.org/reverse\?}) @@ -999,4 +1001,32 @@ def test_mine_success get :mine, :display_name => "non-existent" assert_response :not_found end + + def test_note_comment_notification_email_format + # User with email pref set to multipart gets multipart message + note_with_comments_by_users = create(:note) do |note| + create(:note_comment, :note => note, :author => users(:multipart_emails_user)) + end + assert_difference "ActionMailer::Base.deliveries.size", 1 do + post :comment, :id => note_with_comments_by_users.id, :text => "This is an additional comment", :format => "json" + end + assert_message_is_multipart(ActionMailer::Base.deliveries.first) do |part| + assert_match "This is an additional comment", part.to_s + end + + ActionMailer::Base.deliveries.clear + + # User with email pref set to text-only gets text-only message + note_with_comments_by_users = create(:note) do |note| + create(:note_comment, :note => note, :author => users(:text_only_emails_user)) + end + assert_difference "ActionMailer::Base.deliveries.size", 1 do + post :comment, :id => note_with_comments_by_users.id, :text => "This is an additional comment", :format => "json" + end + assert_message_is_text_only(ActionMailer::Base.deliveries.first) do |part| + assert_match "This is an additional comment", part.to_s + end + + ActionMailer::Base.deliveries.clear + end end diff --git a/test/controllers/user_controller_test.rb b/test/controllers/user_controller_test.rb index 3232b9e2ed..73a595ad52 100644 --- a/test/controllers/user_controller_test.rb +++ b/test/controllers/user_controller_test.rb @@ -1410,7 +1410,7 @@ def test_list_get_paginated get :list, :page => 3 assert_response :success assert_template :list - assert_select "table#user_list tr", :count => 28 + assert_select "table#user_list tr", :count => 30 end def test_list_post_confirm diff --git a/test/fixtures/users.yml b/test/fixtures/users.yml index 18b33f6bfc..48e9b7c85d 100644 --- a/test/fixtures/users.yml +++ b/test/fixtures/users.yml @@ -347,3 +347,29 @@ wikipedia_user: terms_agreed: "2010-01-01 11:22:33" terms_seen: true languages: en + +text_only_emails_user: + id: 26 + email: text-only-user@example.com + status: active + pass_crypt: <%= Digest::MD5.hexdigest('test') %> + creation_time: "2008-05-01 01:23:45" + display_name: textonly_emails + data_public: true + terms_agreed: "2010-01-01 11:22:33" + terms_seen: true + languages: en + preferred_email_format: text_only + +multipart_emails_user: + id: 27 + email: multipart-user@example.com + status: active + pass_crypt: <%= Digest::MD5.hexdigest('test') %> + creation_time: "2008-05-01 01:23:45" + display_name: multipart_emails + data_public: true + terms_agreed: "2010-01-01 11:22:33" + terms_seen: true + languages: en + preferred_email_format: multipart diff --git a/test/integration/user_creation_test.rb b/test/integration/user_creation_test.rb index 93e55939cc..1a755cee1a 100644 --- a/test/integration/user_creation_test.rb +++ b/test/integration/user_creation_test.rb @@ -94,6 +94,8 @@ def test_user_create_success # Check that the confirm account url is correct assert_match /#{@url}/, register_email.body.to_s + assert_message_is_multipart register_email + # Check the page assert_response :success assert_template "user/confirm" diff --git a/test/test_helper.rb b/test/test_helper.rb index dba0051454..01dc7e0582 100644 --- a/test/test_helper.rb +++ b/test/test_helper.rb @@ -180,13 +180,34 @@ def stub_hostip_requests end def email_text_parts(message) - message.parts.each_with_object([]) do |part, text_parts| - if part.content_type.start_with?("text/") - text_parts.push(part) - elsif part.multipart? - text_parts.concat(email_text_parts(part)) + text_parts = [] + if message.parts.empty? + text_parts.push(message) + else + message.parts.each do |part| + if part.content_type.start_with?("text/") + text_parts.push(part) + elsif part.multipart? + text_parts.concat(email_text_parts(part)) + end end end + assert_not_empty text_parts + text_parts + end + + def assert_message_is_multipart(email) + text_parts = email_text_parts(email) + part_types = text_parts.collect { |part| part.content_type.sub(/;.+/, "") } + assert_includes part_types, "text/plain" + assert_includes part_types, "text/html" + text_parts.each { |part| yield part } if block_given? + end + + def assert_message_is_text_only(email) + assert_empty email.parts + assert_match %r{^text/plain}, email.content_type + yield email if block_given? end end end