From 7977b09025751973f7ae1271f68aaab2716e01fa Mon Sep 17 00:00:00 2001 From: Gerhard Schlager Date: Mon, 10 Dec 2018 23:24:02 +0100 Subject: [PATCH] FEATURE: Activate users invited via email when invite is redeemed Do not send an activation email to users invited via email. They already confirmed their email address by clicking the invite link. Users invited via link will need to confirm their email address before they can login. --- app/controllers/invites_controller.rb | 13 ++- app/models/invite.rb | 16 +++- app/models/invite_redeemer.rb | 15 +++- ...20181005144357_add_via_email_to_invites.rb | 5 ++ spec/fabricators/invite_fabricator.rb | 1 + spec/models/invite_redeemer_spec.rb | 3 +- spec/models/invite_spec.rb | 21 +++++ spec/requests/invites_controller_spec.rb | 81 +++++++++++++------ 8 files changed, 121 insertions(+), 34 deletions(-) create mode 100644 db/migrate/20181005144357_add_via_email_to_invites.rb diff --git a/app/controllers/invites_controller.rb b/app/controllers/invites_controller.rb index 87d66fd66d9..ed4844c3573 100644 --- a/app/controllers/invites_controller.rb +++ b/app/controllers/invites_controller.rb @@ -211,12 +211,21 @@ class InvitesController < ApplicationController def post_process_invite(user) user.enqueue_welcome_message('welcome_invite') if user.send_welcome_message + if user.has_password? - email_token = user.email_tokens.create(email: user.email) - Jobs.enqueue(:critical_user_email, type: :signup, user_id: user.id, email_token: email_token.token) + send_activation_email(user) unless user.active elsif !SiteSetting.enable_sso && SiteSetting.enable_local_logins Jobs.enqueue(:invite_password_instructions_email, username: user.username) end end + def send_activation_email(user) + email_token = user.email_tokens.create(email: user.email) + + Jobs.enqueue(:critical_user_email, + type: :signup, + user_id: user.id, + email_token: email_token.token + ) + end end diff --git a/app/models/invite.rb b/app/models/invite.rb index cc44d69151a..2a7f96876aa 100644 --- a/app/models/invite.rb +++ b/app/models/invite.rb @@ -107,10 +107,19 @@ class Invite < ActiveRecord::Base invite = nil end - invite.update_columns(created_at: Time.zone.now, updated_at: Time.zone.now) if invite + if invite + invite.update_columns( + created_at: Time.zone.now, + updated_at: Time.zone.now, + via_email: invite.via_email && send_email + ) + else + create_args = { + invited_by: invited_by, + email: lower_email, + via_email: send_email + } - if !invite - create_args = { invited_by: invited_by, email: lower_email } create_args[:moderator] = true if opts[:moderator] create_args[:custom_message] = custom_message if custom_message invite = Invite.create!(create_args) @@ -257,6 +266,7 @@ end # invalidated_at :datetime # moderator :boolean default(FALSE), not null # custom_message :text +# via_email :boolean default(FALSE), not null # # Indexes # diff --git a/app/models/invite_redeemer.rb b/app/models/invite_redeemer.rb index 0c1702c3641..ffdd84cc837 100644 --- a/app/models/invite_redeemer.rb +++ b/app/models/invite_redeemer.rb @@ -24,7 +24,7 @@ InviteRedeemer = Struct.new(:invite, :username, :name, :password, :user_custom_f email: invite.email, username: available_username, name: available_name, - active: true, + active: false, trust_level: SiteSetting.default_invitee_trust_level } @@ -57,6 +57,12 @@ InviteRedeemer = Struct.new(:invite, :username, :name, :password, :user_custom_f end user.save! + + if invite.via_email + user.email_tokens.create(email: user.email) + user.activate + end + User.find(user.id) end @@ -82,8 +88,9 @@ InviteRedeemer = Struct.new(:invite, :username, :name, :password, :user_custom_f end def mark_invite_redeemed - Invite.where(['id = ? AND redeemed_at IS NULL AND created_at >= ?', - invite.id, SiteSetting.invite_expiry_days.days.ago]).update_all('redeemed_at = CURRENT_TIMESTAMP') + Invite.where('id = ? AND redeemed_at IS NULL AND created_at >= ?', + invite.id, SiteSetting.invite_expiry_days.days.ago) + .update_all('redeemed_at = CURRENT_TIMESTAMP') end def get_invited_user @@ -119,7 +126,7 @@ InviteRedeemer = Struct.new(:invite, :username, :name, :password, :user_custom_f end def send_welcome_message - if Invite.where(['email = ?', invite.email]).update_all(['user_id = ?', invited_user.id]) == 1 + if Invite.where('email = ?', invite.email).update_all(['user_id = ?', invited_user.id]) == 1 invited_user.send_welcome_message = true end end diff --git a/db/migrate/20181005144357_add_via_email_to_invites.rb b/db/migrate/20181005144357_add_via_email_to_invites.rb new file mode 100644 index 00000000000..144987cff1d --- /dev/null +++ b/db/migrate/20181005144357_add_via_email_to_invites.rb @@ -0,0 +1,5 @@ +class AddViaEmailToInvites < ActiveRecord::Migration[5.2] + def change + add_column :invites, :via_email, :boolean, default: false, null: false + end +end diff --git a/spec/fabricators/invite_fabricator.rb b/spec/fabricators/invite_fabricator.rb index ecb9133f45c..d092cf73703 100644 --- a/spec/fabricators/invite_fabricator.rb +++ b/spec/fabricators/invite_fabricator.rb @@ -1,4 +1,5 @@ Fabricator(:invite) do invited_by(fabricator: :user) email 'iceking@ADVENTURETIME.ooo' + via_email true end diff --git a/spec/models/invite_redeemer_spec.rb b/spec/models/invite_redeemer_spec.rb index ac31409b522..870d2b13a20 100644 --- a/spec/models/invite_redeemer_spec.rb +++ b/spec/models/invite_redeemer_spec.rb @@ -38,7 +38,7 @@ describe InviteRedeemer do expect(user.id).to eq(staged_user.id) expect(user.username).to eq('walter') expect(user.name).to eq('Walter White') - expect(user.active).to eq(false) + expect(user.staged).to eq(false) expect(user.email).to eq('staged@account.com') expect(user.approved).to eq(true) end @@ -99,7 +99,6 @@ describe InviteRedeemer do end it "can set password" do - inviter = invite.invited_by user = InviteRedeemer.new(invite, username, name, password).redeem expect(user).to have_password expect(user.confirm_password?(password)).to eq(true) diff --git a/spec/models/invite_spec.rb b/spec/models/invite_spec.rb index 33a49cb1f4e..6ed73ee5383 100644 --- a/spec/models/invite_spec.rb +++ b/spec/models/invite_spec.rb @@ -142,6 +142,27 @@ describe Invite do expect(invite.topics).to match_array([topic, another_topic]) end end + + it 'correctly marks invite as sent via email' do + expect(invite.via_email).to eq(true) + + Invite.invite_by_email(iceking, inviter, topic) + expect(invite.reload.via_email).to eq(true) + end + + it 'does not mark invite as sent via email after generating invite link' do + expect(invite.via_email).to eq(true) + + Invite.generate_invite_link(iceking, inviter, topic) + expect(invite.reload.via_email).to eq(false) + + Invite.invite_by_email(iceking, inviter, topic) + expect(invite.reload.via_email).to eq(false) + + Invite.generate_invite_link(iceking, inviter, topic) + expect(invite.reload.via_email).to eq(false) + end + end end end diff --git a/spec/requests/invites_controller_spec.rb b/spec/requests/invites_controller_spec.rb index 9663b889e9d..37b2c7ec900 100644 --- a/spec/requests/invites_controller_spec.rb +++ b/spec/requests/invites_controller_spec.rb @@ -296,33 +296,68 @@ describe InvitesController do expect(Jobs::SendSystemMessage.jobs.size).to eq(1) end - it "sends password reset email if password is not set" do - put "/invites/show/#{invite.invite_key}.json" - expect(response.status).to eq(200) - expect(Jobs::InvitePasswordInstructionsEmail.jobs.size).to eq(1) + context "without password" do + it "sends password reset email" do + put "/invites/show/#{invite.invite_key}.json" + expect(response.status).to eq(200) + expect(Jobs::InvitePasswordInstructionsEmail.jobs.size).to eq(1) + expect(Jobs::CriticalUserEmail.jobs.size).to eq(0) + end + + it "does not send password reset email if sso is enabled" do + SiteSetting.sso_url = "https://www.example.com/sso" + SiteSetting.enable_sso = true + put "/invites/show/#{invite.invite_key}.json" + expect(response.status).to eq(200) + expect(Jobs::InvitePasswordInstructionsEmail.jobs.size).to eq(0) + expect(Jobs::CriticalUserEmail.jobs.size).to eq(0) + end + + it "does not send password reset email if local login is disabled" do + SiteSetting.enable_local_logins = false + put "/invites/show/#{invite.invite_key}.json" + expect(response.status).to eq(200) + expect(Jobs::InvitePasswordInstructionsEmail.jobs.size).to eq(0) + expect(Jobs::CriticalUserEmail.jobs.size).to eq(0) + end end - it "does not send password reset email if sso is enabled" do - SiteSetting.sso_url = "https://www.example.com/sso" - SiteSetting.enable_sso = true - put "/invites/show/#{invite.invite_key}.json" - expect(response.status).to eq(200) - expect(Jobs::InvitePasswordInstructionsEmail.jobs.size).to eq(0) + context "with password" do + context "user was invited via email" do + before { invite.update_column(:via_email, true) } + + it "doesn't send an activation email and activates the user" do + put "/invites/show/#{invite.invite_key}.json", params: { password: "verystrongpassword" } + expect(response.status).to eq(200) + + expect(Jobs::InvitePasswordInstructionsEmail.jobs.size).to eq(0) + expect(Jobs::CriticalUserEmail.jobs.size).to eq(0) + + invited_user = User.find_by_email(invite.email) + expect(invited_user.active).to eq(true) + expect(invited_user.email_confirmed?).to eq(true) + end + end + + context "user was invited via link" do + before { invite.update_column(:via_email, false) } + + it "sends an activation email and doesn't activate the user" do + put "/invites/show/#{invite.invite_key}.json", params: { password: "verystrongpassword" } + expect(response.status).to eq(200) + + expect(Jobs::InvitePasswordInstructionsEmail.jobs.size).to eq(0) + expect(Jobs::CriticalUserEmail.jobs.size).to eq(1) + expect(Jobs::CriticalUserEmail.jobs.first["args"].first["type"]).to eq("signup") + + invited_user = User.find_by_email(invite.email) + expect(invited_user.active).to eq(false) + expect(invited_user.email_confirmed?).to eq(false) + end + end + end - it "does not send password reset email if local login is disabled" do - SiteSetting.enable_local_logins = false - put "/invites/show/#{invite.invite_key}.json" - expect(response.status).to eq(200) - expect(Jobs::InvitePasswordInstructionsEmail.jobs.size).to eq(0) - end - - it 'sends an activation email if password is set' do - put "/invites/show/#{invite.invite_key}.json", params: { password: "verystrongpassword" } - expect(response.status).to eq(200) - expect(Jobs::InvitePasswordInstructionsEmail.jobs.size).to eq(0) - expect(Jobs::CriticalUserEmail.jobs.size).to eq(1) - end end end end