From f2dd35ab08f4bfcb7a5b77cfee2f96a647f8488e Mon Sep 17 00:00:00 2001
From: Robin Ward <robin.ward@gmail.com>
Date: Tue, 15 Jul 2014 17:19:45 -0400
Subject: [PATCH] Improve the unsubscribe to digest experience. Give a link in
 case it fails, provide a different message if you are logged in as a
 different user, increase expiry to 2 months from 1 week.

---
 app/controllers/email_controller.rb       | 17 +++++++++++------
 app/models/user.rb                        |  2 +-
 app/views/email/resubscribe.html.erb      |  3 +--
 app/views/email/unsubscribe.html.erb      | 19 ++++++++++++++-----
 config/locales/server.en.yml              |  4 +++-
 spec/controllers/email_controller_spec.rb | 23 +++++++++++++++++------
 6 files changed, 47 insertions(+), 21 deletions(-)

diff --git a/app/controllers/email_controller.rb b/app/controllers/email_controller.rb
index 0845570229..7fe28798df 100644
--- a/app/controllers/email_controller.rb
+++ b/app/controllers/email_controller.rb
@@ -13,13 +13,18 @@ class EmailController < ApplicationController
     @user = User.find_by_temporary_key(params[:key])
 
     # Don't allow the use of a key while logged in as a different user
-    @user = nil if current_user.present? && (@user != current_user)
-
-    if @user.present?
-      @user.update_column(:email_digests, false)
-    else
-      @not_found = true
+    if current_user.present? && (@user != current_user)
+      @different_user = true
+      return
     end
+
+    if @user.blank?
+      @not_found = true
+      return
+    end
+
+    @user.update_column(:email_digests, false)
+    @success = true
   end
 
   def resubscribe
diff --git a/app/models/user.rb b/app/models/user.rb
index 4b8e07673b..574152f564 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -185,7 +185,7 @@ class User < ActiveRecord::Base
   # Use a temporary key to find this user, store it in redis with an expiry
   def temporary_key
     key = SecureRandom.hex(32)
-    $redis.setex "temporary_key:#{key}", 1.week, id.to_s
+    $redis.setex "temporary_key:#{key}", 2.months, id.to_s
     key
   end
 
diff --git a/app/views/email/resubscribe.html.erb b/app/views/email/resubscribe.html.erb
index f83d460de7..1cef86a7f0 100644
--- a/app/views/email/resubscribe.html.erb
+++ b/app/views/email/resubscribe.html.erb
@@ -1,7 +1,6 @@
 <div class='container'>
-
   <h2><%= t :'resubscribe.title' %></h2>
+  <br/>
 
   <p><%= t :'resubscribe.description' %></p>
-
 </div>
diff --git a/app/views/email/unsubscribe.html.erb b/app/views/email/unsubscribe.html.erb
index e15f6032a1..488442965f 100644
--- a/app/views/email/unsubscribe.html.erb
+++ b/app/views/email/unsubscribe.html.erb
@@ -1,7 +1,8 @@
 <div class='container'>
 
-  <%- unless @not_found %>
+  <%- if @success %>
     <h2><%= t :'unsubscribed.title' %></h2>
+    <br/>
 
     <p><%= t :'unsubscribed.description' %></p>
 
@@ -11,10 +12,18 @@
       <%= submit_tag t(:'resubscribe.action'), class: 'btn btn-danger' %>
     <% end %>
   <%- else %>
-    <h2><%= t :'unsubscribed.not_found' %></h2>
-    <p><%= t :'unsubscribed.not_found_description' %></p>
+    <h2><%= t :'unsubscribed.error' %></h2>
+    <br/>
+    <%- if @different_user %>
+      <p><%= t :'unsubscribed.different_user_description' %></p>
+    <%- end %>
+
+    <%- if @not_found %>
+      <p><%= t :'unsubscribed.not_found_description' %></p>
+    <%- end %>
+
+    <p><%=raw(t :'unsubscribed.preferences_link') %></p>
+
   <%- end %>
 
-
-
 </div>
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 5330311af2..f9472290fa 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -460,7 +460,9 @@ en:
     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."
-    not_found: "Error Unsubscribing"
+    error: "Error Unsubscribing"
+    preferences_link: "You can also unsubscribe from digest emails on your <a href='/my/preferences'>preferences page</a>"
+    different_user_description: "You are currently logged in as a different user than the one who the digest 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."
 
   resubscribe:
diff --git a/spec/controllers/email_controller_spec.rb b/spec/controllers/email_controller_spec.rb
index 0d1d8d6b34..e6fb69bdaf 100644
--- a/spec/controllers/email_controller_spec.rb
+++ b/spec/controllers/email_controller_spec.rb
@@ -50,8 +50,18 @@ describe EmailController do
         user.email_digests.should be_false
       end
 
-      it "does not set the not_found instance variable" do
-        assigns(:not_found).should be_blank
+      it "sets the appropriate instance variables" do
+        assigns(:success).should be_present
+      end
+    end
+
+    context "with an expired key or invalid key" do
+      before do
+        get :unsubscribe, key: 'watwatwat'
+      end
+
+      it "sets the appropriate instance variables" do
+        assigns(:success).should be_blank
       end
     end
 
@@ -67,8 +77,9 @@ describe EmailController do
         user.email_digests.should be_true
       end
 
-      it 'sets not found' do
-        assigns(:not_found).should be_true
+      it 'sets the appropriate instance variables' do
+        assigns(:success).should be_blank
+        assigns(:different_user).should be_present
       end
     end
 
@@ -84,8 +95,8 @@ describe EmailController do
         user.email_digests.should be_false
       end
 
-      it "doesn't set not found" do
-        assigns(:not_found).should be_blank
+      it 'sets the appropriate instance variables' do
+        assigns(:success).should be_present
       end
     end