FIX: better accept invite flow when user is invited via a link

FIX: better accept invite flow when user is invited via a link

diff --git a/app/assets/javascripts/discourse/controllers/invites-show.js.es6 b/app/assets/javascripts/discourse/controllers/invites-show.js.es6
index 78c6bbc..128269b 100644
--- a/app/assets/javascripts/discourse/controllers/invites-show.js.es6
+++ b/app/assets/javascripts/discourse/controllers/invites-show.js.es6
@@ -94,8 +94,9 @@ export default Ember.Controller.extend(
                 "successMessage",
                 result.message || I18n.t("invites.success")
               );
-              this.set("redirectTo", result.redirect_to);
-              DiscourseURL.redirectTo(result.redirect_to || "/");
+              if (result.redirect_to) {
+                DiscourseURL.redirectTo(result.redirect_to || "/");
+              }
             } else {
               if (
                 result.errors &&
diff --git a/app/assets/javascripts/discourse/templates/invites/show.hbs b/app/assets/javascripts/discourse/templates/invites/show.hbs
index 65c7750..e887723 100644
--- a/app/assets/javascripts/discourse/templates/invites/show.hbs
+++ b/app/assets/javascripts/discourse/templates/invites/show.hbs
@@ -8,13 +8,12 @@
     </div>
 
     <div class="col-form">
-      <p>{{i18n 'invites.invited_by'}}</p>
-
-      <p>{{user-info user=invitedBy}}</p>
-
       {{#if successMessage}}
-        <p>{{successMessage}}</p>
+        <br/><br/>
+        <div class='alert alert-info'><p>{{{successMessage}}}</p></div>
       {{else}}
+        <p>{{i18n 'invites.invited_by'}}</p>
+        <p>{{user-info user=invitedBy}}</p>
 
         <p>{{{yourEmailMessage}}}
         {{#if externalAuthsEnabled}}
@@ -63,7 +62,6 @@
             <br/><br/>
             <div class='alert alert-error'>{{errorMessage}}</div>
           {{/if}}
-
         </form>
       {{/if}}
     </div>
diff --git a/app/controllers/invites_controller.rb b/app/controllers/invites_controller.rb
index c888cff..a178e401 100644
--- a/app/controllers/invites_controller.rb
+++ b/app/controllers/invites_controller.rb
@@ -47,16 +47,19 @@ class InvitesController < ApplicationController
       begin
         user = invite.redeem(username: params[:username], name: params[:name], password: params[:password], user_custom_fields: params[:user_custom_fields])
         if user.present?
-          log_on_user(user)
+          log_on_user(user) if user.active?
           post_process_invite(user)
         end
 
-        topic = user.present? ? invite.topics.first : nil
+        response = { success: true }
+        if user.present? && user.active?
+          topic = invite.topics.first
+          response[:redirect_to] = topic.present? ? path("#{topic.relative_url}") : path("/")
+        else
+          response[:message] = I18n.t('invite.confirm_email')
+        end
 
-        render json: {
-          success: true,
-          redirect_to: topic.present? ? path("#{topic.relative_url}") : path("/")
-        }
+        render json: response
       rescue ActiveRecord::RecordInvalid, ActiveRecord::RecordNotSaved => e
         render json: {
           success: false,
diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml
index 906e5ee..743a573 100644
--- a/config/locales/server.en.yml
+++ b/config/locales/server.en.yml
@@ -185,6 +185,7 @@ en:
 
       <p>Otherwise please <a href="%{base_url}/password-reset">Reset Password</a>.</p>
     user_exists: "There's no need to invite <b>%{email}</b>, they <a href='%{base_path}/u/%{username}/summary'>already have an account!</a>"
+    confirm_email: "<p>You’re almost done! We sent an activation mail to your email address. Please follow the instructions in the mail to activate your account.</p><p>If it doesn’t arrive, check your spam folder.</p>"
 
   bulk_invite:
     file_should_be_csv: "The uploaded file should be of csv format."
diff --git a/spec/requests/invites_controller_spec.rb b/spec/requests/invites_controller_spec.rb
index c2557e5..b814df7 100644
--- a/spec/requests/invites_controller_spec.rb
+++ b/spec/requests/invites_controller_spec.rb
@@ -355,6 +355,7 @@ describe InvitesController do
                 put "/invites/show/#{invite.invite_key}.json", params: { password: "verystrongpassword" }
                 expect(response.status).to eq(200)
                 expect(JSON.parse(response.body)["success"]).to eq(true)
+                expect(JSON.parse(response.body)["message"]).to eq(I18n.t("invite.confirm_email"))
 
                 invited_user = User.find_by_email(invite.email)
                 expect(invited_user.active).to eq(false)

GitHub sha: e0bc8265

1 Like

I think we should use success_json so that we are consistent in what we’re sending to the clients.

If result.redirect_to is present, why do we need || "/"?

Should this change be tested?

Good catch. Fixed in DEV: no need for conditional redirect in invites · discourse/discourse@4ebf170 · GitHub.

@gschlager already added extensive tests for this in FEATURE: Activate users invited via email when invite is redeemed · discourse/discourse@7977b09 · GitHub and DEV: Improve specs and handle invalid email token · discourse/discourse@688755b · GitHub. The behavior is not changed here since user were not getting logged in before this change. This change just ensures that we do not try to log in users unnecessarily since that attempt will fail as the user is not marked as active yet.

I agree that using success_json here would have been better here for consistency but we’re sending success: true in the response ever since “accept invite” page was implemented (~2 years ago). All the specs are testing for that response and we can’t be sure about more places where the same API response is being expected. I am not sure about the benefits we’ll have changing the API response here so late in the feature cycle.

Hmm I’m abit confused here. There isn’t a guard in log_on_user that would make this fail if the user is not active.

I think the benefit here is that it moves us closer to a more consistent response from our API. We don’t really need to sweep through the code base to fix everything now but I think it is helpful to think about the patterns used in our code base when making changes instead of just sticking with what someone else has written in the past. Anyway, this is just a small nitpick :grin:

1 Like

See:

and

That will only happen if you call current_user. Since it can potentially raise an error if we log on a user that hasn’t been activated, shouldn’t we add a test for it and @gschlager 's test doesn’t actually cover this line.

Added in DEV: add a spec for "accept invite" log_on_user behaviour · discourse/discourse@05c015d · GitHub.

1 Like