DEV: extract join_group method from groups#add_members method (#13807)

* Copy the add_members method to the new join method

* Remove unneeded code from the join method

* Rearrange the join method

* Remove unneeded stuff from the add_members method

* Extract add_user_to_group method

* Implement of the client side

* Tests

* Doesn't inline users.uniq

* Return promise from join.then()

* Remove unnecessary begin and end

* Revert "Return promise from join.then()"

This reverts commit bda84d8d

* Remove variable already_in_group

app/assets/javascripts/discourse/app/components/group-membership-button.js

@@ -50,13 +50,13 @@ export default Component.extend({
     joinGroup() {
       if (this.currentUser) {
         this.set("updatingMembership", true);
-        const model = this.model;
+        const group = this.model;
 
-        model
-          .addMembers(this.currentUser.get("username"))
+        group
+          .join()
           .then(() => {
-            model.set("is_group_user", true);
-            this.appEvents.trigger("group:join", model);
+            group.set("is_group_user", true);
+            this.appEvents.trigger("group:join", group);
           })
           .catch(popupAjaxError)
           .finally(() => {

app/assets/javascripts/discourse/app/models/group.js

@@ -127,6 +127,14 @@ const Group = RestModel.extend({
     });
   },
 
+  join() {
+    return ajax(`/groups/${this.id}/join.json`, {
+      type: "PUT",
+    }).then(() => {
+      this.findMembers();
+    });
+  },
+
   addOwners(usernames, filter, notifyUsers) {
     return ajax(`/admin/groups/${this.id}/owners.json`, {
       type: "PUT",

app/controllers/groups_controller.rb

@@ -333,19 +333,9 @@ class GroupsController < ApplicationController
 
   def add_members
     group = Group.find(params[:id])
-    group.public_admission ? ensure_logged_in : guardian.ensure_can_edit!(group)
+    guardian.ensure_can_edit!(group)
+
     users = users_from_params.to_a
-
-    if group.public_admission
-      if !guardian.can_log_group_changes?(group) && current_user != users.first
-        raise Discourse::InvalidAccess
-      end
-
-      unless current_user.staff?
-        RateLimiter.new(current_user, "public_group_membership", 3, 1.minute).performed!
-      end
-    end
-
     emails = []
     if params[:emails]
       params[:emails].split(",").each do |email|
@@ -376,17 +366,10 @@ class GroupsController < ApplicationController
         count: usernames_already_in_group.size
       ))
     else
+      notify = params[:notify_users]&.to_s == "true"
       uniq_users = users.uniq
       uniq_users.each do |user|
-        begin
-          group.add(user)
-          GroupActionLogger.new(current_user, group).log_add_user_to_group(user)
-          group.notify_added_to_group(user) if params[:notify_users]&.to_s == "true"
-        rescue ActiveRecord::RecordNotUnique
-          # Under concurrency, we might attempt to insert two records quickly and hit a DB
-          # constraint. In this case we can safely ignore the error and act as if the user
-          # was added to the group.
-        end
+        add_user_to_group(group, user, notify)
       end
 
       emails.each do |email|
@@ -408,6 +391,19 @@ class GroupsController < ApplicationController
     end
   end
 
+  def join
+    ensure_logged_in
+    unless current_user.staff?
+      RateLimiter.new(current_user, "public_group_membership", 3, 1.minute).performed!
+    end
+
+    group = Group.find(params[:id])
+    raise Discourse::InvalidAccess unless group.public_admission
+
+    return if group.users.exists?(id: current_user.id)
+    add_user_to_group(group, current_user)
+  end
+
   def handle_membership_request
     group = Group.find_by(id: params[:id])
     raise Discourse::InvalidParameters.new(:id) if group.blank?
@@ -650,6 +646,16 @@ class GroupsController < ApplicationController
 
   private
 
+  def add_user_to_group(group, user, notify = false)
+    group.add(user)
+    GroupActionLogger.new(current_user, group).log_add_user_to_group(user)
+    group.notify_added_to_group(user) if notify
+  rescue ActiveRecord::RecordNotUnique
+    # Under concurrency, we might attempt to insert two records quickly and hit a DB
+    # constraint. In this case we can safely ignore the error and act as if the user
+    # was added to the group.
+  end
+
   def group_params(automatic: false)
     permitted_params =
       if automatic

config/routes.rb

@@ -602,6 +602,7 @@ Discourse::Application.routes.draw do
 
           get "permissions" => "groups#permissions"
           put "members" => "groups#add_members"
+          put "join" => "groups#join"
           delete "members" => "groups#remove_member"
           post "request_membership" => "groups#request_membership"
           put "handle_membership_request" => "groups#handle_membership_request"

spec/requests/groups_controller_spec.rb

@@ -1507,26 +1507,57 @@ describe GroupsController do
             expect(group_history.target_user).to eq(other_user)
           end
         end
-
-        it 'should allow a user to join the group' do
-          sign_in(other_user)
-
-          expect do
-            put "/groups/#{group.id}/members.json",
-              params: { usernames: other_user.username }
-          end.to change { group.users.count }.by(1)
-
-          expect(response.status).to eq(200)
+      end
     end
 
-        it 'should not allow an underprivileged user to add another user to a group' do
+    context '#join' do
+      let(:public_group) { Fabricate(:public_group) }
+
+      it 'should allow a user to join a public group' do
         sign_in(user)
 
-          put "/groups/#{group.id}/members.json",
-            params: { usernames: other_user.username }
+        expect do
+          put "/groups/#{public_group.id}/join.json"
+        end.to change { public_group.users.count }.by(1)
+
+        expect(response.status).to eq(204)
+      end
+
+      it 'should not allow a user to join a nonpublic group' do
+        sign_in(user)
+
+        expect do
+          put "/groups/#{group.id}/join.json"
+        end.not_to change { group.users.count }
 
         expect(response).to be_forbidden
       end
+
+      it 'should not allow an anonymous user to call the join method' do
+        expect do
+          put "/groups/#{group.id}/join.json"
+        end.not_to change { group.users.count }
+
+        expect(response).to be_forbidden
+      end
+
+      it 'the join method is idempotent' do
+        sign_in(user)
+
+        expect do
+          put "/groups/#{public_group.id}/join.json"
+        end.to change { public_group.users.count }.by(1)
+        expect(response.status).to eq(204)
+
+        expect do
+          put "/groups/#{public_group.id}/join.json"
+        end.not_to change { public_group.users.count }
+        expect(response.status).to eq(204)
+
+        expect do
+          put "/groups/#{public_group.id}/join.json"
+        end.not_to change { public_group.users.count }
+        expect(response.status).to eq(204)
       end
     end