diff --git a/Gemfile b/Gemfile index 3437f1328..d6e46cf74 100644 --- a/Gemfile +++ b/Gemfile @@ -110,13 +110,14 @@ group :test, :development do gem 'terminal-notifier-guard', require: false gem 'timecop' gem 'rspec-given' + gem 'pry-rails' + gem 'pry-nav' end group :development do gem 'better_errors' gem 'binding_of_caller' gem 'librarian', '>= 0.0.25', require: false - gem 'pry-rails' # https://github.com/ctran/annotate_models/pull/106 gem 'annotate', :git => 'https://github.com/SamSaffron/annotate_models.git' end diff --git a/Gemfile.lock b/Gemfile.lock index 1208a4052..89761e8ee 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -312,6 +312,8 @@ GEM coderay (~> 1.0.5) method_source (~> 0.8) slop (~> 3.4) + pry-nav (0.2.3) + pry (~> 0.9.10) pry-rails (0.2.2) pry (>= 0.9.10) rack (1.4.5) @@ -518,6 +520,7 @@ DEPENDENCIES omniauth-twitter openid-redis-store pg + pry-nav pry-rails rack-cache rack-cors diff --git a/app/controllers/topics_controller.rb b/app/controllers/topics_controller.rb index f8d3585f3..50ccc67ab 100644 --- a/app/controllers/topics_controller.rb +++ b/app/controllers/topics_controller.rb @@ -203,7 +203,7 @@ class TopicsController < ApplicationController @topic = Topic.where(id: params[:topic_id].to_i).first guardian.ensure_can_see!(@topic) - @topic.toggle_mute(current_user, v) + @topic.toggle_mute(current_user) render nothing: true end diff --git a/app/models/topic.rb b/app/models/topic.rb index 6b606d040..5f5b8c0bd 100644 --- a/app/models/topic.rb +++ b/app/models/topic.rb @@ -112,9 +112,7 @@ class Topic < ActiveRecord::Base after_create do changed_to_category(category) - TopicUser.change(user_id, id, - notification_level: TopicUser.notification_levels[:watching], - notifications_reason_id: TopicUser.notification_reasons[:created_topic]) + notifier.created_topic! user_id if archetype == Archetype.private_message DraftSequence.next!(user, Draft::NEW_PRIVATE_MESSAGE) else @@ -162,10 +160,7 @@ class Topic < ActiveRecord::Base end def sanitize_title - if self.title.present? - self.title = sanitize(title, tags: [], attributes: []) - self.title.strip! - end + self.title = sanitize(title.to_s, tags: [], attributes: []).strip.presence end def new_version_required? @@ -220,7 +215,7 @@ class Topic < ActiveRecord::Base end def private_message? - self.archetype == Archetype.private_message + archetype == Archetype.private_message end def links_grouped @@ -485,10 +480,8 @@ class Topic < ActiveRecord::Base def feature_topic_users(args={}) reload - to_feature = posts - # Don't include the OP or the last poster - to_feature = to_feature.where('user_id NOT IN (?, ?)', user_id, last_post_user_id) + to_feature = posts.where('user_id NOT IN (?, ?)', user_id, last_post_user_id) # Exclude a given post if supplied (in the case of deletes) to_feature = to_feature.where("id <> ?", args[:except_post_id]) if args[:except_post_id].present? @@ -534,12 +527,7 @@ class Topic < ActiveRecord::Base end def self.starred_counts_per_day(sinceDaysAgo=30) - TopicUser.where('starred_at > ?', sinceDaysAgo.days.ago).group('date(starred_at)').order('date(starred_at)').count - end - - # Enable/disable the mute on the topic - def toggle_mute(user, muted) - TopicUser.change(user, self.id, notification_level: muted?(user) ? TopicUser.notification_levels[:regular] : TopicUser.notification_levels[:muted] ) + TopicUser.starred_since(sinceDaysAgo).by_date_starred.count end def slug @@ -557,17 +545,17 @@ class Topic < ActiveRecord::Base end def title=(t) - slug = "" - slug = (Slug.for(t).presence || "topic") if t.present? + slug = (Slug.for(t.to_s).presence || "topic") write_attribute(:slug, slug) write_attribute(:title,t) end + # NOTE: These are probably better off somewhere else. + # Having a model know about URLs seems a bit strange. def last_post_url "/t/#{slug}/#{id}/#{posts_count}" end - def self.url(id, slug, post_number=nil) url = "#{Discourse.base_url}/t/#{slug}/#{id}" url << "/#{post_number}" if post_number.to_i > 1 @@ -584,12 +572,6 @@ class Topic < ActiveRecord::Base url end - def muted?(user) - return false unless user && user.id - tu = topic_users.where(user_id: user.id).first - tu && tu.notification_level == TopicUser.notification_levels[:muted] - end - def clear_pin_for(user) return unless user.present? TopicUser.change(user.id, id, cleared_pinned_at: Time.now) @@ -603,26 +585,42 @@ class Topic < ActiveRecord::Base "#{Draft::EXISTING_TOPIC}#{id}" end + def notifier + @topic_notifier ||= TopicNotifier.new(self) + end + # notification stuff def notify_watch!(user) - TopicUser.change(user, id, notification_level: TopicUser.notification_levels[:watching]) + notifier.watch! user end def notify_tracking!(user) - TopicUser.change(user, id, notification_level: TopicUser.notification_levels[:tracking]) + notifier.tracking! user end def notify_regular!(user) - TopicUser.change(user, id, notification_level: TopicUser.notification_levels[:regular]) + notifier.regular! user end def notify_muted!(user) - TopicUser.change(user, id, notification_level: TopicUser.notification_levels[:muted]) + notifier.muted! user + end + + def muted?(user) + if user && user.id + notifier.muted?(user.id) + end + end + + # Enable/disable the mute on the topic + def toggle_mute(user_id) + notifier.toggle_mute user_id end def auto_close_days=(num_days) @ignore_category_auto_close = true - self.auto_close_at = (num_days and num_days.to_i > 0.0 ? num_days.to_i.days.from_now : nil) + num_days = num_days.to_i + self.auto_close_at = (num_days > 0 ? num_days.days.from_now : nil) end def secure_category? diff --git a/app/models/topic_notifier.rb b/app/models/topic_notifier.rb new file mode 100644 index 000000000..5ac1de2ed --- /dev/null +++ b/app/models/topic_notifier.rb @@ -0,0 +1,42 @@ +class TopicNotifier + def initialize(topic) + @topic = topic + end + + { :watch! => :watching, + :tracking! => :tracking, + :regular! => :regular, + :muted! => :muted }.each_pair do |method_name, level| + + define_method method_name do |user_id| + change_level user_id, level + end + + end + + def created_topic!(user_id) + change_level user_id, :watching, :created_topic + end + + # Enable/disable the mute on the topic + def toggle_mute(user_id) + change_level user_id, (muted?(user_id) ? levels[:regular] : levels[:muted]) + end + + def muted?(user_id) + tu = @topic.topic_users.where(user_id: user_id).first + tu && tu.notification_level == levels[:muted] + end + + private + + def levels + @notification_levels ||= TopicUser.notification_levels + end + + def change_level(user_id, level, reason=nil) + attrs = {notification_level: levels[level]} + attrs.merge!(notifications_reason_id: TopicUser.notification_reasons[reason]) if reason + TopicUser.change(user_id, @topic.id, attrs) + end +end diff --git a/app/models/topic_user.rb b/app/models/topic_user.rb index 97aa9915e..e0d98d8be 100644 --- a/app/models/topic_user.rb +++ b/app/models/topic_user.rb @@ -2,6 +2,9 @@ class TopicUser < ActiveRecord::Base belongs_to :user belongs_to :topic + scope :starred_since, lambda { |sinceDaysAgo| where('starred_at > ?', sinceDaysAgo.days.ago) } + scope :by_date_starred, group('date(starred_at)').order('date(starred_at)') + # Class methods class << self diff --git a/lib/guardian.rb b/lib/guardian.rb index 13f5b5362..d21bf0a95 100644 --- a/lib/guardian.rb +++ b/lib/guardian.rb @@ -109,7 +109,7 @@ class Guardian alias :can_activate? :can_approve? def can_ban?(user) - user && is_staff? && not(user.staff?) + user && is_staff? && user.regular? end alias :can_deactivate? :can_ban? diff --git a/lib/rate_limiter.rb b/lib/rate_limiter.rb index 3c027294e..4c349e4f7 100644 --- a/lib/rate_limiter.rb +++ b/lib/rate_limiter.rb @@ -21,33 +21,45 @@ class RateLimiter end def can_perform? - return true if RateLimiter.disabled? - return true if @user.staff? - - result = $redis.get(@key) - return true if result.blank? - return true if result.to_i < @max - false + rate_unlimited? || is_under_limit? end def performed! - return if RateLimiter.disabled? - return if @user.staff? + return if rate_unlimited? - result = $redis.incr(@key).to_i - $redis.expire(@key, @secs) if result == 1 - if result > @max - - # In case we go over, clamp it to the maximum - $redis.decr(@key) - - raise LimitExceeded.new($redis.ttl(@key)) + if is_under_limit? + # simple ring buffer. + $redis.lpush(@key, Time.now.to_i) + $redis.ltrim(@key, 0, @max - 1) + else + raise LimitExceeded.new(seconds_to_wait) end end def rollback! return if RateLimiter.disabled? - $redis.decr(@key) + $redis.lpop(@key) end + private + + def seconds_to_wait + @secs - age_of_oldest + end + + def age_of_oldest + # age of oldest event in buffer, in seconds + Time.now.to_i - $redis.lrange(@key, -1, -1).first.to_i + end + + def is_under_limit? + # number of events in buffer less than max allowed? OR + ($redis.llen(@key) < @max) || + # age bigger than silding window size? + (age_of_oldest > @secs) + end + + def rate_unlimited? + !!(RateLimiter.disabled? || @user.staff?) + end end diff --git a/lib/text_sentinel.rb b/lib/text_sentinel.rb index 7c2d818ca..e7c75f64d 100644 --- a/lib/text_sentinel.rb +++ b/lib/text_sentinel.rb @@ -10,10 +10,6 @@ class TextSentinel @text = text.to_s.encode('UTF-8', invalid: :replace, undef: :replace, replace: '') end - def self.non_symbols_regexp - /[\ -\/\[-\`\:-\@\{-\~]/m - end - def self.body_sentinel(text) TextSentinel.new(text, min_entropy: SiteSetting.body_min_entropy) end @@ -30,23 +26,40 @@ class TextSentinel end def valid? - # Blank strings are not valid @text.present? && - - # Minimum entropy if entropy check required - (@opts[:min_entropy].blank? || (entropy >= @opts[:min_entropy])) && - - # At least some non-symbol characters - # (We don't have a comprehensive list of symbols, but this will eliminate some noise) - (@text.gsub(TextSentinel.non_symbols_regexp, '').size > 0) && - - # Don't allow super long words if there is a word length maximum - (@opts[:max_word_length].blank? || @text.split(/\s/).map(&:size).max <= @opts[:max_word_length] ) && - - # We don't allow all upper case content in english - not((@text =~ /[A-Z]+/) && (@text == @text.upcase)) && - + seems_meaningful? && + seems_pronounceable? && + seems_unpretentious? && + seems_quiet? && true end + private + + def symbols_regex + /[\ -\/\[-\`\:-\@\{-\~]/m + end + + def seems_meaningful? + # Minimum entropy if entropy check required + @opts[:min_entropy].blank? || (entropy >= @opts[:min_entropy]) + end + + def seems_pronounceable? + # At least some non-symbol characters + # (We don't have a comprehensive list of symbols, but this will eliminate some noise) + @text.gsub(symbols_regex, '').size > 0 + end + + def seems_unpretentious? + # Don't allow super long words if there is a word length maximum + @opts[:max_word_length].blank? || (@text.split(/\W/).map(&:size).max <= @opts[:max_word_length]) + end + + + def seems_quiet? + # We don't allow all upper case content in english + not((@text =~ /[A-Z]+/) && (@text == @text.upcase)) + end + end diff --git a/spec/components/rate_limiter_spec.rb b/spec/components/rate_limiter_spec.rb index 8d763a400..7b2f27f50 100644 --- a/spec/components/rate_limiter_spec.rb +++ b/spec/components/rate_limiter_spec.rb @@ -50,7 +50,7 @@ describe RateLimiter do end it "raises an error the third time called" do - lambda { rate_limiter.performed! }.should raise_error + lambda { rate_limiter.performed! }.should raise_error(RateLimiter::LimitExceeded) end context "as an admin/moderator" do diff --git a/spec/controllers/topics_controller_spec.rb b/spec/controllers/topics_controller_spec.rb index 01d9f531d..ff790c9fc 100644 --- a/spec/controllers/topics_controller_spec.rb +++ b/spec/controllers/topics_controller_spec.rb @@ -282,12 +282,12 @@ describe TopicsController do end it "changes the user's starred flag when the parameter is present" do - Topic.any_instance.expects(:toggle_mute).with(@topic.user, true) + Topic.any_instance.expects(:toggle_mute).with(@topic.user) xhr :put, :mute, topic_id: @topic.id, starred: 'true' end it "removes the user's starred flag when the parameter is not true" do - Topic.any_instance.expects(:toggle_mute).with(@topic.user, false) + Topic.any_instance.expects(:toggle_mute).with(@topic.user) xhr :put, :unmute, topic_id: @topic.id, starred: 'false' end