diff --git a/app/jobs/regular/bulk_invite.rb b/app/jobs/regular/bulk_invite.rb index 7c3d1a67cda..f7ccd44fd6a 100644 --- a/app/jobs/regular/bulk_invite.rb +++ b/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)}" diff --git a/app/jobs/regular/invite_email.rb b/app/jobs/regular/invite_email.rb index 0db080ce80d..c91e67ae358 100644 --- a/app/jobs/regular/invite_email.rb +++ b/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 diff --git a/app/jobs/regular/process_bulk_invite_emails.rb b/app/jobs/regular/process_bulk_invite_emails.rb new file mode 100644 index 00000000000..f4d057e7c2a --- /dev/null +++ b/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 diff --git a/app/models/invite.rb b/app/models/invite.rb index 1a909a248d9..e92d5cd4083 100644 --- a/app/models/invite.rb +++ b/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 # diff --git a/app/models/invite_redeemer.rb b/app/models/invite_redeemer.rb index d41662bb271..755241e00d1 100644 --- a/app/models/invite_redeemer.rb +++ b/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 diff --git a/app/models/user_second_factor.rb b/app/models/user_second_factor.rb index 0d3d8ee3512..88dcfefd4e3 100644 --- a/app/models/user_second_factor.rb +++ b/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 # diff --git a/db/migrate/20190711154946_add_emailed_status_to_invite.rb b/db/migrate/20190711154946_add_emailed_status_to_invite.rb new file mode 100644 index 00000000000..9123f508f0e --- /dev/null +++ b/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 diff --git a/db/post_migrate/20190716124050_remove_via_email_from_invite.rb b/db/post_migrate/20190716124050_remove_via_email_from_invite.rb new file mode 100644 index 00000000000..676ea11bcac --- /dev/null +++ b/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 diff --git a/spec/fabricators/invite_fabricator.rb b/spec/fabricators/invite_fabricator.rb index 7b390ae90c4..aedc23c1772 100644 --- a/spec/fabricators/invite_fabricator.rb +++ b/spec/fabricators/invite_fabricator.rb @@ -3,5 +3,4 @@ Fabricator(:invite) do invited_by(fabricator: :user) email 'iceking@ADVENTURETIME.ooo' - via_email true end diff --git a/spec/jobs/bulk_invite_spec.rb b/spec/jobs/bulk_invite_spec.rb index d271df7ba86..283fbee2e2d 100644 --- a/spec/jobs/bulk_invite_spec.rb +++ b/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 diff --git a/spec/jobs/invite_email_spec.rb b/spec/jobs/invite_email_spec.rb index 726a31600a5..f67fd135e76 100644 --- a/spec/jobs/invite_email_spec.rb +++ b/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 diff --git a/spec/jobs/process_bulk_invite_emails_spec.rb b/spec/jobs/process_bulk_invite_emails_spec.rb new file mode 100644 index 00000000000..c0687e9b6d5 --- /dev/null +++ b/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 diff --git a/spec/models/invite_redeemer_spec.rb b/spec/models/invite_redeemer_spec.rb index b370d92debd..f0f2798ba74 100644 --- a/spec/models/invite_redeemer_spec.rb +++ b/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 diff --git a/spec/models/invite_spec.rb b/spec/models/invite_spec.rb index 8f1136efe8c..573f73c9d73 100644 --- a/spec/models/invite_spec.rb +++ b/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 - expect(invite.via_email).to eq(true) + it 'correctly marks invite emailed_status for email invites' do + 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 - expect(invite.via_email).to eq(true) + it 'does not mark emailed_status as sending after generating invite link' do + 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 diff --git a/spec/requests/invites_controller_spec.rb b/spec/requests/invites_controller_spec.rb index 8b833682f69..8fe51106fa3 100644 --- a/spec/requests/invites_controller_spec.rb +++ b/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