Merge branch 'refactoring' of git://github.com/mattvanhorn/discourse

Conflicts:
	lib/text_sentinel.rb

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

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

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
 

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?

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

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
 

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?
 

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

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

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

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