From bb3a54847916d4996d20cd82a0ed7b1ffcafc8ee Mon Sep 17 00:00:00 2001 From: Clarke Brunsdon Date: Tue, 18 Jul 2017 14:49:24 -0700 Subject: [PATCH 1/2] Define Spree::ReturnItem in modules Previously it was unclear when these were evaluated that Spree::ReturnItem is actually an active record class (inherited from Spree::Base). This makes it clear that we're actually nesting a class in a module in a class in a module. --- .../eligibility_validator/base_validator.rb | 34 +++++---- .../eligibility_validator/default.rb | 46 ++++++------ .../inventory_shipped.rb | 26 ++++--- .../no_reimbursements.rb | 27 ++++--- .../eligibility_validator/order_completed.rb | 26 ++++--- .../eligibility_validator/rma_required.rb | 26 ++++--- .../time_since_purchase.rb | 26 ++++--- .../same_option_value.rb | 72 ++++++++++--------- .../same_product.rb | 10 +-- 9 files changed, 163 insertions(+), 130 deletions(-) diff --git a/core/app/models/spree/return_item/eligibility_validator/base_validator.rb b/core/app/models/spree/return_item/eligibility_validator/base_validator.rb index 13e2e36350a..d2039ebef41 100644 --- a/core/app/models/spree/return_item/eligibility_validator/base_validator.rb +++ b/core/app/models/spree/return_item/eligibility_validator/base_validator.rb @@ -1,24 +1,28 @@ module Spree - class Spree::ReturnItem::EligibilityValidator::BaseValidator - attr_reader :errors + class ReturnItem < Spree::Base + module EligibilityValidator + class BaseValidator + attr_reader :errors - def initialize(return_item) - @return_item = return_item - @errors = {} - end + def initialize(return_item) + @return_item = return_item + @errors = {} + end - def eligible_for_return? - raise NotImplementedError, Spree.t(:implement_eligible_for_return) - end + def eligible_for_return? + raise NotImplementedError, Spree.t(:implement_eligible_for_return) + end - def requires_manual_intervention? - raise NotImplementedError, Spree.t(:implement_requires_manual_intervention) - end + def requires_manual_intervention? + raise NotImplementedError, Spree.t(:implement_requires_manual_intervention) + end - private + private - def add_error(key, error) - @errors[key] = error + def add_error(key, error) + @errors[key] = error + end + end end end end diff --git a/core/app/models/spree/return_item/eligibility_validator/default.rb b/core/app/models/spree/return_item/eligibility_validator/default.rb index 1ec5d55f041..72a2d24b417 100644 --- a/core/app/models/spree/return_item/eligibility_validator/default.rb +++ b/core/app/models/spree/return_item/eligibility_validator/default.rb @@ -1,30 +1,34 @@ module Spree - class ReturnItem::EligibilityValidator::Default < Spree::ReturnItem::EligibilityValidator::BaseValidator - class_attribute :permitted_eligibility_validators - self.permitted_eligibility_validators = [ - ReturnItem::EligibilityValidator::OrderCompleted, - ReturnItem::EligibilityValidator::TimeSincePurchase, - ReturnItem::EligibilityValidator::RMARequired, - ReturnItem::EligibilityValidator::InventoryShipped, - ReturnItem::EligibilityValidator::NoReimbursements - ] + class ReturnItem < Spree::Base + module EligibilityValidator + class Default < Spree::ReturnItem::EligibilityValidator::BaseValidator + class_attribute :permitted_eligibility_validators + self.permitted_eligibility_validators = [ + ReturnItem::EligibilityValidator::OrderCompleted, + ReturnItem::EligibilityValidator::TimeSincePurchase, + ReturnItem::EligibilityValidator::RMARequired, + ReturnItem::EligibilityValidator::InventoryShipped, + ReturnItem::EligibilityValidator::NoReimbursements + ] - def eligible_for_return? - validators.all?(&:eligible_for_return?) - end + def eligible_for_return? + validators.all?(&:eligible_for_return?) + end - def requires_manual_intervention? - validators.any?(&:requires_manual_intervention?) - end + def requires_manual_intervention? + validators.any?(&:requires_manual_intervention?) + end - def errors - validators.map(&:errors).reduce({}, :merge) - end + def errors + validators.map(&:errors).reduce({}, :merge) + end - private + private - def validators - @validators ||= permitted_eligibility_validators.map{ |v| v.new(@return_item) } + def validators + @validators ||= permitted_eligibility_validators.map{ |v| v.new(@return_item) } + end + end end end end diff --git a/core/app/models/spree/return_item/eligibility_validator/inventory_shipped.rb b/core/app/models/spree/return_item/eligibility_validator/inventory_shipped.rb index 50f5559f88d..a7e81e7633d 100644 --- a/core/app/models/spree/return_item/eligibility_validator/inventory_shipped.rb +++ b/core/app/models/spree/return_item/eligibility_validator/inventory_shipped.rb @@ -1,16 +1,20 @@ module Spree - class ReturnItem::EligibilityValidator::InventoryShipped < Spree::ReturnItem::EligibilityValidator::BaseValidator - def eligible_for_return? - if @return_item.inventory_unit.shipped? - return true - else - add_error(:inventory_unit_shipped, Spree.t('return_item_inventory_unit_ineligible')) - return false - end - end + class ReturnItem < Spree::Base + module EligibilityValidator + class InventoryShipped < Spree::ReturnItem::EligibilityValidator::BaseValidator + def eligible_for_return? + if @return_item.inventory_unit.shipped? + return true + else + add_error(:inventory_unit_shipped, Spree.t('return_item_inventory_unit_ineligible')) + return false + end + end - def requires_manual_intervention? - @errors.present? + def requires_manual_intervention? + @errors.present? + end + end end end end diff --git a/core/app/models/spree/return_item/eligibility_validator/no_reimbursements.rb b/core/app/models/spree/return_item/eligibility_validator/no_reimbursements.rb index 5f4a45bd911..57ce217b096 100644 --- a/core/app/models/spree/return_item/eligibility_validator/no_reimbursements.rb +++ b/core/app/models/spree/return_item/eligibility_validator/no_reimbursements.rb @@ -1,16 +1,21 @@ module Spree - class ReturnItem::EligibilityValidator::NoReimbursements < Spree::ReturnItem::EligibilityValidator::BaseValidator - def eligible_for_return? - if @return_item.inventory_unit.return_items.reimbursed.valid.any? - add_error(:inventory_unit_reimbursed, Spree.t('return_item_inventory_unit_reimbursed')) - return false - else - return true - end - end + class ReturnItem < Spree::Base + module EligibilityValidator + class NoReimbursements < Spree::ReturnItem::EligibilityValidator::BaseValidator - def requires_manual_intervention? - @errors.present? + def eligible_for_return? + if @return_item.inventory_unit.return_items.reimbursed.valid.any? + add_error(:inventory_unit_reimbursed, Spree.t('return_item_inventory_unit_reimbursed')) + return false + else + return true + end + end + + def requires_manual_intervention? + @errors.present? + end + end end end end diff --git a/core/app/models/spree/return_item/eligibility_validator/order_completed.rb b/core/app/models/spree/return_item/eligibility_validator/order_completed.rb index 97dfeb3d16c..437bf283f28 100644 --- a/core/app/models/spree/return_item/eligibility_validator/order_completed.rb +++ b/core/app/models/spree/return_item/eligibility_validator/order_completed.rb @@ -1,16 +1,20 @@ module Spree - class ReturnItem::EligibilityValidator::OrderCompleted < Spree::ReturnItem::EligibilityValidator::BaseValidator - def eligible_for_return? - if @return_item.inventory_unit.order.completed? - return true - else - add_error(:order_not_completed, Spree.t('return_item_order_not_completed')) - return false - end - end + class ReturnItem < Spree::Base + module EligibilityValidator + class OrderCompleted < Spree::ReturnItem::EligibilityValidator::BaseValidator + def eligible_for_return? + if @return_item.inventory_unit.order.completed? + return true + else + add_error(:order_not_completed, Spree.t('return_item_order_not_completed')) + return false + end + end - def requires_manual_intervention? - false + def requires_manual_intervention? + false + end + end end end end diff --git a/core/app/models/spree/return_item/eligibility_validator/rma_required.rb b/core/app/models/spree/return_item/eligibility_validator/rma_required.rb index 99419427810..f57cd94a1c0 100644 --- a/core/app/models/spree/return_item/eligibility_validator/rma_required.rb +++ b/core/app/models/spree/return_item/eligibility_validator/rma_required.rb @@ -1,16 +1,20 @@ module Spree - class ReturnItem::EligibilityValidator::RMARequired < Spree::ReturnItem::EligibilityValidator::BaseValidator - def eligible_for_return? - if @return_item.return_authorization.present? - return true - else - add_error(:rma_required, Spree.t('return_item_rma_ineligible')) - return false - end - end + class ReturnItem < Spree::Base + module EligibilityValidator + class RMARequired < Spree::ReturnItem::EligibilityValidator::BaseValidator + def eligible_for_return? + if @return_item.return_authorization.present? + return true + else + add_error(:rma_required, Spree.t('return_item_rma_ineligible')) + return false + end + end - def requires_manual_intervention? - false + def requires_manual_intervention? + false + end + end end end end diff --git a/core/app/models/spree/return_item/eligibility_validator/time_since_purchase.rb b/core/app/models/spree/return_item/eligibility_validator/time_since_purchase.rb index 20d86e4e958..5dd39bf90dc 100644 --- a/core/app/models/spree/return_item/eligibility_validator/time_since_purchase.rb +++ b/core/app/models/spree/return_item/eligibility_validator/time_since_purchase.rb @@ -1,16 +1,20 @@ module Spree - class ReturnItem::EligibilityValidator::TimeSincePurchase < Spree::ReturnItem::EligibilityValidator::BaseValidator - def eligible_for_return? - if (@return_item.inventory_unit.order.completed_at + Spree::Config[:return_eligibility_number_of_days].days) > Time.current - return true - else - add_error(:number_of_days, Spree.t('return_item_time_period_ineligible')) - return false - end - end + class ReturnItem < Spree::Base + module EligibilityValidator + class TimeSincePurchase < Spree::ReturnItem::EligibilityValidator::BaseValidator + def eligible_for_return? + if (@return_item.inventory_unit.order.completed_at + Spree::Config[:return_eligibility_number_of_days].days) > Time.current + return true + else + add_error(:number_of_days, Spree.t('return_item_time_period_ineligible')) + return false + end + end - def requires_manual_intervention? - false + def requires_manual_intervention? + false + end + end end end end diff --git a/core/app/models/spree/return_item/exchange_variant_eligibility/same_option_value.rb b/core/app/models/spree/return_item/exchange_variant_eligibility/same_option_value.rb index 28bd715175d..acd91e02835 100644 --- a/core/app/models/spree/return_item/exchange_variant_eligibility/same_option_value.rb +++ b/core/app/models/spree/return_item/exchange_variant_eligibility/same_option_value.rb @@ -1,41 +1,43 @@ module Spree - module ReturnItem::ExchangeVariantEligibility - class SameOptionValue - class_attribute :option_type_restrictions - self.option_type_restrictions = [] - # This can be configured in an initializer, e.g.: - # Spree::ReturnItem::ExchangeVariantEligibility::SameOptionValue.option_type_restrictions = ["size", "color"] - # - # This restriction causes only variants that share the same option value for the - # specified option types to be returned. e.g.: - # - # option_type_restrictions = ["color", "waist"] - # Variant: blue pants with 32 waist and 30 inseam - # - # can be exchanged for: - # blue pants with 32 waist and 31 inseam - # - # cannot be exchanged for: - # green pants with 32 waist and 30 inseam - # blue pants with 34 waist and 32 inseam + class ReturnItem < Spree::Base + module ExchangeVariantEligibility + class SameOptionValue + class_attribute :option_type_restrictions + self.option_type_restrictions = [] + # This can be configured in an initializer, e.g.: + # Spree::ReturnItem::ExchangeVariantEligibility::SameOptionValue.option_type_restrictions = ["size", "color"] + # + # This restriction causes only variants that share the same option value for the + # specified option types to be returned. e.g.: + # + # option_type_restrictions = ["color", "waist"] + # Variant: blue pants with 32 waist and 30 inseam + # + # can be exchanged for: + # blue pants with 32 waist and 31 inseam + # + # cannot be exchanged for: + # green pants with 32 waist and 30 inseam + # blue pants with 34 waist and 32 inseam - def self.eligible_variants(variant, options = {}) - product_variants = SameProduct.eligible_variants(variant, options).includes(option_values: :option_type) - relevant_option_values = variant.option_values.select { |ov| option_type_restrictions.include? ov.option_type.name } + def self.eligible_variants(variant, options = {}) + product_variants = SameProduct.eligible_variants(variant, options).includes(option_values: :option_type) + relevant_option_values = variant.option_values.select { |ov| option_type_restrictions.include? ov.option_type.name } - if relevant_option_values.present? - # Finds all the OptionValueVariants that have any of the - # relevant option values, groups by variant and ensures the variant - # has ALL of the relevant option values. - variant_ids = Spree::OptionValuesVariant. - where(variant_id: product_variants.distinct.pluck(:id)). - where(option_value: relevant_option_values). - group(:variant_id). - having('COUNT(*) = ?', relevant_option_values.size). - pluck(:variant_id) - product_variants.where(id: variant_ids) - else - product_variants + if relevant_option_values.present? + # Finds all the OptionValueVariants that have any of the + # relevant option values, groups by variant and ensures the variant + # has ALL of the relevant option values. + variant_ids = Spree::OptionValuesVariant. + where(variant_id: product_variants.distinct.pluck(:id)). + where(option_value: relevant_option_values). + group(:variant_id). + having('COUNT(*) = ?', relevant_option_values.size). + pluck(:variant_id) + product_variants.where(id: variant_ids) + else + product_variants + end end end end diff --git a/core/app/models/spree/return_item/exchange_variant_eligibility/same_product.rb b/core/app/models/spree/return_item/exchange_variant_eligibility/same_product.rb index de4bff80c85..8c42feb0ac1 100644 --- a/core/app/models/spree/return_item/exchange_variant_eligibility/same_product.rb +++ b/core/app/models/spree/return_item/exchange_variant_eligibility/same_product.rb @@ -1,8 +1,10 @@ module Spree - module ReturnItem::ExchangeVariantEligibility - class SameProduct - def self.eligible_variants(variant, stock_locations: nil) - Spree::Variant.where(product_id: variant.product_id, is_master: variant.is_master?).in_stock(stock_locations) + class ReturnItem < Spree::Base + module ExchangeVariantEligibility + class SameProduct + def self.eligible_variants(variant, stock_locations: nil) + Spree::Variant.where(product_id: variant.product_id, is_master: variant.is_master?).in_stock(stock_locations) + end end end end From fb070a81447e5ec75fee6ebf860a52caeadd75c2 Mon Sep 17 00:00:00 2001 From: Clarke Brunsdon Date: Tue, 18 Jul 2017 16:27:08 -0700 Subject: [PATCH 2/2] Fix nesting of Spree::Reimbursement::Credit Spree::Reimbursement::Credit is a class nested in a class, which isn't clear from the declaration. Spree::Reimbursement is an active record model that inherits from Spree::Base --- core/app/models/spree/reimbursement/credit.rb | 32 ++++++++++--------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/core/app/models/spree/reimbursement/credit.rb b/core/app/models/spree/reimbursement/credit.rb index 2783bada029..85006e07534 100644 --- a/core/app/models/spree/reimbursement/credit.rb +++ b/core/app/models/spree/reimbursement/credit.rb @@ -1,25 +1,27 @@ module Spree - class Reimbursement::Credit < Spree::Base - class_attribute :default_creditable_class - self.default_creditable_class = Spree::StoreCredit + class Reimbursement < Spree::Base + class Credit < Spree::Base + class_attribute :default_creditable_class + self.default_creditable_class = Spree::StoreCredit - belongs_to :reimbursement, inverse_of: :credits - belongs_to :creditable, polymorphic: true + belongs_to :reimbursement, inverse_of: :credits + belongs_to :creditable, polymorphic: true - validates :creditable, presence: true + validates :creditable, presence: true - class << self - def total_amount_reimbursed_for(reimbursement) - reimbursement.credits.to_a.sum(&:amount) + class << self + def total_amount_reimbursed_for(reimbursement) + reimbursement.credits.to_a.sum(&:amount) + end end - end - def description - creditable.class.name.demodulize - end + def description + creditable.class.name.demodulize + end - def display_amount - Spree::Money.new(amount, { currency: creditable.try(:currency) || "USD" }) + def display_amount + Spree::Money.new(amount, { currency: creditable.try(:currency) || "USD" }) + end end end end