From 04744b4dacf80d82c1b61dddc3884d0dd34dcb54 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jonne=20Ha=C3=9F?= Date: Thu, 30 Jan 2020 00:01:05 +0100 Subject: [PATCH] API: Return 409 when trying to create something existing and 410 when trying to delete something already gone Probably missed a few more cases where we always return sucess when the user requests status quo, but this should cover most ground --- app/controllers/api/v1/likes_controller.rb | 4 ++-- .../api/v1/post_interactions_controller.rb | 6 +++++- app/controllers/api/v1/reshares_controller.rb | 4 +++- .../api/v1/tag_followings_controller.rb | 4 ++++ app/controllers/tag_followings_controller.rb | 18 ++++++++--------- app/services/tag_following_service.rb | 17 ++++++++++++---- spec/integration/api/likes_controller_spec.rb | 4 ++-- .../api/post_interactions_controller_spec.rb | 4 ++-- .../api/reshares_controller_spec.rb | 2 +- .../api/tag_followings_controller_spec.rb | 4 ++-- spec/services/tag_following_service_spec.rb | 20 ++++++++++++++++--- 11 files changed, 60 insertions(+), 27 deletions(-) diff --git a/app/controllers/api/v1/likes_controller.rb b/app/controllers/api/v1/likes_controller.rb index 0b4180776..ec20ecf91 100644 --- a/app/controllers/api/v1/likes_controller.rb +++ b/app/controllers/api/v1/likes_controller.rb @@ -36,7 +36,7 @@ module Api like_service.create(params[:post_id]) rescue ActiveRecord::RecordInvalid => e if e.message == "Validation failed: Target has already been taken" - return render_error 422, I18n.t("api.endpoint_errors.likes.like_exists") + return render_error 409, I18n.t("api.endpoint_errors.likes.like_exists") end raise @@ -52,7 +52,7 @@ module Api if success head :no_content else - render_error 404, I18n.t("api.endpoint_errors.likes.no_like") + render_error 410, I18n.t("api.endpoint_errors.likes.no_like") end end diff --git a/app/controllers/api/v1/post_interactions_controller.rb b/app/controllers/api/v1/post_interactions_controller.rb index 5fa9557c3..945ea6314 100644 --- a/app/controllers/api/v1/post_interactions_controller.rb +++ b/app/controllers/api/v1/post_interactions_controller.rb @@ -15,6 +15,8 @@ module Api def subscribe post = find_post + return head :conflict if current_user.participations.find_by(target_id: post.id) + current_user.participate!(post) head :no_content rescue ActiveRecord::RecordInvalid @@ -29,7 +31,9 @@ module Api def mute post = find_post - participation = current_user.participations.find_by!(target_id: post.id) + participation = current_user.participations.find_by(target_id: post.id) + return head :gone unless participation + participation.destroy head :no_content end diff --git a/app/controllers/api/v1/reshares_controller.rb b/app/controllers/api/v1/reshares_controller.rb index 40e9fdf04..dd9052e40 100644 --- a/app/controllers/api/v1/reshares_controller.rb +++ b/app/controllers/api/v1/reshares_controller.rb @@ -34,7 +34,9 @@ module Api def create reshare = reshare_service.create(params.require(:post_id)) - rescue ActiveRecord::RecordNotFound, ActiveRecord::RecordInvalid, RuntimeError + rescue ActiveRecord::RecordInvalid + render_error 409, I18n.t("reshares.create.error") + rescue ActiveRecord::RecordNotFound, RuntimeError render_error 422, I18n.t("reshares.create.error") else render json: PostPresenter.new(reshare, current_user).as_api_response diff --git a/app/controllers/api/v1/tag_followings_controller.rb b/app/controllers/api/v1/tag_followings_controller.rb index 5579a32f6..92a45b493 100755 --- a/app/controllers/api/v1/tag_followings_controller.rb +++ b/app/controllers/api/v1/tag_followings_controller.rb @@ -18,6 +18,8 @@ module Api def create tag_followings_service.create(params.require(:name)) head :no_content + rescue TagFollowingService::DuplicateTag + render_error 409, I18n.t("api.endpoint_errors.tags.cant_process") rescue StandardError render_error 422, I18n.t("api.endpoint_errors.tags.cant_process") end @@ -25,6 +27,8 @@ module Api def destroy tag_followings_service.destroy_by_name(params.require(:id)) head :no_content + rescue ActiveRecord::RecordNotFound + render_error 410, I18n.t("api.endpoint_errors.tags.cant_process") end private diff --git a/app/controllers/tag_followings_controller.rb b/app/controllers/tag_followings_controller.rb index d4c73e19f..7e9ef573d 100644 --- a/app/controllers/tag_followings_controller.rb +++ b/app/controllers/tag_followings_controller.rb @@ -16,6 +16,8 @@ class TagFollowingsController < ApplicationController def create tag = tag_followings_service.create(params["name"]) render json: tag.to_json, status: :created + rescue TagFollowingService::DuplicateTag + render json: tag_followings_service.find(params["name"]), status: created rescue StandardError head :forbidden end @@ -23,16 +25,14 @@ class TagFollowingsController < ApplicationController # DELETE /tag_followings/1 # DELETE /tag_followings/1.xml def destroy - success = tag_followings_service.destroy(params["id"]) + tag_followings_service.destroy(params["id"]) - if success - respond_to do |format| - format.any(:js, :json) { head :no_content } - end - else - respond_to do |format| - format.any(:js, :json) { head :forbidden } - end + respond_to do |format| + format.any(:js, :json) { head :no_content } + end + rescue ActiveRecord::RecordNotFound + respond_to do |format| + format.any(:js, :json) { head :forbidden } end end diff --git a/app/services/tag_following_service.rb b/app/services/tag_following_service.rb index 2bc34fe60..9a56d0607 100644 --- a/app/services/tag_following_service.rb +++ b/app/services/tag_following_service.rb @@ -10,24 +10,33 @@ class TagFollowingService raise ArgumentError, "Name field null or empty" if name_normalized.blank? tag = ActsAsTaggableOn::Tag.find_or_create_by(name: name_normalized) + raise DuplicateTag if @user.tag_followings.exists?(tag_id: tag.id) + tag_following = @user.tag_followings.new(tag_id: tag.id) raise "Can't process tag entity" unless tag_following.save tag end + def find(name) + name_normalized = ActsAsTaggableOn::Tag.normalize(name) + ActsAsTaggableOn::Tag.find_or_create_by(name: name_normalized) + end + def destroy(id) - tag_following = @user.tag_followings.find_by(tag_id: id) - tag_following&.destroy + tag_following = @user.tag_followings.find_by!(tag_id: id) + tag_following.destroy end def destroy_by_name(name) name_normalized = ActsAsTaggableOn::Tag.normalize(name) - followed_tag = @user.followed_tags.find_by(name: name_normalized) - destroy(followed_tag.id) if followed_tag + followed_tag = @user.followed_tags.find_by!(name: name_normalized) + destroy(followed_tag.id) end def index @user.followed_tags end + + class DuplicateTag < RuntimeError; end end diff --git a/spec/integration/api/likes_controller_spec.rb b/spec/integration/api/likes_controller_spec.rb index faf5d3686..56f232d7c 100644 --- a/spec/integration/api/likes_controller_spec.rb +++ b/spec/integration/api/likes_controller_spec.rb @@ -134,7 +134,7 @@ describe Api::V1::LikesController do api_v1_post_likes_path(post_id: @status.guid), params: {access_token: access_token} ) - confirm_api_error(response, 422, I18n.t("api.endpoint_errors.likes.like_exists")) + confirm_api_error(response, 409, I18n.t("api.endpoint_errors.likes.like_exists")) likes = like_service.find_for_post(@status.guid) expect(likes.length).to eq(1) @@ -206,7 +206,7 @@ describe Api::V1::LikesController do api_v1_post_likes_path(post_id: @status.guid), params: {access_token: access_token} ) - confirm_api_error(response, 404, I18n.t("api.endpoint_errors.likes.no_like")) + confirm_api_error(response, 410, I18n.t("api.endpoint_errors.likes.no_like")) likes = like_service.find_for_post(@status.guid) expect(likes.length).to eq(0) diff --git a/spec/integration/api/post_interactions_controller_spec.rb b/spec/integration/api/post_interactions_controller_spec.rb index 742180cdb..e7e9eb35d 100644 --- a/spec/integration/api/post_interactions_controller_spec.rb +++ b/spec/integration/api/post_interactions_controller_spec.rb @@ -72,7 +72,7 @@ describe Api::V1::PostInteractionsController do access_token: access_token } ) - expect(response.status).to eq(422) + expect(response.status).to eq(409) end it "with improper guid" do @@ -225,7 +225,7 @@ describe Api::V1::PostInteractionsController do access_token: access_token } ) - expect(response.status).to eq(404) + expect(response.status).to eq(410) end it "with insufficient token" do diff --git a/spec/integration/api/reshares_controller_spec.rb b/spec/integration/api/reshares_controller_spec.rb index 67a4ae54a..797e0b792 100644 --- a/spec/integration/api/reshares_controller_spec.rb +++ b/spec/integration/api/reshares_controller_spec.rb @@ -137,7 +137,7 @@ describe Api::V1::ResharesController do params: {access_token: access_token} ) - confirm_api_error(response, 422, I18n.t("reshares.create.error")) + confirm_api_error(response, 409, I18n.t("reshares.create.error")) end end diff --git a/spec/integration/api/tag_followings_controller_spec.rb b/spec/integration/api/tag_followings_controller_spec.rb index 3262d7384..75514ae96 100755 --- a/spec/integration/api/tag_followings_controller_spec.rb +++ b/spec/integration/api/tag_followings_controller_spec.rb @@ -67,7 +67,7 @@ describe Api::V1::TagFollowingsController do params: {name: "tag3", access_token: access_token} ) - confirm_api_error(response, 422, I18n.t("api.endpoint_errors.tags.cant_process")) + confirm_api_error(response, 409, I18n.t("api.endpoint_errors.tags.cant_process")) end end @@ -150,7 +150,7 @@ describe Api::V1::TagFollowingsController do api_v1_tag_following_path(SecureRandom.uuid.to_s), params: {access_token: access_token} ) - expect(response.status).to eq(204) + confirm_api_error(response, 410, I18n.t("api.endpoint_errors.tags.cant_process")) get( api_v1_tag_followings_path, diff --git a/spec/services/tag_following_service_spec.rb b/spec/services/tag_following_service_spec.rb index 47a846df5..f5b705df7 100644 --- a/spec/services/tag_following_service_spec.rb +++ b/spec/services/tag_following_service_spec.rb @@ -23,6 +23,14 @@ describe TagFollowingService do expect { tag_following_service(alice).create("#") }.to raise_error(ArgumentError) expect { tag_following_service(alice).create(" ") }.to raise_error(ArgumentError) end + + it "throws an error when trying to follow an already followed tag" do + name = SecureRandom.uuid + tag_following_service.create(name) + expect { + tag_following_service.create(name) + }.to raise_error TagFollowingService::DuplicateTag + end end describe "#destroy" do @@ -44,14 +52,20 @@ describe TagFollowingService do it "Does nothing with tag that isn't already followed" do original_length = alice.followed_tags.length - tag_following_service(alice).destroy_by_name(SecureRandom.uuid) - tag_following_service(alice).destroy(-1) + expect { + tag_following_service(alice).destroy_by_name(SecureRandom.uuid) + }.to raise_error ActiveRecord::RecordNotFound + expect { + tag_following_service(alice).destroy(-1) + }.to raise_error ActiveRecord::RecordNotFound expect(alice.followed_tags.length).to eq(original_length) end it "Does nothing with empty tag name" do original_length = alice.followed_tags.length - tag_following_service(alice).destroy_by_name("") + expect { + tag_following_service(alice).destroy_by_name("") + }.to raise_error ActiveRecord::RecordNotFound expect(alice.followed_tags.length).to eq(original_length) end end