Fix initial implementation to actually render the unsubscribe page

Signed-off-by: Rémy Coutable <remy@rymai.me>

4 files changed, 54 insertions, 13 deletions

diff --git a/app/controllers/sent_notifications_controller.rb b/app/controllers/sent_notifications_controller.rb
index c4abc597cf1..3085ff33aba 100644
--- a/app/controllers/sent_notifications_controller.rb
+++ b/app/controllers/sent_notifications_controller.rb
@@ -2,12 +2,15 @@ class SentNotificationsController < ApplicationController
   skip_before_action :authenticate_user!
 
   def unsubscribe
-    return redirect_to new_user_session_path unless current_user || params[:force]
-
     @sent_notification = SentNotification.for(params[:id])
 
     return render_404 unless @sent_notification && @sent_notification.unsubscribable?
+    return unsubscribe_and_redirect if current_user || params[:force]
+  end
+
+  private
 
+  def unsubscribe_and_redirect
     noteable = @sent_notification.noteable
     noteable.unsubscribe(@sent_notification.recipient)
 
diff --git a/app/views/sent_notifications/unsubscribe.html.haml b/app/views/sent_notifications/unsubscribe.html.haml
index 568d2ac3af1..9baadf8a0c5 100644
--- a/app/views/sent_notifications/unsubscribe.html.haml
+++ b/app/views/sent_notifications/unsubscribe.html.haml
@@ -1,4 +1,15 @@
-%h3.page-title
-  Are you sure you want to unsubscribe from this thread?
+- noteable = @sent_notification.noteable
+- noteable_type = @sent_notification.noteable_type.humanize(capitalize: false)
+- noteable_text = %(#{noteable_type} "#{noteable.title}" (#{noteable.to_reference}))
+- title = "Unsubscribe from #{noteable_text}"
 
-  = link_to "Unsubscribe", unsubscribe_sent_notification_path(@sent_notification, force: true), class: 'btn btn-primary wide'
+- page_title title
+
+%h3.page-title= title
+
+%p= "Are you sure you want to unsubscribe from #{noteable_text}?"
+
+%p
+  = link_to 'Unsubscribe', unsubscribe_sent_notification_path(@sent_notification, force: true),
+            class: 'btn btn-primary append-right-10'
+  = link_to 'Cancel', new_user_session_path, class: 'btn append-right-10'
diff --git a/spec/controllers/sent_notifications_controller_spec.rb b/spec/controllers/sent_notifications_controller_spec.rb
index 4e75372ffb2..191e290a118 100644
--- a/spec/controllers/sent_notifications_controller_spec.rb
+++ b/spec/controllers/sent_notifications_controller_spec.rb
@@ -41,7 +41,7 @@ describe SentNotificationsController, type: :controller do
         end
 
         it 'redirects to the login page' do
-          expect(response).to redirect_to(new_user_session_path)
+          expect(response).to render_template :unsubscribe
         end
       end
     end
@@ -83,19 +83,25 @@ describe SentNotificationsController, type: :controller do
       end
 
       context 'when the force param is not passed' do
+        let(:merge_request) do
+          create(:merge_request, source_project: project, author: user) do |merge_request|
+            merge_request.subscriptions.create(user: user, subscribed: true)
+          end
+        end
+        let(:sent_notification) { create(:sent_notification, noteable: merge_request, recipient: user) }
         before { get(:unsubscribe, id: sent_notification.reply_key) }
 
         it 'unsubscribes the user' do
-          expect(issue.subscribed?(user)).to be_falsey
+          expect(merge_request.subscribed?(user)).to be_falsey
         end
 
         it 'sets the flash message' do
           expect(controller).to set_flash[:notice].to(/unsubscribed/).now
         end
 
-        it 'redirects to the issue page' do
+        it 'redirects to the merge request page' do
           expect(response).
-            to redirect_to(namespace_project_issue_path(project.namespace, project, issue))
+            to redirect_to(namespace_project_merge_request_path(project.namespace, project, merge_request))
         end
       end
     end
diff --git a/spec/features/unsubscribe_links_spec.rb b/spec/features/unsubscribe_links_spec.rb
index eafad1a726d..3802f5f1335 100644
--- a/spec/features/unsubscribe_links_spec.rb
+++ b/spec/features/unsubscribe_links_spec.rb
@@ -19,11 +19,32 @@ describe 'Unsubscribe links', feature: true do
   end
 
   context 'when logged out' do
-    it 'redirects to the login page when visiting the link from the body' do
-      visit body_link
+    context 'when visiting the link from the body' do
+      it 'shows the unsubscribe confirmation page and redirects to root path when confirming' do
+        visit body_link
+
+        expect(current_path).to eq unsubscribe_sent_notification_path(SentNotification.last)
+        expect(page).to have_text(%(Unsubscribe from issue "#{issue.title}" (#{issue.to_reference})))
+        expect(page).to have_text(%(Are you sure you want to unsubscribe from issue "#{issue.title}" (#{issue.to_reference})?))
+        expect(issue.subscribed?(recipient)).to be_truthy
+
+        click_link 'Unsubscribe'
+
+        expect(issue.subscribed?(recipient)).to be_falsey
+        expect(current_path).to eq new_user_session_path
+      end
+
+      it 'shows the unsubscribe confirmation page and redirects to root path when canceling' do
+        visit body_link
+
+        expect(current_path).to eq unsubscribe_sent_notification_path(SentNotification.last)
+        expect(issue.subscribed?(recipient)).to be_truthy
+
+        click_link 'Cancel'
 
-      expect(current_path).to eq new_user_session_path
-      expect(issue.subscribed?(recipient)).to be_truthy
+        expect(issue.subscribed?(recipient)).to be_truthy
+        expect(current_path).to eq new_user_session_path
+      end
     end
 
     it 'unsubscribes from the issue when visiting the link from the header' do