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.

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

Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
21 changes: 11 additions & 10 deletions app/helpers/admin/products_helper.rb
Original file line number Diff line number Diff line change
Expand Up @@ -11,19 +11,20 @@ def product_image_form_path(product)
end

def prepare_new_variant(product)
Copy link
Collaborator

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.

product.variants.build do |variant|
variant.unit_value = 1.0 * (product.variant_unit_scale || 1)
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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
Expand Down
2 changes: 1 addition & 1 deletion app/views/admin/products_v3/_variant_row.html.haml
Original file line number Diff line number Diff line change
Expand Up @@ -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')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Having required here is causing an error upon saving in the following scenario:

  • New varaint button is clicked to create a new variant
  • As per Old UI behavior, unit_value is empty for this product.
  • However, in the new UI, if this variant is changed then the saving form doesn't work because this field is hidden on the UI until its pop-up is opened
  • JS tries to find it to show the required validation error but gets broken because it's not able to find it on the UI
  • Hence neither the required error gets displayed nor the form gets submitted.
  • This PR removes the required validation which won't cause the failure of the above use-case however, the backend is handling this blank field error, so it will return the erorr "can't be blank" which will be displayed below it.

Copy link
Collaborator

Choose a reason for hiding this comment

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

JS tries to find it to show the required validation error but gets broken because it's not able to find it on the UI

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for implementing this one. ♥️
I tried to implement the above, maybe I was looking at it from a different POV and was facing issues. Looking forward to your solution to this in the refactor 😄

Copy link
Member

Choose a reason for hiding this comment

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

The required JS validation was added after discussing with Mario. It ensures that the popout can't be closed if it's empty, which helps avoid the problem being hidden.
But as you've found, we have a problem if the field is empty and the popout never gets opened.

This change seems like the most practical best solution 👍
I'm not sure if the red border will show on the popout, until you open it. But if there error text is there, I think that's ok.

.field
= f.label :display_as, t('admin.products_page.columns.display_as')
= f.text_field :display_as, placeholder: VariantUnits::OptionValueNamer.new(variant).name
Expand Down
48 changes: 48 additions & 0 deletions spec/helpers/admin/products_helper_spec.rb
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
6 changes: 3 additions & 3 deletions spec/system/admin/products_v3/create_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -45,15 +45,15 @@
expect(page).to have_content "New variant"
end

it "has the 1 unit value for the new variant display_as by default" do
it "has the empty unit value for the new variant display_as by default" do
new_variant_button.click

within new_variant_row do
unit_button = find('button[aria-label="Unit"]')
expect(unit_button.text.strip).to eq('1kg')
expect(unit_button.text.strip).to eq('')

unit_button.click
expect(page).to have_field "Display unit as", placeholder: "1kg"
expect(page).to have_field "Display unit as", placeholder: ""
end
end

Expand Down
19 changes: 10 additions & 9 deletions spec/system/admin/products_v3/update_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -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: ""
Copy link
Collaborator Author

Choose a reason for hiding this comment

The 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

Copy link
Member

Choose a reason for hiding this comment

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

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
Expand Down Expand Up @@ -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

Expand Down
Loading