Prevent hard-errors when VatPriceGenerator is invoked without a defau…

[firemind]

Guard against exceptions when calling Price#net_amount without a price.

Skip trying to calculate foreign prices in VatPriceGenerator when the default_price does not have an amount set.

Because the VatPriceGenerator is invoked in a before_validate hook when creating a product, this can lead to exceptions when an admin tries to create a product without specifying a price (and there are non-default countries for which a price needs to be calculated).

This PR allows the validations to run without raising exceptions so the admin can see the validation errors.

[codecov]

Codecov Report

Merging #5324 (94820f8) into v3.4 (3ef8231) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             v3.4    #5324   +/-   ##
=======================================
  Coverage   86.64%   86.64%           
=======================================
  Files         580      580           
  Lines       14741    14743    +2     
=======================================
+ Hits        12772    12774    +2     
  Misses       1969     1969           

Files Changed	Coverage Δ
core/app/models/spree/price.rb	97.72% <100.00%> (+0.05%)	⬆️
...re/app/models/spree/variant/vat_price_generator.rb	100.00% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

[waiting-for-dev]

I labeled the PR as a breaking change, as it's changing a behavior some people could be relying on. It'll be merged in the next major.

[adammathys]

I don't believe it's possible for amount to be nil as of v3.1. (See the "Other important changes" section in the CHANGELOG.md entry for v3.1, or the PR that removes nil as a valid value: #3987)

Which means this shouldn't be possible on recent versions of Solidus. (Assuming the task to remove any records with a nil amount has been run and there isn't any invalid data left in the database from before that change.)

[adammathys]

Oops, nevermind. I missed that we were trying to fix a before_validation callback on Spree::Variant.

In which case I think the preferred fix should be skipping that callback instead of modifying this public method. The callback already has a conditional associated to it, so I believe it would be a less invasive change to prevent it from running in the case when a price is invalid. (And we avoid any future confusion from having a nil check in a method where a nil value is considered invalid.)