From d609435f7cee1ad492b438cfaf6d76c2c06011e5 Mon Sep 17 00:00:00 2001 From: Karina Kwiatek Date: Sun, 16 Jan 2022 18:38:06 +0100 Subject: [PATCH] Update `Ajax::RelationshipController` to use usernames; Test Use Case --- .../ajax/relationship_controller.rb | 6 +- app/controllers/ajax_controller.rb | 24 ++++++ lib/errors.rb | 3 - lib/types.rb | 2 + lib/use_case/base.rb | 8 -- lib/use_case/relationship/create.rb | 17 +---- lib/use_case/relationship/destroy.rb | 19 +---- .../ajax/relationship_controller_spec.rb | 11 +-- spec/lib/use_case/relationship/create_spec.rb | 70 ++++++++++++++++++ .../lib/use_case/relationship/destroy_spec.rb | 73 +++++++++++++++++++ spec/shared_examples/error_raising.rb | 6 ++ 11 files changed, 191 insertions(+), 48 deletions(-) create mode 100644 spec/lib/use_case/relationship/create_spec.rb create mode 100644 spec/lib/use_case/relationship/destroy_spec.rb diff --git a/app/controllers/ajax/relationship_controller.rb b/app/controllers/ajax/relationship_controller.rb index 5adbd958..b24659f4 100644 --- a/app/controllers/ajax/relationship_controller.rb +++ b/app/controllers/ajax/relationship_controller.rb @@ -8,8 +8,10 @@ class Ajax::RelationshipController < AjaxController before_action :authenticate_user! def create + params.require :target_user + UseCase::Relationship::Create.call( - current_user_id: current_user.id, + current_user: current_user.screen_name, target_user: params[:target_user], type: params[:type] ) @@ -23,7 +25,7 @@ class Ajax::RelationshipController < AjaxController def destroy UseCase::Relationship::Destroy.call( - current_user_id: current_user.id, + current_user: current_user.screen_name, target_user: params[:target_user], type: params[:type] ) diff --git a/app/controllers/ajax_controller.rb b/app/controllers/ajax_controller.rb index 11757c9d..5a8fc00f 100644 --- a/app/controllers/ajax_controller.rb +++ b/app/controllers/ajax_controller.rb @@ -18,6 +18,30 @@ class AjaxController < ApplicationController return_response end + rescue_from(KeyError) do |e| + Sentry.capture_exception(e) + + @response = { + success: false, + message: "Missing parameter: #{e.key}", + status: :err + } + + return_response + end + + rescue_from(Dry::Types::CoercionError, Dry::Types::ConstraintError) do |e| + Sentry.capture_exception(e) + + @response = { + success: false, + message: "Invalid parameter", + status: :err + } + + return_response + end + rescue_from(ActiveRecord::RecordNotFound) do |e| Sentry.capture_exception(e) diff --git a/lib/errors.rb b/lib/errors.rb index 20220f55..375db88a 100644 --- a/lib/errors.rb +++ b/lib/errors.rb @@ -19,9 +19,6 @@ module Errors end end - class ParamIsMissing < BadRequest - end - class InvalidBanDuration < BadRequest end diff --git a/lib/types.rb b/lib/types.rb index 51e96551..0995d88f 100644 --- a/lib/types.rb +++ b/lib/types.rb @@ -4,4 +4,6 @@ require 'dry-types' module Types include Dry.Types() + + RelationshipTypes = Types::String.enum('follow') end diff --git a/lib/use_case/base.rb b/lib/use_case/base.rb index 2d9578a2..5468e0ff 100644 --- a/lib/use_case/base.rb +++ b/lib/use_case/base.rb @@ -15,13 +15,5 @@ module UseCase def call raise NotImplementedError end - - private - - def not_blank!(*args) - args.each do |arg| - raise Errors::ParamIsMissing if instance_variable_get("@#{arg}").blank? - end - end end end \ No newline at end of file diff --git a/lib/use_case/relationship/create.rb b/lib/use_case/relationship/create.rb index 54f61f39..5efab61d 100644 --- a/lib/use_case/relationship/create.rb +++ b/lib/use_case/relationship/create.rb @@ -6,15 +6,11 @@ require "errors" module UseCase module Relationship class Create < UseCase::Base - option :current_user_id, type: Types::Coercible::Integer + option :current_user, type: Types::Coercible::String option :target_user, type: Types::Coercible::String - option :type, type: Types::Coercible::String + option :type, type: Types::RelationshipTypes def call - not_blank! :current_user_id, :target_user, :type - - type = @type.downcase - ensure_type(type) source_user = find_source_user target_user = find_target_user @@ -31,19 +27,14 @@ module UseCase private - def ensure_type(type) - raise Errors::BadRequest unless type == 'follow' - end - def find_source_user - user_id = @current_user_id - ::User.find(user_id) + return current_user if current_user.is_a?(::User) + ::User.find_by!(screen_name: current_user) rescue ActiveRecord::RecordNotFound raise Errors::UserNotFound end def find_target_user - target_user = @target_user return target_user if target_user.is_a?(::User) ::User.find_by!(screen_name: target_user) rescue ActiveRecord::RecordNotFound diff --git a/lib/use_case/relationship/destroy.rb b/lib/use_case/relationship/destroy.rb index 3788f38c..d72869d6 100644 --- a/lib/use_case/relationship/destroy.rb +++ b/lib/use_case/relationship/destroy.rb @@ -6,15 +6,11 @@ require "errors" module UseCase module Relationship class Destroy < UseCase::Base - option :current_user_id, type: Types::Coercible::Integer - option :target_user, type: Types::Coercible::String - option :type, type: Types::Coercible::String + option :current_user, type: Types::Coercible::String | Types.Instance(User) + option :target_user, type: Types::Coercible::String | Types.Instance(User) + option :type, type: Types::RelationshipTypes def call - not_blank! :current_user_id, :target_user, :type - - type = @type.downcase - ensure_type(type) source_user = find_source_user target_user = find_target_user @@ -31,20 +27,13 @@ module UseCase private - def ensure_type(type) - raise Errors::BadRequest unless type == 'follow' - end - def find_source_user - user_id = @current_user_id - ::User.find(user_id) + ::User.find_by!(screen_name: current_user) rescue ActiveRecord::RecordNotFound raise Errors::UserNotFound end def find_target_user - target_user = @target_user - return target_user if target_user.is_a?(::User) ::User.find_by!(screen_name: target_user) rescue ActiveRecord::RecordNotFound raise Errors::UserNotFound diff --git a/spec/controllers/ajax/relationship_controller_spec.rb b/spec/controllers/ajax/relationship_controller_spec.rb index 047b57cd..34a941e1 100644 --- a/spec/controllers/ajax/relationship_controller_spec.rb +++ b/spec/controllers/ajax/relationship_controller_spec.rb @@ -5,9 +5,9 @@ require "rails_helper" describe Ajax::RelationshipController, type: :controller do shared_examples_for "params is empty" do - let(:params) { { type: type } } # type is still required as it's part of the route + let(:params) { } - include_examples "ajax does not succeed", "param is missing" + include_examples "ajax does not succeed", "is required" end let!(:user) { FactoryBot.create(:user) } @@ -60,7 +60,7 @@ describe Ajax::RelationshipController, type: :controller do let(:type) { "dick" } it_behaves_like "params is empty" - include_examples "ajax does not succeed", "Bad Request" + include_examples "ajax does not succeed", "Invalid parameter" end end end @@ -85,8 +85,6 @@ describe Ajax::RelationshipController, type: :controller do end end - it_behaves_like "params is empty" - context "target_user does not exist" do let(:target_user) { "peter-witzig" } @@ -114,8 +112,7 @@ describe Ajax::RelationshipController, type: :controller do context "type = 'dick'" do let(:type) { "dick" } - it_behaves_like "params is empty" - include_examples "ajax does not succeed", "Bad Request" + include_examples "ajax does not succeed", "Invalid parameter" end end end diff --git a/spec/lib/use_case/relationship/create_spec.rb b/spec/lib/use_case/relationship/create_spec.rb new file mode 100644 index 00000000..723b1d2e --- /dev/null +++ b/spec/lib/use_case/relationship/create_spec.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +require "rails_helper" + +require "use_case/relationship/create" +require "errors" + +describe UseCase::Relationship::Create do + shared_examples_for "valid relationship type" do + context "current_user does not exist" do + let(:current_user) { "Schweinsbraten" } + + include_examples "raises an error", Errors::UserNotFound + end + + context "target_user does not exist" do + let(:target_user) { "peterwitzig" } + + include_examples "raises an error", Errors::UserNotFound + end + + context "target_user is current_user" do + let(:target_user) { user1.screen_name } + + include_examples "raises an error", Errors::SelfAction + end + + context "target_user is different from current_user" do + its([:status]) { is_expected.to eq(201) } + its([:extra]) { is_expected.to eq(target_user: user2) } + + it "creates a relationship" do + expect { subject }.to change { Relationship.count }.by(1) + last_relationship = Relationship.last + expect(last_relationship.source_id).to eq(user1.id) + expect(last_relationship.target_id).to eq(user2.id) + end + end + end + + let(:base_params) do + { + current_user: current_user, + target_user: target_user, + type: type + } + end + let(:params) { base_params } + let(:current_user) { user1.screen_name } + let(:target_user) { user2.screen_name } + let(:type) { nil } + + # test data: + let!(:user1) { FactoryBot.create(:user, screen_name: "timallen") } + let!(:user2) { FactoryBot.create(:user, screen_name: "joehilyar") } + + subject { described_class.call(params) } + + context "type = 'follow'" do + let(:type) { "follow" } + + include_examples "valid relationship type" + end + + context "type = 'dick'" do + let(:type) { "dick" } + + include_examples "raises an error", Dry::Types::ConstraintError + end +end diff --git a/spec/lib/use_case/relationship/destroy_spec.rb b/spec/lib/use_case/relationship/destroy_spec.rb new file mode 100644 index 00000000..3a7cf62c --- /dev/null +++ b/spec/lib/use_case/relationship/destroy_spec.rb @@ -0,0 +1,73 @@ +# frozen_string_literal: true + +require "rails_helper" + +require "use_case/relationship/destroy" +require "errors" + +describe UseCase::Relationship::Destroy do + shared_examples_for "valid relationship type" do + its([:status]) { is_expected.to eq(204) } + its([:extra]) { is_expected.to eq(target_user: user2) } + + context "relationship exists" do + before do + user1.public_send(type, user2) + end + + it "destroys a relationship" do + expect { subject }.to change { Relationship.count }.by(-1) + end + end + + context "relationship does not exist" do + it "does not change anything at all" do + expect { subject }.to change { Relationship.count }.by(0) + end + end + + context "current_user does not exist" do + let(:current_user) { "Schweinsbraten" } + let(:target_user) { user2.screen_name } + + include_examples "raises an error", Errors::UserNotFound + end + + context "target_user does not exist" do + let(:current_user) { user1.screen_name } + let(:target_user) { "peterwitzig" } + + include_examples "raises an error", Errors::UserNotFound + end + end + + let(:base_params) do + { + current_user: current_user, + target_user: target_user, + type: type + } + end + let(:params) { base_params } + let(:current_user) { user1.screen_name } + let(:target_user) { user2.screen_name } + let(:type) { nil } + + # test data: + let!(:user1) { FactoryBot.create(:user, screen_name: "timallen") } + let!(:user2) { FactoryBot.create(:user, screen_name: "joehilyar") } + + subject { described_class.call(params) } + + context "type = 'follow'" do + let(:type) { "follow" } + + include_examples "valid relationship type" + end + + context "type = 'dick'" do + let(:type) { "dick" } + + include_examples "raises an error", Dry::Types::ConstraintError + end +end diff --git a/spec/shared_examples/error_raising.rb b/spec/shared_examples/error_raising.rb index 5eb073ed..03290783 100644 --- a/spec/shared_examples/error_raising.rb +++ b/spec/shared_examples/error_raising.rb @@ -1,5 +1,11 @@ # frozen_string_literal: true +RSpec.shared_examples_for "raises an error" do |error_klass| + it "raises an error" do + expect { subject }.to raise_error(kind_of(error_klass)) + end +end + RSpec.shared_examples_for "ajax does not succeed" do |part_of_error_message| it "ajax does not succeed" do subject