Currently we have some license issues. We are working on it.

Commit 2a3dde1a authored by theworldbright's avatar theworldbright Committed by Jonne Haß
Browse files

Refactor PostService and extract its tests

Squashed commits:

[ada0f09] Remove favorites from Posts table

closes #6208
parent d7243971
......@@ -2,6 +2,7 @@
## Refactor
* Drop broken correlations from the admin pages [#6223](https://github.com/diaspora/diaspora/pull/6223)
* Extract PostService from PostsController [#6208](https://github.com/diaspora/diaspora/pull/6208)
## Bug fixes
* Fix indentation and a link title on the default home page [#6212](https://github.com/diaspora/diaspora/pull/6212)
......
......@@ -10,7 +10,7 @@ class PostsController < ApplicationController
respond_to :html, :mobile, :json, :xml
rescue_from Diaspora::NonPublic do |_exception|
rescue_from Diaspora::NonPublic do
respond_to do |format|
format.all { render template: "errors/not_public", status: 404, layout: "application" }
end
......@@ -18,7 +18,7 @@ class PostsController < ApplicationController
def show
post_service = PostService.new(id: params[:id], user: current_user)
post_service.assign_post_and_mark_notifications
post_service.mark_user_notifications
@post = post_service.post
respond_to do |format|
format.html { gon.post = post_service.present_json }
......@@ -35,14 +35,12 @@ class PostsController < ApplicationController
post_id = OEmbedPresenter.id_from_url(params.delete(:url))
post_service = PostService.new(id: post_id, user: current_user,
oembed: params.slice(:format, :maxheight, :minheight))
post_service.assign_post
render json: post_service.present_oembed
end
def interactions
post_service = PostService.new(id: params[:id], user: current_user)
post_service.assign_post
respond_with(post_service.present_interactions_json)
respond_with post_service.present_interactions_json
end
def destroy
......
......@@ -149,17 +149,19 @@ class Post < ActiveRecord::Base
end
def self.find_public(id)
post = Post.where(Post.key_sym(id) => id).includes(:author, comments: :author).first
post.try(:public?) || raise(Diaspora::NonPublic)
post || raise(ActiveRecord::RecordNotFound.new("could not find a post with id #{id}"))
where(post_key(id) => id).includes(:author, comments: :author).first.tap do |post|
raise ActiveRecord::RecordNotFound.new("could not find a post with id #{id}") unless post
raise Diaspora::NonPublic unless post.public?
end
end
def self.find_non_public_by_guid_or_id_with_user(id, user)
post = user.find_visible_shareable_by_id(Post, id, key: Post.key_sym(id))
post || raise(ActiveRecord::RecordNotFound.new("could not find a post with id #{id}"))
user.find_visible_shareable_by_id(Post, id, key: post_key(id)).tap do |post|
raise ActiveRecord::RecordNotFound.new("could not find a post with id #{id}") unless post
end
end
def self.key_sym(id)
def self.post_key(id)
id.to_s.length <= 8 ? :id : :guid
end
end
......@@ -29,7 +29,6 @@ class PostPresenter
:post_type => @post.post_type,
:image_url => @post.image_url,
:object_url => @post.object_url,
:favorite => @post.favorite,
:nsfw => @post.nsfw,
:author => @post.author.as_api_response(:backbone),
:o_embed_cache => @post.o_embed_cache.try(:as_api_response, :backbone),
......
......@@ -4,7 +4,16 @@ class PostService
def initialize(params)
@id = params[:id]
@user = params[:user]
@oembed = params[:oembed]
@oembed = params[:oembed] || {}
assign_post
end
def assign_post
if user
@post = Post.find_non_public_by_guid_or_id_with_user(id, user)
else
@post = Post.find_public(id)
end
end
def present_json
......@@ -19,21 +28,12 @@ class PostService
OEmbedPresenter.new(post, oembed)
end
def assign_post_and_mark_notifications
assign_post
def mark_user_notifications
mark_corresponding_notifications_read if user
end
def assign_post
if user
@post = Post.find_non_public_by_guid_or_id_with_user(id, user)
else
@post = Post.find_public(id)
end
end
def retract_post
find_user_post
raise Diaspora::NotMine unless user_owns_post?
user.retract(@post)
end
......@@ -41,8 +41,8 @@ class PostService
attr_reader :user, :id, :oembed
def find_user_post
@post = user.posts.find(id)
def user_owns_post?
post.author == user.person
end
def mark_corresponding_notifications_read
......
class RemoveFavoritesFromPosts < ActiveRecord::Migration
def self.up
remove_column :posts, :favorite
end
def self.down
add_column :posts, :favorite, :boolean, default: false
end
end
......@@ -11,7 +11,7 @@
#
# It's strongly recommended that you check this file into your version control system.
ActiveRecord::Schema.define(version: 20150607143809) do
ActiveRecord::Schema.define(version: 20150724152052) do
create_table "account_deletions", force: :cascade do |t|
t.string "diaspora_handle", limit: 255
......@@ -374,7 +374,6 @@ ActiveRecord::Schema.define(version: 20150607143809) do
t.integer "reshares_count", limit: 4, default: 0
t.datetime "interacted_at"
t.string "frame_name", limit: 255
t.boolean "favorite", default: false
t.string "facebook_id", limit: 255
t.string "tweet_id", limit: 255
t.integer "open_graph_cache_id", limit: 4
......
......@@ -2,186 +2,112 @@
# licensed under the Affero General Public License version 3 or later. See
# the COPYRIGHT file.
require 'spec_helper'
require "spec_helper"
describe PostsController, type: :controller do
let!(:post_service_double) { double("post_service") }
describe PostsController, :type => :controller do
before do
aspect = alice.aspects.first
@message = alice.build_post :status_message, :text => "ohai", :to => aspect.id
@message = alice.build_post :status_message, text: "ohai", to: aspect.id
@message.save!
alice.add_to_streams(@message, [aspect])
alice.dispatch_post @message, :to => aspect.id
alice.dispatch_post @message, to: aspect.id
allow(PostService).to receive(:new).and_return(post_service_double)
end
describe '#show' do
context 'user signed in' do
describe "#show" do
before do
expect(post_service_double).to receive(:mark_user_notifications)
allow(post_service_double).to receive(:present_json)
end
context "user signed in" do
before do
sign_in :user, alice
expect(post_service_double).to receive(:post).and_return(@message)
end
it 'succeeds' do
get :show, "id" => @message.id
it "succeeds" do
get :show, id: @message.id
expect(response).to be_success
end
it 'succeeds after removing a mention when closing the mentioned user\'s account' do
user = FactoryGirl.create(:user, :username => "user")
user = FactoryGirl.create(:user, username: "user")
alice.share_with(user.person, alice.aspects.first)
msg = alice.build_post :status_message, text: "Mention @{User ; #{user.diaspora_handle}}", :public => true, :to => 'all'
msg = alice.build_post :status_message,
text: "Mention @{User ; #{user.diaspora_handle}}", public: true, to: "all"
msg.save!
expect(msg.mentioned_people.count).to eq(1)
user.destroy
get :show, "id" => msg.id
get :show, id: msg.id
expect(response).to be_success
end
it 'renders the application layout on mobile' do
get :show, :id => @message.id, :format => :mobile
expect(response).to render_template('layouts/application')
it "renders the application layout on mobile" do
get :show, id: @message.id, format: :mobile
expect(response).to render_template("layouts/application")
end
it 'succeeds on mobile with a reshare' do
get :show, "id" => FactoryGirl.create(:reshare, :author => alice.person).id, :format => :mobile
it "succeeds on mobile with a reshare" do
get :show, id: FactoryGirl.create(:reshare, author: alice.person).id, format: :mobile
expect(response).to be_success
end
it 'marks a corresponding notifications as read' do
FactoryGirl.create(:notification, :recipient => alice, :target => @message, :unread => true)
note = FactoryGirl.create(:notification, :recipient => alice, :target => @message, :unread => true)
expect {
get :show, :id => @message.id
note.reload
}.to change(Notification.where(:unread => true), :count).by(-2)
end
it 'marks a corresponding mention notification as read' do
status_msg = bob.post(:status_message, {text: "this is a text mentioning @{Mention User ; #{alice.diaspora_handle}} ... have fun testing!", :public => true, :to => 'all'})
mention = status_msg.mentions.where(person_id: alice.person.id).first
note = FactoryGirl.create(:notification, :recipient => alice, :target_type => "Mention", :target_id => mention.id, :unread => true)
expect {
get :show, :id => status_msg.id
note.reload
}.to change(Notification.where(:unread => true), :count).by(-1)
end
it '404 if the post is missing' do
expect {
get :show, :id => 1234567
}.to raise_error(ActiveRecord::RecordNotFound)
end
end
context 'user not signed in' do
context 'given a public post' do
context "user not signed in" do
context "given a public post" do
before :each do
@status = alice.post(:status_message, :text => "hello", :public => true, :to => 'all')
@status = alice.post(:status_message, text: "hello", public: true, to: "all")
expect(post_service_double).to receive(:post).and_return(@status)
end
it 'shows a public post' do
get :show, :id => @status.id
expect(response.status).to eq(200)
it "shows a public post" do
get :show, id: @status.id
expect(response.body).to match "hello"
end
it 'succeeds for statusnet' do
it "succeeds for statusnet" do
@request.env["HTTP_ACCEPT"] = "application/html+xml,text/html"
get :show, :id => @status.id
expect(response).to be_success
get :show, id: @status.id
expect(response.body).to match "hello"
end
it 'responds with diaspora xml if format is xml' do
get :show, :id => @status.guid, :format => :xml
it "responds with diaspora xml if format is xml" do
get :show, id: @status.guid, format: :xml
expect(response.body).to eq(@status.to_diaspora_xml)
end
end
it 'does not show a private post' do
status = alice.post(:status_message, :text => "hello", :public => false, :to => 'all')
get :show, :id => status.id
expect(response.status).to eq(404)
end
# We want to be using guids from now on for this post route, but do not want to break
# pre-exisiting permalinks. We can assume a guid is 8 characters long as we have
# guids set to hex(8) since we started using them.
context 'id/guid switch' do
before do
@status = alice.post(:status_message, :text => "hello", :public => true, :to => 'all')
end
it 'assumes guids less than 8 chars are ids and not guids' do
p = Post.where(:id => @status.id.to_s)
expect(Post).to receive(:where)
.with(hash_including(:id => @status.id.to_s))
.and_return(p)
get :show, :id => @status.id
expect(response).to be_success
end
it 'assumes guids more than (or equal to) 8 chars are actually guids' do
p = Post.where(:guid => @status.guid)
expect(Post).to receive(:where)
.with(hash_including(:guid => @status.guid))
.and_return(p)
get :show, :id => @status.guid
expect(response).to be_success
end
end
end
end
describe 'iframe' do
it 'contains an iframe' do
get :iframe, :id => @message.id
describe "iframe" do
it "contains an iframe" do
get :iframe, id: @message.id
expect(response.body).to match /iframe/
end
end
describe 'oembed' do
it 'works when you can see it' do
sign_in alice
get :oembed, :url => "/posts/#{@message.id}"
expect(response.body).to match /iframe/
end
it 'returns a 404 response when the post is not found' do
get :oembed, :url => "/posts/#{@message.id}"
expect(response.status).to eq(404)
describe "oembed" do
it "receives a present oembed" do
expect(post_service_double).to receive(:present_oembed)
get :oembed, url: "/posts/#{@message.id}"
end
end
describe '#destroy' do
describe "#destroy" do
before do
sign_in alice
end
it 'let a user delete his message' do
message = alice.post(:status_message, :text => "hey", :to => alice.aspects.first.id)
delete :destroy, :format => :js, :id => message.id
expect(response).to be_success
expect(StatusMessage.find_by_id(message.id)).to be_nil
end
it 'sends a retraction on delete' do
allow(controller).to receive(:current_user).and_return alice
message = alice.post(:status_message, :text => "hey", :to => alice.aspects.first.id)
expect(alice).to receive(:retract).with(message)
delete :destroy, :format => :js, :id => message.id
expect(response).to be_success
end
it 'will not let you destroy posts visible to you' do
message = bob.post(:status_message, :text => "hey", :to => bob.aspects.first.id)
expect { delete :destroy, :format => :js, :id => message.id }.to raise_error(ActiveRecord::RecordNotFound)
expect(StatusMessage.exists?(message.id)).to be true
end
it 'will not let you destory posts you do not own' do
message = eve.post(:status_message, :text => "hey", :to => eve.aspects.first.id)
expect { delete :destroy, :format => :js, :id => message.id }.to raise_error(ActiveRecord::RecordNotFound)
expect(StatusMessage.exists?(message.id)).to be true
it "will receive a retract post" do
expect(post_service_double).to receive(:retract_post)
expect(post_service_double).to receive(:post).and_return(@message)
message = alice.post(:status_message, text: "hey", to: alice.aspects.first.id)
delete :destroy, format: :js, id: message.id
end
end
end
......@@ -400,43 +400,55 @@ describe Post, :type => :model do
end
end
describe "#find_by_guid_or_id_with_user" do
describe "#find_public" do
it "succeeds with an id" do
post = FactoryGirl.create :status_message, public: true
expect(Post.find_by_guid_or_id_with_user(post.id)).to eq(post)
expect(Post.find_public post.id).to eq(post)
end
it "succeeds with an guid" do
post = FactoryGirl.create :status_message, public: true
expect(Post.find_by_guid_or_id_with_user(post.guid)).to eq(post)
expect(Post.find_public post.guid).to eq(post)
end
it "looks up on the passed user object if it's non-nil" do
post = FactoryGirl.create :status_message
user = double
expect(user).to receive(:find_visible_shareable_by_id).with(Post, post.id, key: :id).and_return(post)
Post.find_by_guid_or_id_with_user post.id, user
end
it "raises ActiveRecord::RecordNotFound with a non-existing id and a user" do
user = double(find_visible_shareable_by_id: nil)
it "raises ActiveRecord::RecordNotFound for a non-existing id without a user" do
allow(Post).to receive_messages where: double(includes: double(first: nil))
expect {
Post.find_by_guid_or_id_with_user 123, user
Post.find_public 123
}.to raise_error ActiveRecord::RecordNotFound
end
it "raises Diaspora::NonPublic for a non-existing id without a user" do
allow(Post).to receive_messages where: double(includes: double(first: nil))
it "raises Diaspora::NonPublic for a private post without a user" do
post = FactoryGirl.create :status_message
expect {
Post.find_by_guid_or_id_with_user 123
Post.find_public post.id
}.to raise_error Diaspora::NonPublic
end
end
it "raises Diaspora::NonPublic for a private post without a user" do
describe "#find_non_public_by_guid_or_id_with_user" do
it "succeeds with an id" do
post = FactoryGirl.create :status_message_in_aspect
expect(Post.find_non_public_by_guid_or_id_with_user(post.id, post.author.owner)).to eq(post)
end
it "succeeds with an guid" do
post = FactoryGirl.create :status_message_in_aspect
expect(Post.find_non_public_by_guid_or_id_with_user(post.guid, post.author.owner)).to eq(post)
end
it "looks up on the passed user object if it's non-nil" do
post = FactoryGirl.create :status_message
user = double
expect(user).to receive(:find_visible_shareable_by_id).with(Post, post.id, key: :id).and_return(post)
Post.find_non_public_by_guid_or_id_with_user(post.id, user)
end
it "raises ActiveRecord::RecordNotFound with a non-existing id and a user" do
user = double(find_visible_shareable_by_id: nil)
expect {
Post.find_by_guid_or_id_with_user post.id
}.to raise_error Diaspora::NonPublic
Post.find_non_public_by_guid_or_id_with_user(123, user)
}.to raise_error ActiveRecord::RecordNotFound
end
end
end
require "spec_helper"
describe PostService do
before do
aspect = alice.aspects.first
@message = alice.build_post :status_message, text: "ohai", to: aspect.id
@message.save!
alice.add_to_streams(@message, [aspect])
alice.dispatch_post @message, to: aspect.id
end
describe "#assign_post" do
context "when the post is private" do
it "RecordNotFound if the post cannot be found" do
expect { PostService.new(id: 1_234_567, user: alice) }.to raise_error(ActiveRecord::RecordNotFound)
end
it "NonPublic if there is no user" do
expect { PostService.new(id: @message.id) }.to raise_error(Diaspora::NonPublic)
end
it "RecordNotFound if user cannot see post" do
expect { PostService.new(id: @message.id, user: eve) }.to raise_error(ActiveRecord::RecordNotFound)
end
end
context "when the post is public" do
it "RecordNotFound if the post cannot be found" do
expect { PostService.new(id: 1_234_567) }.to raise_error(ActiveRecord::RecordNotFound)
end
end
# We want to be using guids from now on for this post route, but do not want to break
# pre-exisiting permalinks. We can assume a guid is 8 characters long as we have
# guids set to hex(8) since we started using them.
context "id/guid switch" do
before do
@status = alice.post(:status_message, text: "hello", public: true, to: "all")
end
it "assumes guids less than 8 chars are ids and not guids" do
post = Post.where(id: @status.id.to_s)
expect(Post).to receive(:where).with(hash_including(id: @status.id)).and_return(post).at_least(:once)
PostService.new(id: @status.id, user: alice)
end
it "assumes guids more than (or equal to) 8 chars are actually guids" do
post = Post.where(guid: @status.guid)
expect(Post).to receive(:where).with(hash_including(guid: @status.guid)).and_return(post).at_least(:once)
PostService.new(id: @status.guid, user: alice)
end
end
end
describe "#mark_user_notifications" do
it "marks a corresponding notifications as read" do
FactoryGirl.create(:notification, recipient: alice, target: @message, unread: true)
FactoryGirl.create(:notification, recipient: alice, target: @message, unread: true)
post_service = PostService.new(id: @message.id, user: alice)
expect { post_service.mark_user_notifications }.to change(Notification.where(unread: true), :count).by(-2)
end
it "marks a corresponding mention notification as read" do
status_text = "this is a text mentioning @{Mention User ; #{alice.diaspora_handle}} ... have fun testing!"
status_msg =
bob.post(:status_message, text: status_text, public: true, to: "all")
mention = status_msg.mentions.where(person_id: alice.person.id).first
FactoryGirl.create(:notification, recipient: alice, target_type: "Mention", target_id: mention.id, unread: true)
post_service = PostService.new(id: status_msg.id, user: alice)
expect { post_service.mark_user_notifications }.to change(Notification.where(unread: true), :count).by(-1)
end
end
describe "#present_json" do
it "works for a private post" do
post_service = PostService.new(id: @message.id, user: alice)
expect(post_service.present_json.to_json).to match(/\"text\"\:\"ohai\"/)
end
it "works for a public post " do
status = alice.post(:status_message, text: "hello", public: true, to: "all")
post_service = PostService.new(id: status.id)
expect(post_service.present_json.to_json).to match(/\"text\"\:\"hello\"/)
end
end
describe "#present_oembed" do
it "works for a private post" do
post_service = PostService.new(id: @message.id, user: alice)
expect(post_service.present_oembed.to_json).to match(/iframe/)
end
it "works for a public post" do
status = alice.post(:status_message, text: "hello", public: true, to: "all")
post_service = PostService.new(id: status.id)
expect(post_service.present_oembed.to_json).to match(/iframe/)
end
end
describe "#retract_post" do
it "let a user delete his message" do
message = alice.post(:status_message, text: "hey", to: alice.aspects.first.id)
post_service = PostService.new(id: message.id, user: alice)
post_service.retract_post
expect(StatusMessage.find_by_id(message.id)).to be_nil
end
it "sends a retraction on delete" do
message = alice.post(:status_message, text: "hey", to: alice.aspects.first.id)
post_service = PostService.new(id: message.id, user: alice)
expect(alice).to receive(:retract).with(message)
post_service.retract_post
end