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 <Ridhwana.Khan16@gmail.com>

Co-authored-by: Ridhwana <Ridhwana.Khan16@gmail.com>

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: {

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 }

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

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

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

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

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

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"
                 }
               }