clean up 'checked_for_custom_avatars' user history entries

app/models/user_history.rb

@@ -20,7 +20,7 @@ def self.actions
                           :change_site_setting,
                           :change_site_customization,
                           :delete_site_customization,
-                          :checked_for_custom_avatar,
+                          :checked_for_custom_avatar, # not used anymore
                           :notified_about_avatar,
                           :notified_about_sequential_replies,
                           :notified_about_dominating_topic,

db/migrate/20150102113309_clean_up_user_history.rb

@@ -0,0 +1,10 @@
+class CleanUpUserHistory < ActiveRecord::Migration
+  def up
+    # 'checked_for_custom_avatar' is not used anymore
+    # was removed in https://github.com/discourse/discourse/commit/6c1c8be79433f87bef9d768da7b8fa4ec9bb18d7
+    UserHistory.where(action: UserHistory.actions[:checked_for_custom_avatar]).delete_all
+  end
+
+  def down
+  end
+end

lib/composer_messages_finder.rb

@@ -26,9 +26,11 @@ def check_education_message
 
     if count < SiteSetting.educate_until_posts
       education_posts_text = I18n.t('education.until_posts', count: SiteSetting.educate_until_posts)
-      return {templateName: 'composer/education',
-              wait_for_typing: true,
-              body: PrettyText.cook(SiteText.text_for(education_key, education_posts_text: education_posts_text)) }
+      return {
+        templateName: 'composer/education',
+        wait_for_typing: true,
+        body: PrettyText.cook(SiteText.text_for(education_key, education_posts_text: education_posts_text))
+      }
     end
 
     nil
@@ -37,7 +39,11 @@ def check_education_message
   # New users have a limited number of replies in a topic
   def check_new_user_many_replies
     return unless replying? && @user.posted_too_much_in_topic?(@details[:topic_id])
-    {templateName: 'composer/education', body: PrettyText.cook(I18n.t('education.too_many_replies', newuser_max_replies_per_topic: SiteSetting.newuser_max_replies_per_topic)) }
+
+    {
+      templateName: 'composer/education',
+      body: PrettyText.cook(I18n.t('education.too_many_replies', newuser_max_replies_per_topic: SiteSetting.newuser_max_replies_per_topic))
+    }
   end
 
   # Should a user be contacted to update their avatar?
@@ -49,14 +55,14 @@ def check_avatar_notification
     # We don't notify users who have avatars or who have been notified already.
     return if @user.uploaded_avatar_id || UserHistory.exists_for_user?(@user, :notified_about_avatar)
 
-    # Finally, we don't check users whose avatars haven't been examined
-    return unless UserHistory.exists_for_user?(@user, :checked_for_custom_avatar)
-
     # If we got this far, log that we've nagged them about the avatar
     UserHistory.create!(action: UserHistory.actions[:notified_about_avatar], target_user_id: @user.id )
 
     # Return the message
-    {templateName: 'composer/education', body: PrettyText.cook(I18n.t('education.avatar', profile_path: "/users/#{@user.username_lower}")) }
+    {
+      templateName: 'composer/education',
+      body: PrettyText.cook(I18n.t('education.avatar', profile_path: "/users/#{@user.username_lower}"))
+    }
   end
 
   # Is a user replying too much in succession?
@@ -87,10 +93,12 @@ def check_sequential_replies
                         target_user_id: @user.id,
                         topic_id: @details[:topic_id] )
 
-    {templateName: 'composer/education',
-     wait_for_typing: true,
-     extraClass: 'urgent',
-     body: PrettyText.cook(I18n.t('education.sequential_replies')) }
+    {
+      templateName: 'composer/education',
+      wait_for_typing: true,
+      extraClass: 'urgent',
+      body: PrettyText.cook(I18n.t('education.sequential_replies'))
+    }
   end
 
   def check_dominating_topic
@@ -102,6 +110,7 @@ def check_dominating_topic
                   !UserHistory.exists_for_user?(@user, :notified_about_dominating_topic, topic_id: @details[:topic_id])
 
     topic = Topic.find_by(id: @details[:topic_id])
+
     return if topic.blank? ||
               topic.user_id == @user.id ||
               topic.posts_count < SiteSetting.summary_posts_required ||
@@ -117,11 +126,12 @@ def check_dominating_topic
                         target_user_id: @user.id,
                         topic_id: @details[:topic_id])
 
-
-    {templateName: 'composer/education',
-     wait_for_typing: true,
-     extraClass: 'urgent',
-     body: PrettyText.cook(I18n.t('education.dominating_topic', percent: (ratio * 100).round)) }
+    {
+      templateName: 'composer/education',
+      wait_for_typing: true,
+      extraClass: 'urgent',
+      body: PrettyText.cook(I18n.t('education.dominating_topic', percent: (ratio * 100).round))
+    }
   end
 
   def check_reviving_old_topic
@@ -136,20 +146,22 @@ def check_reviving_old_topic
               topic.last_posted_at.nil? ||
               topic.last_posted_at > SiteSetting.warn_reviving_old_topic_age.days.ago
 
-    {templateName: 'composer/education',
-     wait_for_typing: false,
-     extraClass: 'urgent',
-     body: PrettyText.cook(I18n.t('education.reviving_old_topic', days: (Time.zone.now - topic.last_posted_at).round / 1.day)) }
+    {
+      templateName: 'composer/education',
+      wait_for_typing: false,
+      extraClass: 'urgent',
+      body: PrettyText.cook(I18n.t('education.reviving_old_topic', days: (Time.zone.now - topic.last_posted_at).round / 1.day))
+    }
   end
 
   private
 
     def creating_topic?
-      return @details[:composerAction] == "createTopic"
+      @details[:composerAction] == "createTopic"
     end
 
     def replying?
-      return @details[:composerAction] == "reply"
+      @details[:composerAction] == "reply"
     end
 
 end

spec/components/composer_messages_finder_spec.rb

@@ -83,44 +83,31 @@
     let(:finder) { ComposerMessagesFinder.new(user, composerAction: 'createTopic') }
     let(:user) { Fabricate(:user) }
 
-    context "a user who we haven't checked for an avatar yet" do
-      it "returns no avatar message" do
-        finder.check_avatar_notification.should be_blank
-      end
-    end
-
-    context "a user who has been checked for a custom avatar" do
-      before do
-        UserHistory.create!(action: UserHistory.actions[:checked_for_custom_avatar], target_user_id: user.id )
-      end
-
-      context "success" do
-        let!(:message) { finder.check_avatar_notification }
-
-        it "returns an avatar upgrade message" do
-          message.should be_present
-        end
+    context "success" do
+      let!(:message) { finder.check_avatar_notification }
 
-        it "creates a notified_about_avatar log" do
-          UserHistory.exists_for_user?(user, :notified_about_avatar).should == true
-        end
+      it "returns an avatar upgrade message" do
+        message.should be_present
       end
 
-      it "doesn't return notifications for new users" do
-        user.trust_level = TrustLevel[0]
-        finder.check_avatar_notification.should be_blank
+      it "creates a notified_about_avatar log" do
+        UserHistory.exists_for_user?(user, :notified_about_avatar).should == true
       end
+    end
 
-      it "doesn't return notifications for users who have custom avatars" do
-        user.uploaded_avatar_id = 1
-        finder.check_avatar_notification.should be_blank
-      end
+    it "doesn't return notifications for new users" do
+      user.trust_level = TrustLevel[0]
+      finder.check_avatar_notification.should be_blank
+    end
 
-      it "doesn't notify users who have been notified already" do
-        UserHistory.create!(action: UserHistory.actions[:notified_about_avatar], target_user_id: user.id )
-        finder.check_avatar_notification.should be_blank
-      end
+    it "doesn't return notifications for users who have custom avatars" do
+      user.uploaded_avatar_id = 1
+      finder.check_avatar_notification.should be_blank
+    end
 
+    it "doesn't notify users who have been notified already" do
+      UserHistory.create!(action: UserHistory.actions[:notified_about_avatar], target_user_id: user.id )
+      finder.check_avatar_notification.should be_blank
     end
   end