FEATURE: Activate users invited via email when invite is redeemed

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.

From 7977b09025751973f7ae1271f68aaab2716e01fa Mon Sep 17 00:00:00 2001
From: Gerhard Schlager <mail@gerhard-schlager.at>
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.

diff --git a/app/controllers/invites_controller.rb b/app/controllers/invites_controller.rb
index 87d66fd..ed4844c 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 cc44d69..2a7f968 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 0c1702c..ffdd84c 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 0000000..144987c
--- /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 ecb9133..d092cf7 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 ac31409..870d2b1 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 33a49cb..6ed73ee 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 9663b88..37b2c7e 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)
-          end
+          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)
+             

GitHub

This commit has been mentioned on Discourse Meta. There might be relevant details there:

Accordingly to the docs for #create

The resulting object is returned whether the object was saved successfully to the database or not.

Should we handle the case when create doesn’t actually save the object?

If we’re not checking whether the object was actually created, create! will probably be better.

I think this conditional here is not tested.

we probably want to check the user_id args as well.

Damn! I always forget to use create!. I wish there was a Rubocop rule for this.

2 Likes

The tests in invites_controller_spec.rb make sure that this works. I didn’t want to test the same thing in multiple places.

2 Likes

:+1:

If I read the docs correctly, the object is saved unless the validation fails. Looking at the code of EmailToken I don’t see a reason why validation would ever fail. I’m going to leave it as is.

That is not future proofing stuff though. If someone adds a validation in the future, they wouldn’t jnkow they have to update this to raise an error if the validation fails

I’m sorry, but to me that sounds like a theoretical problem. We are using user.email_tokens.create countless times without checking for persisted?, but it doesn’t matter, because tests will start to fail all over the place. I just gave it a try. You can’t confirm an email token if it wasn’t persisted and that makes our specs fail.

Anyway, I’m going to handle it in this case, but please don’t start complaining about my usage of raise ActiveRecord::RecordInvalid.new. This will never happen. :wink:

Follow-up in DEV: Improve specs and handle invalid email token

I’m not following how this is a theoretical problem, we’re using an API that returns an object whether it was saved to the database or not. What it means in this scenario is that email_token.id can have two possible value, however small the chances for each might be, of nil or a valid integer. When that happens, whatever down the line that expects the email_token to have been persisted is going to have problems.

If we look at the current state of things then it might never happen but I don’t think that means we should not consider the possibility that it might/can happen. My strong views on prefering create over create! when we’re not checking for the return value when using create is so that we create visibility on failure. create returning a object that isn’t persisted might or might not cause failure down the line. In this case, the Sidekiq job is en-queued with an email token that was never persisted. The second reason is that create! fails immediately pointing us directly at the problem. For the sidekiq job, it might fail some where else in the code path and we’re left to backtrace where the failure happened when debugging.

1 Like