FEATURE: send max 200 emails every minute for bulk invites (#7875)

DEV: deprecate `invite.via_email` in favor of `invite.emailed_status`

This commit adds a new column `emailed_status` in `invites` table for
 tracking email sending status.
 0 - not required
 1 - pending
 2 - bulk pending
 3 - sending
 4 - sent

For normal email invites, invite record is created with emailed_status
 set to 'pending'.

When bulk invites are sent invite record is created with emailed_status
 set to 'bulk pending'.

For invites that generates link, invite record is created with
 emailed_status set to 'not required'.

When invite email is in queue emailed_status is updated to 'sending'

Once the email is sent via `InviteEmail` job the invite emailed_status
 is updated to 'sent'.

app/jobs/regular/bulk_invite.rb

@@ -23,8 +23,13 @@ module Jobs
       @current_user = User.find_by(id: args[:current_user_id])
       raise Discourse::InvalidParameters.new(:current_user_id) unless @current_user
       @guardian = Guardian.new(@current_user)
+      @total_invites = invites.length
 
       process_invites(invites)
+
+      if @total_invites > Invite::BULK_INVITE_EMAIL_LIMIT
+        Jobs.enqueue(:process_bulk_invite_emails)
+      end
     ensure
       notify_user
     end
@@ -104,7 +109,15 @@ module Jobs
             end
           end
         else
-          Invite.invite_by_email(email, @current_user, topic, groups.map(&:id))
+          if @total_invites > Invite::BULK_INVITE_EMAIL_LIMIT
+            invite = Invite.create_invite_by_email(email, @current_user,
+              topic: topic,
+              group_ids: groups.map(&:id),
+              emailed_status: Invite.emailed_status_types[:bulk_pending]
+            )
+          else
+            Invite.invite_by_email(email, @current_user, topic, groups.map(&:id))
+          end
         end
       rescue => e
         save_log "Error inviting '#{email}' -- #{Rails::Html::FullSanitizer.new.sanitize(e.message)}"

app/jobs/regular/invite_email.rb

@@ -15,8 +15,10 @@ module Jobs
 
       message = InviteMailer.send_invite(invite)
       Email::Sender.new(message, :invite).send
+
+      if invite.emailed_status != Invite.emailed_status_types[:not_required]
+        invite.update_column(:emailed_status, Invite.emailed_status_types[:sent])
+      end
     end
-
   end
-
 end

app/jobs/regular/process_bulk_invite_emails.rb

@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+require_dependency 'email/sender'
+
+module Jobs
+
+  class ProcessBulkInviteEmails < Jobs::Base
+
+    def execute(args)
+      pending_invite_ids = Invite.where(emailed_status: Invite.emailed_status_types[:bulk_pending]).limit(Invite::BULK_INVITE_EMAIL_LIMIT).pluck(:id)
+
+      if pending_invite_ids.length > 0
+        Invite.where(id: pending_invite_ids).update_all(emailed_status: Invite.emailed_status_types[:sending])
+        pending_invite_ids.each do |invite_id|
+          Jobs.enqueue(:invite_email, invite_id: invite_id)
+        end
+        Jobs.enqueue_in(1.minute, :process_bulk_invite_emails)
+      end
+    end
+  end
+end

app/models/invite.rb

@@ -3,10 +3,16 @@
 require_dependency 'rate_limiter'
 
 class Invite < ActiveRecord::Base
+  self.ignored_columns = %w{
+    via_email
+  }
+
   class UserExists < StandardError; end
   include RateLimiter::OnCreateRecord
   include Trashable
 
+  BULK_INVITE_EMAIL_LIMIT = 200
+
   rate_limit :limit_invites_per_day
 
   belongs_to :user
@@ -31,6 +37,10 @@ class Invite < ActiveRecord::Base
   validate :user_doesnt_already_exist
   attr_accessor :email_already_exists
 
+  def self.emailed_status_types
+    @emailed_status_types ||= Enum.new(not_required: 0, pending: 1, bulk_pending: 2, sending: 3, sent: 4)
+  end
+
   def user_doesnt_already_exist
     @email_already_exists = false
     return if email.blank?
@@ -66,7 +76,7 @@ class Invite < ActiveRecord::Base
       topic: topic,
       group_ids: group_ids,
       custom_message: custom_message,
-      send_email: true
+      emailed_status: emailed_status_types[:pending]
     )
   end
 
@@ -75,7 +85,7 @@ class Invite < ActiveRecord::Base
     invite = create_invite_by_email(email, invited_by,
       topic: topic,
       group_ids: group_ids,
-      send_email: false
+      emailed_status: emailed_status_types[:not_required]
     )
 
     "#{Discourse.base_url}/invites/#{invite.invite_key}" if invite
@@ -89,8 +99,8 @@ class Invite < ActiveRecord::Base
 
     topic = opts[:topic]
     group_ids = opts[:group_ids]
-    send_email = opts[:send_email].nil? ? true : opts[:send_email]
     custom_message = opts[:custom_message]
+    emailed_status = opts[:emailed_status] || emailed_status_types[:pending]
     lower_email = Email.downcase(email)
 
     if user = find_user_by_email(lower_email)
@@ -112,16 +122,20 @@ class Invite < ActiveRecord::Base
     end
 
     if invite
+      if invite.emailed_status == Invite.emailed_status_types[:not_required]
+        emailed_status = invite.emailed_status
+      end
+
       invite.update_columns(
         created_at: Time.zone.now,
         updated_at: Time.zone.now,
-        via_email: invite.via_email && send_email
+        emailed_status: emailed_status
       )
     else
       create_args = {
         invited_by: invited_by,
         email: lower_email,
-        via_email: send_email
+        emailed_status: emailed_status
       }
 
       create_args[:moderator] = true if opts[:moderator]
@@ -143,7 +157,10 @@ class Invite < ActiveRecord::Base
       end
     end
 
-    Jobs.enqueue(:invite_email, invite_id: invite.id) if send_email
+    if emailed_status == emailed_status_types[:pending]
+      invite.update_column(:emailed_status, Invite.emailed_status_types[:sending])
+      Jobs.enqueue(:invite_email, invite_id: invite.id)
+    end
 
     invite.reload
     invite
@@ -261,10 +278,11 @@ end
 #  invalidated_at :datetime
 #  moderator      :boolean          default(FALSE), not null
 #  custom_message :text
-#  via_email      :boolean          default(FALSE), not null
+#  emailed_status :integer
 #
 # Indexes
 #
 #  index_invites_on_email_and_invited_by_id  (email,invited_by_id)
+#  index_invites_on_emailed_status           (emailed_status)
 #  index_invites_on_invite_key               (invite_key) UNIQUE
 #

app/models/invite_redeemer.rb

@@ -60,7 +60,7 @@ InviteRedeemer = Struct.new(:invite, :username, :name, :password, :user_custom_f
 
     user.save!
 
-    if invite.via_email
+    if invite.emailed_status != Invite.emailed_status_types[:not_required]
       user.email_tokens.create!(email: user.email)
       user.activate
     end

app/models/user_second_factor.rb

@@ -44,6 +44,7 @@ end
 #  last_used  :datetime
 #  created_at :datetime         not null
 #  updated_at :datetime         not null
+#  name       :string
 #
 # Indexes
 #

db/migrate/20190711154946_add_emailed_status_to_invite.rb

@@ -0,0 +1,14 @@
+# frozen_string_literal: true
+
+class AddEmailedStatusToInvite < ActiveRecord::Migration[5.2]
+  def change
+    add_column :invites, :emailed_status, :integer
+    add_index :invites, :emailed_status
+
+    DB.exec <<~SQL
+      UPDATE invites
+      SET emailed_status = 0
+      WHERE via_email = false
+    SQL
+  end
+end

db/post_migrate/20190716124050_remove_via_email_from_invite.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+require 'migration/column_dropper'
+
+class RemoveViaEmailFromInvite < ActiveRecord::Migration[5.2]
+  def up
+    Migration::ColumnDropper.execute_drop(:invites, %i{via_email})
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

spec/fabricators/invite_fabricator.rb

@@ -3,5 +3,4 @@
 Fabricator(:invite) do
   invited_by(fabricator: :user)
   email 'iceking@ADVENTURETIME.ooo'
-  via_email true
 end

spec/jobs/bulk_invite_spec.rb

@@ -89,6 +89,27 @@ describe Jobs::BulkInvite do
       expect(Invite.exists?(email: "test2@discourse.org")).to eq(true)
       expect(existing_user.reload.groups).to eq([group1])
     end
-  end
 
+    context 'invites are more than 200' do
+      let(:bulk_invites) { [] }
+
+      before do
+        202.times do |i|
+          bulk_invites << { "email": "test_#{i}@discourse.org" }
+        end
+      end
+
+      it 'rate limits email sending' do
+        described_class.new.execute(
+          current_user_id: admin.id,
+          invites: bulk_invites
+        )
+
+        invite = Invite.last
+        expect(invite.email).to eq("test_201@discourse.org")
+        expect(invite.emailed_status).to eq(Invite.emailed_status_types[:bulk_pending])
+        expect(Jobs::ProcessBulkInviteEmails.jobs.size).to eq(1)
+      end
+    end
+  end
 end

spec/jobs/invite_email_spec.rb

@@ -27,8 +27,15 @@ describe Jobs::InviteEmail do
         InviteMailer.expects(:send_invite).never
         Jobs::InviteEmail.new.execute(invite_id: invite.id)
       end
+
+      it "updates invite emailed_status" do
+        invite.emailed_status = Invite.emailed_status_types[:pending]
+        invite.save!
+        Jobs::InviteEmail.new.execute(invite_id: invite.id)
+
+        invite.reload
+        expect(invite.emailed_status).to eq(Invite.emailed_status_types[:sent])
+      end
     end
-
   end
-
 end

spec/jobs/process_bulk_invite_emails_spec.rb

@@ -0,0 +1,18 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+describe Jobs::ProcessBulkInviteEmails do
+  describe '#execute' do
+    it 'processes pending invites' do
+      invite = Fabricate(:invite, emailed_status: Invite.emailed_status_types[:bulk_pending])
+
+      described_class.new.execute({})
+
+      invite.reload
+      expect(invite.emailed_status).to eq(Invite.emailed_status_types[:sending])
+      expect(Jobs::InviteEmail.jobs.size).to eq(1)
+      expect(Jobs::ProcessBulkInviteEmails.jobs.size).to eq(1)
+    end
+  end
+end

spec/models/invite_redeemer_spec.rb

@@ -47,6 +47,15 @@ describe InviteRedeemer do
       expect(user.email).to eq('staged@account.com')
       expect(user.approved).to eq(true)
     end
+
+    it "should not activate user invited via links" do
+      user = InviteRedeemer.create_user_from_invite(Fabricate(:invite, email: 'walter.white@email.com', emailed_status: Invite.emailed_status_types[:not_required]), 'walter', 'Walter White')
+      expect(user.username).to eq('walter')
+      expect(user.name).to eq('Walter White')
+      expect(user.email).to eq('walter.white@email.com')
+      expect(user.approved).to eq(true)
+      expect(user.active).to eq(false)
+    end
   end
 
   describe "#redeem" do

spec/models/invite_spec.rb

@@ -73,6 +73,14 @@ describe Invite do
         end
       end
 
+      context 'links' do
+        it 'does not enqueue a job to email the invite' do
+          expect do
+            Invite.generate_invite_link(iceking, inviter, topic)
+          end.not_to change { Jobs::InviteEmail.jobs.size }
+        end
+      end
+
       context 'destroyed' do
         it "can invite the same user after their invite was destroyed" do
           Invite.invite_by_email(iceking, inviter, topic).destroy!
@@ -151,26 +159,25 @@ describe Invite do
           end
         end
 
-        it 'correctly marks invite as sent via email' do
+        it 'correctly marks invite emailed_status for email invites' do
-          expect(invite.via_email).to eq(true)
+          expect(invite.emailed_status).to eq(Invite.emailed_status_types[:sending])
 
           Invite.invite_by_email(iceking, inviter, topic)
-          expect(invite.reload.via_email).to eq(true)
+          expect(invite.reload.emailed_status).to eq(Invite.emailed_status_types[:sending])
         end
 
-        it 'does not mark invite as sent via email after generating invite link' do
+        it 'does not mark emailed_status as sending after generating invite link' do
-          expect(invite.via_email).to eq(true)
+          expect(invite.emailed_status).to eq(Invite.emailed_status_types[:sending])
 
           Invite.generate_invite_link(iceking, inviter, topic)
-          expect(invite.reload.via_email).to eq(false)
+          expect(invite.reload.emailed_status).to eq(Invite.emailed_status_types[:not_required])
 
           Invite.invite_by_email(iceking, inviter, topic)
-          expect(invite.reload.via_email).to eq(false)
+          expect(invite.reload.emailed_status).to eq(Invite.emailed_status_types[:not_required])
 
           Invite.generate_invite_link(iceking, inviter, topic)
-          expect(invite.reload.via_email).to eq(false)
+          expect(invite.reload.emailed_status).to eq(Invite.emailed_status_types[:not_required])
         end
-
       end
     end
   end
@@ -496,4 +503,20 @@ describe Invite do
       expect(expired_invite.deleted_at).to be_present
     end
   end
+
+  describe '#emailed_status_types' do
+    context "verify enum sequence" do
+      before do
+        @emailed_status_types = Invite.emailed_status_types
+      end
+
+      it "'not_required' should be at 0 position" do
+        expect(@emailed_status_types[:not_required]).to eq(0)
+      end
+
+      it "'sent' should be at 4th position" do
+        expect(@emailed_status_types[:sent]).to eq(4)
+      end
+    end
+  end
 end

spec/requests/invites_controller_spec.rb

@@ -353,7 +353,7 @@ describe InvitesController do
 
           context "with password" do
             context "user was invited via email" do
-              before { invite.update_column(:via_email, true) }
+              before { invite.update_column(:emailed_status, Invite.emailed_status_types[:pending]) }
 
               it "doesn't send an activation email and activates the user" do
                 expect do
@@ -373,7 +373,7 @@ describe InvitesController do
             end
 
             context "user was invited via link" do
-              before { invite.update_column(:via_email, false) }
+              before { invite.update_column(:emailed_status, Invite.emailed_status_types[:not_required]) }
 
               it "sends an activation email and doesn't activate the user" do
                 expect do