More ListingsToolkit refactoring (#16221)

app/controllers/admin/listings_controller.rb

@@ -1,6 +1,5 @@
 module Admin
   class ListingsController < Admin::ApplicationController
-    include ListingsToolkit
     ALLOWED_PARAMS = %i[
       published body_markdown title category listing_category_id tag_list action organization_id
     ].freeze
@@ -22,9 +21,14 @@ module Admin
     def update
       @listing = Listing.find(params[:id])
       handle_publish_status if listing_params[:published]
-      bump_listing(@listing.cost) if listing_params[:action] == "bump"
-      update_listing_details
-      clear_listings_cache
+
+      if listing_params[:action] == "bump"
+        bump_success = Listings::Bump.call(@listing, user: current_user)
+        return process_no_credit_left unless bump_success
+      end
+
+      @listing.update(listing_params.compact)
+      @listing.clear_cache
       flash[:success] = "Listing updated successfully"
       redirect_to edit_admin_listing_path(@listing)
     end
@@ -52,5 +56,9 @@ module Admin
     def include_unpublished?
       params[:include_unpublished] == "1"
     end
+
+    def process_no_credit_left
+      redirect_to admin_listings_path, notice: "Not enough available credits"
+    end
   end
 end

app/controllers/concerns/listings_toolkit.rb

@@ -4,31 +4,6 @@ module ListingsToolkit
 
   MANDATORY_FIELDS_FOR_UPDATE = %i[body_markdown title tag_list].freeze
 
-  def unpublish_listing
-    @listing.update(published: false)
-  end
-
-  def publish_listing
-    @listing.update(published: true)
-  end
-
-  def update_listing_details
-    # [@forem/oss] Not entirely sure what the intention behind the
-    # original code was, but at least this is more compact.
-
-    filtered_params = listing_params.compact
-
-    @listing.update(filtered_params)
-  end
-
-  def bump_listing_success
-    @listing.update(bumped_at: Time.current)
-  end
-
-  def clear_listings_cache
-    Listings::BustCacheWorker.perform_async(@listing.id)
-  end
-
   def set_listing
     @listing = Listing.find(params[:id])
   end
@@ -106,7 +81,7 @@ module ListingsToolkit
 
     if create_result.success?
       rate_limiter.track_limit_by_action(:listing_creation)
-      clear_listings_cache
+      @listing.clear_cache
       process_successful_creation
     else
       @credits = current_user.credits.unspent
@@ -119,55 +94,32 @@ module ListingsToolkit
   def update
     authorize @listing
 
-    cost = @listing.cost
-
     # NOTE: this should probably be split in three different actions: bump, unpublish, publish
-    return bump_listing(cost) if listing_params[:action] == "bump"
+    if listing_params[:action] == "bump"
+      bump_result = Listings::Bump.call(@listing, user: current_user)
+      return process_no_credit_left unless bump_result
+    end
 
     if listing_params[:action] == "unpublish"
-      unpublish_listing
+      @listing.unpublish
       process_after_unpublish
       return
     elsif listing_params[:action] == "publish"
       unless @listing.bumped_at?
-        first_publish(cost)
+        first_publish(@listing.cost)
         return
       end
 
-      publish_listing
+      @listing.publish
     elsif listing_updatable?
-      saved = update_listing_details
+      saved = @listing.update(listing_params.compact)
       return process_unsuccessful_update unless saved
     end
 
-    clear_listings_cache
+    @listing.clear_cache
     process_after_update
   end
 
-  def bump_listing(cost)
-    org = Organization.find_by(id: @listing.organization_id)
-
-    if org&.enough_credits?(cost)
-      charge_credits_before_bump(org, cost)
-    elsif current_user.enough_credits?(cost)
-      charge_credits_before_bump(current_user, cost)
-    else
-      process_no_credit_left && return
-    end
-  end
-
-  def charge_credits_before_bump(purchaser, cost)
-    ActiveRecord::Base.transaction do
-      Credits::Buyer.call(
-        purchaser: purchaser,
-        purchase: @listing,
-        cost: cost,
-      )
-
-      raise ActiveRecord::Rollback unless bump_listing_success
-    end
-  end
-
   def first_publish(cost)
     author = @listing.author
     available_user_credits = author.is_a?(Organization) ? current_user.credits.unspent.size : 0
@@ -190,8 +142,6 @@ module ListingsToolkit
   end
 
   def bumped_in_last_24_hrs?
-    return unless (last_bumped_at = @listing.bumped_at)
-
-    last_bumped_at > 24.hours.ago
+    @listing.bumped_at&.after?(24.hours.ago)
   end
 end

app/models/listing.rb

@@ -65,6 +65,22 @@ class Listing < ApplicationRecord
     (bumped_at || created_at) + 30.days
   end
 
+  def publish
+    update(published: true)
+  end
+
+  def unpublish
+    update(published: false)
+  end
+
+  def bump
+    update(bumped_at: Time.current)
+  end
+
+  def clear_cache
+    Listings::BustCacheWorker.perform_async(id)
+  end
+
   private
 
   def evaluate_markdown

app/services/credits/buy.rb

@@ -1,16 +1,6 @@
 module Credits
-  class Buyer
-    def initialize(purchaser:, purchase:, cost:)
-      @purchaser = purchaser
-      @purchase = purchase
-      @cost = cost
-    end
-
-    def self.call(...)
-      new(...).call
-    end
-
-    def call
+  class Buy
+    def self.call(purchaser:, purchase:, cost:)
       return false unless purchaser.enough_credits?(cost)
 
       purchaser.credits.unspent.limit(cost).update_all(
@@ -23,9 +13,5 @@ module Credits
 
       true
     end
-
-    private
-
-    attr_reader :purchaser, :purchase, :cost
   end
 end

app/services/listings/bump.rb

@@ -0,0 +1,38 @@
+module Listings
+  class Bump
+    def self.call(listing, user:)
+      new(listing, user: user).call
+    end
+
+    def initialize(listing, user:)
+      @listing = listing
+      @cost = listing.cost
+      @user = user
+      @org = Organization.find_by(id: listing.organization_id)
+    end
+
+    def call
+      purchaser = [@org, @user].detect { |who| who&.enough_credits?(@cost) }
+      return false unless purchaser
+
+      charge_credits_before_bump(purchaser)
+      true
+    end
+
+    private
+
+    def charge_credits_before_bump(purchaser)
+      ActiveRecord::Base.transaction do
+        enough_credits = Credits::Buy.call(
+          purchaser: purchaser,
+          purchase: @listing,
+          cost: @cost,
+        )
+
+        unless enough_credits && @listing.bump
+          raise ActiveRecord::Rollback
+        end
+      end
+    end
+  end
+end

app/services/listings/create.rb

@@ -20,7 +20,7 @@ module Listings
     def call
       ActiveRecord::Base.transaction do
         # Subtract credits
-        enough_credits = Credits::Buyer.call(
+        enough_credits = Credits::Buy.call(
           purchaser: @purchaser,
           purchase: @listing,
           cost: @cost,

spec/requests/api/v0/listings_spec.rb

@@ -373,7 +373,7 @@ RSpec.describe "Api::V0::Listings", type: :request do
       end
 
       it "creates listing draft and does not subtract credits" do
-        allow(Credits::Buyer).to receive(:call).and_raise(ActiveRecord::Rollback)
+        allow(Credits::Buy).to receive(:call).and_raise(ActiveRecord::Rollback)
         expect do
           post_listing(**draft_params)
         end.to change(Listing, :count).by(1)
@@ -381,7 +381,7 @@ RSpec.describe "Api::V0::Listings", type: :request do
       end
 
       it "does not create a listing or subtract credits if the purchase does not go through" do
-        allow(Credits::Buyer).to receive(:call).and_raise(ActiveRecord::Rollback)
+        allow(Credits::Buy).to receive(:call).and_raise(ActiveRecord::Rollback)
         expect do
           post_listing(**listing_params)
         end.to change(Listing, :count).by(0)
@@ -491,7 +491,7 @@ RSpec.describe "Api::V0::Listings", type: :request do
 
       it "does not bump the listing or subtract credits if the purchase does not go through" do
         previous_bumped_at = listing.bumped_at
-        allow(Credits::Buyer).to receive(:call).and_raise(ActiveRecord::Rollback)
+        allow(Credits::Buy).to receive(:call).and_raise(ActiveRecord::Rollback)
         expect do
           put_listing(listing.id, action: "bump")
         end.to change(user.credits.spent, :size).by(0)

spec/requests/listings_spec.rb

@@ -314,7 +314,7 @@ RSpec.describe "/listings", type: :request do
       end
 
       it "creates listing draft and does not subtract credits" do
-        allow(Credits::Buyer).to receive(:call).and_raise(ActiveRecord::Rollback)
+        allow(Credits::Buy).to receive(:call).and_raise(ActiveRecord::Rollback)
         expect do
           post "/listings", params: draft_params
         end.to change(Listing, :count).by(1)
@@ -322,7 +322,7 @@ RSpec.describe "/listings", type: :request do
       end
 
       it "does not create a listing or subtract credits if the purchase does not go through" do
-        allow(Credits::Buyer).to receive(:call).and_raise(ActiveRecord::Rollback)
+        allow(Credits::Buy).to receive(:call).and_raise(ActiveRecord::Rollback)
         expect do
           post "/listings", params: listing_params
         end.to change(Listing, :count).by(0)
@@ -393,7 +393,7 @@ RSpec.describe "/listings", type: :request do
 
       it "does not bump the listing or subtract credits if the purchase does not go through" do
         previous_bumped_at = listing.bumped_at
-        allow(Credits::Buyer).to receive(:call).and_raise(ActiveRecord::Rollback)
+        allow(Credits::Buy).to receive(:call).and_raise(ActiveRecord::Rollback)
         expect do
           put "/listings/#{listing.id}", params: params
         end.to change(user.credits.spent, :size).by(0)

spec/services/credits/buy_spec.rb

@@ -1,6 +1,6 @@
 require "rails_helper"
 
-RSpec.describe Credits::Buyer, type: :service do
+RSpec.describe Credits::Buy, type: :service do
   let(:user) { create(:user) }
   let(:org) { create(:organization) }
   let(:listing) { create(:listing, user: user) }

spec/system/dashboards/user_visits_dashboard_spec.rb

@@ -12,7 +12,7 @@ RSpec.describe "Dashboard", type: :system, js: true do
     it "shows the count of unspent credits" do
       Credit.add_to(user, 2)
 
-      Credits::Buyer.call(
+      Credits::Buy.call(
         purchaser: user,
         purchase: listing,
         cost: 1,