From d9ff4d576563fd1e5b694ad4940de082a2a6673e Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Sun, 23 Jan 2022 19:25:56 +0100 Subject: [PATCH] Refactor Ajax::QuestionController#create into separate UseCases Co-authored-by: Georg Gadinger --- app/controllers/ajax/question_controller.rb | 80 ++++++------------ app/models/anonymous_block.rb | 2 + app/workers/question_worker.rb | 2 +- lib/use_case/question/create.rb | 82 +++++++++++++++++++ lib/use_case/question/create_followers.rb | 40 +++++++++ .../ajax/question_controller_spec.rb | 82 ++++++++++++++----- spec/workers/question_worker_spec.rb | 4 +- 7 files changed, 213 insertions(+), 79 deletions(-) create mode 100644 lib/use_case/question/create.rb create mode 100644 lib/use_case/question/create_followers.rb diff --git a/app/controllers/ajax/question_controller.rb b/app/controllers/ajax/question_controller.rb index 484480de..be7e54cb 100644 --- a/app/controllers/ajax/question_controller.rb +++ b/app/controllers/ajax/question_controller.rb @@ -2,6 +2,8 @@ require "digest" require "errors" +require "use_case/question/create" +require "use_case/question/create_followers" class Ajax::QuestionController < AjaxController def destroy @@ -14,7 +16,7 @@ class Ajax::QuestionController < AjaxController return end - if not (current_user.mod? or question.user == current_user) + unless current_user&.mod? || question.user == current_user @response[:status] = :not_authorized @response[:message] = t(".noauth") return @@ -32,65 +34,29 @@ class Ajax::QuestionController < AjaxController params.require :anonymousQuestion params.require :rcpt - is_never_anonymous = user_signed_in? && params[:rcpt] == 'followers' - author_is_anonymous = is_never_anonymous ? false : params[:anonymousQuestion] + # set up fake success response -- the use cases raise errors on exceptions + # which get rescued by the base class + @response = { + success: true, + message: t(".success"), + status: :okay + } - begin - question = Question.create!(content: params[:question], - author_is_anonymous: author_is_anonymous, - author_identifier: AnonymousBlock.get_identifier(request.ip), - user: current_user, - direct: params[:rcpt] != "followers") - rescue ActiveRecord::RecordInvalid => e - Sentry.capture_exception(e) - @response[:status] = :rec_inv - @response[:message] = t(".invalid") + if user_signed_in? && params[:rcpt] == "followers" + UseCase::Question::CreateFollowers.call( + source_user_id: current_user.id, + content: params[:question], + author_identifier: AnonymousBlock.get_identifier(request.ip) + ) return end - if !user_signed_in? && !question.author_is_anonymous - question.delete - return - end - - unless current_user.nil? - current_user.increment! :asked_count unless params[:anonymousQuestion] == 'true' - end - - if params[:rcpt] == 'followers' - QuestionWorker.perform_async(current_user.id, question.id) unless current_user.nil? - else - target_user = User.find_by(id: params[:rcpt]) - - if target_user.nil? - @response[:status] = :not_found - @response[:message] = t(".notfound") - question.delete - return - end - - if target_user.blocking?(current_user) - question.delete - raise Errors::AskingOtherBlockedSelf - end - if current_user&.blocking?(target_user) - question.delete - raise Errors::AskingSelfBlockedOther - end - - if !target_user.privacy_allow_anonymous_questions && question.author_is_anonymous - question.delete - return - end - - unless target_user.mute_rules.any? { |rule| rule.applies_to? question } || - (author_is_anonymous && target_user.anonymous_blocks.where(identifier: AnonymousBlock.get_identifier(request.ip)).any?) - Inbox.create!(user_id: target_user.id, question_id: question.id, new: true) - end - end - - @response[:status] = :okay - @response[:message] = t(".success") - @response[:success] = true + UseCase::Question::Create.call( + source_user_id: user_signed_in? ? current_user.id : nil, + target_user_id: params[:rcpt], + content: params[:question], + anonymous: params[:anonymousQuestion], + author_identifier: AnonymousBlock.get_identifier(request.ip) + ) end end diff --git a/app/models/anonymous_block.rb b/app/models/anonymous_block.rb index 087b7b8c..f5213979 100644 --- a/app/models/anonymous_block.rb +++ b/app/models/anonymous_block.rb @@ -1,5 +1,7 @@ # frozen_string_literal: true +require "digest" + class AnonymousBlock < ApplicationRecord belongs_to :user belongs_to :question, optional: true diff --git a/app/workers/question_worker.rb b/app/workers/question_worker.rb index cd054e6b..cb13edf6 100644 --- a/app/workers/question_worker.rb +++ b/app/workers/question_worker.rb @@ -12,7 +12,7 @@ class QuestionWorker question = Question.find(question_id) user.followers.each do |f| - unless MuteRule.where(user: f).any? { |rule| rule.applies_to? question } + if MuteRule.where(user: f).none? { |rule| rule.applies_to? question } Inbox.create(user_id: f.id, question_id: question_id, new: true) end end diff --git a/lib/use_case/question/create.rb b/lib/use_case/question/create.rb new file mode 100644 index 00000000..2d878e58 --- /dev/null +++ b/lib/use_case/question/create.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +require "use_case/base" + +module UseCase + module Question + class Create < UseCase::Base + option :source_user_id, type: Types::Coercible::Integer | Types::Nil, optional: true + option :target_user_id, type: Types::Coercible::Integer + option :content, type: Types::Coercible::String + option :anonymous, type: Types::Params::Bool, default: proc { false } + option :author_identifier, type: Types::Coercible::String | Types::Nil + + def call + check_anonymous_rules + check_blocks + + question = ::Question.create!( + content: content, + author_is_anonymous: anonymous, + author_identifier: author_identifier, + user: source_user_id.nil? ? nil : source_user + ) + + increment_asked_count + + return if filtered?(question) + + inbox = ::Inbox.create!(user: target_user, question: question, new: true) + + { + question: question, + inbox: inbox + } + end + + private + + def check_anonymous_rules + if !source_user_id && !anonymous + # We can not create a non-anonymous question without a source user + raise Errors::BadRequest.new("anonymous must be set to true") + end + + if !target_user.privacy_allow_anonymous_questions && anonymous + # The target user does not want questions from strangers + raise Errors::Forbidden.new("no anonymous questions allowed") + end + end + + def check_blocks + if source_user_id.present? + raise Errors::AskingOtherBlockedSelf if target_user.blocking?(source_user) + raise Errors::AskingSelfBlockedOther if source_user.blocking?(target_user) + end + end + + def increment_asked_count + unless source_user_id && !anonymous + # Only increment the asked count of the source user if the question + # is not anonymous, and we actually have a source user + return + end + + source_user.increment!(:asked_count) + end + + def filtered?(question) + target_user.mute_rules.any? { |rule| rule.applies_to? question } || + (anonymous && target_user.anonymous_blocks.where(identifier: question.author_identifier).any?) + end + + def source_user + @source_user ||= ::User.find(source_user_id) + end + + def target_user + @target_user ||= ::User.find(target_user_id) + end + end + end +end diff --git a/lib/use_case/question/create_followers.rb b/lib/use_case/question/create_followers.rb new file mode 100644 index 00000000..08385b4f --- /dev/null +++ b/lib/use_case/question/create_followers.rb @@ -0,0 +1,40 @@ +# frozen_string_literal: true + +require "use_case/base" + +module UseCase + module Question + class CreateFollowers < UseCase::Base + option :source_user_id, type: Types::Coercible::Integer + option :content, type: Types::Coercible::String + option :author_identifier, type: Types::Coercible::String | Types::Nil + + def call + question = ::Question.create!( + content: content, + author_is_anonymous: false, + author_identifier: author_identifier, + user: source_user + ) + + increment_asked_count + + QuestionWorker.perform_async(source_user_id, question.id) + + { + question: question + } + end + + private + + def increment_asked_count + source_user.increment!(:asked_count) + end + + def source_user + @source_user ||= ::User.find(source_user_id) + end + end + end +end diff --git a/spec/controllers/ajax/question_controller_spec.rb b/spec/controllers/ajax/question_controller_spec.rb index bf79958c..047e2f8e 100644 --- a/spec/controllers/ajax/question_controller_spec.rb +++ b/spec/controllers/ajax/question_controller_spec.rb @@ -10,7 +10,6 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do expect(Question.last.content).to eq(question_content) expect(Question.last.author_is_anonymous).to be(expected_question_anonymous) expect(Question.last.user).to eq(expected_question_user) - expect(Question.last.direct).to eq(expected_question_direct) end if check_for_inbox @@ -72,9 +71,9 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do let(:target_user) { FactoryBot.create(:user, privacy_allow_anonymous_questions: user_allows_anonymous_questions) } let(:params) do { - question: question_content, + question: question_content, anonymousQuestion: anonymous_question, - rcpt: rcpt + rcpt: rcpt } end @@ -85,7 +84,7 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do let(:expected_response) do { "success" => true, - "status" => "okay", + "status" => "okay", "message" => anything } end @@ -98,7 +97,6 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do context "when user allows anonymous questions" do let(:user_allows_anonymous_questions) { true } - let(:expected_question_direct) { true } context "when anonymousQuestion is true" do let(:anonymous_question) { "true" } @@ -123,7 +121,7 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do let(:expected_response) do { "success" => false, - "status" => "unknown", + "status" => "forbidden", "message" => anything } end @@ -134,7 +132,6 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do context "when anonymousQuestion is false" do let(:anonymous_question) { "false" } let(:expected_question_anonymous) { false } - let(:expected_question_direct) { true } include_examples "creates the question" end @@ -179,7 +176,6 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do context "when rcpt is followers" do let(:rcpt) { "followers" } - let(:expected_question_direct) { false } context "when anonymousQuestion is true" do let(:anonymous_question) { "true" } @@ -198,13 +194,29 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do end end - context "when rcpt is a non-existent user" do + context "when rcpt is an invalid value" do let(:rcpt) { "tripmeister_eder" } let(:anonymous_question) { "false" } let(:expected_response) do { "success" => false, - "status" => "not_found", + "status" => "err", + "message" => "Invalid parameter" + } + end + + include_examples "does not create the question", false + + include_examples "returns the expected response" + end + + context "when rcpt is a non-existent user" do + let(:rcpt) { "-1" } + let(:anonymous_question) { "false" } + let(:expected_response) do + { + "success" => false, + "status" => "not_found", "message" => anything } end @@ -222,7 +234,7 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do let(:expected_response) do { "success" => true, - "status" => "okay", + "status" => "okay", "message" => anything } end @@ -234,7 +246,6 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do context "when user allows anonymous questions" do let(:user_allows_anonymous_questions) { true } - let(:expected_question_direct) { true } include_examples "creates the question" @@ -243,7 +254,7 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do let(:expected_response) do { "success" => false, - "status" => "unknown", + "status" => "bad_request", "message" => anything } end @@ -297,7 +308,7 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do let(:expected_response) do { "success" => false, - "status" => "unknown", + "status" => "forbidden", "message" => anything } end @@ -306,6 +317,13 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do context "when anonymousQuestion is false" do let(:anonymous_question) { "false" } + let(:expected_response) do + { + "success" => false, + "status" => "bad_request", + "message" => anything + } + end include_examples "does not create the question" end @@ -314,16 +332,36 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do context "when rcpt is followers" do let(:rcpt) { "followers" } + let(:expected_response) do + { + "success" => false, + "status" => "err", + "message" => "Invalid parameter" + } + end include_examples "does not enqueue a QuestionWorker job" end - context "when rcpt is a non-existent user" do + context "when rcpt is an invalid value" do let(:rcpt) { "tripmeister_eder" } let(:expected_response) do { "success" => false, - "status" => "not_found", + "status" => "err", + "message" => "Invalid parameter" + } + end + + include_examples "does not create the question", false + end + + context "when rcpt is a non-existent user" do + let(:rcpt) { "-1" } + let(:expected_response) do + { + "success" => false, + "status" => "not_found", "message" => anything } end @@ -338,7 +376,7 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do let(:expected_response) do { "success" => false, - "status" => expected_status, + "status" => expected_status, "message" => anything } end @@ -368,7 +406,7 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do let(:expected_response) do { "success" => true, - "status" => "okay", + "status" => "okay", "message" => anything } end @@ -424,6 +462,12 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do end context "when the question does not exist" do + let(:question_id) { "-1" } + + include_examples "does not delete the question", "not_found" + end + + context "when the question is an invalid value" do let(:question_id) { "sonic_the_hedgehog" } include_examples "does not delete the question", "not_found" @@ -431,7 +475,7 @@ describe Ajax::QuestionController, :ajax_controller, type: :controller do end context "when user is not signed in" do - include_examples "does not delete the question", "err" + include_examples "does not delete the question", "not_authorized" end end end diff --git a/spec/workers/question_worker_spec.rb b/spec/workers/question_worker_spec.rb index dc069f82..94f45a50 100644 --- a/spec/workers/question_worker_spec.rb +++ b/spec/workers/question_worker_spec.rb @@ -28,10 +28,10 @@ describe QuestionWorker do end it "respects mute rules" do - question.content = 'Some spicy question text' + question.content = "Some spicy question text" question.save - MuteRule.create(user_id: user.followers.first.id, muted_phrase: 'spicy') + MuteRule.create(user_id: user.followers.first.id, muted_phrase: "spicy") expect { subject } .to(