From c57a1b393f0fefb9e3094514fa03c9c5046cc6f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9gis=20Hanol?= Date: Fri, 2 Jan 2015 12:37:17 +0100 Subject: [PATCH] clean up 'checked_for_custom_avatars' user history entries --- app/models/user_history.rb | 2 +- .../20150102113309_clean_up_user_history.rb | 10 ++++ lib/composer_messages_finder.rb | 58 +++++++++++-------- .../composer_messages_finder_spec.rb | 53 +++++++---------- 4 files changed, 66 insertions(+), 57 deletions(-) create mode 100644 db/migrate/20150102113309_clean_up_user_history.rb diff --git a/app/models/user_history.rb b/app/models/user_history.rb index 90aae0c0bde..b4bfcad6a82 100644 --- a/app/models/user_history.rb +++ b/app/models/user_history.rb @@ -20,7 +20,7 @@ class UserHistory < ActiveRecord::Base :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, diff --git a/db/migrate/20150102113309_clean_up_user_history.rb b/db/migrate/20150102113309_clean_up_user_history.rb new file mode 100644 index 00000000000..8ddbe9be05c --- /dev/null +++ b/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 diff --git a/lib/composer_messages_finder.rb b/lib/composer_messages_finder.rb index 8d2865ac257..e61d0729d84 100644 --- a/lib/composer_messages_finder.rb +++ b/lib/composer_messages_finder.rb @@ -26,9 +26,11 @@ class ComposerMessagesFinder 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 @@ class ComposerMessagesFinder # 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 @@ class ComposerMessagesFinder # 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 @@ class ComposerMessagesFinder 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 @@ class ComposerMessagesFinder !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 @@ class ComposerMessagesFinder 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 @@ class ComposerMessagesFinder 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 diff --git a/spec/components/composer_messages_finder_spec.rb b/spec/components/composer_messages_finder_spec.rb index f119bf4dafd..fdca2b68303 100644 --- a/spec/components/composer_messages_finder_spec.rb +++ b/spec/components/composer_messages_finder_spec.rb @@ -83,44 +83,31 @@ describe ComposerMessagesFinder do 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 + context "success" do + let!(:message) { finder.check_avatar_notification } + + it "returns an avatar upgrade message" do + message.should be_present + end + + it "creates a notified_about_avatar log" do + UserHistory.exists_for_user?(user, :notified_about_avatar).should == true 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 + it "doesn't return notifications for new users" do + user.trust_level = TrustLevel[0] + finder.check_avatar_notification.should be_blank + end - context "success" do - let!(:message) { finder.check_avatar_notification } - - it "returns an avatar upgrade message" do - message.should be_present - end - - 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 new users" do - user.trust_level = TrustLevel[0] - 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 + 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