Refactor Ajax::QuestionController#create into separate UseCases

Co-authored-by: Georg Gadinger <nilsding@nilsding.org>

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'
+    # set up fake success response -- the use cases raise errors on exceptions
-    author_is_anonymous = is_never_anonymous ? false : params[:anonymousQuestion]
+    # which get rescued by the base class
+    @response = {
+      success: true,
+      message: t(".success"),
+      status:  :okay
+    }
 
-    begin
+    if user_signed_in? && params[:rcpt] == "followers"
-      question = Question.create!(content:             params[:question],
+      UseCase::Question::CreateFollowers.call(
-                                  author_is_anonymous: author_is_anonymous,
+        source_user_id:    current_user.id,
-                                  author_identifier:   AnonymousBlock.get_identifier(request.ip),
+        content:           params[:question],
-                                  user:                current_user,
+        author_identifier: AnonymousBlock.get_identifier(request.ip)
-                                  direct:              params[:rcpt] != "followers")
+      )
-    rescue ActiveRecord::RecordInvalid => e
-      Sentry.capture_exception(e)
-      @response[:status] = :rec_inv
-      @response[:message] = t(".invalid")
       return
     end
 
-    if !user_signed_in? && !question.author_is_anonymous
+    UseCase::Question::Create.call(
-      question.delete
+      source_user_id:    user_signed_in? ? current_user.id : nil,
-      return
+      target_user_id:    params[:rcpt],
-    end
+      content:           params[:question],
-
+      anonymous:         params[:anonymousQuestion],
-    unless current_user.nil?
+      author_identifier: AnonymousBlock.get_identifier(request.ip)
-      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
   end
 end

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

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

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

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

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
@@ -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,9 +194,25 @@ 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"  => "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,
@@ -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,12 +332,32 @@ 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"  => "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,
@@ -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

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(