diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index 1d8dda53ff..d91498e310 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -69,7 +69,7 @@ module Spree %w(weight volume).include?(variant.product&.variant_unit) } - validates :unit_value, numericality: { greater_than: 0 } + validates :unit_value, numericality: { greater_than: 0 }, allow_blank: true validates :price, numericality: { greater_than_or_equal_to: 0 } validates :unit_description, presence: true, if: ->(variant) { diff --git a/app/views/admin/products_v3/_product_row.html.haml b/app/views/admin/products_v3/_product_row.html.haml new file mode 100644 index 0000000000..3e6f5f6bb7 --- /dev/null +++ b/app/views/admin/products_v3/_product_row.html.haml @@ -0,0 +1,35 @@ +%td.with-image + %a.image-field{ href: admin_product_images_path(product), data: { controller: "modal", reflex: "click->products#edit_image", "current-id": product.id} } + = image_tag product.image&.url(:mini) || Spree::Image.default_image_url(:mini), width: 40, height: 40 + .button.secondary.mini= t('admin.products_page.image.edit') +%td.field.align-left.header + = f.hidden_field :id + = f.text_field :name, 'aria-label': t('admin.products_page.columns.name') + = error_message_on product, :name +%td.field + = f.text_field :sku, 'aria-label': t('admin.products_page.columns.sku') + = error_message_on product, :sku +%td.align-right + .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"] + ")" +%td.align-right + -# empty +%td.align-right + -# empty +%td.align-left + .content= product.supplier&.name +%td.align-left + .content= product.primary_taxon&.name +%td.align-left +%td.align-left + .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(VerticalEllipsisMenu::Component.new) do + = link_to t('admin.products_page.actions.edit'), edit_admin_product_path(product) + = link_to t('admin.products_page.actions.clone'), clone_admin_product_path(product) + %a{ "data-controller": "modal-link", "data-action": "click->modal-link#setModalDataSetOnConfirm click->modal-link#open", + "data-modal-link-target-value": "product-delete-modal", "class": "delete", + "data-modal-link-modal-dataset-value": {'data-current-id': product.id}.to_json } + = t('admin.products_page.actions.delete') diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index d14f073714..46de4db20d 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/app/views/admin/products_v3/_table.html.haml @@ -1,9 +1,9 @@ = form_with url: bulk_update_admin_products_path, method: :patch, id: "products-form", builder: BulkFormBuilder, - html: {'data-reflex-serialize-form': true, 'data-reflex': 'submit->products#bulk_update', - 'data-controller': "bulk-form", 'data-bulk-form-disable-selector-value': "#sort,#filters", - 'data-bulk-form-error-value': defined?(error_counts), - } do |form| + html: { data: { reflex: 'submit->products#bulk_update', 'reflex-serialize-form': true, + controller: "bulk-form", 'bulk-form-disable-selector-value': "#sort,#filters", + 'bulk-form-error-value': defined?(error_counts), + } } do |form| = render(partial: "admin/shared/flashes", locals: { flashes: }) if defined? flashes %table.products %col{ width:"4%" } @@ -50,85 +50,25 @@ %th.align-right= t('admin.products_page.columns.actions') - products.each_with_index do |product, product_index| = form.fields_for("products", product, index: product_index) do |product_form| - %tbody.relaxed{ 'data-record-id': product_form.object.id } + %tbody.relaxed{ data: { 'record-id': product_form.object.id, controller: "nested-form", + action: 'nested-form:add->bulk-form#registerElements' } } %tr - %td.with-image - %a.image-field{ href: admin_product_images_path(product), data: { controller: "modal", reflex: "click->products#edit_image", "current-id": product.id} } - = image_tag product.image&.url(:mini) || Spree::Image.default_image_url(:mini), width: 40, height: 40 - .button.secondary.mini= t('admin.products_page.image.edit') - %td.field.align-left.header - = product_form.hidden_field :id - = product_form.text_field :name, 'aria-label': t('admin.products_page.columns.name') - = 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 - .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"] + ")" - %td.align-right - -# empty - %td.align-right - -# empty - %td.align-left - .content= product.supplier&.name - %td.align-left - .content= product.primary_taxon&.name - %td.align-left - %td.align-left - .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(VerticalEllipsisMenu::Component.new) do - = link_to t('admin.products_page.actions.edit'), edit_admin_product_path(product) - = link_to t('admin.products_page.actions.clone'), clone_admin_product_path(product) - %a{ "data-controller": "modal-link", "data-action": "click->modal-link#setModalDataSetOnConfirm click->modal-link#open", - "data-modal-link-target-value": "product-delete-modal", "class": "delete", - "data-modal-link-modal-dataset-value": {'data-current-id': product.id}.to_json } - = t('admin.products_page.actions.delete') + = render partial: 'product_row', locals: { product:, f: product_form } - product.variants.each_with_index do |variant, variant_index| - = form.fields_for("products][#{product_index}][variants_attributes][", variant, variant_index:) do |variant_form| + = form.fields_for("products][#{product_index}][variants_attributes][", variant, index: variant_index) do |variant_form| %tr.condensed - %td - -# empty - %td.field - = variant_form.hidden_field :id - = variant_form.text_field :display_name, 'aria-label': t('admin.products_page.columns.name'), placeholder: product.name - = 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 - .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.field.on-hand__wrapper{'data-controller': "popout"} - %button.on-hand__button{'data-popout-target': "button", 'aria-label': t('admin.products_page.columns.on_hand')} - = variant.on_demand ? t(:on_demand) : variant.on_hand - %div.on-hand__popout{ style: 'display: none;', 'data-controller': 'toggle-control', 'data-popout-target': "dialog" } - .field - = variant_form.number_field :on_hand, min: 0, 'aria-label': t('admin.products_page.columns.on_hand'), 'data-toggle-control-target': 'control', disabled: variant_form.object.on_demand - = error_message_on variant, :on_hand - .field.checkbox - = variant_form.label :on_demand do - = variant_form.check_box :on_demand, 'data-action': 'change->toggle-control#disableIfPresent change->popout#closeIfChecked' - = t(:on_demand) - %td.align-left - .content= variant.product.supplier&.name # same as product - %td.align-left - -# empty - %td.align-left - .content= variant.tax_category&.name || "None" # TODO: convert to dropdown, else translate hardcoded string. - %td.align-left - -# empty - %td.align-right - = render(VerticalEllipsisMenu::Component.new) do - = link_to t('admin.products_page.actions.edit'), edit_admin_product_variant_path(product, variant) - - if product.variants.size > 1 - %a{ "data-controller": "modal-link", "data-action": "click->modal-link#setModalDataSetOnConfirm click->modal-link#open", - "data-modal-link-target-value": "variant-delete-modal", "class": "delete", - "data-modal-link-modal-dataset-value": {'data-current-id': variant.id}.to_json } - = t('admin.products_page.actions.delete') + = render partial: 'variant_row', locals: { variant:, f: variant_form } + + = form.fields_for("products][#{product_index}][variants_attributes][NEW_RECORD", product.variants.build) do |new_variant_form| + %template{ 'data-nested-form-target': "template" } + %tr.condensed + = render partial: 'variant_row', locals: { variant: new_variant_form.object, f: new_variant_form } + + %tr{ 'data-nested-form-target': "target" } + %tr.condensed + %td + %td{ colspan: 10 } + %button.secondary.condensed.naked.icon-plus{ 'data-action': "nested-form#add", + 'aria-label': t('.new_variant') } + =t('.new_variant') diff --git a/app/views/admin/products_v3/_variant_row.html.haml b/app/views/admin/products_v3/_variant_row.html.haml new file mode 100644 index 0000000000..e7d1100d4c --- /dev/null +++ b/app/views/admin/products_v3/_variant_row.html.haml @@ -0,0 +1,47 @@ +%td + -# empty +%td.field + = f.hidden_field :id + = f.text_field :display_name, 'aria-label': t('admin.products_page.columns.name'), placeholder: variant.product.name + = error_message_on variant, :display_name +%td.field + = f.text_field :sku, 'aria-label': t('admin.products_page.columns.sku') + = error_message_on variant, :sku +- if variant.persisted? + %td.align-right + .content= variant.unit_to_display +- else # until unit component is developed, use a basic input just so we can create new records + %td.field + = f.number_field :unit_value, 'aria-label': t('admin.products_page.columns.unit') + = error_message_on variant, :unit_value +%td.field + = f.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.field.on-hand__wrapper{'data-controller': "popout"} + %button.on-hand__button{'data-popout-target': "button", 'aria-label': t('admin.products_page.columns.on_hand')} + = variant.on_demand ? t(:on_demand) : variant.on_hand + %div.on-hand__popout{ style: 'display: none;', 'data-controller': 'toggle-control', 'data-popout-target': "dialog" } + .field + = f.number_field :on_hand, min: 0, 'aria-label': t('admin.products_page.columns.on_hand'), 'data-toggle-control-target': 'control', disabled: f.object.on_demand + = error_message_on variant, :on_hand + .field.checkbox + = f.label :on_demand do + = f.check_box :on_demand, 'data-action': 'change->toggle-control#disableIfPresent change->popout#closeIfChecked' + = t(:on_demand) +%td.align-left + .content= variant.product.supplier&.name # same as product +%td.align-left + -# empty +%td.align-left + .content= variant.tax_category&.name || "None" # TODO: convert to dropdown, else translate hardcoded string. +%td.align-left + -# empty +%td.align-right + - if variant.persisted? + = render(VerticalEllipsisMenu::Component.new) do + = link_to t('admin.products_page.actions.edit'), edit_admin_product_variant_path(variant.product, variant) + - if variant.product.variants.size > 1 + %a{ "data-controller": "modal-link", "data-action": "click->modal-link#setModalDataSetOnConfirm click->modal-link#open", + "data-modal-link-target-value": "variant-delete-modal", "class": "delete", + "data-modal-link-modal-dataset-value": {'data-current-id': variant.id}.to_json } + = t('admin.products_page.actions.delete') diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js index 91af33e9e0..98a6063a0e 100644 --- a/app/webpacker/controllers/bulk_form_controller.js +++ b/app/webpacker/controllers/bulk_form_controller.js @@ -13,18 +13,7 @@ export default class BulkFormController extends Controller { this.form = this.element; // Start listening for any changes within the form - // this.element.addEventListener('change', this.toggleChanged.bind(this)); // dunno why this doesn't work - for (const element of this.form.elements) { - element.addEventListener("input", this.toggleChanged.bind(this)); // immediately respond to any change - - // Set up a tree of fields according to their associated record - const recordContainer = element.closest("[data-record-id]"); // The JS could be more efficient if this data was added to each element. But I didn't want to pollute the HTML too much. - const recordId = recordContainer && recordContainer.dataset.recordId; - if (recordId) { - this.recordElements[recordId] ||= []; - this.recordElements[recordId].push(element); - } - } + this.#registerElements(this.form.elements); this.toggleFormChanged(); } @@ -35,6 +24,15 @@ export default class BulkFormController extends Controller { window.removeEventListener("beforeunload", this.preventLeavingBulkForm); } + // Register any new elements (may be called by another controller after dynamically adding fields) + registerElements() { + const registeredElements = Object.values(this.recordElements).flat(); + // Select only elements that haven't been registered yet + const newElements = Array.from(this.form.elements).filter(n => !registeredElements.includes(n)); + + this.#registerElements(newElements); + } + toggleChanged(e) { const element = e.target; element.classList.toggle("changed", this.#isChanged(element)); @@ -50,7 +48,7 @@ export default class BulkFormController extends Controller { const formChanged = changedRecordCount > 0 || this.errorValue; // Show actions - this.actionsTarget.classList.toggle("hidden", !formChanged); + this.hasActionsTarget && this.actionsTarget.classList.toggle("hidden", !formChanged); this.#disableOtherElements(formChanged); // like filters and sorting // Display number of records changed @@ -78,6 +76,20 @@ export default class BulkFormController extends Controller { // private + #registerElements(elements) { + for (const element of elements) { + element.addEventListener("input", this.toggleChanged.bind(this)); // immediately respond to any change + + // Set up a tree of fields according to their associated record + const recordContainer = element.closest("[data-record-id]"); // The JS could be more efficient if this data was added to each element. But I didn't want to pollute the HTML too much. + const recordId = recordContainer && recordContainer.dataset.recordId; + if (recordId) { + this.recordElements[recordId] ||= []; + this.recordElements[recordId].push(element); + } + } + } + #disableOtherElements(disable) { if (!this.hasDisableSelectorValue) return; diff --git a/app/webpacker/controllers/index.js b/app/webpacker/controllers/index.js index 00245fdf88..4e31f9a433 100644 --- a/app/webpacker/controllers/index.js +++ b/app/webpacker/controllers/index.js @@ -6,12 +6,15 @@ import StimulusReflex from "stimulus_reflex"; import consumer from "../channels/consumer"; import controller from "../controllers/application_controller"; import CableReady from "cable_ready"; +import NestedForm from 'stimulus-rails-nested-form/dist/stimulus-rails-nested-form.umd.js' // the default module entry point is broken + const application = Application.start(); const context = require.context("controllers", true, /_controller\.js$/); const contextComponents = require.context("../../components", true, /_controller\.js$/); application.load(definitionsFromContext(context).concat(definitionsFromContext(contextComponents))); +application.register('nested-form', NestedForm); application.consumer = consumer; StimulusReflex.initialize(application, { controller, isolate: true }); StimulusReflex.debug = process.env.RAILS_ENV === "development"; diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss index 9cc1d1845f..731f487dba 100644 --- a/app/webpacker/css/admin/products_v3.scss +++ b/app/webpacker/css/admin/products_v3.scss @@ -88,6 +88,11 @@ height: 100%; } } + + // Reveal naked button text when any part of row is hovered + button.naked { + color: $color-link; + } } th, @@ -160,20 +165,18 @@ } // "Naked" inputs. Row hover helps reveal them. - tbody input:not([type="checkbox"]) { - background-color: $color-tbl-cell-bg; - height: auto; - font-size: inherit; - font-weight: inherit; - - &:not(:focus):not(.changed):not([disabled]) { - border-color: transparent; + tbody { + input:not([type="checkbox"]) { + background-color: $color-tbl-cell-bg; + height: auto; + font-size: inherit; + font-weight: inherit; } - } - .field_with_errors { - input { - border-color: $color-error; + :not(.field_with_errors) > { + input:not([type="checkbox"]):not(:focus):not(.changed):not([disabled]) { + border-color: transparent; + } } } } diff --git a/app/webpacker/css/admin_v3/components/buttons.scss b/app/webpacker/css/admin_v3/components/buttons.scss index 0215f75952..f213dd7bc3 100644 --- a/app/webpacker/css/admin_v3/components/buttons.scss +++ b/app/webpacker/css/admin_v3/components/buttons.scss @@ -1,3 +1,6 @@ +// Button styles +// Design reference: https://github.com/openfoodfoundation/openfoodnetwork/wiki/Design-styleguide%3A-links-and-buttons + @mixin disabled-button() { &:disabled, &:disabled:hover { @@ -47,11 +50,7 @@ button:not(.plain):not(.trix-button), color: $color-btn-hover-text; } - &.fullwidth { - width: 100%; - text-align: center; - } - + // --- Colours --- &.secondary { background-color: $color-btn-secondary-bg; color: $color-link; @@ -65,6 +64,25 @@ button:not(.plain):not(.trix-button), border-color: $color-link-focus; color: $color-link-focus; } + + // "Naked" variation: text appears on hover + &.naked { + border: none; + color: transparent; + + &:before { + color: $color-link; + } + + &:hover { + background-color: $color-btn-secondary-bg; + color: $color-link; + } + &:focus { + background-color: $color-btn-secondary-hover-bg; + color: $color-link-focus; + } + } } &.disruptive { @@ -86,6 +104,7 @@ button:not(.plain):not(.trix-button), } } + // --- Sizes --- &.mini { line-height: 18px; height: auto; // DC: I don't like fixed heights. @@ -110,15 +129,9 @@ button:not(.plain):not(.trix-button), text-transform: uppercase; } - &.red { - background-color: $color-btn-red-bg; - border-color: $color-btn-red-bg; - @include disabled-button(); // required for specifity - - &:hover { - background-color: $color-btn-red-hover-bg; - border-color: $color-btn-red-hover-bg; - } + &.fullwidth { + width: 100%; + text-align: center; } .badge { @@ -144,6 +157,7 @@ button:not(.plain):not(.trix-button), } } +// --- Reset buttons --- input[type="reset"] { // Reset button looks like a link, but has a border the same as buttons when active. background: none; diff --git a/app/webpacker/css/admin_v3/shared/typography.scss b/app/webpacker/css/admin_v3/shared/typography.scss index a716aeeeaf..84822b39da 100644 --- a/app/webpacker/css/admin_v3/shared/typography.scss +++ b/app/webpacker/css/admin_v3/shared/typography.scss @@ -45,6 +45,7 @@ b { } // links +// Design reference: https://github.com/openfoodfoundation/openfoodnetwork/wiki/Design-styleguide%3A-links-and-buttons //-------------------------------------------------------------- a:not(.button) { color: $color-link; diff --git a/config/locales/en.yml b/config/locales/en.yml index 6f3341ee7d..e615792f8c 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -872,8 +872,9 @@ en: invalid: one: "%{count} product could not be saved. Please review the errors and try again." other: "%{count} products could not be saved. Please review the errors and try again." - save: Save changes reset: Discard changes + save: Save changes + new_variant: New variant bulk_update: success: Changes saved edit_image: diff --git a/package.json b/package.json index 873c88bff3..f5bd1b0f56 100644 --- a/package.json +++ b/package.json @@ -34,6 +34,7 @@ "shortcut-buttons-flatpickr": "^0.4.0", "stimulus": "^3.2.2", "stimulus-flatpickr": "^1.4.0", + "stimulus-rails-nested-form": "https://github.com/openfoodfoundation/stimulus-rails-nested-form.git#dist", "stimulus_reflex": "3.5.0-rc3", "tom-select": "^2.3.1", "trix": "^2.0.10", diff --git a/spec/javascripts/stimulus/bulk_form_controller_test.js b/spec/javascripts/stimulus/bulk_form_controller_test.js index b0a3386ce9..7571191708 100644 --- a/spec/javascripts/stimulus/bulk_form_controller_test.js +++ b/spec/javascripts/stimulus/bulk_form_controller_test.js @@ -181,6 +181,48 @@ describe("BulkFormController", () => { }); }); + describe("Adding new fields", () => { + beforeEach(() => { + document.body.innerHTML = ` +
+ + + +
+ +
+ +
+ `; + }); + + describe("registerElements", () => { + beforeEach(() => { + // Add new field after controller has initialised + input1a.insertAdjacentHTML("afterend", template.innerHTML); + + // Trigger bulk-form#registerElements + form.dispatchEvent(new Event("custom-event")); + }); + + it("onInput", () => { + input1b.value = 'updated1b'; + input1b.dispatchEvent(new Event("input")); + // Expect only updated field to show changed + expect(input1b.classList).toContain('changed'); + expect(input2.classList).not.toContain('changed'); + + // Change back to original value + input1b.value = 'initial1b'; + input1b.dispatchEvent(new Event("input")); + expect(input1b.classList).not.toContain('changed'); + }); + }) + }); + // unable to test disconnect at this stage // describe("disconnect()", () => { // it("resets other elements", () => { diff --git a/spec/reflexes/products_reflex_spec.rb b/spec/reflexes/products_reflex_spec.rb index fa87b367e4..72bab6b015 100644 --- a/spec/reflexes/products_reflex_spec.rb +++ b/spec/reflexes/products_reflex_spec.rb @@ -36,8 +36,10 @@ describe ProductsReflex, type: :reflex, feature: :admin_style_v3 do describe '#bulk_update' do let!(:variant_a1) { - create(:variant, product: product_a, display_name: "Medium box", sku: "APL-01", price: 5.25, - on_hand: 5, on_demand: false) + product_a.variants.first.tap{ |v| + v.update! display_name: "Medium box", sku: "APL-01", price: 5.25, on_hand: 5, + on_demand: false + } } let!(:product_c) { create(:simple_product, name: "Carrots", sku: "CAR-00") } let!(:product_b) { create(:simple_product, name: "Bananas", sku: "BAN-00") } @@ -101,6 +103,63 @@ describe ProductsReflex, type: :reflex, feature: :admin_style_v3 do expect(flash).to include success: "Changes saved" end + it "creates new variants" do + # Form field names: + # '[products][0][id]' (hidden field) + # '[products][0][name]' + # '[products][0][variants_attributes][0][id]' (hidden field) + # '[products][0][variants_attributes][0][display_name]' + # '[products][0][variants_attributes][1][display_name]' (id is omitted for new record) + # '[products][0][variants_attributes][2][display_name]' (more than 1 new record is allowed) + params = { + "products" => { + "0" => { + "id" => product_a.id.to_s, + "name" => "Pommes", + "variants_attributes" => { + "0" => { + "id" => variant_a1.id.to_s, + "display_name" => "Large box", + }, + "1" => { + "display_name" => "Small box", + "sku" => "POM-02", + "price" => "5.25", + "unit_value" => "0.5", + }, + "2" => { + "sku" => "POM-03", + "price" => "15.25", + "unit_value" => "2", + }, + }, + }, + }, + } + + expect{ + run_reflex(:bulk_update, params:) + product_a.reload + variant_a1.reload + }.to change{ product_a.name }.to("Pommes") + .and change{ variant_a1.display_name }.to("Large box") + .and change{ product_a.variants.count }.by(2) + + variant_a2 = product_a.variants[1] + expect(variant_a2.display_name).to eq "Small box" + expect(variant_a2.sku).to eq "POM-02" + expect(variant_a2.price).to eq 5.25 + expect(variant_a2.unit_value).to eq 0.5 + + variant_a3 = product_a.variants[2] + expect(variant_a3.display_name).to be_nil + expect(variant_a3.sku).to eq "POM-03" + expect(variant_a3.price).to eq 15.25 + expect(variant_a3.unit_value).to eq 2 + + expect(flash).to include success: "Changes saved" + end + describe "sorting" do let(:params) { { diff --git a/spec/services/sets/product_set_spec.rb b/spec/services/sets/product_set_spec.rb index 4dd6b94315..e35dfa6d17 100644 --- a/spec/services/sets/product_set_spec.rb +++ b/spec/services/sets/product_set_spec.rb @@ -243,6 +243,46 @@ describe Sets::ProductSet do end end end + + context "new variant" do + let(:variants_attributes) { + [ + { id: product.variants.first.id.to_s }, # default variant unchanged + { sku: "new sku", price: "5.00", unit_value: "5" }, # omit ID for new variant + ] + } + + it "creates new variant" do + expect { + product_set.save + expect(product_set.errors).to be_empty + }.to change { product.variants.count }.by(1) + + expect(product.variants.last.sku).to eq "new sku" + expect(product.variants.last.price).to eq 5.00 + expect(product.variants.last.unit_value).to eq 5 + end + + context "variant has error" do + let(:variants_attributes) { + [ + { id: product.variants.first.id.to_s }, # default variant unchanged + { sku: "new sku", unit_value: "blah" }, # price missing, unit_value should be number + ] + } + + include_examples "nothing saved" + + it "logs variant errors" do + product_set.save + expect(product_set.errors.full_messages).to include( + "Variant price is not a number", + "Variant price can't be blank", + "Variant unit value is not a number" + ) + end + end + end end context 'when there are multiple products' do diff --git a/spec/system/admin/bulk_product_update_spec.rb b/spec/system/admin/bulk_product_update_spec.rb index 199c8e218f..a15d7621b4 100644 --- a/spec/system/admin/bulk_product_update_spec.rb +++ b/spec/system/admin/bulk_product_update_spec.rb @@ -307,7 +307,7 @@ describe ' click_button 'Save Changes', match: :first expect(page.find("#status-message")) - .to have_content "Variant unit value can't be blank Variant unit value is not a number" + .to have_content "Variant unit value can't be blank" end it "creating a variant with unit value is: '120g' and 'on_demand' checked" do @@ -323,7 +323,7 @@ describe ' click_button 'Save Changes', match: :first expect(page.find("#status-message")) - .to have_content "Variant unit value can't be blank Variant unit value is not a number" + .to have_content "Variant unit value can't be blank" end end end diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb index 8c8d2e18de..125fe3c185 100644 --- a/spec/system/admin/products_v3/products_spec.rb +++ b/spec/system/admin/products_v3/products_spec.rb @@ -164,8 +164,10 @@ describe 'As an admin, I can manage products', feature: :admin_style_v3 do describe "updating" do let!(:variant_a1) { - create(:variant, product: product_a, display_name: "Medium box", sku: "APL-01", price: 5.25, - on_hand: 5, on_demand: false) + product_a.variants.first.tap{ |v| + v.update! display_name: "Medium box", sku: "APL-01", price: 5.25, on_hand: 5, + on_demand: false + } } let!(:product_a) { create(:simple_product, name: "Apples", sku: "APL-00") } before do @@ -193,6 +195,8 @@ describe 'As an admin, I can manage products', feature: :admin_style_v3 do expect { click_button "Save changes" + + expect(page).to have_content "Changes saved" product_a.reload variant_a1.reload }.to change { product_a.name }.to("Pommes") @@ -213,7 +217,6 @@ describe 'As an admin, I can manage products', feature: :admin_style_v3 do expect(page).to have_css "button[aria-label='On Hand']", text: "6" end - expect(page).to have_content "Changes saved" end it "switches stock to on-demand" do @@ -226,6 +229,8 @@ describe 'As an admin, I can manage products', feature: :admin_style_v3 do expect { click_button "Save changes" + + expect(page).to have_content "Changes saved" product_a.reload variant_a1.reload }.to change{ variant_a1.on_demand }.to(true) @@ -233,8 +238,6 @@ describe 'As an admin, I can manage products', feature: :admin_style_v3 do within row_containing_name("Medium box") do expect(page).to have_css "button[aria-label='On Hand']", text: "On demand" end - - expect(page).to have_content "Changes saved" end it "discards changes and reloads latest data" do @@ -288,13 +291,13 @@ describe 'As an admin, I can manage products', feature: :admin_style_v3 do expect { click_button "Save changes" + + expect(page).to have_content "1 product was saved correctly" + expect(page).to have_content "1 product could not be saved" + expect(page).to have_content "Please review the errors and try again" product_a.reload }.to_not change { product_a.name } - expect(page).to have_content("1 product was saved correctly") - expect(page).to have_content("1 product could not be saved") - expect(page).to have_content "Please review the errors and try again" - # (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: "" @@ -313,19 +316,264 @@ describe 'As an admin, I can manage products', feature: :admin_style_v3 do end it "saves changes after fixing errors" do - within row_containing_name("Apples") do + expect { + click_button "Save changes" + + expect(page).to have_content("1 product could not be saved") + product_a.reload + }.to_not change { product_a.name } + + within row_containing_name("") do fill_in "Name", with: "Pommes" fill_in "SKU", with: "POM-00" end expect { click_button "Save changes" + + expect(page).to have_content "Changes saved" product_a.reload variant_a1.reload }.to change { product_a.name }.to("Pommes") .and change{ product_a.sku }.to("POM-00") + end + end - expect(page).to have_content "Changes saved" + describe "adding variants" do + it "creates a new variant" do + click_on "New variant" + + # find empty row for Apples + new_variant_row = find_field("Name", placeholder: "Apples", with: "").ancestor("tr") + expect(new_variant_row).to be_present + + within new_variant_row do + fill_in "Name", with: "Large box" + fill_in "SKU", with: "APL-02" + fill_in "Unit", with: 1000 + fill_in "Price", with: 10.25 + click_on "On Hand" # activate popout + end + fill_in "On Hand", with: "3" + + expect { + click_button "Save changes" + + expect(page).to have_content "Changes saved" + product_a.reload + }.to change { product_a.variants.count }.by(1) + + new_variant = product_a.variants.last + expect(new_variant.display_name).to eq "Large box" + expect(new_variant.sku).to eq "APL-02" + expect(new_variant.price).to eq 10.25 + expect(new_variant.unit_value).to eq 1000 + expect(new_variant.on_hand).to eq 3 + + within row_containing_name("Large box") do + expect(page).to have_field "Name", with: "Large box" + expect(page).to have_field "SKU", with: "APL-02" + expect(page).to have_field "Price", with: "10.25" + expect(page).to have_content "1kg" + expect(page).to have_css "button[aria-label='On Hand']", text: "3" + end + end + + context "with invalid data" do + before do + click_on "New variant" + + # find empty row for Apples + new_variant_row = find_field("Name", placeholder: "Apples", with: "").ancestor("tr") + expect(new_variant_row).to be_present + + within new_variant_row do + fill_in "Name", with: "N" * 256 # too long + fill_in "SKU", with: "n" * 256 + fill_in "Unit", with: "" # can't be blank + fill_in "Price", with: "10.25" # valid + end + end + + it "shows errors for both existing and new variant fields" do + # Update existing variant with invalid data too + within row_containing_name("Medium box") do + fill_in "Name", with: "M" * 256 + fill_in "SKU", with: "m" * 256 + fill_in "Price", with: "10.25" + end + + expect { + click_button "Save changes" + + expect(page).to have_content "1 product could not be saved" + expect(page).to have_content "Please review the errors and try again" + variant_a1.reload + }.to_not change { variant_a1.display_name } + + # New variant + within row_containing_name("N" * 256) do + expect(page).to have_field "Name", with: "N" * 256 + expect(page).to have_field "SKU", with: "n" * 256 + expect(page).to have_content "is too long" + expect(page).to have_field "Unit", with: "" + expect(page).to have_content "can't be blank" + expect(page).to have_field "Price", with: "10.25" # other updated value is retained + end + + # Existing variant + within row_containing_name("M" * 256) do + expect(page).to have_field "Name", with: "M" * 256 + expect(page).to have_field "SKU", with: "m" * 256 + expect(page).to have_content "is too long" + end + end + + it "saves changes after fixing errors" do + expect { + click_button "Save changes" + + variant_a1.reload + }.to_not change { variant_a1.display_name } + + within row_containing_name("N" * 256) do + fill_in "Name", with: "Nice box" + fill_in "SKU", with: "APL-02" + fill_in "Unit", with: 200 + end + + expect { + click_button "Save changes" + + expect(page).to have_content "Changes saved" + product_a.reload + }.to change { product_a.variants.count }.by(1) + + new_variant = product_a.variants.last + expect(new_variant.display_name).to eq "Nice box" + expect(new_variant.sku).to eq "APL-02" + expect(new_variant.price).to eq 10.25 + expect(new_variant.unit_value).to eq 200 + end + end + end + + describe "adding variants" do + it "creates a new variant" do + click_on "New variant" + + # find empty row for Apples + new_variant_row = find_field("Name", placeholder: "Apples", with: "").ancestor("tr") + expect(new_variant_row).to be_present + + within new_variant_row do + fill_in "Name", with: "Large box" + fill_in "SKU", with: "APL-02" + fill_in "Unit", with: 1000 + fill_in "Price", with: 10.25 + click_on "On Hand" # activate popout + end + fill_in "On Hand", with: "3" + + expect { + click_button "Save changes" + + expect(page).to have_content "Changes saved" + product_a.reload + }.to change { product_a.variants.count }.by(1) + + new_variant = product_a.variants.last + expect(new_variant.display_name).to eq "Large box" + expect(new_variant.sku).to eq "APL-02" + expect(new_variant.price).to eq 10.25 + expect(new_variant.unit_value).to eq 1000 + expect(new_variant.on_hand).to eq 3 + + within row_containing_name("Large box") do + expect(page).to have_field "Name", with: "Large box" + expect(page).to have_field "SKU", with: "APL-02" + expect(page).to have_field "Price", with: "10.25" + expect(page).to have_content "1kg" + expect(page).to have_css "button[aria-label='On Hand']", text: "3" + end + end + + context "with invalid data" do + before do + click_on "New variant" + + # find empty row for Apples + new_variant_row = find_field("Name", placeholder: "Apples", with: "").ancestor("tr") + expect(new_variant_row).to be_present + + within new_variant_row do + fill_in "Name", with: "N" * 256 # too long + fill_in "SKU", with: "n" * 256 + fill_in "Unit", with: "" # can't be blank + fill_in "Price", with: "10.25" # valid + end + end + + it "shows errors for both existing and new variant fields" do + # Update existing variant with invalid data too + within row_containing_name("Medium box") do + fill_in "Name", with: "M" * 256 + fill_in "SKU", with: "m" * 256 + fill_in "Price", with: "10.25" + end + + expect { + click_button "Save changes" + + expect(page).to have_content "1 product could not be saved" + expect(page).to have_content "Please review the errors and try again" + variant_a1.reload + }.to_not change { variant_a1.display_name } + + # New variant + within row_containing_name("N" * 256) do + expect(page).to have_field "Name", with: "N" * 256 + expect(page).to have_field "SKU", with: "n" * 256 + expect(page).to have_content "is too long" + expect(page).to have_field "Unit", with: "" + expect(page).to have_content "can't be blank" + expect(page).to have_field "Price", with: "10.25" # other updated value is retained + end + + # Existing variant + within row_containing_name("M" * 256) do + expect(page).to have_field "Name", with: "M" * 256 + expect(page).to have_field "SKU", with: "m" * 256 + expect(page).to have_content "is too long" + end + end + + it "saves changes after fixing errors" do + expect { + click_button "Save changes" + + variant_a1.reload + }.to_not change { variant_a1.display_name } + + within row_containing_name("N" * 256) do + fill_in "Name", with: "Nice box" + fill_in "SKU", with: "APL-02" + fill_in "Unit", with: "200" + end + + expect { + click_button "Save changes" + + expect(page).to have_content "Changes saved" + product_a.reload + }.to change { product_a.variants.count }.by(1) + + new_variant = product_a.variants.last + expect(new_variant.display_name).to eq "Nice box" + expect(new_variant.sku).to eq "APL-02" + expect(new_variant.price).to eq 10.25 + expect(new_variant.unit_value).to eq 200 + end end end diff --git a/yarn.lock b/yarn.lock index 966d460acf..433742f9b6 100644 --- a/yarn.lock +++ b/yarn.lock @@ -8350,6 +8350,10 @@ stimulus-flatpickr@^1.4.0: resolved "https://registry.yarnpkg.com/stimulus-flatpickr/-/stimulus-flatpickr-1.4.0.tgz#a41071a3e69cfc50b7eaaacf356fc0ab1ab0543c" integrity sha512-rcC/c9+E+f5W2kOjaaLShtf3i+p95ACqt+oGzSAgeuZh2YeIN8gW4EWO7h0STBLzSVPl6BjIfPWP7upMPavIVQ== +"stimulus-rails-nested-form@https://github.com/openfoodfoundation/stimulus-rails-nested-form.git#dist": + version "4.1.0" + resolved "https://github.com/openfoodfoundation/stimulus-rails-nested-form.git#d3b82ea638a7156f1122736cf739ab1821a1817e" + stimulus@^3.2.2: version "3.2.2" resolved "https://registry.yarnpkg.com/stimulus/-/stimulus-3.2.2.tgz#a2e955f43e12e2e5784b175d4df5517ef678aa68"