nocebo/diaspora
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

refactoring PostService

Browse source 
* move presenters back to controllers, this is view-logic
* use PostService in CommentService
* remove iframe route, this is not used anymore
* id/guid limit at 16 chars, hex(8) is 16 chars long
This commit is contained in:
Benjamin Neff 2016-03-06 15:26:05 +01:00
parent d872c64369
commit d94eae0d45
13 changed files with 208 additions and 299 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

42
app/controllers/posts_controller.rb
View file

@ -3,8 +3,6 @@
# the COPYRIGHT file.
class PostsController < ApplicationController
include PostsHelper
before_action :authenticate_user!, only: :destroy
before_action :set_format_if_malformed_from_status_net, only: :show
@ -26,38 +24,36 @@ class PostsController < ApplicationController
end
def show
post_service = PostService.new(id: params[:id], user: current_user)
post_service.mark_user_notifications
@post = post_service.post
post = post_service.find(params[:id])
post_service.mark_user_notifications(post.id)
respond_to do |format|
format.html { gon.post = post_service.present_json }
format.xml { render xml: @post.to_diaspora_xml }
format.json { render json: post_service.present_json }
format.html {
gon.post = PostPresenter.new(post, current_user)
render locals: {post: post}
}
format.mobile { render locals: {post: post} }
format.xml { render xml: post.to_diaspora_xml }
format.json { render json: PostPresenter.new(post, current_user) }
end
end
def iframe
render text: post_iframe_url(params[:id]), layout: false
end
def oembed
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))
render json: post_service.present_oembed
post = post_service.find(post_id)
oembed = params.slice(:format, :maxheight, :minheight)
render json: OEmbedPresenter.new(post, oembed)
rescue
render nothing: true, status: 404
end
def interactions
post_service = PostService.new(id: params[:id], user: current_user)
respond_with post_service.present_interactions_json
post = post_service.find(params[:id])
respond_with PostInteractionPresenter.new(post, current_user)
end
def destroy
post_service = PostService.new(id: params[:id], user: current_user)
post_service.retract_post
@post = post_service.post
post_service.destroy(params[:id])
respond_to do |format|
format.js { render "destroy", layout: false, format: :js }
format.json { render nothing: true, status: 204 }
format.any { redirect_to stream_path }
end
@ -65,6 +61,10 @@ class PostsController < ApplicationController
private
def post_service
@post_service ||= PostService.new(current_user)
end
def set_format_if_malformed_from_status_net
request.format = :html if request.format == "application/html+xml"
end

17
app/models/post.rb
View file

@ -155,21 +155,4 @@ class Post < ActiveRecord::Base
def nsfw
self.author.profile.nsfw?
end
def self.find_public(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)
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.post_key(id)
id.to_s.length <= 8 ? :id : :guid
end
end

18
app/services/comment_service.rb
View file

@ -4,7 +4,7 @@ class CommentService
end
def create(post_id, text)
post = find_post!(post_id)
post = post_service.find(post_id)
user.comment!(post, text)
end
@ -19,24 +19,14 @@ class CommentService
end
def find_for_post(post_id)
find_post!(post_id).comments.for_a_stream
post_service.find(post_id).comments.for_a_stream
end
private
attr_reader :user
def find_post!(post_id)
find_post(post_id).tap do |post|
raise ActiveRecord::RecordNotFound unless post
end
end
def find_post(post_id)
if user
user.find_visible_shareable_by_id(Post, post_id)
else
Post.find_by_id_and_public(post_id, true)
end
def post_service
@post_service ||= PostService.new(user)
end
end

71
app/services/post_service.rb
View file

@ -1,64 +1,59 @@
class PostService
attr_reader :post
def initialize(params)
@id = params[:id]
@user = params[:user]
@oembed = params[:oembed] || {}
assign_post
def initialize(user=nil)
@user = user
end
def assign_post
def find(id_or_guid)
if user
@post = Post.find_non_public_by_guid_or_id_with_user(id, user)
find_non_public_by_guid_or_id_with_user(id_or_guid)
else
@post = Post.find_public(id)
find_public(id_or_guid)
end
end
def present_json
PostPresenter.new(post, user)
def mark_user_notifications(post_id)
return unless user
mark_comment_reshare_like_notifications_read(post_id)
mark_mention_notifications_read(post_id)
end
def present_interactions_json
PostInteractionPresenter.new(post, user)
end
def present_oembed
OEmbedPresenter.new(post, oembed)
end
def mark_user_notifications
mark_corresponding_notifications_read if user
end
def retract_post
raise Diaspora::NotMine unless user_owns_post?
user.retract(@post)
def destroy(post_id)
post = find(post_id)
raise Diaspora::NotMine unless post.author == user.person
user.retract(post)
end
private
attr_reader :user, :id, :oembed
attr_reader :user
def user_owns_post?
post.author == user.person
def find_public(id_or_guid)
Post.where(post_key(id_or_guid) => id_or_guid).first.tap do |post|
raise ActiveRecord::RecordNotFound, "could not find a post with id #{id_or_guid}" unless post
raise Diaspora::NonPublic unless post.public?
end
end
def mark_corresponding_notifications_read
mark_comment_reshare_like_notifications_read
mark_mention_notifications_read
def find_non_public_by_guid_or_id_with_user(id_or_guid)
user.find_visible_shareable_by_id(Post, id_or_guid, key: post_key(id_or_guid)).tap do |post|
raise ActiveRecord::RecordNotFound, "could not find a post with id #{id_or_guid} for user #{user.id}" unless post
end
end
def mark_comment_reshare_like_notifications_read
notification = Notification.where(recipient_id: user.id, target_type: "Post", target_id: post.id, unread: true)
notification.each do |notification|
# We can assume a guid is at least 16 characters long as we have guids set to hex(8) since we started using them.
def post_key(id_or_guid)
id_or_guid.to_s.length < 16 ? :id : :guid
end
def mark_comment_reshare_like_notifications_read(post_id)
notifications = Notification.where(recipient_id: user.id, target_type: "Post", target_id: post_id, unread: true)
notifications.each do |notification|
notification.set_read_state(true)
end
end
def mark_mention_notifications_read
mention = post.mentions.where(person_id: user.person_id).first
def mark_mention_notifications_read(post_id)
mention = find(post_id).mentions.where(person_id: user.person_id).first
Notification.where(recipient_id: user.id, target_type: "Mention", target_id: mention.id, unread: true)
.first.try(:set_read_state, true) if mention
end

2
app/views/posts/destroy.js.erb
View file

@ -1,2 +0,0 @@
var target = $("#<%= @post.guid %>")
target.hide('blind', { direction: 'vertical' }, 300, function(){ target.remove() });

2
app/views/posts/show.html.haml
View file

@ -3,7 +3,7 @@
-# the COPYRIGHT file.
- content_for :page_title do
= post_page_title @post
= post_page_title post
- content_for :content do
#container.container-fluid

4
app/views/posts/show.mobile.haml
View file

@ -3,6 +3,6 @@
-# the COPYRIGHT file.
.stream
= render :partial => 'shared/stream_element',
:locals => {:post => @post, :commenting_disabled => commenting_disabled?(@post), :expanded_info => true}
= render partial: "shared/stream_element",
locals: {post: post, commenting_disabled: commenting_disabled?(post), expanded_info: true}

2
app/views/shared/_stream_element.mobile.haml
View file

@ -26,7 +26,7 @@
!= reactions_link(post, "active")
.comment-container
%ul.comments
= render partial: "comments/comment", collection: @post.comments.for_a_stream, locals: {post: @post}
= render partial: "comments/comment", collection: post.comments.for_a_stream, locals: {post: post}
- else
!= reactions_link(post)

3
config/routes.rb
View file

@ -40,10 +40,7 @@ Diaspora::Application.routes.draw do
resources :comments, only: %i(new create destroy index)
end
get 'p/:id' => 'posts#show', :as => 'short_post'
get 'posts/:id/iframe' => 'posts#iframe', :as => 'iframe'
# roll up likes into a nested resource above
resources :comments, :only => [:create, :destroy] do

123
spec/controllers/posts_controller_spec.rb
View file

@ -5,104 +5,86 @@
require "spec_helper"
describe PostsController, type: :controller do
let!(:post_service_double) { double("post_service") }
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
allow(PostService).to receive(:new).and_return(post_service_double)
end
let(:post) { alice.post(:status_message, text: "ohai", to: alice.aspects.first) }
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
context "given a post that the user is allowed to see" 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
expect_any_instance_of(PostService).to receive(:mark_user_notifications).with(post.id)
get :show, id: post.id
expect(response).to be_success
end
it 'succeeds after removing a mention when closing the mentioned user\'s account' do
it "succeeds after removing a mention when closing the mentioned user's account" do
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.save!
msg = alice.post(:status_message, text: "Mention @{User ; #{user.diaspora_handle}}", public: true)
expect(msg.mentioned_people.count).to eq(1)
user.destroy
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
get :show, id: post.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
reshare_id = FactoryGirl.create(:reshare, author: alice.person).id
expect_any_instance_of(PostService).to receive(:mark_user_notifications).with(reshare_id)
get :show, id: reshare_id, format: :mobile
expect(response).to be_success
end
end
context "given a post that the user is not allowed to see" do
before do
sign_in :user, alice
expect(post_service_double).to receive(:post).and_raise(Diaspora::NonPublic)
sign_in :user, eve
end
it "returns a 404" do
get :show, id: @message.id
expect(response.code).to eq("404")
expect {
get :show, id: post.id
}.to raise_error ActiveRecord::RecordNotFound
end
end
end
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")
expect(post_service_double).to receive(:post).and_return(@status)
end
let(:public) { alice.post(:status_message, text: "hello", public: true) }
it "shows a public post" do
get :show, id: @status.id
get :show, id: public.id
expect(response.body).to match "hello"
end
it "succeeds for statusnet" do
@request.env["HTTP_ACCEPT"] = "application/html+xml,text/html"
get :show, id: @status.id
get :show, id: public.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
expect(response.body).to eq(@status.to_diaspora_xml)
get :show, id: public.guid, format: :xml
expect(response.body).to eq(public.to_diaspora_xml)
end
end
context "given a limited post" do
before do
expect(post_service_double).to receive(:post).and_raise(Diaspora::NonPublic)
end
it "forces the user to sign" do
get :show, id: @message.id
get :show, id: post.id
expect(response).to be_redirect
expect(response).to redirect_to new_user_session_path
end
@ -110,40 +92,55 @@ describe PostsController, type: :controller do
end
end
describe "iframe" do
it "contains an iframe" do
get :iframe, id: @message.id
describe "oembed" do
it "works when you can see it" do
sign_in alice
get :oembed, url: "/posts/#{post.id}"
expect(response.body).to match /iframe/
end
end
describe "oembed" do
it "receives a present oembed" do
expect(post_service_double).to receive(:present_oembed)
get :oembed, url: "/posts/#{@message.id}"
it "returns a 404 response when the post is not found" do
get :oembed, url: "/posts/#{post.id}"
expect(response.status).to eq(404)
end
end
describe "#destroy" do
before do
sign_in alice
context "own post" do
before do
sign_in alice
end
it "works when it is your post" do
expect_any_instance_of(PostService).to receive(:destroy).with(post.id.to_s)
delete :destroy, format: :json, id: post.id
expect(response.status).to eq(204)
end
it "redirects to stream on mobile" do
delete :destroy, format: :mobile, id: post.id
expect(response).to be_redirect
expect(response).to redirect_to stream_path
end
end
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
context "when Diaspora::NotMine is raised by retract post" do
context "post of another user" do
it "will respond with a 403" do
expect(post_service_double).to receive(:retract_post).and_raise(Diaspora::NotMine)
message = alice.post(:status_message, text: "hey", to: alice.aspects.first.id)
delete :destroy, format: :js, id: message.id
sign_in :user, bob
delete :destroy, format: :json, id: post.id
expect(response.body).to eq("You are not allowed to do that")
expect(response.status).to eq(403)
end
it "will respond with a 404 if the post is not visible" do
sign_in :user, eve
expect {
delete :destroy, format: :json, id: post.id
}.to raise_error ActiveRecord::RecordNotFound
end
end
end
end

52
spec/models/post_spec.rb
View file

@ -415,56 +415,4 @@ describe Post, :type => :model do
expect(post.interacted_at).not_to be_blank
end
end
describe "#find_public" do
it "succeeds with an id" do
post = FactoryGirl.create :status_message, public: true
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_public post.guid).to eq(post)
end
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_public 123
}.to raise_error ActiveRecord::RecordNotFound
end
it "raises Diaspora::NonPublic for a private post without a user" do
post = FactoryGirl.create :status_message
expect {
Post.find_public post.id
}.to raise_error Diaspora::NonPublic
end
end
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_non_public_by_guid_or_id_with_user(123, user)
}.to raise_error ActiveRecord::RecordNotFound
end
end
end

2
spec/services/comment_service_spec.rb
View file

@ -88,7 +88,7 @@ describe CommentService do
it "does not return comments for private post" do
expect {
CommentService.new.find_for_post(post.id)
}.to raise_error ActiveRecord::RecordNotFound
}.to raise_error Diaspora::NonPublic
end
end

169
spec/services/post_service_spec.rb
View file

@ -1,127 +1,128 @@
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!
let(:post) { alice.post(:status_message, text: "ohai", to: alice.aspects.first) }
let(:public) { alice.post(:status_message, text: "hey", public: true) }
alice.add_to_streams(@message, [aspect])
alice.dispatch_post @message, to: aspect.id
end
describe "#find" do
context "with user" do
it "returns the post, if it is the users post" do
expect(PostService.new(alice).find(post.id)).to eq(post)
end
it "works with guid" do
expect(PostService.new(alice).find(post.guid)).to eq(post)
end
it "returns the post, if the user can see the it" do
expect(PostService.new(bob).find(post.id)).to eq(post)
end
it "returns the post, if it is public" do
expect(PostService.new(eve).find(public.id)).to eq(public)
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)
expect {
PostService.new(alice).find("unknown")
}.to raise_error ActiveRecord::RecordNotFound, "could not find a post with id unknown for user #{alice.id}"
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)
it "RecordNotFound if user cannot see the post" do
expect {
PostService.new(eve).find(post.id)
}.to raise_error ActiveRecord::RecordNotFound, "could not find a post with id #{post.id} for user #{eve.id}"
end
end
context "when the post is public" do
context "without user" do
it "returns the post, if it is public" do
expect(PostService.new.find(public.id)).to eq(public)
end
it "works with guid" do
expect(PostService.new.find(public.guid)).to eq(public)
end
it "NonPublic if the post is private" do
expect {
PostService.new.find(post.id)
}.to raise_error Diaspora::NonPublic
end
it "RecordNotFound if the post cannot be found" do
expect { PostService.new(id: 1_234_567) }.to raise_error(ActiveRecord::RecordNotFound)
expect {
PostService.new.find("unknown")
}.to raise_error ActiveRecord::RecordNotFound, "could not find a post with id unknown"
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")
let(:public) { alice.post(:status_message, text: "ohai", public: true) }
it "assumes ids less than 16 chars are ids and not guids" do
post = Post.where(id: public.id)
expect(Post).to receive(:where).with(hash_including(id: "123456789012345")).and_return(post).at_least(:once)
PostService.new(alice).find("123456789012345")
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)
it "assumes ids more than (or equal to) 16 chars are actually guids" do
post = Post.where(guid: public.guid)
expect(Post).to receive(:where).with(hash_including(guid: "1234567890123456")).and_return(post).at_least(:once)
PostService.new(alice).find("1234567890123456")
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)
FactoryGirl.create(:notification, recipient: alice, target: post, unread: true)
FactoryGirl.create(:notification, recipient: alice, target: post, unread: true)
expect {
PostService.new(alice).mark_user_notifications(post.id)
}.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)
mention_post = bob.post(:status_message, text: status_text, public: true)
expect {
PostService.new(alice).mark_user_notifications(mention_post.id)
}.to change(Notification.where(unread: true), :count).by(-1)
end
it "does nothing without a user" do
expect_any_instance_of(PostService).not_to receive(:mark_comment_reshare_like_notifications_read).with(post.id)
expect_any_instance_of(PostService).not_to receive(:mark_mention_notifications_read).with(post.id)
PostService.new.mark_user_notifications(post.id)
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
describe "#destroy" 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
PostService.new(alice).destroy(post.id)
expect(StatusMessage.find_by_id(post.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
expect(alice).to receive(:retract).with(post)
PostService.new(alice).destroy(post.id)
end
it "will not let you destroy posts visible to you but that you do not own" do
message = bob.post(:status_message, text: "hey", to: bob.aspects.first.id)
post_service = PostService.new(id: message.id, user: alice)
expect { post_service.retract_post }.to raise_error(Diaspora::NotMine)
expect(StatusMessage.exists?(message.id)).to be true
expect {
PostService.new(bob).destroy(post.id)
}.to raise_error Diaspora::NotMine
expect(StatusMessage.find_by_id(post.id)).not_to be_nil
end
it "will not let you destroy posts that are not visible to you" do
message = eve.post(:status_message, text: "hey", to: eve.aspects.first.id)
expect { PostService.new(id: message.id, user: alice) }.to raise_error(ActiveRecord::RecordNotFound)
expect(StatusMessage.exists?(message.id)).to be true
expect {
PostService.new(eve).destroy(post.id)
}.to raise_error(ActiveRecord::RecordNotFound)
expect(StatusMessage.find_by_id(post.id)).not_to be_nil
end
end
end