Merge branch 'stable' into develop

Changelog.md

@@ -61,6 +61,7 @@ Ruby 2.0 is no longer officially supported.
 * Improved logging source [#6041](https://github.com/diaspora/diaspora/pull/6041)
 * Gracefully handle duplicate entry while receiving share-visibility in parallel [#6068](https://github.com/diaspora/diaspora/pull/6068)
 * Update twitter gem to get rid of deprecation warnings [#6083](https://github.com/diaspora/diaspora/pull/6083)
+* Refactor photos federation to get rid of some hacks [#6082](https://github.com/diaspora/diaspora/pull/6082)
 
 ## Bug fixes
 * Disable auto follow back on aspect deletion [#5846](https://github.com/diaspora/diaspora/pull/5846)
@@ -93,6 +94,7 @@ Ruby 2.0 is no longer officially supported.
 * Only strip text direction codepoints around hashtags [#6067](https://github.com/diaspora/diaspora/issues/6067)
 * Fix selected week on admin weekly stats page [#6079](https://github.com/diaspora/diaspora/pull/6079)
 * Fix that some unread conversations may be hidden [#6060](https://github.com/diaspora/diaspora/pull/6060)
+* Fix photo links in the mobile interface [#6082](https://github.com/diaspora/diaspora/pull/6082)
 
 ## Features
 * Hide post title of limited post in comment notification email [#5843](https://github.com/diaspora/diaspora/pull/5843)

app/controllers/status_messages_controller.rb

@@ -73,11 +73,6 @@ class StatusMessagesController < ApplicationController
 
       current_user.dispatch_post(@status_message, :url => short_post_url(@status_message.guid), :service_types => receiving_services)
 
-      #this is done implicitly, somewhere else, but it doesnt work, says max. :'(
-      @status_message.photos.each do |photo|
-        current_user.dispatch_post(photo)
-      end
-
       current_user.participate!(@status_message)
 
       if coming_from_profile_page? && !own_profile_page? # if this is a post coming from a profile page

app/models/status_message.rb

@@ -116,6 +116,7 @@ class StatusMessage < Post
 
   def update_and_dispatch_attached_photos(sender)
     if self.photos.any?
+      logger.info "dispatch photos for StatusMessage:#{guid}"
       Photo.where(status_message_guid: guid).update_all(:public => self.public)
       self.photos.each do |photo|
         if photo.pending

db/migrate/20150607143809_fix_photo_public_flag.rb

@@ -0,0 +1,9 @@
+class FixPhotoPublicFlag < ActiveRecord::Migration
+  def up
+    Photo.joins(:status_message).where(posts: {public: true}).update_all(public: true)
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

db/schema.rb

@@ -11,7 +11,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema.define(version: 20150531005120) do
+ActiveRecord::Schema.define(version: 20150607143809) do
 
   create_table "account_deletions", force: :cascade do |t|
     t.string   "diaspora_handle", limit: 255

lib/diaspora/federated/base.rb

@@ -23,9 +23,11 @@ module Diaspora
 
       module InstanceMethods
         def to_diaspora_xml
+          xml = to_xml
+          ::Logging::Logger["XMLLogger"].debug "to_xml: #{xml}"
           <<-XML
           <XML>
-          <post>#{to_xml.to_s}</post>
+            <post>#{xml}</post>
           </XML>
           XML
         end

lib/diaspora/federated/shareable.rb

@@ -44,6 +44,16 @@ module Diaspora
           end
         end
 
+        # @return [void]
+        def receive_public
+          local_shareable = persisted_shareable
+          if local_shareable
+            update_existing_sharable(local_shareable) if verify_persisted_shareable(local_shareable)
+          else
+            save!
+          end
+        end
+
         # The list of people that should receive this Shareable.
         #
         # @param [User] user The context, or dispatching user.

lib/diaspora/parser.rb

@@ -8,6 +8,7 @@ module Diaspora
       doc = Nokogiri::XML(xml) {|cfg| cfg.noblanks }
       return unless body = doc.xpath("/XML/post").children.first
       class_name = body.name.gsub("-", "/")
+      ::Logging::Logger["XMLLogger"].debug "from_xml: #{body}"
       begin
         class_name.camelize.constantize.from_xml body.to_s
       rescue NameError => e

lib/postzord/receiver.rb

@@ -14,12 +14,19 @@ class Postzord::Receiver
     self.receive!
   end
 
+  private
+
   def author_does_not_match_xml_author?
-    if (@author.diaspora_handle != xml_author)
-      logger.warn "event=receive status=abort reason='author in xml does not match retrieved person' " \
-                  "payload_type=#{@object.class} sender=#{@author.diaspora_handle}"
-      return true
-    end
-  end
+    return false unless @author.diaspora_handle != xml_author
+    logger.error "event=receive status=abort reason='author in xml does not match retrieved person' " \
+                 "type=#{@object.class} sender=#{@author.diaspora_handle}"
+    true
   end
 
+  def relayable_without_parent?
+    return false unless @object.respond_to?(:relayable?) && @object.parent.nil?
+    logger.error "event=receive status=abort reason='no corresponding post' type=#{@object.class} " \
+                 "sender=#{@author.diaspora_handle}"
+    true
+  end
+end

lib/postzord/receiver/private.rb

@@ -9,30 +9,28 @@ class Postzord::Receiver::Private < Postzord::Receiver
     @user_person = @user.person
     @salmon_xml = opts[:salmon_xml]
 
-    @sender = opts[:person] || Webfinger.new(self.salmon.author_id).fetch
-    @author = @sender
+    @author = opts[:person] || Webfinger.new(salmon.author_id).fetch
 
     @object = opts[:object]
   end
 
   def receive!
-    if @sender && self.salmon.verified_for_key?(@sender.public_key)
+    if @author && salmon.verified_for_key?(@author.public_key)
       parse_and_receive(salmon.parsed_data)
     else
       logger.error "event=receive status=abort reason='not_verified for key' " \
                    "recipient=#{@user.diaspora_handle} sender=#{@salmon.author_id}"
     end
   rescue => e
-    logger.error "failed to receive #{@object.class} from sender:#{@sender.id} for user:#{@user.id}: #{e.message}\n" \
+    logger.error "failed to receive #{@object.class} from sender:#{@author.id} for user:#{@user.id}: #{e.message}\n" \
                  "#{@object.inspect}"
     raise e
   end
 
   def parse_and_receive(xml)
     @object ||= Diaspora::Parser.from_xml(xml)
-    return if @object.nil?
 
-    logger.info "user:#{@user.id} starting private receive from person:#{@sender.guid}"
+    logger.info "user:#{@user.id} starting private receive from person:#{@author.guid}"
 
     validate_object
     set_author!
@@ -43,27 +41,17 @@ class Postzord::Receiver::Private < Postzord::Receiver
   def receive_object
     obj = @object.receive(@user, @author)
     Notification.notify(@user, obj, @author) if obj.respond_to?(:notification_type)
-    logger.info "user:#{@user.id} successfully received #{@object.class} from person #{@sender.guid}" \
+    logger.info "user:#{@user.id} successfully received #{@object.class} from person #{@author.guid}" \
                 "#{": #{@object.guid}" if @object.respond_to?(:guid)}"
     logger.debug "received: #{@object.inspect}"
   end
 
   protected
+
   def salmon
     @salmon ||= Salmon::EncryptedSlap.from_xml(@salmon_xml, @user)
   end
 
-  def validate_object
-    raise Diaspora::ContactRequiredUnlessRequest if contact_required_unless_request
-    raise Diaspora::RelayableObjectWithoutParent if relayable_without_parent?
-
-    assign_sender_handle_if_request
-
-    raise Diaspora::AuthorXMLAuthorMismatch if author_does_not_match_xml_author?
-
-    @object
-  end
-
   def xml_author
     if @object.respond_to?(:relayable?)
       #if A and B are friends, and A sends B a comment from C, we delegate the validation to the owner of the post being commented on
@@ -85,18 +73,21 @@ class Postzord::Receiver::Private < Postzord::Receiver
   private
 
   # validations
-  def relayable_without_parent?
-    if @object.respond_to?(:relayable?) && @object.parent.nil?
-      logger.error "event=receive status=abort reason='no corresponding post' type=#{@object.class} " \
-                   "recipient=#{@user_person.diaspora_handle} sender=#{@sender.diaspora_handle}"
-      return true
-    end
+
+  def validate_object
+    raise Diaspora::XMLNotParseable if @object.nil?
+    raise Diaspora::ContactRequiredUnlessRequest if contact_required_unless_request
+    raise Diaspora::RelayableObjectWithoutParent if relayable_without_parent?
+
+    assign_sender_handle_if_request
+
+    raise Diaspora::AuthorXMLAuthorMismatch if author_does_not_match_xml_author?
   end
 
   def contact_required_unless_request
-    unless @object.is_a?(Request) || @user.contact_for(@sender)
+    unless @object.is_a?(Request) || @user.contact_for(@author)
       logger.error "event=receive status=abort reason='sender not connected to recipient' type=#{@object.class} " \
-                   "recipient=#{@user_person.diaspora_handle} sender=#{@sender.diaspora_handle}"
+                   "recipient=#{@user_person.diaspora_handle} sender=#{@author.diaspora_handle}"
       return true
     end
   end
@@ -104,7 +95,7 @@ class Postzord::Receiver::Private < Postzord::Receiver
   def assign_sender_handle_if_request
     #special casey
     if @object.is_a?(Request)
-      @object.sender_handle = @sender.diaspora_handle
+      @object.sender_handle = @author.diaspora_handle
     end
   end
 end

lib/postzord/receiver/public.rb

@@ -9,8 +9,6 @@ class Postzord::Receiver::Public < Postzord::Receiver
   def initialize(xml)
     @salmon = Salmon::Slap.from_xml(xml)
     @author = Webfinger.new(@salmon.author_id).fetch
-
-    logger.info "Receiving public object from person:#{@author.id}"
   end
 
   # @return [Boolean]
@@ -23,7 +21,7 @@ class Postzord::Receiver::Public < Postzord::Receiver
     return unless verified_signature?
     # return false unless account_deletion_is_from_author
 
-    return unless save_object
+    parse_and_receive(@salmon.parsed_data)
 
     logger.info "received a #{@object.inspect}"
     if @object.is_a?(SignedRetraction) # feels like a hack
@@ -51,15 +49,23 @@ class Postzord::Receiver::Public < Postzord::Receiver
     receiver.notify_users
   end
 
-  # @return [Object]
-  def save_object
-    @object = Diaspora::Parser.from_xml(@salmon.parsed_data)
-    raise Diaspora::XMLNotParseable if @object.nil?
-    raise Diaspora::NonPublic if object_can_be_public_and_it_is_not?
-    raise Diaspora::RelayableObjectWithoutParent if object_must_have_parent_and_does_not?
-    raise Diaspora::AuthorXMLAuthorMismatch if author_does_not_match_xml_author?
-    @object.save! if @object.respond_to?(:save!)
-    @object
+  # @return [void]
+  def parse_and_receive(xml)
+    @object = Diaspora::Parser.from_xml(xml)
+
+    logger.info "starting public receive from person:#{@author.guid}"
+
+    validate_object
+    receive_object
+  end
+
+  # @return [void]
+  def receive_object
+    if @object.respond_to?(:receive_public)
+      @object.receive_public
+    elsif @object.respond_to?(:save!)
+      @object.save!
+    end
   end
 
   # @return [Array<Integer>] User ids
@@ -78,6 +84,15 @@ class Postzord::Receiver::Public < Postzord::Receiver
 
   private
 
+  # validations
+
+  def validate_object
+    raise Diaspora::XMLNotParseable if @object.nil?
+    raise Diaspora::NonPublic if object_can_be_public_and_it_is_not?
+    raise Diaspora::RelayableObjectWithoutParent if relayable_without_parent?
+    raise Diaspora::AuthorXMLAuthorMismatch if author_does_not_match_xml_author?
+  end
+
   def account_deletion_is_from_author
     return true unless @object.is_a?(AccountDeletion)
     return false if @object.diaspora_handle != @author.diaspora_handle
@@ -88,12 +103,4 @@ class Postzord::Receiver::Public < Postzord::Receiver
   def object_can_be_public_and_it_is_not?
     @object.respond_to?(:public) && !@object.public?
   end
-
-  def object_must_have_parent_and_does_not?
-    if @object.respond_to?(:relayable?) # comment, like
-      @object.parent.nil?
-    else
-      false
-    end
-  end
 end

spec/lib/postzord/receiver/private_spec.rb

@@ -18,7 +18,7 @@ describe Postzord::Receiver::Private do
 
       zord = Postzord::Receiver::Private.new(bob, :person => alice.person, :object => @alices_post)
       expect(zord.instance_variable_get(:@user)).not_to be_nil
-      expect(zord.instance_variable_get(:@sender)).not_to be_nil
+      expect(zord.instance_variable_get(:@author)).not_to be_nil
       expect(zord.instance_variable_get(:@object)).not_to be_nil
     end
 
@@ -32,7 +32,7 @@ describe Postzord::Receiver::Private do
 
       zord = Postzord::Receiver::Private.new(bob, :salmon_xml => @salmon_xml)
       expect(zord.instance_variable_get(:@user)).not_to be_nil
-      expect(zord.instance_variable_get(:@sender)).not_to be_nil
+      expect(zord.instance_variable_get(:@author)).not_to be_nil
       expect(zord.instance_variable_get(:@salmon_xml)).not_to be_nil
     end
   end
@@ -45,13 +45,13 @@ describe Postzord::Receiver::Private do
 
     context "does not parse and receive" do
       it "if the salmon author does not exist" do
-        @zord.instance_variable_set(:@sender, nil)
+        @zord.instance_variable_set(:@author, nil)
         expect(@zord).not_to receive(:parse_and_receive)
         @zord.receive!
       end
 
       it "if the author does not match the signature" do
-        @zord.instance_variable_set(:@sender, FactoryGirl.create(:person))
+        @zord.instance_variable_set(:@author, FactoryGirl.create(:person))
         expect(@zord).not_to receive(:parse_and_receive)
         @zord.receive!
       end

spec/lib/postzord/receiver/public_spec.rb

@@ -47,7 +47,7 @@ describe Postzord::Receiver::Public do
 
     it "does not save the object if signature is not verified" do
       expect(@receiver).to receive(:verified_signature?).and_return(false)
-      expect(@receiver).not_to receive(:save_object)
+      expect(@receiver).not_to receive(:parse_and_receive)
       @receiver.perform!
     end
 
@@ -58,7 +58,7 @@ describe Postzord::Receiver::Public do
       end
 
       it 'saves the parsed object' do
-        expect(@receiver).to receive(:save_object)
+        expect(@receiver).to receive(:parse_and_receive).and_call_original
         @receiver.perform!
       end
 
@@ -120,14 +120,15 @@ describe Postzord::Receiver::Public do
     end
   end
 
-  describe "#save_object" do
+  describe "#parse_and_receive" do
     before do
       @receiver = Postzord::Receiver::Public.new(@xml)
+      @parsed_salmon = Salmon::Slap.from_xml(@xml)
     end
 
     it "should raise a Diaspora::XMLNotParseable when the parsed object is nil" do
       expect(Diaspora::Parser).to receive(:from_xml).and_return(nil)
-      expect { @receiver.save_object }.to raise_error(Diaspora::XMLNotParseable)
+      expect { @receiver.parse_and_receive(@parsed_salmon.parsed_data) }.to raise_error(Diaspora::XMLNotParseable)
     end
   end
 end

spec/lib/postzord/receiver_spec.rb

@@ -2,7 +2,7 @@
 #   licensed under the Affero General Public License version 3 or later.  See
 #   the COPYRIGHT file.
 
-require 'spec_helper'
+require "spec_helper"
 
 describe Postzord::Receiver do
   before do
@@ -14,10 +14,53 @@ describe Postzord::Receiver do
       allow(@receiver).to receive(:receive!).and_return(true)
     end
 
-    it 'calls receive!' do
+    it "calls receive!" do
       expect(@receiver).to receive(:receive!)
       @receiver.perform!
     end
   end
+
+  describe "#author_does_not_match_xml_author?" do
+    before do
+      @receiver.instance_variable_set(:@author, alice.person)
+      allow(@receiver).to receive(:xml_author).and_return(alice.diaspora_handle)
     end
 
+    it "should return false if the author matches" do
+      allow(@receiver).to receive(:xml_author).and_return(alice.diaspora_handle)
+      expect(@receiver.send(:author_does_not_match_xml_author?)).to be_falsey
+    end
+
+    it "should return true if the author does not match" do
+      allow(@receiver).to receive(:xml_author).and_return(bob.diaspora_handle)
+      expect(@receiver.send(:author_does_not_match_xml_author?)).to be_truthy
+    end
+  end
+
+  describe "#relayable_without_parent?" do
+    before do
+      @receiver.instance_variable_set(:@author, alice.person)
+    end
+
+    it "should return false if object is not relayable" do
+      @receiver.instance_variable_set(:@object, nil)
+      expect(@receiver.send(:relayable_without_parent?)).to be_falsey
+    end
+
+    context "if object is relayable" do
+      before do
+        @comment = bob.build_comment(text: "yo", post: FactoryGirl.create(:status_message))
+        @receiver.instance_variable_set(:@object, @comment)
+      end
+
+      it "should return false if object has parent" do
+        expect(@receiver.send(:relayable_without_parent?)).to be_falsey
+      end
+
+      it "should return true if object has no parent" do
+        @comment.parent = nil
+        expect(@receiver.send(:relayable_without_parent?)).to be_truthy
+      end
+    end
+  end
+end

spec/models/photo_spec.rb

@@ -277,4 +277,19 @@ describe Photo, :type => :model do
       }.to_not change(StatusMessage, :count)
     end
   end
+
+  describe "#receive_public" do
+    it "updates the photo if it is already persisted" do
+      allow(@photo).to receive(:persisted_shareable).and_return(@photo2)
+      expect(@photo2).to receive(:update_attributes)
+      @photo.receive_public
+    end
+
+    it "does not update the photo if the author mismatches" do
+      @photo.author = bob.person
+      allow(@photo).to receive(:persisted_shareable).and_return(@photo2)
+      expect(@photo).not_to receive(:update_existing_sharable)
+      @photo.receive_public
+    end
+  end
 end

spec/models/poll_participation_spec.rb

@@ -2,7 +2,6 @@ require 'spec_helper'
 require Rails.root.join("spec", "shared_behaviors", "relayable")
 
 describe PollParticipation, :type => :model do
-  
   before do
     @alices_aspect = alice.aspects.first
     @status = bob.post(:status_message, :text => "hello", :to => bob.aspects.first.id)
@@ -98,7 +97,8 @@ describe PollParticipation, :type => :model do
       allow_any_instance_of(Person).to receive(:local?).and_return(false)
       expect{
         salmon = Salmon::Slap.create_by_user_and_activity(alice, @poll_participation_alice.to_diaspora_xml).xml_for(@poll_participant)
-        Postzord::Receiver::Public.new(salmon).save_object
+        parsed_salmon = Salmon::Slap.from_xml(salmon)
+        Postzord::Receiver::Public.new(salmon).parse_and_receive(parsed_salmon.parsed_data)
       }.to_not raise_error
     end
   end

spec/models/post_spec.rb

@@ -336,6 +336,22 @@ describe Post, :type => :model do
     end
   end
 
+  describe "#receive_public" do
+    it "saves the post if the post is unknown" do
+      @post = FactoryGirl.create(:status_message, author: bob.person)
+      allow(@post).to receive(:persisted_shareable).and_return(nil)
+      expect(@post).to receive(:save!)
+      @post.receive_public
+    end
+
+    it "does not update the post because not mutable" do
+      @post = FactoryGirl.create(:status_message, author: bob.person)
+      expect(@post).to receive(:update_existing_sharable).and_call_original
+      expect(@post).not_to receive(:update_attributes)
+      @post.receive_public
+    end
+  end
+
   describe '#reshares_count' do
     before :each do
       @post = @user.post :status_message, :text => "hello", :to => @aspect.id, :public => true