DEV: extract leave_group method from the group#remove_member method (#13823)

* Copy remove_member to new `leave` method

* Remove unneeded code from the leave method

* Rearrange the leave method

* Remove unneeded code from the remove_member method

* Add tests

* Implement on the client side

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

@@ -37,7 +37,7 @@ export default Component.extend({
   removeFromGroup() {
     const model = this.model;
     model
-      .removeMember(this.currentUser)
+      .leave()
       .then(() => {
         model.set("is_group_user", false);
         this.appEvents.trigger("group:leave", model);

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

@@ -114,6 +114,12 @@ const Group = RestModel.extend({
     }).then(() => this.findMembers(params, true));
   },
 
+  leave() {
+    return ajax(`/groups/${this.id}/leave.json`, {
+      type: "DELETE",
+    }).then(() => this.findMembers({}, true));
+  },
+
   addMembers(usernames, filter, notifyUsers, emails = []) {
     return ajax(`/groups/${this.id}/members.json`, {
       type: "PUT",

app/controllers/groups_controller.rb

@@ -464,7 +464,7 @@ class GroupsController < ApplicationController
   def remove_member
     group = Group.find_by(id: params[:id])
     raise Discourse::NotFound unless group
-    group.public_exit ? ensure_logged_in : guardian.ensure_can_edit!(group)
+    guardian.ensure_can_edit!(group)
 
     # Maintain backwards compatibility
     params[:usernames] = params[:username] if params[:username].present?
@@ -475,16 +475,6 @@ class GroupsController < ApplicationController
       'user_ids or usernames or user_emails must be present'
     ) if users.empty?
 
-    if group.public_exit
-      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
-
     removed_users = []
     skipped_users = []
 
@@ -507,6 +497,21 @@ class GroupsController < ApplicationController
     )
   end
 
+  def leave
+    ensure_logged_in
+    unless current_user.staff?
+      RateLimiter.new(current_user, "public_group_membership", 3, 1.minute).performed!
+    end
+
+    group = Group.find_by(id: params[:id])
+    raise Discourse::NotFound unless group
+    raise Discourse::InvalidAccess unless group.public_exit
+
+    if group.remove(current_user)
+      GroupActionLogger.new(current_user, group).log_remove_user_from_group(current_user)
+    end
+  end
+
   MAX_NOTIFIED_OWNERS ||= 20
 
   def request_membership

config/routes.rb

@@ -604,6 +604,7 @@ Discourse::Application.routes.draw do
           put "members" => "groups#add_members"
           put "join" => "groups#join"
           delete "members" => "groups#remove_member"
+          delete "leave" => "groups#leave"
           post "request_membership" => "groups#request_membership"
           put "handle_membership_request" => "groups#handle_membership_request"
           post "notifications" => "groups#set_notifications"

spec/requests/groups_controller_spec.rb

@@ -1234,11 +1234,11 @@ describe GroupsController do
   describe "membership edits" do
     fab!(:admin) { Fabricate(:admin) }
 
-    before do
-      sign_in(admin)
-    end
-
     context '#add_members' do
+      before do
+        sign_in(admin)
+      end
+
       it "can make incremental adds" do
         user2 = Fabricate(:user)
 
@@ -1562,6 +1562,10 @@ describe GroupsController do
     end
 
     context '#remove_member' do
+      before do
+        sign_in(admin)
+      end
+
       it "cannot remove members from automatic groups" do
         group.update!(automatic: true)
 
@@ -1641,17 +1645,6 @@ describe GroupsController do
             end
           end
 
-          it 'should allow a user to leave a group' do
-            sign_in(other_user)
-
-            expect do
-              delete "/groups/#{group.id}/members.json",
-              params: { username: other_user.username }
-            end.to change { group.users.count }.by(-1)
-
-            expect(response.status).to eq(200)
-          end
-
           it 'should not allow a underprivileged user to leave a group for another user' do
             sign_in(user)
 
@@ -1716,6 +1709,57 @@ describe GroupsController do
         end
       end
     end
+
+    context '#leave' do
+      let(:group_with_public_exit) { Fabricate(:group, public_exit: true, users: [user]) }
+
+      it 'should allow a user to leave a group with public exit' do
+        sign_in(user)
+
+        expect do
+          delete "/groups/#{group_with_public_exit.id}/leave.json"
+        end.to change { group_with_public_exit.users.count }.by(-1)
+
+        expect(response.status).to eq(204)
+      end
+
+      it 'should not allow a user to leave a group without public exit' do
+        sign_in(user)
+
+        expect do
+          delete "/groups/#{group.id}/leave.json"
+        end.not_to change { group.users.count }
+
+        expect(response).to be_forbidden
+      end
+
+      it 'should not allow an anonymous user to call the leave method' do
+        expect do
+          delete "/groups/#{group_with_public_exit.id}/leave.json"
+        end.not_to change { group_with_public_exit.users.count }
+
+        expect(response).to be_forbidden
+      end
+
+      it 'the leave method is idempotent' do
+        sign_in(user)
+
+        expect do
+          delete "/groups/#{group_with_public_exit.id}/leave.json"
+        end.to change { group_with_public_exit.users.count }.by(-1)
+        expect(response.status).to eq(204)
+
+        expect do
+          delete "/groups/#{group_with_public_exit.id}/leave.json"
+        end.not_to change { group_with_public_exit.users.count }
+        expect(response.status).to eq(204)
+
+        expect do
+          delete "/groups/#{group_with_public_exit.id}/leave.json"
+        end.not_to change { group_with_public_exit.users.count }
+        expect(response.status).to eq(204)
+      end
+    end
   end
 
   describe "#handle_membership_request" do