From 68e9ff7b4adde5e9479fde7ea0cbce3d15a5f9fe Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 13 Feb 2025 12:01:35 +0100 Subject: [PATCH 1/2] Enable linting workflow This also updates the `.rubocop_todo.yml` and fixes some easy-to-fix Rubocop offenses. --- .github/workflows/lint.yml | 25 ++++++ .rubocop_todo.yml | 78 +++++++------------ .../solidus_subscriptions/installment.rb | 8 +- .../solidus_subscriptions/subscription.rb | 16 ++-- .../concerns/create_subscription_spec.rb | 3 +- spec/features/admin/subscriptions_spec.rb | 2 +- .../process_installment_job_spec.rb | 2 +- .../solidus_subscriptions/checkout_spec.rb | 2 - spec/requests/api/v1/subscriptions_spec.rb | 4 - 9 files changed, 69 insertions(+), 71 deletions(-) create mode 100644 .github/workflows/lint.yml diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml new file mode 100644 index 00000000..dc2a0cfa --- /dev/null +++ b/.github/workflows/lint.yml @@ -0,0 +1,25 @@ +name: Lint + +on: [pull_request] + +concurrency: + group: lint-${{ github.ref_name }} + cancel-in-progress: ${{ github.ref_name != 'main' }} + +permissions: + contents: read + +jobs: + ruby: + name: Check Ruby + runs-on: ubuntu-24.04 + steps: + - name: Checkout code + uses: actions/checkout@v3 + - name: Install Ruby and gems + uses: ruby/setup-ruby@v1 + with: + ruby-version: "3.2" + bundler-cache: true + - name: Lint Ruby files + run: bundle exec rubocop -ESP diff --git a/.rubocop_todo.yml b/.rubocop_todo.yml index 195ab44e..009de7e8 100644 --- a/.rubocop_todo.yml +++ b/.rubocop_todo.yml @@ -1,37 +1,41 @@ # This configuration was generated by # `rubocop --auto-gen-config` -# on 2020-09-24 12:23:52 UTC using RuboCop version 0.91.1. +# on 2025-02-13 10:58:32 UTC using RuboCop version 1.71.2. # The point is for the user to remove these configuration records # one by one as the offenses are removed from the code base. # Note that changes in the inspected code, or installation of new # versions of RuboCop, may require this file to be generated again. -# Offense count: 1 -Lint/MissingSuper: - Exclude: - - 'app/services/solidus_subscriptions/user_mismatch_error.rb' - -# Offense count: 1 -# Configuration parameters: EnforcedStyleForLeadingUnderscores. -# SupportedStylesForLeadingUnderscores: disallowed, required, optional -Naming/MemoizedInstanceVariableName: +# Offense count: 3 +# Configuration parameters: EnforcedStyle. +# SupportedStyles: have_received, receive +RSpec/MessageSpies: Exclude: - - 'lib/solidus_subscriptions/processor.rb' + - 'spec/controllers/concerns/create_subscription_spec.rb' -# Offense count: 2 +# Offense count: 9 RSpec/MultipleExpectations: - Max: 2 + Max: 7 -# Offense count: 90 -# Configuration parameters: IgnoreSharedExamples. +# Offense count: 24 +# Configuration parameters: EnforcedStyle, IgnoreSharedExamples. +# SupportedStyles: always, named_only RSpec/NamedSubject: - Enabled: false + Exclude: + - 'spec/decorators/models/solidus_subscriptions/spree/variant/auto_delete_from_subscriptions_spec.rb' + - 'spec/models/solidus_subscriptions/subscription_spec.rb' -# Offense count: 11 +# Offense count: 24 +# Configuration parameters: AllowedGroups. RSpec/NestedGroups: Max: 4 -# Offense count: 4 +# Offense count: 3 +RSpec/SubjectStub: + Exclude: + - 'spec/controllers/concerns/create_subscription_spec.rb' + +# Offense count: 5 # Configuration parameters: Include. # Include: app/models/**/*.rb Rails/HasManyOrHasOneDependent: @@ -39,25 +43,8 @@ Rails/HasManyOrHasOneDependent: - 'app/models/solidus_subscriptions/installment.rb' - 'app/models/solidus_subscriptions/subscription.rb' -# Offense count: 2 -# Configuration parameters: Include. -# Include: db/migrate/*.rb -Rails/ReversibleMigration: - Exclude: - - 'db/migrate/20160922164101_add_interval_length_and_units_to_subscription_line_items.rb' - - 'db/migrate/20170106224713_change_line_item_max_installments_to_end_date.rb' - -# Offense count: 5 -# Configuration parameters: ForbiddenMethods, AllowedMethods. -# ForbiddenMethods: decrement!, decrement_counter, increment!, increment_counter, insert, insert!, insert_all, insert_all!, toggle!, touch, touch_all, update_all, update_attribute, update_column, update_columns, update_counters, upsert, upsert_all -Rails/SkipsModelValidations: - Exclude: - - 'app/services/solidus_subscriptions/failure_dispatcher.rb' - - 'app/services/solidus_subscriptions/order_builder.rb' - - 'app/services/solidus_subscriptions/payment_failed_dispatcher.rb' - # Offense count: 1 -# Cop supports --auto-correct. +# This cop supports safe autocorrection (--autocorrect). # Configuration parameters: EnforcedStyle, SingleLineConditionsOnly, IncludeTernaryExpressions. # SupportedStyles: assign_to_condition, assign_inside_condition Style/ConditionalAssignment: @@ -65,22 +52,15 @@ Style/ConditionalAssignment: - 'app/controllers/spree/admin/subscriptions_controller.rb' # Offense count: 4 -# Configuration parameters: MinBodyLength. +# This cop supports safe autocorrection (--autocorrect). +# Configuration parameters: MinBodyLength, AllowConsecutiveConditionals. Style/GuardClause: Exclude: - 'app/models/solidus_subscriptions/subscription.rb' -# Offense count: 1 -# Cop supports --auto-correct. -# Configuration parameters: EnforcedStyle. -# SupportedStyles: compact, exploded -Style/RaiseArgs: - Exclude: - - 'app/services/solidus_subscriptions/checkout.rb' - -# Offense count: 11 -# Cop supports --auto-correct. -# Configuration parameters: AutoCorrect, AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, IgnoredPatterns. +# Offense count: 25 +# This cop supports safe autocorrection (--autocorrect). +# Configuration parameters: AllowHeredoc, AllowURI, URISchemes, IgnoreCopDirectives, AllowedPatterns, SplitStrings. # URISchemes: http, https Layout/LineLength: - Max: 290 + Max: 145 diff --git a/app/models/solidus_subscriptions/installment.rb b/app/models/solidus_subscriptions/installment.rb index 69d99c67..14a92e8d 100644 --- a/app/models/solidus_subscriptions/installment.rb +++ b/app/models/solidus_subscriptions/installment.rb @@ -30,6 +30,10 @@ class Installment < ApplicationRecord unfulfilled.where("#{table_name}.actionable_date <= ?", Time.zone.today) end) + def self.ransackable_attributes(_auth_object = nil) + %w[actionable_date created_at updated_at] + end + # Mark this installment as out of stock. # # @return [SolidusSubscriptions::InstallmentDetail] The record of the failed @@ -131,9 +135,5 @@ def next_actionable_date (DateTime.current + SolidusSubscriptions.configuration.reprocessing_interval).beginning_of_minute end - - def self.ransackable_attributes(_auth_object = nil) - %w[actionable_date created_at updated_at] - end end end diff --git a/app/models/solidus_subscriptions/subscription.rb b/app/models/solidus_subscriptions/subscription.rb index 62a4db75..0f9ffcc3 100644 --- a/app/models/solidus_subscriptions/subscription.rb +++ b/app/models/solidus_subscriptions/subscription.rb @@ -92,6 +92,14 @@ def self.ransackable_scopes(_auth_object = nil) [:in_processing_state, :with_subscribable] end + def self.ransackable_attributes(_auth_object = nil) + %w[actionable_date created_at end_date state updated_at user_id] + end + + def self.ransackable_associations(_auth_object = nil) + %w[events user] + end + def self.processing_states PROCESSING_STATES end @@ -432,13 +440,5 @@ def emit_events_for_update emit_event(type: 'subscription_payment_method_changed') end end - - def self.ransackable_attributes(_auth_object = nil) - %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 59338b83..99274c30 100644 --- a/spec/controllers/concerns/create_subscription_spec.rb +++ b/spec/controllers/concerns/create_subscription_spec.rb @@ -8,8 +8,7 @@ attr_accessor :params, :current_order def initialize(params = {}) - @params = params - @current_order = nil + super end end.new end diff --git a/spec/features/admin/subscriptions_spec.rb b/spec/features/admin/subscriptions_spec.rb index e975cd60..32f36ead 100644 --- a/spec/features/admin/subscriptions_spec.rb +++ b/spec/features/admin/subscriptions_spec.rb @@ -25,7 +25,7 @@ expect(subscription.billing_address.zipcode).to eq('33167') end - it 'Creates a subscription' do # rubocop:disable RSpec/MultipleExpectations + it 'Creates a subscription' do variant = create(:variant, subscribable: true) create(:user) create(:store) diff --git a/spec/jobs/solidus_subscriptions/process_installment_job_spec.rb b/spec/jobs/solidus_subscriptions/process_installment_job_spec.rb index 767d6506..b0a1460c 100644 --- a/spec/jobs/solidus_subscriptions/process_installment_job_spec.rb +++ b/spec/jobs/solidus_subscriptions/process_installment_job_spec.rb @@ -12,7 +12,7 @@ end context 'when handling #perform errors' do - it 'by default logs exception data without raising exceptions' do # rubocop:disable RSpec/MultipleExpectations + it 'by default logs exception data without raising exceptions' do installment = build_stubbed(:installment) checkout = instance_double(SolidusSubscriptions::Checkout).tap do |c| allow(c).to receive(:process).and_raise('test error') diff --git a/spec/lib/solidus_subscriptions/checkout_spec.rb b/spec/lib/solidus_subscriptions/checkout_spec.rb index e7c18fa1..d15a99de 100644 --- a/spec/lib/solidus_subscriptions/checkout_spec.rb +++ b/spec/lib/solidus_subscriptions/checkout_spec.rb @@ -2,7 +2,6 @@ RSpec.describe SolidusSubscriptions::Checkout, :checkout do context 'when the order can be created and paid' do - # rubocop:disable RSpec/MultipleExpectations it 'creates and finalizes a new order for the installment' do stub_spree_preferences(auto_capture: true) installment = create(:installment, :actionable) @@ -39,7 +38,6 @@ expect(order.subscription).to eq(subscription) expect(order.subscription_order).to eq(true) end - # rubocop:enable RSpec/MultipleExpectations it 'matches the total on the subscription' do stub_spree_preferences(auto_capture: true) diff --git a/spec/requests/api/v1/subscriptions_spec.rb b/spec/requests/api/v1/subscriptions_spec.rb index c90fa977..30b47fb9 100644 --- a/spec/requests/api/v1/subscriptions_spec.rb +++ b/spec/requests/api/v1/subscriptions_spec.rb @@ -37,7 +37,6 @@ end context 'when valid payment attributes are provided' do - # rubocop:disable RSpec/MultipleExpectations it 'creates the subscription using the specified payment' do user = create(:user, &:generate_spree_api_key!) payment_source = create(:credit_card, user: user) @@ -53,10 +52,8 @@ end.to change(SolidusSubscriptions::Subscription, :count).from(0).to(1) expect(SolidusSubscriptions::Subscription.last).to have_attributes(payment_params) end - # rubocop:enable RSpec/MultipleExpectations end - # rubocop:disable RSpec/MultipleExpectations context 'when an invalid payment method is provided' do it "doesn't create the subscription and responds with 422 Unprocessable Entity" do user = create(:user, &:generate_spree_api_key!) @@ -98,7 +95,6 @@ expect(response.status).to eq(422) end end - # rubocop:enable RSpec/MultipleExpectations end describe 'PATCH /:id' do From 30c451bda1ab4ce3d6a212b85531ca7dc0991be2 Mon Sep 17 00:00:00 2001 From: Martin Meyerhoff Date: Thu, 13 Feb 2025 12:12:10 +0100 Subject: [PATCH 2/2] Use current order helper instead of instance variable The API to get the current order in a controller is the helper, not the instance variable. --- .../create_subscription_line_items.rb | 2 +- .../concerns/create_subscription.rb | 2 +- .../concerns/create_subscription_spec.rb | 29 ++++++++++--------- 3 files changed, 17 insertions(+), 16 deletions(-) diff --git a/lib/decorators/frontend/controllers/solidus_subscriptions/spree/orders_controller/create_subscription_line_items.rb b/lib/decorators/frontend/controllers/solidus_subscriptions/spree/orders_controller/create_subscription_line_items.rb index 6b39b4cd..c6cb6ffa 100644 --- a/lib/decorators/frontend/controllers/solidus_subscriptions/spree/orders_controller/create_subscription_line_items.rb +++ b/lib/decorators/frontend/controllers/solidus_subscriptions/spree/orders_controller/create_subscription_line_items.rb @@ -24,7 +24,7 @@ def self.prepended(base) private def handle_subscription_line_items - line_item = @current_order.line_items.find_by(variant_id: params[:variant_id]) + line_item = current_order.line_items.find_by(variant_id: params[:variant_id]) create_subscription_line_item(line_item) end end 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 f0f013b8..f3a9d46e 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 @@ -11,7 +11,7 @@ module CreateSubscription private def handle_subscription_line_items - line_item = @current_order.line_items.find_by(variant_id: params[:variant_id]) + line_item = current_order.line_items.find_by(variant_id: params[:variant_id]) create_subscription_line_item(line_item) end diff --git a/spec/controllers/concerns/create_subscription_spec.rb b/spec/controllers/concerns/create_subscription_spec.rb index 99274c30..cfe1136d 100644 --- a/spec/controllers/concerns/create_subscription_spec.rb +++ b/spec/controllers/concerns/create_subscription_spec.rb @@ -5,11 +5,6 @@ subject(:controller_instance) do Class.new(ApplicationController) do include CreateSubscription - attr_accessor :params, :current_order - - def initialize(params = {}) - super - end end.new end @@ -17,7 +12,7 @@ def initialize(params = {}) let(:order) { create(:order) } before do - controller_instance.current_order = order + allow(controller_instance).to receive(:current_order).and_return(order) end describe '#subscription_line_item_params_present?' do @@ -49,15 +44,19 @@ def initialize(params = {}) 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 - order.line_items.count + before do + allow(controller_instance).to receive(:params).and_return(params) + end - controller_instance.params = { + context 'when subscription params are missing' do + let(:params) do + { variant_id: variant.id, subscription_line_item: {} } + end + it 'does not invoke handle_subscription_line_items and does not create a subscription line item' do expect(controller_instance.send(:valid_subscription_line_item_params?)).to be false expect(controller_instance).not_to receive(:handle_subscription_line_items) @@ -67,10 +66,8 @@ def initialize(params = {}) 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 = { + let(:params) do + { variant_id: variant.id, subscription_line_item: { subscribable_id: 1, @@ -78,6 +75,10 @@ def initialize(params = {}) interval_length: 1 } } + end + + it 'calls create_subscription_line_item with the correct line item' do + line_item = create(:line_item, order: order, variant: variant) allow(order.line_items).to receive(:find_by).with(variant_id: variant.id).and_return(line_item)