From 2ff67bda246ffd584afc3e456f023aa244507e16 Mon Sep 17 00:00:00 2001 From: Joshua Wehner Date: Tue, 30 Aug 2022 16:43:18 +0200 Subject: [PATCH] Try an idempotentish reaction create API (#18377) * Try an idempotentish reaction create API * Add tests for create API endpoint * Try -> ReactionHandler * Fix existing tests first * Tests for .toggle; fix broken handle_existing * Use the Handler * Tweak http status for reaction create * Use the category reader Co-authored-by: Ridhwana Co-authored-by: Ridhwana --- .../api/v1/reactions_controller.rb | 21 +++- app/controllers/reactions_controller.rb | 2 +- ...reaction_toggle.rb => reaction_handler.rb} | 57 +++++---- config/routes.rb | 1 + spec/requests/api/v1/docs/reactions_spec.rb | 56 ++++++++- spec/requests/api/v1/reactions_spec.rb | 65 +++++++++- spec/services/reaction_handler_spec.rb | 113 ++++++++++++++++++ swagger/v1/api_v1.json | 111 +++++++++++++---- 8 files changed, 364 insertions(+), 62 deletions(-) rename app/services/{reaction_toggle.rb => reaction_handler.rb} (80%) create mode 100644 spec/services/reaction_handler_spec.rb diff --git a/app/controllers/api/v1/reactions_controller.rb b/app/controllers/api/v1/reactions_controller.rb index a6512c299..341e12f54 100644 --- a/app/controllers/api/v1/reactions_controller.rb +++ b/app/controllers/api/v1/reactions_controller.rb @@ -3,10 +3,29 @@ module Api class ReactionsController < ApiController before_action :authenticate! + def create + remove_count_cache_key + + result = ReactionHandler.create(params, current_user: current_user || @user) + + if result.success? + render json: { + result: result.action, + category: result.category, + id: result.reaction.id, + reactable_id: result.reaction.reactable_id, + reactable_type: result.reaction.reactable_type + }, + status: (result.action == "create" ? :created : :ok) + else + render json: { error: result.errors_as_sentence, status: 422 }, status: :unprocessable_entity + end + end + def toggle remove_count_cache_key - result = ReactionToggle.toggle(params, current_user: current_user || @user) + result = ReactionHandler.toggle(params, current_user: current_user || @user) if result.success? render json: { diff --git a/app/controllers/reactions_controller.rb b/app/controllers/reactions_controller.rb index 4ac6ce829..84e1cdeb6 100644 --- a/app/controllers/reactions_controller.rb +++ b/app/controllers/reactions_controller.rb @@ -55,7 +55,7 @@ class ReactionsController < ApplicationController def create remove_count_cache_key - result = ReactionToggle.toggle(params, current_user: current_user) + result = ReactionHandler.toggle(params, current_user: current_user) if result.success? render json: { result: result.action, category: result.category } diff --git a/app/services/reaction_toggle.rb b/app/services/reaction_handler.rb similarity index 80% rename from app/services/reaction_toggle.rb rename to app/services/reaction_handler.rb index a8f45ecb3..c2c47c89f 100644 --- a/app/services/reaction_toggle.rb +++ b/app/services/reaction_handler.rb @@ -1,4 +1,4 @@ -class ReactionToggle +class ReactionHandler class Result attr_accessor :action, :category, :reaction @@ -15,6 +15,10 @@ class ReactionToggle end end + def self.create(params, current_user:) + new(params, current_user: current_user).create + end + def self.toggle(params, current_user:) new(params, current_user: current_user).toggle end @@ -28,26 +32,29 @@ class ReactionToggle @category = params[:category] || "like" end - attr_reader :reactable_id, :reactable_type, :category, :params, :current_user + attr_reader :category, :params, :current_user, :reactable_type, :reactable_id delegate :rate_limiter, to: :current_user - def toggle - if reactable_type == "Article" && category.in?(Reaction::PRIVILEGED_CATEGORIES) - destroy_contradictory_mod_reactions - end + def create + destroy_contradictory_mod_reactions if reactable_type == "Article" + return noop_result if existing_reaction - @existing_reaction ||= existing_reaction - if @existing_reaction - handle_existing_reaction - else - create_new_reaction - end + create_new_reaction + end + + def toggle + destroy_contradictory_mod_reactions if reactable_type == "Article" + return handle_existing_reaction if existing_reaction + + create_new_reaction end private def destroy_contradictory_mod_reactions + return unless category.in?(Reaction::PRIVILEGED_CATEGORIES) + reactions = Reaction.contradictory_mod_reactions( category: category, reactable_id: reactable_id, @@ -66,7 +73,7 @@ class ReactionToggle end def existing_reaction - Reaction.where( + @existing_reaction ||= Reaction.where( user_id: current_user.id, reactable_id: reactable_id, reactable_type: reactable_type, @@ -74,14 +81,6 @@ class ReactionToggle ).first end - def handle_existing_reaction - return unless @existing_reaction - - destroy_reaction(@existing_reaction) - log_audit(@existing_reaction) - result(@existing_reaction, "destroy") - end - def create_new_reaction reaction = build_reaction(category) result = result(reaction, nil) @@ -119,6 +118,12 @@ class ReactionToggle Reaction.new(create_params) end + def handle_existing_reaction + destroy_reaction(existing_reaction) + log_audit(existing_reaction) + result(existing_reaction, "destroy") + end + def log_audit(reaction) return unless reaction.negative? && current_user.auditable? @@ -128,11 +133,7 @@ class ReactionToggle end def result(reaction, action) - if action - Result.new category: category, reaction: reaction, action: action - else - Result.new category: category, reaction: reaction - end + Result.new category: category, reaction: reaction, action: action end def rate_limit_reaction_creation @@ -167,4 +168,8 @@ class ReactionToggle context: "readinglist_reaction", rating: user_experience_level) end + + def noop_result + Result.new category: category, action: "none", reaction: existing_reaction + end end diff --git a/config/routes.rb b/config/routes.rb index 3ceead8d8..4f3dc43d1 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -48,6 +48,7 @@ Rails.application.routes.draw do put "/articles/:id/unpublish", to: "articles#unpublish", as: :article_unpublish put "/users/:id/unpublish", to: "users#unpublish", as: :user_unpublish + post "/reactions", to: "reactions#create" post "/reactions/toggle", to: "reactions#toggle" draw :api diff --git a/spec/requests/api/v1/docs/reactions_spec.rb b/spec/requests/api/v1/docs/reactions_spec.rb index 7622f33fb..5492f1319 100644 --- a/spec/requests/api/v1/docs/reactions_spec.rb +++ b/spec/requests/api/v1/docs/reactions_spec.rb @@ -10,18 +10,18 @@ RSpec.describe "api/v1/reactions", type: :request do let(:category) { "like" } let(:reactable) { create :article } let(:reaction) { reactable.reactions.create user: user, category: "like" } - let(:result) { ReactionToggle::Result.new reaction: reaction } + let(:result) { ReactionHandler::Result.new reaction: reaction } let(:user) { api_secret.user } before do result.category = category allow(FeatureFlag).to receive(:enabled?).with(:api_v1).and_return(true) - allow(ReactionToggle).to receive(:toggle).and_return(result) + allow(ReactionHandler).to receive(:toggle).and_return(result) end path "/api/reactions/toggle" do - describe "post to create reaction" do - post("create reaction") do + describe "post to toggle reaction" do + post("toggle reaction") do tags "reactions" description(<<-DESCRIBE.strip) This endpoint allows the client to toggle the user's reaction to a specified reactable (eg, Article, Comment, or User). For examples: @@ -66,6 +66,54 @@ RSpec.describe "api/v1/reactions", type: :request do end end end + + path "/api/reactions" do + describe "post to create reaction" do + post("create reaction") do + tags "reactions" + description(<<-DESCRIBE.strip) + This endpoint allows the client to create a reaction to a specified reactable (eg, Article, Comment, or User). For examples: + * "Like"ing an Article will create a new "like" Reaction from the user for that Articles + * "Like"ing that Article a second time will return the previous "like" + DESCRIBE + + produces "application/json" + parameter name: :category, in: :query, required: true, schema: { + type: :string, + enum: Reaction::PUBLIC_CATEGORIES + } + parameter name: :reactable_id, in: :query, required: true, schema: { + type: :integer, + format: :int32 + } + parameter name: :reactable_type, in: :query, required: true, schema: { + type: :string, + enum: Reaction::REACTABLE_TYPES + } + + let(:reactable_id) { reactable.id } + let(:reactable_type) { "Article" } + + before do + result.action = "create" + end + + response(200, "successful") do + let(:"api-key") { api_secret.secret } + add_examples + + run_test! + end + + response "401", "unauthorized" do + let(:"api-key") { "invalid" } + add_examples + + run_test! + end + end + end + end end # rubocop:enable RSpec/VariableName diff --git a/spec/requests/api/v1/reactions_spec.rb b/spec/requests/api/v1/reactions_spec.rb index ca95c7d7b..45d3d9fe9 100644 --- a/spec/requests/api/v1/reactions_spec.rb +++ b/spec/requests/api/v1/reactions_spec.rb @@ -18,14 +18,14 @@ RSpec.describe "Api::V1::Reactions", type: :request do let(:auth_header) { v1_headers.merge({ "api-key" => api_secret.secret }) } end - context "when unauthenticated and post to create" do + context "when unauthenticated and post to toggle" do it "returns unauthorized" do post api_reactions_toggle_path, params: params.to_json, headers: v1_headers expect(response).to have_http_status(:unauthorized) end end - context "when unauthorized and post to create" do + context "when unauthorized and post to toggle" do it "returns unauthorized" do post api_reactions_toggle_path, params: params.to_json, headers: v1_headers.merge({ "api-key" => "invalid api key" }) @@ -33,16 +33,16 @@ RSpec.describe "Api::V1::Reactions", type: :request do end end - context "when authorized and post to create" do + context "when authorized and post to toggle" do include_context "when user is authorized" before do allow(Rails.cache).to receive(:delete) - allow(ReactionToggle).to receive(:toggle).and_return(result) + allow(ReactionHandler).to receive(:toggle).and_return(result) end context "when toggled successfully" do - let(:result) { ReactionToggle::Result.new reaction: Reaction.new } + let(:result) { ReactionHandler::Result.new reaction: Reaction.new } it "responds with success" do post api_reactions_toggle_path, params: params.to_json, headers: auth_header @@ -58,7 +58,7 @@ RSpec.describe "Api::V1::Reactions", type: :request do context "when toggled unsuccessfully" do let(:result) do - ReactionToggle::Result.new.tap do |bad_result| + ReactionHandler::Result.new.tap do |bad_result| allow(bad_result).to receive(:success?).and_return(false) allow(bad_result).to receive(:errors_as_sentence).and_return("Stuff was bad") end @@ -70,4 +70,57 @@ RSpec.describe "Api::V1::Reactions", type: :request do end end end + + context "when unauthenticated and post to create" do + it "returns unauthorized" do + post api_reactions_path, params: params.to_json, headers: v1_headers + expect(response).to have_http_status(:unauthorized) + end + end + + context "when unauthorized and post to create" do + it "returns unauthorized" do + post api_reactions_path, params: params.to_json, + headers: v1_headers.merge({ "api-key" => "invalid api key" }) + expect(response).to have_http_status(:unauthorized) + end + end + + context "when authorized and post to create" do + include_context "when user is authorized" + + before do + allow(Rails.cache).to receive(:delete) + allow(ReactionHandler).to receive(:create).and_return(result) + end + + context "when created successfully" do + let(:result) { ReactionHandler::Result.new reaction: Reaction.new } + + it "responds with success" do + post api_reactions_path, params: params.to_json, headers: auth_header + expect(response).to have_http_status(:success) + end + + it "responds with expected JSON" do + post api_reactions_path, params: params.to_json, headers: auth_header + expect(JSON.parse(response.body).keys).to contain_exactly("id", "result", "category", "reactable_type", + "reactable_id") + end + end + + context "when created unsuccessfully" do + let(:result) do + ReactionHandler::Result.new.tap do |bad_result| + allow(bad_result).to receive(:success?).and_return(false) + allow(bad_result).to receive(:errors_as_sentence).and_return("Stuff was bad") + end + end + + it "responds with success" do + post api_reactions_path, params: params.to_json, headers: auth_header + expect(response).to have_http_status(:unprocessable_entity) + end + end + end end diff --git a/spec/services/reaction_handler_spec.rb b/spec/services/reaction_handler_spec.rb new file mode 100644 index 000000000..dbb8e3797 --- /dev/null +++ b/spec/services/reaction_handler_spec.rb @@ -0,0 +1,113 @@ +require "rails_helper" + +RSpec.describe ReactionHandler, type: :service do + # existing reaction by other user + # existing reaction by same user, other category + # no existing reaction = create + # existing reaction = no-op + # existing contradictory mod reaction + + let(:user) { create :user } + let(:article) { create :article } + let(:category) { "like" } + + let!(:other_category) { article.reactions.create! user: user, category: "hands" } + let!(:other_existing) { article.reactions.create! user: create(:user), category: "like" } + + let(:moderator) { create :user, :trusted } + let!(:contradictory_mod) { article.reactions.create! user: moderator, category: "thumbsup" } + + let(:params) do + { + reactable_id: article.id, + reactable_type: article.class.polymorphic_name, + category: category + } + end + + describe "#create" do + subject(:result) { described_class.new(params, current_user: user).create } + + context "when no existing/matching reaction by user" do + it "justs create" do + expect(result).to be_success + expect(result.action).to eq("create") + end + + it "ignores other existing reactions" do + expect(Reaction.ids).to include(other_category.id, other_existing.id, contradictory_mod.id) + end + end + + context "when there's an existing/matching reaction by user" do + let!(:existing) { article.reactions.create! user: user, category: "like" } + + it "does nothing" do + expect(result).to be_success + expect(result.action).to eq("none") + end + + it "ignores other existing reactions" do + expect(Reaction.ids).to include(other_category.id, other_existing.id, contradictory_mod.id, existing.id) + end + end + + context "when there's an existing, contradictory mod reaction" do + let(:user) { moderator } + let(:category) { "vomit" } + + it "creates" do + expect(result).to be_success + expect(result.action).to eq("create") + end + + it "destroys the other reaction as a side-effect" do + expect(result).to be_success + expect(Reaction.ids).not_to include(contradictory_mod.id) + end + end + end + + describe "#toggle" do + subject(:result) { described_class.new(params, current_user: user).toggle } + + context "when no existing/matching reaction by user" do + it "justs create" do + expect(result).to be_success + expect(result.action).to eq("create") + end + + it "ignores other existing reactions" do + expect(Reaction.ids).to include(other_category.id, other_existing.id, contradictory_mod.id) + end + end + + context "when there's an existing/matching reaction by user" do + let!(:existing) { article.reactions.create! user: user, category: "like" } + + it "un-likes" do + expect(result).to be_success + expect(result.action).to eq("destroy") + end + + it "ignores other existing reactions" do + expect(Reaction.ids).to include(other_category.id, other_existing.id, contradictory_mod.id, existing.id) + end + end + + context "when there's an existing, contradictory mod reaction" do + let(:user) { moderator } + let(:category) { "vomit" } + + it "creates" do + expect(result).to be_success + expect(result.action).to eq("create") + end + + it "destroys the other reaction as a side-effect" do + expect(result).to be_success + expect(Reaction.ids).not_to include(contradictory_mod.id) + end + end + end +end diff --git a/swagger/v1/api_v1.json b/swagger/v1/api_v1.json index 53ce1b774..0133a0476 100644 --- a/swagger/v1/api_v1.json +++ b/swagger/v1/api_v1.json @@ -103,45 +103,45 @@ "example": [ { "type_of": "article", - "id": 4453, - "title": "East of Eden168", - "description": "Put a bird on it aesthetic narwhal xoxo pug. Disrupt slow-carb williamsburg crucifix...", - "readable_publish_date": "Aug 23", - "slug": "east-of-eden168-21ml", - "path": "/username372/east-of-eden168-21ml", - "url": "http://localhost:3000/username372/east-of-eden168-21ml", + "id": 5137, + "title": "The Yellow Meads of Asphodel168", + "description": "Scenester tofu cold-pressed flannel vhs. Yr park portland organic offal fingerstache forage....", + "readable_publish_date": "Aug 30", + "slug": "the-yellow-meads-of-asphodel168-3bgf", + "path": "/username372/the-yellow-meads-of-asphodel168-3bgf", + "url": "http://localhost:3000/username372/the-yellow-meads-of-asphodel168-3bgf", "comments_count": 0, "public_reactions_count": 0, "collection_id": null, - "published_timestamp": "2022-08-23T12:08:21Z", + "published_timestamp": "2022-08-30T09:45:05Z", "positive_reactions_count": 0, - "cover_image": "http://localhost:3000/assets/24-377bc0861a9a539e8d1875ea9d4eea9e0226d93e1b6e9317e0c73c754699cc14.png", - "social_image": "http://localhost:3000/assets/24-377bc0861a9a539e8d1875ea9d4eea9e0226d93e1b6e9317e0c73c754699cc14.png", - "canonical_url": "http://localhost:3000/username372/east-of-eden168-21ml", - "created_at": "2022-08-23T12:08:21Z", + "cover_image": "http://localhost:3000/assets/18-0c8db7667732647d3000787a9481d38dc0dbe1b8ebc0b097db816f8db8cd097a.png", + "social_image": "http://localhost:3000/assets/18-0c8db7667732647d3000787a9481d38dc0dbe1b8ebc0b097db816f8db8cd097a.png", + "canonical_url": "http://localhost:3000/username372/the-yellow-meads-of-asphodel168-3bgf", + "created_at": "2022-08-30T09:45:05Z", "edited_at": null, "crossposted_at": null, - "published_at": "2022-08-23T12:08:21Z", - "last_comment_at": "2022-08-23T12:08:21Z", + "published_at": "2022-08-30T09:45:05Z", + "last_comment_at": "2022-08-30T09:45:05Z", "reading_time_minutes": 1, "tag_list": ["discuss"], "tags": "discuss", "user": { - "name": "Angelena \"Charis\" \\:/ Lang", + "name": "Jeffry \"Lon\" \\:/ Daniel", "username": "username372", "twitter_username": "twitter372", "github_username": "github372", - "user_id": 10747, + "user_id": 12567, "website_url": null, - "profile_image": "/uploads/user/profile_image/10747/79dbb51d-a63a-4475-af29-a84fe417b9d8.jpeg", - "profile_image_90": "/uploads/user/profile_image/10747/79dbb51d-a63a-4475-af29-a84fe417b9d8.jpeg" + "profile_image": "/uploads/user/profile_image/12567/d75daa5f-a5e3-4a5d-90e5-d2a06dba0890.jpeg", + "profile_image_90": "/uploads/user/profile_image/12567/d75daa5f-a5e3-4a5d-90e5-d2a06dba0890.jpeg" }, "organization": { - "name": "Vandervort-Sanford", + "name": "Abernathy and Sons", "username": "org56", "slug": "org56", - "profile_image": "/uploads/organization/profile_image/1275/45556538-13d9-44af-af7e-c9fcdbe2617c.png", - "profile_image_90": "/uploads/organization/profile_image/1275/45556538-13d9-44af-af7e-c9fcdbe2617c.png" + "profile_image": "/uploads/organization/profile_image/1473/9e5cefc8-2a19-45d2-91db-1d18f26a8cd2.png", + "profile_image_90": "/uploads/organization/profile_image/1473/9e5cefc8-2a19-45d2-91db-1d18f26a8cd2.png" }, "flare_tag": { "name": "discuss", @@ -213,7 +213,7 @@ }, "/api/reactions/toggle": { "post": { - "summary": "create reaction", + "summary": "toggle reaction", "tags": ["reactions"], "description": "This endpoint allows the client to toggle the user's reaction to a specified reactable (eg, Article, Comment, or User). For examples:\n * \"Like\"ing an Article will create a new \"like\" Reaction from the user for that Articles\n * \"Like\"ing that Article a second time will remove the \"like\" from the user", "parameters": [ @@ -253,8 +253,71 @@ "example": { "result": "create", "category": "like", - "id": 955, - "reactable_id": 4457, + "id": 1437, + "reactable_id": 5141, + "reactable_type": "Article" + } + } + } + }, + "401": { + "description": "unauthorized", + "content": { + "application/json": { + "example": { + "error": "unauthorized", + "status": 401 + } + } + } + } + } + } + }, + "/api/reactions": { + "post": { + "summary": "create reaction", + "tags": ["reactions"], + "description": "This endpoint allows the client to create a reaction to a specified reactable (eg, Article, Comment, or User). For examples:\n * \"Like\"ing an Article will create a new \"like\" Reaction from the user for that Articles\n * \"Like\"ing that Article a second time will return the previous \"like\"", + "parameters": [ + { + "name": "category", + "in": "query", + "required": true, + "schema": { + "type": "string", + "enum": ["like", "readinglist", "unicorn", "thinking", "hands"] + } + }, + { + "name": "reactable_id", + "in": "query", + "required": true, + "schema": { + "type": "integer", + "format": "int32" + } + }, + { + "name": "reactable_type", + "in": "query", + "required": true, + "schema": { + "type": "string", + "enum": ["Comment", "Article", "User"] + } + } + ], + "responses": { + "200": { + "description": "successful", + "content": { + "application/json": { + "example": { + "result": "none", + "category": "like", + "id": 1439, + "reactable_id": 5143, "reactable_type": "Article" } }