Merge pull request #11750 from dacook/buu-editing-part5c-show-summary-11059

[BUU] Showing error summary at top of form
This commit is contained in:
Maikel
2023-11-03 15:45:24 +11:00
committed by GitHub
12 changed files with 366 additions and 129 deletions

View File

@@ -41,8 +41,7 @@ class ProductsReflex < ApplicationReflex
# flash[:success] = with_locale { I18n.t('.success') }
# morph_admin_flashes # ERROR: selector morph type has already been set
elsif product_set.errors.present?
# @error_msg = with_locale{ I18n.t('.products_have_error', count: product_set.invalid.count) }
@error_msg = "#{product_set.invalid.count} products have errors."
@error_counts = { saved: product_set.saved_count, invalid: product_set.invalid.count }
end
render_products_form
@@ -87,10 +86,12 @@ class ProductsReflex < ApplicationReflex
end
def render_products_form
locals = { products: @products }
locals[:error_counts] = @error_counts if @error_counts.present?
cable_ready.replace(
selector: "#products-form",
html: render(partial: "admin/products_v3/table",
locals: { products: @products, error_msg: @error_msg })
html: render(partial: "admin/products_v3/table", locals:)
).broadcast
morph :nothing

View File

@@ -8,11 +8,15 @@ module Sets
# }
#
class ProductSet < ModelSet
attr_reader :saved_count
def initialize(attributes = {})
super(Spree::Product, [], attributes)
end
def save
@saved_count = 0
# Attempt to save all records, collecting model errors.
@collection_hash.each_value.map do |product_attributes|
update_product_attributes(product_attributes)
@@ -71,7 +75,10 @@ module Sets
validate_presence_of_unit_value_in_product(product)
product.errors.empty? && product.save
changed = product.changed?
success = product.errors.empty? && product.save
count_result(success && changed)
success
end
def validate_presence_of_unit_value_in_product(product)
@@ -105,8 +112,15 @@ module Sets
if variant.present?
variant.update(variant_attributes.except(:id))
else
create_variant(product, variant_attributes)
variant = create_variant(product, variant_attributes)
end
# Copy any variant errors to product
variant&.errors&.each do |error|
# The name is namespaced to avoid confusion with product attrs of same name.
product.errors.add("variant_#{error.attribute}".to_sym, error.message)
end
variant&.errors.blank?
end
def create_variant(product, variant_attributes)
@@ -117,11 +131,7 @@ module Sets
on_demand = variant_attributes.delete(:on_demand)
variant = product.variants.create(variant_attributes)
if variant.errors.present?
product.errors.merge!(variant.errors)
return false
end
return variant if variant.errors.present?
begin
variant.on_demand = on_demand if on_demand.present?
@@ -130,6 +140,12 @@ module Sets
notify_bugsnag(e, product, variant, variant_attributes)
raise e
end
variant
end
def count_result(saved)
@saved_count += 1 if saved
end
def notify_bugsnag(error, product, variant, variant_attributes)

View File

@@ -4,6 +4,6 @@
- if search_term.present? || producer_id.present? || category_id.present?
%a{ href: "#", class: "button disruptive", data: { reflex: "click->products#clear_search" } }
= t(".pagination.clear_search")
%div.with-dropdown
%form.with-dropdown
= t(".pagination.per_page.show")
= select_tag :per_page, options_for_select([15, 25, 50, 100].collect{|i| [t('.pagination.per_page.per_page', num: i), i]}, pagy.items), data: { reflex: "change->products#change_per_page" }

View File

@@ -1,15 +1,18 @@
= 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"} do |form|
%fieldset.form-actions.hidden{ 'data-bulk-form-target': "actions" }
'data-controller': "bulk-form", 'data-bulk-form-disable-selector-value': "#sort,#filters",
'data-bulk-form-error-value': defined?(error_counts),
} do |form|
%fieldset.form-actions{ class: ("hidden" unless defined?(error_counts)), 'data-bulk-form-target': "actions" }
.container
.status.ten.columns
.changed_summary{ 'data-bulk-form-target': "changedSummary", 'data-translation-key': 'admin.products_v3.table.changed_summary'}
- if defined?(error_msg) && error_msg.present?
.error
= error_msg
.form-buttons.six.columns
.status.eleven.columns
.modified_summary{ 'data-bulk-form-target': "changedSummary", 'data-translation-key': 'admin.products_v3.table.changed_summary'}
- if defined?(error_counts)
.error_summary
-# X products were saved correctly, but Y products could not be saved correctly. Please review errors and try again
= t('.error_summary.saved', count: error_counts[:saved]) + t('.error_summary.invalid', count: error_counts[:invalid])
.form-buttons.five.columns
= form.submit t('.reset'), type: :reset, class: "medium", 'data-reflex': 'click->products#fetch'
= form.submit t('.save'), class: "medium"
%table.products

View File

@@ -5,6 +5,7 @@ export default class BulkFormController extends Controller {
static targets = ["actions", "changedSummary"];
static values = {
disableSelector: String,
error: Boolean,
};
recordElements = {};
@@ -25,6 +26,8 @@ export default class BulkFormController extends Controller {
this.recordElements[recordId].push(element);
}
}
this.toggleFormChanged();
}
disconnect() {
@@ -45,15 +48,16 @@ export default class BulkFormController extends Controller {
const changedRecordCount = Object.values(this.recordElements).filter((elements) =>
elements.some(this.#isChanged)
).length;
const formChanged = changedRecordCount > 0;
const formChanged = changedRecordCount > 0 || this.errorValue;
// Show actions
this.actionsTarget.classList.toggle("hidden", !formChanged);
this.#disableOtherElements(formChanged); // like filters and sorting
// Display number of records changed
const key = this.changedSummaryTarget && this.changedSummaryTarget.dataset.translationKey;
const key = this.hasChangedSummaryTarget && this.changedSummaryTarget.dataset.translationKey;
if (key) {
// TODO: save processing and only run if changedRecordCount has changed.
this.changedSummaryTarget.textContent = I18n.t(key, { count: changedRecordCount });
}
@@ -76,13 +80,23 @@ export default class BulkFormController extends Controller {
// private
#disableOtherElements(disable) {
if (!this.hasDisableSelectorValue) return;
this.disableElements ||= document.querySelectorAll(this.disableSelectorValue);
if (this.disableElements) {
this.disableElements.forEach((element) => {
element.classList.toggle("disabled-section", disable);
});
}
if (!this.disableElements) return;
this.disableElements.forEach((element) => {
element.classList.toggle("disabled-section", disable);
// Also disable any form elements
let forms = element.tagName == "FORM" ? [element] : element.querySelectorAll("form");
forms &&
forms.forEach((form) =>
Array.from(form.elements).forEach((formElement) => (formElement.disabled = disable))
);
});
}
#isChanged(element) {

View File

@@ -282,6 +282,20 @@ fieldset {
text-align: center;
}
.modified_summary {
font-size: inherit;
}
.error_summary {
font-size: inherit;
@extend .icon-remove-sign;
&:before {
font-family: FontAwesome;
color: $color-error;
}
}
select {
@extend input, [type="text"];
background-color: white;

View File

@@ -837,13 +837,18 @@ en:
zero: ""
one: "%{count} product modified."
other: "%{count} products modified."
error_summary:
saved:
zero: ""
one: "%{count} product was saved correctly, but "
other: "%{count} products were saved correctly, but "
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
bulk_update: # TODO: fix these
success: "Products successfully updated" #TODO: add count
products_have_error:
one: "%{count} product has an error."
other: "%{count} products have an error."
product_import:
title: Product Import
file_not_found: File not found or could not be opened

View File

@@ -11,44 +11,43 @@ describe("BulkFormController", () => {
application.register("bulk-form", bulk_form_controller);
});
// Mock I18n. TODO: moved to a shared helper
beforeAll(() => {
const mockedT = jest.fn();
mockedT.mockImplementation((string, opts) => (string + ', ' + JSON.stringify(opts)));
global.I18n = {
t: mockedT
};
})
// (jest still doesn't have aroundEach https://github.com/jestjs/jest/issues/4543 )
afterAll(() => {
delete global.I18n;
})
beforeEach(() => {
document.body.innerHTML = `
<div id="disable1"></div>
<div id="disable2"></div>
<form data-controller="bulk-form" data-bulk-form-disable-selector-value="#disable1,#disable2">
<div id="actions" data-bulk-form-target="actions" class="hidden"></div>
<div id="changed_summary" data-bulk-form-target="changedSummary" data-translation-key="changed_summary"></div>
<div data-record-id="1">
<input id="input1a" type="text" value="initial1a">
<input id="input1b" type="text" value="initial1b">
</div>
<div data-record-id="2">
<input id="input2" type="text" value="initial2">
</div>
<input type="submit">
</form>
`;
});
describe("Modifying input values", () => {
// Mock I18n. TODO: moved to a shared helper
beforeAll(() => {
const mockedT = jest.fn();
mockedT.mockImplementation((string, opts) => (string + ', ' + JSON.stringify(opts)));
global.I18n = {
t: mockedT
};
})
// (jest still doesn't have aroundEach https://github.com/jestjs/jest/issues/4543 )
afterAll(() => {
delete global.I18n;
})
beforeEach(() => {
document.body.innerHTML = `
<form id="disable1"><input id="disable1_element"></form>
<div id="disable2"><form><input id="disable2_element"></form></div>
<form data-controller="bulk-form" data-bulk-form-disable-selector-value="#disable1,#disable2">
<div id="actions" data-bulk-form-target="actions" class="hidden"></div>
<div id="changed_summary" data-bulk-form-target="changedSummary" data-translation-key="changed_summary"></div>
<div data-record-id="1">
<input id="input1a" type="text" value="initial1a">
<input id="input1b" type="text" value="initial1b">
</div>
<div data-record-id="2">
<input id="input2" type="text" value="initial2">
</div>
<input type="submit">
</form>
`;
const disable1 = document.getElementById("disable1");
const disable1_element = document.getElementById("disable1_element");
const disable2 = document.getElementById("disable2");
const disable2_element = document.getElementById("disable2_element");
const actions = document.getElementById("actions");
const changed_summary = document.getElementById("changed_summary");
const input1a = document.getElementById("input1a");
@@ -115,7 +114,9 @@ describe("BulkFormController", () => {
expect(actions.classList).not.toContain('hidden');
expect(changed_summary.textContent).toBe('changed_summary, {"count":1}');
expect(disable1.classList).toContain('disabled-section');
expect(disable1_element.disabled).toBe(true);
expect(disable2.classList).toContain('disabled-section');
expect(disable2_element.disabled).toBe(true);
// Record 1: Second field changed
input1b.value = 'updated1b';
@@ -148,7 +149,9 @@ describe("BulkFormController", () => {
expect(actions.classList).not.toContain('hidden');
expect(changed_summary.textContent).toBe('changed_summary, {"count":1}');
expect(disable1.classList).toContain('disabled-section');
expect(disable1_element.disabled).toBe(true);
expect(disable2.classList).toContain('disabled-section');
expect(disable2_element.disabled).toBe(true);
// Record 2: Change back to original value
input2.value = 'initial2';
@@ -157,11 +160,50 @@ describe("BulkFormController", () => {
expect(actions.classList).toContain('hidden');
expect(changed_summary.textContent).toBe('changed_summary, {"count":0}');
expect(disable1.classList).not.toContain('disabled-section');
expect(disable1_element.disabled).toBe(false);
expect(disable2.classList).not.toContain('disabled-section');
expect(disable2_element.disabled).toBe(false);
});
});
});
describe("When there are errors", () => {
beforeEach(() => {
document.body.innerHTML = `
<form data-controller="bulk-form" data-bulk-form-error-value="true">
<div id="actions" data-bulk-form-target="actions">
An error occurred.
<input type="submit">
</div>
<div data-record-id="1">
<input id="input1a" type="text" value="initial1a">
</div>
</form>
`;
const actions = document.getElementById("actions");
const changed_summary = document.getElementById("changed_summary");
const input1a = document.getElementById("input1a");
});
it("form actions section remains visible", () => {
// Expect actions to remain visible
expect(actions.classList).not.toContain('hidden');
// Record 1: First field changed
input1a.value = 'updated1a';
input1a.dispatchEvent(new Event("change"));
// Expect actions to remain visible
expect(actions.classList).not.toContain('hidden');
// Change back to original value
input1a.value = 'initial1a';
input1a.dispatchEvent(new Event("change"));
// Expect actions to remain visible
expect(actions.classList).not.toContain('hidden');
});
});
// unable to test disconnect at this stage
// describe("disconnect()", () => {
// it("resets other elements", () => {

View File

@@ -13,7 +13,7 @@ describe ProductsReflex, type: :reflex do
Flipper.enable(:admin_style_v3)
end
describe 'fetch' do
describe '#fetch' do
subject{ build_reflex(method_name: :fetch, **context) }
describe "sorting" do
@@ -41,6 +41,7 @@ describe ProductsReflex, type: :reflex do
sku: "APL-01",
price: 5.25)
}
let!(:product_c) { create(:simple_product, name: "Carrots", sku: "CAR-00") }
let!(:product_b) { create(:simple_product, name: "Bananas", sku: "BAN-00") }
let!(:product_a) { create(:simple_product, name: "Apples", sku: "APL-00") }
@@ -123,17 +124,21 @@ describe ProductsReflex, type: :reflex do
"products" => [
{
"id" => product_a.id.to_s,
"name" => "",
"name" => "Pommes",
},
{
"id" => product_b.id.to_s,
"name" => "",
"name" => "", # Name can't be blank
},
{
"id" => product_c.id.to_s,
"name" => "", # Name can't be blank
},
]
}
reflex = run_reflex(:bulk_update, params:)
expect(reflex.get(:error_msg)).to include "2 products have errors"
expect(reflex.get(:error_counts)).to eq({ saved: 1, invalid: 2 })
# # WTF
# expect{ reflex(:bulk_update, params:) }.to broadcast(

View File

@@ -7,6 +7,7 @@ describe Sets::ProductSet do
let(:product_set) do
described_class.new(collection_attributes: collection_hash)
end
subject{ product_set.save }
context 'when the product does not exist yet' do
let!(:stock_location) { create(:stock_location, backorderable_default: false) }
@@ -33,6 +34,30 @@ describe Sets::ProductSet do
end
context 'when the product does exist' do
let(:product) { create(:simple_product, name: "product name") }
context "with valid name" do
let(:collection_hash) {
{ 0 => { id: product.id, name: "New season product" } }
}
it "returns true and counts results" do
is_expected.to eq true
expect(product_set.saved_count).to eq 1
end
end
context "with invalid name" do
let(:collection_hash) {
{ 0 => { id: product.id, name: "" } } # Product Name can't be blank
}
it "returns false and counts results" do
is_expected.to eq false
expect(product_set.saved_count).to eq 0
end
end
context 'when a different variant_unit is passed' do
let!(:product) do
create(
@@ -57,6 +82,7 @@ describe Sets::ProductSet do
it 'updates the product without error' do
expect(product_set.save).to eq true
expect(product_set.saved_count).to eq 1
expect(product.reload.attributes).to include(
'variant_unit' => 'weight'
@@ -68,7 +94,6 @@ describe Sets::ProductSet do
context "when the product is in an order cycle" do
let(:producer) { create(:enterprise) }
let(:product) { create(:simple_product) }
let(:distributor) { create(:distributor_enterprise) }
let!(:order_cycle) {
@@ -111,36 +136,117 @@ describe Sets::ProductSet do
end
end
describe "updating variants" do
let!(:product) { create(:simple_product) }
let(:collection_hash) { { 0 => { id: product.id } } }
describe "updating a product's variants" do
let(:product) { create(:simple_product) }
let(:variant) { product.variants.first }
let(:product_attributes) { {} }
let(:variant_attributes) { { sku: "var_sku" } }
let(:variants_attributes) { [{ **variant_attributes, id: product.variants.first.id.to_s }] }
let(:collection_hash) {
{
0 => { id: product.id, **product_attributes, variants_attributes: }
}
}
context 'when :variants_attributes are passed' do
let(:variants_attributes) { [{ sku: '123', id: product.variants.first.id.to_s }] }
before { collection_hash[0][:variants_attributes] = variants_attributes }
it 'updates the attributes of the variant' do
it "updates the variant" do
expect {
product_set.save
variant.reload
}.to change { variant.sku }.to("var_sku")
expect(product.reload.variants.first[:sku]).to eq variants_attributes.first[:sku]
pending
expect(product_set.saved_count).to eq 1
end
shared_examples "nothing saved" do
it "doesn't update product" do
expect {
product_set.save
product.reload
}.to_not change { product.sku }
expect(product_set.saved_count).to be_zero
expect(product_set.invalid.count).to be_positive
end
context 'and when product attributes are also passed' do
it 'updates product and variant attributes' do
collection_hash[0][:sku] = "test_sku"
it "doesn't update variant" do
expect {
product_set.save
variant.reload
}.to_not change { variant.sku }
end
expect {
product_set.save
product.reload
}.to change { product.sku }.to("test_sku")
.and change { product.variants.first.sku }.to("123")
it 'assigns the in-memory attributes of the variant' do
pending
expect {
product_set.save
}.to change { variant.sku }.to("123")
end
end
context "variant has error" do
let(:variant_attributes) { { sku: "var_sku", display_name: "A" * 256 } } # maximum length
include_examples "nothing saved"
end
context "when products attributes are also updated" do
let(:product_attributes) {
{ sku: "prod_sku" }
}
it "updates product and variant" do
expect {
product_set.save
product.reload
}.to change { product.sku }.to("prod_sku")
.and change { product.variants.first.sku }.to("var_sku")
expect(product_set.saved_count).to eq 1
end
xcontext "variant has error" do
let(:variant_attributes) { { sku: "var_sku", display_name: "A" * 256 } } # maximum length
include_examples "nothing saved"
end
context "product has error" do
before { collection_hash[0][:name] = "" } # product.name can't be blank
include_examples "nothing saved"
end
end
context "when multiple variants are updated" do
let(:variant2) { create(:variant, product:) }
let(:variants_attributes) {
[
{ **variant_attributes, id: product.variants.first.id.to_s },
{ sku: "var_sku2", id: variant2.id.to_s },
]
}
it "updates each variant" do
expect {
product_set.save
variant2.reload
}.to change { product.variants.first.sku }.to("var_sku")
.and change { variant2.sku }.to("var_sku2")
end
xcontext "variant has error" do
let(:variant_attributes) { { sku: "var_sku", display_name: "A" * 256 } } # maximum length
include_examples "nothing saved" do
after { expect(variant2.reload.sku).to_not eq "var_sku2" }
end
end
end
end
context 'when there are multiple products' do
let(:product_c) { create(:simple_product, name: "Carrots") }
let!(:product_b) { create(:simple_product, name: "Bananas") }
let!(:product_a) { create(:simple_product, name: "Apples") }
@@ -154,39 +260,37 @@ describe Sets::ProductSet do
id: product_b.id,
name: "Bananes",
},
2 => {
id: product_c.id,
name: "Carrots",
},
}
end
it 'updates the products' do
product_set.save
expect(product_set.save).to eq true
expect(product_set.saved_count).to eq 2 # only two were changed
expect(product_a.reload.name).to eq "Pommes"
expect(product_b.reload.name).to eq "Bananes"
expect(product_c.reload.name).to eq "Carrots" # no change
end
it 'retains the order of products' do
product_set.save
# even though the first product is now alphabetically last
expect(product_set.collection[0]).to eq product_a.reload
expect(product_set.collection[1]).to eq product_b.reload
expect(product_set.collection[2]).to eq product_c.reload
end
context 'first product has an error' do
let(:collection_hash) do
{
0 => {
id: product_a.id,
name: "", # Product Name can't be blank
},
1 => {
id: product_b.id,
name: "Bananes",
},
}
end
before { collection_hash[0][:name] = "" } # product.name can't be blank
it 'continues to update subsequent products' do
product_set.save
expect(product_set.saved_count).to eq 1
# Errors are logged on the model
first_item = product_set.collection[0]

View File

@@ -307,7 +307,7 @@ describe '
click_button 'Save Changes', match: :first
expect(page.find("#status-message"))
.to have_content "Unit value can't be blank Unit value is not a number"
.to have_content "Variant unit value can't be blank Variant unit value is not a number"
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 "Unit value can't be blank Unit value is not a number"
.to have_content "Variant unit value can't be blank Variant unit value is not a number"
end
end
end

View File

@@ -203,7 +203,6 @@ describe 'As an admin, I can see the new product page' do
price: 5.25)
}
let!(:product_a) { create(:simple_product, name: "Apples", sku: "APL-00") }
before do
visit admin_products_url
end
@@ -270,37 +269,71 @@ describe 'As an admin, I can see the new product page' do
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
context "with invalid data" do
let!(:product_b) { create(:simple_product, name: "Bananas") }
before 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
fill_in "Price", with: "10.25"
end
end
expect {
click_button "Save changes"
product_a.reload
}.to_not change { product_a.name }
it "shows errors for both product and variant fields" do
# Also update another product with valid data
within row_containing_name("Bananas") do
fill_in "Name", with: "Bananes"
end
# (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"
expect {
click_button "Save changes"
product_a.reload
}.to_not change { product_a.name }
# pending("unchanged rows are being saved") # TODO: don't report unchanged rows
# expect(page).to_not have_content("rows were saved correctly")
# Both the product and variant couldn't be saved.
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: ""
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 "bug #11748"
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" # other updated value is retained
end
end
expect(page).to have_content "Please review the errors and try again"
it "saves changes after fixing errors" do
within row_containing_name("Apples") do
fill_in "Name", with: "Pommes"
fill_in "SKU", with: "POM-00"
end
expect {
click_button "Save changes"
product_a.reload
variant_a1.reload
}.to change { product_a.name }.to("Pommes")
.and change{ product_a.sku }.to("POM-00")
pending
expect(page).to have_content "Changes saved"
end
end
end