Fix Spree::Variant inconsistency due to lack of product association #3043

rubenochiavone · 2019-01-18T02:36:56Z

Description

Spree::Variant new instance throw error during validation because it lacks a product association. The lack of product causes

Error to be raised because price is inferred using product.master.price
Module::DelegationError to be raised because to generate VAT prices, variant.tax_category is required and this method is delegated to product.tax_category

To fix it, removed raising an error if price can't be inferred using product.master.price. Reporting missing product is enough for an invalid variant instance. Also add product verification before generating VAT prices for variant.

Closes #3036

Checklist:

Pull Request guidelines are respected
Changes are covered by tests (if possible)
Each commit has a meaningful message attached that described WHAT changed, and WHY

aitbw

Great job @rubenochiavone ! 👏

jacobherrington · 2019-01-19T15:37:04Z

core/app/models/spree/variant.rb

-        raise 'No master variant found to infer price' unless product && product.master
-        self.price = product.master.price
-      end
+      self.price = product.master.price if price.nil? && Spree::Config[:require_master_price] && !is_master? && product && product.master


I'm not sure about a guard clause being this long, but it's certainly not blocking.

Agreed, it got quite long. Ruby's statement if condition is too good to don't use it. 😄

@kennyadsl contribution might reduce it though - #3043 (comment).

Yes, I agree with his feedback.

kennyadsl

Thanks, left a couple of questions 🙂

kennyadsl · 2019-01-22T10:32:59Z

core/app/models/spree/variant.rb

@@ -73,7 +73,7 @@ class Variant < Spree::Base

    before_validation :set_cost_currency
    before_validation :set_price


what about moving if: -> { product && product.master } here?

Sounds good

kennyadsl · 2019-01-22T10:39:28Z

core/app/models/spree/variant.rb

@@ -73,7 +73,7 @@ class Variant < Spree::Base

    before_validation :set_cost_currency
    before_validation :set_price
-    before_validation :build_vat_prices, if: -> { rebuild_vat_prices? || new_record? }
+    before_validation :build_vat_prices, if: -> { rebuild_vat_prices? || new_record? && product }

    validate :check_price


What about adding a presence validation on the product here? There are too many places where Solidus gives for granted that a product should be associated with a variant and I think it could make sense to don't allow saving a variant without a product associated. Also, this way users will have the right validation message attached to the invalid object.

Hm, after reviewing the code I'm having second thoughts. How can we validate product presence in variant if product requires a master variant to be present?

The funny thing is that product automatically builds master variant, but I was unable to find build_master method definition. Maybe I'm missing something ☹️

solidus/core/app/models/spree/product.rb

Lines 74 to 76 in adc7534

def find_or_build_master

master || build_master

end

Any idea?

build_master is a method coming from has_one association. That variant will be associated with the product automatically so I think it will be valid, even with the proposed product presence validation.

I see. 👍

spaghetticode · 2019-01-24T10:35:26Z

core/spec/models/spree/variant_spec.rb

@@ -3,6 +3,13 @@
 require 'rails_helper'

 RSpec.describe Spree::Variant, type: :model do
+  context "new instance" do


@rubenochiavone I think this example readability may benefit from using Rspec implicit subject. In Rspec the default subject is an instance of the class being specified (similar to described_class.new(args) but without any arguments... see here):

it { expect(subject).to be_invalid }

or, even more concisely:

it { is_expected.to be_invalid }

Whoa, that's great and works perfectly! Do you mind if I add a reference to implicit subject?

it { is_expected.to be_invalid } # implicitly defined subject, see https://relishapp.com/rspec/rspec-core/v/3-5/docs/subject/implicitly-defined-subject

I'm 👍 on using this syntax but 👎 on adding a code comment. I think you could add a reference in the commit description instead (please keep it all in a single commit 🙂).

Ok, cool. 👍

Remove raising an error if price can't be inferred using product.master.price. Reporting missing product is enough for an invalid variant instance - add product presence validation. Also add product verification before generating VAT prices for variant. Specs improvements includes usage of implicitly defined subject, see https://relishapp.com/rspec/rspec-core/v/3-5/docs/subject/implicitly-defined-subject.

kennyadsl · 2019-01-24T16:12:56Z

@rubenochiavone thanks! 🎉

aitbw approved these changes Jan 18, 2019

View reviewed changes

jacobherrington approved these changes Jan 19, 2019

View reviewed changes

kennyadsl reviewed Jan 22, 2019

View reviewed changes

rubenochiavone force-pushed the fix_variant_inconsistency branch from f9a67b7 to f51633b Compare January 23, 2019 23:35

spaghetticode reviewed Jan 24, 2019

View reviewed changes

rubenochiavone force-pushed the fix_variant_inconsistency branch from f51633b to 7b80a71 Compare January 24, 2019 15:43

kennyadsl approved these changes Jan 24, 2019

View reviewed changes

kennyadsl merged commit 686da1d into solidusio:master Jan 24, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix Spree::Variant inconsistency due to lack of product association #3043

Fix Spree::Variant inconsistency due to lack of product association #3043

rubenochiavone commented Jan 18, 2019

aitbw left a comment

jacobherrington Jan 19, 2019

rubenochiavone Jan 23, 2019 •

edited

Loading

jacobherrington Jan 23, 2019

kennyadsl left a comment

kennyadsl Jan 22, 2019

rubenochiavone Jan 23, 2019

rubenochiavone Jan 23, 2019

kennyadsl Jan 22, 2019

rubenochiavone Jan 23, 2019

rubenochiavone Jan 23, 2019

kennyadsl Jan 23, 2019

rubenochiavone Jan 23, 2019

rubenochiavone Jan 23, 2019

spaghetticode Jan 24, 2019

rubenochiavone Jan 24, 2019

kennyadsl Jan 24, 2019

rubenochiavone Jan 24, 2019

rubenochiavone Jan 24, 2019

kennyadsl commented Jan 24, 2019

		@@ -73,7 +73,7 @@ class Variant < Spree::Base

		before_validation :set_cost_currency
		before_validation :set_price

Fix Spree::Variant inconsistency due to lack of product association #3043

Fix Spree::Variant inconsistency due to lack of product association #3043

Conversation

rubenochiavone commented Jan 18, 2019

Description

Checklist:

aitbw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rubenochiavone Jan 23, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kennyadsl commented Jan 24, 2019

rubenochiavone Jan 23, 2019 •

edited

Loading