Merge pull request #3400 from JDutil/fixes-3294

Update one letter variables to be more descriptive

.rubocop.yml

@@ -27,6 +27,10 @@ Style/ClassVars:
 Naming/PredicateName:
   Enabled: false
 
+# We want to name rescued errors as error not simply e.
+Naming/RescuedExceptionsVariableName:
+  Enabled: false
+
 Naming/AccessorMethodName:
   Enabled: false
 
@@ -117,6 +121,11 @@ Rails/DynamicFindBy:
     - find_by_param
     - find_by_param!
 
+# It's okay to skip model validations to setup a spec.
+Rails/SkipsModelValidations:
+  Exclude:
+    - '*/spec/**/*'
+
 # We use a lot of
 #
 #     expect {

api/app/controllers/spree/api/base_controller.rb

@@ -49,9 +49,9 @@ def load_user
       def authenticate_user
         unless @current_api_user
           if requires_authentication? && api_key.blank? && order_token.blank?
-            render "spree/api/errors/must_specify_api_key", status: 401
+            render "spree/api/errors/must_specify_api_key", status: :unauthorized
           elsif order_token.blank? && (requires_authentication? || api_key.present?)
-            render "spree/api/errors/invalid_api_key", status: 401
+            render "spree/api/errors/invalid_api_key", status: :unauthorized
           end
         end
       end
@@ -65,7 +65,7 @@ def load_user_roles
       end
 
       def unauthorized
-        render "spree/api/errors/unauthorized", status: 401
+        render "spree/api/errors/unauthorized", status: :unauthorized
       end
 
       def gateway_error(exception)
@@ -78,15 +78,15 @@ def parameter_missing_error(exception)
           exception: exception.message,
           error: exception.message,
           missing_param: exception.param
-        }, status: 422
+        }, status: :unprocessable_entity
       end
 
       def requires_authentication?
         Spree::Api::Config[:requires_authentication]
       end
 
       def not_found
-        render "spree/api/errors/not_found", status: 404
+        render "spree/api/errors/not_found", status: :not_found
       end
 
       def current_ability
@@ -96,7 +96,7 @@ def current_ability
       def invalid_resource!(resource)
         Rails.logger.error "invalid_resouce_errors=#{resource.errors.full_messages}"
         @resource = resource
-        render "spree/api/errors/invalid_resource", status: 422
+        render "spree/api/errors/invalid_resource", status: :unprocessable_entity
       end
 
       def api_key
@@ -112,7 +112,7 @@ def bearer_token
 
       def spree_token
         token = request.headers["X-Spree-Token"]
-        return unless token.present?
+        return if token.blank?
 
         Spree::Deprecation.warn(
           'The custom X-Spree-Token request header is deprecated and will be removed in the next release.' \
@@ -164,8 +164,8 @@ def authorize_for_order
 
       def lock_order
         OrderMutex.with_lock!(@order) { yield }
-      rescue Spree::OrderMutex::LockFailed => e
-        render plain: e.message, status: 409
+      rescue Spree::OrderMutex::LockFailed => error
+        render plain: error.message, status: :conflict
       end
 
       def insufficient_stock_error(exception)
@@ -175,7 +175,7 @@ def insufficient_stock_error(exception)
             errors: [I18n.t(:quantity_is_not_available, scope: "spree.api.order")],
             type: 'insufficient_stock'
           },
-          status: 422
+          status: :unprocessable_entity
         )
       end
 

api/app/controllers/spree/api/checkouts_controller.rb

@@ -23,8 +23,8 @@ def next
         authorize! :update, @order, order_token
         @order.next!
         respond_with(@order, default_template: 'spree/api/orders/show', status: 200)
-      rescue StateMachines::InvalidTransition => e
-        logger.error("invalid_transition #{e.event} from #{e.from} for #{e.object.class.name}. Error: #{e.inspect}")
+      rescue StateMachines::InvalidTransition => error
+        logger.error("invalid_transition #{error.event} from #{error.from} for #{error.object.class.name}. Error: #{error.inspect}")
         respond_with(@order, default_template: 'spree/api/orders/could_not_transition', status: 422)
       end
 
@@ -42,8 +42,8 @@ def complete
           @order.complete!
           respond_with(@order, default_template: 'spree/api/orders/show', status: 200)
         end
-      rescue StateMachines::InvalidTransition => e
-        logger.error("invalid_transition #{e.event} from #{e.from} for #{e.object.class.name}. Error: #{e.inspect}")
+      rescue StateMachines::InvalidTransition => error
+        logger.error("invalid_transition #{error.event} from #{error.from} for #{error.object.class.name}. Error: #{error.inspect}")
         respond_with(@order, default_template: 'spree/api/orders/could_not_transition', status: 422)
       end
 

api/app/helpers/spree/api/api_helpers.rb

@@ -40,7 +40,7 @@ module ApiHelpers
 
       def required_fields_for(model)
         required_fields = model._validators.select do |_field, validations|
-          validations.any? { |v| v.is_a?(ActiveModel::Validations::PresenceValidator) }
+          validations.any? { |validation| validation.is_a?(ActiveModel::Validations::PresenceValidator) }
         end.map(&:first) # get fields that are invalid
         # Permalinks presence is validated, but are really automatically generated
         # Therefore we shouldn't tell API clients that they MUST send one through

api/app/views/spree/api/images/_image.json.jbuilder

@@ -2,6 +2,6 @@
 
 json.(image, *image_attributes)
 json.(image, :viewable_type, :viewable_id)
-Spree::Image.attachment_definitions[:attachment][:styles].each do |k, _v|
-  json.set! "#{k}_url", image.attachment.url(k)
+Spree::Image.attachment_definitions[:attachment][:styles].each do |key, _value|
+  json.set! "#{key}_url", image.attachment.url(key)
 end

api/spec/controllers/spree/api/resource_controller_spec.rb

@@ -25,10 +25,10 @@ def permitted_widget_attributes
     end
 
     with_model 'Widget', scope: :all do
-      table do |t|
-        t.string :name
-        t.integer :position
-        t.timestamps null: false
+      table do |widget|
+        widget.string :name
+        widget.integer :position
+        widget.timestamps null: false
       end
 
       model do

api/spec/requests/api/address_books_spec.rb

@@ -80,7 +80,7 @@ module Spree
             user_address = UserAddress.last
 
             expect(response.status).to eq(200)
-            update_target_ids = JSON.parse(response.body).select { |a| a['update_target'] }.map { |a| a['id'] }
+            update_target_ids = JSON.parse(response.body).select { |target| target['update_target'] }.map { |location| location['id'] }
             expect(update_target_ids).to eq([user_address.address_id])
           end
         end
@@ -97,7 +97,7 @@ module Spree
             }.to_not change { UserAddress.count }
 
             expect(response.status).to eq(200)
-            update_target_ids = JSON.parse(response.body).select { |a| a['update_target'] }.map { |a| a['id'] }
+            update_target_ids = JSON.parse(response.body).select { |target| target['update_target'] }.map { |location| location['id'] }
             expect(update_target_ids).to eq([address.id])
           end
         end

api/spec/requests/spree/api/checkouts_controller_spec.rb

@@ -132,9 +132,9 @@ module Spree
         put spree.api_checkout_path(order.to_param), params: { order_token: order.guest_token, order: { shipments_attributes: { "0" => { selected_shipping_rate_id: shipping_rate.id, id: shipment.id } } } }
         expect(response.status).to eq(200)
         # Find the correct shipment...
-        json_shipment = json_response['shipments'].detect { |s| s["id"] == shipment.id }
+        json_shipment = json_response['shipments'].detect { |value| value["id"] == shipment.id }
         # Find the correct shipping rate for that shipment...
-        json_shipping_rate = json_shipment['shipping_rates'].detect { |sr| sr["id"] == shipping_rate.id }
+        json_shipping_rate = json_shipment['shipping_rates'].detect { |value| value["id"] == shipping_rate.id }
         # ... And finally ensure that it's selected
         expect(json_shipping_rate['selected']).to be true
         # Order should automatically transfer to payment because all criteria are met

api/spec/requests/spree/api/option_types_controller_spec.rb

@@ -34,9 +34,9 @@ def check_option_values(option_values)
     end
 
     it "can retrieve a list of specific option types" do
-      option_type_1 = create(:option_type)
+      option_type_one = create(:option_type)
       create(:option_type)
-      get spree.api_option_types_path, params: { ids: "#{option_type.id},#{option_type_1.id}" }
+      get spree.api_option_types_path, params: { ids: "#{option_type.id},#{option_type_one.id}" }
       expect(json_response.count).to eq(2)
 
       check_option_values(json_response.first["option_values"])

api/spec/requests/spree/api/option_values_controller_spec.rb

@@ -48,9 +48,9 @@ def check_option_values(option_values)
       end
 
       it "can retrieve a list of option types" do
-        option_value_1 = create(:option_value, option_type: option_type)
+        option_value_one = create(:option_value, option_type: option_type)
         create(:option_value, option_type: option_type)
-        get spree.api_option_values_path, params: { ids: [option_value.id, option_value_1.id] }
+        get spree.api_option_values_path, params: { ids: [option_value.id, option_value_one.id] }
         expect(json_response.count).to eq(2)
       end
 

api/spec/requests/spree/api/orders_controller_spec.rb

@@ -246,21 +246,21 @@ module Spree
       it "returns orders in reverse chronological order by completed_at" do
         order.update_columns completed_at: Time.current, created_at: 3.days.ago
 
-        order2 = Order.create user: order.user, completed_at: Time.current - 1.day, created_at: 2.day.ago, store: store
-        expect(order2.created_at).to be > order.created_at
-        order3 = Order.create user: order.user, completed_at: nil, created_at: 1.day.ago, store: store
-        expect(order3.created_at).to be > order2.created_at
-        order4 = Order.create user: order.user, completed_at: nil, created_at: 0.days.ago, store: store
-        expect(order4.created_at).to be > order3.created_at
+        order_two = Order.create user: order.user, completed_at: Time.current - 1.day, created_at: 2.days.ago, store: store
+        expect(order_two.created_at).to be > order.created_at
+        order_three = Order.create user: order.user, completed_at: nil, created_at: 1.day.ago, store: store
+        expect(order_three.created_at).to be > order_two.created_at
+        order_four = Order.create user: order.user, completed_at: nil, created_at: 0.days.ago, store: store
+        expect(order_four.created_at).to be > order_three.created_at
 
         get spree.api_my_orders_path, headers: { 'SERVER_NAME' => store.url }
         expect(response.status).to eq(200)
         expect(json_response["pages"]).to eq(1)
         orders = json_response["orders"]
         expect(orders.length).to eq(4)
         expect(orders[0]["number"]).to eq(order.number)
-        expect(orders[1]["number"]).to eq(order2.number)
-        expect([orders[2]["number"], orders[3]["number"]]).to match_array([order3.number, order4.number])
+        expect(orders[1]["number"]).to eq(order_two.number)
+        expect([orders[2]["number"], orders[3]["number"]]).to match_array([order_three.number, order_four.number])
       end
     end
 
@@ -502,18 +502,18 @@ module Spree
       end
 
       it "adds an extra line item" do
-        variant2 = create(:variant)
+        variant_two = create(:variant)
         put spree.api_order_path(order), params: { order: {
           line_items: {
             "0" => { id: line_item.id, quantity: 10 },
-            "1" => { variant_id: variant2.id, quantity: 1 }
+            "1" => { variant_id: variant_two.id, quantity: 1 }
           }
         } }
 
         expect(response.status).to eq(200)
         expect(json_response['line_items'].count).to eq(2)
         expect(json_response['line_items'][0]['quantity']).to eq(10)
-        expect(json_response['line_items'][1]['variant_id']).to eq(variant2.id)
+        expect(json_response['line_items'][1]['variant_id']).to eq(variant_two.id)
         expect(json_response['line_items'][1]['quantity']).to eq(1)
       end
 
@@ -727,7 +727,7 @@ module Spree
 
           orders = json_response[:orders]
           expect(orders.count).to be >= 3
-          expect(orders.map { |o| o[:id] }).to match_array Order.pluck(:id)
+          expect(orders.map { |order| order[:id] }).to match_array Order.pluck(:id)
         end
 
         after { ActionController::Base.perform_caching = false }

api/spec/requests/spree/api/products_controller_spec.rb

@@ -17,8 +17,8 @@ module Spree
         shipping_category_id: create(:shipping_category).id }
     end
     let(:attributes_for_variant) do
-      h = attributes_for(:variant).except(:option_values, :product)
-      h.merge({
+      attributes = attributes_for(:variant).except(:option_values, :product)
+      attributes.merge({
         options: [
           { name: "size", value: "small" },
           { name: "color", value: "black" }
@@ -40,7 +40,7 @@ module Spree
 
         it "returns unique products" do
           get spree.api_products_path
-          product_ids = json_response["products"].map { |p| p["id"] }
+          product_ids = json_response["products"].map { |product| product["id"] }
           expect(product_ids.uniq.count).to eq(product_ids.count)
         end
 
@@ -361,8 +361,8 @@ module Spree
           expect(response.status).to eq 200
           expect(json_response['variants'].count).to eq(2) # 2 variants
 
-          variants = json_response['variants'].reject { |v| v['is_master'] }
-          size_option_value = variants.last['option_values'].detect{ |x| x['option_type_name'] == 'size' }
+          variants = json_response['variants'].reject { |variant| variant['is_master'] }
+          size_option_value = variants.last['option_values'].detect{ |value| value['option_type_name'] == 'size' }
           expect(size_option_value['name']).to eq('small')
 
           expect(json_response['option_types'].count).to eq(2) # size, color
@@ -385,7 +385,7 @@ module Spree
           } }
 
           expect(json_response['variants'].count).to eq(1)
-          variants = json_response['variants'].reject { |v| v['is_master'] }
+          variants = json_response['variants'].reject { |variant| variant['is_master'] }
           expect(variants.last['option_values'][0]['name']).to eq('large')
           expect(variants.last['sku']).to eq('456')
           expect(variants.count).to eq(1)

api/spec/requests/spree/api/taxons_controller_spec.rb

@@ -93,9 +93,9 @@ module Spree
         it 'returns only requested ids' do
           # We need a completly new branch to avoid having parent that can be preloaded from the rails ancestors
           python   = create(:taxon, name: "Python", parent: taxonomy.root, taxonomy: taxonomy)
-          python_3 = create(:taxon, name: "3.0", parent: python, taxonomy: taxonomy)
+          python_three = create(:taxon, name: "3.0", parent: python, taxonomy: taxonomy)
 
-          get spree.api_taxons_path, params: { ids: [rails_v3_2_2.id, python_3.id] }
+          get spree.api_taxons_path, params: { ids: [rails_v3_2_2.id, python_three.id] }
 
           expect(json_response['taxons'].size).to eq 2
         end

backend/app/controllers/spree/admin/orders_controller.rb

@@ -105,8 +105,8 @@ def complete
         @order.complete!
         flash[:success] = t('spree.order_completed')
         redirect_to edit_admin_order_url(@order)
-      rescue StateMachines::InvalidTransition => e
-        flash[:error] = e.message
+      rescue StateMachines::InvalidTransition => error
+        flash[:error] = error.message
         redirect_to confirm_admin_order_url(@order)
       end
 

backend/app/controllers/spree/admin/payment_methods_controller.rb

@@ -28,9 +28,9 @@ def update
         invoke_callbacks(:update, :before)
 
         attributes = payment_method_params
-        attributes.each do |k, _v|
-          if k.include?("password") && attributes[k].blank?
-            attributes.delete(k)
+        attributes.each do |key, _value|
+          if key.include?("password") && attributes[key].blank?
+            attributes.delete(key)
           end
         end
 

backend/app/controllers/spree/admin/payments_controller.rb

@@ -46,8 +46,8 @@ def create
             flash[:error] = t('spree.payment_could_not_be_created')
             render :new
           end
-        rescue Spree::Core::GatewayError => e
-          flash[:error] = e.message.to_s
+        rescue Spree::Core::GatewayError => error
+          flash[:error] = error.message.to_s
           redirect_to new_admin_order_payment_path(@order)
         end
       end

core/app/helpers/spree/products_helper.rb

@@ -37,6 +37,7 @@ def variant_full_price(variant)
       return if variant.product.variants
                   .with_prices(current_pricing_options)
                   .all? { |variant_with_prices| variant_with_prices.price_same_as_master?(current_pricing_options) }
+
       variant.price_for(current_pricing_options).to_html
     end
 

core/app/jobs/spree/promotion_code_batch_job.rb

@@ -14,13 +14,13 @@ def perform(promotion_code_batch)
           .promotion_code_batch_finished(promotion_code_batch)
           .deliver_now
       end
-    rescue StandardError => e
+    rescue StandardError => error
       if promotion_code_batch.email?
         Spree::Config.promotion_code_batch_mailer_class
           .promotion_code_batch_errored(promotion_code_batch)
           .deliver_now
       end
-      raise e
+      raise error
     end
   end
 end

core/app/models/concerns/spree/user_methods.rb

@@ -74,7 +74,7 @@ def total_available_store_credit
 
     def available_store_credit_total(currency:)
       store_credits.reload.to_a.
-        select { |c| c.currency == currency }.
+        select { |credit| credit.currency == currency }.
         sum(&:amount_remaining)
     end
 

core/app/models/spree/address.rb

@@ -121,7 +121,7 @@ def same_as(other_address)
     # @deprecated Do not use this
     def empty?
       Spree::Deprecation.warn("Address#empty? is deprecated.", caller)
-      attributes.except('id', 'created_at', 'updated_at', 'country_id').all? { |_, v| v.nil? }
+      attributes.except('id', 'created_at', 'updated_at', 'country_id').all? { |_, value| value.nil? }
     end
 
     # This exists because the default Object#blank?, checks empty? if it is