diff --git a/app/assets/javascripts/admin/templates/group.hbs b/app/assets/javascripts/admin/templates/group.hbs index 42993a1e923..3b06790e439 100644 --- a/app/assets/javascripts/admin/templates/group.hbs +++ b/app/assets/javascripts/admin/templates/group.hbs @@ -4,15 +4,15 @@ {{#if model.automatic}}

{{model.name}}

{{else}} - - {{text-field name="name" value=model.name placeholderKey="group.name_placeholder"}} + + {{text-field name="name" value=model.name placeholderKey="groups.name_placeholder"}} {{/if}} {{#if model.id}} {{#unless model.automatic}}
- + {{d-editor value=model.bio_raw}}
@@ -63,12 +63,19 @@ {{#unless model.automatic}} -
- -
+
+ +
+ +
+ +
{{/unless}}
diff --git a/app/assets/javascripts/discourse/components/group-flair-inputs.js.es6 b/app/assets/javascripts/discourse/components/group-flair-inputs.js.es6 index f14d45da630..ce4113450b3 100644 --- a/app/assets/javascripts/discourse/components/group-flair-inputs.js.es6 +++ b/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}`); } }); diff --git a/app/assets/javascripts/discourse/components/group-logs-filter.js.es6 b/app/assets/javascripts/discourse/components/group-logs-filter.js.es6 new file mode 100644 index 00000000000..ba4ccc3f742 --- /dev/null +++ b/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); + } + } +}); diff --git a/app/assets/javascripts/discourse/components/group-logs-row.js.es6 b/app/assets/javascripts/discourse/components/group-logs-row.js.es6 new file mode 100644 index 00000000000..5b46af4d61d --- /dev/null +++ b/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); + } + } +}); diff --git a/app/assets/javascripts/discourse/controllers/group-index.js.es6 b/app/assets/javascripts/discourse/controllers/group-index.js.es6 index 57a3f4c8bd5..385ad7414e6 100644 --- a/app/assets/javascripts/discourse/controllers/group-index.js.es6 +++ b/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; } diff --git a/app/assets/javascripts/discourse/controllers/group-logs.js.es6 b/app/assets/javascripts/discourse/controllers/group-logs.js.es6 new file mode 100644 index 00000000000..0019127c3a6 --- /dev/null +++ b/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}`, ''); + } + } +}); diff --git a/app/assets/javascripts/discourse/controllers/group.js.es6 b/app/assets/javascripts/discourse/controllers/group.js.es6 index c4c4eae3267..800097d86db 100644 --- a/app/assets/javascripts/discourse/controllers/group.js.es6 +++ b/app/assets/javascripts/discourse/controllers/group.js.es6 @@ -6,9 +6,9 @@ var Tab = Em.Object.extend({ return 'group.' + name; }, - @computed('name') - message(name) { - return I18n.t('groups.' + name); + @computed('name', 'i18nKey') + message(name, i18nKey) { + 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') diff --git a/app/assets/javascripts/discourse/models/group-history.js.es6 b/app/assets/javascripts/discourse/models/group-history.js.es6 new file mode 100644 index 00000000000..18b644d163c --- /dev/null +++ b/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}`); + } +}); diff --git a/app/assets/javascripts/discourse/models/group.js.es6 b/app/assets/javascripts/discourse/models/group.js.es6 index 42f9575e1f7..7d8a58cec66 100644 --- a/app/assets/javascripts/discourse/models/group.js.es6 +++ b/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 || {}; diff --git a/app/assets/javascripts/discourse/routes/app-route-map.js.es6 b/app/assets/javascripts/discourse/routes/app-route-map.js.es6 index a1de0bfc74e..37733386f1f 100644 --- a/app/assets/javascripts/discourse/routes/app-route-map.js.es6 +++ b/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 diff --git a/app/assets/javascripts/discourse/routes/group-logs.js.es6 b/app/assets/javascripts/discourse/routes/group-logs.js.es6 new file mode 100644 index 00000000000..0d365fab188 --- /dev/null +++ b/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(); + } + } +}); diff --git a/app/assets/javascripts/discourse/templates/components/group-flair-inputs.hbs b/app/assets/javascripts/discourse/templates/components/group-flair-inputs.hbs index 1e75c9655f7..5693422c4d5 100644 --- a/app/assets/javascripts/discourse/templates/components/group-flair-inputs.hbs +++ b/app/assets/javascripts/discourse/templates/components/group-flair-inputs.hbs @@ -1,31 +1,31 @@
- + {{text-field name="flair_url" value=model.flair_url - placeholderKey="group.flair_url_placeholder"}} + placeholderKey="groups.flair_url_placeholder"}}
- + {{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"}}
{{#if flairPreviewIcon}}
- + {{text-field name="flair_color" class="group-flair-color" value=model.flair_color - placeholderKey="group.flair_color_placeholder"}} + placeholderKey="groups.flair_color_placeholder"}}
{{/if}}
- {{i18n 'group.flair_note'}} + {{i18n 'groups.flair_note'}}
diff --git a/app/assets/javascripts/discourse/templates/components/group-logs-filter.hbs b/app/assets/javascripts/discourse/templates/components/group-logs-filter.hbs new file mode 100644 index 00000000000..11c1eb7b558 --- /dev/null +++ b/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}} + {{label}}: {{filterText}} + {{fa-icon "times-circle"}} + {{/d-button}} +{{/if}} diff --git a/app/assets/javascripts/discourse/templates/components/group-logs-row.hbs b/app/assets/javascripts/discourse/templates/components/group-logs-row.hbs new file mode 100644 index 00000000000..31d63039952 --- /dev/null +++ b/app/assets/javascripts/discourse/templates/components/group-logs-row.hbs @@ -0,0 +1,59 @@ + + + {{#d-button class="btn-small" action="filter" actionParam=(hash value=log.action key="action")}} + {{log.actionTitle}} + {{/d-button}} + + + + {{avatar log.acting_user imageSize='tiny'}} + + {{#d-button class="btn-small" action="filter" actionParam=(hash value=log.acting_user.username key="acting_user")}} + {{log.acting_user.username}} + {{/d-button}} + + + + {{#if log.target_user}} + {{avatar log.target_user imageSize='tiny'}} + + {{#d-button class="btn-small" action="filter" actionParam=(hash value=log.target_user.username key="target_user")}} + {{log.target_user.username}} + {{/d-button}} + {{/if}} + + + + {{#if log.subject}} + {{#d-button class="btn-small" action="filter" actionParam=(hash value=log.subject key="subject")}} + {{log.subject}} + {{/d-button}} + {{/if}} + + + {{bound-date log.created_at}} + + + {{#if log.prev_value}} + {{#if expandDetails}} + {{fa-icon 'ellipsis-v'}} + {{else}} + {{fa-icon 'ellipsis-h'}} + {{/if}} + {{/if}} + + + +{{#if expandDetails}} + + +

+ {{i18n 'groups.logs.from'}}: {{log.prev_value}} +

+ +

+ {{i18n 'groups.logs.to'}}: {{log.new_value}} +

+ + +{{/if}} diff --git a/app/assets/javascripts/discourse/templates/group-index.hbs b/app/assets/javascripts/discourse/templates/group-index.hbs index 7ccbf6ea1c6..694750e6821 100644 --- a/app/assets/javascripts/discourse/templates/group-index.hbs +++ b/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"}} + {{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"}} diff --git a/app/assets/javascripts/discourse/templates/group-logs.hbs b/app/assets/javascripts/discourse/templates/group-logs.hbs new file mode 100644 index 00000000000..a43424da344 --- /dev/null +++ b/app/assets/javascripts/discourse/templates/group-logs.hbs @@ -0,0 +1,33 @@ +{{#if model}} +
+ {{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"}} +
+ + {{#load-more selector=".group-logs .group-logs-row" action="loadMore"}} + + + + + + + + + + + + {{#each model.logs as |log|}} + {{group-logs-row + log=log + filters=filters}} + {{/each}} + +
{{i18n 'groups.logs.action'}}{{i18n 'groups.logs.acting_user'}}{{i18n 'groups.logs.target_user'}}{{i18n 'groups.logs.subject'}}{{i18n 'groups.logs.when'}}
+ {{/load-more}} + + {{conditional-loading-spinner condition=loading}} +{{else}} +
{{i18n "groups.empty.logs"}}
+{{/if}} diff --git a/app/assets/javascripts/discourse/templates/group.hbs b/app/assets/javascripts/discourse/templates/group.hbs index fac08c84288..f7188592d2b 100644 --- a/app/assets/javascripts/discourse/templates/group.hbs +++ b/app/assets/javascripts/discourse/templates/group.hbs @@ -23,7 +23,7 @@ {{#if canEditGroup}} - {{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"}} {{/if}}
@@ -41,6 +41,7 @@ {{#each getTabs as |tab|}}
  • {{#link-to tab.location model title=tab.message}} + {{#if tab.icon}}{{fa-icon tab.icon}}{{/if}} {{tab.message}} {{#if tab.count}}({{tab.count}}){{/if}} {{/link-to}} diff --git a/app/assets/javascripts/discourse/templates/modal/edit-group.hbs b/app/assets/javascripts/discourse/templates/modal/edit-group.hbs index 60a762eb7e9..3c65ade87ed 100644 --- a/app/assets/javascripts/discourse/templates/modal/edit-group.hbs +++ b/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"}}
    - + {{input type='text' name='title' value=model.title class='edit-group-title'}} - + {{d-editor value=model.bio_raw class="edit-group-bio"}} {{group-flair-inputs model=model}} + +
    {{/d-modal-body}} diff --git a/app/assets/stylesheets/common/base/group.scss b/app/assets/stylesheets/common/base/group.scss index 08f2bc37e16..ef9ee506f60 100644 --- a/app/assets/stylesheets/common/base/group.scss +++ b/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 { - textarea { - width: 99%; - } - - label { - font-weight: bold; - } - - input[type="text"] { - width: 80% !important; - margin-bottom: 10px; - } - +.form-horizontal { .group-flair-inputs { display: inline-block; - margin-top: 30px; - margin-bottom: 30px; + margin: 15px 0px; + + 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; diff --git a/app/assets/stylesheets/mobile/group.scss b/app/assets/stylesheets/mobile/group.scss index 120e980d0f1..c254518732c 100644 --- a/app/assets/stylesheets/mobile/group.scss +++ b/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; diff --git a/app/controllers/admin/groups_controller.rb b/app/controllers/admin/groups_controller.rb index e363aff74a9..7f4109277e6 100644 --- a/app/controllers/admin/groups_controller.rb +++ b/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 diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index 855e3ab6384..c8702c28c50 100644 --- a/app/controllers/admin/users_controller.rb +++ b/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.users << @user rescue ActiveRecord::RecordNotUnique + group.add(@user) + 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 diff --git a/app/controllers/groups_controller.rb b/app/controllers/groups_controller.rb index 4e439aec486..cfb8b686743 100644 --- a/app/controllers/groups_controller.rb +++ b/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 = User.where(username: params[:usernames].split(",")) - elsif params[:user_ids].present? - users = User.find(params[:user_ids].split(",")) - elsif params[:user_emails].present? - users = User.where(email: params[:user_emails].split(",")) - else - raise Discourse::InvalidParameters.new('user_ids or usernames or user_emails must be present') + users = + if params[:usernames].present? + User.where(username: params[:usernames].split(",")) + elsif params[:user_ids].present? + User.find(params[:user_ids].split(",")) + elsif params[:user_emails].present? + User.where(email: params[:user_emails].split(",")) + 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.find(params[:user_id]) - elsif params[:username].present? - user = User.find_by_username(params[:username]) - elsif params[:user_email].present? - user = User.find_by_email(params[:user_email]) - else - raise Discourse::InvalidParameters.new('user_id or username must be present') + user = + if params[:user_id].present? + User.find_by(id: params[:user_id]) + elsif params[:username].present? + User.find_by_username(params[:username]) + elsif params[:user_email].present? + User.find_by_email(params[:user_email]) + 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 diff --git a/app/jobs/regular/automatic_group_membership.rb b/app/jobs/regular/automatic_group_membership.rb index e66942fbce0..a6220db4277 100644 --- a/app/jobs/regular/automatic_group_membership.rb +++ b/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 - # we don't care about this - end + GroupActionLogger.new(Discourse.system_user, group).log_add_user_to_group(user) end Group.reset_counters(group.id, :group_users) diff --git a/app/models/group.rb b/app/models/group.rb index dde84c29225..44391dc96dd 100644 --- a/app/models/group.rb +++ b/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 # diff --git a/app/models/group_history.rb b/app/models/group_history.rb new file mode 100644 index 00000000000..338cf75575a --- /dev/null +++ b/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) +# diff --git a/app/models/invite.rb b/app/models/invite.rb index 8ecef5eda06..6d20f14ed1e 100644 --- a/app/models/invite.rb +++ b/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 diff --git a/app/models/topic.rb b/app/models/topic.rb index a5117113238..577c9315a01 100644 --- a/app/models/topic.rb +++ b/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 # diff --git a/app/models/user.rb b/app/models/user.rb index d292d4917b4..cd90c63c6c5 100644 --- a/app/models/user.rb +++ b/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) - group.add(self) rescue ActiveRecord::RecordNotUnique + if self.email =~ Regexp.new("@(#{domains})$", true) && !group.users.include?(self) + group.add(self) + GroupActionLogger.new(Discourse.system_user, group).log_add_user_to_group(self) end end end diff --git a/app/serializers/basic_group_history_serializer.rb b/app/serializers/basic_group_history_serializer.rb new file mode 100644 index 00000000000..f413dfb8cae --- /dev/null +++ b/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 diff --git a/app/serializers/basic_group_serializer.rb b/app/serializers/basic_group_serializer.rb index 401a88b2ff1..a4f1e4a2d0c 100644 --- a/app/serializers/basic_group_serializer.rb +++ b/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? diff --git a/app/services/group_action_logger.rb b/app/services/group_action_logger.rb new file mode 100644 index 00000000000..1518c0d0138 --- /dev/null +++ b/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 diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 820734bf5c9..cb0c4bc99f4 100644 --- a/config/locales/client.en.yml +++ b/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: ' 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… ' none: '(no category)' diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index e5a78f46506..766a77124dd 100644 --- a/config/locales/server.en.yml +++ b/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." diff --git a/config/routes.rb b/config/routes.rb index 5bffa05f745..e3bfdb39b28 100644 --- a/config/routes.rb +++ b/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" diff --git a/db/migrate/20161207030057_add_public_to_groups.rb b/db/migrate/20161207030057_add_public_to_groups.rb new file mode 100644 index 00000000000..edcceb25053 --- /dev/null +++ b/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 diff --git a/db/migrate/20161208064834_create_group_histories.rb b/db/migrate/20161208064834_create_group_histories.rb new file mode 100644 index 00000000000..1b18971c4ed --- /dev/null +++ b/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 diff --git a/lib/guardian/group_guardian.rb b/lib/guardian/group_guardian.rb index 8cc3ffa3899..95ec4f2fdef 100644 --- a/lib/guardian/group_guardian.rb +++ b/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 diff --git a/spec/controllers/admin/groups_controller_spec.rb b/spec/controllers/admin/groups_controller_spec.rb index 53cb83436a4..a5b696932fe 100644 --- a/spec/controllers/admin/groups_controller_spec.rb +++ b/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) diff --git a/spec/controllers/admin/users_controller_spec.rb b/spec/controllers/admin/users_controller_spec.rb index ac851a9c9fd..912338f262c 100644 --- a/spec/controllers/admin/users_controller_spec.rb +++ b/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 diff --git a/spec/controllers/groups_controller_spec.rb b/spec/controllers/groups_controller_spec.rb index de32060f6c2..aeaa1c61f31 100644 --- a/spec/controllers/groups_controller_spec.rb +++ b/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 diff --git a/spec/fabricators/group_history_fabricator.rb b/spec/fabricators/group_history_fabricator.rb new file mode 100644 index 00000000000..9b5b44c056b --- /dev/null +++ b/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 diff --git a/spec/integration/groups_spec.rb b/spec/integration/groups_spec.rb index 6858e1ac5a0..e13677f9793 100644 --- a/spec/integration/groups_spec.rb +++ b/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: { - flair_bg_color: 'FFF', - flair_color: 'BBB', - flair_url: 'fa-adjust', - bio_raw: 'testing', - title: 'awesome team' - } } + expect do + xhr :put, "/groups/#{group.id}", { group: { + flair_bg_color: 'FFF', + flair_color: 'BBB', + flair_url: 'fa-adjust', + 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 diff --git a/spec/models/group_history_spec.rb b/spec/models/group_history_spec.rb new file mode 100644 index 00000000000..2b287de4348 --- /dev/null +++ b/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 diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index 31dca6883f0..614cf17697e 100644 --- a/spec/models/user_spec.rb +++ b/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 diff --git a/spec/services/group_action_logger_spec.rb b/spec/services/group_action_logger_spec.rb new file mode 100644 index 00000000000..0ec4efe2617 --- /dev/null +++ b/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 diff --git a/test/javascripts/acceptance/groups-logs-test.js.es6 b/test/javascripts/acceptance/groups-logs-test.js.es6 new file mode 100644 index 00000000000..478955549df --- /dev/null +++ b/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'); + }); +}); diff --git a/test/javascripts/acceptance/groups-test.js.es6 b/test/javascripts/acceptance/groups-test.js.es6 index bc6a6ee44a1..d3ff6e5c221 100644 --- a/test/javascripts/acceptance/groups-test.js.es6 +++ b/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'); }); }); diff --git a/test/javascripts/controllers/group-index-test.js.es6 b/test/javascripts/controllers/group-index-test.js.es6 new file mode 100644 index 00000000000..d3ac38a18a0 --- /dev/null +++ b/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"); +});