From 7d9a84b496a4e8afac667b2a9c7f04e5e990cba6 Mon Sep 17 00:00:00 2001 From: Robin Ward Date: Thu, 12 Sep 2013 17:46:43 -0400 Subject: [PATCH] New User Education goes through a server side ComposerMessages check. Composer message for users who don't have avatars. --- .../controllers/composer_controller.js | 44 +++---- .../composer_messages_controller.js | 70 +++++++++++- .../discourse/models/composer_message.js | 35 ++++++ .../composer_messages_controller.rb | 13 +++ app/controllers/education_controller.rb | 25 ---- app/controllers/posts_controller.rb | 5 - app/helpers/composer_messages_helper.rb | 2 + app/models/user.rb | 4 + app/models/user_history.rb | 4 + config/locales/server.en.yml | 9 ++ config/routes.rb | 2 +- lib/composer_messages_finder.rb | 62 ++++++++++ .../composer_messages_finder_spec.rb | 108 ++++++++++++++++++ .../composer_messages_controller_spec.rb | 32 ++++++ spec/controllers/education_controller_spec.rb | 46 -------- spec/models/topic_spec.rb | 4 + 16 files changed, 356 insertions(+), 109 deletions(-) create mode 100644 app/assets/javascripts/discourse/models/composer_message.js create mode 100644 app/controllers/composer_messages_controller.rb delete mode 100644 app/controllers/education_controller.rb create mode 100644 app/helpers/composer_messages_helper.rb create mode 100644 lib/composer_messages_finder.rb create mode 100644 spec/components/composer_messages_finder_spec.rb create mode 100644 spec/controllers/composer_messages_controller_spec.rb delete mode 100644 spec/controllers/education_controller_spec.rb diff --git a/app/assets/javascripts/discourse/controllers/composer_controller.js b/app/assets/javascripts/discourse/controllers/composer_controller.js index 7732468f5..156cf85e2 100644 --- a/app/assets/javascripts/discourse/controllers/composer_controller.js +++ b/app/assets/javascripts/discourse/controllers/composer_controller.js @@ -10,7 +10,7 @@ Discourse.ComposerController = Discourse.Controller.extend({ needs: ['modal', 'topic', 'composerMessages'], replyAsNewTopicDraft: Em.computed.equal('model.draftKey', Discourse.Composer.REPLY_AS_NEW_TOPIC_KEY), - + checkedMessages: false, init: function() { this._super(); @@ -117,33 +117,18 @@ Discourse.ComposerController = Discourse.Controller.extend({ }); }, - _considerNewUserEducation: function() { - - // We don't show education when editing a post. - if (this.get('model.editingPost')) return; - - // If creating a topic, use topic_count, otherwise post_count - var count = this.get('model.creatingTopic') ? Discourse.User.currentProp('topic_count') : Discourse.User.currentProp('reply_count'); - if (count >= Discourse.SiteSettings.educate_until_posts) { return; } - - // The user must have typed a reply - if (!this.get('typedReply')) return; - - // If visible update the text - var educationKey = this.get('model.creatingTopic') ? 'new-topic' : 'new-reply', - messageController = this.get('controllers.composerMessages'); - - Discourse.ajax("/education/" + educationKey, {dataType: 'html'}).then(function(result) { - messageController.popup({ - templateName: 'composer/education', - body: result - }); - }); - - }.observes('typedReply', 'model.creatingTopic', 'currentUser.reply_count'), + /** + Checks to see if a reply has been typed. This is signaled by a keyUp + event in a view. + @method checkReplyLength + **/ checkReplyLength: function() { - this.set('typedReply', this.present('model.reply')); + if (this.present('model.reply')) { + // Notify the composer messages controller that a reply has been typed. Some + // messages only appear after typing. + this.get('controllers.composerMessages').typedReply() + } }, /** @@ -171,11 +156,11 @@ Discourse.ComposerController = Discourse.Controller.extend({ similarTopics.clear(); similarTopics.pushObjects(newTopics); - messageController.popup({ + messageController.popup(Discourse.ComposerMessage.create({ templateName: 'composer/similar_topics', similarTopics: similarTopics, extraClass: 'similar-topics' - }); + })); }); }, @@ -203,7 +188,6 @@ Discourse.ComposerController = Discourse.Controller.extend({ var promise = opts.promise || Ember.Deferred.create(); opts.promise = promise; - this.set('typedReply', false); if (!opts.draftKey) { alert("composer was opened without a draft key"); @@ -272,8 +256,10 @@ Discourse.ComposerController = Discourse.Controller.extend({ composer = composer || Discourse.Composer.create(); composer.open(opts); + this.set('model', composer); composer.set('composeState', Discourse.Composer.OPEN); + composerMessages.queryFor(this.get('model')); promise.resolve(); return promise; }, diff --git a/app/assets/javascripts/discourse/controllers/composer_messages_controller.js b/app/assets/javascripts/discourse/controllers/composer_messages_controller.js index ccfe883c6..8dfe43e42 100644 --- a/app/assets/javascripts/discourse/controllers/composer_messages_controller.js +++ b/app/assets/javascripts/discourse/controllers/composer_messages_controller.js @@ -9,28 +9,92 @@ Discourse.ComposerMessagesController = Ember.ArrayController.extend({ needs: ['composer'], + // Whether we've checked our messages + checkedMessages: false, + + /** + Initialize the controller + **/ init: function() { this._super(); - this.set('messagesByTemplate', {}); + this.reset(); }, + /** + Displays a new message + + @method popup + @params {Object} msg The message to display + **/ popup: function(msg) { var messagesByTemplate = this.get('messagesByTemplate'), - existing = messagesByTemplate[msg.templateName]; + templateName = msg.get('templateName'), + existing = messagesByTemplate[templateName]; if (!existing) { this.pushObject(msg); - messagesByTemplate[msg.templateName] = msg; + messagesByTemplate[templateName] = msg; } }, + /** + Closes and hides a message. + + @method closeMessage + @params {Object} message The message to dismiss + **/ closeMessage: function(message) { this.removeObject(message); }, + /** + Resets all active messages. For example if composing a new post. + + @method reset + **/ reset: function() { this.clear(); this.set('messagesByTemplate', {}); + this.set('queuedForTyping', new Em.Set()); + this.set('checkedMessages', false); + }, + + /** + Called after the user has typed a reply. Some messages only get shown after being + typed. + + @method typedReply + **/ + typedReply: function() { + var self = this; + this.get('queuedForTyping').forEach(function (msg) { + self.popup(msg); + }) + }, + + /** + Figure out if there are any messages that should be displayed above the composer. + + @method queryFor + @params {Discourse.Composer} composer The composer model + **/ + queryFor: function(composer) { + if (this.get('checkedMessages')) { return; } + + var self = this, + queuedForTyping = self.get('queuedForTyping'); + + Discourse.ComposerMessage.find(composer).then(function (messages) { + self.set('checkedMessages', true); + messages.forEach(function (msg) { + console.log(msg); + if (msg.wait_for_typing) { + queuedForTyping.addObject(msg); + } else { + self.popup(msg); + } + }); + }); } }); \ No newline at end of file diff --git a/app/assets/javascripts/discourse/models/composer_message.js b/app/assets/javascripts/discourse/models/composer_message.js new file mode 100644 index 000000000..4381df9b4 --- /dev/null +++ b/app/assets/javascripts/discourse/models/composer_message.js @@ -0,0 +1,35 @@ +/** + Represents a pop up message displayed over the composer + + @class ComposerMessage + @extends Ember.Object + @namespace Discourse + @module Discourse +**/ +Discourse.ComposerMessage = Em.Object.extend({}); + +Discourse.ComposerMessage.reopenClass({ + /** + Look for composer messages given the current composing settings. + + @method find + @param {Discourse.Composer} composer The current composer + @returns {Discourse.ComposerMessage} the composer message to display (or null) + **/ + find: function(composer) { + + var data = { composerAction: composer.get('action') }, + topicId = composer.get('topic.id'), + postId = composer.get('post.id'); + + if (topicId) { data.topic_id = topicId }; + if (postId) { data.post_id = postId }; + + return Discourse.ajax('/composer-messages', { data: data }).then(function (messages) { + return messages.map(function (message) { + return Discourse.ComposerMessage.create(message); + }); + }); + } + +}) \ No newline at end of file diff --git a/app/controllers/composer_messages_controller.rb b/app/controllers/composer_messages_controller.rb new file mode 100644 index 000000000..161988ac8 --- /dev/null +++ b/app/controllers/composer_messages_controller.rb @@ -0,0 +1,13 @@ +require_dependency 'composer_messages_finder' + +class ComposerMessagesController < ApplicationController + + before_filter :ensure_logged_in + + def index + finder = ComposerMessagesFinder.new(current_user, params.slice(:composerAction, :topic_id, :post_id)) + render_json_dump([finder.find].compact) + end + +end + diff --git a/app/controllers/education_controller.rb b/app/controllers/education_controller.rb deleted file mode 100644 index 8b7933cd6..000000000 --- a/app/controllers/education_controller.rb +++ /dev/null @@ -1,25 +0,0 @@ -class EducationController < ApplicationController - - before_filter :ensure_logged_in - - def show - raise Discourse::InvalidAccess.new unless params[:id] =~ /^[a-z0-9\-\_]+$/ - raise Discourse::NotFound.new if I18n.t("education.#{params[:id]}", default: MISSING_KEY) == MISSING_KEY - - - education_posts_text = I18n.t('education.until_posts', count: SiteSetting.educate_until_posts) - - markdown_content = "" - if params[:id] == 'new-topic' - markdown_content = SiteContent.content_for(:education_new_topic, education_posts_text: education_posts_text) - else - markdown_content = SiteContent.content_for(:education_new_reply, education_posts_text: education_posts_text) - end - - render text: PrettyText.cook(markdown_content) - end - - private - MISSING_KEY = '_MISSING_KEY_'.freeze - -end diff --git a/app/controllers/posts_controller.rb b/app/controllers/posts_controller.rb index 78de8e35a..7023c5c5f 100644 --- a/app/controllers/posts_controller.rb +++ b/app/controllers/posts_controller.rb @@ -177,11 +177,6 @@ class PostsController < ApplicationController render_serialized(post.replies, PostSerializer) end - # Returns the "you're creating a post education" - def education_text - - end - def bookmark post = find_post_from_params if current_user diff --git a/app/helpers/composer_messages_helper.rb b/app/helpers/composer_messages_helper.rb new file mode 100644 index 000000000..dd95c7517 --- /dev/null +++ b/app/helpers/composer_messages_helper.rb @@ -0,0 +1,2 @@ +module ComposerMessagesHelper +end diff --git a/app/models/user.rb b/app/models/user.rb index ec6579473..1d95fa9fc 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -160,6 +160,9 @@ class User < ActiveRecord::Base key end + def created_topic_count + topics.count + end # tricky, we need our bus to be subscribed from the right spot def sync_notification_channel_position @@ -502,6 +505,7 @@ class User < ActiveRecord::Base Category.topic_create_allowed(self.id).select(:id) end + # Flag all posts from a user as spam def flag_linked_posts_as_spam admin = Discourse.system_user diff --git a/app/models/user_history.rb b/app/models/user_history.rb index a68c1a023..d8b9a245a 100644 --- a/app/models/user_history.rb +++ b/app/models/user_history.rb @@ -46,6 +46,10 @@ class UserHistory < ActiveRecord::Base query end + def self.exists_for_user?(user, action_type) + self.where(target_user_id: user.id, action: UserHistory.actions[action_type]).exists? + end + def new_value_is_json? [UserHistory.actions[:change_site_customization], UserHistory.actions[:delete_site_customization]].include?(action) end diff --git a/config/locales/server.en.yml b/config/locales/server.en.yml index e1c658384..67cbae13d 100644 --- a/config/locales/server.en.yml +++ b/config/locales/server.en.yml @@ -104,6 +104,15 @@ en: For more guidance, [see our FAQ](/faq). This panel will only appear for your first %{education_posts_text}. + avatar: | + ### How about a new picture for your account? + + You've posted a few topics and replies, but your avatar isn't as unique as you are -- it's the same default avatar all new users have. + + Have you considered **[visiting your user profile](%{profile_path})** and uploading a custom image that represents you? + + It's easier to follow community discussions and find interesting people in conversations when everyone has a unique avatar! + activerecord: attributes: diff --git a/config/routes.rb b/config/routes.rb index 441d65282..eea9ce32e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -105,6 +105,7 @@ Discourse::Application.routes.draw do end get 'session/csrf' => 'session#csrf' + get 'composer-messages' => 'composer_messages#index' resources :users, except: [:show, :update] do collection do @@ -183,7 +184,6 @@ Discourse::Application.routes.draw do end end resources :user_actions - resources :education resources :categories, :except => :show get 'category/:id/show' => 'categories#show' diff --git a/lib/composer_messages_finder.rb b/lib/composer_messages_finder.rb new file mode 100644 index 000000000..c3d6c1398 --- /dev/null +++ b/lib/composer_messages_finder.rb @@ -0,0 +1,62 @@ +class ComposerMessagesFinder + + def initialize(user, details) + @user = user + @details = details + end + + def find + check_education_message || + check_avatar_notification + end + + # Determines whether to show the user education text + def check_education_message + if creating_topic? + count = @user.created_topic_count + education_key = :education_new_topic + else + count = @user.topic_reply_count + education_key = :education_new_reply + end + + 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(SiteContent.content_for(education_key, education_posts_text: education_posts_text)) } + end + + nil + end + + # Should a user be contacted to update their avatar? + def check_avatar_notification + + # A user has to be basic at least to be considered for an avatar notification + return unless @user.has_trust_level?(:basic) + + # We don't notify users who have avatars or who have been notified already. + return if @user.user_stat.has_custom_avatar? || 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}")) } + end + + private + + def creating_topic? + return @details[:composerAction] == "createTopic" + end + + def replying? + return @details[:composerAction] == "reply" + end + +end \ No newline at end of file diff --git a/spec/components/composer_messages_finder_spec.rb b/spec/components/composer_messages_finder_spec.rb new file mode 100644 index 000000000..1f0214f7c --- /dev/null +++ b/spec/components/composer_messages_finder_spec.rb @@ -0,0 +1,108 @@ +# encoding: utf-8 +require 'spec_helper' +require 'composer_messages_finder' + +describe ComposerMessagesFinder do + + context "delegates work" do + let(:user) { Fabricate.build(:user) } + let(:finder) { ComposerMessagesFinder.new(user, composerAction: 'createTopic') } + + it "calls all the message finders" do + finder.expects(:check_education_message).once + finder.expects(:check_avatar_notification).once + finder.find + + end + + end + + context '.check_education_message' do + let(:user) { Fabricate.build(:user) } + + context 'creating topic' do + let(:finder) { ComposerMessagesFinder.new(user, composerAction: 'createTopic') } + + before do + SiteSetting.stubs(:educate_until_posts).returns(10) + end + + it "returns a message for a user who has not posted any topics" do + user.expects(:created_topic_count).returns(10) + finder.check_education_message.should be_present + end + + it "returns no message when the user has posted enough topics" do + user.expects(:created_topic_count).returns(11) + finder.check_education_message.should be_blank + end + end + + context 'creating reply' do + let(:finder) { ComposerMessagesFinder.new(user, composerAction: 'reply') } + + before do + SiteSetting.stubs(:educate_until_posts).returns(10) + end + + it "returns a message for a user who has not posted any topics" do + user.expects(:topic_reply_count).returns(10) + finder.check_education_message.should be_present + end + + it "returns no message when the user has posted enough topics" do + user.expects(:topic_reply_count).returns(11) + finder.check_education_message.should be_blank + end + end + + end + + context '.check_avatar_notification' 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 + 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 + + it "creates a notified_about_avatar log" do + UserHistory.exists_for_user?(user, :notified_about_avatar).should be_true + end + end + + it "doesn't return notifications for new users" do + user.trust_level = 0 + finder.check_avatar_notification.should be_blank + end + + it "doesn't return notifications for users who have custom avatars" do + user.user_stat.has_custom_avatar = true + 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 + + end + +end + diff --git a/spec/controllers/composer_messages_controller_spec.rb b/spec/controllers/composer_messages_controller_spec.rb new file mode 100644 index 000000000..45aafd94e --- /dev/null +++ b/spec/controllers/composer_messages_controller_spec.rb @@ -0,0 +1,32 @@ +require 'spec_helper' + +describe ComposerMessagesController do + + context '.index' do + + it 'requires you to be logged in' do + lambda { xhr :get, :index }.should raise_error(Discourse::NotLoggedIn) + end + + context 'when logged in' do + let!(:user) { log_in } + let(:args) { {'topic_id' => '123', 'post_id' => '333', 'composerAction' => 'reply'} } + + it 'redirects to your user preferences' do + xhr :get, :index + response.should be_success + end + + it 'delegates args to the finder' do + finder = mock + ComposerMessagesFinder.expects(:new).with(instance_of(User), has_entries(args)).returns(finder) + finder.expects(:find) + xhr :get, :index, args + end + + end + + end + +end + diff --git a/spec/controllers/education_controller_spec.rb b/spec/controllers/education_controller_spec.rb deleted file mode 100644 index cc2af3dee..000000000 --- a/spec/controllers/education_controller_spec.rb +++ /dev/null @@ -1,46 +0,0 @@ -require 'spec_helper' - -describe EducationController do - - it "requires you to be logged in" do - lambda { xhr :get, :show, id: 'topic' }.should raise_error(Discourse::NotLoggedIn) - end - - context 'when logged in' do - - let!(:user) { log_in(:user) } - - it "returns 404 from a missing id" do - xhr :get, :show, id: 'made-up' - response.response_code.should == 404 - end - - it 'raises an error with a weird id' do - xhr :get, :show, id: '../some-path' - response.should_not be_success - end - - context 'with a valid id' do - - let(:markdown_content) { "Education *markdown* content" } - let(:html_content) {"HTML Content"} - - before do - SiteContent.expects(:content_for).with(:education_new_topic, anything).returns(markdown_content) - PrettyText.expects(:cook).with(markdown_content).returns(html_content) - xhr :get, :show, id: 'new-topic' - end - - it "succeeds" do - response.should be_success - end - - it "converts markdown into HTML" do - response.body.should == html_content - end - - end - - end - -end diff --git a/spec/models/topic_spec.rb b/spec/models/topic_spec.rb index 556e2113e..11103d163 100644 --- a/spec/models/topic_spec.rb +++ b/spec/models/topic_spec.rb @@ -688,6 +688,10 @@ describe Topic do topic.moderator_posts_count.should == 0 end + it "its user has a topics_count of 1" do + topic.user.created_topic_count.should == 1 + end + context 'post' do let(:post) { Fabricate(:post, topic: topic, user: topic.user) }