REFACTOR: Simplify and DRY `Group#invite`.

app/controllers/topics_controller.rb

@@ -477,9 +477,19 @@ class TopicsController < ApplicationController
   end
 
   def invite
-    username_or_email = params[:user] ? fetch_username : fetch_email
+    unless guardian.is_staff?
+      RateLimiter.new(
+        current_user,
+        "topic-invitations-per-day",
+        SiteSetting.max_topic_invitations_per_day,
+        1.day.to_i
+      ).performed!
+    end
 
     topic = Topic.find_by(id: params[:topic_id])
+    raise Discourse::InvalidParameters.new unless topic
+
+    username_or_email = params[:user] ? fetch_username : fetch_email
 
     groups = Group.lookup_groups(
       group_ids: params[:group_ids],
@@ -492,6 +502,7 @@ class TopicsController < ApplicationController
     begin
       if topic.invite(current_user, username_or_email, group_ids, params[:custom_message])
         user = User.find_by_username_or_email(username_or_email)
+
         if user
           render_json_dump BasicUserSerializer.new(user, scope: guardian, root: 'user')
         else

app/models/topic.rb

@@ -792,62 +792,42 @@ SQL
     true
   end
 
-  # Invite a user to the topic by username or email. Returns success/failure
   def invite(invited_by, username_or_email, group_ids = nil, custom_message = nil)
     user = User.find_by_username_or_email(username_or_email)
 
-    if private_message?
+    if user && topic_allowed_users.where(user_id: user.id).exists?
-      # If the user exists, add them to the message.
+      raise UserExists.new(I18n.t("topic_invite.user_exists"))
-      raise UserExists.new I18n.t("topic_invite.user_exists") if user.present? && topic_allowed_users.where(user_id: user.id).exists?
-
-      if user && topic_allowed_users.create!(user_id: user.id)
-        # Create a small action message
-        add_small_action(invited_by, "invited_user", user.username)
-
-        # Notify the user they've been invited
-        user.notifications.create(notification_type: Notification.types[:invited_to_private_message],
-                                  topic_id: id,
-                                  post_number: 1,
-                                  data: { topic_title: title,
-                                          display_username: invited_by.username }.to_json)
-        return true
-      end
     end
 
-    if username_or_email =~ /^.+@.+$/ && Guardian.new(invited_by).can_invite_via_email?(self)
+    if user && private_message? && topic_allowed_users.create!(user_id: user.id)
-      RateLimiter.new(invited_by, "topic-invitations-per-day", SiteSetting.max_topic_invitations_per_day, 1.day.to_i).performed!
+      add_small_action(invited_by, "invited_user", user.username)
 
-      if user.present?
+      create_invite_notification!(
-        # add existing users
+        Notification.types[:invited_to_private_message],
+        invited_by.username
+      )
+
+      true
+    elsif username_or_email =~ /^.+@.+$/ && Guardian.new(invited_by).can_invite_via_email?(self)
+      if user
         Invite.extend_permissions(self, user, invited_by)
 
-        # Notify the user they've been invited
+        create_invite_notification!(
-        user.notifications.create(notification_type: Notification.types[:invited_to_topic],
+          Notification.types[:invited_to_topic],
-                                  topic_id: id,
+          invited_by.username
-                                  post_number: 1,
+        )
-                                  data: { topic_title: title,
-                                          display_username: invited_by.username }.to_json)
-        return true
       else
-        # NOTE callers expect an invite object if an invite was sent via email
         invite_by_email(invited_by, username_or_email, group_ids, custom_message)
       end
-    else
-      raise UserExists.new I18n.t("topic_invite.user_exists") if user.present? && topic_allowed_users.where(user_id: user.id).exists?
 
-      if user && topic_allowed_users.create!(user_id: user.id)
+      true
-        RateLimiter.new(invited_by, "topic-invitations-per-day", SiteSetting.max_topic_invitations_per_day, 1.day.to_i).performed!
+    elsif user && topic_allowed_users.create!(user_id: user.id)
+      create_invite_notification!(
+        Notification.types[:invited_to_topic],
+        invited_by.username
+      )
 
-        # Notify the user they've been invited
+      true
-        user.notifications.create(notification_type: Notification.types[:invited_to_topic],
-                                  topic_id: id,
-                                  post_number: 1,
-                                  data: { topic_title: title,
-                                          display_username: invited_by.username }.to_json)
-        return true
-      else
-        false
-      end
     end
   end
 
@@ -1310,6 +1290,17 @@ SQL
     RateLimiter.new(user, "#{key}-per-day", SiteSetting.send(method_name), 1.day.to_i)
   end
 
+  def create_invite_notification!(notification_type, username)
+    user.notifications.create!(
+      notification_type: notification_type,
+      topic_id: self.id,
+      post_number: 1,
+      data: {
+        topic_title: self.title,
+        display_username: username
+      }.to_json
+    )
+  end
 end
 
 # == Schema Information

spec/controllers/topics_controller_spec.rb

@@ -1260,103 +1260,6 @@ describe TopicsController do
     end
   end
 
-  describe 'invite' do
-
-    describe "group invites" do
-      it "works correctly" do
-        group = Fabricate(:group)
-        topic = Fabricate(:topic)
-        _admin = log_in(:admin)
-
-        post :invite, params: {
-          topic_id: topic.id, email: 'hiro@from.heros', group_ids: "#{group.id}"
-        }, format: :json
-
-        expect(response).to be_success
-
-        invite = Invite.find_by(email: 'hiro@from.heros')
-        groups = invite.groups.to_a
-        expect(groups.count).to eq(1)
-        expect(groups[0].id).to eq(group.id)
-      end
-    end
-
-    it "won't allow us to invite toa topic when we're not logged in" do
-      post :invite, params: {
-        topic_id: 1, email: 'jake@adventuretime.ooo'
-      }, format: :json
-      expect(response.status).to eq(403)
-    end
-
-    describe 'when logged in as group manager' do
-      let(:group_manager) { log_in }
-      let(:group) { Fabricate(:group).tap { |g| g.add_owner(group_manager) } }
-      let(:private_category)  { Fabricate(:private_category, group: group) }
-      let(:group_private_topic) { Fabricate(:topic, category: private_category, user: group_manager) }
-      let(:recipient) { 'jake@adventuretime.ooo' }
-
-      it "should attach group to the invite" do
-        post :invite, params: {
-          topic_id: group_private_topic.id, user: recipient
-        }, format: :json
-
-        expect(response).to be_success
-        expect(Invite.find_by(email: recipient).groups).to eq([group])
-      end
-    end
-
-    describe 'when logged in' do
-      before do
-        @topic = Fabricate(:topic, user: log_in)
-      end
-
-      it 'requires an email parameter' do
-        expect do
-          post :invite, params: { topic_id: @topic.id }, format: :json
-        end.to raise_error(ActionController::ParameterMissing)
-      end
-
-      describe 'without permission' do
-        it "raises an exception when the user doesn't have permission to invite to the topic" do
-          post :invite, params: {
-            topic_id: @topic.id, user: 'jake@adventuretime.ooo'
-          }, format: :json
-
-          expect(response).to be_forbidden
-        end
-      end
-
-      describe 'with admin permission' do
-
-        let!(:admin) do
-          log_in :admin
-        end
-
-        it 'should work as expected' do
-          post :invite, params: {
-            topic_id: @topic.id, user: 'jake@adventuretime.ooo'
-          }, format: :json
-
-          expect(response).to be_success
-          expect(::JSON.parse(response.body)).to eq('success' => 'OK')
-          expect(Invite.where(invited_by_id: admin.id).count).to eq(1)
-        end
-
-        it 'should fail on shoddy email' do
-          post :invite, params: {
-            topic_id: @topic.id, user: 'i_am_not_an_email'
-          }, format: :json
-
-          expect(response).not_to be_success
-          expect(::JSON.parse(response.body)).to eq('failed' => 'FAILED')
-        end
-
-      end
-
-    end
-
-  end
-
   describe 'make_banner' do
 
     it 'needs you to be a staff member' do

spec/models/topic_spec.rb

@@ -493,8 +493,7 @@ describe Topic do
       expect(Guardian.new(coding_horror).can_see?(topic)).to eq(true)
       expect(TopicQuery.new(evil_trout).list_latest.topics).not_to include(topic)
 
-      # invites
+      expect(topic.invite(topic.user, 'duhhhhh')).to eq(nil)
-      expect(topic.invite(topic.user, 'duhhhhh')).to eq(false)
     end
 
     context 'invite' do
@@ -567,7 +566,8 @@ describe Topic do
           it 'adds user correctly' do
             expect {
               expect(topic.invite(topic.user, walter.email)).to eq(true)
-            }.to change(Notification, :count)
+            }.to change(Notification, :count).by(1)
+
             expect(topic.allowed_users.include?(walter)).to eq(true)
           end
 
@@ -591,35 +591,6 @@ describe Topic do
 
   end
 
-  context 'rate limits' do
-
-    it "rate limits topic invitations" do
-      SiteSetting.max_topic_invitations_per_day = 2
-      RateLimiter.enable
-      RateLimiter.clear_all!
-
-      start = Time.now.tomorrow.beginning_of_day
-      freeze_time(start)
-
-      user = Fabricate(:user)
-      trust_level_2 = Fabricate(:user, trust_level: 2)
-      topic = Fabricate(:topic, user: trust_level_2)
-
-      freeze_time(start + 10.minutes)
-      topic.invite(topic.user, user.username)
-
-      freeze_time(start + 20.minutes)
-      topic.invite(topic.user, "walter@white.com")
-
-      freeze_time(start + 30.minutes)
-
-      expect {
-        topic.invite(topic.user, "user@example.com")
-      }.to raise_error(RateLimiter::LimitExceeded)
-    end
-
-  end
-
   context 'bumping topics' do
 
     before do
@@ -1824,8 +1795,8 @@ describe Topic do
       let(:randolph) { 'randolph@duke.ooo' }
 
       it "should attach group to the invite" do
-        invite = group_private_topic.invite(group_manager, randolph)
+        expect(group_private_topic.invite(group_manager, randolph)).to eq(true)
-        expect(invite.groups).to eq([group])
+        expect(Invite.last.groups).to eq([group])
       end
     end
 

spec/requests/topics_controller_spec.rb

@@ -156,6 +156,111 @@ RSpec.describe TopicsController do
     end
   end
 
+  describe '#invite' do
+    describe 'when not logged in' do
+      it "should return the right response" do
+        post "/t/#{topic.id}/invite.json", params: {
+          email: 'jake@adventuretime.ooo'
+        }
+
+        expect(response.status).to eq(403)
+      end
+    end
+
+    describe 'when logged in' do
+      before do
+        sign_in(user)
+      end
+
+      describe 'as a valid user' do
+        let(:topic) { Fabricate(:topic, user: user) }
+
+        it 'should return the right response' do
+          user.update!(trust_level: TrustLevel[2])
+
+          expect do
+            post "/t/#{topic.id}/invite.json", params: {
+              email: 'someguy@email.com'
+            }
+          end.to change { Invite.where(invited_by_id: user.id).count }.by(1)
+
+          expect(response.status).to eq(200)
+        end
+      end
+
+      describe 'when user is a group manager' do
+        let(:group) { Fabricate(:group).tap { |g| g.add_owner(user) } }
+        let(:private_category)  { Fabricate(:private_category, group: group) }
+
+        let(:group_private_topic) do
+          Fabricate(:topic, category: private_category, user: user)
+        end
+
+        let(:recipient) { 'jake@adventuretime.ooo' }
+
+        it "should attach group to the invite" do
+
+          post "/t/#{group_private_topic.id}/invite.json", params: {
+            user: recipient
+          }
+
+          expect(response.status).to eq(200)
+          expect(Invite.find_by(email: recipient).groups).to eq([group])
+        end
+      end
+
+      describe 'when topic id is invalid' do
+        it 'should return the right response' do
+          post "/t/999/invite.json", params: {
+            email: Fabricate(:user).email
+          }
+
+          expect(response.status).to eq(400)
+        end
+      end
+
+      it 'requires an email parameter' do
+        post "/t/#{topic.id}/invite.json"
+
+        expect(response.status).to eq(400)
+      end
+
+      describe 'when user does not have permission to invite to the topic' do
+        let(:topic) { Fabricate(:private_message_topic) }
+
+        it "should return the right response" do
+          post "/t/#{topic.id}/invite.json", params: {
+            user: user.username
+          }
+
+          expect(response.status).to eq(403)
+        end
+      end
+    end
+
+    describe "when inviting a group to a topic" do
+      let(:group) { Fabricate(:group) }
+
+      before do
+        sign_in(Fabricate(:admin))
+      end
+
+      it "should work correctly" do
+        email = 'hiro@from.heros'
+
+        post "/t/#{topic.id}/invite.json", params: {
+          email: email, group_ids: group.id
+        }
+
+        expect(response.status).to eq(200)
+
+        groups = Invite.find_by(email: email).groups
+        expect(groups.count).to eq(1)
+        expect(groups.first.id).to eq(group.id)
+      end
+    end
+  end
+
   describe 'invite_group' do
     let(:admins) { Group[:admins] }
     let(:pm) { Fabricate(:private_message_topic) }