diff --git a/Gemfile b/Gemfile index b0fd553864..b71f0ccdc6 100644 --- a/Gemfile +++ b/Gemfile @@ -32,6 +32,7 @@ gem "db2fog", github: "openfoodfoundation/db2fog", branch: "rails-7" gem "fog-aws", "~> 2.0" # db2fog does not support v3 gem "mime-types" # required by fog +gem "validates_lengths_from_database" gem "valid_email2" gem "catalog", path: "./engines/catalog" diff --git a/Gemfile.lock b/Gemfile.lock index b0d84fdc61..b4edeb19b5 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -736,6 +736,8 @@ GEM validate_url (1.0.15) activemodel (>= 3.0.0) public_suffix + validates_lengths_from_database (0.8.0) + activerecord (>= 4) vcr (6.2.0) view_component (3.6.0) activesupport (>= 5.2.0, < 8.0) @@ -909,6 +911,7 @@ DEPENDENCIES stripe timecop valid_email2 + validates_lengths_from_database vcr view_component view_component_reflex (= 3.1.14.pre9) diff --git a/app/helpers/spree/admin/base_helper.rb b/app/helpers/spree/admin/base_helper.rb index 9109d082f0..5615d4d62a 100644 --- a/app/helpers/spree/admin/base_helper.rb +++ b/app/helpers/spree/admin/base_helper.rb @@ -6,9 +6,6 @@ module Spree def field_container(model, method, options = {}, &) css_classes = options[:class].to_a css_classes << 'field' - if error_message_on(model, method).present? - css_classes << 'withError' - end content_tag(:div, capture(&), class: css_classes.join(' '), diff --git a/app/models/spree/product.rb b/app/models/spree/product.rb index be905c8ccc..7f13d46c0d 100755 --- a/app/models/spree/product.rb +++ b/app/models/spree/product.rb @@ -54,6 +54,7 @@ module Spree has_many :variant_images, -> { order(:position) }, source: :images, through: :variants + validates_lengths_from_database validates :name, presence: true validates :variant_unit, presence: true diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index 3ca3317aa8..9768f00ced 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -61,6 +61,7 @@ module Spree localize_number :price, :weight + validates_lengths_from_database validate :check_currency validates :price, numericality: { greater_than_or_equal_to: 0 }, presence: true validates :tax_category, presence: true, @@ -80,8 +81,8 @@ module Spree before_validation :ensure_shipping_category before_validation :ensure_unit_value before_validation :update_weight_from_unit_value, if: ->(v) { v.product.present? } + before_validation :convert_variant_weight_to_decimal - before_save :convert_variant_weight_to_decimal before_save :assign_units, if: ->(variant) { variant.new_record? || variant.changed_attributes.keys.intersection(NAME_FIELDS).any? } diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index b64072a270..28d2a3ded3 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/app/views/admin/products_v3/_table.html.haml @@ -38,13 +38,15 @@ = form.fields_for("products", product, index: nil) do |product_form| %tbody.relaxed{ 'data-record-id': product_form.object.id } %tr - %td.align-left.header + %td.field.align-left.header = product_form.hidden_field :id = product_form.text_field :name, 'aria-label': t('admin.products_page.columns.name') - %td.align-right + = error_message_on product, :name + %td.field = product_form.text_field :sku, 'aria-label': t('admin.products_page.columns.sku') + = error_message_on product, :sku %td.align-right - .line-clamp-1 + .content = product.variant_unit.upcase_first / TODO: properly handle custom unit names = WeightsAndMeasures::UNITS[product.variant_unit] && "(" + WeightsAndMeasures::UNITS[product.variant_unit][product.variant_unit_scale]["name"] + ")" @@ -52,36 +54,39 @@ -# empty %td.align-right -# TODO: new requirement "DISPLAY ON DEMAND IF ALL VARIANTS ARE ON DEMAND". And translate value - .line-clamp-1= if product.variants.all?(&:on_demand) then "On demand" else product.on_hand || 0 end + .content= if product.variants.all?(&:on_demand) then "On demand" else product.on_hand || 0 end %td.align-left - .line-clamp-1= product.supplier.name + .content= product.supplier.name %td.align-left - .line-clamp-1= product.primary_taxon.name + .content= product.primary_taxon.name %td.align-left %td.align-left - .line-clamp-1= product.inherits_properties ? 'YES' : 'NO' #TODO: consider using https://github.com/RST-J/human_attribute_values, else use I18n.t (also below) + .content= product.inherits_properties ? 'YES' : 'NO' #TODO: consider using https://github.com/RST-J/human_attribute_values, else use I18n.t (also below) %td.align-right = render partial: 'admin/products_v3/components/product_actions', locals: { product: product } - product.variants.each do |variant| = form.fields_for("products][][variants_attributes][", variant, index: nil) do |variant_form| %tr.condensed - %td.align-left + %td.field = variant_form.hidden_field :id = variant_form.text_field :display_name, 'aria-label': t('admin.products_page.columns.name'), placeholder: product.name - %td.align-right + = error_message_on variant, :display_name + %td.field = variant_form.text_field :sku, 'aria-label': t('admin.products_page.columns.sku') + = error_message_on variant, :sku %td.align-right - .line-clamp-1= variant.unit_to_display - %td.align-right + .content= variant.unit_to_display + %td.field = variant_form.text_field :price, 'aria-label': t('admin.products_page.columns.price'), value: number_to_currency(variant.price, unit: '')&.strip # TODO: add a spec to prove that this formatting is necessary. If so, it should be in a shared form helper for currency inputs + = error_message_on variant, :price %td.align-right - .line-clamp-1= variant.on_hand || 0 #TODO: spec for this according to requirements. + .content= variant.on_hand || 0 #TODO: spec for this according to requirements. %td.align-left - .line-clamp-1= variant.product.supplier.name # same as product + .content= variant.product.supplier.name # same as product %td.align-left -# empty %td.align-left - .line-clamp-1= variant.tax_category&.name || "None" # TODO: convert to dropdown, else translate hardcoded string. + .content= variant.tax_category&.name || "None" # TODO: convert to dropdown, else translate hardcoded string. %td.align-left -# empty %td.align-right diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss index da512aecf7..4ea63f3a28 100644 --- a/app/webpacker/css/admin/products_v3.scss +++ b/app/webpacker/css/admin/products_v3.scss @@ -62,8 +62,16 @@ th, td { + vertical-align: top; padding: $padding-tbl-cell; border: none; + + .content { + // Plain content fields need help to align with text in inputs (due to vertical-align) + margin: $vpadding-txt+1px 0; + + @extend .line-clamp-1; + } } th { diff --git a/app/webpacker/css/admin/shared/forms.scss b/app/webpacker/css/admin/shared/forms.scss index da593e3300..03bee63048 100644 --- a/app/webpacker/css/admin/shared/forms.scss +++ b/app/webpacker/css/admin/shared/forms.scss @@ -97,21 +97,21 @@ span.info { } } - &.withError { - .field_with_errors { - label { - color: $color-error; - } - - input { - border-color: $color-error; - } - } - .formError { + // Errors described by default form builder + .field_with_errors { + label { color: $color-error; - font-style: italic; - font-size: 85%; } + + input { + border-color: $color-error; + } + } + // Errors described by Spree::Admin::BaseHelper + .formError { + color: $color-error; + font-style: italic; + font-size: 85%; } } diff --git a/app/webpacker/css/admin_v3/shared/forms.scss b/app/webpacker/css/admin_v3/shared/forms.scss index 6ee6f4271b..6b9cb9cf8e 100644 --- a/app/webpacker/css/admin_v3/shared/forms.scss +++ b/app/webpacker/css/admin_v3/shared/forms.scss @@ -108,20 +108,31 @@ span.info { } } - &.withError { - .field_with_errors { - label { - color: $color-error; - } - - input { - border-color: $color-error; - } - } - .formError { + // Errors + .field_with_errors { + label { color: $color-error; - font-style: italic; - font-size: 85%; + } + + input { + border-color: $color-error; + } + } + // Inline error message + .formError { + display: inline-block; + color: $color-error; + font-size: 85%; + + // Icon on left, with subsequent lines indented + text-indent: -0.6em; + margin-left: 1.2em; + + @extend .icon-remove-sign; + @extend [class^="icon-"]; + + &:before { + margin-right: 0.3em; } } } diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index 06ba1e2626..34d16b641f 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -163,6 +163,10 @@ module Spree expect(build(:product)).to be_valid end + it { is_expected.to validate_presence_of :name } + it { is_expected.to validate_length_of(:name).is_at_most(255) } + it { is_expected.to validate_length_of(:sku).is_at_most(255) } + it "requires a primary taxon" do expect(build(:simple_product, primary_taxon: nil)).not_to be_valid end diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb index 3630f727e3..a706beeb41 100644 --- a/spec/system/admin/products_v3/products_spec.rb +++ b/spec/system/admin/products_v3/products_spec.rb @@ -201,7 +201,7 @@ describe 'As an admin, I can see the new product page' do visit admin_products_v3_index_url end - it "can update product and variant fields" do + it "updates product and variant fields" do within row_containing_name("Apples") do fill_in "Name", with: "Pommes" fill_in "SKU", with: "POM-00" @@ -236,7 +236,7 @@ describe 'As an admin, I can see the new product page' do expect(page).to have_content "Changes saved" end - it "can discard changes and reload latest data" do + it "discards changes and reloads latest data" do within row_containing_name("Apples") do fill_in "Name", with: "Pommes" end @@ -262,6 +262,39 @@ describe 'As an admin, I can see the new product page' do expect(page).to have_field "SKU", with: "APL-10" # Updated value shown end end + + it "shows errors for both product and variant fields" do + within row_containing_name("Apples") do + fill_in "Name", with: "" + fill_in "SKU", with: "A" * 256 + end + within row_containing_name("Medium box") do + fill_in "Name", with: "L" * 256 + fill_in "SKU", with: "1" * 256 + end + + expect { + click_button "Save changes" + product_a.reload + }.to_not change { product_a.name } + + # (there's no identifier displayed, so the user must remember which product it is..) + within row_containing_name("") do + expect(page).to have_field "Name", with: "" + expect(page).to have_content "can't be blank" + expect(page).to have_field "SKU", with: "A" * 256 + expect(page).to have_content "is too long" + end + pending + within row_containing_name("L" * 256) do + expect(page).to have_field "Name", with: "L" * 256 + expect(page).to have_field "SKU", with: "1" * 256 + expect(page).to have_content "is too long" + expect(page).to have_field "Price", with: "10.25" + end + + expect(page).to have_content "Please review the errors and try again" + end end def expect_page_to_be(page_number)