From 60639bcb71fa063e83f17fb2e12436ddff52eb6e Mon Sep 17 00:00:00 2001 From: Michael Kohl Date: Thu, 27 Jan 2022 09:47:41 +0700 Subject: [PATCH] More `ListingsToolkit` refactoring (#16221) --- app/controllers/admin/listings_controller.rb | 16 +++-- app/controllers/concerns/listings_toolkit.rb | 72 +++---------------- app/models/listing.rb | 16 +++++ app/services/credits/{buyer.rb => buy.rb} | 18 +---- app/services/listings/bump.rb | 38 ++++++++++ app/services/listings/create.rb | 2 +- spec/requests/api/v0/listings_spec.rb | 6 +- spec/requests/listings_spec.rb | 6 +- .../credits/{buyer_spec.rb => buy_spec.rb} | 2 +- .../dashboards/user_visits_dashboard_spec.rb | 2 +- 10 files changed, 88 insertions(+), 90 deletions(-) rename app/services/credits/{buyer.rb => buy.rb} (54%) create mode 100644 app/services/listings/bump.rb rename spec/services/credits/{buyer_spec.rb => buy_spec.rb} (97%) diff --git a/app/controllers/admin/listings_controller.rb b/app/controllers/admin/listings_controller.rb index b5cffc7f3..148df4eb3 100644 --- a/app/controllers/admin/listings_controller.rb +++ b/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 diff --git a/app/controllers/concerns/listings_toolkit.rb b/app/controllers/concerns/listings_toolkit.rb index 07393c498..c3aecf653 100644 --- a/app/controllers/concerns/listings_toolkit.rb +++ b/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 diff --git a/app/models/listing.rb b/app/models/listing.rb index 2b81eef90..2ac91fe37 100644 --- a/app/models/listing.rb +++ b/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 diff --git a/app/services/credits/buyer.rb b/app/services/credits/buy.rb similarity index 54% rename from app/services/credits/buyer.rb rename to app/services/credits/buy.rb index bdd898a60..8acdced5c 100644 --- a/app/services/credits/buyer.rb +++ b/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 diff --git a/app/services/listings/bump.rb b/app/services/listings/bump.rb new file mode 100644 index 000000000..ae371f6e3 --- /dev/null +++ b/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 diff --git a/app/services/listings/create.rb b/app/services/listings/create.rb index e6458c843..455b0a0e9 100644 --- a/app/services/listings/create.rb +++ b/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, diff --git a/spec/requests/api/v0/listings_spec.rb b/spec/requests/api/v0/listings_spec.rb index a52ad7917..a1559ae2b 100644 --- a/spec/requests/api/v0/listings_spec.rb +++ b/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) diff --git a/spec/requests/listings_spec.rb b/spec/requests/listings_spec.rb index 06a7f5dc9..7aeeb8ec2 100644 --- a/spec/requests/listings_spec.rb +++ b/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) diff --git a/spec/services/credits/buyer_spec.rb b/spec/services/credits/buy_spec.rb similarity index 97% rename from spec/services/credits/buyer_spec.rb rename to spec/services/credits/buy_spec.rb index 6c2e07e5f..0c428043f 100644 --- a/spec/services/credits/buyer_spec.rb +++ b/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) } diff --git a/spec/system/dashboards/user_visits_dashboard_spec.rb b/spec/system/dashboards/user_visits_dashboard_spec.rb index b8aabcfa4..aef259c64 100644 --- a/spec/system/dashboards/user_visits_dashboard_spec.rb +++ b/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,