From 852860de6688668d6abe566cb1f4953fd13e2f35 Mon Sep 17 00:00:00 2001 From: Sam Date: Fri, 17 Jun 2016 11:27:52 +1000 Subject: [PATCH] FEATURE: simpler and friendlier unsubscribe workflow - All unsubscribes go to the exact same page - You may unsubscribe from watching a category on that page - You no longer need to be logged in to unsubscribe from a topic - Simplified footer on emails --- app/controllers/email_controller.rb | 126 +++++++-- app/controllers/session_controller.rb | 8 +- app/jobs/scheduled/clean_up_digest_keys.rb | 13 - .../scheduled/clean_up_unsubscribe_keys.rb | 13 + app/mailers/subscription_mailer.rb | 4 +- app/mailers/user_notifications.rb | 4 +- app/models/digest_unsubscribe_key.rb | 33 --- app/models/post.rb | 4 + app/models/unsubscribe_key.rb | 41 +++ app/views/email/unsubscribe.html.erb | 91 +++++-- app/views/email/unsubscribed.html.erb | 14 + config/locales/server.en.yml | 39 +-- config/routes.rb | 4 +- ...615024524_rename_digest_unsbscribe_keys.rb | 19 ++ spec/components/email/message_builder_spec.rb | 4 +- spec/controllers/email_controller_spec.rb | 241 +++++++++++------- spec/models/digest_unsubscribe_key_spec.rb | 31 --- spec/models/unsubscribe_key_spec.rb | 44 ++++ 18 files changed, 499 insertions(+), 234 deletions(-) delete mode 100644 app/jobs/scheduled/clean_up_digest_keys.rb create mode 100644 app/jobs/scheduled/clean_up_unsubscribe_keys.rb delete mode 100644 app/models/digest_unsubscribe_key.rb create mode 100644 app/models/unsubscribe_key.rb create mode 100644 app/views/email/unsubscribed.html.erb create mode 100644 db/migrate/20160615024524_rename_digest_unsbscribe_keys.rb delete mode 100644 spec/models/digest_unsubscribe_key_spec.rb create mode 100644 spec/models/unsubscribe_key_spec.rb diff --git a/app/controllers/email_controller.rb b/app/controllers/email_controller.rb index 22bbec051..bcc8041b6 100644 --- a/app/controllers/email_controller.rb +++ b/app/controllers/email_controller.rb @@ -1,45 +1,123 @@ class EmailController < ApplicationController - skip_before_filter :check_xhr, :preload_json + skip_before_filter :check_xhr, :preload_json, :redirect_to_login_if_required layout 'no_ember' before_filter :ensure_logged_in, only: :preferences_redirect - skip_before_filter :redirect_to_login_if_required def preferences_redirect redirect_to(email_preferences_path(current_user.username_lower)) end def unsubscribe - @user = DigestUnsubscribeKey.user_for_key(params[:key]) - RateLimiter.new(@user, "unsubscribe_via_email", 3, 1.day).performed! unless @user && @user.staff? + key = UnsubscribeKey.find_by(key: params[:key]) - # Don't allow the use of a key while logged in as a different user - if current_user.present? && (@user != current_user) - @different_user = true - return + if key + @user = key.user + post = key.post + @topic = (post && post.topic) || key.topic + @type = key.unsubscribe_key_type + + if current_user.present? && (@user != current_user) + @different_user = @user.name + @return_url = request.original_url + end + + @watching_topic = @topic && TopicUser.exists?(user_id: @user.id, + notification_level: TopicUser.notification_levels[:watching], + topic_id: @topic.id) + + @watched_count = nil + if @topic && @topic.category_id + if CategoryUser.exists?(user_id: @user.id, + notification_level: CategoryUser.notification_levels[:watching], + category_id: @topic.category_id) + @watched_count = TopicUser.joins(:topic) + .where(:user => @user, + :notification_level => TopicUser.notification_levels[:watching], + "topics.category_id" => @topic.category_id + ).count + end + end end if @user.blank? @not_found = true - return end - - if params[:from_all] - @user.user_option.update_columns(email_always: false, - email_digests: false, - email_direct: false, - email_private_messages: false) - else - @user.user_option.update_column(:email_digests, false) - end - - @success = true end - def resubscribe - @user = DigestUnsubscribeKey.user_for_key(params[:key]) - raise Discourse::NotFound unless @user.present? - @user.user_option.update_column(:email_digests, true) + def perform_unsubscribe + + key = UnsubscribeKey.find_by(key: params[:key]) + unless key && key.user + raise Discourse::NotFound + end + + topic = (key.post && key.post.topic) || key.topic + user = key.user + + updated = false + + if topic + if params["unwatch_topic"] + TopicUser.where(topic_id: topic.id, user_id: user.id) + .update_all(notification_level: TopicUser.notification_levels[:tracking]) + updated = true + end + + if params["unwatch_category"] && topic.category_id + TopicUser.joins(:topic) + .where(:user => user, + :notification_level => TopicUser.notification_levels[:watching], + "topics.category_id" => topic.category_id) + .update_all(notification_level: TopicUser.notification_levels[:tracking]) + + CategoryUser.where(user_id: user.id, + category_id: topic.category_id, + notification_level: CategoryUser.notification_levels[:watching] + ) + .destroy_all + updated = true + end + + if params["mute_topic"] + TopicUser.where(topic_id: topic.id, user_id: user.id) + .update_all(notification_level: TopicUser.notification_levels[:muted]) + updated = true + end + end + + if params["disable_mailing_list"] + user.user_option.update_columns(email_always: false) + updated = true + end + + if params["disable_digest_emails"] + user.user_option.update_columns(email_digests: false) + updated = true + end + + if params["unsubscribe_all"] + user.user_option.update_columns(email_always: false, + email_digests: false, + email_direct: false, + email_private_messages: false) + updated = true + end + + unless updated + redirect_to :back + else + if topic + redirect_to path("/email/unsubscribed?topic_id=#{topic.id}") + else + redirect_to path("/email/unsubscribed") + end + end + + end + + def unsubscribed + @topic = Topic.find_by(id: params[:topic_id].to_i) if params[:topic_id] end end diff --git a/app/controllers/session_controller.rb b/app/controllers/session_controller.rb index e252ec6b3..98d59f386 100644 --- a/app/controllers/session_controller.rb +++ b/app/controllers/session_controller.rb @@ -4,7 +4,7 @@ require_dependency 'single_sign_on' class SessionController < ApplicationController skip_before_filter :redirect_to_login_if_required - skip_before_filter :preload_json, :check_xhr, only: ['sso', 'sso_login', 'become', 'sso_provider'] + skip_before_filter :preload_json, :check_xhr, only: ['sso', 'sso_login', 'become', 'sso_provider', 'destroy'] def csrf render json: {csrf: form_authenticity_token } @@ -237,7 +237,11 @@ class SessionController < ApplicationController def destroy reset_session log_off_user - render nothing: true + if request.xhr? + render nothing: true + else + redirect_to (params[:return_url] || path("/")) + end end private diff --git a/app/jobs/scheduled/clean_up_digest_keys.rb b/app/jobs/scheduled/clean_up_digest_keys.rb deleted file mode 100644 index a74eee87e..000000000 --- a/app/jobs/scheduled/clean_up_digest_keys.rb +++ /dev/null @@ -1,13 +0,0 @@ -module Jobs - - class CleanUpDigestKeys < Jobs::Scheduled - every 1.day - - def execute(args) - DigestUnsubscribeKey.where('created_at < ?', 2.months.ago).delete_all - end - - end - -end - diff --git a/app/jobs/scheduled/clean_up_unsubscribe_keys.rb b/app/jobs/scheduled/clean_up_unsubscribe_keys.rb new file mode 100644 index 000000000..053586667 --- /dev/null +++ b/app/jobs/scheduled/clean_up_unsubscribe_keys.rb @@ -0,0 +1,13 @@ +module Jobs + + class CleanUpUnsubscribeKeys < Jobs::Scheduled + every 1.day + + def execute(args) + UnsubscribeKey.where('created_at < ?', 2.months.ago).delete_all + end + + end + +end + diff --git a/app/mailers/subscription_mailer.rb b/app/mailers/subscription_mailer.rb index c36228713..51a66797a 100644 --- a/app/mailers/subscription_mailer.rb +++ b/app/mailers/subscription_mailer.rb @@ -4,11 +4,11 @@ class SubscriptionMailer < ActionMailer::Base include Email::BuildEmailHelper def confirm_unsubscribe(user, opts={}) - unsubscribe_key = DigestUnsubscribeKey.create_key_for(user) + unsubscribe_key = UnsubscribeKey.create_key_for(user, "all") build_email user.email, template: "unsubscribe_mailer", site_title: SiteSetting.title, site_domain_name: Discourse.current_hostname, - confirm_unsubscribe_link: "#{Discourse.base_url}/unsubscribe/#{unsubscribe_key}?from_all=true" + confirm_unsubscribe_link: "#{Discourse.base_url}/unsubscribe/#{unsubscribe_key}" end end diff --git a/app/mailers/user_notifications.rb b/app/mailers/user_notifications.rb index b863955fe..be69a384e 100644 --- a/app/mailers/user_notifications.rb +++ b/app/mailers/user_notifications.rb @@ -382,7 +382,7 @@ class UserNotifications < ActionMailer::Base username: username, add_unsubscribe_link: !user.staged, mailing_list_mode: user.user_option.mailing_list_mode, - unsubscribe_url: post.topic.unsubscribe_url, + unsubscribe_url: post.unsubscribe_url(user), allow_reply_by_email: allow_reply_by_email, only_reply_by_email: allow_reply_by_email && user.staged, use_site_subject: use_site_subject, @@ -416,6 +416,6 @@ class UserNotifications < ActionMailer::Base @header_color = ColorScheme.hex_for_name('header_background') @anchor_color = ColorScheme.hex_for_name('tertiary') @markdown_linker = MarkdownLinker.new(@base_url) - @unsubscribe_key = DigestUnsubscribeKey.create_key_for(@user) + @unsubscribe_key = UnsubscribeKey.create_key_for(@user, "digest") end end diff --git a/app/models/digest_unsubscribe_key.rb b/app/models/digest_unsubscribe_key.rb deleted file mode 100644 index 34ed72fd1..000000000 --- a/app/models/digest_unsubscribe_key.rb +++ /dev/null @@ -1,33 +0,0 @@ -class DigestUnsubscribeKey < ActiveRecord::Base - belongs_to :user - - before_create :generate_random_key - - def self.create_key_for(user) - DigestUnsubscribeKey.create(user_id: user.id).key - end - - def self.user_for_key(key) - where(key: key).first.try(:user) - end - - private - - def generate_random_key - self.key = SecureRandom.hex(32) - end -end - -# == Schema Information -# -# Table name: digest_unsubscribe_keys -# -# key :string(64) not null, primary key -# user_id :integer not null -# created_at :datetime -# updated_at :datetime -# -# Indexes -# -# index_digest_unsubscribe_keys_on_created_at (created_at) -# diff --git a/app/models/post.rb b/app/models/post.rb index 65a1e7fcb..eaa3c7c25 100644 --- a/app/models/post.rb +++ b/app/models/post.rb @@ -385,6 +385,10 @@ class Post < ActiveRecord::Base end end + def unsubscribe_url(user) + "#{Discourse.base_url}/email/unsubscribe/#{UnsubscribeKey.create_key_for(user, self)}" + end + def self.url(slug, topic_id, post_number) "/t/#{slug}/#{topic_id}/#{post_number}" end diff --git a/app/models/unsubscribe_key.rb b/app/models/unsubscribe_key.rb new file mode 100644 index 000000000..c7220cf9e --- /dev/null +++ b/app/models/unsubscribe_key.rb @@ -0,0 +1,41 @@ +class UnsubscribeKey < ActiveRecord::Base + belongs_to :user + belongs_to :post + belongs_to :topic + + before_create :generate_random_key + + def self.create_key_for(user, type) + if Post === type + create(user_id: user.id, unsubscribe_key_type: "topic", topic_id: type.topic_id, post_id: type.id).key + else + create(user_id: user.id, unsubscribe_key_type: type).key + end + end + + def self.user_for_key(key) + where(key: key).first.try(:user) + end + + private + + def generate_random_key + self.key = SecureRandom.hex(32) + end +end + +# == Schema Information +# +# Table name: unsubscribe_keys +# +# key :string(64) not null, primary key +# user_id :integer not null +# created_at :datetime +# updated_at :datetime +# unsubscribe_key_type :string +# topic_id :integer +# +# Indexes +# +# index_unsubscribe_keys_on_created_at (created_at) +# diff --git a/app/views/email/unsubscribe.html.erb b/app/views/email/unsubscribe.html.erb index 488442965..b2df6ad2c 100644 --- a/app/views/email/unsubscribe.html.erb +++ b/app/views/email/unsubscribe.html.erb @@ -1,28 +1,79 @@ -
+
+ <%- if @not_found || @different_user %> - <%- if @success %> -

<%= t :'unsubscribed.title' %>

-
- -

<%= t :'unsubscribed.description' %>

- -

<%= t :'unsubscribed.oops' %>

- - <%= form_tag(email_resubscribe_path(key: params[:key])) do %> - <%= submit_tag t(:'resubscribe.action'), class: 'btn btn-danger' %> - <% end %> + <%if @not_found%> +

<%= t "unsubscribe.not_found_description" %>

+ <%- else %> +

<%= t("unsubscribe.different_user_description").html_safe %>

+ <%= form_tag(session_path(id: current_user.username_lower), method: :delete) do %> + <%= hidden_field_tag(:return_url, @return_url) %> + <%= submit_tag t('unsubscribe.log_out'), class: 'btn btn-danger' %> + <%- end%> + <%- end %> <%- else %> -

<%= t :'unsubscribed.error' %>


- <%- if @different_user %> -

<%= t :'unsubscribed.different_user_description' %>

+

<%= t 'unsubscribe.title'%>

+
+ <%= form_tag(email_perform_unsubscribe_path(key: params[:key])) do %> + <%if @topic %> + <% if @watching_topic %> +

+ +

+ <% else %> +

+ +

+ <% end %> + + + <% if @watched_count %> +

+ +

+ <% end %> + <% end %> + + <% if @user.user_option.email_always && !SiteSetting.disable_mailing_list_mode %> +

+ +

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

+ +

+ <% end %> + <% end %> + +

+ +

+ +
+ <%= submit_tag t('unsubscribe.title'), class: 'btn btn-danger' %> <%- end %> - <%- if @not_found %> -

<%= t :'unsubscribed.not_found_description' %>

- <%- end %> - -

<%=raw(t :'unsubscribed.preferences_link') %>

<%- end %> diff --git a/app/views/email/unsubscribed.html.erb b/app/views/email/unsubscribed.html.erb new file mode 100644 index 000000000..9bdcc0d46 --- /dev/null +++ b/app/views/email/unsubscribed.html.erb @@ -0,0 +1,14 @@ +
+
+

<%=t "unsubscribed.title"%>

+
+

+ <%=t("unsubscribed.description", url: path("/my/prefrences")).html_safe %> +

+ + <% if @topic %> +

+ <%=t("unsubscribed.topic_description", link: render_topic_title(@topic)).html_safe%> +

+ <% end %> +
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index ba8863be8..070a3d58d 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -621,18 +621,21 @@ en: remove: "This topic is no longer a banner. It will no longer appear at the top of every page." unsubscribed: - title: 'Unsubscribed' - description: "You have been unsubscribed. We won't contact you again!" - oops: "In case you didn't mean to do this, click below." - error: "Error Unsubscribing" - preferences_link: "You can also unsubscribe from summary emails on your preferences page" - different_user_description: "You are currently logged in as a different user than the one who the summary was mailed to. Please log out and try again." - not_found_description: "Sorry, we couldn't unsubscribe you. It's possible the link in your email has expired." + title: "Unsubscribed!" + description: "You have been unsubscribed. If you wish 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." - resubscribe: - action: "Re-Subscribe" - title: "Re-Subscribed!" - description: "You have been re-subscribed." + unsubscribe: + title: "Unsubscribe" + stop_watching_topic: "Stop watching this topic, %{link}" + mute_topic: "Mute all notifications for this topic, %{link}" + unwatch_category: "Stop watching any topics in %{category}" + mailing_list_mode: "Disable mailing list mode" + disable_digest_emails: "Stop sending me digest 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 and try again." + not_found_description: "Sorry, we couldn't find the unsubscription. It's possible the link in your email has expired." + log_out: "Log Out" reports: visits: @@ -2164,19 +2167,19 @@ en: [Please review and fix them](%{base_url}/admin). unsubscribe_link: | - To stop receiving notifications for this particular topic, [click here](%{unsubscribe_url}). - - To unsubscribe from these emails, change your [user preferences](%{user_preferences_url}). + To unsubscribe from these emails, [click here](%{unsubscribe_url}). unsubscribe_link_and_mail: | - To stop receiving notifications for this particular topic, [click here](%{unsubscribe_url}). + To unsubscribe from these emails, [click here](%{unsubscribe_url}). - To unsubscribe from these emails, change your [user preferences](%{user_preferences_url}) or [click here](mailto:reply@%{hostname}?subject=unsubscribe) to unsubscribe via email. + Alternarively you may [click here](mailto:reply@%{hostname}?subject=unsubscribe) to unsubscribe via email. unsubscribe_mailing_list: | - You have mailing list mode enabled. To stop receiving notifications for this particular topic, [click here](%{unsubscribe_url}). + You are receiving these emails cause you have enabled mailing list mode. - To unsubscribe from these emails, change your [user preferences](%{user_preferences_url}) or [click here](mailto:reply@%{hostname}?subject=unsubscribe) to unsubscribe via email. + To unsubscribe from these emails, [click here](%{unsubscribe_url}). + + Alternatively you may [click here](mailto:reply@%{hostname}?subject=unsubscribe) to unsubscribe via email. subject_re: "Re: " subject_pm: "[PM] " diff --git a/config/routes.rb b/config/routes.rb index 92497616f..106949136 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -235,8 +235,10 @@ Discourse::Application.routes.draw do end # admin namespace get "email_preferences" => "email#preferences_redirect", :as => "email_preferences_redirect" + get "email/unsubscribe/:key" => "email#unsubscribe", as: "email_unsubscribe" - post "email/resubscribe/:key" => "email#resubscribe", as: "email_resubscribe" + get "email/unsubscribed" => "email#unsubscribed", as: "email_unsubscribed" + post "email/unsubscribe/:key" => "email#perform_unsubscribe", as: "email_perform_unsubscribe" resources :session, id: USERNAME_ROUTE_FORMAT, only: [:create, :destroy, :become] do get 'become' diff --git a/db/migrate/20160615024524_rename_digest_unsbscribe_keys.rb b/db/migrate/20160615024524_rename_digest_unsbscribe_keys.rb new file mode 100644 index 000000000..e296c9b69 --- /dev/null +++ b/db/migrate/20160615024524_rename_digest_unsbscribe_keys.rb @@ -0,0 +1,19 @@ +class RenameDigestUnsbscribeKeys < ActiveRecord::Migration + def up + rename_table :digest_unsubscribe_keys, :unsubscribe_keys + + add_column :unsubscribe_keys, :unsubscribe_key_type, :string + add_column :unsubscribe_keys, :topic_id, :int + add_column :unsubscribe_keys, :post_id, :int + + execute "UPDATE unsubscribe_keys SET unsubscribe_key_type = 'digest' WHERE unsubscribe_key_type IS NULL" + end + + def down + remove_column :unsubscribe_keys, :unsubscribe_key_type + remove_column :unsubscribe_keys, :topic_id + remove_column :unsubscribe_keys, :post_id + + rename_table :unsubscribe_keys, :digest_unsubscribe_keys + end +end diff --git a/spec/components/email/message_builder_spec.rb b/spec/components/email/message_builder_spec.rb index 9e0e7990f..e63e704e2 100644 --- a/spec/components/email/message_builder_spec.rb +++ b/spec/components/email/message_builder_spec.rb @@ -187,8 +187,8 @@ describe Email::MessageBuilder do expect(message_with_unsubscribe.header_args['List-Unsubscribe']).to be_present end - it "has the user preferences url in the body" do - expect(message_with_unsubscribe.body).to match(builder.template_args[:user_preferences_url]) + it "has the unsubscribe url in the body" do + expect(message_with_unsubscribe.body).to match('/t/1234/unsubscribe') end it "can add an unsubscribe via email link" do diff --git a/spec/controllers/email_controller_spec.rb b/spec/controllers/email_controller_spec.rb index 0376076b9..5e4c41056 100644 --- a/spec/controllers/email_controller_spec.rb +++ b/spec/controllers/email_controller_spec.rb @@ -19,117 +19,186 @@ describe EmailController do end - context '.resubscribe' do - - let(:user) { - user = Fabricate(:user) - user.user_option.update_columns(email_digests: false) - user - } - let(:key) { DigestUnsubscribeKey.create_key_for(user) } - - context 'with a valid key' do - before do - get :resubscribe, key: key - user.reload - end - - it 'subscribes the user' do - expect(user.user_option.email_digests).to eq(true) - end + context '.perform unsubscribe' do + it 'raises not found on invalid key' do + post :perform_unsubscribe, key: "123" + expect(response.status).to eq(404) end + it 'can fully unsubscribe' do + user = Fabricate(:user) + key = UnsubscribeKey.create_key_for(user, "all") + + user.user_option.update_columns(email_always: true, + email_digests: true, + email_direct: true, + email_private_messages: true) + + post :perform_unsubscribe, key: key, unsubscribe_all: "1" + expect(response.status).to eq(302) + + user.user_option.reload + + expect(user.user_option.email_always).to eq(false) + expect(user.user_option.email_digests).to eq(false) + expect(user.user_option.email_direct).to eq(false) + expect(user.user_option.email_private_messages).to eq(false) + + end + + it 'can disable mailing list' do + user = Fabricate(:user) + key = UnsubscribeKey.create_key_for(user, "all") + + user.user_option.update_columns(email_always: true) + + post :perform_unsubscribe, key: key, disable_mailing_list: "1" + expect(response.status).to eq(302) + + user.user_option.reload + + expect(user.user_option.email_always).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) + + post :perform_unsubscribe, key: key, disable_digest_emails: "1" + expect(response.status).to eq(302) + + user.user_option.reload + + expect(user.user_option.email_digests).to eq(false) + end + + it 'can unwatch topic' do + p = Fabricate(:post) + key = UnsubscribeKey.create_key_for(p.user, p) + + TopicUser.change(p.user_id, p.topic_id, notification_level: TopicUser.notification_levels[:watching]) + post :perform_unsubscribe, key: key, unwatch_topic: "1" + expect(response.status).to eq(302) + + expect(TopicUser.get(p.topic, p.user).notification_level).to eq(TopicUser.notification_levels[:tracking]) + end + + it 'can mute topic' do + p = Fabricate(:post) + key = UnsubscribeKey.create_key_for(p.user, p) + + TopicUser.change(p.user_id, p.topic_id, notification_level: TopicUser.notification_levels[:watching]) + post :perform_unsubscribe, key: key, mute_topic: "1" + expect(response.status).to eq(302) + + expect(TopicUser.get(p.topic, p.user).notification_level).to eq(TopicUser.notification_levels[:muted]) + end + + it 'can unwatch category' do + p = Fabricate(:post) + key = UnsubscribeKey.create_key_for(p.user, p) + + cu = CategoryUser.create!(user_id: p.user.id, + category_id: p.topic.category_id, + notification_level: CategoryUser.notification_levels[:watching]) + + post :perform_unsubscribe, key: key, unwatch_category: "1" + expect(response.status).to eq(302) + + expect(CategoryUser.find_by(id: cu.id)).to eq(nil) + end end context '.unsubscribe' do - let(:user) { + render_views + + it 'displays logo ut button if wrong user logged in' do + log_in_user Fabricate(:admin) user = Fabricate(:user) - user.user_option.update_columns(email_always: true, email_digests: true, email_direct: true, email_private_messages: true) - user - } - let(:key) { DigestUnsubscribeKey.create_key_for(user) } + key = UnsubscribeKey.create_key_for(user, "digest") - context 'from confirm unsubscribe email' do - before do - get :unsubscribe, key: key, from_all: true - user.reload - end + get :unsubscribe, key: key - it 'unsubscribes from all emails' do - options = user.user_option - expect(options.email_digests).to eq false - expect(options.email_direct).to eq false - expect(options.email_private_messages).to eq false - expect(options.email_always).to eq false - end + expect(response.body).to include(I18n.t("unsubscribe.log_out")) + expect(response.body).to include(I18n.t("unsubscribe.different_user_description")) end - context 'with a valid key' do - before do - get :unsubscribe, key: key - user.reload - end - - it 'unsubscribes the user' do - expect(user.user_option.email_digests).to eq(false) - end - - it "sets the appropriate instance variables" do - expect(assigns(:success)).to be_present - end + it 'displays not found if key is not found' do + get :unsubscribe, key: SecureRandom.hex + expect(response.body).to include(CGI.escapeHTML(I18n.t("unsubscribe.not_found_description"))) end - context "with an expired key or invalid key" do - before do - get :unsubscribe, key: 'watwatwat' - end + it 'correctly handles mailing list mode' do + + user = Fabricate(:user) + key = UnsubscribeKey.create_key_for(user, "digest") + + user.user_option.update_columns(email_always: true) + + get :unsubscribe, key: key + expect(response.body).to include(I18n.t("unsubscribe.mailing_list_mode")) + + SiteSetting.disable_mailing_list_mode = true + + get :unsubscribe, key: key + expect(response.body).not_to include(I18n.t("unsubscribe.mailing_list_mode")) + + user.user_option.update_columns(email_always: false) + SiteSetting.disable_mailing_list_mode = false + + get :unsubscribe, key: key + expect(response.body).not_to include(I18n.t("unsubscribe.mailing_list_mode")) - it "sets the appropriate instance variables" do - expect(assigns(:success)).to be_blank - end end - context 'when logged in as a different user' do - let!(:logged_in_user) { log_in(:coding_horror) } + it 'correctly handles digest unsubscribe' do - before do - get :unsubscribe, key: key - user.reload - end + user = Fabricate(:user) + user.user_option.update_columns(email_digests: false) + key = UnsubscribeKey.create_key_for(user, "digest") - it 'does not unsubscribe the user' do - expect(user.user_option.email_digests).to eq(true) - end + # because we are type digest we will always show digest and it will be selected + get :unsubscribe, key: key + expect(response.body).to include(I18n.t("unsubscribe.disable_digest_emails")) - it 'sets the appropriate instance variables' do - expect(assigns(:success)).to be_blank - expect(assigns(:different_user)).to be_present - end + source = Nokogiri::HTML::fragment(response.body) + expect(source.css("#disable_digest_emails")[0]["checked"]).to eq("checked") + + SiteSetting.disable_digest_emails = true + + get :unsubscribe, key: key + expect(response.body).not_to include(I18n.t("unsubscribe.disable_digest_emails")) + + SiteSetting.disable_digest_emails = false + key = UnsubscribeKey.create_key_for(user, "not_digest") + + get :unsubscribe, key: key + expect(response.body).to include(I18n.t("unsubscribe.disable_digest_emails")) end - context 'when logged in as the keyed user' do + 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]) - before do - log_in_user(user) - get :unsubscribe, key: DigestUnsubscribeKey.create_key_for(user) - user.reload - end - it 'unsubscribes the user' do - expect(user.user_option.email_digests).to eq(false) - end + key = UnsubscribeKey.create_key_for(user, post) + get :unsubscribe, key: key + expect(response.body).to include("unwatch_category") + + cu.destroy! + + get :unsubscribe, key: key + expect(response.body).not_to include("unwatch_category") - it 'sets the appropriate instance variables' do - expect(assigns(:success)).to be_present - end end - - it "sets not_found when the key didn't match anything" do - get :unsubscribe, key: 'asdfasdf' - expect(assigns(:not_found)).to eq(true) - end - end + + end diff --git a/spec/models/digest_unsubscribe_key_spec.rb b/spec/models/digest_unsubscribe_key_spec.rb deleted file mode 100644 index c9b9bcab5..000000000 --- a/spec/models/digest_unsubscribe_key_spec.rb +++ /dev/null @@ -1,31 +0,0 @@ -require 'rails_helper' -require_dependency 'digest_unsubscribe_key' - -describe DigestUnsubscribeKey do - - it { is_expected.to belong_to :user } - - describe 'key' do - - let(:user) { Fabricate(:user) } - let!(:key) { DigestUnsubscribeKey.create_key_for(user) } - - it 'has a temporary key' do - expect(key).to be_present - end - - describe '#user_for_key' do - - it 'can be used to find the user' do - expect(DigestUnsubscribeKey.user_for_key(key)).to eq(user) - end - - it 'returns nil with an invalid key' do - expect(DigestUnsubscribeKey.user_for_key('asdfasdf')).to be_blank - end - - end - - end - -end diff --git a/spec/models/unsubscribe_key_spec.rb b/spec/models/unsubscribe_key_spec.rb new file mode 100644 index 000000000..3ef9842ea --- /dev/null +++ b/spec/models/unsubscribe_key_spec.rb @@ -0,0 +1,44 @@ +require 'rails_helper' +require_dependency 'unsubscribe_key' + +describe UnsubscribeKey do + + describe 'post unsubscribe key' do + it 'can generate a correct url' do + post = Fabricate(:post) + url = post.unsubscribe_url(post.user) + + route = Rails.application.routes.recognize_path(url) + + key = UnsubscribeKey.find_by(key: route[:key]) + + expect(key.post_id).to eq(post.id) + expect(key.topic_id).to eq(post.topic_id) + expect(key.unsubscribe_key_type).to eq("topic") + end + end + + describe 'key' do + + let(:user) { Fabricate(:user) } + let!(:key) { UnsubscribeKey.create_key_for(user, "digest") } + + it 'has a temporary key' do + expect(key).to be_present + end + + describe '#user_for_key' do + + it 'can be used to find the user' do + expect(UnsubscribeKey.user_for_key(key)).to eq(user) + end + + it 'returns nil with an invalid key' do + expect(UnsubscribeKey.user_for_key('asdfasdf')).to be_blank + end + + end + + end + +end