FEATURE: Public groups.

app/assets/javascripts/discourse/controllers/group-index.js.es6

@@ -1,6 +1,6 @@
 import { popupAjaxError } from 'discourse/lib/ajax-error';
 import Group from 'discourse/models/group';
-import { observes } from 'ember-addons/ember-computed-decorators';
+import { default as computed, observes } from 'ember-addons/ember-computed-decorators';
 
 export default Ember.Controller.extend({
   queryParams: ['order', 'desc'],
@@ -17,6 +17,11 @@ export default Ember.Controller.extend({
       this.get('model').findMembers({ order: this.get('order'), desc: this.get('desc') });
   },
 
+  @computed("model.public")
+  canJoinGroup(publicGroup) {
+    return !!(this.currentUser) && publicGroup;
+  },
+
   actions: {
     removeMember(user) {
       this.get('model').removeMember(user);
@@ -29,6 +34,28 @@ export default Ember.Controller.extend({
       }
     },
 
+    joinGroup() {
+      this.set('updatingMembership', true);
+      const model = this.get('model');
+
+      model.addMembers(this.currentUser.get('username')).then(() => {
+        model.set('is_group_user', true);
+      }).catch(popupAjaxError).finally(() => {
+        this.set('updatingMembership', false);
+      });
+    },
+
+    leaveGroup() {
+      this.set('updatingMembership', true);
+      const model = this.get('model');
+
+      model.removeMember(this.currentUser).then(() => {
+        model.set('is_group_user', false);
+      }).catch(popupAjaxError).finally(() => {
+        this.set('updatingMembership', false);
+      });
+    },
+
     loadMore() {
       if (this.get("loading")) { return; }
       if (this.get("model.members.length") >= this.get("model.user_count")) { return; }

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

@@ -114,7 +114,8 @@ const Group = Discourse.Model.extend({
       flair_url: this.get('flair_url'),
       flair_bg_color: this.get('flairBackgroundHexColor'),
       flair_color: this.get('flairHexColor'),
-      bio_raw: this.get('bio_raw')
+      bio_raw: this.get('bio_raw'),
+      public: this.get('public')
     };
   },
 

app/assets/javascripts/discourse/templates/group-index.hbs

@@ -4,6 +4,20 @@
       {{user-selector usernames=usernames placeholderKey="groups.selector_placeholder" id="user-search-selector" name="usernames"}}
       {{d-button action="addMembers" class="add" icon="plus" label="groups.add"}}
     </form>
+  {{else if canJoinGroup}}
+    {{#if model.is_group_user}}
+      {{d-button action="leaveGroup"
+          class="btn-danger group-index-leave"
+          icon="minus"
+          label="group.leave"
+          disabled=updatingMembership}}
+    {{else}}
+      {{d-button action="joinGroup"
+          class="group-index-join"
+          icon="plus"
+          label="group.join"
+          disabled=updatingMembership}}
+    {{/if}}
   {{/if}}
 
   {{#load-more selector=".group-members tr" action="loadMore"}}

app/assets/javascripts/discourse/templates/modal/edit-group.hbs

@@ -1,4 +1,4 @@
-{{#d-modal-body title="group.edit.title" class="edit-group groups"}}
+{{#d-modal-body title="group.title" class="edit-group groups"}}
   <form class="form-horizontal">
     <label for='title'>{{i18n 'group.title'}}</label>
     {{input type='text' name='title' value=model.title class='edit-group-title'}}
@@ -7,6 +7,11 @@
     {{d-editor value=model.bio_raw class="edit-group-bio"}}
 
     {{group-flair-inputs model=model}}
+
+    <label>
+      {{input type='checkbox' checked=model.public class="edit-group-public"}}
+      {{i18n 'group.public'}}
+    </label>
   </form>
 {{/d-modal-body}}
 

app/assets/stylesheets/common/base/group.scss

@@ -105,8 +105,7 @@ table.group-members {
 
   .group-flair-inputs {
     display: inline-block;
-    margin-top: 30px;
+    margin: 15px 0px;
-    margin-bottom: 30px;
 
     .group-flair-left {
       float: left;

app/controllers/groups_controller.rb

@@ -107,16 +107,29 @@ class GroupsController < ApplicationController
 
   def add_members
     group = Group.find(params[:id])
-    guardian.ensure_can_edit!(group)
+    group.public ? ensure_logged_in : guardian.ensure_can_edit!(group)
 
-    if params[:usernames].present?
+    users =
-      users = User.where(username: params[:usernames].split(","))
+      if params[:usernames].present?
-    elsif params[:user_ids].present?
+        User.where(username: params[:usernames].split(","))
-      users = User.find(params[:user_ids].split(","))
+      elsif params[:user_ids].present?
-    elsif params[:user_emails].present?
+        User.find(params[:user_ids].split(","))
-      users = User.where(email: params[:user_emails].split(","))
+      elsif params[:user_emails].present?
-    else
+        User.where(email: params[:user_emails].split(","))
-      raise Discourse::InvalidParameters.new('user_ids or usernames or user_emails must be present')
+      else
+        raise Discourse::InvalidParameters.new(
+          'user_ids or usernames or user_emails must be present'
+        )
+      end
+
+    raise Discourse::NotFound if users.blank?
+
+    if group.public
+      raise Discourse::InvalidAccess unless current_user == users.first
+
+      unless current_user.staff?
+        RateLimiter.new(current_user, "public_group_membership", 3, 1.minute).performed!
+      end
     end
 
     users.each do |user|
@@ -146,16 +159,27 @@ class GroupsController < ApplicationController
 
   def remove_member
     group = Group.find(params[:id])
-    guardian.ensure_can_edit!(group)
+    group.public ? ensure_logged_in : guardian.ensure_can_edit!(group)
 
-    if params[:user_id].present?
+    user =
-      user = User.find(params[:user_id])
+      if params[:user_id].present?
-    elsif params[:username].present?
+        User.find_by(id: params[:user_id])
-      user = User.find_by_username(params[:username])
+      elsif params[:username].present?
-    elsif params[:user_email].present?
+        User.find_by_username(params[:username])
-      user = User.find_by_email(params[:user_email])
+      elsif params[:user_email].present?
-    else
+        User.find_by_email(params[:user_email])
-      raise Discourse::InvalidParameters.new('user_id or username must be present')
+      else
+        raise Discourse::InvalidParameters.new('user_id or username must be present')
+      end
+
+    raise Discourse::NotFound unless user
+
+    if group.public
+      raise Discourse::InvalidAccess unless current_user == user
+
+      unless current_user.staff?
+        RateLimiter.new(current_user, "public_group_membership", 3, 1.minute).performed!
+      end
     end
 
     user.primary_group_id = nil if user.primary_group_id == group.id
@@ -189,7 +213,8 @@ class GroupsController < ApplicationController
       :flair_bg_color,
       :flair_color,
       :bio_raw,
-      :title
+      :title,
+      :public
     )
   end
 

app/serializers/basic_group_serializer.rb

@@ -16,7 +16,8 @@ class BasicGroupSerializer < ApplicationSerializer
              :flair_bg_color,
              :flair_color,
              :bio_raw,
-             :bio_cooked
+             :bio_cooked,
+             :public
 
   def include_incoming_email?
     staff?

config/locales/client.en.yml

@@ -1848,9 +1848,12 @@ en:
     group:
       edit:
         title: 'Edit Group'
+      join: "Join Group"
+      leave: "Leave Group"
       title: 'Title'
       name: "Name"
       bio: "About Group"
+      public: "Allow users to join/leave the group freely"
       name_placeholder: "Group name, no spaces, same as username rule"
       flair_url: "Avatar Flair Image"
       flair_url_placeholder: "(Optional) Image URL or Font Awesome class"

config/locales/server.en.yml

@@ -443,6 +443,7 @@ en:
       create_topic: "You're creating topics too quickly. Please wait %{time_left} before trying again."
       create_post: "You're replying too quickly. Please wait %{time_left} before trying again."
       delete_post: "You're deleting posts too quickly. Please wait %{time_left} before trying again."
+      public_group_membership: "You're joining/leaving groups too frequently. Please wait %{time_left} before trying again."
       topics_per_day: "You've reached the maximum number of new topics today. Please wait %{time_left} before trying again."
       pms_per_day: "You've reached the maximum number of messages today. Please wait %{time_left} before trying again."
       create_like: "You've reached the maximum number of likes today. Please wait %{time_left} before trying again."

spec/controllers/admin/groups_controller_spec.rb

@@ -39,8 +39,10 @@ describe Admin::GroupsController do
         "flair_bg_color"=>nil,
         "flair_color"=>nil,
         "bio_raw"=>nil,
-        "bio_cooked"=>nil
+        "bio_cooked"=>nil,
+        "public"=>false
       }])
+
     end
 
   end

spec/controllers/groups_controller_spec.rb

@@ -69,162 +69,6 @@ describe GroupsController do
     end
   end
 
-
-  describe "membership edit permission" do
-    it "refuses membership changes to unauthorized users" do
-      Guardian.any_instance.stubs(:can_edit?).with(group).returns(false)
-
-      xhr :put, :add_members, id: group.id, usernames: "bob"
-      expect(response).to be_forbidden
-
-      xhr :delete, :remove_member, id: group.id, username: "bob"
-      expect(response).to be_forbidden
-    end
-
-    it "cannot add members to automatic groups" do
-      Guardian.any_instance.stubs(:is_admin?).returns(true)
-      group = Fabricate(:group, name: "auto_group", automatic: true)
-
-      xhr :put, :add_members, id: group.id, usernames: "bob"
-      expect(response).to be_forbidden
-    end
-  end
-
-  describe "membership edits" do
-    before do
-      @user1 = Fabricate(:user)
-      group.add(@user1)
-      group.reload
-
-      Guardian.any_instance.stubs(:can_edit?).with(group).returns(true)
-    end
-
-    it "can make incremental adds" do
-      user2 = Fabricate(:user)
-      xhr :put, :add_members, id: group.id, usernames: user2.username
-
-      expect(response).to be_success
-      group.reload
-      expect(group.users.count).to eq(2)
-    end
-
-    it "can make incremental deletes" do
-      xhr :delete, :remove_member, id: group.id, username: @user1.username
-
-      expect(response).to be_success
-      group.reload
-      expect(group.users.count).to eq(0)
-    end
-
-  end
-
-  context ".add_members" do
-
-    before do
-      @admin = log_in(:admin)
-    end
-
-    it "cannot add members to automatic groups" do
-      xhr :put, :add_members, id: 1, usernames: "l77t"
-      expect(response.status).to eq(403)
-    end
-
-    context "is able to add several members to a group" do
-
-      let(:user1) { Fabricate(:user) }
-      let(:user2) { Fabricate(:user) }
-      let(:group) { Fabricate(:group) }
-
-      it "adds by username" do
-        xhr :put, :add_members, id: group.id, usernames: [user1.username, user2.username].join(",")
-
-        expect(response).to be_success
-        group.reload
-        expect(group.users.count).to eq(2)
-      end
-
-      it "adds by id" do
-        xhr :put, :add_members, id: group.id, user_ids: [user1.id, user2.id].join(",")
-
-        expect(response).to be_success
-        group.reload
-        expect(group.users.count).to eq(2)
-      end
-    end
-
-    it "returns 422 if member already exists" do
-      group = Fabricate(:group)
-      existing_member = Fabricate(:user)
-      group.add(existing_member)
-      group.save
-
-      xhr :put, :add_members, id: group.id, usernames: existing_member.username
-      expect(response.status).to eq(422)
-    end
-
-  end
-
-  context ".remove_member" do
-
-    before do
-      @admin = log_in(:admin)
-    end
-
-    it "cannot remove members from automatic groups" do
-      xhr :put, :remove_member, id: 1, user_id: 42
-      expect(response.status).to eq(403)
-    end
-
-    context "is able to remove a member" do
-
-      let(:user) { Fabricate(:user) }
-      let(:group) { Fabricate(:group) }
-
-      before do
-        group.add(user)
-        group.save
-      end
-
-      it "removes by id" do
-        expect do
-          xhr :delete, :remove_member, id: group.id, user_id: user.id
-
-          expect(response).to be_success
-          group.reload
-        end.to change{group.users.count}.from(1).to(0)
-      end
-
-      it "removes by username" do
-        expect do
-          xhr :delete, :remove_member, id: group.id, username: user.username
-
-          expect(response).to be_success
-          group.reload
-
-        end.to change{group.users.count}.from(1).to(0)
-      end
-
-      it "removes user.primary_group_id when user is removed from group" do
-        user.primary_group_id = group.id
-        user.save
-
-        xhr :delete, :remove_member, id: group.id, username: user.username
-
-        user.reload
-        expect(user.primary_group_id).to eq(nil)
-      end
-
-      it "removes by user_email" do
-        expect do
-          xhr :delete, :remove_member, id: group.id, user_email: user.email
-          expect(response).to be_success
-          group.reload
-        end.to change{group.users.count}.from(1).to(0)
-      end
-    end
-
-  end
-
   describe '.posts_feed' do
     it 'renders RSS' do
       get :posts_feed, group_id: group.name, format: :rss

spec/integration/groups_spec.rb

@@ -2,6 +2,7 @@ require 'rails_helper'
 
 describe "Groups" do
   let(:user) { Fabricate(:user) }
+  let(:group) { Fabricate(:group, users: [user]) }
 
   def sign_in(user)
     password = 'somecomplicatedpassword'
@@ -12,11 +13,9 @@ describe "Groups" do
   end
 
   describe "checking if a group can be mentioned" do
-    let(:group) { Fabricate(:group, name: 'test', users: [user]) }
-
     it "should return the right response" do
       sign_in(user)
-      group
+      group.update_attributes!(name: 'test')
 
       get "/groups/test/mentionable.json", { name: group.name }
 
@@ -36,7 +35,11 @@ describe "Groups" do
   end
 
   describe "group can be updated" do
-    let(:group) { Fabricate(:group, name: 'test', users: [user]) }
+    let(:group) { Fabricate(:group, name: 'test', users: [user], public: false) }
+
+    before do
+      sign_in(user)
+    end
 
     context "when user is group owner" do
       before do
@@ -50,7 +53,8 @@ describe "Groups" do
           flair_color: 'BBB',
           flair_url: 'fa-adjust',
           bio_raw: 'testing',
-          title: 'awesome team'
+          title: 'awesome team',
+          public: true
         } }
 
         expect(response).to be_success
@@ -62,6 +66,7 @@ describe "Groups" do
         expect(group.flair_url).to eq('fa-adjust')
         expect(group.bio_raw).to eq('testing')
         expect(group.title).to eq('awesome team')
+        expect(group.public).to eq(true)
       end
     end
 
@@ -145,4 +150,220 @@ describe "Groups" do
       expect(members.map { |m| m["id"] }).to eq([user1.id, user2.id])
     end
   end
+
+  describe "membership edit permissions" do
+    let(:group) { Fabricate(:group) }
+
+    context 'when user is not signed in' do
+      it 'should be fobidden' do
+        xhr :put, "/groups/#{group.id}/members", usernames: "bob"
+        expect(response).to be_forbidden
+
+        xhr :delete, "/groups/#{group.id}/members", username: "bob"
+        expect(response).to be_forbidden
+      end
+
+      context 'public group' do
+        it 'should be fobidden' do
+          group.update_attributes!(public: true)
+
+          expect { xhr :put, "/groups/#{group.id}/members", usernames: "bob" }
+            .to raise_error(Discourse::NotLoggedIn)
+
+          expect { xhr :delete, "/groups/#{group.id}/members", username: "bob" }
+            .to raise_error(Discourse::NotLoggedIn)
+        end
+      end
+    end
+
+    context 'when user is not an owner of the group' do
+      before do
+        sign_in(user)
+      end
+
+      it "refuses membership changes to unauthorized users" do
+        xhr :put, "/groups/#{group.id}/members", usernames: "bob"
+        expect(response).to be_forbidden
+
+        xhr :delete, "/groups/#{group.id}/members", username: "bob"
+        expect(response).to be_forbidden
+      end
+    end
+
+    context 'when user is an admin' do
+      let(:user) { Fabricate(:admin) }
+      let(:group) { Fabricate(:group, users: [user], automatic: true) }
+
+      before do
+        sign_in(user)
+      end
+
+      it "cannot add members to automatic groups" do
+        xhr :put, "/groups/#{group.id}/members", usernames: "bob"
+        expect(response).to be_forbidden
+
+        xhr :delete, "/groups/#{group.id}/members", username: "bob"
+        expect(response).to be_forbidden
+      end
+    end
+  end
+
+  describe "membership edits" do
+    let(:admin) { Fabricate(:admin) }
+
+    before do
+      sign_in(admin)
+    end
+
+    context 'adding members' do
+      it "can make incremental adds" do
+        user2 = Fabricate(:user)
+        expect(group.users.count).to eq(1)
+
+        xhr :put, "/groups/#{group.id}/members", usernames: user2.username
+
+        expect(response).to be_success
+        expect(group.reload.users.count).to eq(2)
+      end
+
+      it "can make incremental deletes" do
+        expect(group.users.count).to eq(1)
+
+        xhr :delete, "/groups/#{group.id}/members", username: user.username
+
+        expect(response).to be_success
+        expect(group.reload.users.count).to eq(0)
+      end
+
+      it "cannot add members to automatic groups" do
+        group.update!(automatic: true)
+
+        xhr :put, "/groups/#{group.id}/members", usernames: "l77t"
+        expect(response.status).to eq(403)
+      end
+
+      context "is able to add several members to a group" do
+        let(:user1) { Fabricate(:user) }
+        let(:user2) { Fabricate(:user) }
+
+        it "adds by username" do
+          expect { xhr :put, "/groups/#{group.id}/members", usernames: [user1.username, user2.username].join(",") }
+            .to change { group.users.count }.by(2)
+
+          expect(response).to be_success
+        end
+
+        it "adds by id" do
+          expect { xhr :put, "/groups/#{group.id}/members", user_ids: [user1.id, user2.id].join(",") }
+            .to change { group.users.count }.by(2)
+
+          expect(response).to be_success
+        end
+      end
+
+      it "returns 422 if member already exists" do
+        xhr :put, "/groups/#{group.id}/members", usernames: user.username
+
+        expect(response.status).to eq(422)
+      end
+
+      it "returns 404 if member is not found" do
+        xhr :put, "/groups/#{group.id}/members", usernames: 'some donkey'
+
+        expect(response.status).to eq(404)
+      end
+
+      context 'public group' do
+        let(:other_user) { Fabricate(:user) }
+
+        before do
+          group.update!(public: true)
+        end
+
+        it 'should allow a user to join the group' do
+          sign_in(other_user)
+
+          expect { xhr :put, "/groups/#{group.id}/members", usernames: other_user.username }
+            .to change { group.users.count }.by(1)
+
+          expect(response).to be_success
+        end
+
+        it 'should not allow a user to add another user to a group' do
+          xhr :put, "/groups/#{group.id}/members", usernames: other_user.username
+
+          expect(response).to be_forbidden
+        end
+      end
+    end
+
+    context 'removing members' do
+      it "cannot remove members from automatic groups" do
+        group.update!(automatic: true)
+
+        xhr :delete, "/groups/#{group.id}/members", user_id: 42
+        expect(response.status).to eq(403)
+      end
+
+      it "raises an error if user to be removed is not found" do
+        xhr :delete, "/groups/#{group.id}/members", user_id: -10
+        expect(response.status).to eq(404)
+      end
+
+      context "is able to remove a member" do
+        it "removes by id" do
+          expect { xhr :delete, "/groups/#{group.id}/members", user_id: user.id }
+            .to change { group.users.count }.by(-1)
+
+          expect(response).to be_success
+        end
+
+        it "removes by username" do
+          expect { xhr :delete, "/groups/#{group.id}/members", username: user.username }
+            .to change { group.users.count }.by(-1)
+
+          expect(response).to be_success
+        end
+
+        it "removes user.primary_group_id when user is removed from group" do
+          user.update!(primary_group_id: group.id)
+
+          xhr :delete, "/groups/#{group.id}/members", user_id: user.id
+
+          expect(user.reload.primary_group_id).to eq(nil)
+        end
+
+        it "removes by user_email" do
+          expect { xhr :delete, "/groups/#{group.id}/members", user_email: user.email }
+            .to change { group.users.count }.by(-1)
+
+          expect(response).to be_success
+        end
+
+        context 'public group' do
+          let(:other_user) { Fabricate(:user) }
+          let(:group) { Fabricate(:group, users: [other_user]) }
+
+          before do
+            group.update!(public: true)
+          end
+
+          it 'should allow a user to leave a group' do
+            sign_in(other_user)
+
+            expect { xhr :delete, "/groups/#{group.id}/members", username: other_user.username }
+              .to change { group.users.count }.by(-1)
+
+            expect(response).to be_success
+          end
+
+          it 'should not allow a user to leave a group for another user' do
+            xhr :delete, "/groups/#{group.id}/members", username: other_user.username
+
+            expect(response).to be_forbidden
+          end
+        end
+      end
+    end
+  end
 end

test/javascripts/acceptance/groups-test.js.es6

@@ -50,5 +50,6 @@ test("Admin Browsing Groups", () => {
     ok(find('.group-flair-inputs').length === 1, 'it should display avatar flair inputs');
     ok(find('.edit-group-bio').length === 1, 'it should display group bio input');
     ok(find('.edit-group-title').length === 1, 'it should display group title input');
+    ok(find('.edit-group-public').length === 1, 'it should display group public input');
   });
 });

test/javascripts/controllers/group-index-test.js.es6

@@ -0,0 +1,21 @@
+import { currentUser } from "helpers/qunit-helpers";
+
+moduleFor("controller:group-index");
+
+test("canJoinGroup", function() {
+  this.subject().setProperties({
+    model: { public: false }
+  });
+
+  this.subject().set("currentUser", currentUser());
+
+  equal(this.subject().get("canJoinGroup"), false, "non public group cannot be joined");
+
+  this.subject().set("model.public", true);
+
+  equal(this.subject().get("canJoinGroup"), true, "public group can be joined");
+
+  this.subject().setProperties({ currentUser: null, model: { public: true } });
+
+  equal(this.subject().get("canJoinGroup"), false, "can't join group when not logged in");
+});