From 5bea42412e41b1121af04dbe6ef5c9bed22219b8 Mon Sep 17 00:00:00 2001
From: Matt Jankowski <mjankowski@thoughtbot.com>
Date: Mon, 8 May 2017 18:45:02 -0400
Subject: [PATCH] Services specs for subscribe and unsubscribe (#2928)

* Add specs for unsubscribe service

* Fix non existent methods in unsubscribe service

* Clean up status handling in subscribe service
---
 app/services/subscribe_service.rb         | 16 +++++-----
 app/services/unsubscribe_service.rb       |  4 +--
 spec/services/subscribe_service_spec.rb   |  5 +++
 spec/services/unsubscribe_service_spec.rb | 37 +++++++++++++++++++++++
 4 files changed, 52 insertions(+), 10 deletions(-)
 create mode 100644 spec/services/unsubscribe_service_spec.rb

diff --git a/app/services/subscribe_service.rb b/app/services/subscribe_service.rb
index 138718f14..1e7984a7f 100644
--- a/app/services/subscribe_service.rb
+++ b/app/services/subscribe_service.rb
@@ -8,28 +8,28 @@ class SubscribeService < BaseService
     response     = subscription.subscribe
 
     if response_failed_permanently?(response)
-      # An error in the 4xx range (except for 429, which is rate limiting)
-      # means we're not allowed to subscribe. Fail and move on
+      # We're not allowed to subscribe. Fail and move on.
       account.secret = ''
       account.save!
     elsif response_successful?(response)
-      # Anything in the 2xx range means the subscription will be confirmed
-      # asynchronously, we've done what we needed to do
+      # The subscription will be confirmed asynchronously.
       account.save!
     else
-      # What's left is the 5xx range and 429, which means we need to retry
-      # at a later time. Fail loudly!
+      # The response was either a 429 rate limit, or a 5xx error.
+      # We need to retry at a later time. Fail loudly!
       raise "Subscription attempt failed for #{account.acct} (#{account.hub_url}): HTTP #{response.code}"
     end
   end
 
   private
 
+  # Any response in the 3xx or 4xx range, except for 429 (rate limit)
   def response_failed_permanently?(response)
-    response.code > 299 && response.code < 500 && response.code != 429
+    (response.status.redirect? || response.status.client_error?) && !response.status.too_many_requests?
   end
 
+  # Any response in the 2xx range
   def response_successful?(response)
-    response.code > 199 && response.code < 300
+    response.status.success?
   end
 end
diff --git a/app/services/unsubscribe_service.rb b/app/services/unsubscribe_service.rb
index 1a951d1b4..6db8dbdc4 100644
--- a/app/services/unsubscribe_service.rb
+++ b/app/services/unsubscribe_service.rb
@@ -5,8 +5,8 @@ class UnsubscribeService < BaseService
     subscription = account.subscription(api_subscription_url(account.id))
     response = subscription.unsubscribe
 
-    unless response.successful?
-      Rails.logger.debug "PuSH unsubscribe for #{account.acct} failed: #{response.message}"
+    unless response.status.success?
+      Rails.logger.debug "PuSH unsubscribe for #{account.acct} failed: #{response.status}"
     end
 
     account.secret = ''
diff --git a/spec/services/subscribe_service_spec.rb b/spec/services/subscribe_service_spec.rb
index 01cdb29c0..5db91ad99 100644
--- a/spec/services/subscribe_service_spec.rb
+++ b/spec/services/subscribe_service_spec.rb
@@ -35,4 +35,9 @@ RSpec.describe SubscribeService do
     stub_request(:post, 'http://hub.example.com/').to_return(status: 503)
     expect { subject.call(account) }.to raise_error(/Subscription attempt failed/)
   end
+
+  it 'fails loudly if rate limited' do
+    stub_request(:post, 'http://hub.example.com/').to_return(status: 429)
+    expect { subject.call(account) }.to raise_error(/Subscription attempt failed/)
+  end
 end
diff --git a/spec/services/unsubscribe_service_spec.rb b/spec/services/unsubscribe_service_spec.rb
new file mode 100644
index 000000000..c81772037
--- /dev/null
+++ b/spec/services/unsubscribe_service_spec.rb
@@ -0,0 +1,37 @@
+require 'rails_helper'
+
+RSpec.describe UnsubscribeService do
+  let(:account) { Fabricate(:account, username: 'bob', domain: 'example.com', hub_url: 'http://hub.example.com') }
+  subject { UnsubscribeService.new }
+
+  it 'removes the secret and resets expiration on account' do
+    stub_request(:post, 'http://hub.example.com/').to_return(status: 204)
+    subject.call(account)
+    account.reload
+
+    expect(account.secret).to be_blank
+    expect(account.subscription_expires_at).to be_blank
+  end
+
+  it 'logs error on subscription failure' do
+    logger = stub_logger
+    stub_request(:post, 'http://hub.example.com/').to_return(status: 404)
+    subject.call(account)
+
+    expect(logger).to have_received(:debug).with(/unsubscribe for bob@example.com failed/)
+  end
+
+  it 'logs error on connection failure' do
+    logger = stub_logger
+    stub_request(:post, 'http://hub.example.com/').to_raise(HTTP::Error)
+    subject.call(account)
+
+    expect(logger).to have_received(:debug).with(/PuSH subscription request for bob@example.com could not be made due to HTTP or SSL error/)
+  end
+
+  def stub_logger
+    double(debug: nil).tap do |logger|
+      allow(Rails).to receive(:logger).and_return(logger)
+    end
+  end
+end