From c9fcee8490f502eed0bcda9bac657896c092c45b Mon Sep 17 00:00:00 2001
From: Matt Van Horn <mattvanhorn@gmail.com>
Date: Thu, 23 May 2013 10:48:37 -0700
Subject: [PATCH 1/6] simplify, clarify TextSentinel

codeclimate pointed this out. I agree it is better
to simplify and reveal intentions.
---
 lib/text_sentinel.rb | 50 +++++++++++++++++++++++++++-----------------
 1 file changed, 31 insertions(+), 19 deletions(-)

diff --git a/lib/text_sentinel.rb b/lib/text_sentinel.rb
index 439781b71..7ed52d53e 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,39 @@ 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(/\W/).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

From 247a0b3ea1960da51e12cc471a4d1129f0f81b8f Mon Sep 17 00:00:00 2001
From: Matt Van Horn <mattvanhorn@gmail.com>
Date: Thu, 23 May 2013 17:18:59 -0700
Subject: [PATCH 2/6] small refactor of RateLimiter for clarity

---
 lib/rate_limiter.rb | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/lib/rate_limiter.rb b/lib/rate_limiter.rb
index 3c027294e..5d794e9c9 100644
--- a/lib/rate_limiter.rb
+++ b/lib/rate_limiter.rb
@@ -21,18 +21,11 @@ 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
@@ -50,4 +43,13 @@ class RateLimiter
     $redis.decr(@key)
   end
 
+  private
+
+  def is_under_limit?
+    $redis.get(@key).to_i < @max
+  end
+
+  def rate_unlimited?
+    !!(RateLimiter.disabled? || @user.staff?)
+  end
 end

From d7817cf314be9e40ad6c2d7c946222cd1b1dd021 Mon Sep 17 00:00:00 2001
From: Matt Van Horn <mattvanhorn@gmail.com>
Date: Thu, 23 May 2013 23:06:38 -0700
Subject: [PATCH 3/6] extract TopicNotifier class from topic

---
 app/controllers/topics_controller.rb       |  2 +-
 app/models/topic.rb                        | 49 +++++++++++-----------
 app/models/topic_notifier.rb               | 42 +++++++++++++++++++
 spec/controllers/topics_controller_spec.rb |  4 +-
 4 files changed, 69 insertions(+), 28 deletions(-)
 create mode 100644 app/models/topic_notifier.rb

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 9563ddc2e..ef15c0316 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?
@@ -641,11 +636,6 @@ class Topic < ActiveRecord::Base
     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] )
-  end
-
   def slug
     unless slug = read_attribute(:slug)
       return '' unless title.present?
@@ -661,17 +651,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
@@ -684,12 +674,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)
@@ -703,21 +687,36 @@ 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)
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/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
 

From e5e904aa4e17a5bf0f8a0c1bc6330701520ea00d Mon Sep 17 00:00:00 2001
From: Matt Van Horn <mattvanhorn@gmail.com>
Date: Fri, 24 May 2013 09:13:31 -0700
Subject: [PATCH 4/6] minor refactorings

---
 app/models/topic.rb      | 11 +++++------
 app/models/topic_user.rb |  3 +++
 lib/guardian.rb          |  2 +-
 3 files changed, 9 insertions(+), 7 deletions(-)

diff --git a/app/models/topic.rb b/app/models/topic.rb
index ef15c0316..e7bb0f060 100644
--- a/app/models/topic.rb
+++ b/app/models/topic.rb
@@ -215,7 +215,7 @@ class Topic < ActiveRecord::Base
   end
 
   def private_message?
-    self.archetype == Archetype.private_message
+    archetype == Archetype.private_message
   end
 
   def links_grouped
@@ -532,10 +532,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?
@@ -633,7 +631,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
+    TopicUser.starred_since(sinceDaysAgo).by_date_starred.count
   end
 
   def slug
@@ -721,7 +719,8 @@ class Topic < ActiveRecord::Base
 
   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_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?
 

From e72694c4ee0043f6b88715ec823ff692b4679da3 Mon Sep 17 00:00:00 2001
From: Matt Van Horn <mattvanhorn@gmail.com>
Date: Fri, 24 May 2013 13:35:16 -0700
Subject: [PATCH 5/6] Make pry a bit more useful

---
 Gemfile      | 3 ++-
 Gemfile.lock | 3 +++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/Gemfile b/Gemfile
index 4fc40030f..10da0bc8c 100644
--- a/Gemfile
+++ b/Gemfile
@@ -109,13 +109,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 0498faa13..6b7faa26f 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)
@@ -514,6 +516,7 @@ DEPENDENCIES
   omniauth-twitter
   openid-redis-store
   pg
+  pry-nav
   pry-rails
   rack-cache
   rack-cors

From d5958f877984383b30f6340aeeb1734caa1711bd Mon Sep 17 00:00:00 2001
From: Matt Van Horn <mattvanhorn@gmail.com>
Date: Sat, 25 May 2013 12:37:28 -0700
Subject: [PATCH 6/6] Sliding window rate limiting

Switched the algorithm to use a circular buffer
based on a redis list
---
 lib/rate_limiter.rb                  | 30 ++++++++++++++++++----------
 spec/components/rate_limiter_spec.rb |  2 +-
 2 files changed, 21 insertions(+), 11 deletions(-)

diff --git a/lib/rate_limiter.rb b/lib/rate_limiter.rb
index 5d794e9c9..4c349e4f7 100644
--- a/lib/rate_limiter.rb
+++ b/lib/rate_limiter.rb
@@ -27,26 +27,36 @@ class RateLimiter
   def performed!
     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?
-    $redis.get(@key).to_i < @max
+      # 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?
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