Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
wants to merge 1 commit into
base: v3.4
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions core/app/models/spree/price.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ def price=(price)
end

def net_amount
return nil unless amount
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.)


amount / (1 + sum_of_vat_amounts)
end

Expand Down
1 change: 1 addition & 0 deletions core/app/models/spree/variant/vat_price_generator.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ def initialize(variant)
def run
# Early return if there is no VAT rates in the current store.
return if !variant.tax_category || variant_vat_rates.empty?
return unless variant.default_price.net_amount

country_isos_requiring_price.each do |country_iso|
# Don't re-create the default price
Expand Down