From 9e4a155562944050fa4735f0a511542633d7bd4a Mon Sep 17 00:00:00 2001 From: Mayur Shah Date: Fri, 10 Jan 2025 14:44:24 +0530 Subject: [PATCH 1/7] Added validation for subscribable(true) --- app/models/solidus_subscriptions/line_item.rb | 7 +++++++ config/locales/en.yml | 4 ++++ 2 files changed, 11 insertions(+) diff --git a/app/models/solidus_subscriptions/line_item.rb b/app/models/solidus_subscriptions/line_item.rb index 2583ecc5..556a105a 100644 --- a/app/models/solidus_subscriptions/line_item.rb +++ b/app/models/solidus_subscriptions/line_item.rb @@ -38,5 +38,12 @@ class LineItem < ApplicationRecord validates :subscribable_id, presence: true validates :quantity, numericality: { greater_than: 0 } validates :interval_length, numericality: { greater_than: 0 }, unless: -> { subscription } + validate :ensure_subscribable_is_true + + def ensure_subscribable_is_true + return unless subscribable && subscribable.subscribable != true + + errors.add(:subscribable, :cannot_subscribe) + end end end diff --git a/config/locales/en.yml b/config/locales/en.yml index c1c1fce3..f58c203c 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -136,3 +136,7 @@ en: not_active: "cannot pause/resume a subscription which is not active" state: cannot_skip: cannot skip a subscription which is canceled or inactive + solidus_subscriptions/line_item: + attributes: + subscribable: + cannot_subscribe: "The requested item cannot be subscribed" From 57b50b3efa2dd52715e8b88b33bd96cb7fc40625 Mon Sep 17 00:00:00 2001 From: Mayur Shah Date: Fri, 10 Jan 2025 14:45:30 +0530 Subject: [PATCH 2/7] Show/Hide subscribable form based on subscribable flag --- .../install/install_generator.rb | 4 ++ .../concerns/create_subscription.rb | 12 +++- .../_subscription_fields.html.erb | 71 +++++++++++-------- 3 files changed, 57 insertions(+), 30 deletions(-) diff --git a/lib/generators/solidus_subscriptions/install/install_generator.rb b/lib/generators/solidus_subscriptions/install/install_generator.rb index e9805d87..2081deab 100644 --- a/lib/generators/solidus_subscriptions/install/install_generator.rb +++ b/lib/generators/solidus_subscriptions/install/install_generator.rb @@ -30,6 +30,10 @@ def copy_starter_frontend_files RUBY end + + inject_into_file 'app/views/cart_line_items/_product_variants.html.erb', + " \"data-subscribable\" => variant.subscribable,\n", + before: " \"data-price\" => variant.price_for_options(current_pricing_options)&.money&.to_html\n" end def add_migrations diff --git a/lib/generators/solidus_subscriptions/install/templates/app/controllers/concerns/create_subscription.rb b/lib/generators/solidus_subscriptions/install/templates/app/controllers/concerns/create_subscription.rb index ed9cf2fb..433db814 100644 --- a/lib/generators/solidus_subscriptions/install/templates/app/controllers/concerns/create_subscription.rb +++ b/lib/generators/solidus_subscriptions/install/templates/app/controllers/concerns/create_subscription.rb @@ -5,7 +5,7 @@ module CreateSubscription include SolidusSubscriptions::SubscriptionLineItemBuilder included do - after_action :handle_subscription_line_items, only: :create, if: ->{ params[:subscription_line_item] } + after_action :handle_subscription_line_items, only: :create, if: :subscription_line_item_params_present? end private @@ -14,4 +14,14 @@ def handle_subscription_line_items line_item = @current_order.line_items.find_by(variant_id: params[:variant_id]) create_subscription_line_item(line_item) end + + def subscription_params + params.fetch(:subscription_line_item, {}) + end + + def subscription_line_item_params_present? + subscription_params[:subscribable_id].present? && + subscription_params[:quantity].present? && + subscription_params[:interval_length].present? + end end diff --git a/lib/generators/solidus_subscriptions/install/templates/app/views/cart_line_items/_subscription_fields.html.erb b/lib/generators/solidus_subscriptions/install/templates/app/views/cart_line_items/_subscription_fields.html.erb index dfda14f1..2fffa2f2 100644 --- a/lib/generators/solidus_subscriptions/install/templates/app/views/cart_line_items/_subscription_fields.html.erb +++ b/lib/generators/solidus_subscriptions/install/templates/app/views/cart_line_items/_subscription_fields.html.erb @@ -1,39 +1,52 @@ -<% if @product.subscribable %> - <%= content_tag :h3, t('.subscription_fields') %> - <%= fields_for :'subscription_line_item', SolidusSubscriptions::LineItem.new do |ff| %> -
- <%= ff.label :quantity, t('.quantity') %> - <%= ff.number_field :quantity %> - <%= ff.label :quantity, t('.quantity_suffix') %> -
- -
- <%= ff.label :interval_length, t('.interval_length') %> - <%= ff.number_field :interval_length %> - - <%= ff.collection_radio_buttons :interval_units, SolidusSubscriptions::LineItem.interval_units.to_a, :first, :first %> -
- - <%= ff.hidden_field :subscribable_id %> +
+ <% if @product.subscribable %> + <%= content_tag :h3, t('.subscription_fields') %> + <%= fields_for :'subscription_line_item', SolidusSubscriptions::LineItem.new do |ff| %> +
+ <%= ff.label :quantity, t('.quantity') %> + <%= ff.number_field :quantity %> + <%= ff.label :quantity, t('.quantity_suffix') %> +
+ +
+ <%= ff.label :interval_length, t('.interval_length') %> + <%= ff.number_field :interval_length %> + + <%= ff.collection_radio_buttons :interval_units, SolidusSubscriptions::LineItem.interval_units.to_a, :first, :first %> +
+ + <%= ff.hidden_field :subscribable_id %> + <% end %> <% end %> -<% end %> +
From 631aabd917617f41e47828093f6971e0e1d6f8b5 Mon Sep 17 00:00:00 2001 From: Mayur Shah Date: Fri, 10 Jan 2025 14:46:39 +0530 Subject: [PATCH 3/7] Added test cases --- .../concerns/create_subscription_spec.rb | 91 +++++++++++++++++++ .../spree/api/line_items_controller_spec.rb | 4 +- .../spree/api/orders_controller_spec.rb | 2 +- .../create_subscription_line_items_spec.rb | 2 +- .../solidus_subscriptions/line_item_spec.rb | 22 +++++ 5 files changed, 117 insertions(+), 4 deletions(-) create mode 100644 spec/controllers/concerns/create_subscription_spec.rb diff --git a/spec/controllers/concerns/create_subscription_spec.rb b/spec/controllers/concerns/create_subscription_spec.rb new file mode 100644 index 00000000..98b79302 --- /dev/null +++ b/spec/controllers/concerns/create_subscription_spec.rb @@ -0,0 +1,91 @@ +require 'spec_helper' +require_relative '../../../lib/generators/solidus_subscriptions/install/templates/app/controllers/concerns/create_subscription' + +RSpec.describe CreateSubscription, type: :controller do + subject(:controller_instance) do + Class.new(ApplicationController) do + include CreateSubscription + attr_accessor :params, :current_order + + def initialize(params = {}) + @params = params + @current_order = nil + end + end.new + end + + let(:variant) { create(:variant) } + let(:order) { create(:order) } + + before do + controller_instance.current_order = order + end + + describe '#subscription_line_item_params_present?' do + context 'when all required params are present' do + it 'returns true' do + controller_instance.params = { + subscription_line_item: { + subscribable_id: 1, + quantity: 2, + interval_length: 1 + } + } + expect(controller_instance.send(:subscription_line_item_params_present?)).to be true + end + end + + context 'when required params are missing' do + it 'returns false' do + controller_instance.params = { + subscription_line_item: { + subscribable_id: '', + quantity: '', + interval_length: '' + } + } + expect(controller_instance.send(:subscription_line_item_params_present?)).to be false + end + end + end + + describe '#handle_subscription_line_items' do + context 'when subscription params are missing' do + it 'does not invoke handle_subscription_line_items and does not create a subscription line item' do + initial_line_item_count = order.line_items.count + + controller_instance.params = { + variant_id: variant.id, + subscription_line_item: {} + } + + expect(controller_instance.send(:subscription_line_item_params_present?)).to be false + + expect(controller_instance).not_to receive(:handle_subscription_line_items) + + expect(controller_instance).not_to receive(:create_subscription_line_item) + end + end + + context 'when subscription params are present' do + it 'calls create_subscription_line_item with the correct line item' do + line_item = create(:line_item, order: order, variant: variant) + + controller_instance.params = { + variant_id: variant.id, + subscription_line_item: { + subscribable_id: 1, + quantity: 2, + interval_length: 1 + } + } + + allow(order.line_items).to receive(:find_by).with(variant_id: variant.id).and_return(line_item) + + expect(controller_instance).to receive(:create_subscription_line_item).with(line_item) + + controller_instance.send(:handle_subscription_line_items) + end + end + end +end diff --git a/spec/controllers/spree/api/line_items_controller_spec.rb b/spec/controllers/spree/api/line_items_controller_spec.rb index 478f9fdc..3293fc9a 100644 --- a/spec/controllers/spree/api/line_items_controller_spec.rb +++ b/spec/controllers/spree/api/line_items_controller_spec.rb @@ -11,7 +11,7 @@ subject(:post_create) { post :create, params: params } let(:params) { line_item_params } - let!(:variant) { create :variant } + let!(:variant) { create :variant, subscribable: true } let!(:order) { create :order } let(:line_item_params) do @@ -70,7 +70,7 @@ let(:params) { line_item_params } context 'when adding subscription information' do - let(:variant) { create :variant } + let(:variant) { create :variant, subscribable: true } let(:order) { create :order } let(:line_item) { create :line_item, order: order, variant: variant } let(:line_item_params) do diff --git a/spec/controllers/spree/api/orders_controller_spec.rb b/spec/controllers/spree/api/orders_controller_spec.rb index 5f32a569..81a0f234 100644 --- a/spec/controllers/spree/api/orders_controller_spec.rb +++ b/spec/controllers/spree/api/orders_controller_spec.rb @@ -8,7 +8,7 @@ routes { Spree::Core::Engine.routes } let(:order) { create :order } - let(:variant) { create :variant } + let(:variant) { create :variant, subscribable: true } describe 'patch /update' do subject(:subscription_line_items) do diff --git a/spec/decorators/controllers/solidus_subscriptions/spree/orders_controller/create_subscription_line_items_spec.rb b/spec/decorators/controllers/solidus_subscriptions/spree/orders_controller/create_subscription_line_items_spec.rb index 90bc9d17..67543923 100644 --- a/spec/decorators/controllers/solidus_subscriptions/spree/orders_controller/create_subscription_line_items_spec.rb +++ b/spec/decorators/controllers/solidus_subscriptions/spree/orders_controller/create_subscription_line_items_spec.rb @@ -16,7 +16,7 @@ describe 'POST /orders/populate' do subject(:populate) { post :populate, params: params } - let!(:variant) { create :variant } + let!(:variant) { create :variant, subscribable: true } let(:params) { line_item_params } let(:line_item_params) do { diff --git a/spec/models/solidus_subscriptions/line_item_spec.rb b/spec/models/solidus_subscriptions/line_item_spec.rb index d7c1bdba..de9bc3c2 100644 --- a/spec/models/solidus_subscriptions/line_item_spec.rb +++ b/spec/models/solidus_subscriptions/line_item_spec.rb @@ -26,4 +26,26 @@ expect(interval.from_now).to eq Date.parse("2016-10-22") end end + + describe "custom validation" do + context "when subscribable is not true" do + let(:subscribable) { create(:variant, subscribable: false) } + let(:line_item) { build(:subscription_line_item, subscribable: subscribable) } + + it "adds an error to subscribable" do + line_item.valid? + expect(line_item.errors[:subscribable]).to include("The requested item cannot be subscribed") + end + end + + context "when subscribable is true" do + let(:subscribable) { create(:variant, subscribable: true) } + let(:line_item) { build(:subscription_line_item, subscribable: subscribable) } + + it "does not add an error to subscribable" do + line_item.valid? + expect(line_item.errors[:subscribable]).to be_empty + end + end + end end From c9daca8f8860056597ad6b4b3d9eefa00081e7f5 Mon Sep 17 00:00:00 2001 From: Mayur Shah Date: Fri, 10 Jan 2025 16:30:57 +0530 Subject: [PATCH 4/7] Params improvements --- app/models/solidus_subscriptions/line_item.rb | 4 ++-- .../app/controllers/concerns/create_subscription.rb | 13 ++++--------- .../concerns/create_subscription_spec.rb | 6 +++--- 3 files changed, 9 insertions(+), 14 deletions(-) diff --git a/app/models/solidus_subscriptions/line_item.rb b/app/models/solidus_subscriptions/line_item.rb index 556a105a..efd9d662 100644 --- a/app/models/solidus_subscriptions/line_item.rb +++ b/app/models/solidus_subscriptions/line_item.rb @@ -38,9 +38,9 @@ class LineItem < ApplicationRecord validates :subscribable_id, presence: true validates :quantity, numericality: { greater_than: 0 } validates :interval_length, numericality: { greater_than: 0 }, unless: -> { subscription } - validate :ensure_subscribable_is_true + validate :ensure_subscribable_valid - def ensure_subscribable_is_true + def ensure_subscribable_valid return unless subscribable && subscribable.subscribable != true errors.add(:subscribable, :cannot_subscribe) diff --git a/lib/generators/solidus_subscriptions/install/templates/app/controllers/concerns/create_subscription.rb b/lib/generators/solidus_subscriptions/install/templates/app/controllers/concerns/create_subscription.rb index 433db814..f0f013b8 100644 --- a/lib/generators/solidus_subscriptions/install/templates/app/controllers/concerns/create_subscription.rb +++ b/lib/generators/solidus_subscriptions/install/templates/app/controllers/concerns/create_subscription.rb @@ -5,7 +5,7 @@ module CreateSubscription include SolidusSubscriptions::SubscriptionLineItemBuilder included do - after_action :handle_subscription_line_items, only: :create, if: :subscription_line_item_params_present? + after_action :handle_subscription_line_items, only: :create, if: :valid_subscription_line_item_params? end private @@ -15,13 +15,8 @@ def handle_subscription_line_items create_subscription_line_item(line_item) end - def subscription_params - params.fetch(:subscription_line_item, {}) - end - - def subscription_line_item_params_present? - subscription_params[:subscribable_id].present? && - subscription_params[:quantity].present? && - subscription_params[:interval_length].present? + def valid_subscription_line_item_params? + subscription_params = params[:subscription_line_item] + %i[subscribable_id quantity interval_length].all? { |key| subscription_params[key].present? } end end diff --git a/spec/controllers/concerns/create_subscription_spec.rb b/spec/controllers/concerns/create_subscription_spec.rb index 98b79302..3f9973cc 100644 --- a/spec/controllers/concerns/create_subscription_spec.rb +++ b/spec/controllers/concerns/create_subscription_spec.rb @@ -31,7 +31,7 @@ def initialize(params = {}) interval_length: 1 } } - expect(controller_instance.send(:subscription_line_item_params_present?)).to be true + expect(controller_instance.send(:valid_subscription_line_item_params?)).to be true end end @@ -44,7 +44,7 @@ def initialize(params = {}) interval_length: '' } } - expect(controller_instance.send(:subscription_line_item_params_present?)).to be false + expect(controller_instance.send(:valid_subscription_line_item_params?)).to be false end end end @@ -59,7 +59,7 @@ def initialize(params = {}) subscription_line_item: {} } - expect(controller_instance.send(:subscription_line_item_params_present?)).to be false + expect(controller_instance.send(:valid_subscription_line_item_params?)).to be false expect(controller_instance).not_to receive(:handle_subscription_line_items) From 6df03f23176924300667edd0e6cb2e8598540b9a Mon Sep 17 00:00:00 2001 From: Mayur Shah Date: Fri, 10 Jan 2025 17:19:44 +0530 Subject: [PATCH 5/7] check the existence of legacy promotion --- db/migrate/20210323165714_update_promotion_rule_names.rb | 2 ++ lib/solidus_subscriptions/engine.rb | 8 +++++--- 2 files changed, 7 insertions(+), 3 deletions(-) diff --git a/db/migrate/20210323165714_update_promotion_rule_names.rb b/db/migrate/20210323165714_update_promotion_rule_names.rb index f6ee6c23..1adfe1b6 100644 --- a/db/migrate/20210323165714_update_promotion_rule_names.rb +++ b/db/migrate/20210323165714_update_promotion_rule_names.rb @@ -5,6 +5,8 @@ class UpdatePromotionRuleNames < ActiveRecord::Migration[5.2] }.freeze def change + return unless Object.const_defined?("Spree::Promotion") + reversible do |dir| dir.up do TYPE_RENAMES.each do |old_type, new_type| diff --git a/lib/solidus_subscriptions/engine.rb b/lib/solidus_subscriptions/engine.rb index 682ebccc..f8fd7c29 100644 --- a/lib/solidus_subscriptions/engine.rb +++ b/lib/solidus_subscriptions/engine.rb @@ -31,9 +31,11 @@ class Engine < Rails::Engine } end - initializer 'solidus_subscriptions.register_promotion_rules', after: 'spree.promo.register.promotion.rules' do |app| - app.config.spree.promotions.rules << 'SolidusSubscriptions::Promotion::Rules::SubscriptionCreationOrder' - app.config.spree.promotions.rules << 'SolidusSubscriptions::Promotion::Rules::SubscriptionInstallmentOrder' + if Object.const_defined?("Spree::Promotion") + initializer 'solidus_subscriptions.register_promotion_rules', after: 'spree.promo.register.promotion.rules' do |app| + app.config.spree.promotions.rules << 'SolidusSubscriptions::Promotion::Rules::SubscriptionCreationOrder' + app.config.spree.promotions.rules << 'SolidusSubscriptions::Promotion::Rules::SubscriptionInstallmentOrder' + end end initializer 'solidus_subscriptions.configure_backend' do From 936ce21bf72a32c39c1c5dc9122ca7cba13901d4 Mon Sep 17 00:00:00 2001 From: Mayur Shah Date: Sat, 11 Jan 2025 21:11:57 +0530 Subject: [PATCH 6/7] Linting --- app/models/solidus_subscriptions/subscription.rb | 3 +-- spec/controllers/concerns/create_subscription_spec.rb | 2 +- .../solidus_subscriptions/process_subscription_job_spec.rb | 2 +- 3 files changed, 3 insertions(+), 4 deletions(-) diff --git a/app/models/solidus_subscriptions/subscription.rb b/app/models/solidus_subscriptions/subscription.rb index d261d6e4..62a4db75 100644 --- a/app/models/solidus_subscriptions/subscription.rb +++ b/app/models/solidus_subscriptions/subscription.rb @@ -434,12 +434,11 @@ def emit_events_for_update end def self.ransackable_attributes(_auth_object = nil) - %w[actionable_date created_at end_date state updated_at user_id] + %w[actionable_date created_at end_date state updated_at user_id] end def self.ransackable_associations(_auth_object = nil) %w[events user] end - end end diff --git a/spec/controllers/concerns/create_subscription_spec.rb b/spec/controllers/concerns/create_subscription_spec.rb index 3f9973cc..59338b83 100644 --- a/spec/controllers/concerns/create_subscription_spec.rb +++ b/spec/controllers/concerns/create_subscription_spec.rb @@ -52,7 +52,7 @@ def initialize(params = {}) describe '#handle_subscription_line_items' do context 'when subscription params are missing' do it 'does not invoke handle_subscription_line_items and does not create a subscription line item' do - initial_line_item_count = order.line_items.count + order.line_items.count controller_instance.params = { variant_id: variant.id, diff --git a/spec/jobs/solidus_subscriptions/process_subscription_job_spec.rb b/spec/jobs/solidus_subscriptions/process_subscription_job_spec.rb index 1a3cf6fe..511c34af 100644 --- a/spec/jobs/solidus_subscriptions/process_subscription_job_spec.rb +++ b/spec/jobs/solidus_subscriptions/process_subscription_job_spec.rb @@ -5,7 +5,7 @@ it 'voids the actionable date of the unfulfilled installments' do stub_config(clear_past_installments: true) subscription = create(:subscription) - unfulfilled_installment = create(:installment, :failed, subscription: subscription) + unfulfilled_installment = create(:installment, :failed, subscription: subscription) described_class.perform_now(subscription) From 188fe8e2be37b6134670a2f5064bcea3afabeed2 Mon Sep 17 00:00:00 2001 From: Mayur Shah Date: Sat, 11 Jan 2025 21:26:23 +0530 Subject: [PATCH 7/7] Trying to update solidus_support --- solidus_subscriptions.gemspec | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/solidus_subscriptions.gemspec b/solidus_subscriptions.gemspec index 47e3f9cd..9659c349 100644 --- a/solidus_subscriptions.gemspec +++ b/solidus_subscriptions.gemspec @@ -33,7 +33,7 @@ Gem::Specification.new do |spec| spec.add_dependency 'httparty', '~> 0.18' spec.add_dependency 'i18n' spec.add_dependency 'solidus_core', '>= 2.11', '< 5' - spec.add_dependency 'solidus_support', '~> 0.9' + spec.add_dependency 'solidus_support', '~> 0.11' spec.add_dependency 'state_machines' spec.add_development_dependency 'rspec-activemodel-mocks'