Cleaned up TopicUserSpec, introduces clearing of pinned topics

Gemfile

@@ -1,4 +1,4 @@
-source 'https://rubygems.org'
+source 'http://rubygems.org'
 
 gem 'active_model_serializers', git: 'git://github.com/rails-api/active_model_serializers.git'
 gem 'ember-rails', git: 'git://github.com/emberjs/ember-rails.git' # so we get the pre version

Gemfile.lock

@@ -71,7 +71,7 @@ PATH
       rails (~> 3.1)
 
 GEM
-  remote: https://rubygems.org/
+  remote: http://rubygems.org/
   specs:
     actionmailer (3.2.12)
       actionpack (= 3.2.12)

app/assets/javascripts/discourse/controllers/topic_controller.js

@@ -255,6 +255,15 @@ Discourse.TopicController = Discourse.ObjectController.extend({
     this.get('content').toggleStar();
   },
 
+  /**
+    Clears the pin from a topic for the currentUser
+
+    @method clearPin
+  **/
+  clearPin: function() {
+    this.get('content').clearPin();
+  },
+
   // Receive notifications for this topic
   subscribe: function() {
     var bus,

app/assets/javascripts/discourse/models/topic.js

@@ -329,6 +329,27 @@ Discourse.Topic = Discourse.Model.extend({
     });
   },
 
+  /**
+    Clears the pin from a topic for the currentUser
+
+    @method clearPin
+  **/
+  clearPin: function() {
+
+    var topic = this;
+
+    // Clear the pin optimistically from the object
+    topic.set('pinned', false);
+
+    $.ajax("/t/" + this.get('id') + "/clear-pin", {
+      type: 'PUT',
+      error: function() {
+        // On error, put the pin back
+        topic.set('pinned', true);
+      }
+    });
+  },
+
   // Is the reply to a post directly below it?
   isReplyDirectlyBelow: function(post) {
     var postBelow, posts;

app/assets/javascripts/discourse/views/topic_footer_buttons_view.js

@@ -68,6 +68,30 @@ Discourse.TopicFooterButtonsView = Ember.ContainerView.extend({
             buffer.push("<i class='icon icon-share'></i>");
           }
         }));
+
+        // Add our clear pin button
+        this.addObject(Discourse.ButtonView.createWithMixins({
+          textKey: 'topic.clear_pin.title',
+          helpKey: 'topic.clear_pin.help',
+          classNameBindings: ['unpinned'],
+
+          // Hide the button if it becomes unpinned
+          unpinned: function() {
+            // When not logged in don't show the button
+            if (!Discourse.get('currentUser')) return 'hidden'
+
+            return this.get('controller.pinned') ? null : 'hidden';
+          }.property('controller.pinned'),
+
+          click: function(buffer) {
+            this.get('controller').clearPin();
+          },
+
+          renderIcon: function(buffer) {
+            buffer.push("<i class='icon icon-pushpin'></i>");
+          }
+        }));
+
       }
 
       this.addObject(Discourse.ButtonView.createWithMixins({

app/controllers/topics_controller.rb

@@ -14,7 +14,9 @@ class TopicsController < ApplicationController
                                           :mute,
                                           :unmute,
                                           :set_notifications,
-                                          :move_posts]
+                                          :move_posts,
+                                          :clear_pin]
+
   before_filter :consider_user_for_promotion, only: :show
 
   skip_before_filter :check_xhr, only: [:avatar, :show, :feed]
@@ -127,16 +129,21 @@ class TopicsController < ApplicationController
     end
   end
 
+  def clear_pin
+    topic = Topic.where(id: params[:topic_id].to_i).first
+    guardian.ensure_can_see!(topic)
+    topic.clear_pin_for(current_user)
+    render nothing: true
+  end
+
   def timings
-
     PostTiming.process_timings(
-        current_user,
+      current_user,
-        params[:topic_id].to_i,
+      params[:topic_id].to_i,
-        params[:highest_seen].to_i,
+      params[:highest_seen].to_i,
-        params[:topic_time].to_i,
+      params[:topic_time].to_i,
-        (params[:timings] || []).map{|post_number, t| [post_number.to_i, t.to_i]}
+      (params[:timings] || []).map{|post_number, t| [post_number.to_i, t.to_i]}
     )
-
     render nothing: true
   end
 

app/models/post.rb

@@ -54,7 +54,7 @@ class Post < ActiveRecord::Base
   after_commit :store_unique_post_key, on: :create
 
   after_create do
-    TopicUser.auto_track(user_id, topic_id, TopicUser::NotificationReasons::CREATED_POST)
+    TopicUser.auto_track(user_id, topic_id, TopicUser.notification_reasons[:created_post])
   end
 
   scope :by_newest, order('created_at desc, id desc')

app/models/post_alert_observer.rb

@@ -80,7 +80,7 @@ class PostAlertObserver < ActiveRecord::Observer
       return unless Guardian.new(user).can_see?(post)
 
       # skip if muted on the topic
-      return if TopicUser.get(post.topic, user).try(:notification_level) == TopicUser::NotificationLevel::MUTED
+      return if TopicUser.get(post.topic, user).try(:notification_level) == TopicUser.notification_levels[:muted]
 
       # Don't notify the same user about the same notification on the same post
       return if user.notifications.exists?(notification_type: type, topic_id: post.topic_id, post_number: post.post_number)
@@ -132,7 +132,7 @@ class PostAlertObserver < ActiveRecord::Observer
         exclude_user_ids << extract_mentioned_users(post).map(&:id)
         exclude_user_ids << extract_quoted_users(post).map(&:id)
         exclude_user_ids.flatten!
-        TopicUser.where(topic_id: post.topic_id, notification_level: TopicUser::NotificationLevel::WATCHING).includes(:user).each do |tu|
+        TopicUser.where(topic_id: post.topic_id, notification_level: TopicUser.notification_levels[:watching]).includes(:user).each do |tu|
           create_notification(tu.user, Notification.types[:posted], post) unless exclude_user_ids.include?(tu.user_id)
         end
       end

app/models/topic.rb

@@ -83,11 +83,9 @@ class Topic < ActiveRecord::Base
 
   after_create do
     changed_to_category(category)
-    TopicUser.change(
+    TopicUser.change(user_id, id,
-                     user_id, id,
+                     notification_level: TopicUser.notification_levels[:watching],
-                     notification_level: TopicUser::NotificationLevel::WATCHING,
+                     notifications_reason_id: TopicUser.notification_reasons[:created_topic])
-                     notifications_reason_id: TopicUser::NotificationReasons::CREATED_TOPIC
-                    )
     if archetype == Archetype.private_message
       DraftSequence.next!(user, Draft::NEW_PRIVATE_MESSAGE)
     else
@@ -206,8 +204,17 @@ class Topic < ActiveRecord::Base
   end
 
   def update_status(property, status, user)
+
     Topic.transaction do
-      update_column(property, status)
+
+      # Special case: if it's pinned, update that
+      if property.to_sym == :pinned
+        update_pinned(status)
+      else
+        # otherwise update the column
+        update_column(property, status)
+      end
+
       key = "topic_statuses.#{property}_"
       key << (status ? 'enabled' : 'disabled')
 
@@ -506,7 +513,7 @@ class Topic < ActiveRecord::Base
 
   # Enable/disable the mute on the topic
   def toggle_mute(user, muted)
-    TopicUser.change(user, self.id, notification_level: muted?(user) ? TopicUser::NotificationLevel::REGULAR : TopicUser::NotificationLevel::MUTED )
+    TopicUser.change(user, self.id, notification_level: muted?(user) ? TopicUser.notification_levels[:regular] : TopicUser.notification_levels[:muted] )
   end
 
   def slug
@@ -526,7 +533,17 @@ class Topic < ActiveRecord::Base
   def muted?(user)
     return false unless user && user.id
     tu = topic_users.where(user_id: user.id).first
-    tu && tu.notification_level == TopicUser::NotificationLevel::MUTED
+    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)
+  end
+
+  def update_pinned(status)
+    update_column(:pinned_at, status ? Time.now : nil)
   end
 
   def draft_key
@@ -535,18 +552,18 @@ class Topic < ActiveRecord::Base
 
   # notification stuff
   def notify_watch!(user)
-    TopicUser.change(user, id, notification_level: TopicUser::NotificationLevel::WATCHING)
+    TopicUser.change(user, id, notification_level: TopicUser.notification_levels[:watching])
   end
 
   def notify_tracking!(user)
-    TopicUser.change(user, id, notification_level: TopicUser::NotificationLevel::TRACKING)
+    TopicUser.change(user, id, notification_level: TopicUser.notification_levels[:tracking])
   end
 
   def notify_regular!(user)
-    TopicUser.change(user, id, notification_level: TopicUser::NotificationLevel::REGULAR)
+    TopicUser.change(user, id, notification_level: TopicUser.notification_levels[:regular])
   end
 
   def notify_muted!(user)
-    TopicUser.change(user, id, notification_level: TopicUser::NotificationLevel::MUTED)
+    TopicUser.change(user, id, notification_level: TopicUser.notification_levels[:muted])
   end
 end

app/models/topic_user.rb

@@ -2,185 +2,173 @@ class TopicUser < ActiveRecord::Base
   belongs_to :user
   belongs_to :topic
 
-  module NotificationLevel
+  # Class methods
-    WATCHING = 3
+  class << self
-    TRACKING = 2
-    REGULAR = 1
-    MUTED = 0
-  end
 
-  module NotificationReasons
+    # Enums
-    CREATED_TOPIC = 1
+    def notification_levels
-    USER_CHANGED = 2
+      @notification_levels ||= Enum.new(:muted, :regular, :tracking, :watching, start: 0)
-    USER_INTERACTED = 3
+    end
-    CREATED_POST = 4
-  end
 
-  def self.auto_track(user_id, topic_id, reason)
+    def notification_reasons
-    if TopicUser.where(user_id: user_id, topic_id: topic_id, notifications_reason_id: nil).exists?
+      @notification_reasons ||= Enum.new(:created_topic, :user_changed, :user_interacted, :created_post)
-      change(user_id, topic_id,
+    end
-          notification_level: NotificationLevel::TRACKING,
+
+    def auto_track(user_id, topic_id, reason)
+      if TopicUser.where(user_id: user_id, topic_id: topic_id, notifications_reason_id: nil).exists?
+        change(user_id, topic_id,
+          notification_level: notification_levels[:tracking],
           notifications_reason_id: reason
-      )
+        )
 
-      MessageBus.publish("/topic/#{topic_id}", {
+        MessageBus.publish("/topic/#{topic_id}", {
-        notification_level_change: NotificationLevel::TRACKING,
+          notification_level_change: notification_levels[:tracking],
-        notifications_reason_id: reason
+          notifications_reason_id: reason
-      }, user_ids: [user_id])
+        }, user_ids: [user_id])
-    end
-  end
-
-
-  # Find the information specific to a user in a forum topic
-  def self.lookup_for(user, topics)
-    # If the user isn't logged in, there's no last read posts
-    return {} if user.blank? || topics.blank?
-
-    topic_ids = topics.map(&:id)
-    create_lookup(TopicUser.where(topic_id: topic_ids, user_id: user.id))
-  end
-
-  def self.create_lookup(topic_users)
-    topic_users = topic_users.to_a
-
-    result = {}
-    return result if topic_users.blank?
-
-    topic_users.each do |ftu|
-      result[ftu.topic_id] = ftu
-    end
-    result
-  end
-
-  def self.get(topic,user)
-    if Topic === topic
-      topic = topic.id
-    end
-    if User === user
-      user = user.id
-    end
-
-    TopicUser.where('topic_id = ? and user_id = ?', topic, user).first
-  end
-
-  # Change attributes for a user (creates a record when none is present). First it tries an update
-  # since there's more likely to be an existing record than not. If the update returns 0 rows affected
-  # it then creates the row instead.
-  def self.change(user_id, topic_id, attrs)
-    # Sometimes people pass objs instead of the ids. We can handle that.
-    topic_id = topic_id.id if topic_id.is_a?(Topic)
-    user_id = user_id.id if user_id.is_a?(User)
-
-    TopicUser.transaction do
-      attrs = attrs.dup
-      attrs[:starred_at] = DateTime.now if attrs[:starred_at].nil? && attrs[:starred]
-
-      if attrs[:notification_level]
-        attrs[:notifications_changed_at] ||= DateTime.now
-        attrs[:notifications_reason_id] ||= TopicUser::NotificationReasons::USER_CHANGED
       end
-      attrs_array = attrs.to_a
+    end
 
-      attrs_sql = attrs_array.map { |t| "#{t[0]} = ?" }.join(", ")
+    # Find the information specific to a user in a forum topic
-      vals = attrs_array.map { |t| t[1] }
+    def lookup_for(user, topics)
-      rows = TopicUser.update_all([attrs_sql, *vals], topic_id: topic_id.to_i, user_id: user_id)
+      # If the user isn't logged in, there's no last read posts
+      return {} if user.blank? || topics.blank?
 
-      if rows == 0
+      topic_ids = topics.map(&:id)
-        now = DateTime.now
+      create_lookup(TopicUser.where(topic_id: topic_ids, user_id: user.id))
-        auto_track_after = self.exec_sql("select auto_track_topics_after_msecs from users where id = ?", user_id).values[0][0]
+    end
-        auto_track_after ||= SiteSetting.auto_track_topics_after
-        auto_track_after = auto_track_after.to_i
 
-        if auto_track_after >= 0 && auto_track_after <= (attrs[:total_msecs_viewed] || 0)
+    def create_lookup(topic_users)
-          attrs[:notification_level] ||= TopicUser::NotificationLevel::TRACKING
+      topic_users = topic_users.to_a
+
+      result = {}
+      return result if topic_users.blank?
+
+      topic_users.each do |ftu|
+        result[ftu.topic_id] = ftu
+      end
+      result
+    end
+
+    def get(topic,user)
+      topic = topic.id if Topic === topic
+      user = user.id if User === user
+      TopicUser.where('topic_id = ? and user_id = ?', topic, user).first
+    end
+
+    # Change attributes for a user (creates a record when none is present). First it tries an update
+    # since there's more likely to be an existing record than not. If the update returns 0 rows affected
+    # it then creates the row instead.
+    def change(user_id, topic_id, attrs)
+      # Sometimes people pass objs instead of the ids. We can handle that.
+      topic_id = topic_id.id if topic_id.is_a?(Topic)
+      user_id = user_id.id if user_id.is_a?(User)
+
+      TopicUser.transaction do
+        attrs = attrs.dup
+        attrs[:starred_at] = DateTime.now if attrs[:starred_at].nil? && attrs[:starred]
+
+        if attrs[:notification_level]
+          attrs[:notifications_changed_at] ||= DateTime.now
+          attrs[:notifications_reason_id] ||= TopicUser.notification_reasons[:user_changed]
         end
+        attrs_array = attrs.to_a
 
-        TopicUser.create(attrs.merge!(user_id: user_id, topic_id: topic_id.to_i, first_visited_at: now ,last_visited_at: now))
+        attrs_sql = attrs_array.map { |t| "#{t[0]} = ?" }.join(", ")
+        vals = attrs_array.map { |t| t[1] }
+        rows = TopicUser.update_all([attrs_sql, *vals], topic_id: topic_id.to_i, user_id: user_id)
+
+        if rows == 0
+          now = DateTime.now
+          auto_track_after = User.select(:auto_track_topics_after_msecs).where(id: user_id).first.auto_track_topics_after_msecs
+          auto_track_after ||= SiteSetting.auto_track_topics_after
+
+          if auto_track_after >= 0 && auto_track_after <= (attrs[:total_msecs_viewed] || 0)
+            attrs[:notification_level] ||= notification_levels[:tracking]
+          end
+
+          TopicUser.create(attrs.merge!(user_id: user_id, topic_id: topic_id.to_i, first_visited_at: now ,last_visited_at: now))
+        end
       end
+    rescue ActiveRecord::RecordNotUnique
+      # In case of a race condition to insert, do nothing
     end
-  rescue ActiveRecord::RecordNotUnique
-    # In case of a race condition to insert, do nothing
-  end
 
-  def self.track_visit!(topic,user)
+    def track_visit!(topic,user)
-    now = DateTime.now
+      now = DateTime.now
-    rows = exec_sql_row_count(
+      rows = TopicUser.update_all({last_visited_at: now}, {topic_id: topic.id, user_id: user.id})
-      "update topic_users set last_visited_at=? where topic_id=? and user_id=?",
+      if rows == 0
-      now, topic.id, user.id
+        TopicUser.create(topic_id: topic.id, user_id: user.id, last_visited_at: now, first_visited_at: now)
-    )
-
-    if rows == 0
-      exec_sql('insert into topic_users(topic_id, user_id, last_visited_at, first_visited_at)
-               values(?,?,?,?)',
-               topic.id, user.id, now, now)
-    end
-  end
-
-  # Update the last read and the last seen post count, but only if it doesn't exist.
-  # This would be a lot easier if psql supported some kind of upsert
-  def self.update_last_read(user, topic_id, post_number, msecs)
-    return if post_number.blank?
-    msecs = 0 if msecs.to_i < 0
-
-    args = {
-      user_id: user.id,
-      topic_id: topic_id,
-      post_number: post_number,
-      now: DateTime.now,
-      msecs: msecs,
-      tracking: TopicUser::NotificationLevel::TRACKING,
-      threshold: SiteSetting.auto_track_topics_after
-    }
-
-    rows = exec_sql("UPDATE topic_users
-                                  SET
-                                    last_read_post_number = greatest(:post_number, tu.last_read_post_number),
-                                    seen_post_count = t.highest_post_number,
-                                    total_msecs_viewed = tu.total_msecs_viewed + :msecs,
-                                    notification_level =
-                                       case when tu.notifications_reason_id is null and (tu.total_msecs_viewed + :msecs) >
-                                          coalesce(u.auto_track_topics_after_msecs,:threshold) and
-                                          coalesce(u.auto_track_topics_after_msecs, :threshold) >= 0 then
-                                            :tracking
-                                       else
-                                          tu.notification_level
-                                       end
-                                FROM topic_users tu
-                                join topics t on t.id = tu.topic_id
-                                join users u on u.id = :user_id
-                                WHERE
-                                     tu.topic_id = topic_users.topic_id AND
-                                     tu.user_id = topic_users.user_id AND
-                                     tu.topic_id = :topic_id AND
-                                     tu.user_id = :user_id
-                                RETURNING
-                                  topic_users.notification_level, tu.notification_level old_level
-                              ",
-                              args).values
-
-    if rows.length == 1
-      before = rows[0][1].to_i
-      after = rows[0][0].to_i
-
-      if before != after
-        MessageBus.publish("/topic/#{topic_id}", {notification_level_change: after}, user_ids: [user.id])
       end
     end
 
-    if rows.length == 0
+    # Update the last read and the last seen post count, but only if it doesn't exist.
-      args[:tracking] = TopicUser::NotificationLevel::TRACKING
+    # This would be a lot easier if psql supported some kind of upsert
-      args[:regular] = TopicUser::NotificationLevel::REGULAR
+    def update_last_read(user, topic_id, post_number, msecs)
-      args[:site_setting] = SiteSetting.auto_track_topics_after
+      return if post_number.blank?
-      exec_sql("INSERT INTO topic_users (user_id, topic_id, last_read_post_number, seen_post_count, last_visited_at, first_visited_at, notification_level)
+      msecs = 0 if msecs.to_i < 0
-                SELECT :user_id, :topic_id, :post_number, ft.highest_post_number, :now, :now,
+
-                  case when coalesce(u.auto_track_topics_after_msecs, :site_setting) = 0 then :tracking else :regular end
+      args = {
-                FROM topics AS ft
+        user_id: user.id,
-                JOIN users u on u.id = :user_id
+        topic_id: topic_id,
-                WHERE ft.id = :topic_id
+        post_number: post_number,
-                  AND NOT EXISTS(SELECT 1
+        now: DateTime.now,
-                                 FROM topic_users AS ftu
+        msecs: msecs,
-                                 WHERE ftu.user_id = :user_id and ftu.topic_id = :topic_id)",
+        tracking: notification_levels[:tracking],
-                args)
+        threshold: SiteSetting.auto_track_topics_after
+      }
+
+      rows = exec_sql("UPDATE topic_users
+                                    SET
+                                      last_read_post_number = greatest(:post_number, tu.last_read_post_number),
+                                      seen_post_count = t.highest_post_number,
+                                      total_msecs_viewed = tu.total_msecs_viewed + :msecs,
+                                      notification_level =
+                                         case when tu.notifications_reason_id is null and (tu.total_msecs_viewed + :msecs) >
+                                            coalesce(u.auto_track_topics_after_msecs,:threshold) and
+                                            coalesce(u.auto_track_topics_after_msecs, :threshold) >= 0 then
+                                              :tracking
+                                         else
+                                            tu.notification_level
+                                         end
+                                  FROM topic_users tu
+                                  join topics t on t.id = tu.topic_id
+                                  join users u on u.id = :user_id
+                                  WHERE
+                                       tu.topic_id = topic_users.topic_id AND
+                                       tu.user_id = topic_users.user_id AND
+                                       tu.topic_id = :topic_id AND
+                                       tu.user_id = :user_id
+                                  RETURNING
+                                    topic_users.notification_level, tu.notification_level old_level
+                                ",
+                                args).values
+
+      if rows.length == 1
+        before = rows[0][1].to_i
+        after = rows[0][0].to_i
+
+        if before != after
+          MessageBus.publish("/topic/#{topic_id}", {notification_level_change: after}, user_ids: [user.id])
+        end
+      end
+
+      if rows.length == 0
+        args[:tracking] = notification_levels[:tracking]
+        args[:regular] = notification_levels[:regular]
+        args[:site_setting] = SiteSetting.auto_track_topics_after
+        exec_sql("INSERT INTO topic_users (user_id, topic_id, last_read_post_number, seen_post_count, last_visited_at, first_visited_at, notification_level)
+                  SELECT :user_id, :topic_id, :post_number, ft.highest_post_number, :now, :now,
+                    case when coalesce(u.auto_track_topics_after_msecs, :site_setting) = 0 then :tracking else :regular end
+                  FROM topics AS ft
+                  JOIN users u on u.id = :user_id
+                  WHERE ft.id = :topic_id
+                    AND NOT EXISTS(SELECT 1
+                                   FROM topic_users AS ftu
+                                   WHERE ftu.user_id = :user_id and ftu.topic_id = :topic_id)",
+                  args)
+      end
     end
+
   end
+
 end

app/models/user.rb

@@ -443,11 +443,8 @@ class User < ActiveRecord::Base
   end
 
   def readable_name
-    if name.present? && name != username
+    return "#{name} (#{username})" if name.present? && name != username
-      "#{name} (#{username})"
+    username
-    else
-      username
-    end
   end
 
   protected
@@ -461,25 +458,14 @@ class User < ActiveRecord::Base
     end
 
     def update_tracked_topics
-      if auto_track_topics_after_msecs_changed?
+      return unless auto_track_topics_after_msecs_changed?
 
-        if auto_track_topics_after_msecs < 0
+      where_conditions = {notifications_reason_id: nil, user_id: id}
-
+      if auto_track_topics_after_msecs < 0
-          User.exec_sql('update topic_users set notification_level = ?
+        TopicUser.update_all({notification_level: TopicUser.notification_levels[:regular]}, where_conditions)
-                         where notifications_reason_id is null and
+      else
-                           user_id = ?' , TopicUser::NotificationLevel::REGULAR , id)
+        TopicUser.update_all(["notification_level = CASE WHEN total_msecs_viewed < ? THEN ? ELSE ? END",
-        else
+                              auto_track_topics_after_msecs, TopicUser.notification_levels[:regular], TopicUser.notification_levels[:tracking]], where_conditions)
-
-          User.exec_sql('update topic_users set notification_level = ?
-                         where notifications_reason_id is null and
-                           user_id = ? and
-                           total_msecs_viewed < ?' , TopicUser::NotificationLevel::REGULAR , id, auto_track_topics_after_msecs)
-
-          User.exec_sql('update topic_users set notification_level = ?
-                         where notifications_reason_id is null and
-                           user_id = ? and
-                           total_msecs_viewed >= ?' , TopicUser::NotificationLevel::TRACKING , id, auto_track_topics_after_msecs)
-        end
       end
     end
 

app/serializers/category_topic_serializer.rb

@@ -2,7 +2,6 @@ class CategoryTopicSerializer < BasicTopicSerializer
 
   attributes :slug,
              :visible,
-             :pinned,
              :closed,
              :archived
 

app/serializers/topic_list_item_serializer.rb

@@ -1,3 +1,5 @@
+require_dependency 'pinned_check'
+
 class TopicListItemSerializer < BasicTopicSerializer
 
   attributes :views,
@@ -29,4 +31,8 @@ class TopicListItemSerializer < BasicTopicSerializer
     object.posters || []
   end
 
+  def pinned
+    PinnedCheck.new(object, object.user_data).pinned?
+  end
+
 end

app/serializers/topic_view_serializer.rb

@@ -1,3 +1,5 @@
+require_dependency 'pinned_check'
+
 class TopicViewSerializer < ApplicationSerializer
 
   # These attributes will be delegated to the topic
@@ -12,7 +14,6 @@ class TopicViewSerializer < ApplicationSerializer
      :last_posted_at,
      :visible,
      :closed,
-     :pinned,
      :archived,
      :moderator_posts_count,
      :has_best_of,
@@ -42,7 +43,8 @@ class TopicViewSerializer < ApplicationSerializer
              :notifications_reason_id,
              :posts,
              :at_bottom,
-             :highest_post_number
+             :highest_post_number,
+             :pinned
 
   has_one :created_by, serializer: BasicUserSerializer, embed: :objects
   has_one :last_poster, serializer: BasicUserSerializer, embed: :objects
@@ -193,6 +195,10 @@ class TopicViewSerializer < ApplicationSerializer
     object.highest_post_number
   end
 
+  def pinned
+    PinnedCheck.new(object.topic, object.topic_user).pinned?
+  end
+
   def posts
     return @posts if @posts.present?
     @posts = []

config/locales/client.en.yml

@@ -452,6 +452,10 @@ en:
         title: 'Reply'
         help: 'begin composing a reply to this topic'
 
+      clear_pin:
+        title: "Clear pin"
+        help: "Clear the pinned status of this topic so it no longer appears at the top of your topic list"
+
       share:
         title: 'Share'
         help: 'share a link to this topic'

config/routes.rb

@@ -175,6 +175,7 @@ Discourse::Application.routes.draw do
   put 't/:topic_id/star' => 'topics#star', :constraints => {:topic_id => /\d+/}
   put 't/:slug/:topic_id/status' => 'topics#status', :constraints => {:topic_id => /\d+/}
   put 't/:topic_id/status' => 'topics#status', :constraints => {:topic_id => /\d+/}
+  put 't/:topic_id/clear-pin' => 'topics#clear_pin', :constraints => {:topic_id => /\d+/}
   put 't/:topic_id/mute' => 'topics#mute', :constraints => {:topic_id => /\d+/}
   put 't/:topic_id/unmute' => 'topics#unmute', :constraints => {:topic_id => /\d+/}
 

db/migrate/20130306180148_add_cleared_pinned_to_topic_users.rb

@@ -0,0 +1,9 @@
+class AddClearedPinnedToTopicUsers < ActiveRecord::Migration
+  def change
+    add_column :topic_users, :cleared_pinned_at, :datetime, null: true
+
+    add_column :topics, :pinned_at, :datetime, null: true
+    execute "UPDATE topics SET pinned_at = created_at WHERE pinned"
+    remove_column :topics, :pinned
+  end
+end

lib/pinned_check.rb

@@ -0,0 +1,24 @@
+# Helps us determine whether a topic should be displayed as pinned or not,
+# taking into account anonymous users and users who have dismissed it
+class PinnedCheck
+
+  def initialize(topic, topic_user=nil)
+    @topic, @topic_user = topic, topic_user
+  end
+
+  def pinned?
+
+    # If the topic isn't pinned the answer is false
+    return false if @topic.pinned_at.blank?
+
+    # If the user is anonymous or hasn't entered the topic, the value is always true
+    return true if @topic_user.blank?
+
+    # If the user hasn't cleared the pin, it's true
+    return true if @topic_user.cleared_pinned_at.blank?
+
+    # The final check is to see whether the cleared the pin before or after it was last pinned
+    @topic_user.cleared_pinned_at < @topic.pinned_at
+  end
+
+end

lib/topic_query.rb

@@ -6,6 +6,47 @@ require_dependency 'topic_list'
 
 class TopicQuery
 
+  class << self
+    # use the constants in conjuction with COALESCE to determine the order with regard to pinned
+    # topics that have been cleared by the user. There
+    # might be a cleaner way to do this.
+    def lowest_date
+      "2010-01-01"
+    end
+
+    def highest_date
+      "3000-01-01"
+    end
+
+    # If you've clearned the pin, use bumped_at, otherwise put it at the top
+    def order_with_pinned_sql
+      "CASE
+        WHEN (COALESCE(topics.pinned_at, '#{lowest_date}') > COALESCE(tu.cleared_pinned_at, '#{lowest_date}'))
+          THEN '#{highest_date}'
+        ELSE topics.bumped_at
+       END DESC"
+    end
+
+    # If you've clearned the pin, use bumped_at, otherwise put it at the top
+    def order_nocategory_with_pinned_sql
+      "CASE
+        WHEN topics.category_id IS NULL and (COALESCE(topics.pinned_at, '#{lowest_date}') > COALESCE(tu.cleared_pinned_at, '#{lowest_date}'))
+          THEN '#{highest_date}'
+        ELSE topics.bumped_at
+       END DESC"
+    end
+
+    # For anonymous users
+    def order_nocategory_basic_bumped
+      "CASE WHEN topics.category_id IS NULL and (topics.pinned_at IS NOT NULL) THEN 0 ELSE 1 END, topics.bumped_at DESC"
+    end
+
+    def order_basic_bumped
+      "CASE WHEN (topics.pinned_at IS NOT NULL) THEN 0 ELSE 1 END, topics.bumped_at DESC"
+    end
+
+  end
+
   def initialize(user=nil, opts={})
     @user = user
 
@@ -64,23 +105,19 @@ class TopicQuery
 
   # The popular view of topics
   def list_popular
-    return_list(unordered: true) do |list|
+    TopicList.new(@user, default_list)
-      list.order('CASE WHEN topics.category_id IS NULL and topics.pinned THEN 0 ELSE 1 END, topics.bumped_at DESC')
-    end
   end
 
   # The favorited topics
   def list_favorited
     return_list do |list|
-      list.joins("INNER JOIN topic_users AS tu ON (topics.id = tu.topic_id AND tu.starred AND tu.user_id = #{@user_id})")
+      list.where('tu.starred')
     end
   end
 
   def list_read
     return_list(unordered: true) do |list|
-      list
+      list.order('COALESCE(tu.last_visited_at, topics.bumped_at) DESC')
-        .joins("INNER JOIN topic_users AS tu ON (topics.id = tu.topic_id AND tu.user_id = #{@user_id})")
-        .order('COALESCE(tu.last_visited_at, topics.bumped_at) DESC')
     end
   end
 
@@ -93,17 +130,30 @@ class TopicQuery
   end
 
   def list_posted
-    return_list do |list|
+    return_list {|l| l.where('tu.user_id IS NOT NULL') }
-      list.joins("INNER JOIN topic_users AS tu ON (tu.topic_id = topics.id AND tu.posted AND tu.user_id = #{@user_id})")
-    end
   end
 
   def list_uncategorized
-    return_list {|l| l.where(category_id: nil).order('topics.pinned desc')}
+    return_list(unordered: true) do |list|
+      list = list.where(category_id: nil)
+
+      if @user_id.present?
+        list.order(TopicQuery.order_with_pinned_sql)
+      else
+        list.order(TopicQuery.order_nocategory_basic_bumped)
+      end
+    end
   end
 
   def list_category(category)
-    return_list {|l| l.where(category_id: category.id).order('topics.pinned desc')}
+    return_list(unordered: true) do |list|
+      list = list.where(category_id: category.id)
+      if @user_id.present?
+        list.order(TopicQuery.order_with_pinned_sql)
+      else
+        list.order(TopicQuery.order_basic_bumped)
+      end
+    end
   end
 
   def unread_count
@@ -130,8 +180,22 @@ class TopicQuery
       query_opts = @opts.merge(list_opts)
       page_size = query_opts[:per_page] || SiteSetting.topics_per_page
 
+      # Start with a list of all topics
       result = Topic
-      result = result.topic_list_order unless query_opts[:unordered]
+
+      if @user_id.present?
+        result = result.joins("LEFT OUTER JOIN topic_users AS tu ON (topics.id = tu.topic_id AND tu.user_id = #{@user_id})")
+      end
+
+      unless query_opts[:unordered]
+        # If we're logged in, we have to pay attention to our pinned settings
+        if @user_id.present?
+          result = result.order(TopicQuery.order_nocategory_with_pinned_sql)
+        else
+          result = result.order(TopicQuery.order_basic_bumped)
+        end
+      end
+
       result = result.listable_topics.includes(:category)
       result = result.where('categories.name is null or categories.name <> ?', query_opts[:exclude_category]) if query_opts[:exclude_category]
       result = result.where('categories.name = ?', query_opts[:only_category]) if query_opts[:only_category]
@@ -145,16 +209,15 @@ class TopicQuery
     def new_results(list_opts={})
 
       default_list(list_opts)
-        .joins("LEFT OUTER JOIN topic_users AS tu ON (topics.id = tu.topic_id AND tu.user_id = #{@user_id})")
         .where("topics.created_at >= :created_at", created_at: @user.treat_as_new_topic_start_date)
         .where("tu.last_read_post_number IS NULL")
-        .where("COALESCE(tu.notification_level, :tracking) >= :tracking", tracking: TopicUser::NotificationLevel::TRACKING)
+        .where("COALESCE(tu.notification_level, :tracking) >= :tracking", tracking: TopicUser.notification_levels[:tracking])
     end
 
     def unread_results(list_opts={})
       default_list(list_opts)
-        .joins("INNER JOIN topic_users AS tu ON (topics.id = tu.topic_id AND tu.user_id = #{@user_id} AND tu.last_read_post_number < topics.highest_post_number)")
+        .where("tu.last_read_post_number < topics.highest_post_number")
-        .where("COALESCE(tu.notification_level, :regular) >= :tracking", regular: TopicUser::NotificationLevel::REGULAR, tracking: TopicUser::NotificationLevel::TRACKING)
+        .where("COALESCE(tu.notification_level, :regular) >= :tracking", regular: TopicUser.notification_levels[:regular], tracking: TopicUser.notification_levels[:tracking])
     end
 
     def random_suggested_results_for(topic, count, exclude_topic_ids)

lib/unread.rb

@@ -27,7 +27,7 @@ class Unread
   protected
 
   def do_not_notify?(notification_level)
-    [TopicUser::NotificationLevel::MUTED, TopicUser::NotificationLevel::REGULAR].include?(notification_level)
+    [TopicUser.notification_levels[:muted], TopicUser.notification_levels[:regular]].include?(notification_level)
   end
 
 end

spec/components/pinned_check_spec.rb

@@ -0,0 +1,58 @@
+require 'pinned_check'
+
+describe PinnedCheck do
+
+  #let(:topic) { Fabricate.build(:topic) }
+
+  let(:pinned_at) { 12.hours.ago }
+  let(:unpinned_topic) { Fabricate.build(:topic) }
+  let(:pinned_topic) { Fabricate.build(:topic, pinned_at: pinned_at) }
+
+  context "without a topic_user record (either anonymous or never been in the topic)" do
+
+    it "returns false if the topic is not pinned" do
+      PinnedCheck.new(unpinned_topic).should_not be_pinned
+    end
+
+    it "returns true if the topic is pinned" do
+      PinnedCheck.new(unpinned_topic).should_not be_pinned
+    end
+
+  end
+
+  context "with a topic_user record" do
+    let(:user) { Fabricate.build(:user) }
+    let(:unpinned_topic_user) { Fabricate.build(:topic_user, user: user, topic: unpinned_topic) }
+
+    describe "unpinned topic" do
+      let(:topic_user) { TopicUser.new(topic: unpinned_topic, user: user) }
+
+      it "returns false" do
+        PinnedCheck.new(unpinned_topic, topic_user).should_not be_pinned
+      end
+
+    end
+
+    describe "pinned topic" do
+      let(:topic_user) { TopicUser.new(topic: pinned_topic, user: user) }
+
+      it "is pinned if the topic_user's cleared_pinned_at is blank" do
+        PinnedCheck.new(pinned_topic, topic_user).should be_pinned
+      end
+
+      it "is not pinned if the topic_user's cleared_pinned_at is later than when it was pinned_at" do
+        topic_user.cleared_pinned_at = (pinned_at + 1.hour)
+        PinnedCheck.new(pinned_topic, topic_user).should_not be_pinned
+      end
+
+      it "is pinned if the topic_user's cleared_pinned_at is earlier than when it was pinned_at" do
+        topic_user.cleared_pinned_at = (pinned_at - 3.hours)
+        PinnedCheck.new(pinned_topic, topic_user).should be_pinned
+      end
+    end
+
+
+  end
+
+end
+

spec/components/topic_query_spec.rb

@@ -12,14 +12,13 @@ describe TopicQuery do
 
   context 'a bunch of topics' do
     let!(:regular_topic) { Fabricate(:topic, title: 'this is a regular topic', user: creator, bumped_at: 15.minutes.ago) }
-    let!(:pinned_topic) { Fabricate(:topic, title: 'this is a pinned topic', user: creator, pinned: true, bumped_at: 10.minutes.ago) }
+    let!(:pinned_topic) { Fabricate(:topic, title: 'this is a pinned topic', user: creator, pinned_at: 10.minutes.ago, bumped_at: 10.minutes.ago) }
     let!(:archived_topic) { Fabricate(:topic, title: 'this is an archived topic', user: creator, archived: true, bumped_at: 6.minutes.ago) }
     let!(:invisible_topic) { Fabricate(:topic, title: 'this is an invisible topic', user: creator, visible: false, bumped_at: 5.minutes.ago) }
     let!(:closed_topic) { Fabricate(:topic, title: 'this is a closed topic', user: creator, closed: true, bumped_at: 1.minute.ago) }
+    let(:topics) { topic_query.list_popular.topics }
 
     context 'list_popular' do
-      let(:topics) { topic_query.list_popular.topics }
-
       it "returns the topics in the correct order" do
         topics.should == [pinned_topic, closed_topic, archived_topic, regular_topic]
       end
@@ -33,6 +32,17 @@ describe TopicQuery do
       end
     end
 
+    context 'after clearring a pinned topic' do
+      before do
+        pinned_topic.clear_pin_for(user)
+      end
+
+      it "no longer shows the pinned topic at the top" do
+        topics.should == [closed_topic, archived_topic, pinned_topic, regular_topic]
+      end
+
+    end
+
   end
 
   context 'categorized' do

spec/components/unread_spec.rb

@@ -7,8 +7,8 @@ describe Unread do
   before do
     @topic = Fabricate(:topic, posts_count: 13, highest_post_number: 13)
     @topic_user = TopicUser.get(@topic, @topic.user)
-    @topic_user.stubs(:notification_level).returns(TopicUser::NotificationLevel::TRACKING)
+    @topic_user.stubs(:notification_level).returns(TopicUser.notification_levels[:tracking])
-    @topic_user.notification_level = TopicUser::NotificationLevel::TRACKING
+    @topic_user.notification_level = TopicUser.notification_levels[:tracking]
     @unread = Unread.new(@topic, @topic_user)
   end
 
@@ -51,7 +51,7 @@ describe Unread do
 
     it 'has 0 new posts if the user has read 10 posts but is not tracking' do
       @topic_user.stubs(:seen_post_count).returns(10)
-      @topic_user.stubs(:notification_level).returns(TopicUser::NotificationLevel::REGULAR)
+      @topic_user.stubs(:notification_level).returns(TopicUser.notification_levels[:regular])
       @unread.new_posts.should == 0
     end
 

spec/controllers/topics_controller_spec.rb

@@ -74,6 +74,37 @@ describe TopicsController do
 
   end
 
+  context 'clear_pin' do
+    it 'needs you to be logged in' do
+      lambda { xhr :put, :clear_pin, topic_id: 1 }.should raise_error(Discourse::NotLoggedIn)
+    end
+
+    context 'when logged in' do
+      let(:topic) { Fabricate(:topic) }
+      let!(:user) { log_in }
+
+      it "fails when the user can't see the topic" do
+        Guardian.any_instance.expects(:can_see?).with(topic).returns(false)
+        xhr :put, :clear_pin, topic_id: topic.id
+        response.should_not be_success
+      end
+
+      describe 'when the user can see the topic' do
+        it "calls clear_pin_for if the user can see the topic" do
+          Topic.any_instance.expects(:clear_pin_for).with(user).once
+          xhr :put, :clear_pin, topic_id: topic.id
+        end
+
+        it "succeeds" do
+          xhr :put, :clear_pin, topic_id: topic.id
+          response.should be_success
+        end
+      end
+
+    end
+
+  end
+
   context 'status' do
     it 'needs you to be logged in' do
       lambda { xhr :put, :status, topic_id: 1, status: 'visible', enabled: true }.should raise_error(Discourse::NotLoggedIn)

spec/models/topic_spec.rb

@@ -547,8 +547,12 @@ describe Topic do
           @topic.reload
         end
 
+        it "doesn't have a pinned_at" do
+          @topic.pinned_at.should be_blank
+        end
+
         it 'should not be pinned' do
-          @topic.should_not be_pinned
+          @topic.pinned_at.should be_blank
         end
 
         it 'adds a moderator post' do
@@ -562,13 +566,13 @@ describe Topic do
 
       context 'enable' do
         before do
-          @topic.update_attribute :pinned, false
+          @topic.update_attribute :pinned_at, nil
           @topic.update_status('pinned', true, @user)
           @topic.reload
         end
 
         it 'should be pinned' do
-          @topic.should be_pinned
+          @topic.pinned_at.should be_present
         end
 
         it 'adds a moderator post' do
@@ -588,7 +592,7 @@ describe Topic do
           @topic.reload
         end
 
-        it 'should not be pinned' do
+        it 'should not be archived' do
           @topic.should_not be_archived
         end
 
@@ -866,8 +870,12 @@ describe Topic do
       topic.should be_visible
     end
 
+    it "has an empty pinned_at" do
+      topic.pinned_at.should be_blank
+    end
+
     it 'is not pinned' do
-      topic.should_not be_pinned
+      topic.pinned_at.should be_blank
     end
 
     it 'is not closed' do

spec/models/topic_user_spec.rb

@@ -5,155 +5,165 @@ describe TopicUser do
   it { should belong_to :user }
   it { should belong_to :topic }
 
+  let!(:yesterday) { DateTime.now.yesterday }
+
   before do
-    #mock time so we can test dates
+    DateTime.expects(:now).at_least_once.returns(yesterday)
-    @now = DateTime.now.yesterday
+  end
-    DateTime.expects(:now).at_least_once.returns(@now)
+
-    @topic = Fabricate(:topic)
+  let!(:topic) { Fabricate(:topic) }
-    @user = Fabricate(:coding_horror)
+  let!(:user) { Fabricate(:coding_horror) }
+  let(:topic_user) { TopicUser.get(topic,user) }
+  let(:topic_creator_user) { TopicUser.get(topic, topic.user) }
+
+  let(:post) { Fabricate(:post, topic: topic, user: user) }
+  let(:new_user) { Fabricate(:user, auto_track_topics_after_msecs: 1000) }
+  let(:topic_new_user) { TopicUser.get(topic, new_user)}
+
+
+  describe "unpinned" do
+
+    before do
+      TopicUser.change(user, topic, {:starred_at => yesterday})
+    end
+
+    it "defaults to blank" do
+      topic_user.cleared_pinned_at.should be_blank
+    end
+
   end
 
   describe 'notifications' do
 
     it 'should be set to tracking if auto_track_topics is enabled' do
-      @user.auto_track_topics_after_msecs = 0
+      user.update_column(:auto_track_topics_after_msecs, 0)
-      @user.save
+      TopicUser.change(user, topic, {:starred_at => yesterday})
-      TopicUser.change(@user, @topic, {:starred_at => DateTime.now})
+      TopicUser.get(topic, user).notification_level.should == TopicUser.notification_levels[:tracking]
-      TopicUser.get(@topic,@user).notification_level.should == TopicUser::NotificationLevel::TRACKING
     end
 
     it 'should reset regular topics to tracking topics if auto track is changed' do
-      TopicUser.change(@user, @topic, {:starred_at => DateTime.now})
+      TopicUser.change(user, topic, {:starred_at => yesterday})
-      @user.auto_track_topics_after_msecs = 0
+      user.auto_track_topics_after_msecs = 0
-      @user.save
+      user.save
-      TopicUser.get(@topic,@user).notification_level.should == TopicUser::NotificationLevel::TRACKING
+      topic_user.notification_level.should == TopicUser.notification_levels[:tracking]
     end
 
     it 'should be set to "regular" notifications, by default on non creators' do
-      TopicUser.change(@user, @topic, {:starred_at => DateTime.now})
+      TopicUser.change(user, topic, {:starred_at => yesterday})
-      TopicUser.get(@topic,@user).notification_level.should == TopicUser::NotificationLevel::REGULAR
+      TopicUser.get(topic,user).notification_level.should == TopicUser.notification_levels[:regular]
     end
 
     it 'reason should reset when changed' do
-      @topic.notify_muted!(@topic.user)
+      topic.notify_muted!(topic.user)
-      TopicUser.get(@topic,@topic.user).notifications_reason_id.should == TopicUser::NotificationReasons::USER_CHANGED
+      TopicUser.get(topic,topic.user).notifications_reason_id.should == TopicUser.notification_reasons[:user_changed]
     end
 
     it 'should have the correct reason for a user change when watched' do
-      @topic.notify_watch!(@user)
+      topic.notify_watch!(user)
-      tu = TopicUser.get(@topic,@user)
+      topic_user.notification_level.should == TopicUser.notification_levels[:watching]
-      tu.notification_level.should == TopicUser::NotificationLevel::WATCHING
+      topic_user.notifications_reason_id.should == TopicUser.notification_reasons[:user_changed]
-      tu.notifications_reason_id.should == TopicUser::NotificationReasons::USER_CHANGED
+      topic_user.notifications_changed_at.should_not be_nil
-      tu.notifications_changed_at.should_not be_nil
     end
 
     it 'should have the correct reason for a user change when set to regular' do
-      @topic.notify_regular!(@user)
+      topic.notify_regular!(user)
-      tu = TopicUser.get(@topic,@user)
+      topic_user.notification_level.should == TopicUser.notification_levels[:regular]
-      tu.notification_level.should == TopicUser::NotificationLevel::REGULAR
+      topic_user.notifications_reason_id.should == TopicUser.notification_reasons[:user_changed]
-      tu.notifications_reason_id.should == TopicUser::NotificationReasons::USER_CHANGED
+      topic_user.notifications_changed_at.should_not be_nil
-      tu.notifications_changed_at.should_not be_nil
     end
 
     it 'should have the correct reason for a user change when set to regular' do
-      @topic.notify_muted!(@user)
+      topic.notify_muted!(user)
-      tu = TopicUser.get(@topic,@user)
+      topic_user.notification_level.should == TopicUser.notification_levels[:muted]
-      tu.notification_level.should == TopicUser::NotificationLevel::MUTED
+      topic_user.notifications_reason_id.should == TopicUser.notification_reasons[:user_changed]
-      tu.notifications_reason_id.should == TopicUser::NotificationReasons::USER_CHANGED
+      topic_user.notifications_changed_at.should_not be_nil
-      tu.notifications_changed_at.should_not be_nil
     end
 
     it 'should watch topics a user created' do
-      tu = TopicUser.get(@topic,@topic.user)
+      topic_creator_user.notification_level.should == TopicUser.notification_levels[:watching]
-      tu.notification_level.should == TopicUser::NotificationLevel::WATCHING
+      topic_creator_user.notifications_reason_id.should == TopicUser.notification_reasons[:created_topic]
-      tu.notifications_reason_id.should == TopicUser::NotificationReasons::CREATED_TOPIC
     end
   end
 
   describe 'visited at' do
-    before do
-      TopicUser.track_visit!(@topic, @user)
-      @topic_user = TopicUser.get(@topic,@user)
 
+    before do
+      TopicUser.track_visit!(topic, user)
     end
 
     it 'set upon initial visit' do
-      @topic_user.first_visited_at.to_i.should == @now.to_i
+      topic_user.first_visited_at.to_i.should == yesterday.to_i
-      @topic_user.last_visited_at.to_i.should == @now.to_i
+      topic_user.last_visited_at.to_i.should == yesterday.to_i
     end
 
     it 'updates upon repeat visit' do
-      tomorrow = @now.tomorrow
+      today = yesterday.tomorrow
-      DateTime.expects(:now).returns(tomorrow)
+      DateTime.expects(:now).returns(today)
 
-      TopicUser.track_visit!(@topic,@user)
+      TopicUser.track_visit!(topic,user)
       # reload is a no go
-      @topic_user = TopicUser.get(@topic,@user)
+      topic_user = TopicUser.get(topic,user)
-      @topic_user.first_visited_at.to_i.should == @now.to_i
+      topic_user.first_visited_at.to_i.should == yesterday.to_i
-      @topic_user.last_visited_at.to_i.should == tomorrow.to_i
+      topic_user.last_visited_at.to_i.should == today.to_i
     end
 
   end
 
   describe 'read tracking' do
-    before do
-      @post = Fabricate(:post, topic: @topic, user: @topic.user)
-      TopicUser.update_last_read(@user, @topic.id, 1, 0)
-      @topic_user = TopicUser.get(@topic,@user)
-    end
 
-    it 'should create a new record for a visit' do
+    context "without auto tracking" do
-      @topic_user.last_read_post_number.should == 1
-      @topic_user.last_visited_at.to_i.should == @now.to_i
-      @topic_user.first_visited_at.to_i.should == @now.to_i
-    end
 
-    it 'should update the record for repeat visit' do
+      before do
-      Fabricate(:post, topic: @topic, user: @user)
+        TopicUser.update_last_read(user, topic.id, 1, 0)
-      TopicUser.update_last_read(@user, @topic.id, 2, 0)
+      end
-      @topic_user = TopicUser.get(@topic,@user)
+
-      @topic_user.last_read_post_number.should == 2
+      let(:topic_user) { TopicUser.get(topic,user) }
-      @topic_user.last_visited_at.to_i.should == @now.to_i
+
-      @topic_user.first_visited_at.to_i.should == @now.to_i
+      it 'should create a new record for a visit' do
+        topic_user.last_read_post_number.should == 1
+        topic_user.last_visited_at.to_i.should == yesterday.to_i
+        topic_user.first_visited_at.to_i.should == yesterday.to_i
+      end
+
+      it 'should update the record for repeat visit' do
+        Fabricate(:post, topic: topic, user: user)
+        TopicUser.update_last_read(user, topic.id, 2, 0)
+        topic_user = TopicUser.get(topic,user)
+        topic_user.last_read_post_number.should == 2
+        topic_user.last_visited_at.to_i.should == yesterday.to_i
+        topic_user.first_visited_at.to_i.should == yesterday.to_i
+      end
     end
 
     context 'auto tracking' do
+
       before do
-        Fabricate(:post, topic: @topic, user: @user)
+        TopicUser.update_last_read(new_user, topic.id, 2, 0)
-        @new_user = Fabricate(:user, auto_track_topics_after_msecs: 1000)
-        TopicUser.update_last_read(@new_user, @topic.id, 2, 0)
-        @topic_user = TopicUser.get(@topic,@new_user)
       end
 
       it 'should automatically track topics you reply to' do
-        post = Fabricate(:post, topic: @topic, user: @new_user)
+        post = Fabricate(:post, topic: topic, user: new_user)
-        @topic_user = TopicUser.get(@topic,@new_user)
+        topic_new_user.notification_level.should == TopicUser.notification_levels[:tracking]
-        @topic_user.notification_level.should == TopicUser::NotificationLevel::TRACKING
+        topic_new_user.notifications_reason_id.should == TopicUser.notification_reasons[:created_post]
-        @topic_user.notifications_reason_id.should == TopicUser::NotificationReasons::CREATED_POST
       end
 
       it 'should not automatically track topics you reply to and have set state manually' do
-        Fabricate(:post, topic: @topic, user: @new_user)
+        Fabricate(:post, topic: topic, user: new_user)
-        TopicUser.change(@new_user, @topic, notification_level: TopicUser::NotificationLevel::REGULAR)
+        TopicUser.change(new_user, topic, notification_level: TopicUser.notification_levels[:regular])
-        @topic_user = TopicUser.get(@topic,@new_user)
+        topic_new_user.notification_level.should == TopicUser.notification_levels[:regular]
-        @topic_user.notification_level.should == TopicUser::NotificationLevel::REGULAR
+        topic_new_user.notifications_reason_id.should == TopicUser.notification_reasons[:user_changed]
-        @topic_user.notifications_reason_id.should == TopicUser::NotificationReasons::USER_CHANGED
       end
 
       it 'should automatically track topics after they are read for long enough' do
-        @topic_user.notification_level.should == TopicUser::NotificationLevel::REGULAR
+        topic_new_user.notification_level.should ==TopicUser.notification_levels[:regular]
-        TopicUser.update_last_read(@new_user, @topic.id, 2, 1001)
+        TopicUser.update_last_read(new_user, topic.id, 2, 1001)
-        @topic_user = TopicUser.get(@topic,@new_user)
+        TopicUser.get(topic, new_user).notification_level.should == TopicUser.notification_levels[:tracking]
-        @topic_user.notification_level.should == TopicUser::NotificationLevel::TRACKING
       end
 
       it 'should not automatically track topics after they are read for long enough if changed manually' do
-        TopicUser.change(@new_user, @topic, notification_level: TopicUser::NotificationLevel::REGULAR)
+        TopicUser.change(new_user, topic, notification_level: TopicUser.notification_levels[:regular])
-        @topic_user = TopicUser.get(@topic,@new_user)
+        TopicUser.update_last_read(new_user, topic, 2, 1001)
-
+        topic_new_user.notification_level.should == TopicUser.notification_levels[:regular]
-        TopicUser.update_last_read(@new_user, @topic, 2, 1001)
-        @topic_user = TopicUser.get(@topic,@new_user)
-        @topic_user.notification_level.should == TopicUser::NotificationLevel::REGULAR
       end
     end
   end
@@ -162,34 +172,33 @@ describe TopicUser do
 
     it 'creates a forum topic user record' do
       lambda {
-        TopicUser.change(@user, @topic.id, starred: true)
+        TopicUser.change(user, topic.id, starred: true)
       }.should change(TopicUser, :count).by(1)
     end
 
     it "only inserts a row once, even on repeated calls" do
       lambda {
-        TopicUser.change(@user, @topic.id, starred: true)
+        TopicUser.change(user, topic.id, starred: true)
-        TopicUser.change(@user, @topic.id, starred: false)
+        TopicUser.change(user, topic.id, starred: false)
-        TopicUser.change(@user, @topic.id, starred: true)
+        TopicUser.change(user, topic.id, starred: true)
       }.should change(TopicUser, :count).by(1)
     end
 
     describe 'after creating a row' do
       before do
-        TopicUser.change(@user, @topic.id, starred: true)
+        TopicUser.change(user, topic.id, starred: true)
-        @topic_user = TopicUser.where(user_id: @user.id, topic_id: @topic.id).first
       end
 
       it 'has the correct starred value' do
-        @topic_user.should be_starred
+        TopicUser.get(topic, user).should be_starred
       end
 
       it 'has a lookup' do
-        TopicUser.lookup_for(@user, [@topic]).should be_present
+        TopicUser.lookup_for(user, [topic]).should be_present
       end
 
       it 'has a key in the lookup for this forum topic' do
-        TopicUser.lookup_for(@user, [@topic]).has_key?(@topic.id).should be_true
+        TopicUser.lookup_for(user, [topic]).has_key?(topic.id).should be_true
       end
 
     end