From ae3cd6f7e046bb1eac5d8e8800bee3dba1f0f7f6 Mon Sep 17 00:00:00 2001 From: David Cook Date: Thu, 13 Jul 2023 16:51:40 +1000 Subject: [PATCH] Add bulk_update product form for product name (For now at least,) we use one big standard Rails form, and ModelSet to update each record. Submitting with Reflex allows us to manage the loading state along with the rest of the page (although I would rather use the built in HTTP POST standard). Aria-label makes it a bit easier for testing (and accessibility software of course!). Technically it should have been aria-labelledby="id_of_column_header" but that would have resulted in more HTML and processing, which seemed silly. Best viewed with whitespace ignored. --- app/reflexes/products_reflex.rb | 51 +++++++ app/views/admin/products_v3/_table.html.haml | 138 +++++++++--------- config/locales/en.yml | 5 + config/routes/admin.rb | 4 +- spec/reflexes/products_reflex_spec.rb | 27 ++++ .../system/admin/products_v3/products_spec.rb | 35 ++++- 6 files changed, 190 insertions(+), 70 deletions(-) diff --git a/app/reflexes/products_reflex.rb b/app/reflexes/products_reflex.rb index bc8bcd9081..b2c845d864 100644 --- a/app/reflexes/products_reflex.rb +++ b/app/reflexes/products_reflex.rb @@ -31,6 +31,24 @@ class ProductsReflex < ApplicationReflex fetch_and_render_products end + def bulk_update + product_set = product_set_from_params + + product_set.collection.each { |p| authorize! :update, p } + + if product_set.save + # flash[:success] = with_locale { I18n.t('.success') } + # morph_admin_flashes # ERROR: selector morph type has already been set + + fetch_and_render_products + elsif product_set.errors.present? + # todo: render form with error messages + render json: { errors: product_set.errors }, status: :bad_request + else + render body: nil, status: :internal_server_error + end + end + private def init_filters_params @@ -130,4 +148,37 @@ class ProductsReflex < ApplicationReflex url.query += "&_category_id=#{@category_id}" if @category_id.present? url.to_s end + + # Similar to spree/admin/products_controller + def product_set_from_params + # Form field names: + # '[products][][name]' + # + # Resulting in params: + # "products" => { + # "" => { + # "name" => "Pommes", + # } + # } + + # For ModelSet, we transform to: + # { + # 0=> {:id=>"7449", "name"=>"Pommes"} + # } + # + # TO Consider: We could actually rearrange the form to suit that format more directly. eg: + # '[products][0][id]' (hidden field) + # '[products][0][name]' + collection_hash = products_bulk_params[:products].map { |id, attributes| + { id:, **attributes } + }.each_with_index.to_h { |p, i| + [i, p] + } + Sets::ProductSet.new(collection_attributes: collection_hash) + end + + def products_bulk_params + params.permit(products: ::PermittedAttributes::Product.attributes) + .to_h.with_indifferent_access + end end diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index 779f9c5275..bd27a5417e 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/app/views/admin/products_v3/_table.html.haml @@ -1,66 +1,74 @@ -%table.products - %col{ width:"15%" } - %col{ width:"5%", style: "max-width:5em" } - %col{ width:"8%" } - %col{ width:"5%", style: "max-width:5em"} - %col{ width:"5%", style: "max-width:5em"} - %col{ width:"10%" }= # producer - %col{ width:"10%" } - %col{ width:"5%" } - %col{ width:"5%", style: "max-width:5em" } - %thead - %tr - %th.align-left= t('admin.products_page.columns.name') - %th.align-right= t('admin.products_page.columns.sku') - %th.align-right= t('admin.products_page.columns.unit') - %th.align-right= t('admin.products_page.columns.price') - %th.align-right= t('admin.products_page.columns.on_hand') - %th.align-left= t('admin.products_page.columns.producer') - %th.align-left= t('admin.products_page.columns.category') - %th.align-left= t('admin.products_page.columns.tax_category') - %th.align-left= t('admin.products_page.columns.inherits_properties') - - products.each do |product| - %tbody.relaxed += form_with url: bulk_update_admin_products_v3_index_path, method: :patch, + html: {'data-reflex-serialize-form': true, 'data-reflex': 'submit->products#bulk_update'} do |form| + .container + .sixteen.columns.align-right + #fieldset + / = t('.products_modified', count: 'X') + = form.submit t('.reset'), type: :reset + = form.submit t('.save') + %table.products + %col{ width:"15%" } + %col{ width:"5%", style: "max-width:5em" } + %col{ width:"8%" } + %col{ width:"5%", style: "max-width:5em"} + %col{ width:"5%", style: "max-width:5em"} + %col{ width:"10%" }= # producer + %col{ width:"10%" } + %col{ width:"5%" } + %col{ width:"5%", style: "max-width:5em" } + %thead %tr - %td.align-left.header - .line-clamp-1= product.name - %td.align-right - .line-clamp-1= product.sku - %td.align-right - .line-clamp-1 - = 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 - -# 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 - %td.align-left - .line-clamp-1= product.supplier.name - %td.align-left - .line-clamp-1= 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) - - product.variants.each do |variant| - %tr.condensed - %td.align-left - .line-clamp-1= variant.display_name - %td.align-right - .line-clamp-1= variant.sku - %td.align-right - .line-clamp-1= variant.unit_to_display - %td.align-right - .line-clamp-1= number_to_currency(variant.price) - %td.align-right - .line-clamp-1= variant.on_hand || 0 #TODO: spec for this according to requirements. - %td.align-left - .line-clamp-1= 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. - %td.align-left - -# empty - + %th.align-left= t('admin.products_page.columns.name') + %th.align-right= t('admin.products_page.columns.sku') + %th.align-right= t('admin.products_page.columns.unit') + %th.align-right= t('admin.products_page.columns.price') + %th.align-right= t('admin.products_page.columns.on_hand') + %th.align-left= t('admin.products_page.columns.producer') + %th.align-left= t('admin.products_page.columns.category') + %th.align-left= t('admin.products_page.columns.tax_category') + %th.align-left= t('admin.products_page.columns.inherits_properties') + - products.each do |product| + = form.fields_for(product) do |product_form| + %tbody.relaxed + %tr + %td.align-left.header + .line-clamp-1= product_form.text_field :name, name: "[products][#{product.id}][name]", id: "_product_name_#{product.id}", 'aria-label': t('admin.products_page.columns.name') + %td.align-right + .line-clamp-1= product.sku + %td.align-right + .line-clamp-1 + = 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 + -# 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 + %td.align-left + .line-clamp-1= product.supplier.name + %td.align-left + .line-clamp-1= 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) + - product.variants.each do |variant| + %tr.condensed + %td.align-left + .line-clamp-1= variant.display_name + %td.align-right + .line-clamp-1= variant.sku + %td.align-right + .line-clamp-1= variant.unit_to_display + %td.align-right + .line-clamp-1= number_to_currency(variant.price) + %td.align-right + .line-clamp-1= variant.on_hand || 0 #TODO: spec for this according to requirements. + %td.align-left + .line-clamp-1= 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. + %td.align-left + -# empty diff --git a/config/locales/en.yml b/config/locales/en.yml index c9f57070de..7a3570d985 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -818,6 +818,11 @@ en: no_products_found: No products found import_products: Import multiple products no_products_found_for_search: No products found for your search criteria + table: + save: Save changes + reset: Discard changes + bulk_update: + success: "Products successfully updated" product_import: title: Product Import file_not_found: File not found or could not be opened diff --git a/config/routes/admin.rb b/config/routes/admin.rb index dea9207fb2..95da893872 100644 --- a/config/routes/admin.rb +++ b/config/routes/admin.rb @@ -70,7 +70,9 @@ Openfoodnetwork::Application.routes.draw do post '/product_import/reset_absent', to: 'product_import#reset_absent_products', as: 'product_import_reset_async' constraints FeatureToggleConstraint.new(:admin_style_v3) do - resources :products_v3, only: :index + resources :products_v3, as: :products_v3, only: :index do + patch :bulk_update, on: :collection + end end resources :variant_overrides do diff --git a/spec/reflexes/products_reflex_spec.rb b/spec/reflexes/products_reflex_spec.rb index 4b1b6a6b90..60fdcce81e 100644 --- a/spec/reflexes/products_reflex_spec.rb +++ b/spec/reflexes/products_reflex_spec.rb @@ -32,4 +32,31 @@ describe ProductsReflex, type: :reflex do end end end + + describe '#bulk_update' do + let!(:product_z) { create(:simple_product, name: "Zucchini") } + let!(:product_a) { create(:simple_product, name: "Apples") } + + it "saves valid changes" do + params = { + # '[products][][name]' + "products" => { + product_a.id.to_s => { + "name" => "Pommes", + } + } + } + + expect{ + reflex(:bulk_update, params:) + product_a.reload + }.to change(product_a, :name).to("Pommes") + end + end +end + +# Build and run a reflex using the context +# Parameters can be added with params: option +def reflex(method_name, opts = {}) + build_reflex(method_name:, **context.merge(opts)).run(method_name) end diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb index bddcfab8c1..b0e98484c4 100644 --- a/spec/system/admin/products_v3/products_spec.rb +++ b/spec/system/admin/products_v3/products_spec.rb @@ -24,7 +24,7 @@ describe 'As an admin, I can see the new product page' do end describe "sorting" do - let!(:product_z) { create(:simple_product, name: "Bananas") } + let!(:product_b) { create(:simple_product, name: "Bananas") } let!(:product_a) { create(:simple_product, name: "Apples") } before do @@ -32,7 +32,13 @@ describe 'As an admin, I can see the new product page' do end it "Should sort products alphabetically by default" do - expect(page).to have_content /Apples.*Bananas/ + within "table.products" do + # Gather input values, because page.content doesn't include them. + input_content = page.find_all('input[type=text]').map(&:value).join + + # Products are in correct order. + expect(input_content).to match /Apples.*Bananas/ + end end end @@ -91,7 +97,7 @@ describe 'As an admin, I can see the new product page' do search_for "searchable product" expect(page).to have_field "search_term", with: "searchable product" expect_products_count_to_be 1 - expect(page).to have_selector "table.products tbody tr td", text: product_by_name.name + expect(page).to have_field "Name", with: product_by_name.name click_link "Clear search" expect(page).to have_field "search_term", with: "" @@ -130,11 +136,32 @@ describe 'As an admin, I can see the new product page' do expect(page).to have_select "category_id", selected: "Category 1" expect_products_count_to_be 1 - expect(page).to have_selector "table.products tbody tr td", text: product_by_category.name + expect(page).to have_field "Name", with: product_by_category.name end end end + describe "updating" do + before do + visit admin_products_v3_index_url + end + + it "can update product fields" do + fill_in id: "_product_name_#{product_1.id}", with: "An updated name" + + expect { + click_button "Save changes" + product_1.reload + }.to( + change { product_1.name }.to("An updated name") + ) + + expect(page).to have_field id: "_product_name_#{product_1.id}", with: "An updated name" + pending + expect(page).to have_content "Changes saved" + end + end + def expect_page_to_be(page_number) expect(page).to have_selector ".pagination span.page.current", text: page_number.to_s end