-
-
Notifications
You must be signed in to change notification settings - Fork 719
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.
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
[BUU] Fixes Products Page ActionView::Template::Error #12710
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -11,19 +11,20 @@ def product_image_form_path(product) | |
end | ||
|
||
def prepare_new_variant(product) | ||
product.variants.build do |variant| | ||
variant.unit_value = 1.0 * (product.variant_unit_scale || 1) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mimicking old UI behavior, when creating a new variant, then both of these are empty |
||
variant.unit_presentation = VariantUnits::OptionValueNamer.new(variant).name | ||
end | ||
product.variants.build | ||
end | ||
|
||
def unit_value_with_description(variant) | ||
scaled_unit_value = variant.unit_value / (variant.product.variant_unit_scale || 1) | ||
precised_unit_value = number_with_precision( | ||
scaled_unit_value, | ||
precision: nil, | ||
strip_insignificant_zeros: true | ||
) | ||
precised_unit_value = nil | ||
|
||
if variant.unit_value | ||
scaled_unit_value = variant.unit_value / (variant.product.variant_unit_scale || 1) | ||
precised_unit_value = number_with_precision( | ||
scaled_unit_value, | ||
precision: nil, | ||
strip_insignificant_zeros: true | ||
) | ||
end | ||
|
||
[precised_unit_value, variant.unit_description].compact_blank.join(" ") | ||
end | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -18,7 +18,7 @@ | |
= f.hidden_field :unit_value | ||
= f.hidden_field :unit_description | ||
= f.text_field :unit_value_with_description, | ||
value: unit_value_with_description(variant), 'aria-label': t('admin.products_page.columns.unit_value'), required: true | ||
value: unit_value_with_description(variant), 'aria-label': t('admin.products_page.columns.unit_value') | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Having
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This validation is handled by the browser, probably in Javascript. I think that's a fair trade off, I am wondering if we could show the pop up with error on form submit so we can still benefit from the browser validation but it's probably not worth the effort. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI: I ran into the same issue while working on the Product Refactor and I managed to implement the solution I was talking about above. It ended up pretty simple in the end. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks for implementing this one. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The This change seems like the most practical best solution 👍 |
||
.field | ||
= f.label :display_as, t('admin.products_page.columns.display_as') | ||
= f.text_field :display_as, placeholder: VariantUnits::OptionValueNamer.new(variant).name | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,48 @@ | ||
# frozen_string_literal: true | ||
|
||
require "spec_helper" | ||
|
||
RSpec.describe Admin::ProductsHelper do | ||
describe '#unit_value_with_description' do | ||
let(:product) { create(:product, variant_unit_scale: 1000.0) } | ||
let(:variant) { create(:variant, product:, unit_value: 2000.0, unit_description: 'kg') } | ||
|
||
context 'when unit_value and unit_description are present' do | ||
it 'returns the scaled unit value with the description' do | ||
expect(helper.unit_value_with_description(variant)).to eq('2 kg') | ||
end | ||
end | ||
|
||
context 'when unit_value is nil' do | ||
before { variant.update_column(:unit_value, nil) } | ||
|
||
it 'returns the description' do | ||
expect(helper.unit_value_with_description(variant)).to eq('kg') | ||
end | ||
end | ||
|
||
context 'when unit_description is nil' do | ||
before { variant.update_column(:unit_description, nil) } | ||
|
||
it 'returns only the scaled unit value' do | ||
expect(helper.unit_value_with_description(variant)).to eq('2') | ||
end | ||
end | ||
|
||
context 'when variant_unit_scale is nil' do | ||
before { product.update_column(:variant_unit_scale, nil) } | ||
|
||
it 'uses default scale of 1 and returns the unscaled unit value with the description' do | ||
expect(helper.unit_value_with_description(variant)).to eq('2000 kg') | ||
end | ||
end | ||
|
||
context 'when both unit_value and unit_description are nil' do | ||
before { variant.update_columns(unit_description: nil, unit_value: nil) } | ||
|
||
it 'returns empty string' do | ||
expect(helper.unit_value_with_description(variant)).to eq('') | ||
end | ||
end | ||
end | ||
end |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -50,20 +50,20 @@ | |
fill_in "SKU", with: "POM-00" | ||
tomselect_select "Volume (mL)", from: "Unit scale" | ||
end | ||
within row_containing_name("Medium box") do | ||
fill_in "Name", with: "Large box" | ||
fill_in "SKU", with: "POM-01" | ||
|
||
click_on "Unit" # activate popout | ||
end | ||
|
||
# Unit popout | ||
fill_in "Unit value", with: "" | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. As we don't have required validation as per this, so we can remove its expectation There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can the case of blank unit value now be added here? |
||
click_button "Save changes" # attempt to save or close the popout | ||
expect(page).to have_field "Unit value", with: "" # popout is still open | ||
click_on "Unit" # activate popout | ||
# have to use below method to trigger the +change+ event, | ||
# +fill_in "Unit value", with: ""+ does not trigger +change+ event | ||
find_field('Unit value').send_keys(:control, 'a', :backspace) # empty the field | ||
click_button "Save changes" # attempt to save and should fail with below error | ||
expect(page).to have_content "must be greater than 0" | ||
click_on "Unit" # activate popout | ||
fill_in "Unit value", with: "500.1" | ||
|
||
within row_containing_name("Medium box") do | ||
fill_in "Name", with: "Large box" | ||
fill_in "SKU", with: "POM-01" | ||
fill_in "Price", with: "10.25" | ||
|
||
click_on "On Hand" # activate popout | ||
|
@@ -517,6 +517,7 @@ | |
expect(page).to have_field "SKU", with: "n" * 256 | ||
expect(page).to have_content "is too long" | ||
expect(page.find_button("Unit")).to have_text "" # have_button selector don't work here | ||
expect(page).to have_content "can't be blank" | ||
expect(page).to have_field "Price", with: "10.25" # other updated value is retained | ||
end | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could remove the helper has it's not doing much now, but that's fine as it is.