Merge pull request #4592 from tgxworld/feature_public_groups

FEATURE: Public groups

app/assets/javascripts/admin/templates/group.hbs

@@ -4,15 +4,15 @@
     {{#if model.automatic}}
       <h3>{{model.name}}</h3>
     {{else}}
-      <label for="name">{{i18n 'group.name'}}</label>
+      <label for="name">{{i18n 'groups.name'}}</label>
-      {{text-field name="name" value=model.name placeholderKey="group.name_placeholder"}}
+      {{text-field name="name" value=model.name placeholderKey="groups.name_placeholder"}}
     {{/if}}
   </div>
 
   {{#if model.id}}
     {{#unless model.automatic}}
       <div>
-        <label for="bio">{{i18n 'group.bio'}}</label>
+        <label for="bio">{{i18n 'groups.bio'}}</label>
         {{d-editor value=model.bio_raw}}
       </div>
 
@@ -63,12 +63,19 @@
   </div>
 
   {{#unless model.automatic}}
-  <div>
+    <div>
-    <label for="primary_group">
+      <label for="primary_group">
-      {{input type="checkbox" checked=model.primary_group}}
+        {{input type="checkbox" checked=model.primary_group}}
-      {{i18n 'admin.groups.primary_group'}}
+        {{i18n 'admin.groups.primary_group'}}
-    </label>
+      </label>
-  </div>
+    </div>
+
+    <div>
+      <label>
+        {{input type="checkbox" checked=model.public}}
+        {{i18n 'groups.public'}}
+      </label>
+    </div>
   {{/unless}}
 
   <div>

app/assets/javascripts/discourse/components/group-flair-inputs.js.es6

@@ -45,6 +45,6 @@ export default Ember.Component.extend({
   @computed('flairPreviewImage')
   flairPreviewLabel(flairPreviewImage) {
     const key = flairPreviewImage ? 'image' : 'icon';
-    return I18n.t(`group.flair_preview_${key}`);
+    return I18n.t(`groups.flair_preview_${key}`);
   }
 });

app/assets/javascripts/discourse/components/group-logs-filter.js.es6

@@ -0,0 +1,21 @@
+import computed from 'ember-addons/ember-computed-decorators';
+
+export default Ember.Component.extend({
+  tagName: '',
+
+  @computed('type')
+  label(type) {
+    return I18n.t(`groups.logs.${type}`);
+  },
+
+  @computed('value', 'type')
+  filterText(value, type) {
+    return type === 'action' ? I18n.t(`group_histories.actions.${value}`) : value;
+  },
+
+  actions: {
+    clearFilter(param) {
+      this.sendAction("clearFilter", param);
+    }
+  }
+});

app/assets/javascripts/discourse/components/group-logs-row.js.es6

@@ -0,0 +1,14 @@
+export default Ember.Component.extend({
+  tagName: '',
+  expandDetails: false,
+
+  actions: {
+    toggleDetails() {
+      this.toggleProperty('expandDetails');
+    },
+
+    filter(params) {
+      this.set(`filters.${params.key}`, params.value);
+    }
+  }
+});

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/controllers/group-logs.js.es6

@@ -0,0 +1,57 @@
+import { default as computed, observes } from 'ember-addons/ember-computed-decorators';
+
+export default Ember.Controller.extend({
+  group: Ember.inject.controller(),
+  loading: false,
+  offset: 0,
+
+  init() {
+    this._super();
+    this.set('filters', Ember.Object.create());
+  },
+
+  @computed('filters.action', 'filters.acting_user', 'filters.target_user', 'filters.subject')
+  filterParams(action, acting_user, target_user, subject) {
+    return { action, acting_user, target_user, subject };
+  },
+
+  @observes('filters.action', 'filters.acting_user', 'filters.target_user', 'filters.subject')
+  _refreshModel() {
+    this.get('group.model').findLogs(0, this.get('filterParams')).then(results => {
+      this.set('offset', 0);
+
+      this.get('model').setProperties({
+        logs: results.logs,
+        all_loaded: results.all_loaded
+      });
+    });
+  },
+
+  reset() {
+    this.setProperties({
+      offset: 0,
+      filters: Ember.Object.create()
+    });
+  },
+
+  actions: {
+    loadMore() {
+      if (this.get('model.all_loaded')) return;
+
+      this.set('loading', true);
+
+      this.get('group.model').findLogs(this.get('offset') + 1, this.get('filterParams'))
+        .then(results => {
+          results.logs.forEach(result => this.get('model.logs').addObject(result));
+          this.incrementProperty('offset');
+          this.set('model.all_loaded', results.all_loaded);
+        }).finally(() => {
+          this.set('loading', false);
+        });
+    },
+
+    clearFilter(key) {
+      this.set(`filters.${key}`, '');
+    }
+  }
+});

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

@@ -6,9 +6,9 @@ var Tab = Em.Object.extend({
     return 'group.' + name;
   },
 
-  @computed('name')
+  @computed('name', 'i18nKey')
-  message(name) {
+  message(name, i18nKey) {
-    return I18n.t('groups.' + name);
+    return I18n.t(`groups.${i18nKey || name}`);
   }
 });
 
@@ -20,7 +20,8 @@ export default Ember.Controller.extend({
     Tab.create({ name: 'posts' }),
     Tab.create({ name: 'topics' }),
     Tab.create({ name: 'mentions' }),
-    Tab.create({ name: 'messages', requiresMembership: true })
+    Tab.create({ name: 'messages', requiresMembership: true }),
+    Tab.create({ name: 'logs', i18nKey: 'logs.title', icon: 'shield', requiresMembership: true })
   ],
 
   @computed('model.is_group_owner', 'model.automatic')

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

@@ -0,0 +1,9 @@
+import computed from 'ember-addons/ember-computed-decorators';
+import RestModel from 'discourse/models/rest';
+
+export default RestModel.extend({
+  @computed('action')
+  actionTitle(action) {
+    return I18n.t(`group_histories.actions.${action}`);
+  }
+});

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

@@ -1,5 +1,6 @@
 import { ajax } from 'discourse/lib/ajax';
 import computed from "ember-addons/ember-computed-decorators";
+import GroupHistory from 'discourse/models/group-history';
 
 const Group = Discourse.Model.extend({
   limit: 50,
@@ -114,7 +115,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')
     };
   },
 
@@ -140,6 +142,15 @@ const Group = Discourse.Model.extend({
     return ajax("/admin/groups/" + this.get('id'), { type: "DELETE" });
   },
 
+  findLogs(offset, filters) {
+    return ajax(`/groups/${this.get('name')}/logs.json`, { data: { offset, filters } }).then(results => {
+      return Ember.Object.create({
+        logs: results["logs"].map(log => GroupHistory.create(log)),
+        all_loaded: results["all_loaded"]
+      });
+    });
+  },
+
   findPosts(opts) {
     opts = opts || {};
 

app/assets/javascripts/discourse/routes/app-route-map.js.es6

@@ -56,6 +56,7 @@ export default function() {
     this.route('topics');
     this.route('mentions');
     this.route('messages');
+    this.route('logs');
   });
 
   // User routes

app/assets/javascripts/discourse/routes/group-logs.js.es6

@@ -0,0 +1,20 @@
+export default Ember.Route.extend({
+  titleToken() {
+    return I18n.t('groups.logs');
+  },
+
+  model() {
+    return this.modelFor('group').findLogs();
+  },
+
+  setupController(controller, model) {
+    this.controllerFor('group-logs').setProperties({ model });
+    this.controllerFor("group").set("showing", 'logs');
+  },
+
+  actions: {
+    willTransition() {
+      this.controllerFor('group-logs').reset();
+    }
+  }
+});

app/assets/javascripts/discourse/templates/components/group-flair-inputs.hbs

@@ -1,31 +1,31 @@
 <div class="group-flair-left">
   <div>
-    <label for="flair_url">{{i18n 'group.flair_url'}}</label>
+    <label for="flair_url">{{i18n 'groups.flair_url'}}</label>
     {{text-field name="flair_url"
         value=model.flair_url
-        placeholderKey="group.flair_url_placeholder"}}
+        placeholderKey="groups.flair_url_placeholder"}}
   </div>
 
   <div>
-    <label for="flair_bg_color">{{i18n 'group.flair_bg_color'}}</label>
+    <label for="flair_bg_color">{{i18n 'groups.flair_bg_color'}}</label>
     {{text-field name="flair_bg_color"
         class="group-flair-bg-color"
         value=model.flair_bg_color
-        placeholderKey="group.flair_bg_color_placeholder"}}
+        placeholderKey="groups.flair_bg_color_placeholder"}}
   </div>
 
   {{#if flairPreviewIcon}}
     <div>
-      <label for="flair_color">{{i18n 'group.flair_color'}}</label>
+      <label for="flair_color">{{i18n 'groups.flair_color'}}</label>
       {{text-field name="flair_color"
           class="group-flair-color"
           value=model.flair_color
-          placeholderKey="group.flair_color_placeholder"}}
+          placeholderKey="groups.flair_color_placeholder"}}
     </div>
   {{/if}}
 
   <div>
-    <strong>{{i18n 'group.flair_note'}}</strong>
+    <strong>{{i18n 'groups.flair_note'}}</strong>
   </div>
 </div>
 

app/assets/javascripts/discourse/templates/components/group-logs-filter.hbs

@@ -0,0 +1,6 @@
+{{#if value}}
+  {{#d-button class="btn-small group-logs-filter" action="clearFilter" actionParam=type}}
+    <span>{{label}}</span>: {{filterText}}
+    {{fa-icon "times-circle"}}
+  {{/d-button}}
+{{/if}}

app/assets/javascripts/discourse/templates/components/group-logs-row.hbs

@@ -0,0 +1,59 @@
+<tr class="group-logs-row">
+  <td>
+    {{#d-button class="btn-small" action="filter" actionParam=(hash value=log.action key="action")}}
+      {{log.actionTitle}}
+    {{/d-button}}
+  </td>
+
+  <td>
+    <span>{{avatar log.acting_user imageSize='tiny'}}</span>
+
+    {{#d-button class="btn-small" action="filter" actionParam=(hash value=log.acting_user.username key="acting_user")}}
+      {{log.acting_user.username}}
+    {{/d-button}}
+  </td>
+
+  <td>
+    {{#if log.target_user}}
+      <span>{{avatar log.target_user imageSize='tiny'}}</span>
+
+      {{#d-button class="btn-small" action="filter" actionParam=(hash value=log.target_user.username key="target_user")}}
+        {{log.target_user.username}}
+      {{/d-button}}
+    {{/if}}
+  </td>
+
+  <td>
+    {{#if log.subject}}
+      {{#d-button class="btn-small" action="filter" actionParam=(hash value=log.subject key="subject")}}
+        {{log.subject}}
+      {{/d-button}}
+    {{/if}}
+  </td>
+
+  <td>{{bound-date log.created_at}}</td>
+
+  <td {{action "toggleDetails"}} class="group-logs-expand-details">
+    {{#if log.prev_value}}
+      {{#if expandDetails}}
+        {{fa-icon 'ellipsis-v'}}
+      {{else}}
+        {{fa-icon 'ellipsis-h'}}
+      {{/if}}
+    {{/if}}
+  </td>
+</tr>
+
+{{#if expandDetails}}
+  <tr>
+    <td colspan='6'>
+      <p>
+        <strong>{{i18n 'groups.logs.from'}}</strong>: <code>{{log.prev_value}}</code>
+      </p>
+
+      <p>
+        <strong>{{i18n 'groups.logs.to'}}</strong>: <code>{{log.new_value}}</code>
+      </p>
+    </td>
+  </tr>
+{{/if}}

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="groups.leave"
+          disabled=updatingMembership}}
+    {{else}}
+      {{d-button action="joinGroup"
+          class="group-index-join"
+          icon="plus"
+          label="groups.join"
+          disabled=updatingMembership}}
+    {{/if}}
   {{/if}}
 
   {{#load-more selector=".group-members tr" action="loadMore"}}

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

@@ -0,0 +1,33 @@
+{{#if model}}
+  <div class="group-logs-controls">
+    {{group-logs-filter clearFilter="clearFilter" value=filters.action type="action"}}
+    {{group-logs-filter clearFilter="clearFilter" value=filters.acting_user type="acting_user"}}
+    {{group-logs-filter clearFilter="clearFilter" value=filters.target_user type="target_user"}}
+    {{group-logs-filter clearFilter="clearFilter" value=filters.subject type="subject"}}
+  </div>
+
+  {{#load-more selector=".group-logs .group-logs-row" action="loadMore"}}
+    <table class="group-logs">
+      <thead>
+        <th>{{i18n 'groups.logs.action'}}</th>
+        <th>{{i18n 'groups.logs.acting_user'}}</th>
+        <th>{{i18n 'groups.logs.target_user'}}</th>
+        <th>{{i18n 'groups.logs.subject'}}</th>
+        <th>{{i18n 'groups.logs.when'}}</th>
+        <th></th>
+      </thead>
+
+      <tbody>
+        {{#each model.logs as |log|}}
+          {{group-logs-row
+              log=log
+              filters=filters}}
+        {{/each}}
+      </tbody>
+    </table>
+  {{/load-more}}
+
+  {{conditional-loading-spinner condition=loading}}
+{{else}}
+  <div>{{i18n "groups.empty.logs"}}</div>
+{{/if}}

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

@@ -23,7 +23,7 @@
 
       {{#if canEditGroup}}
         <span class="group-edit">
-          {{d-button action="showGroupEditor" label="group.edit.title" class="group-edit-btn" icon="pencil"}}
+          {{d-button action="showGroupEditor" label="groups.edit.title" class="group-edit-btn" icon="pencil"}}
         </span>
       {{/if}}
     </div>
@@ -41,6 +41,7 @@
     {{#each getTabs as |tab|}}
       <li class="{{if tab.active 'active'}}">
         {{#link-to tab.location model title=tab.message}}
+          {{#if tab.icon}}{{fa-icon tab.icon}}{{/if}}
           {{tab.message}}
           {{#if tab.count}}<span class='count'>({{tab.count}})</span>{{/if}}
         {{/link-to}}

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

@@ -1,12 +1,17 @@
-{{#d-modal-body title="group.edit.title" class="edit-group groups"}}
+{{#d-modal-body title="groups.edit.title" class="edit-group groups"}}
   <form class="form-horizontal">
-    <label for='title'>{{i18n 'group.title'}}</label>
+    <label for='title'>{{i18n 'groups.edit.group_title'}}</label>
     {{input type='text' name='title' value=model.title class='edit-group-title'}}
 
-    <label for='bio'>{{i18n 'group.bio'}}</label>
+    <label for='bio'>{{i18n 'groups.bio'}}</label>
     {{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 'groups.public'}}
+    </label>
   </form>
 {{/d-modal-body}}
 

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

@@ -15,6 +15,39 @@
   margin-bottom: 30px;
 }
 
+.group-logs-filter {
+  margin-right: 10px;
+
+  &:hover {
+    background-color: $danger;
+  }
+}
+
+table.group-logs {
+  width: 100%;
+
+  th, tr {
+    border-bottom: 1px solid dark-light-diff($primary, $secondary, 90%, -60%);
+  }
+
+  th {
+    text-align: left;
+    padding: 5px 0px;
+  }
+
+  td {
+    padding: 10px 0px;
+  }
+
+  .group-logs-expand-details {
+    cursor: pointer;
+
+    i {
+      color: dark-light-diff($primary, $secondary, 50%, -50%);
+    }
+  }
+}
+
 table.group-members {
   width: 100%;
   table-layout: fixed;
@@ -89,24 +122,15 @@ table.group-members {
   float: right;
 }
 
-.groups.edit-group .form-horizontal {
+.form-horizontal {
-  textarea {
-    width: 99%;
-  }
-
-  label {
-    font-weight: bold;
-  }
-
-  input[type="text"] {
-    width: 80% !important;
-    margin-bottom: 10px;
-  }
-
   .group-flair-inputs {
     display: inline-block;
-    margin-top: 30px;
+    margin: 15px 0px;
-    margin-bottom: 30px;
+
+    input[type="text"] {
+      width: 80% !important;
+      margin-bottom: 10px;
+    }
 
     .group-flair-left {
       float: left;
@@ -128,6 +152,16 @@ table.group-members {
   }
 }
 
+.groups.edit-group .form-horizontal {
+  textarea {
+    width: 99%;
+  }
+
+  label {
+    font-weight: bold;
+  }
+}
+
 #add-user-to-group {
   margin: 0px;
 

app/assets/stylesheets/mobile/group.scss

@@ -32,6 +32,10 @@
   background-color: $quaternary;
 }
 
+table.group-logs {
+  width: 130%;
+}
+
 table.group-members {
   th {
     text-align: center;

app/controllers/admin/groups_controller.rb

@@ -45,7 +45,7 @@ class Admin::GroupsController < Admin::AdminController
 
     # group rename is ignored for automatic groups
     group.name = group_params[:name] if group_params[:name] && !group.automatic
-    save_group(group)
+    save_group(group) { |g| GroupActionLogger.new(current_user, g).log_change_group_settings }
   end
 
   def save_group(group)
@@ -67,9 +67,14 @@ class Admin::GroupsController < Admin::AdminController
     group.flair_url      = group_params[:flair_url].presence
     group.flair_bg_color = group_params[:flair_bg_color].presence
     group.flair_color    = group_params[:flair_color].presence
+    group.public = group_params[:public] if group_params[:public]
+    group.bio_raw = group_params[:bio_raw] if group_params[:bio_raw]
 
     if group.save
       Group.reset_counters(group.id, :group_users)
+
+      yield(group) if block_given?
+
       render_serialized(group, BasicGroupSerializer)
     else
       render_json_error group
@@ -99,10 +104,14 @@ class Admin::GroupsController < Admin::AdminController
     users = User.where(username: params[:usernames].split(","))
 
     users.each do |user|
+      group_action_logger = GroupActionLogger.new(current_user, group)
+
       if !group.users.include?(user)
         group.add(user)
+        group_action_logger.log_add_user_to_group(user)
       end
       group.group_users.where(user_id: user.id).update_all(owner: true)
+      group_action_logger.log_make_user_group_owner(user)
     end
 
     Group.reset_counters(group.id, :group_users)
@@ -116,6 +125,7 @@ class Admin::GroupsController < Admin::AdminController
 
     user = User.find(params[:user_id].to_i)
     group.group_users.where(user_id: user.id).update_all(owner: false)
+    GroupActionLogger.new(current_user, group).log_remove_user_as_group_owner(user)
 
     Group.reset_counters(group.id, :group_users)
 
@@ -135,7 +145,7 @@ class Admin::GroupsController < Admin::AdminController
       :name, :alias_level, :visible, :automatic_membership_email_domains,
       :automatic_membership_retroactive, :title, :primary_group,
       :grant_trust_level, :incoming_email, :flair_url, :flair_bg_color,
-      :flair_color
+      :flair_color, :bio_raw, :public
     )
   end
 end

app/controllers/admin/users_controller.rb

@@ -127,8 +127,8 @@ class Admin::UsersController < Admin::AdminController
     group = Group.find(params[:group_id].to_i)
     return render_json_error group unless group && !group.automatic
 
-    # We don't care about duplicate group assignment
+    group.add(@user)
-    group.users << @user rescue ActiveRecord::RecordNotUnique
+    GroupActionLogger.new(current_user, group).log_add_user_to_group(@user)
 
     render nothing: true
   end
@@ -136,7 +136,8 @@ class Admin::UsersController < Admin::AdminController
   def remove_group
     group = Group.find(params[:group_id].to_i)
     return render_json_error group unless group && !group.automatic
-    group.users.delete(@user)
+    group.remove(@user)
+    GroupActionLogger.new(current_user, group).log_remove_user_from_group(user)
     render nothing: true
   end
 

app/controllers/groups_controller.rb

@@ -3,7 +3,9 @@ class GroupsController < ApplicationController
   before_filter :ensure_logged_in, only: [
     :set_notifications,
     :mentionable,
-    :update
+    :update,
+    :messages,
+    :histories
   ]
 
   skip_before_filter :preload_json, :check_xhr, only: [:posts_feed, :mentions_feed]
@@ -17,6 +19,8 @@ class GroupsController < ApplicationController
     guardian.ensure_can_edit!(group)
 
     if group.update_attributes(group_params)
+      GroupActionLogger.new(current_user, group).log_change_group_settings
+
       render json: success_json
     else
       render_json_error(group)
@@ -107,21 +111,35 @@ 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|
       if !group.users.include?(user)
         group.add(user)
+        GroupActionLogger.new(current_user, group).log_add_user_to_group(user)
       else
         return render_json_error I18n.t('groups.errors.member_already_exist', username: user.username)
       end
@@ -146,21 +164,33 @@ 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
 
-    group.users.delete(user.id)
+    group.remove(user)
+    GroupActionLogger.new(current_user, group).log_remove_user_from_group(user)
 
     if group.save && user.save
       render json: success_json
@@ -181,6 +211,23 @@ class GroupsController < ApplicationController
     render json: success_json
   end
 
+  def histories
+    group = find_group(:group_id)
+    guardian.ensure_can_edit!(group)
+
+    page_size = 25
+    offset = (params[:offset] && params[:offset].to_i) || 0
+
+    group_histories = GroupHistory.with_filters(group, params[:filters])
+      .limit(page_size)
+      .offset(offset * page_size)
+
+    render_json_dump(
+      logs: serialize_data(group_histories, BasicGroupHistorySerializer),
+      all_loaded: group_histories.count < page_size
+    )
+  end
+
   private
 
   def group_params
@@ -189,7 +236,8 @@ class GroupsController < ApplicationController
       :flair_bg_color,
       :flair_color,
       :bio_raw,
-      :title
+      :title,
+      :public
     )
   end
 

app/jobs/regular/automatic_group_membership.rb

@@ -16,11 +16,9 @@ module Jobs
       User.where("email ~* '@(#{domains})$'")
           .where("users.id NOT IN (SELECT user_id FROM group_users WHERE group_users.group_id = ?)", group_id)
           .find_each do |user|
-        begin
+
           group.add(user)
-        rescue ActiveRecord::RecordNotUnique, PG::UniqueViolation
+          GroupActionLogger.new(Discourse.system_user, group).log_add_user_to_group(user)
-          # we don't care about this
-        end
       end
 
       Group.reset_counters(group.id, :group_users)

app/models/group.rb

@@ -9,6 +9,7 @@ class Group < ActiveRecord::Base
 
   has_many :categories, through: :category_groups
   has_many :users, through: :group_users
+  has_many :group_histories, dependent: :destroy
 
   has_and_belongs_to_many :web_hooks
 
@@ -347,7 +348,8 @@ class Group < ActiveRecord::Base
   end
 
   def add(user)
-    self.users.push(user)
+    self.users.push(user) unless self.users.include?(user)
+    self
   end
 
   def remove(user)
@@ -508,6 +510,9 @@ end
 #  flair_url                          :string
 #  flair_bg_color                     :string
 #  flair_color                        :string
+#  bio_raw                            :text
+#  bio_cooked                         :text
+#  public                             :boolean          default(FALSE), not null
 #
 # Indexes
 #

app/models/group_history.rb

@@ -0,0 +1,72 @@
+class GroupHistory < ActiveRecord::Base
+  belongs_to :group
+  belongs_to :acting_user, class_name: 'User'
+  belongs_to :target_user, class_name: 'User'
+
+  validates :acting_user_id, presence: true
+  validates :group_id, presence: true
+  validates :action, presence: true
+
+  def self.actions
+    @actions ||= Enum.new(
+      change_group_setting: 1,
+      add_user_to_group: 2,
+      remove_user_from_group: 3,
+      make_user_group_owner: 4,
+      remove_user_as_group_owner: 5
+    )
+  end
+
+  def self.filters
+    [
+      :acting_user,
+      :target_user,
+      :action,
+      :subject
+    ]
+  end
+
+  def self.with_filters(group, params = {})
+    records = self.includes(:acting_user, :target_user)
+      .where(group_id: group.id)
+      .order('group_histories.created_at DESC')
+
+    if !params.blank?
+      params = params.slice(*filters)
+      records = records.where(action: self.actions[params[:action].to_sym]) unless params[:action].blank?
+      records = records.where(subject: params[:subject]) unless params[:subject].blank?
+
+      [:acting_user, :target_user].each do |filter|
+        unless params[filter].blank?
+          id = User.where(username_lower: params[filter]).pluck(:id)
+          records = records.where("#{filter}_id" => id)
+        end
+      end
+    end
+
+    records
+  end
+end
+
+# == Schema Information
+#
+# Table name: group_histories
+#
+#  id             :integer          not null, primary key
+#  group_id       :integer          not null
+#  acting_user_id :integer          not null
+#  target_user_id :integer
+#  action         :integer          not null
+#  subject        :string
+#  prev_value     :text
+#  new_value      :text
+#  created_at     :datetime         not null
+#  updated_at     :datetime         not null
+#
+# Indexes
+#
+#  index_group_histories_on_acting_user_id  (acting_user_id)
+#  index_group_histories_on_action          (action)
+#  index_group_histories_on_group_id        (group_id)
+#  index_group_histories_on_target_user_id  (target_user_id)
+#

app/models/invite.rb

@@ -55,19 +55,15 @@ class Invite < ActiveRecord::Base
     InviteRedeemer.new(self).redeem unless expired? || destroyed? || !link_valid?
   end
 
-
-  def add_groups_for_topic(topic)
-    if topic.category
-      (topic.category.groups - groups).each { |group| group.add(user) }
-    end
-  end
-
   def self.extend_permissions(topic, user, invited_by)
     if topic.private_message?
       topic.grant_permission_to_user(user.email)
     elsif topic.category && topic.category.groups.any?
       if Guardian.new(invited_by).can_invite_to?(topic) && !SiteSetting.enable_sso
-        (topic.category.groups - user.groups).each { |group| group.add(user) }
+        (topic.category.groups - user.groups).each do |group|
+          group.add(user)
+          GroupActionLogger.new(Discourse.system_user, group).log_add_user_to_group(user)
+        end
       end
     end
   end

app/models/topic.rb

@@ -1231,10 +1231,8 @@ end
 #  deleted_at                    :datetime
 #  highest_post_number           :integer          default(0), not null
 #  image_url                     :string
-#  off_topic_count               :integer          default(0), not null
 #  like_count                    :integer          default(0), not null
 #  incoming_link_count           :integer          default(0), not null
-#  bookmark_count                :integer          default(0), not null
 #  category_id                   :integer
 #  visible                       :boolean          default(TRUE), not null
 #  moderator_posts_count         :integer          default(0), not null
@@ -1247,12 +1245,9 @@ end
 #  featured_user4_id             :integer
 #  notify_moderators_count       :integer          default(0), not null
 #  spam_count                    :integer          default(0), not null
-#  illegal_count                 :integer          default(0), not null
-#  inappropriate_count           :integer          default(0), not null
 #  pinned_at                     :datetime
 #  score                         :float
 #  percent_rank                  :float            default(1.0), not null
-#  notify_user_count             :integer          default(0), not null
 #  subtype                       :string
 #  slug                          :string
 #  auto_close_at                 :datetime
@@ -1267,6 +1262,7 @@ end
 #  auto_close_hours              :float
 #  pinned_until                  :datetime
 #  fancy_title                   :string(400)
+#  highest_staff_post_number     :integer          default(0), not null
 #
 # Indexes
 #

app/models/user.rb

@@ -66,6 +66,9 @@ class User < ActiveRecord::Base
 
   belongs_to :uploaded_avatar, class_name: 'Upload'
 
+  has_many :acting_group_histories, dependent: :destroy, foreign_key: :acting_user_id, class_name: GroupHistory
+  has_many :targeted_group_histories, dependent: :destroy, foreign_key: :target_user_id, class_name: GroupHistory
+
   delegate :last_sent_email_address, :to => :email_logs
 
   before_validation :strip_downcase_email
@@ -927,8 +930,9 @@ class User < ActiveRecord::Base
          .where("LENGTH(COALESCE(automatic_membership_email_domains, '')) > 0")
          .each do |group|
       domains = group.automatic_membership_email_domains.gsub('.', '\.')
-      if self.email =~ Regexp.new("@(#{domains})$", true)
+      if self.email =~ Regexp.new("@(#{domains})$", true) && !group.users.include?(self)
-        group.add(self) rescue ActiveRecord::RecordNotUnique
+        group.add(self)
+        GroupActionLogger.new(Discourse.system_user, group).log_add_user_to_group(self)
       end
     end
   end

app/serializers/basic_group_history_serializer.rb

@@ -0,0 +1,14 @@
+class BasicGroupHistorySerializer < ApplicationSerializer
+  attributes :action,
+             :subject,
+             :prev_value,
+             :new_value,
+             :created_at
+
+  has_one :acting_user, embed: :objects, serializer: BasicUserSerializer
+  has_one :target_user, embed: :objects, serializer: BasicUserSerializer
+
+  def action
+    GroupHistory.actions[object.action]
+  end
+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?

app/services/group_action_logger.rb

@@ -0,0 +1,78 @@
+class GroupActionLogger
+  def initialize(acting_user, group)
+    @acting_user = acting_user
+    @group = group
+  end
+
+  def log_make_user_group_owner(target_user)
+    can_edit?
+
+    GroupHistory.create!(default_params.merge(
+      action: GroupHistory.actions[:make_user_group_owner],
+      target_user: target_user
+    ))
+  end
+
+  def log_remove_user_as_group_owner(target_user)
+    can_edit?
+
+    GroupHistory.create!(default_params.merge(
+      action: GroupHistory.actions[:remove_user_as_group_owner],
+      target_user: target_user
+    ))
+  end
+
+  def log_add_user_to_group(target_user)
+    @group.public || can_edit?
+
+    GroupHistory.create!(default_params.merge(
+      action: GroupHistory.actions[:add_user_to_group],
+      target_user: target_user
+    ))
+  end
+
+  def log_remove_user_from_group(target_user)
+    @group.public || can_edit?
+
+    GroupHistory.create!(default_params.merge(
+      action: GroupHistory.actions[:remove_user_from_group],
+      target_user: target_user
+    ))
+  end
+
+  def log_change_group_settings
+    can_edit?
+
+    @group.previous_changes.except(*excluded_attributes).each do |attribute_name, value|
+      next if value[0].blank? && value[1].blank?
+
+      GroupHistory.create!(default_params.merge(
+        action: GroupHistory.actions[:change_group_setting],
+        subject: attribute_name,
+        prev_value: value[0],
+        new_value: value[1]
+      ))
+    end
+  end
+
+  private
+
+  def excluded_attributes
+    [
+      :bio_cooked,
+      :updated_at,
+      :created_at,
+      :user_count
+    ]
+  end
+
+  def default_params
+    { group: @group, acting_user: @acting_user }
+  end
+
+  def can_edit?
+    unless Guardian.new(@acting_user).can_log_group_changes?(@group)
+      raise Discourse::InvalidParameter
+    end
+  end
+end

config/locales/client.en.yml

@@ -375,14 +375,42 @@ en:
         one: "1 user"
         other: "%{count} users"
 
+    group_histories:
+      actions:
+        change_group_setting: "Change group setting"
+        add_user_to_group: "Add user"
+        remove_user_from_group: "Remove user"
+        make_user_group_owner: "Make owner"
+        remove_user_as_group_owner: "Revoke owner"
+
     groups:
+      logs:
+        title: "Logs"
+        when: "When"
+        action: "Action"
+        acting_user: "Acting user"
+        target_user: "Target user"
+        subject: "Subject"
+        details: "Details"
+        from: "From"
+        to: "To"
+      edit:
+        title: 'Edit Group'
+        group_title: 'Title'
+      name_placeholder: "Group name, no spaces, same as username rule"
+      public: "Allow users to join/leave the group freely"
       empty:
         posts: "There is no post by members of this group."
         members: "There is no member in this group."
         mentions: "There is no mention of this group."
         messages: "There is no message for this group."
         topics: "There is no topic by members of this group."
+        logs: "There is no logs for this group."
       add: "Add"
+      join: "Join Group"
+      leave: "Leave Group"
+      name: "Name"
+      bio: "About Group"
       selector_placeholder: "Add members"
       owner: "owner"
       visible: "Group is visible to all users"
@@ -421,6 +449,15 @@ en:
         muted:
           title: "Muted"
           description: "You will never be notified of anything about new topics in this group."
+      flair_url: "Avatar Flair Image"
+      flair_url_placeholder: "(Optional) Image URL or Font Awesome class"
+      flair_bg_color: "Avatar Flair Background Color"
+      flair_bg_color_placeholder: "(Optional) Hex color value"
+      flair_color: "Avatar Flair Color"
+      flair_color_placeholder: "(Optional) Hex color value"
+      flair_preview_icon: "Preview Icon"
+      flair_preview_image: "Preview Image"
+      flair_note: "Note: Flair will only show for a user's primary group."
 
     user_action_groups:
       "1": "Likes Given"
@@ -1845,23 +1882,6 @@ en:
             title: "Show the raw source diffs side-by-side"
             button: '<i class="fa fa-columns"></i> Raw'
 
-    group:
-      edit:
-        title: 'Edit Group'
-      title: 'Title'
-      name: "Name"
-      bio: "About Group"
-      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"
-      flair_bg_color: "Avatar Flair Background Color"
-      flair_bg_color_placeholder: "(Optional) Hex color value"
-      flair_color: "Avatar Flair Color"
-      flair_color_placeholder: "(Optional) Hex color value"
-      flair_preview_icon: "Preview Icon"
-      flair_preview_image: "Preview Image"
-      flair_note: "Note: Flair will only show for a user's primary group."
-
     category:
       can: 'can&hellip; '
       none: '(no category)'

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."

config/routes.rb

@@ -410,6 +410,7 @@ Discourse::Application.routes.draw do
     get 'messages'
     get 'counts'
     get 'mentionable'
+    get 'logs' => 'groups#histories'
 
     member do
       put "members" => "groups#add_members"

db/migrate/20161207030057_add_public_to_groups.rb

@@ -0,0 +1,5 @@
+class AddPublicToGroups < ActiveRecord::Migration
+  def change
+    add_column :groups, :public, :boolean, default: :false, null: false
+  end
+end

db/migrate/20161208064834_create_group_histories.rb

@@ -0,0 +1,19 @@
+class CreateGroupHistories < ActiveRecord::Migration
+  def change
+    create_table :group_histories do |t|
+      t.integer :group_id, null: false
+      t.integer :acting_user_id, null: false
+      t.integer :target_user_id
+      t.integer :action, index: true, null: false
+      t.string :subject
+      t.text :prev_value
+      t.text :new_value
+
+      t.timestamps null: false
+    end
+
+    add_index :group_histories, :group_id
+    add_index :group_histories, :acting_user_id
+    add_index :group_histories, :target_user_id
+  end
+end

lib/guardian/group_guardian.rb

@@ -5,11 +5,14 @@ module GroupGuardian
   # Automatic groups are not represented in the GROUP_USERS
   # table and thus do not allow membership changes.
   def can_edit_group?(group)
-    (is_admin? || group.users.where('group_users.owner').include?(user)) && !group.automatic
+    can_log_group_changes?(group) && !group.automatic
+  end
+
+  def can_log_group_changes?(group)
+    (is_admin? || group.users.where('group_users.owner').include?(user))
   end
 
   def can_see_group_messages?(group)
     is_admin? || group.users.include?(user)
   end
-
 end

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
@@ -83,7 +85,10 @@ describe Admin::GroupsController do
   context ".update" do
 
     it "ignore name change on automatic group" do
-      xhr :put, :update, { id: 1, group: { name: "WAT", visible: "true" } }
+      expect do
+        xhr :put, :update, { id: 1, group: { name: "WAT", visible: "true" } }
+      end.to_not change { GroupHistory.count }
+
       expect(response).to be_success
 
       group = Group.find(1)

spec/controllers/admin/users_controller_spec.rb

@@ -178,10 +178,16 @@ describe Admin::UsersController do
 
       it 'adds the user to the group' do
         xhr :post, :add_group, group_id: group.id, user_id: user.id
-        expect(response).to be_success
 
+        expect(response).to be_success
         expect(GroupUser.where(user_id: user.id, group_id: group.id).exists?).to eq(true)
 
+        group_history = GroupHistory.last
+
+        expect(group_history.action).to eq(GroupHistory.actions[:add_user_to_group])
+        expect(group_history.acting_user).to eq(@user)
+        expect(group_history.target_user).to eq(user)
+
         # Doing it again doesn't raise an error
         xhr :post, :add_group, group_id: group.id, user_id: user.id
         expect(response).to be_success

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/fabricators/group_history_fabricator.rb

@@ -0,0 +1,6 @@
+Fabricator(:group_history) do
+  group
+  action GroupHistory.actions[:add_user_to_group]
+  acting_user { Fabricate(:user) }
+  target_user { Fabricate(:user) }
+end

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
@@ -45,13 +48,16 @@ describe "Groups" do
       end
 
       it "should be able update the group" do
-        xhr :put, "/groups/#{group.id}", { group: {
+        expect do
-          flair_bg_color: 'FFF',
+          xhr :put, "/groups/#{group.id}", { group: {
-          flair_color: 'BBB',
+            flair_bg_color: 'FFF',
-          flair_url: 'fa-adjust',
+            flair_color: 'BBB',
-          bio_raw: 'testing',
+            flair_url: 'fa-adjust',
-          title: 'awesome team'
+            bio_raw: 'testing',
-        } }
+            title: 'awesome team',
+            public: true
+          } }
+        end.to change { GroupHistory.count }.by(6)
 
         expect(response).to be_success
 
@@ -62,6 +68,8 @@ 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)
+        expect(GroupHistory.last.subject).to eq('public')
       end
     end
 
@@ -145,4 +153,310 @@ 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 do
+          xhr :put, "/groups/#{group.id}/members", usernames: user2.username
+        end.to change { group.users.count }.by(1)
+
+        expect(response).to be_success
+
+        group_history = GroupHistory.last
+
+        expect(group_history.action).to eq(GroupHistory.actions[:add_user_to_group])
+        expect(group_history.acting_user).to eq(admin)
+        expect(group_history.target_user).to eq(user2)
+      end
+
+      it "can make incremental deletes" do
+        expect do
+          xhr :delete, "/groups/#{group.id}/members", username: user.username
+        end.to change { group.users.count }.by(-1)
+
+        expect(response).to be_success
+
+        group_history = GroupHistory.last
+
+        expect(group_history.action).to eq(GroupHistory.actions[:remove_user_from_group])
+        expect(group_history.acting_user).to eq(admin)
+        expect(group_history.target_user).to eq(user)
+      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
+
+  describe "group histories" do
+    context 'when user is not signed in' do
+      it 'should raise the right error' do
+        expect { xhr :get, "/groups/#{group.name}/logs" }
+          .to raise_error(Discourse::NotLoggedIn)
+      end
+    end
+
+    context 'when user is not a group owner' do
+      before do
+        sign_in(user)
+      end
+
+      it 'should be forbidden' do
+        xhr :get, "/groups/#{group.name}/logs"
+
+        expect(response).to be_forbidden
+      end
+    end
+
+    describe 'viewing history' do
+      context 'public group' do
+        before do
+          group.add_owner(user)
+          group.update_attributes!(public: true)
+          GroupActionLogger.new(user, group).log_change_group_settings
+          sign_in(user)
+        end
+
+        it 'should allow group owner to view history' do
+          xhr :get, "/groups/#{group.name}/logs"
+
+          expect(response).to be_success
+
+          result = JSON.parse(response.body)["logs"].first
+
+          expect(result["action"]).to eq(GroupHistory.actions[1].to_s)
+          expect(result["subject"]).to eq('public')
+          expect(result["prev_value"]).to eq('f')
+          expect(result["new_value"]).to eq('t')
+        end
+      end
+
+      context 'admin' do
+        let(:admin) { Fabricate(:admin) }
+
+        before do
+          sign_in(admin)
+        end
+
+        it 'should be able to view history' do
+          GroupActionLogger.new(admin, group).log_remove_user_from_group(user)
+
+          xhr :get, "/groups/#{group.name}/logs"
+
+          expect(response).to be_success
+
+          result = JSON.parse(response.body)["logs"].first
+
+          expect(result["action"]).to eq(GroupHistory.actions[3].to_s)
+        end
+
+        it 'should be able to filter through the history' do
+          GroupActionLogger.new(admin, group).log_add_user_to_group(user)
+          GroupActionLogger.new(admin, group).log_remove_user_from_group(user)
+
+          xhr :get, "/groups/#{group.name}/logs", filters: { "action" => "add_user_to_group" }
+
+          expect(response).to be_success
+
+          logs = JSON.parse(response.body)["logs"]
+
+          expect(logs.count).to eq(1)
+          expect(logs.first["action"]).to eq(GroupHistory.actions[2].to_s)
+        end
+      end
+    end
+  end
 end

spec/models/group_history_spec.rb

@@ -0,0 +1,73 @@
+require 'rails_helper'
+
+RSpec.describe GroupHistory do
+  let(:group_history) { Fabricate(:group_history) }
+
+  let(:other_group_history) do
+    Fabricate(:group_history,
+      action: GroupHistory.actions[:remove_user_from_group],
+      group: group_history.group
+    )
+  end
+
+  describe '.with_filters' do
+    it 'should return the right records' do
+      expect(described_class.with_filters(group_history.group))
+        .to eq([group_history])
+    end
+
+    it 'should filter by action correctly' do
+      other_group_history
+
+      expect(described_class.with_filters(
+        group_history.group,
+        { :action => GroupHistory.actions[3]}
+      )).to eq([other_group_history])
+    end
+
+    it 'should filter by subject correctly' do
+      other_group_history.update_attributes!(subject: "test")
+
+      expect(described_class.with_filters(
+        group_history.group,
+        subject: 'test'
+      )).to eq([other_group_history])
+    end
+
+    it 'should filter by multiple filters correctly' do
+      group_history.update_attributes!(action: GroupHistory.actions[:remove_user_from_group])
+      other_group_history.update_attributes!(subject: "test")
+
+      expect(described_class.with_filters(group_history.group,
+        :action => GroupHistory.actions[3], subject: 'test'
+      )).to eq([other_group_history])
+    end
+
+    it 'should filter by target_user and acting_user correctly' do
+      group_history
+      other_group_history
+
+      group_history_3 = Fabricate(:group_history,
+        group: group_history.group,
+        acting_user: other_group_history.acting_user,
+        target_user: other_group_history.target_user,
+        action: GroupHistory.actions[:remove_user_as_group_owner]
+      )
+
+      expect(described_class.with_filters(
+        group_history.group,
+        target_user: other_group_history.target_user.username
+      ).sort).to eq([other_group_history, group_history_3])
+
+      expect(described_class.with_filters(
+        group_history.group,
+        acting_user: group_history.acting_user.username
+      )).to eq([group_history])
+
+      expect(described_class.with_filters(
+        group_history.group,
+        acting_user: group_history_3.acting_user.username, target_user: other_group_history.target_user.username
+      ).sort).to eq([other_group_history, group_history_3])
+    end
+  end
+end

spec/models/user_spec.rb

@@ -1163,6 +1163,12 @@ describe User do
       user = Fabricate(:user, email: "foo@bar.com")
       group.reload
       expect(group.users.include?(user)).to eq(true)
+
+      group_history = GroupHistory.last
+
+      expect(group_history.action).to eq(GroupHistory.actions[:add_user_to_group])
+      expect(group_history.acting_user).to eq(Discourse.system_user)
+      expect(group_history.target_user).to eq(user)
     end
 
   end

spec/services/group_action_logger_spec.rb

@@ -0,0 +1,77 @@
+require 'rails_helper'
+
+RSpec.describe GroupActionLogger do
+  let(:group_owner) { Fabricate(:user) }
+  let(:group) { Fabricate(:group) }
+  let(:user) { Fabricate(:user) }
+
+  subject { described_class.new(group_owner, group) }
+
+  before do
+    group.add_owner(group_owner)
+  end
+
+  describe '#log_make_user_group_owner' do
+    it 'should create the right record' do
+      subject.log_make_user_group_owner(user)
+
+      group_history = GroupHistory.last
+
+      expect(group_history.action).to eq(GroupHistory.actions[:make_user_group_owner])
+      expect(group_history.acting_user).to eq(group_owner)
+      expect(group_history.target_user).to eq(user)
+    end
+  end
+
+  describe '#log_remove_user_as_group_owner' do
+    it 'should create the right record' do
+      subject.log_remove_user_as_group_owner(user)
+
+      group_history = GroupHistory.last
+
+      expect(group_history.action).to eq(GroupHistory.actions[:remove_user_as_group_owner])
+      expect(group_history.acting_user).to eq(group_owner)
+      expect(group_history.target_user).to eq(user)
+    end
+  end
+
+  describe '#log_add_user_to_group' do
+    it 'should create the right record' do
+      subject.log_add_user_to_group(user)
+
+      group_history = GroupHistory.last
+
+      expect(group_history.action).to eq(GroupHistory.actions[:add_user_to_group])
+      expect(group_history.acting_user).to eq(group_owner)
+      expect(group_history.target_user).to eq(user)
+    end
+  end
+
+  describe '#log_remove_user_from_group' do
+    it 'should create the right record' do
+      subject.log_remove_user_from_group(user)
+
+      group_history = GroupHistory.last
+
+      expect(group_history.action).to eq(GroupHistory.actions[:remove_user_from_group])
+      expect(group_history.acting_user).to eq(group_owner)
+      expect(group_history.target_user).to eq(user)
+    end
+  end
+
+  describe '#log_change_group_settings' do
+    it 'should create the right record' do
+      group.update_attributes!(public: true, created_at: Time.zone.now)
+
+      expect { subject.log_change_group_settings }.to change { GroupHistory.count }.by(1)
+
+      group_history = GroupHistory.last
+
+      expect(group_history.action).to eq(GroupHistory.actions[:change_group_setting])
+      expect(group_history.acting_user).to eq(group_owner)
+      expect(group_history.subject).to eq('public')
+      expect(group_history.prev_value).to eq('f')
+      expect(group_history.new_value).to eq('t')
+    end
+  end
+end

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

@@ -0,0 +1,40 @@
+import { acceptance } from "helpers/qunit-helpers";
+
+acceptance("Group Logs", {
+  loggedIn: true,
+  setup() {
+    const response = object => {
+      return [
+        200,
+        { "Content-Type": "application/json" },
+        object
+      ];
+    };
+
+    server.get('/groups/snorlax.json', () => { // eslint-disable-line no-undef
+      return response({"basic_group":{"id":41,"automatic":false,"name":"snorlax","user_count":1,"alias_level":0,"visible":true,"automatic_membership_email_domains":"","automatic_membership_retroactive":false,"primary_group":true,"title":"Team Snorlax","grant_trust_level":null,"incoming_email":null,"has_messages":false,"flair_url":"","flair_bg_color":"","flair_color":"","bio_raw":"","bio_cooked":null,"public":true,"is_group_user":true,"is_group_owner":true}});
+    });
+
+    // Workaround while awaiting https://github.com/tildeio/route-recognizer/issues/53
+    server.get('/groups/snorlax/logs.json', request => { // eslint-disable-line no-undef
+      if (request.queryParams["filters[action]"]) {
+        return response({"logs":[{"action":"change_group_setting","subject":"title","prev_value":null,"new_value":"Team Snorlax","created_at":"2016-12-12T08:27:46.408Z","acting_user":{"id":1,"username":"tgx","avatar_template":"/letter_avatar_proxy/v2/letter/t/ecae2f/{size}.png"},"target_user":null}],"all_loaded":true});
+      } else {
+        return response({"logs":[{"action":"change_group_setting","subject":"title","prev_value":null,"new_value":"Team Snorlax","created_at":"2016-12-12T08:27:46.408Z","acting_user":{"id":1,"username":"tgx","avatar_template":"/letter_avatar_proxy/v2/letter/t/ecae2f/{size}.png"},"target_user":null},{"action":"add_user_to_group","subject":null,"prev_value":null,"new_value":null,"created_at":"2016-12-12T08:27:27.725Z","acting_user":{"id":1,"username":"tgx","avatar_template":"/letter_avatar_proxy/v2/letter/t/ecae2f/{size}.png"},"target_user":{"id":1,"username":"tgx","avatar_template":"/letter_avatar_proxy/v2/letter/t/ecae2f/{size}.png"}}],"all_loaded":true});
+      }
+    });
+  }
+});
+
+test("Browsing group logs", () => {
+  visit("/groups/snorlax/logs");
+
+  andThen(() => {
+    ok(find('tr.group-logs-row').length === 2, 'it should display the right number of logs');
+    click(find(".group-logs-row button")[0]);
+  });
+
+  andThen(() => {
+    ok(find('tr.group-logs-row').length === 1, 'it should display the right number of logs');
+  });
+});

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

@@ -39,7 +39,8 @@ test("Admin Browsing Groups", () => {
   visit("/groups/discourse");
 
   andThen(() => {
-    ok(find('.nav-stacked li').length === 5, 'it should show messages tab if user is admin');
+    ok(find(".nav-stacked li a[title='Messages']").length === 1, 'it should show messages tab if user is admin');
+    ok(find(".nav-stacked li a[title='Logs']").length === 1, 'it should show Logs tab if user is admin');
     equal(find('.group-title').text(), 'Awesome Team', 'it should display the group title');
     equal(find('.group-name').text(), '@discourse', 'it should display the group name');
   });
@@ -50,5 +51,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");
+});