From 12a5c69abdfb7aa5dfab2e849e9f2180bd3b1203 Mon Sep 17 00:00:00 2001 From: Roman Rizzi Date: Wed, 17 Apr 2019 12:14:40 -0300 Subject: [PATCH] FEATURE: Allow users to tone down digest emails (#7353) * FEATURE: Allow user to tone down email digest insteand of only unsubscribing * Reordered options and select the next slowest frequency by default --- app/controllers/email_controller.rb | 37 ++++- app/views/email/unsubscribe.html.erb | 28 +++- config/locales/server.en.yml | 18 ++- spec/requests/email_controller_spec.rb | 196 ++++++++++++++----------- 4 files changed, 182 insertions(+), 97 deletions(-) diff --git a/app/controllers/email_controller.rb b/app/controllers/email_controller.rb index b4c8f0b7497..7f24d0fb95b 100644 --- a/app/controllers/email_controller.rb +++ b/app/controllers/email_controller.rb @@ -16,6 +16,7 @@ class EmailController < ApplicationController if @user = key.user post = key.post @topic = post&.topic || key.topic + @digest_unsubscribe = !@topic && !SiteSetting.disable_digest_emails @type = key.unsubscribe_key_type @not_found = false @@ -35,6 +36,8 @@ class EmailController < ApplicationController .count end end + else + @digest_frequencies = digest_frequencies(@user) end end end @@ -85,8 +88,13 @@ class EmailController < ApplicationController updated = true end - if params["disable_digest_emails"] - user.user_option.update_columns(email_digests: false) + if params['digest_after_minutes'] + digest_frequency = params['digest_after_minutes'].to_i + + user.user_option.update_columns( + digest_after_minutes: digest_frequency, + email_digests: digest_frequency.positive? + ) updated = true end @@ -123,4 +131,29 @@ class EmailController < ApplicationController @topic = topic if topic && Guardian.new(nil).can_see?(topic) end + private + + def digest_frequencies(user) + frequency_in_minutes = user.user_option.digest_after_minutes + frequencies = DigestEmailSiteSetting.values.dup + never = frequencies.delete_at(0) + allowed_frequencies = %w[never weekly every_month every_six_months] + + result = frequencies.reduce(frequencies: [], current: nil, selected: nil, take_next: false) do |memo, v| + memo[:current] = v[:name] if v[:value] == frequency_in_minutes + next(memo) unless allowed_frequencies.include?(v[:name]) + + memo.tap do |m| + m[:selected] = v[:value] if m[:take_next] + m[:frequencies] << [I18n.t("unsubscribe.digest_frequency.#{v[:name]}"), v[:value]] + m[:take_next] = !m[:take_next] && m[:current] + end + end + + result.slice(:frequencies, :current, :selected).tap do |r| + r[:frequencies] << [I18n.t("unsubscribe.digest_frequency.#{never[:name]}"), never[:value]] + r[:selected] ||= never[:value] + r[:current] ||= never[:name] + end + end end diff --git a/app/views/email/unsubscribe.html.erb b/app/views/email/unsubscribe.html.erb index 762cd8afae4..aee145e5ba3 100644 --- a/app/views/email/unsubscribe.html.erb +++ b/app/views/email/unsubscribe.html.erb @@ -52,15 +52,26 @@

<% end %> - <% if !@topic %> - <% unless SiteSetting.disable_digest_emails %> + <% if @digest_unsubscribe %>

- + + <% if @digest_frequencies[:current] %> +

+ <%= t( + 'unsubscribe.digest_frequency.title', + frequency: t("unsubscribe.digest_frequency.#{@digest_frequencies[:current]}") + ) %> +

+
+ <% end %> + + + <%= + select_tag :digest_after_minutes, + options_for_select(@digest_frequencies[:frequencies], @digest_frequencies[:selected]), + class: 'combobox' + %>

- <% end %> <% end %>

@@ -71,7 +82,8 @@


- <%= submit_tag t('unsubscribe.title'), class: 'btn btn-danger' %> + <% text = @type=='digest' ? t('unsubscribe.submit') : t('unsubscribe.title') %> + <%= submit_tag text, class: 'btn btn-danger' %> <%- end %> diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index 68d599da38f..a008d44dbeb 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -893,8 +893,8 @@ en: remove: "This topic is no longer a banner. It will no longer appear at the top of every page." unsubscribed: - title: "Unsubscribed!" - description: "%{email} has been unsubscribed. To change your email settings visit your user preferences." + title: "Email preferences updated!" + description: "email preferences for %{email} were updated. To change your email settings visit your user preferences." topic_description: "To re-subscribe to %{link}, use the notification control at the bottom or right of the topic." private_topic_description: "To re-subscribe, use the notification control at the bottom or right of the topic." @@ -904,11 +904,23 @@ en: mute_topic: "Mute all notifications for this topic, %{link}" unwatch_category: "Stop watching all topics in %{category}" mailing_list_mode: "Turn off mailing list mode" - disable_digest_emails: "Stop sending me summary emails" all: "Don't send me any mail from %{sitename}" different_user_description: "You are currently logged in as a different user than the one we emailed. Please log out, or enter anonymous mode, and try again." not_found_description: "Sorry, we couldn't find this unsubscribe. It's possible the link in your email has expired?" log_out: "Log Out" + submit: "Save preferences" + digest_frequency: + title: "You are receiving summary emails %{frequency}" + select_title: 'Set summary emails frequency to:' + + never: 'never' + every_30_minutes: "every 30 minutes" + every_hour: "hourly" + daily: "daily" + weekly: "weekly" + every_month: "every month" + every_six_months: "every six months" + user_api_key: title: "Authorize application access" diff --git a/spec/requests/email_controller_spec.rb b/spec/requests/email_controller_spec.rb index df4be4fe857..3af77b39d04 100644 --- a/spec/requests/email_controller_spec.rb +++ b/spec/requests/email_controller_spec.rb @@ -11,10 +11,10 @@ RSpec.describe EmailController do expect(response.status).to eq(404) end - it 'can fully unsubscribe' do - user = Fabricate(:user) - key = UnsubscribeKey.create_key_for(user, "all") + let(:user) { Fabricate(:user) } + let(:key) { UnsubscribeKey.create_key_for(user, "all") } + it 'can fully unsubscribe' do user.user_option.update_columns(email_digests: true, email_level: UserOption.email_level_types[:never], email_messages_level: UserOption.email_level_types[:never]) @@ -37,9 +37,6 @@ RSpec.describe EmailController do end it 'can disable mailing list' do - user = Fabricate(:user) - key = UnsubscribeKey.create_key_for(user, "all") - user.user_option.update_columns(mailing_list_mode: true) post "/email/unsubscribe/#{key}.json", @@ -52,19 +49,31 @@ RSpec.describe EmailController do expect(user.user_option.mailing_list_mode).to eq(false) end - it 'can disable digest' do - user = Fabricate(:user) - key = UnsubscribeKey.create_key_for(user, "all") - - user.user_option.update_columns(email_digests: true) + it 'Can change digest frequency' do + weekly_interval_minutes = 10080 + user.user_option.update_columns(email_digests: true, digest_after_minutes: 0) post "/email/unsubscribe/#{key}.json", - params: { disable_digest_emails: "1" } + params: { digest_after_minutes: weekly_interval_minutes.to_s } expect(response.status).to eq(302) user.user_option.reload + expect(user.user_option.digest_after_minutes).to eq(weekly_interval_minutes) + end + + it 'Can disable email digests setting frequency to zero' do + user.user_option.update_columns(email_digests: true, digest_after_minutes: 10080) + + post "/email/unsubscribe/#{key}.json", + params: { digest_after_minutes: '0' } + + expect(response.status).to eq(302) + + user.user_option.reload + + expect(user.user_option.digest_after_minutes).to be_zero expect(user.user_option.email_digests).to eq(false) end @@ -175,99 +184,118 @@ RSpec.describe EmailController do end context '#unsubscribe' do - it 'displays log out button if wrong user logged in' do - sign_in(Fabricate(:admin)) - user = Fabricate(:user) - key = UnsubscribeKey.create_key_for(user, "digest") - - get "/email/unsubscribe/#{key}" - - expect(response.body).to include(I18n.t("unsubscribe.log_out")) - expect(response.body).to include(I18n.t("unsubscribe.different_user_description")) - end - it 'displays not found if key is not found' do - get "/email/unsubscribe/#{SecureRandom.hex}" + navigate_to_unsubscribe(SecureRandom.hex) + expect(response.body).to include(CGI.escapeHTML(I18n.t("unsubscribe.not_found_description"))) end - it 'correctly handles mailing list mode' do - user = Fabricate(:user) - key = UnsubscribeKey.create_key_for(user, "digest") + let(:user) { Fabricate(:user) } + let(:unsubscribe_key) { UnsubscribeKey.create_key_for(user, key_type) } - user.user_option.update_columns(mailing_list_mode: true) + context 'Unsubscribe from digest' do + let(:key_type) { 'digest' } - get "/email/unsubscribe/#{key}" - expect(response.body).to include(I18n.t("unsubscribe.mailing_list_mode")) + it 'displays log out button if wrong user logged in' do + sign_in(Fabricate(:admin)) - SiteSetting.disable_mailing_list_mode = true + navigate_to_unsubscribe - get "/email/unsubscribe/#{key}" - expect(response.body).not_to include(I18n.t("unsubscribe.mailing_list_mode")) + expect(response.body).to include(I18n.t("unsubscribe.log_out")) + expect(response.body).to include(I18n.t("unsubscribe.different_user_description")) + end - user.user_option.update_columns(mailing_list_mode: false) - SiteSetting.disable_mailing_list_mode = false + it 'correctly handles mailing list mode' do + user.user_option.update_columns(mailing_list_mode: true) - get "/email/unsubscribe/#{key}" - expect(response.body).not_to include(I18n.t("unsubscribe.mailing_list_mode")) + navigate_to_unsubscribe + expect(response.body).to include(I18n.t("unsubscribe.mailing_list_mode")) + + SiteSetting.disable_mailing_list_mode = true + + navigate_to_unsubscribe + expect(response.body).not_to include(I18n.t("unsubscribe.mailing_list_mode")) + + user.user_option.update_columns(mailing_list_mode: false) + SiteSetting.disable_mailing_list_mode = false + + navigate_to_unsubscribe + expect(response.body).not_to include(I18n.t("unsubscribe.mailing_list_mode")) + end + + it 'Lets you select the digest frequency ranging from never to half a year' do + selected_digest_frequency = 0 + slow_digest_frequencies = ['weekly', 'every month', 'every six months', 'never'] + + navigate_to_unsubscribe + + source = Nokogiri::HTML::fragment(response.body) + expect(source.css(".combobox option").map(&:inner_text)).to eq(slow_digest_frequencies) + end + + it 'Selects the next slowest frequency by default' do + every_month_freq = 43200 + six_months_freq = 259200 + user.user_option.update_columns(digest_after_minutes: every_month_freq) + + navigate_to_unsubscribe + + source = Nokogiri::HTML::fragment(response.body) + expect(source.css(".combobox option[selected='selected']")[0]['value']).to eq(six_months_freq.to_s) + end + + it 'Uses never as the selected frequency if current one is six months' do + never_frequency = 0 + six_months_freq = 259200 + user.user_option.update_columns(digest_after_minutes: six_months_freq) + + navigate_to_unsubscribe + + source = Nokogiri::HTML::fragment(response.body) + expect(source.css(".combobox option[selected='selected']")[0]['value']).to eq(never_frequency.to_s) + end end - it 'correctly handles digest unsubscribe' do - user = Fabricate(:user) - user.user_option.update_columns(email_digests: false) - key = UnsubscribeKey.create_key_for(user, "digest") + context 'Unsubscribe from a post' do + let(:post) { Fabricate(:post) } + let(:user) { post.user } + let(:key_type) { post } - # because we are type digest we will always show digest and it will be selected - get "/email/unsubscribe/#{key}" - expect(response.body).to include(I18n.t("unsubscribe.disable_digest_emails")) + it 'correctly handles watched categories' do + cu = create_category_user(:watching) - source = Nokogiri::HTML::fragment(response.body) - expect(source.css("#disable_digest_emails")[0]["checked"]).to eq("checked") + navigate_to_unsubscribe + expect(response.body).to include("unwatch_category") - SiteSetting.disable_digest_emails = true + cu.destroy! - get "/email/unsubscribe/#{key}" - expect(response.body).not_to include(I18n.t("unsubscribe.disable_digest_emails")) + navigate_to_unsubscribe + expect(response.body).not_to include("unwatch_category") + end - SiteSetting.disable_digest_emails = false - key = UnsubscribeKey.create_key_for(user, "not_digest") + it 'correctly handles watched first post categories' do + cu = create_category_user(:watching_first_post) - get "/email/unsubscribe/#{key}" - expect(response.body).to include(I18n.t("unsubscribe.disable_digest_emails")) + navigate_to_unsubscribe + expect(response.body).to include("unwatch_category") + + cu.destroy! + + navigate_to_unsubscribe + expect(response.body).not_to include("unwatch_category") + end + + def create_category_user(notification_level) + CategoryUser.create!( + user_id: user.id, + category_id: post.topic.category_id, + notification_level: CategoryUser.notification_levels[notification_level] + ) + end end - it 'correctly handles watched categories' do - post = Fabricate(:post) - user = post.user - cu = CategoryUser.create!(user_id: user.id, - category_id: post.topic.category_id, - notification_level: CategoryUser.notification_levels[:watching]) - - key = UnsubscribeKey.create_key_for(user, post) + def navigate_to_unsubscribe(key = unsubscribe_key) get "/email/unsubscribe/#{key}" - expect(response.body).to include("unwatch_category") - - cu.destroy! - - get "/email/unsubscribe/#{key}" - expect(response.body).not_to include("unwatch_category") - end - - it 'correctly handles watched first post categories' do - post = Fabricate(:post) - user = post.user - cu = CategoryUser.create!(user_id: user.id, - category_id: post.topic.category_id, - notification_level: CategoryUser.notification_levels[:watching_first_post]) - - key = UnsubscribeKey.create_key_for(user, post) - get "/email/unsubscribe/#{key}" - expect(response.body).to include("unwatch_category") - - cu.destroy! - - get "/email/unsubscribe/#{key}" - expect(response.body).not_to include("unwatch_category") end end end