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-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 9676cf18100..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, @@ -141,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-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-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 3b20460b2c8..f7188592d2b 100644 --- a/app/assets/javascripts/discourse/templates/group.hbs +++ b/app/assets/javascripts/discourse/templates/group.hbs @@ -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/stylesheets/common/base/group.scss b/app/assets/stylesheets/common/base/group.scss index f2b48532aff..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; 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 f98b43e0e91..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) @@ -72,6 +72,9 @@ class Admin::GroupsController < Admin::AdminController if group.save Group.reset_counters(group.id, :group_users) + + yield(group) if block_given? + render_serialized(group, BasicGroupSerializer) else render_json_error group @@ -101,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) @@ -118,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) 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 346c0ae5dad..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) @@ -135,6 +139,7 @@ class GroupsController < ApplicationController 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 @@ -184,7 +189,8 @@ class GroupsController < ApplicationController 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 @@ -205,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 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 fc57ad0c346..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) 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/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/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 1e7081eda17..cb0c4bc99f4 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -375,7 +375,25 @@ 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' @@ -387,6 +405,7 @@ en: 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" 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/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 d3aa8418251..a5b696932fe 100644 --- a/spec/controllers/admin/groups_controller_spec.rb +++ b/spec/controllers/admin/groups_controller_spec.rb @@ -85,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/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 3ca20455f9c..e13677f9793 100644 --- a/spec/integration/groups_spec.rb +++ b/spec/integration/groups_spec.rb @@ -48,14 +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', - public: true - } } + 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 @@ -67,6 +69,7 @@ describe "Groups" do 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 @@ -218,21 +221,32 @@ describe "Groups" do context 'adding members' do it "can make incremental adds" do user2 = Fabricate(:user) - expect(group.users.count).to eq(1) - xhr :put, "/groups/#{group.id}/members", usernames: user2.username + expect do + xhr :put, "/groups/#{group.id}/members", usernames: user2.username + end.to change { group.users.count }.by(1) expect(response).to be_success - expect(group.reload.users.count).to eq(2) + + 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(group.users.count).to eq(1) - - xhr :delete, "/groups/#{group.id}/members", username: user.username + expect do + xhr :delete, "/groups/#{group.id}/members", username: user.username + end.to change { group.users.count }.by(-1) expect(response).to be_success - expect(group.reload.users.count).to eq(0) + + 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 @@ -366,4 +380,83 @@ describe "Groups" do 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 9ebe4826943..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'); });