New User Education goes through a server side ComposerMessages check. Composer message for users

who don't have avatars.

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;
   },

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);
+        }
+      });
+    });
   }
 
 });

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);
+      });
+    });
+  }
+
+})

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
+

app/controllers/education_controller.rb

@@ -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

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

app/helpers/composer_messages_helper.rb

@@ -0,0 +1,2 @@
+module ComposerMessagesHelper
+end

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

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

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:

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'

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

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
+

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
+

spec/controllers/education_controller_spec.rb

@@ -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

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) }