diff --git a/app/assets/javascripts/admin/bulk_product_update.js.coffee b/app/assets/javascripts/admin/bulk_product_update.js.coffee index 9f0c5a7a81..910fb39f8b 100644 --- a/app/assets/javascripts/admin/bulk_product_update.js.coffee +++ b/app/assets/javascripts/admin/bulk_product_update.js.coffee @@ -248,7 +248,6 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", ($scope, $timeout else product.variant_unit = product.variant_unit_scale = null - $scope.packVariant product, product.master if product.master if product.variants for id, variant of product.variants @@ -299,7 +298,6 @@ filterSubmitProducts = (productsToFilter) -> if product.hasOwnProperty("id") filteredProduct = {id: product.id} filteredVariants = [] - filteredMaster = null hasUpdatableProperty = false if product.hasOwnProperty("variants") @@ -309,16 +307,6 @@ filterSubmitProducts = (productsToFilter) -> variantHasUpdatableProperty = result.hasUpdatableProperty filteredVariants.push filteredVariant if variantHasUpdatableProperty - if product.master?.hasOwnProperty("unit_value") - filteredMaster ?= { id: product.master.id } - filteredMaster.unit_value = product.master.unit_value - if product.master?.hasOwnProperty("unit_description") - filteredMaster ?= { id: product.master.id } - filteredMaster.unit_description = product.master.unit_description - if product.master?.hasOwnProperty("display_as") - filteredMaster ?= { id: product.master.id } - filteredMaster.display_as = product.master.display_as - if product.hasOwnProperty("sku") filteredProduct.sku = product.sku hasUpdatableProperty = true @@ -350,9 +338,6 @@ filterSubmitProducts = (productsToFilter) -> if product.hasOwnProperty("inherits_properties") filteredProduct.inherits_properties = product.inherits_properties hasUpdatableProperty = true - if filteredMaster? - filteredProduct.master_attributes = filteredMaster - hasUpdatableProperty = true if filteredVariants.length > 0 # Note that the name of the property changes to enable mass assignment of variants attributes with rails filteredProduct.variants_attributes = filteredVariants hasUpdatableProperty = true diff --git a/app/helpers/bulk_form_builder.rb b/app/helpers/bulk_form_builder.rb new file mode 100644 index 0000000000..dcb45ae3bc --- /dev/null +++ b/app/helpers/bulk_form_builder.rb @@ -0,0 +1,13 @@ +# frozen_string_literal: true + +class BulkFormBuilder < ActionView::Helpers::FormBuilder + def text_field(field, **opts) + # Mark field if it is changed (unsaved) + changed_method = "#{field}_changed?" + if object.respond_to?(changed_method) && object.public_send(changed_method) + opts[:class] = "#{opts[:class]} changed".strip + end + + super(field, **opts) + end +end diff --git a/app/models/spree/price.rb b/app/models/spree/price.rb index 68d5caf306..710e4ef5c3 100644 --- a/app/models/spree/price.rb +++ b/app/models/spree/price.rb @@ -24,6 +24,10 @@ module Spree amount end + def price_changed? + amount_changed? + end + def price=(price) self[:amount] = parse_price(price) end diff --git a/app/models/spree/variant.rb b/app/models/spree/variant.rb index 528790313b..ea05767a63 100644 --- a/app/models/spree/variant.rb +++ b/app/models/spree/variant.rb @@ -50,7 +50,8 @@ module Spree has_many :prices, class_name: 'Spree::Price', dependent: :destroy - delegate :display_price, :display_amount, :price, :price=, :currency, :currency=, + delegate :display_price, :display_amount, :price, :price_changed?, :price=, + :currency, :currency=, to: :find_or_build_default_price has_many :exchange_variants diff --git a/app/services/sets/model_set.rb b/app/services/sets/model_set.rb index 7347e2d9c9..1db02abf92 100644 --- a/app/services/sets/model_set.rb +++ b/app/services/sets/model_set.rb @@ -79,5 +79,11 @@ module Sets def persisted? false end + + def find_model(collection, model_id) + collection.find do |model| + model.id.to_s == model_id.to_s && model.persisted? + end + end end end diff --git a/app/services/sets/product_set.rb b/app/services/sets/product_set.rb index 7eb73e3bee..23be044239 100644 --- a/app/services/sets/product_set.rb +++ b/app/services/sets/product_set.rb @@ -59,12 +59,11 @@ module Sets ExchangeVariantDeleter.new.delete(product) if product.saved_change_to_supplier_id? - update_product_variants(product, attributes) && - update_product_master(product, attributes) + update_product_variants(product, attributes) end def update_product_only_attributes(product, attributes) - variant_related_attrs = [:id, :variants_attributes, :master_attributes] + variant_related_attrs = [:id, :variants_attributes] product_related_attrs = attributes.except(*variant_related_attrs) return true if product_related_attrs.blank? @@ -94,12 +93,6 @@ module Sets update_variants_attributes(product, attributes[:variants_attributes]) end - def update_product_master(product, attributes) - return true unless attributes[:master_attributes] - - create_or_update_variant(product, attributes[:master_attributes]) - end - def update_variants_attributes(product, variants_attributes) variants_attributes.each do |attributes| create_or_update_variant(product, attributes) @@ -119,6 +112,7 @@ module Sets def create_variant(product, variant_attributes) return if variant_attributes.blank? + # 'You need to save the variant to create a stock item before you can set stock levels.' on_hand = variant_attributes.delete(:on_hand) on_demand = variant_attributes.delete(:on_demand) @@ -147,11 +141,5 @@ module Sets report.add_metadata(:variant_error, variant.errors.first) unless variant.valid? end end - - def find_model(collection, model_id) - collection.find do |model| - model.id.to_s == model_id.to_s && model.persisted? - end - end end end diff --git a/app/views/admin/products_v3/_table.html.haml b/app/views/admin/products_v3/_table.html.haml index 7417f676cd..33806fc195 100644 --- a/app/views/admin/products_v3/_table.html.haml +++ b/app/views/admin/products_v3/_table.html.haml @@ -1,10 +1,11 @@ = 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" } .container .status.ten.columns - .modified_summary{ 'data-bulk-form-target': "modifiedSummary", 'data-translation-key': 'admin.products_v3.table.modified_summary'} + .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 diff --git a/app/webpacker/controllers/bulk_form_controller.js b/app/webpacker/controllers/bulk_form_controller.js index 526aa1d5b5..842a1aaa89 100644 --- a/app/webpacker/controllers/bulk_form_controller.js +++ b/app/webpacker/controllers/bulk_form_controller.js @@ -1,8 +1,8 @@ import { Controller } from "stimulus"; -// Manages "modified" state for a form with multiple records +// Manages "changed" state for a form with multiple records export default class BulkFormController extends Controller { - static targets = ["actions", "modifiedSummary"]; + static targets = ["actions", "changedSummary"]; static values = { disableSelector: String, }; @@ -12,10 +12,10 @@ export default class BulkFormController extends Controller { this.form = this.element; // Start listening for any changes within the form - // this.element.addEventListener('change', this.toggleModified.bind(this)); // dunno why this doesn't work + // this.element.addEventListener('change', this.toggleChanged.bind(this)); // dunno why this doesn't work for (const element of this.form.elements) { - element.addEventListener("keyup", this.toggleModified.bind(this)); // instant response - element.addEventListener("change", this.toggleModified.bind(this)); // just in case (eg right-click paste) + element.addEventListener("keyup", this.toggleChanged.bind(this)); // instant response + element.addEventListener("change", this.toggleChanged.bind(this)); // just in case (eg right-click paste) // 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. @@ -33,32 +33,32 @@ export default class BulkFormController extends Controller { window.removeEventListener("beforeunload", this.preventLeavingBulkForm); } - toggleModified(e) { + toggleChanged(e) { const element = e.target; - element.classList.toggle("modified", this.#isModified(element)); + element.classList.toggle("changed", this.#isChanged(element)); - this.toggleFormModified(); + this.toggleFormChanged(); } - toggleFormModified() { - // For each record, check if any fields are modified - const modifiedRecordCount = Object.values(this.recordElements).filter((elements) => - elements.some(this.#isModified) + toggleFormChanged() { + // For each record, check if any fields are changed + const changedRecordCount = Object.values(this.recordElements).filter((elements) => + elements.some(this.#isChanged) ).length; - const formModified = modifiedRecordCount > 0; + const formChanged = changedRecordCount > 0; // Show actions - this.actionsTarget.classList.toggle("hidden", !formModified); - this.#disableOtherElements(formModified); // like filters and sorting + this.actionsTarget.classList.toggle("hidden", !formChanged); + this.#disableOtherElements(formChanged); // like filters and sorting - // Display number of records modified - const key = this.modifiedSummaryTarget && this.modifiedSummaryTarget.dataset.translationKey; + // Display number of records changed + const key = this.changedSummaryTarget && this.changedSummaryTarget.dataset.translationKey; if (key) { - this.modifiedSummaryTarget.textContent = I18n.t(key, { count: modifiedRecordCount }); + this.changedSummaryTarget.textContent = I18n.t(key, { count: changedRecordCount }); } // Prevent accidental data loss - if (formModified) { + if (formChanged) { window.addEventListener("beforeunload", this.preventLeavingBulkForm); } else { window.removeEventListener("beforeunload", this.preventLeavingBulkForm); @@ -85,7 +85,7 @@ export default class BulkFormController extends Controller { } } - #isModified(element) { + #isChanged(element) { return element.value != element.defaultValue; } } diff --git a/app/webpacker/css/admin/products_v3.scss b/app/webpacker/css/admin/products_v3.scss index 4ea63f3a28..2764f4b954 100644 --- a/app/webpacker/css/admin/products_v3.scss +++ b/app/webpacker/css/admin/products_v3.scss @@ -68,7 +68,7 @@ .content { // Plain content fields need help to align with text in inputs (due to vertical-align) - margin: $vpadding-txt+1px 0; + margin: $vpadding-txt + 1px 0; @extend .line-clamp-1; } @@ -126,8 +126,8 @@ border-color: $color-txt-hover-brd; } - &.modified { - border-color: $color-txt-modified-brd; + &.changed { + border-color: $color-txt-changed-brd; } } diff --git a/app/webpacker/css/admin_v3/globals/variables.scss b/app/webpacker/css/admin_v3/globals/variables.scss index aa45159e59..73a32c877b 100644 --- a/app/webpacker/css/admin_v3/globals/variables.scss +++ b/app/webpacker/css/admin_v3/globals/variables.scss @@ -74,7 +74,7 @@ $color-sel-hover-bg: $lighter-grey !default; $color-txt-brd: $color-border !default; $color-txt-text: $near-black !default; $color-txt-hover-brd: $teal !default; -$color-txt-modified-brd: $bright-orange !default; +$color-txt-changed-brd: $bright-orange !default; $vpadding-txt: 5px; $hpadding-txt: 8px; diff --git a/app/webpacker/css/admin_v3/shared/forms.scss b/app/webpacker/css/admin_v3/shared/forms.scss index 6b9cb9cf8e..96cef47615 100644 --- a/app/webpacker/css/admin_v3/shared/forms.scss +++ b/app/webpacker/css/admin_v3/shared/forms.scss @@ -31,8 +31,8 @@ fieldset { opacity: 0.7; } - &.modified { - border-color: $color-txt-modified-brd; + &.changed { + border-color: $color-txt-changed-brd; } } diff --git a/config/locales/en.yml b/config/locales/en.yml index 2e9842a73f..68cd6ef091 100644 --- a/config/locales/en.yml +++ b/config/locales/en.yml @@ -833,7 +833,7 @@ en: import_products: Import multiple products no_products_found_for_search: No products found for your search criteria table: - modified_summary: + changed_summary: zero: "" one: "%{count} product modified." other: "%{count} products modified." diff --git a/spec/helpers/bulk_form_builder_spec.rb b/spec/helpers/bulk_form_builder_spec.rb new file mode 100644 index 0000000000..00ab150c2c --- /dev/null +++ b/spec/helpers/bulk_form_builder_spec.rb @@ -0,0 +1,26 @@ +# frozen_string_literal: true + +require 'spec_helper' + +class TestHelper < ActionView::Base; end + +describe BulkFormBuilder do + describe '#text_field' do + let(:product) { create(:product) } + let(:form) { BulkFormBuilder.new(:product, product, self, {}) } + + it { expect(form.text_field(:name)).to_not include "changed" } + + context "attribute has been changed" do + before { product.assign_attributes name: "updated name" } + + it { expect(form.text_field(:name)).to include "changed" } + + context "and saved" do + before { product.save } + + it { expect(form.text_field(:name)).to_not include "changed" } + end + end + end +end diff --git a/spec/javascripts/stimulus/bulk_form_controller_test.js b/spec/javascripts/stimulus/bulk_form_controller_test.js index cc31531950..db0f990eef 100644 --- a/spec/javascripts/stimulus/bulk_form_controller_test.js +++ b/spec/javascripts/stimulus/bulk_form_controller_test.js @@ -32,7 +32,7 @@ describe("BulkFormController", () => {
-
+
@@ -50,7 +50,7 @@ describe("BulkFormController", () => { const disable1 = document.getElementById("disable1"); const disable2 = document.getElementById("disable2"); const actions = document.getElementById("actions"); - const modified_summary = document.getElementById("modified_summary"); + const changed_summary = document.getElementById("changed_summary"); const input1a = document.getElementById("input1a"); const input1b = document.getElementById("input1b"); const input2 = document.getElementById("input2"); @@ -60,30 +60,30 @@ describe("BulkFormController", () => { it("onChange", () => { input1a.value = 'updated1a'; input1a.dispatchEvent(new Event("change")); - // Expect only first field to show modified - expect(input1a.classList).toContain('modified'); - expect(input1b.classList).not.toContain('modified'); - expect(input2.classList).not.toContain('modified'); + // Expect only first field to show changed + expect(input1a.classList).toContain('changed'); + expect(input1b.classList).not.toContain('changed'); + expect(input2.classList).not.toContain('changed'); // Change back to original value input1a.value = 'initial1a'; input1a.dispatchEvent(new Event("change")); - expect(input1a.classList).not.toContain('modified'); + expect(input1a.classList).not.toContain('changed'); }); it("onKeyup", () => { input1a.value = 'u1a'; input1a.dispatchEvent(new Event("keyup")); - // Expect only first field to show modified - expect(input1a.classList).toContain('modified'); - expect(input1b.classList).not.toContain('modified'); - expect(input2.classList).not.toContain('modified'); + // Expect only first field to show changed + expect(input1a.classList).toContain('changed'); + expect(input1b.classList).not.toContain('changed'); + expect(input2.classList).not.toContain('changed'); // Change back to original value input1a.value = 'initial1a'; input1a.dispatchEvent(new Event("keyup")); - expect(input1a.classList).not.toContain('modified'); + expect(input1a.classList).not.toContain('changed'); }); it("multiple fields", () => { @@ -91,29 +91,29 @@ describe("BulkFormController", () => { input1a.dispatchEvent(new Event("change")); input2.value = 'updated2'; input2.dispatchEvent(new Event("change")); - // Expect only first field to show modified - expect(input1a.classList).toContain('modified'); - expect(input1b.classList).not.toContain('modified'); - expect(input2.classList).toContain('modified'); + // Expect only first field to show changed + expect(input1a.classList).toContain('changed'); + expect(input1b.classList).not.toContain('changed'); + expect(input2.classList).toContain('changed'); // Change only one back to original value input1a.value = 'initial1a'; input1a.dispatchEvent(new Event("change")); - expect(input1a.classList).not.toContain('modified'); - expect(input1b.classList).not.toContain('modified'); - expect(input2.classList).toContain('modified'); + expect(input1a.classList).not.toContain('changed'); + expect(input1b.classList).not.toContain('changed'); + expect(input2.classList).toContain('changed'); }); }) describe("activating sections, and showing a summary", () => { // This scenario should probably be broken up into smaller units. - it("counts modified records ", () => { + it("counts changed records ", () => { // Record 1: First field changed input1a.value = 'updated1a'; input1a.dispatchEvent(new Event("change")); - // Actions and modified summary are shown, with other sections disabled + // Actions and changed summary are shown, with other sections disabled expect(actions.classList).not.toContain('hidden'); - expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); + expect(changed_summary.textContent).toBe('changed_summary, {"count":1}'); expect(disable1.classList).toContain('disabled-section'); expect(disable2.classList).toContain('disabled-section'); @@ -122,31 +122,31 @@ describe("BulkFormController", () => { input1b.dispatchEvent(new Event("change")); // Expect to show same summary translation expect(actions.classList).not.toContain('hidden'); - expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); + expect(changed_summary.textContent).toBe('changed_summary, {"count":1}'); // Record 2: has been changed input2.value = 'updated2'; input2.dispatchEvent(new Event("change")); // Expect summary to count both records expect(actions.classList).not.toContain('hidden'); - expect(modified_summary.textContent).toBe('modified_summary, {"count":2}'); + expect(changed_summary.textContent).toBe('changed_summary, {"count":2}'); // Record 1: Change first field back to original value input1a.value = 'initial1a'; input1a.dispatchEvent(new Event("change")); - // Both records are still modified. - expect(input1a.classList).not.toContain('modified'); - expect(input1b.classList).toContain('modified'); - expect(input2.classList).toContain('modified'); + // Both records are still changed. + expect(input1a.classList).not.toContain('changed'); + expect(input1b.classList).toContain('changed'); + expect(input2.classList).toContain('changed'); expect(actions.classList).not.toContain('hidden'); - expect(modified_summary.textContent).toBe('modified_summary, {"count":2}'); + expect(changed_summary.textContent).toBe('changed_summary, {"count":2}'); // Record 1: Change second field back to original value input1b.value = 'initial1b'; input1b.dispatchEvent(new Event("change")); - // Both fields for record 1 show unmodified, but second record is still modified + // Both fields for record 1 show unchanged, but second record is still changed expect(actions.classList).not.toContain('hidden'); - expect(modified_summary.textContent).toBe('modified_summary, {"count":1}'); + expect(changed_summary.textContent).toBe('changed_summary, {"count":1}'); expect(disable1.classList).toContain('disabled-section'); expect(disable2.classList).toContain('disabled-section'); @@ -155,7 +155,7 @@ describe("BulkFormController", () => { input2.dispatchEvent(new Event("change")); // Actions are hidden and other sections are now re-enabled expect(actions.classList).toContain('hidden'); - expect(modified_summary.textContent).toBe('modified_summary, {"count":0}'); + expect(changed_summary.textContent).toBe('changed_summary, {"count":0}'); expect(disable1.classList).not.toContain('disabled-section'); expect(disable2.classList).not.toContain('disabled-section'); }); @@ -170,7 +170,7 @@ describe("BulkFormController", () => { // const controller = document.querySelector('[data-controller="bulk-form"]'); // const form = document.querySelector('[data-controller="bulk-form"]'); - // // Form is modified and other sections are disabled + // // Form is changed and other sections are disabled // input1a.value = 'updated1a'; // input1a.dispatchEvent(new Event("change")); // expect(disable1.classList).toContain('disabled-section'); diff --git a/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee b/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee index 28ff35e1e8..aa56ccd595 100644 --- a/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee +++ b/spec/javascripts/unit/admin/bulk_product_update_spec.js.coffee @@ -195,10 +195,6 @@ describe "filtering products for submission to database", -> producer_id: 5 group_buy: null group_buy_unit_size: null - master: - id: 2 - unit_value: 250 - unit_description: "foo" variants: [ id: 1 on_hand: 2 @@ -221,10 +217,6 @@ describe "filtering products for submission to database", -> variant_unit: 'volume' variant_unit_scale: 1 variant_unit_name: 'loaf' - master_attributes: - id: 2 - unit_value: 250 - unit_description: "foo" variants_attributes: [ id: 1 on_hand: 2 @@ -558,17 +550,6 @@ describe "AdminProductEditCtrl", -> variant_unit_scale: null variant_unit_with_scale: 'items' - it "packs the master variant", -> - spyOn $scope, "packVariant" - testVariant = {id: 1} - testProduct = - id: 1 - master: testVariant - - $scope.packProduct(testProduct) - - expect($scope.packVariant).toHaveBeenCalledWith(testProduct, testVariant) - it "packs each variant", -> spyOn $scope, "packVariant" testVariant = {id: 1} @@ -986,10 +967,6 @@ describe "AdminProductEditCtrl", -> producer_id: 5 group_buy: null group_buy_unit_size: null - master: - id: 2 - unit_value: 250 - unit_description: "foo" describe 'product has variant', -> it 'should load the edit product variant page', -> diff --git a/spec/services/sets/product_set_spec.rb b/spec/services/sets/product_set_spec.rb index 72de1ab124..18964d7f21 100644 --- a/spec/services/sets/product_set_spec.rb +++ b/spec/services/sets/product_set_spec.rb @@ -4,210 +4,179 @@ require 'spec_helper' describe Sets::ProductSet do describe '#save' do - context 'when passing :collection_attributes' do - let(:product_set) do - described_class.new(collection_attributes: collection_hash) + let(:product_set) do + described_class.new(collection_attributes: collection_hash) + end + + context 'when the product does not exist yet' do + let!(:stock_location) { create(:stock_location, backorderable_default: false) } + let(:collection_hash) do + { + 0 => { + name: 'a product', + price: 2.0, + supplier_id: create(:enterprise).id, + primary_taxon_id: create(:taxon).id, + unit_description: 'description', + variant_unit: 'items', + variant_unit_name: 'bunches', + shipping_category_id: create(:shipping_category).id + } + } end - context 'when the product does not exist yet' do - let!(:stock_location) { create(:stock_location, backorderable_default: false) } + it 'does not create a new product' do + product_set.save + + expect(Spree::Product.last).to be nil + end + end + + context 'when the product does exist' do + context 'when a different variant_unit is passed' do + let!(:product) do + create( + :simple_product, + variant_unit: 'items', + variant_unit_scale: nil, + variant_unit_name: 'bunches', + unit_value: nil, + unit_description: 'some description' + ) + end + let(:collection_hash) do { 0 => { - name: 'a product', - price: 2.0, - supplier_id: create(:enterprise).id, - primary_taxon_id: create(:taxon).id, - unit_description: 'description', - variant_unit: 'items', - variant_unit_name: 'bunches', - shipping_category_id: create(:shipping_category).id + id: product.id, + variant_unit: 'weight', + variant_unit_scale: 1 } } end - it 'does not create a new product' do + it 'updates the product without error' do + expect(product_set.save).to eq true + + expect(product.reload.attributes).to include( + 'variant_unit' => 'weight' + ) + + expect(product_set.errors).to be_empty + end + end + + 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) { + create(:simple_order_cycle, variants: [product.variants.first], + coordinator: distributor, + distributors: [distributor]) + } + + context 'and only the name changes' do + let(:collection_hash) do + { 0 => { id: product.id, name: "New season product" } } + end + + it 'updates the product and keeps it in order cycles' do + expect { + product_set.save + product.reload + }.to change { product.name }.to("New season product"). + and change { order_cycle.distributed_variants.count }.by(0) + + expect(order_cycle.distributed_variants).to include product.variants.first + end + end + + context 'and a different supplier is passed' do + let(:collection_hash) do + { 0 => { id: product.id, supplier_id: producer.id } } + end + + it 'updates the product and removes the product from order cycles' do + expect { + product_set.save + product.reload + }.to change { product.supplier }.to(producer). + and change { order_cycle.distributed_variants.count }.by(-1) + + expect(order_cycle.distributed_variants).to_not include product.variants.first + end + end + end + end + + describe "updating variants" do + let!(:product) { create(:simple_product) } + let(:collection_hash) { { 0 => { id: product.id } } } + + 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 product_set.save - expect(Spree::Product.last).to be nil - end - end - - context 'when the product does exist' do - context 'when a different variant_unit is passed' do - let!(:product) do - create( - :simple_product, - variant_unit: 'items', - variant_unit_scale: nil, - variant_unit_name: 'bunches', - unit_value: nil, - unit_description: 'some description' - ) - end - - let(:collection_hash) do - { - 0 => { - id: product.id, - variant_unit: 'weight', - variant_unit_scale: 1 - } - } - end - - it 'updates the product' do - product_set.save - - expect(product.reload.attributes).to include( - 'variant_unit' => 'weight' - ) - end - - it 'does not add an error' do - product_set.save - expect(product_set.errors) - .to be_empty - end - - it 'returns true' do - expect(product_set.save).to eq(true) - end + expect(product.reload.variants.first[:sku]).to eq variants_attributes.first[:sku] end - context "when the product is in an order cycle" do - let(:producer) { create(:enterprise) } - let(:product) { create(:simple_product) } + context 'and when product attributes are also passed' do + it 'updates product and variant attributes' do + collection_hash[0][:sku] = "test_sku" - let(:distributor) { create(:distributor_enterprise) } - let!(:order_cycle) { - create(:simple_order_cycle, variants: [product.variants.first], - coordinator: distributor, - distributors: [distributor]) - } - - context 'and only the name changes' do - let(:collection_hash) do - { 0 => { id: product.id, name: "New season product" } } - end - - it 'updates the product and keeps it in order cycles' do - expect { - product_set.save - product.reload - }.to change { product.name }.to("New season product"). - and change { order_cycle.distributed_variants.count }.by(0) - - expect(order_cycle.distributed_variants).to include product.variants.first - end - end - - context 'and a different supplier is passed' do - let(:collection_hash) do - { 0 => { id: product.id, supplier_id: producer.id } } - end - - it 'updates the product and removes the product from order cycles' do - expect { - product_set.save - product.reload - }.to change { product.supplier }.to(producer). - and change { order_cycle.distributed_variants.count }.by(-1) - - expect(order_cycle.distributed_variants).to_not include product.variants.first - end - end - end - - context 'when attributes of the variants are passed' do - let!(:product) { create(:simple_product) } - let(:collection_hash) { { 0 => { id: product.id } } } - - 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 + expect { product_set.save - - expect(product.reload.variants.first[:sku]).to eq variants_attributes.first[:sku] - end - - context 'and when product attributes are also passed' do - it 'updates product and variant attributes' do - collection_hash[0][:sku] = "test_sku" - - product_set.save - - expect(product.reload.variants.first[:sku]).to eq variants_attributes.first[:sku] - expect(product.reload.attributes).to include( - 'sku' => "test_sku" - ) - end - end - end - - context 'when :master_attributes is passed' do - let(:master_attributes) { { sku: '123' } } - - before do - collection_hash[0][:master_attributes] = master_attributes - end - - context 'and the variant does exist' do - let!(:variant) { create(:variant, product:) } - - before { master_attributes[:id] = variant.id } - - it 'updates the attributes of the master variant' do - product_set.save - expect(variant.reload.sku).to eq('123') - end - end - - context 'and the variant does not exist' do - context 'and attributes provided are valid' do - let(:master_attributes) do - attributes_for(:variant).merge(sku: '123') - end - - it 'creates it with the specified attributes' do - number_of_variants = Spree::Variant.all.size - product_set.save - expect(Spree::Variant.last.sku).to eq('123') - expect(Spree::Variant.all.size).to eq number_of_variants + 1 - end - end - - context 'and attributes provided are not valid' do - let(:master_attributes) do - # unit_value nil makes the variant invalid - # on_hand with a value would make on_hand be updated and fail with exception - attributes_for(:variant).merge(unit_value: nil, on_hand: 1, sku: '321') - end - - it 'does not create variant and notifies bugsnag still raising the exception' do - number_of_variants = Spree::Variant.all.size - expect(product_set.save).to eq(false) - expect(Spree::Variant.all.size).to eq number_of_variants - expect(Spree::Variant.last.sku).not_to eq('321') - end - end - end + product.reload + }.to change { product.sku }.to("test_sku") + .and change { product.variants.first.sku }.to("123") end end end + end - context 'when there are multiple products' do - let!(:product_b) { create(:simple_product, name: "Bananas") } - let!(:product_a) { create(:simple_product, name: "Apples") } + context 'when there are multiple products' do + let!(:product_b) { create(:simple_product, name: "Bananas") } + let!(:product_a) { create(:simple_product, name: "Apples") } + let(:collection_hash) do + { + 0 => { + id: product_a.id, + name: "Pommes", + }, + 1 => { + id: product_b.id, + name: "Bananes", + }, + } + end + + it 'updates the products' do + product_set.save + + expect(product_a.reload.name).to eq "Pommes" + expect(product_b.reload.name).to eq "Bananes" + end + + it 'retains the order of products' do + product_set.save + + expect(product_set.collection[0]).to eq product_a.reload + expect(product_set.collection[1]).to eq product_b.reload + end + + context 'first product has an error' do let(:collection_hash) do { 0 => { id: product_a.id, - name: "Pommes", + name: "", # Product Name can't be blank }, 1 => { id: product_b.id, @@ -216,46 +185,17 @@ describe Sets::ProductSet do } end - it 'updates the products' do + it 'continues to update subsequent products' do product_set.save - expect(product_a.reload.name).to eq "Pommes" + # Errors are logged on the model + first_item = product_set.collection[0] + expect(first_item.errors.full_messages.to_sentence).to eq "Product Name can't be blank" + expect(first_item.name).to eq "" + + # Subsequent product was updated expect(product_b.reload.name).to eq "Bananes" end - - it 'retains the order of products' do - product_set.save - - expect(product_set.collection[0]).to eq product_a.reload - expect(product_set.collection[1]).to eq product_b.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 - - it 'continues to update subsequent products' do - product_set.save - - # Errors are logged on the model - first_item = product_set.collection[0] - expect(first_item.errors.full_messages.to_sentence).to eq "Product Name can't be blank" - expect(first_item.name).to eq "" - - # Subsequent product was updated - expect(product_b.reload.name).to eq "Bananes" - end - end end end end diff --git a/spec/system/admin/products_v3/products_spec.rb b/spec/system/admin/products_v3/products_spec.rb index c6ddc8e7f3..8ea6f57c48 100644 --- a/spec/system/admin/products_v3/products_spec.rb +++ b/spec/system/admin/products_v3/products_spec.rb @@ -7,11 +7,6 @@ describe 'As an admin, I can see the new product page' do include AuthenticationHelper include FileHelper - # create lot of products - 70.times do |i| - let!("product_#{i}".to_sym) { create(:simple_product, name: "product #{i}") } - end - before do # activate feature toggle admin_style_v3 to use new admin interface Flipper.enable(:admin_style_v3) @@ -43,24 +38,26 @@ describe 'As an admin, I can see the new product page' do end describe "pagination" do - before do - visit admin_products_url - end - it "has a pagination, has 15 products per page by default and can change the page" do + create_products 16 + visit admin_products_url + expect(page).to have_selector ".pagination" expect_products_count_to_be 15 within ".pagination" do click_link "2" end - expect(page).to have_content "Showing 16 to 30" + expect(page).to have_content "Showing 16 to 16" # todo: remove unnecessary duplication expect_page_to_be 2 expect_per_page_to_be 15 - expect_products_count_to_be 15 + expect_products_count_to_be 1 end it "can change the number of products per page" do + create_products 51 + visit admin_products_url + select "50", from: "per_page" expect(page).to have_content "Showing 1 to 50" @@ -75,11 +72,10 @@ describe 'As an admin, I can see the new product page' do # create a product with a name that can be searched let!(:product_by_name) { create(:simple_product, name: "searchable product") } - before do - visit admin_products_url - end - it "can search for a product" do + create_products 1 + visit admin_products_url + search_for "searchable product" expect(page).to have_field "search_term", with: "searchable product" @@ -88,20 +84,26 @@ describe 'As an admin, I can see the new product page' do end it "reset the page when searching" do + create_products 15 + visit admin_products_url + within ".pagination" do click_link "2" end - expect(page).to have_content "Showing 16 to 30" + expect(page).to have_content "Showing 16 to 16" expect_page_to_be 2 expect_per_page_to_be 15 - expect_products_count_to_be 15 + expect_products_count_to_be 1 search_for "searchable product" # expect(page).to have_content "1 product found for your search criteria." expect_products_count_to_be 1 end it "can clear filters" do + create_products 1 + visit admin_products_url + search_for "searchable product" expect(page).to have_field "search_term", with: "searchable product" # expect(page).to have_content "1 product found for your search criteria." @@ -110,12 +112,13 @@ describe 'As an admin, I can see the new product page' do click_link "Clear search" expect(page).to have_field "search_term", with: "" - expect(page).to have_content "Showing 1 to 15" - expect_page_to_be 1 - expect_products_count_to_be 15 + expect(page).to have_content "Showing 1 to 2" + expect_products_count_to_be 2 end it "shows a message when there are no results" do + visit admin_products_url + search_for "no results" expect(page).to have_content "No products found for your search criteria" expect(page).to have_link "Clear search" @@ -123,7 +126,9 @@ describe 'As an admin, I can see the new product page' do end context "product has producer" do - # create a product with a supplier that can be searched + before { create_products 1 } + + # create a product with a different supplier let!(:producer) { create(:supplier_enterprise, name: "Producer 1") } let!(:product_by_supplier) { create(:simple_product, supplier: producer) } @@ -139,7 +144,9 @@ describe 'As an admin, I can see the new product page' do end context "product has category" do - # create a product with a category that can be searched + before { create_products 1 } + + # create a product with a different category let!(:product_by_category) { create(:simple_product, primary_taxon: create(:taxon, name: "Category 1")) } @@ -297,6 +304,12 @@ describe 'As an admin, I can see the new product page' do end end + def create_products(amount) + amount.times do |i| + create(:simple_product, name: "product #{i}") + end + end + def expect_page_to_be(page_number) expect(page).to have_selector ".pagination span.page.current", text: page_number.to_s end