Merge 6a800f7 into 90def2a

app/models/user.rb

@@ -46,6 +46,7 @@ class User < ApplicationRecord
   include User::LoginToken
   include User::OneTimePassword
   include User::Slug
+  include User::SpreadableAlerts
   include User::Survey
   include Rails.application.routes.url_helpers
   include LinkToHelper
@@ -180,8 +181,6 @@ class User < ApplicationRecord
 
   validate :email_and_name_are_valid
 
-  after_initialize :set_defaults
-
   after_update :update_pro_account
   after_update :reindex_referencing_models, :invalidate_cached_pages,
                unless: :no_xapian_reindex
@@ -273,29 +272,6 @@ def self.stay_logged_in_on_redirect?(user)
     user&.is_admin?
   end
 
-  # Used for default values of last_daily_track_email
-  def self.random_time_in_last_day
-    earliest_time = Time.zone.now - 1.day
-    latest_time = Time.zone.now
-    earliest_time + rand(latest_time - earliest_time).seconds
-  end
-
-  # Alters last_daily_track_email for every user, so alerts will be sent
-  # spread out fairly evenly throughout the day, balancing load on the
-  # server. This is intended to be called by hand from the Ruby console.  It
-  # will mean quite a few users may get more than one email alert the day you
-  # do it, so have a care and run it rarely.
-  #
-  # This SQL statement is useful for seeing how spread out users are at the moment:
-  # select extract(hour from last_daily_track_email) as h, count(*) from users group by extract(hour from last_daily_track_email) order by h;
-  def self.spread_alert_times_across_day
-    find_each do |user|
-      user.update!(last_daily_track_email: User.random_time_in_last_day)
-    end
-
-    nil # so doesn't print all users on console
-  end
-
   def self.record_bounce_for_email(email, message)
     user = User.find_user_by_email(email)
     return false if user.nil?
@@ -698,19 +674,6 @@ def cached_urls
 
   private
 
-  def set_defaults
-    return unless new_record?
-
-    # make alert emails go out at a random time for each new user, so
-    # overall they are spread out throughout the day.
-    self.last_daily_track_email = self.class.random_time_in_last_day
-
-    # Make daily summary emails go out at a random time for each new user
-    # too, if it's not already set
-    self.daily_summary_hour ||= self.class.random_time_in_last_day.hour
-    self.daily_summary_minute ||= self.class.random_time_in_last_day.min
-  end
-
   def email_and_name_are_valid
     if email != "" && !MySociety::Validate.is_valid_email(email)
       errors.add(:email, _("Please enter a valid email address"))

app/models/user/spreadable_alerts.rb

@@ -0,0 +1,51 @@
+# Handles spreading alerts over the day so that we have a more even server load.
+module User::SpreadableAlerts
+  extend ActiveSupport::Concern
+
+  included do
+    after_initialize :set_alert_times
+  end
+
+  class_methods do
+    # Used for default values of last_daily_track_email
+    def random_time_in_last_day
+      Time.zone.now - rand(24.hours).seconds
+    end
+
+    # Alters last_daily_track_email for every user, so alerts will be sent
+    # spread out fairly evenly throughout the day, balancing load on the server.
+    # This is intended to be called by hand from the Ruby console. It will mean
+    # quite a few users may get more than one email alert the day you do it, so
+    # have a care and run it rarely.
+    #
+    # This SQL statement is useful for seeing how spread out users are at the
+    # moment:
+    #
+    # SELECT extract(hour from last_daily_track_email) AS h, COUNT(*)
+    # FROM users
+    # GROUP BY extract(hour from last_daily_track_email)
+    # ORDER BY h;
+    def spread_alert_times_across_day
+      find_each do |user|
+        user.update!(last_daily_track_email: random_time_in_last_day)
+      end
+
+      nil # so doesn't print all users on console
+    end
+  end
+
+  private
+
+  def set_alert_times
+    return unless new_record?
+
+    # make alert emails go out at a random time for each new user, so
+    # overall they are spread out throughout the day.
+    self.last_daily_track_email = self.class.random_time_in_last_day
+
+    # Make daily summary emails go out at a random time for each new user
+    # too, if it's not already set
+    self.daily_summary_hour ||= self.class.random_time_in_last_day.hour
+    self.daily_summary_minute ||= self.class.random_time_in_last_day.min
+  end
+end

spec/models/user/spreadable_alerts.rb

@@ -0,0 +1,59 @@
+RSpec.shared_examples 'user/spreadable_alerts' do
+  describe '.random_time_in_last_day' do
+    subject { described_class.random_time_in_last_day }
+
+    it 'returns a time within the last day' do
+      expect(subject).to be >= Time.zone.now - 24.hours
+      expect(subject).to be <= Time.zone.now
+    end
+
+    it 'returns different times on subsequent calls' do
+      random_time = described_class.random_time_in_last_day
+      expect(subject).not_to eq(random_time)
+    end
+  end
+
+  describe '.spread_alert_times_across_day' do
+    # TODO
+  end
+
+  describe '#daily_summary_time' do
+    let(:user) do
+      FactoryBot.create(:user, daily_summary_hour: 7,
+                               daily_summary_minute: 56)
+    end
+
+    it "returns the hour and minute of the user's daily summary time" do
+      expected_hash = { hour: 7, min: 56 }
+      expect(user.daily_summary_time).to eq(expected_hash)
+    end
+  end
+
+  describe "setting daily_summary_time on new users" do
+    let(:user) { FactoryBot.create(:user) }
+    let(:expected_time) { Time.zone.now.change(hour: 7, min: 57) }
+
+    before do
+      allow(User).
+        to receive(:random_time_in_last_day).and_return(expected_time)
+    end
+
+    it "sets a random hour and minute on initialization" do
+      expect(user.daily_summary_hour).to eq(7)
+      expect(user.daily_summary_minute).to eq(57)
+    end
+
+    it "doesn't override the hour and minute if they're already set" do
+      user = FactoryBot.create(:user, daily_summary_hour: 9,
+                                      daily_summary_minute: 15)
+      expect(user.daily_summary_hour).to eq(9)
+      expect(user.daily_summary_minute).to eq(15)
+    end
+
+    it "doesn't change the the hour and minute once they're set" do
+      user.save!
+      expect(user.daily_summary_hour).to eq(7)
+      expect(user.daily_summary_minute).to eq(57)
+    end
+  end
+end

spec/models/user_spec.rb

@@ -41,8 +41,11 @@
 
 require 'spec_helper'
 
+require 'models/user/spreadable_alerts'
+
 RSpec.describe User do
   it_behaves_like 'PhaseCounts'
+  it_behaves_like 'user/spreadable_alerts'
 end
 
 RSpec.describe User, "making up the URL name" do
@@ -1845,46 +1848,6 @@ def create_user(options = {})
     end
   end
 
-  describe '#daily_summary_time' do
-    let(:user) do
-      FactoryBot.create(:user, daily_summary_hour: 7,
-                               daily_summary_minute: 56)
-    end
-
-    it "returns the hour and minute of the user's daily summary time" do
-      expected_hash = { hour: 7, min: 56 }
-      expect(user.daily_summary_time).to eq(expected_hash)
-    end
-  end
-
-  describe "setting daily_summary_time on new users" do
-    let(:user) { FactoryBot.create(:user) }
-    let(:expected_time) { Time.zone.now.change(hour: 7, min: 57) }
-
-    before do
-      allow(User).
-        to receive(:random_time_in_last_day).and_return(expected_time)
-    end
-
-    it "sets a random hour and minute on initialization" do
-      expect(user.daily_summary_hour).to eq(7)
-      expect(user.daily_summary_minute).to eq(57)
-    end
-
-    it "doesn't override the hour and minute if they're already set" do
-      user = FactoryBot.create(:user, daily_summary_hour: 9,
-                                      daily_summary_minute: 15)
-      expect(user.daily_summary_hour).to eq(9)
-      expect(user.daily_summary_minute).to eq(15)
-    end
-
-    it "doesn't change the the hour and minute once they're set" do
-      user.save!
-      expect(user.daily_summary_hour).to eq(7)
-      expect(user.daily_summary_minute).to eq(57)
-    end
-  end
-
   describe '#notification_frequency' do
     context 'when the user has :notifications' do
       let(:user) { FactoryBot.create(:user) }