From 5393dbf4a208b2893ef1f0b08259b14580f032bf Mon Sep 17 00:00:00 2001 From: Matt Jankowski Date: Fri, 5 May 2017 14:56:00 -0400 Subject: [PATCH] Misc spec coverage improvements (#2821) * Dont use raise_error by itself (avoids warning) * Add coverage for AccountFilter * Improve coverage and refactor for Subscription#lease_seconds * Improve coverage and refactor for NotificationMailer * Simplify assignment of min/max threshold on subscription --- app/mailers/notification_mailer.rb | 20 +++++++--- app/models/subscription.rb | 24 +++++++++--- spec/mailers/notification_mailer_spec.rb | 30 +++++++++++++++ spec/models/account_filter_spec.rb | 18 +++++++-- spec/models/subscription_spec.rb | 48 ++++++++++++++++++++++++ spec/rails_helper.rb | 1 + spec/services/subscribe_service_spec.rb | 2 +- 7 files changed, 128 insertions(+), 15 deletions(-) diff --git a/app/mailers/notification_mailer.rb b/app/mailers/notification_mailer.rb index 2bfc777211..f308c403bf 100644 --- a/app/mailers/notification_mailer.rb +++ b/app/mailers/notification_mailer.rb @@ -7,7 +7,7 @@ class NotificationMailer < ApplicationMailer @me = recipient @status = notification.target_status - I18n.with_locale(@me.user.locale || I18n.default_locale) do + locale_for_account(@me) do mail to: @me.user.email, subject: I18n.t('notification_mailer.mention.subject', name: @status.account.acct) end end @@ -16,7 +16,7 @@ class NotificationMailer < ApplicationMailer @me = recipient @account = notification.from_account - I18n.with_locale(@me.user.locale || I18n.default_locale) do + locale_for_account(@me) do mail to: @me.user.email, subject: I18n.t('notification_mailer.follow.subject', name: @account.acct) end end @@ -26,7 +26,7 @@ class NotificationMailer < ApplicationMailer @account = notification.from_account @status = notification.target_status - I18n.with_locale(@me.user.locale || I18n.default_locale) do + locale_for_account(@me) do mail to: @me.user.email, subject: I18n.t('notification_mailer.favourite.subject', name: @account.acct) end end @@ -36,7 +36,7 @@ class NotificationMailer < ApplicationMailer @account = notification.from_account @status = notification.target_status - I18n.with_locale(@me.user.locale || I18n.default_locale) do + locale_for_account(@me) do mail to: @me.user.email, subject: I18n.t('notification_mailer.reblog.subject', name: @account.acct) end end @@ -45,7 +45,7 @@ class NotificationMailer < ApplicationMailer @me = recipient @account = notification.from_account - I18n.with_locale(@me.user.locale || I18n.default_locale) do + locale_for_account(@me) do mail to: @me.user.email, subject: I18n.t('notification_mailer.follow_request.subject', name: @account.acct) end end @@ -58,7 +58,7 @@ class NotificationMailer < ApplicationMailer return if @notifications.empty? - I18n.with_locale(@me.user.locale || I18n.default_locale) do + locale_for_account(@me) do mail to: @me.user.email, subject: I18n.t( :subject, @@ -67,4 +67,12 @@ class NotificationMailer < ApplicationMailer ) end end + + private + + def locale_for_account(account) + I18n.with_locale(account.user.locale || I18n.default_locale) do + yield + end + end end diff --git a/app/models/subscription.rb b/app/models/subscription.rb index 6046ae4c2a..35a228df07 100644 --- a/app/models/subscription.rb +++ b/app/models/subscription.rb @@ -1,4 +1,5 @@ # frozen_string_literal: true + # == Schema Information # # Table name: subscriptions @@ -15,18 +16,20 @@ # class Subscription < ApplicationRecord - MIN_EXPIRATION = 3600 * 24 * 7 - MAX_EXPIRATION = 3600 * 24 * 30 + MIN_EXPIRATION = 7.days.seconds.to_i + MAX_EXPIRATION = 30.days.seconds.to_i belongs_to :account, required: true validates :callback_url, presence: true validates :callback_url, uniqueness: { scope: :account_id } - scope :active, -> { where(confirmed: true).where('expires_at > ?', Time.now.utc) } + scope :confirmed, -> { where(confirmed: true) } + scope :future_expiration, -> { where(arel_table[:expires_at].gt(Time.now.utc)) } + scope :active, -> { confirmed.future_expiration } - def lease_seconds=(str) - self.expires_at = Time.now.utc + [[MIN_EXPIRATION, str.to_i].max, MAX_EXPIRATION].min.seconds + def lease_seconds=(value) + self.expires_at = future_expiration(value) end def lease_seconds @@ -41,6 +44,17 @@ class Subscription < ApplicationRecord private + def future_expiration(value) + Time.now.utc + future_offset(value).seconds + end + + def future_offset(seconds) + [ + [MIN_EXPIRATION, seconds.to_i].max, + MAX_EXPIRATION, + ].min + end + def set_min_expiration self.lease_seconds = 0 unless expires_at end diff --git a/spec/mailers/notification_mailer_spec.rb b/spec/mailers/notification_mailer_spec.rb index 3beaebeb1c..faa3197c83 100644 --- a/spec/mailers/notification_mailer_spec.rb +++ b/spec/mailers/notification_mailer_spec.rb @@ -62,4 +62,34 @@ RSpec.describe NotificationMailer, type: :mailer do end end + describe 'follow_request' do + let(:follow_request) { Fabricate(:follow_request, account: sender, target_account: receiver.account) } + let(:mail) { NotificationMailer.follow_request(receiver.account, Notification.create!(account: receiver.account, activity: follow_request)) } + + it 'renders the headers' do + expect(mail.subject).to eq('Pending follower: bob') + expect(mail.to).to eq([receiver.email]) + end + + it 'renders the body' do + expect(mail.body.encoded).to match("bob has requested to follow you") + end + end + + describe 'digest' do + before do + mention = Fabricate(:mention, account: receiver.account) + Fabricate(:notification, account: receiver.account, activity: mention) + end + let(:mail) { NotificationMailer.digest(receiver.account, since: 5.days.ago) } + + it 'renders the headers' do + expect(mail.subject).to match('notification since your last') + expect(mail.to).to eq([receiver.email]) + end + + it 'renders the body' do + expect(mail.body.encoded).to match('brief summary') + end + end end diff --git a/spec/models/account_filter_spec.rb b/spec/models/account_filter_spec.rb index 1599c5ae8b..52b4c48935 100644 --- a/spec/models/account_filter_spec.rb +++ b/spec/models/account_filter_spec.rb @@ -3,7 +3,7 @@ require 'rails_helper' describe AccountFilter do describe 'with empty params' do it 'defaults to alphabetic account list' do - filter = AccountFilter.new({}) + filter = described_class.new({}) expect(filter.results).to eq Account.alphabetic end @@ -11,7 +11,7 @@ describe AccountFilter do describe 'with invalid params' do it 'raises with key error' do - filter = AccountFilter.new(wrong: true) + filter = described_class.new(wrong: true) expect { filter.results }.to raise_error(/wrong/) end @@ -19,7 +19,7 @@ describe AccountFilter do describe 'with valid params' do it 'combines filters on Account' do - filter = AccountFilter.new(by_domain: 'test.com', silenced: true) + filter = described_class.new(by_domain: 'test.com', silenced: true) allow(Account).to receive(:where).and_return(Account.none) allow(Account).to receive(:silenced).and_return(Account.none) @@ -27,5 +27,17 @@ describe AccountFilter do expect(Account).to have_received(:where).with(domain: 'test.com') expect(Account).to have_received(:silenced) end + + describe 'that call account methods' do + %i(local remote silenced recent).each do |option| + it "delegates the #{option} option" do + allow(Account).to receive(option).and_return(Account.none) + filter = described_class.new({ option => true }) + filter.results + + expect(Account).to have_received(option) + end + end + end end end diff --git a/spec/models/subscription_spec.rb b/spec/models/subscription_spec.rb index 11fd8fadc8..84096e6ae8 100644 --- a/spec/models/subscription_spec.rb +++ b/spec/models/subscription_spec.rb @@ -16,4 +16,52 @@ RSpec.describe Subscription, type: :model do expect(subject.expired?).to be false end end + + describe 'lease_seconds' do + it 'returns the time remaing until expiration' do + datetime = 1.day.from_now + subscription = Subscription.new(expires_at: datetime) + travel_to(datetime - 12.hours) do + expect(subscription.lease_seconds).to eq(12.hours) + end + end + end + + describe 'lease_seconds=' do + it 'sets expires_at to min expiration when small value is provided' do + subscription = Subscription.new + datetime = 1.day.from_now + too_low = Subscription::MIN_EXPIRATION - 1000 + travel_to(datetime) do + subscription.lease_seconds = too_low + end + + expected = datetime + Subscription::MIN_EXPIRATION.seconds + expect(subscription.expires_at).to be_within(1.0).of(expected) + end + + it 'sets expires_at to value when valid value is provided' do + subscription = Subscription.new + datetime = 1.day.from_now + valid = Subscription::MIN_EXPIRATION + 1000 + travel_to(datetime) do + subscription.lease_seconds = valid + end + + expected = datetime + valid.seconds + expect(subscription.expires_at).to be_within(1.0).of(expected) + end + + it 'sets expires_at to max expiration when large value is provided' do + subscription = Subscription.new + datetime = 1.day.from_now + too_high = Subscription::MAX_EXPIRATION + 1000 + travel_to(datetime) do + subscription.lease_seconds = too_high + end + + expected = datetime + Subscription::MAX_EXPIRATION.seconds + expect(subscription.expires_at).to be_within(1.0).of(expected) + end + end end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index 4ddc6d032d..c440574127 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -25,6 +25,7 @@ RSpec.configure do |config| config.include Devise::Test::ControllerHelpers, type: :controller config.include Devise::Test::ControllerHelpers, type: :view config.include Paperclip::Shoulda::Matchers + config.include ActiveSupport::Testing::TimeHelpers config.before :each, type: :feature do https = ENV['LOCAL_HTTPS'] == 'true' diff --git a/spec/services/subscribe_service_spec.rb b/spec/services/subscribe_service_spec.rb index 8cf0100c63..01cdb29c06 100644 --- a/spec/services/subscribe_service_spec.rb +++ b/spec/services/subscribe_service_spec.rb @@ -33,6 +33,6 @@ RSpec.describe SubscribeService do it 'fails loudly if PuSH hub is unavailable' do stub_request(:post, 'http://hub.example.com/').to_return(status: 503) - expect { subject.call(account) }.to raise_error + expect { subject.call(account) }.to raise_error(/Subscription attempt failed/) end end