Merge pull request #6921 from SuperTux88/cleanup-status_message

Cleanup StatusMessage

Changelog.md

@@ -112,6 +112,7 @@ before.
 * Use Poltergeist instead of Selenium [#6768](https://github.com/diaspora/diaspora/pull/6768)
 * Redesigned the landing page and added dedicated notes for podmins [#6268](https://github.com/diaspora/diaspora/pull/6268)
 * Moved the entire federation implementation into its own gem. 🎉 [#6873](https://github.com/diaspora/diaspora/pull/6873)
+* Remove `StatusMessage#raw_message` [#6921](https://github.com/diaspora/diaspora/pull/6921)
 
 ## Bug fixes
 * Destroy Participation when removing interactions with a post [#5852](https://github.com/diaspora/diaspora/pull/5852)

app/models/post.rb

@@ -59,7 +59,6 @@ class Post < ActiveRecord::Base
   end
 
   def root; end
-  def raw_message; ""; end
   def mentioned_people; []; end
   def photos; []; end
 

app/models/reshare.rb

@@ -29,8 +29,8 @@ class Reshare < Post
            :message, :nsfw,
            to: :absolute_root, allow_nil: true
 
-  def raw_message
-    absolute_root.try(:raw_message) || super
+  def text
+    absolute_root.try(:text) || ""
   end
 
   def mentioned_people

app/models/status_message.rb

@@ -8,23 +8,18 @@ class StatusMessage < Post
   include PeopleHelper
 
   acts_as_taggable_on :tags
-  extract_tags_from :raw_message
+  extract_tags_from :text
 
   validates_length_of :text, :maximum => 65535, :message => proc {|p, v| I18n.t('status_messages.too_long', :count => 65535, :current_length => v[:value].length)}
 
   # don't allow creation of empty status messages
-  validate :presence_of_content, on: :create, if: proc {|sm| sm.author && sm.author.local? }
+  validate :presence_of_content, on: :create
 
   has_many :photos, :dependent => :destroy, :foreign_key => :status_message_guid, :primary_key => :guid
 
   has_one :location
   has_one :poll, autosave: true
 
-
-  # a StatusMessage is federated before its photos are so presence_of_content() fails erroneously if no text is present
-  # therefore, we put the validation in a before_destory callback instead of a validation
-  before_destroy :absence_of_content
-
   attr_accessor :oembed_url
   attr_accessor :open_graph_url
 
@@ -42,29 +37,23 @@ class StatusMessage < Post
   end
 
   def self.user_tag_stream(user, tag_ids)
-    owned_or_visible_by_user(user).
-      tag_stream(tag_ids)
+    owned_or_visible_by_user(user).tag_stream(tag_ids)
   end
 
   def self.public_tag_stream(tag_ids)
-    all_public.
-      tag_stream(tag_ids)
-  end
-
-  def raw_message
-    read_attribute(:text) || ""
+    all_public.tag_stream(tag_ids)
   end
 
-  def raw_message=(text)
-    write_attribute(:text, text)
+  def self.tag_stream(tag_ids)
+    joins(:taggings).where("taggings.tag_id IN (?)", tag_ids)
   end
 
   def nsfw
-    self.raw_message.match(/#nsfw/i) || super
+    text.try(:match, /#nsfw/i) || super
   end
 
   def message
-    @message ||= Diaspora::MessageRenderer.new raw_message, mentioned_people: mentioned_people
+    @message ||= Diaspora::MessageRenderer.new(text, mentioned_people: mentioned_people)
   end
 
   def mentioned_people
@@ -72,7 +61,7 @@ class StatusMessage < Post
       create_mentions if self.mentions.empty?
       self.mentions.includes(:person => :profile).map{ |mention| mention.person }
     else
-      Diaspora::Mentionable.people_from_string(self.raw_message)
+      Diaspora::Mentionable.people_from_string(text)
     end
   end
 
@@ -84,7 +73,7 @@ class StatusMessage < Post
   ## ---- ----
 
   def create_mentions
-    ppl = Diaspora::Mentionable.people_from_string(self.raw_message)
+    ppl = Diaspora::Mentionable.people_from_string(text)
     ppl.each do |person|
       self.mentions.find_or_create_by(person_id: person.id)
     end
@@ -103,7 +92,7 @@ class StatusMessage < Post
   end
 
   def text_and_photos_blank?
-    self.raw_message.blank? && self.photos.blank?
+    text.blank? && photos.blank?
   end
 
   def queue_gather_oembed_data
@@ -132,22 +121,10 @@ class StatusMessage < Post
     }
   end
 
-  protected
-  def presence_of_content
-    if text_and_photos_blank?
-      errors[:base] << "Cannot create a StatusMessage without content"
-    end
-  end
-
-  def absence_of_content
-    unless text_and_photos_blank?
-      errors[:base] << "Cannot destory a StatusMessage with text and/or photos present"
-    end
-  end
-
   private
-  def self.tag_stream(tag_ids)
-    joins(:taggings).where('taggings.tag_id IN (?)', tag_ids)
+
+  def presence_of_content
+    errors[:base] << "Cannot create a StatusMessage without content" if text_and_photos_blank?
   end
 end
 

app/presenters/post_presenter.rb

@@ -42,7 +42,7 @@ class PostPresenter < BasePresenter
     if @post.message
       @post.message.plain_text_for_json
     else
-      @post.raw_message
+      @post.text
     end
   end
 

lib/diaspora/federation/entities.rb

@@ -222,7 +222,7 @@ module Diaspora
         DiasporaFederation::Entities::StatusMessage.new(
           author:                status_message.diaspora_handle,
           guid:                  status_message.guid,
-          text:                  status_message.raw_message,
+          text:                  status_message.text,
           photos:                status_message.photos.map {|photo| photo(photo) },
           location:              status_message.location ? location(status_message.location) : nil,
           poll:                  status_message.poll ? poll(status_message.poll) : nil,

lib/diaspora/federation/receive.rb

@@ -161,9 +161,20 @@ module Diaspora
       end
 
       def self.status_message(entity)
-        save_status_message(entity).tap do
-          entity.photos.map do |photo|
-            ignore_existing_guid(Photo, photo.guid, author_of(photo)) { save_photo(photo) }
+        try_load_existing_guid(StatusMessage, entity.guid, author_of(entity)) do
+          StatusMessage.new(
+            author:                author_of(entity),
+            guid:                  entity.guid,
+            text:                  entity.text,
+            public:                entity.public,
+            created_at:            entity.created_at,
+            provider_display_name: entity.provider_display_name
+          ).tap do |status_message|
+            status_message.location = build_location(entity.location) if entity.location
+            status_message.poll = build_poll(entity.poll) if entity.poll
+            status_message.photos = save_or_load_photos(entity.photos)
+
+            status_message.save!
           end
         end
       end
@@ -228,6 +239,12 @@ module Diaspora
         )
       end
 
+      private_class_method def self.save_or_load_photos(photos)
+        photos.map do |photo|
+          try_load_existing_guid(Photo, photo.guid, author_of(photo)) { save_photo(photo) }
+        end
+      end
+
       private_class_method def self.receive_relayable(klass, entity)
         save_relayable(klass, entity) { yield }.tap {|relayable| relay_relayable(relayable) if relayable }
       end
@@ -243,24 +260,6 @@ module Diaspora
         end
       end
 
-      private_class_method def self.save_status_message(entity)
-        try_load_existing_guid(StatusMessage, entity.guid, author_of(entity)) do
-          StatusMessage.new(
-            author:                author_of(entity),
-            guid:                  entity.guid,
-            raw_message:           entity.text,
-            public:                entity.public,
-            created_at:            entity.created_at,
-            provider_display_name: entity.provider_display_name
-          ).tap do |status_message|
-            status_message.location = build_location(entity.location) if entity.location
-            status_message.poll = build_poll(entity.poll) if entity.poll
-
-            status_message.save!
-          end
-        end
-      end
-
       private_class_method def self.retract_if_author_ignored(relayable)
         parent_author = relayable.parent.author.owner
         return unless parent_author && parent_author.ignored_people.include?(relayable.author)

lib/diaspora/message_renderer.rb

@@ -120,7 +120,7 @@ module Diaspora
 
     delegate :empty?, :blank?, :present?, to: :raw
 
-    # @param [String] raw_message Raw input text
+    # @param [String] text Raw input text
     # @param [Hash] opts Global options affecting output
     # @option opts [Array<Person>] :mentioned_people ([]) List of people
     #   allowed to mention
@@ -147,8 +147,8 @@ module Diaspora
     #   to Redcarpet
     # @option opts [Hash] :markdown_render_options Override default options
     #   passed to the Redcarpet renderer
-    def initialize raw_message, opts={}
-      @raw_message = raw_message
+    def initialize(text, opts={})
+      @text = text
       @options = DEFAULTS.deep_merge opts
     end
 
@@ -211,10 +211,10 @@ module Diaspora
     #   this length. If not given defaults to 70.
     def title opts={}
       # Setext-style header
-      heading = if /\A(?<setext_content>.{1,200})\n(?:={1,200}|-{1,200})(?:\r?\n|$)/ =~ @raw_message.lstrip
+      heading = if /\A(?<setext_content>.{1,200})\n(?:={1,200}|-{1,200})(?:\r?\n|$)/ =~ @text.lstrip
                   setext_content
                 # Atx-style header
-      elsif /\A\#{1,6}\s+(?<atx_content>.{1,200}?)(?:\s+#+)?(?:\r?\n|$)/ =~ @raw_message.lstrip
+                elsif /\A\#{1,6}\s+(?<atx_content>.{1,200}?)(?:\s+#+)?(?:\r?\n|$)/ =~ @text.lstrip
                   atx_content
                 end
 
@@ -236,7 +236,7 @@ module Diaspora
     end
 
     def raw
-      @raw_message
+      @text
     end
 
     def to_s
@@ -245,8 +245,8 @@ module Diaspora
 
     private
 
-    def process opts, &block
-      Processor.process(@raw_message, @options.deep_merge(opts), &block)
+    def process(opts, &block)
+      Processor.process(@text, @options.deep_merge(opts), &block)
     end
   end
 end

spec/controllers/users_controller_spec.rb

@@ -61,7 +61,7 @@ describe UsersController, :type => :controller do
     it 'renders xml if atom is requested' do
       sm = FactoryGirl.create(:status_message, :public => true, :author => @user.person)
       get :public, :username => @user.username, :format => :atom
-      expect(response.body).to include(sm.raw_message)
+      expect(response.body).to include(sm.text)
     end
 
     it 'renders xml if atom is requested with clickalbe urls' do
@@ -77,7 +77,7 @@ describe UsersController, :type => :controller do
     it 'includes reshares in the atom feed' do
       reshare = FactoryGirl.create(:reshare, :author => @user.person)
       get :public, :username => @user.username, :format => :atom
-      expect(response.body).to include reshare.root.raw_message
+      expect(response.body).to include reshare.root.text
     end
 
     it 'do not show reshares in atom feed if origin post is deleted' do

spec/lib/diaspora/federation/receive_spec.rb

@@ -591,6 +591,19 @@ describe Diaspora::Federation::Receive do
         expect(status_message.photos.map(&:guid)).to include(photo1.guid, photo2.guid)
       end
 
+      it "receives a status message only with photos and without text" do
+        entity = DiasporaFederation::Entities::StatusMessage.new(status_message_entity.to_h.merge(text: nil))
+        received = Diaspora::Federation::Receive.perform(entity)
+
+        status_message = StatusMessage.find_by!(guid: status_message_entity.guid)
+
+        expect(received).to eq(status_message)
+        expect(status_message.author).to eq(sender)
+
+        expect(status_message.text).to be_nil
+        expect(status_message.photos.map(&:guid)).to include(photo1.guid, photo2.guid)
+      end
+
       it "does not overwrite the photos if they already exist" do
         received_photo = Diaspora::Federation::Receive.photo(photo1)
         received_photo.text = "foobar"

spec/lib/diaspora/fetcher/public_spec.rb

@@ -122,10 +122,10 @@ describe Diaspora::Fetcher::Public do
         end
       end
 
-      it 'copied the text correctly' do
+      it "copied the text correctly" do
         @data.each do |post|
-          entry = StatusMessage.find_by_guid(post['guid'])
-          expect(entry.raw_message).to eql(post['text'])
+          entry = StatusMessage.find_by_guid(post["guid"])
+          expect(entry.text).to eql(post["text"])
         end
       end
 

spec/lib/diaspora/mentionable_spec.rb

@@ -24,7 +24,7 @@ STR
   describe "#format" do
     context "html output" do
       it "adds the links to the formatted message" do
-        fmt_msg = Diaspora::Mentionable.format(@status_msg.raw_message, @people)
+        fmt_msg = Diaspora::Mentionable.format(@status_msg.text, @people)
 
         @people.each do |person|
           expect(fmt_msg).to include person_link(person, class: "mention hovercardable")
@@ -32,7 +32,7 @@ STR
       end
 
       it "should work correct when message is escaped html" do
-        raw_msg = @status_msg.raw_message
+        raw_msg = @status_msg.text
         fmt_msg = Diaspora::Mentionable.format(CGI.escapeHTML(raw_msg), @people)
 
         @people.each do |person|
@@ -45,7 +45,7 @@ STR
         p.first_name = "</a><script>alert('h')</script>"
         p.save!
 
-        fmt_msg = Diaspora::Mentionable.format(@status_msg.raw_message, @people)
+        fmt_msg = Diaspora::Mentionable.format(@status_msg.text, @people)
 
         expect(fmt_msg).not_to include(p.first_name)
         expect(fmt_msg).to include("&gt;", "&lt;", "&#39;") # ">", "<", "'"
@@ -54,7 +54,7 @@ STR
 
     context "plain text output" do
       it "removes mention markup and displays unformatted name" do
-        fmt_msg = Diaspora::Mentionable.format(@status_msg.raw_message, @people, plain_text: true)
+        fmt_msg = Diaspora::Mentionable.format(@status_msg.text, @people, plain_text: true)
 
         @people.each do |person|
           expect(fmt_msg).to include person.first_name
@@ -64,7 +64,7 @@ STR
     end
 
     it "leaves the name of people that cannot be found" do
-      fmt_msg = Diaspora::Mentionable.format(@status_msg.raw_message, [])
+      fmt_msg = Diaspora::Mentionable.format(@status_msg.text, [])
       expect(fmt_msg).to eql @test_txt_plain
     end
   end

spec/models/status_message_spec.rb

@@ -91,7 +91,7 @@ describe StatusMessage, type: :model do
     end
   end
 
-  context "emptyness" do
+  context "emptiness" do
     it "needs either a message or at least one photo" do
       post = user.build_post(:status_message, text: nil)
       expect(post).not_to be_valid
@@ -109,14 +109,14 @@ describe StatusMessage, type: :model do
       post.photos << photo
       expect(post).to be_valid
       expect(post.message.to_s).to be_empty
-      expect(post.raw_message).to eq ""
-      expect(post.nsfw).to be_falsy
+      expect(post.text).to be_nil
+      expect(post.nsfw).to be_falsey
       expect(post.errors.full_messages).to eq([])
     end
 
-    it "doesn't check for content when author is remote (federation...)" do
+    it "also checks for content when author is remote" do
       post = FactoryGirl.build(:status_message, text: nil)
-      expect(post).to be_valid
+      expect(post).not_to be_valid
     end
   end