diff --git a/app/controllers/admin/users_controller.rb b/app/controllers/admin/users_controller.rb index b526e89f156..976ea91beb8 100644 --- a/app/controllers/admin/users_controller.rb +++ b/app/controllers/admin/users_controller.rb @@ -288,7 +288,12 @@ class Admin::UsersController < Admin::AdminController end def approve - Reviewable.bulk_perform_targets(current_user, :approve, 'ReviewableUser', [@user.id]) + guardian.ensure_can_approve!(@user) + + reviewable = ReviewableUser.find_by(target: @user) || + Jobs::CreateUserReviewable.new.execute(user_id: @user.id).reviewable + + reviewable.perform(current_user, :approve) render body: nil end diff --git a/app/jobs/regular/create_user_reviewable.rb b/app/jobs/regular/create_user_reviewable.rb index 8d06f9474ae..288ba7ff2ac 100644 --- a/app/jobs/regular/create_user_reviewable.rb +++ b/app/jobs/regular/create_user_reviewable.rb @@ -1,4 +1,6 @@ class Jobs::CreateUserReviewable < Jobs::Base + attr_reader :reviewable + def execute(args) raise Discourse::InvalidParameters unless args[:user_id].present? @@ -11,7 +13,7 @@ class Jobs::CreateUserReviewable < Jobs::Base if user = User.find_by(id: args[:user_id]) return if user.approved? - reviewable = ReviewableUser.needs_review!( + @reviewable = ReviewableUser.needs_review!( target: user, created_by: Discourse.system_user, reviewable_by_moderator: true, @@ -21,7 +23,7 @@ class Jobs::CreateUserReviewable < Jobs::Base email: user.email } ) - reviewable.add_score( + @reviewable.add_score( Discourse.system_user, ReviewableScore.types[:needs_approval], reason: reason, diff --git a/app/models/emoji_set_site_setting.rb b/app/models/emoji_set_site_setting.rb index 003eb770e4a..fcc3759b0c2 100644 --- a/app/models/emoji_set_site_setting.rb +++ b/app/models/emoji_set_site_setting.rb @@ -2,25 +2,6 @@ require 'enum_site_setting' class EmojiSetSiteSetting < EnumSiteSetting - # fix the URLs when changing the site setting - DiscourseEvent.on(:site_setting_saved) do |site_setting| - if site_setting.name.to_s == "emoji_set" && site_setting.value_changed? - Emoji.clear_cache - - previous_value = site_setting.attribute_in_database(:value) || SiteSetting.defaults[:emoji_set] - before = "/images/emoji/#{previous_value}/" - after = "/images/emoji/#{site_setting.value}/" - - Scheduler::Defer.later("Fix Emoji Links") do - DB.exec("UPDATE posts SET cooked = REPLACE(cooked, :before, :after) WHERE cooked LIKE :like", - before: before, - after: after, - like: "%#{before}%" - ) - end - end - end - def self.valid_value?(val) values.any? { |v| v[:value] == val.to_s } end diff --git a/app/models/report.rb b/app/models/report.rb index e5b5e4e37bf..67bcb26d82f 100644 --- a/app/models/report.rb +++ b/app/models/report.rb @@ -305,12 +305,6 @@ class Report add_counts report, subject, 'topics.created_at' end - DiscourseEvent.on(:site_setting_saved) do |site_setting| - if ["backup_location", "s3_backup_bucket"].include?(site_setting.name.to_s) - clear_cache(:storage_stats) - end - end - def rgba_color(hex, opacity = 1) if hex.size == 3 chars = hex.scan(/\w/) diff --git a/app/models/reviewable_user.rb b/app/models/reviewable_user.rb index 8feefedf612..d19f7702cf1 100644 --- a/app/models/reviewable_user.rb +++ b/app/models/reviewable_user.rb @@ -1,6 +1,7 @@ require_dependency 'reviewable' class ReviewableUser < Reviewable + def self.create_for(user) create( created_by_id: Discourse.system_user.id, diff --git a/app/models/topic.rb b/app/models/topic.rb index f7ee82ec51d..e685aa7ca82 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -33,14 +33,6 @@ class Topic < ActiveRecord::Base attr_accessor :allowed_user_ids, :tags_changed, :includes_destination_category - DiscourseEvent.on(:site_setting_saved) do |site_setting| - if site_setting.name.to_s == "slug_generation_method" && site_setting.saved_change_to_value? - Scheduler::Defer.later("Null topic slug") do - Topic.update_all(slug: nil) - end - end - end - def self.max_fancy_title_length 400 end diff --git a/app/serializers/admin_user_serializer.rb b/app/serializers/admin_user_serializer.rb index 2ed6825b12e..97d587ba81a 100644 --- a/app/serializers/admin_user_serializer.rb +++ b/app/serializers/admin_user_serializer.rb @@ -14,8 +14,7 @@ class AdminUserSerializer < AdminUserListSerializer has_one :single_sign_on_record, serializer: SingleSignOnRecordSerializer, embed: :objects def can_approve - reviewable = ReviewableUser.find_by(target: object) - reviewable.present? && reviewable.actions_for(scope).has?(:approve) + scope.can_approve?(object) end def include_can_approve? diff --git a/config/initializers/014-track-setting-changes.rb b/config/initializers/014-track-setting-changes.rb new file mode 100644 index 00000000000..f4836d9be86 --- /dev/null +++ b/config/initializers/014-track-setting-changes.rb @@ -0,0 +1,38 @@ +# Enabling `must_approve_users` on an existing site is odd, so we assume that the +# existing users are approved. +DiscourseEvent.on(:site_setting_saved) do |site_setting| + name = site_setting.name.to_sym + next unless site_setting.value_changed? + + if name == :must_approve_users && site_setting.value == 't' + User.where(approved: false).update_all(approved: true) + end + + if name == :emoji_set + Emoji.clear_cache + + previous_value = site_setting.attribute_in_database(:value) || SiteSetting.defaults[:emoji_set] + before = "/images/emoji/#{previous_value}/" + after = "/images/emoji/#{site_setting.value}/" + + Scheduler::Defer.later("Fix Emoji Links") do + DB.exec("UPDATE posts SET cooked = REPLACE(cooked, :before, :after) WHERE cooked LIKE :like", + before: before, + after: after, + like: "%#{before}%" + ) + end + end + + Report.clear_cache(:storage_stats) if [:backup_location, :s3_backup_bucket].include?(name) + + if name == :slug_generation_method + Scheduler::Defer.later("Null topic slug") do + Topic.update_all(slug: nil) + end + end + + Jobs.enqueue(:update_s3_inventory) if [:s3_inventory, :s3_upload_bucket].include?(name) + + SvgSprite.expire_cache if name.to_s.include?("_icon") +end diff --git a/lib/discourse.rb b/lib/discourse.rb index 4ef1ebb15e0..a6476dc5af6 100644 --- a/lib/discourse.rb +++ b/lib/discourse.rb @@ -468,11 +468,6 @@ module Discourse end end - DiscourseEvent.on(:site_setting_saved) do |site_setting| - name = site_setting.name.to_s - Jobs.enqueue(:update_s3_inventory) if name.include?("s3_inventory") || name == "s3_upload_bucket" - end - def self.current_user_provider @current_user_provider || Auth::DefaultCurrentUserProvider end diff --git a/lib/guardian.rb b/lib/guardian.rb index 558de2cfbed..fa24d8426be 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -220,7 +220,7 @@ class Guardian # Can we approve it? def can_approve?(target) - is_staff? && target && target.active? && not(target.approved?) + is_staff? && target && target.active? && !target.approved? end def can_activate?(target) diff --git a/lib/site_settings/local_process_provider.rb b/lib/site_settings/local_process_provider.rb index e3974b0ea4d..dd9a1cedd78 100644 --- a/lib/site_settings/local_process_provider.rb +++ b/lib/site_settings/local_process_provider.rb @@ -4,7 +4,22 @@ class SiteSettings::LocalProcessProvider attr_accessor :current_site - Setting = Struct.new(:name, :value, :data_type) unless defined? SiteSettings::LocalProcessProvider::Setting + class Setting + attr_accessor :name, :data_type, :value + + def value_changed? + true + end + + def saved_change_to_value? + true + end + + def initialize(name, data_type) + self.name = name + self.data_type = data_type + end + end def settings @settings[current_site] ||= {} @@ -26,8 +41,14 @@ class SiteSettings::LocalProcessProvider def save(name, value, data_type) # NOTE: convert to string to simulate the conversion that is happening # when using DbProvider - value = value.to_s - settings[name] = Setting.new(name, value, data_type) + setting = settings[name] + if setting.blank? + setting = Setting.new(name, data_type) + settings[name] = setting + end + setting.value = value.to_s + DiscourseEvent.trigger(:site_setting_saved, setting) + setting end def destroy(name) diff --git a/lib/svg_sprite/svg_sprite.rb b/lib/svg_sprite/svg_sprite.rb index 955fea3dc8b..4ec7a1ff9af 100644 --- a/lib/svg_sprite/svg_sprite.rb +++ b/lib/svg_sprite/svg_sprite.rb @@ -329,7 +329,6 @@ License - https://fontawesome.com/license/free (Icons: CC BY 4.0, Fonts: SIL OFL end DiscourseEvent.on(:site_setting_saved) do |site_setting| - expire_cache if site_setting.name.to_s.include?("_icon") end def self.plugin_icons diff --git a/spec/components/site_settings/db_provider_spec.rb b/spec/components/site_settings/db_provider_spec.rb index 6cfec41d329..9ed47a79dcc 100644 --- a/spec/components/site_settings/db_provider_spec.rb +++ b/spec/components/site_settings/db_provider_spec.rb @@ -14,7 +14,7 @@ describe SiteSettings::DbProvider do end # integration test, requires db access - it "act correctly" do + it "acts correctly" do setting = Struct.new(:name, :value, :data_type) SiteSetting.destroy_all diff --git a/spec/components/site_settings/local_process_provider_spec.rb b/spec/components/site_settings/local_process_provider_spec.rb index 4b3909d8519..65ab40114e2 100644 --- a/spec/components/site_settings/local_process_provider_spec.rb +++ b/spec/components/site_settings/local_process_provider_spec.rb @@ -9,16 +9,10 @@ describe SiteSettings::LocalProcessProvider do expect(actual.data_type).to eq(expected.data_type) end - let :provider do - SiteSettings::LocalProcessProvider.new - end + let(:provider) { described_class.new } def setting(name, value, data_type) - OpenStruct.new.tap do |setting| - setting.name = name - setting.value = value - setting.data_type = data_type - end + described_class::Setting.new(name, data_type).tap { |s| s.value = value } end describe "all" do diff --git a/spec/jobs/enqueue_digest_emails_spec.rb b/spec/jobs/enqueue_digest_emails_spec.rb index 63d97c9e8e9..d4687932ad0 100644 --- a/spec/jobs/enqueue_digest_emails_spec.rb +++ b/spec/jobs/enqueue_digest_emails_spec.rb @@ -15,12 +15,13 @@ describe Jobs::EnqueueDigestEmails do end context 'unapproved users' do - let!(:unapproved_user) { Fabricate(:active_user, approved: false, last_emailed_at: 8.days.ago, last_seen_at: 10.days.ago) } before do SiteSetting.must_approve_users = true end + let!(:unapproved_user) { Fabricate(:active_user, approved: false, last_emailed_at: 8.days.ago, last_seen_at: 10.days.ago) } + it 'should enqueue the right digest emails' do expect(Jobs::EnqueueDigestEmails.new.target_user_ids.include?(unapproved_user.id)).to eq(false) diff --git a/spec/jobs/notify_mailing_list_subscribers_spec.rb b/spec/jobs/notify_mailing_list_subscribers_spec.rb index 18b67f20d55..a2be03645da 100644 --- a/spec/jobs/notify_mailing_list_subscribers_spec.rb +++ b/spec/jobs/notify_mailing_list_subscribers_spec.rb @@ -42,6 +42,8 @@ describe Jobs::NotifyMailingListSubscribers do before do SiteSetting.login_required = true SiteSetting.must_approve_users = true + + User.update_all(approved: false) end include_examples "no emails" end diff --git a/spec/models/category_spec.rb b/spec/models/category_spec.rb index b5412e394c8..14670022b50 100644 --- a/spec/models/category_spec.rb +++ b/spec/models/category_spec.rb @@ -434,7 +434,7 @@ describe Category do end it 'triggers a extensibility event' do - event = DiscourseEvent.track_events { @category.destroy }.first + event = DiscourseEvent.track(:category_destroyed) { @category.destroy } expect(event[:event_name]).to eq(:category_destroyed) expect(event[:params].first).to eq(@category) diff --git a/spec/models/reviewable_user_spec.rb b/spec/models/reviewable_user_spec.rb index 1b914d826d3..189e99fe4d6 100644 --- a/spec/models/reviewable_user_spec.rb +++ b/spec/models/reviewable_user_spec.rb @@ -100,6 +100,15 @@ RSpec.describe ReviewableUser, type: :model do end end + describe "changing must_approve_users" do + it "will approve any existing users" do + user = Fabricate(:user) + expect(user).not_to be_approved + SiteSetting.must_approve_users = true + expect(user.reload).to be_approved + end + end + describe 'when must_approve_users is true' do before do SiteSetting.must_approve_users = true diff --git a/spec/requests/admin/users_controller_spec.rb b/spec/requests/admin/users_controller_spec.rb index d376bf789e8..6c8aeae4a70 100644 --- a/spec/requests/admin/users_controller_spec.rb +++ b/spec/requests/admin/users_controller_spec.rb @@ -56,12 +56,11 @@ RSpec.describe Admin::UsersController do end describe '#approve' do + let(:evil_trout) { Fabricate(:evil_trout) } before do SiteSetting.must_approve_users = true end - let(:evil_trout) { Fabricate(:evil_trout) } - it "raises an error when the user doesn't have permission" do sign_in(user) put "/admin/users/#{evil_trout.id}/approve.json" @@ -70,6 +69,15 @@ RSpec.describe Admin::UsersController do expect(evil_trout.approved).to eq(false) end + it "will create a reviewable if one does not exist" do + evil_trout.update!(active: true) + expect(ReviewableUser.find_by(target: evil_trout)).to be_blank + put "/admin/users/#{evil_trout.id}/approve.json" + expect(response.code).to eq("200") + expect(ReviewableUser.find_by(target: evil_trout)).to be_present + expect(evil_trout.reload).to be_approved + end + it 'calls approve' do Jobs.run_immediately! evil_trout.activate diff --git a/spec/requests/session_controller_spec.rb b/spec/requests/session_controller_spec.rb index 2795ffdb09f..d16442a34b7 100644 --- a/spec/requests/session_controller_spec.rb +++ b/spec/requests/session_controller_spec.rb @@ -1111,6 +1111,7 @@ RSpec.describe SessionController do context 'with an unapproved user' do before do + user.update_columns(approved: false) post "/session.json", params: { login: user.email, password: 'myawesomepassword' }