From 25c7450ea7cc080f56be65eef1a9db28a7aa4057 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Thu, 17 Sep 2015 10:55:06 +0800 Subject: [PATCH 1/3] Use existing function to extract error message. --- .../discourse/controllers/edit-category.js.es6 | 15 +++------------ 1 file changed, 3 insertions(+), 12 deletions(-) diff --git a/app/assets/javascripts/discourse/controllers/edit-category.js.es6 b/app/assets/javascripts/discourse/controllers/edit-category.js.es6 index bfda086b643..0810068b477 100644 --- a/app/assets/javascripts/discourse/controllers/edit-category.js.es6 +++ b/app/assets/javascripts/discourse/controllers/edit-category.js.es6 @@ -1,5 +1,6 @@ import ModalFunctionality from 'discourse/mixins/modal-functionality'; import DiscourseURL from 'discourse/lib/url'; +import { extractError } from 'discourse/lib/ajax-error'; // Modal for editing / creating a category export default Ember.Controller.extend(ModalFunctionality, { @@ -73,11 +74,7 @@ export default Ember.Controller.extend(ModalFunctionality, { model.setProperties({slug: result.category.slug, id: result.category.id }); DiscourseURL.redirectTo("/c/" + Discourse.Category.slugFor(model)); }).catch(function(error) { - if (error && error.responseText) { - self.flash($.parseJSON(error.responseText).errors[0], 'error'); - } else { - self.flash(I18n.t('generic_error'), 'error'); - } + self.flash(extractError(error), 'error'); self.set('saving', false); }); }, @@ -94,13 +91,7 @@ export default Ember.Controller.extend(ModalFunctionality, { self.send('closeModal'); DiscourseURL.redirectTo("/categories"); }, function(error){ - - if (error && error.responseText) { - self.flash($.parseJSON(error.responseText).errors[0]); - } else { - self.flash(I18n.t('generic_error')); - } - + self.flash(extractError(error), 'error'); self.send('reopenModal'); self.displayErrors([I18n.t("category.delete_error")]); self.set('deleting', false); From c29b7ce498d7f1a4e622a8d3819339b47801f2ae Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Thu, 17 Sep 2015 10:58:31 +0800 Subject: [PATCH 2/3] FIX: Set saving to false about model has been saved. --- .../javascripts/discourse/controllers/edit-category.js.es6 | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/discourse/controllers/edit-category.js.es6 b/app/assets/javascripts/discourse/controllers/edit-category.js.es6 index 0810068b477..d77783bc0a3 100644 --- a/app/assets/javascripts/discourse/controllers/edit-category.js.es6 +++ b/app/assets/javascripts/discourse/controllers/edit-category.js.es6 @@ -68,8 +68,8 @@ export default Ember.Controller.extend(ModalFunctionality, { this.set('saving', true); model.set('parentCategory', parentCategory); - self.set('saving', false); this.get('model').save().then(function(result) { + self.set('saving', false); self.send('closeModal'); model.setProperties({slug: result.category.slug, id: result.category.id }); DiscourseURL.redirectTo("/c/" + Discourse.Category.slugFor(model)); From f39b9124b659f579645eb60754fe81121221da38 Mon Sep 17 00:00:00 2001 From: Guo Xiang Tan Date: Thu, 17 Sep 2015 15:51:32 +0800 Subject: [PATCH 3/3] FEATURE: Log staff actions for Category changes. --- .../admin/models/staff_action_log.js | 3 +- app/controllers/categories_controller.rb | 32 +++++++- app/models/category.rb | 8 ++ app/models/category_group.rb | 2 + app/models/user_history.rb | 13 +++- app/serializers/user_history_serializer.rb | 1 + app/services/staff_action_logger.rb | 62 +++++++++++++++ config/locales/client.en.yml | 4 + ...71017_add_category_id_to_user_histories.rb | 6 ++ .../controllers/categories_controller_spec.rb | 13 ++++ spec/fabricators/category_group_fabricator.rb | 5 ++ spec/models/category_spec.rb | 9 +++ spec/services/staff_action_logger_spec.rb | 77 +++++++++++++++++++ 13 files changed, 228 insertions(+), 7 deletions(-) create mode 100644 db/migrate/20150917071017_add_category_id_to_user_histories.rb create mode 100644 spec/fabricators/category_group_fabricator.rb diff --git a/app/assets/javascripts/admin/models/staff_action_log.js b/app/assets/javascripts/admin/models/staff_action_log.js index dc52a7b170a..0dde7736616 100644 --- a/app/assets/javascripts/admin/models/staff_action_log.js +++ b/app/assets/javascripts/admin/models/staff_action_log.js @@ -11,6 +11,7 @@ Discourse.StaffActionLog = Discourse.Model.extend({ formatted += this.format('admin.logs.ip_address', 'ip_address'); formatted += this.format('admin.logs.topic_id', 'topic_id'); formatted += this.format('admin.logs.post_id', 'post_id'); + formatted += this.format('admin.logs.category_id', 'category_id'); if (!this.get('useCustomModalForDetails')) { formatted += this.format('admin.logs.staff_actions.new_value', 'new_value'); formatted += this.format('admin.logs.staff_actions.previous_value', 'previous_value'); @@ -19,7 +20,7 @@ Discourse.StaffActionLog = Discourse.Model.extend({ if (this.get('details')) formatted += Handlebars.Utils.escapeExpression(this.get('details')) + '
'; } return formatted; - }.property('ip_address', 'email', 'topic_id', 'post_id'), + }.property('ip_address', 'email', 'topic_id', 'post_id', 'category_id'), format: function(label, propertyName) { if (this.get(propertyName)) { diff --git a/app/controllers/categories_controller.rb b/app/controllers/categories_controller.rb index b63ec503dbf..52437468d36 100644 --- a/app/controllers/categories_controller.rb +++ b/app/controllers/categories_controller.rb @@ -4,6 +4,7 @@ class CategoriesController < ApplicationController before_filter :ensure_logged_in, except: [:index, :show, :redirect] before_filter :fetch_category, only: [:show, :update, :destroy] + before_filter :initialize_staff_action_logger, only: [:create, :update, :destroy] skip_before_filter :check_xhr, only: [:index, :redirect] def redirect @@ -81,10 +82,18 @@ class CategoriesController < ApplicationController position = category_params.delete(:position) @category = Category.create(category_params.merge(user: current_user)) - return render_json_error(@category) unless @category.save - @category.move_to(position.to_i) if position - render_serialized(@category, CategorySerializer) + if @category.save + @category.move_to(position.to_i) if position + + Scheduler::Defer.later "Log staff action create category" do + @staff_action_logger.log_category_creation(@category) + end + + render_serialized(@category, CategorySerializer) + else + return render_json_error(@category) unless @category.save + end end def update @@ -103,8 +112,15 @@ class CategoriesController < ApplicationController end category_params.delete(:position) + old_permissions = Category.find(@category.id).permissions_params - cat.update_attributes(category_params) + if result = cat.update_attributes(category_params) + Scheduler::Defer.later "Log staff action change category settings" do + @staff_action_logger.log_category_settings_change(@category, category_params, old_permissions) + end + end + + result end end @@ -133,6 +149,10 @@ class CategoriesController < ApplicationController guardian.ensure_can_delete!(@category) @category.destroy + Scheduler::Defer.later "Log staff action delete category" do + @staff_action_logger.log_category_deletion(@category) + end + render json: success_json end @@ -175,4 +195,8 @@ class CategoriesController < ApplicationController def fetch_category @category = Category.find_by(slug: params[:id]) || Category.find_by(id: params[:id].to_i) end + + def initialize_staff_action_logger + @staff_action_logger = StaffActionLogger.new(current_user) + end end diff --git a/app/models/category.rb b/app/models/category.rb index 7e550aef28c..68eed7090d9 100644 --- a/app/models/category.rb +++ b/app/models/category.rb @@ -283,6 +283,14 @@ SQL set_permissions(permissions) end + def permissions_params + hash = {} + category_groups.includes(:group).each do |category_group| + hash[category_group.group_name] = category_group.permission_type + end + hash + end + def apply_permissions if @permissions category_groups.destroy_all diff --git a/app/models/category_group.rb b/app/models/category_group.rb index fa9fd3a21e7..849fcba0260 100644 --- a/app/models/category_group.rb +++ b/app/models/category_group.rb @@ -2,6 +2,8 @@ class CategoryGroup < ActiveRecord::Base belongs_to :category belongs_to :group + delegate :name, to: :group, prefix: true + def self.permission_types @permission_types ||= Enum.new(:full, :create_post, :readonly) end diff --git a/app/models/user_history.rb b/app/models/user_history.rb index 77867f7d466..038df0e9c2e 100644 --- a/app/models/user_history.rb +++ b/app/models/user_history.rb @@ -7,6 +7,7 @@ class UserHistory < ActiveRecord::Base belongs_to :post belongs_to :topic + belongs_to :category validates_presence_of :action @@ -39,7 +40,10 @@ class UserHistory < ActiveRecord::Base :custom, :custom_staff, :anonymize_user, - :reviewed_post) + :reviewed_post, + :change_category_settings, + :delete_category, + :create_category) end # Staff actions is a subset of all actions, used to audit actions taken by staff users. @@ -61,7 +65,10 @@ class UserHistory < ActiveRecord::Base :change_username, :custom_staff, :anonymize_user, - :reviewed_post] + :reviewed_post, + :change_category_settings, + :delete_category, + :create_category] end def self.staff_action_ids @@ -144,11 +151,13 @@ end # admin_only :boolean default(FALSE) # post_id :integer # custom_type :string(255) +# category_id :integer # # Indexes # # index_user_histories_on_acting_user_id_and_action_and_id (acting_user_id,action,id) # index_user_histories_on_action_and_id (action,id) +# index_user_histories_on_category_id (category_id) # index_user_histories_on_subject_and_id (subject,id) # index_user_histories_on_target_user_id_and_id (target_user_id,id) # diff --git a/app/serializers/user_history_serializer.rb b/app/serializers/user_history_serializer.rb index 2af3c7dc9b8..039e25c61d0 100644 --- a/app/serializers/user_history_serializer.rb +++ b/app/serializers/user_history_serializer.rb @@ -10,6 +10,7 @@ class UserHistorySerializer < ApplicationSerializer :new_value, :topic_id, :post_id, + :category_id, :action, :custom_type diff --git a/app/services/staff_action_logger.rb b/app/services/staff_action_logger.rb index cd681eea92a..cbb56693a6a 100644 --- a/app/services/staff_action_logger.rb +++ b/app/services/staff_action_logger.rb @@ -210,6 +210,64 @@ class StaffActionLogger })) end + def log_category_settings_change(category, category_params, old_permissions=nil) + validate_category(category) + + changed_attributes = category.previous_changes.slice(*category_params.keys) + + if old_permissions != category_params[:permissions] + changed_attributes.merge!({ permissions: [old_permissions.to_json, category_params[:permissions].to_json] }) + end + + changed_attributes.each do |key, value| + UserHistory.create(params.merge({ + action: UserHistory.actions[:change_category_settings], + category_id: category.id, + context: category.url, + subject: key, + previous_value: value[0], + new_value: value[1] + })) + end + end + + def log_category_deletion(category) + validate_category(category) + + details = [ + "created_at: #{category.created_at}", + "name: #{category.name}", + "permissions: #{category.permissions_params}" + ] + + if parent_category = category.parent_category + details << "parent_category: #{parent_category.name}" + end + + UserHistory.create(params.merge({ + action: UserHistory.actions[:delete_category], + category_id: category.id, + details: details.join("\n"), + context: category.url + })) + end + + def log_category_creation(category) + validate_category(category) + + details = [ + "created_at: #{category.created_at}", + "name: #{category.name}" + ] + + UserHistory.create(params.merge({ + action: UserHistory.actions[:create_category], + details: details.join("\n"), + category_id: category.id, + context: category.url + })) + end + private def params(opts=nil) @@ -217,4 +275,8 @@ class StaffActionLogger { acting_user_id: @admin.id, context: opts[:context] } end + def validate_category(category) + raise Discourse::InvalidParameters.new(:category) unless category && category.is_a?(Category) + end + end diff --git a/config/locales/client.en.yml b/config/locales/client.en.yml index 63f225130e8..389764b1d1e 100644 --- a/config/locales/client.en.yml +++ b/config/locales/client.en.yml @@ -2163,6 +2163,7 @@ en: ip_address: "IP" topic_id: "Topic ID" post_id: "Post ID" + category_id: "Category ID" delete: 'Delete' edit: 'Edit' save: 'Save' @@ -2203,6 +2204,9 @@ en: impersonate: "impersonate" anonymize_user: "anonymize user" roll_up: "roll up IP blocks" + change_category_settings: "change category settings" + delete_category: "delete category" + create_category: "create category" screened_emails: title: "Screened Emails" description: "When someone tries to create a new account, the following email addresses will be checked and the registration will be blocked, or some other action performed." diff --git a/db/migrate/20150917071017_add_category_id_to_user_histories.rb b/db/migrate/20150917071017_add_category_id_to_user_histories.rb new file mode 100644 index 00000000000..117edef54af --- /dev/null +++ b/db/migrate/20150917071017_add_category_id_to_user_histories.rb @@ -0,0 +1,6 @@ +class AddCategoryIdToUserHistories < ActiveRecord::Migration + def change + add_column :user_histories, :category_id, :integer + add_index :user_histories, :category_id + end +end diff --git a/spec/controllers/categories_controller_spec.rb b/spec/controllers/categories_controller_spec.rb index f03cfd05e8c..e545faedda0 100644 --- a/spec/controllers/categories_controller_spec.rb +++ b/spec/controllers/categories_controller_spec.rb @@ -64,6 +64,7 @@ describe CategoriesController do expect(category.slug).to eq("hello-cat") expect(category.color).to eq("ff0") expect(category.auto_close_hours).to eq(72) + expect(UserHistory.count).to eq(1) end end end @@ -90,6 +91,7 @@ describe CategoriesController do it "deletes the record" do Guardian.any_instance.expects(:can_delete_category?).returns(true) expect { xhr :delete, :destroy, id: @category.slug}.to change(Category, :count).by(-1) + expect(UserHistory.count).to eq(1) end end @@ -215,6 +217,17 @@ describe CategoriesController do expect(@category.auto_close_hours).to eq(72) expect(@category.custom_fields).to eq({"dancing" => "frogs"}) end + + it 'logs the changes correctly' do + xhr :put , :update, id: @category.id, name: 'new name', + color: @category.color, text_color: @category.text_color, + slug: @category.slug, + permissions: { + "everyone" => CategoryGroup.permission_types[:create_post] + } + + expect(UserHistory.count).to eq(2) + end end end diff --git a/spec/fabricators/category_group_fabricator.rb b/spec/fabricators/category_group_fabricator.rb new file mode 100644 index 00000000000..898825b80ea --- /dev/null +++ b/spec/fabricators/category_group_fabricator.rb @@ -0,0 +1,5 @@ +Fabricator(:category_group) do + category + group + permission_type 1 +end diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb index a00529fb218..8fdf5952af6 100644 --- a/spec/models/category_spec.rb +++ b/spec/models/category_spec.rb @@ -36,6 +36,15 @@ describe Category do end end + describe "permissions_params" do + it "returns the right group names and permission type" do + category = Fabricate(:category) + group = Fabricate(:group) + category_group = Fabricate(:category_group, category: category, group: group) + expect(category.permissions_params).to eq({ "#{group.name}" => category_group.permission_type }) + end + end + describe "topic_create_allowed and post_create_allowed" do it "works" do diff --git a/spec/services/staff_action_logger_spec.rb b/spec/services/staff_action_logger_spec.rb index cb7d6e17586..00eb85eee2e 100644 --- a/spec/services/staff_action_logger_spec.rb +++ b/spec/services/staff_action_logger_spec.rb @@ -271,4 +271,81 @@ describe StaffActionLogger do expect(logged.topic_id).to be === 1234 end end + + describe 'log_category_settings_change' do + let(:category) { Fabricate(:category, name: 'haha') } + let(:category_group) { Fabricate(:category_group, category: category, permission_type: 1) } + + it "raises an error when category is missing" do + expect { logger.log_category_settings_change(nil, nil) }.to raise_error(Discourse::InvalidParameters) + end + + it "creates new UserHistory records" do + attributes = { + name: 'new_name', + permissions: { category_group.group_name => 2 } + } + + category.update!(attributes) + + logger.log_category_settings_change(category, attributes, + { category_group.group_name => category_group.permission_type } + ) + + expect(UserHistory.count).to eq(2) + + permission_user_history = UserHistory.find_by_subject('permissions') + expect(permission_user_history.category_id).to eq(category.id) + expect(permission_user_history.previous_value).to eq({ category_group.group_name => 1 }.to_json) + expect(permission_user_history.new_value).to eq({ category_group.group_name => 2 }.to_json) + expect(permission_user_history.action).to eq(UserHistory.actions[:change_category_settings]) + expect(permission_user_history.context).to eq(category.url) + + name_user_history = UserHistory.find_by_subject('name') + expect(name_user_history.category).to eq(category) + expect(name_user_history.previous_value).to eq('haha') + expect(name_user_history.new_value).to eq('new_name') + end + end + + describe 'log_category_deletion' do + let(:parent_category) { Fabricate(:category) } + let(:category) { Fabricate(:category, parent_category: parent_category) } + + it "raises an error when category is missing" do + expect { logger.log_category_deletion(nil) }.to raise_error(Discourse::InvalidParameters) + end + + it "creates a new UserHistory record" do + logger.log_category_deletion(category) + + expect(UserHistory.count).to eq(1) + user_history = UserHistory.last + + expect(user_history.subject).to eq(nil) + expect(user_history.category).to eq(category) + expect(user_history.details).to include("parent_category: #{parent_category.name}") + expect(user_history.context).to eq(category.url) + expect(user_history.action).to eq(UserHistory.actions[:delete_category]) + end + end + + describe 'log_category_creation' do + let(:category) { Fabricate(:category) } + + it "raises an error when category is missing" do + expect { logger.log_category_deletion(nil) }.to raise_error(Discourse::InvalidParameters) + end + + it "creates a new UserHistory record" do + logger.log_category_creation(category) + + expect(UserHistory.count).to eq(1) + user_history = UserHistory.last + + expect(user_history.category).to eq(category) + expect(user_history.context).to eq(category.url) + expect(user_history.action).to eq(UserHistory.actions[:create_category]) + end + end end