From 29fbf0438b9c8d6596e25ba9c2942545d18190ab Mon Sep 17 00:00:00 2001 From: Rob H Date: Wed, 4 Jun 2014 11:42:42 +1000 Subject: [PATCH 01/30] Adding all fields to new admin product spec --- spec/features/admin/products_spec.rb | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/spec/features/admin/products_spec.rb b/spec/features/admin/products_spec.rb index 3224254f62..35c6bc10cd 100644 --- a/spec/features/admin/products_spec.rb +++ b/spec/features/admin/products_spec.rb @@ -15,7 +15,7 @@ feature %q{ end describe "creating a product" do - scenario "assigning a important attributes" do + scenario "assigning a important attributes", js: true do login_to_admin_section click_link 'Products' @@ -23,8 +23,8 @@ feature %q{ select 'New supplier', from: 'product_supplier_id' fill_in 'product_name', with: 'A new product !!!' - # select "Weight (kg)", from: 'product_variant_unit_with_scale' - # fill_in 'product_unit_value_with_description', with: 5 + select "Weight (kg)", from: 'product_variant_unit_with_scale' + fill_in 'product_unit_value_with_description', with: 5 select taxon.name, from: "product_primary_taxon_id" fill_in 'product_price', with: '19.99' fill_in 'product_on_hand', with: 5 @@ -35,11 +35,11 @@ feature %q{ flash_message.should == 'Product "A new product !!!" has been successfully created!' product = Spree::Product.find_by_name('A new product !!!') product.supplier.should == @supplier - #product.variant_unit.should == 'weight' - #product.variant_unit_scale.should == 1000 - #product.unit_value.should == 5 - #product.unit_value_description.should == "" - #product.unit_name.should == "" + product.variant_unit.should == 'weight' + product.variant_unit_scale.should == 1000 + product.unit_value.should == 5000 + product.unit_description.should == "" + product.variant_unit_name.should == "" product.primary_taxon_id.should == taxon.id product.price.to_s.should == '19.99' product.on_hand.should == 5 @@ -50,9 +50,9 @@ feature %q{ visit spree.product_distributions_admin_product_path(product) check @distributors[0].name - select @enterprise_fees[0].name, :from => 'product_product_distributions_attributes_0_enterprise_fee_id' + select2_select @enterprise_fees[0].name, :from => 'product_product_distributions_attributes_0_enterprise_fee_id' check @distributors[2].name - select @enterprise_fees[2].name, :from => 'product_product_distributions_attributes_2_enterprise_fee_id' + select2_select @enterprise_fees[2].name, :from => 'product_product_distributions_attributes_2_enterprise_fee_id' click_button 'Update' From 8d87cfbc353bea347b1433fd7cbe66bec9231589 Mon Sep 17 00:00:00 2001 From: Rob H Date: Wed, 4 Jun 2014 14:54:42 +1000 Subject: [PATCH 02/30] Move option value naming logic into separate lib class --- app/models/spree/variant_decorator.rb | 62 +------- lib/open_food_network/option_value_namer.rb | 59 ++++++++ .../option_value_namer_spec.rb | 137 ++++++++++++++++++ spec/models/spree/variant_spec.rb | 124 ---------------- 4 files changed, 200 insertions(+), 182 deletions(-) create mode 100644 lib/open_food_network/option_value_namer.rb create mode 100644 spec/lib/open_food_network/option_value_namer_spec.rb diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index 42f6efe0ac..4ec19cf6ad 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -1,3 +1,5 @@ +require 'open_food_network/option_value_namer' + Spree::Variant.class_eval do has_many :exchange_variants, dependent: :destroy has_many :exchanges, through: :exchange_variants @@ -54,67 +56,11 @@ Spree::Variant.class_eval do delete_unit_option_values option_type = self.product.variant_unit_option_type + option_value_namer = OpenFoodNetwork::OptionValueNamer.new self if option_type - name = option_value_name + name = option_value_namer.name ov = Spree::OptionValue.where(option_type_id: option_type, name: name, presentation: name).first || Spree::OptionValue.create!({option_type: option_type, name: name, presentation: name}, without_protection: true) option_values << ov end end - - def option_value_name - value, unit = option_value_value_unit - separator = value_scaled? ? '' : ' ' - - name_fields = [] - name_fields << "#{value}#{separator}#{unit}" if value.present? && unit.present? - name_fields << unit_description if unit_description.present? - name_fields.join ' ' - end - - def value_scaled? - self.product.variant_unit_scale.present? - end - - def option_value_value_unit - if unit_value.present? - if %w(weight volume).include? self.product.variant_unit - value, unit_name = option_value_value_unit_scaled - - else - value = unit_value - unit_name = self.product.variant_unit_name - unit_name = unit_name.pluralize if value > 1 - end - - value = value.to_i if value == value.to_i - - else - value = unit_name = nil - end - - [value, unit_name] - end - - def option_value_value_unit_scaled - unit_scale, unit_name = scale_for_unit_value - - value = unit_value / unit_scale - - [value, unit_name] - end - - def scale_for_unit_value - units = {'weight' => {1.0 => 'g', 1000.0 => 'kg', 1000000.0 => 'T'}, - 'volume' => {0.001 => 'mL', 1.0 => 'L', 1000000.0 => 'ML'}} - - # Find the largest available unit where unit_value comes to >= 1 when expressed in it. - # If there is none available where this is true, use the smallest available unit. - unit = units[self.product.variant_unit].select { |scale, unit_name| - unit_value / scale >= 1 - }.to_a.last - unit = units[self.product.variant_unit].first if unit.nil? - - unit - end - end diff --git a/lib/open_food_network/option_value_namer.rb b/lib/open_food_network/option_value_namer.rb new file mode 100644 index 0000000000..2447b9fee3 --- /dev/null +++ b/lib/open_food_network/option_value_namer.rb @@ -0,0 +1,59 @@ +module OpenFoodNetwork + class OptionValueNamer < Struct.new(:variant) + def name + value, unit = self.option_value_value_unit + separator = self.value_scaled? ? '' : ' ' + + name_fields = [] + name_fields << "#{value}#{separator}#{unit}" if value.present? && unit.present? + name_fields << variant.unit_description if variant.unit_description.present? + name_fields.join ' ' + end + + def value_scaled? + variant.product.variant_unit_scale.present? + end + + def option_value_value_unit + if variant.unit_value.present? + if %w(weight volume).include? variant.product.variant_unit + value, unit_name = self.option_value_value_unit_scaled + + else + value = variant.unit_value + unit_name = variant.product.variant_unit_name + unit_name = unit_name.pluralize if value > 1 + end + + value = value.to_i if value == value.to_i + + else + value = unit_name = nil + end + + [value, unit_name] + end + + def option_value_value_unit_scaled + unit_scale, unit_name = self.scale_for_unit_value + + value = variant.unit_value / unit_scale + + [value, unit_name] + end + + def scale_for_unit_value + units = {'weight' => {1.0 => 'g', 1000.0 => 'kg', 1000000.0 => 'T'}, + 'volume' => {0.001 => 'mL', 1.0 => 'L', 1000000.0 => 'ML'}} + + # Find the largest available unit where unit_value comes to >= 1 when expressed in it. + # If there is none available where this is true, use the smallest available unit. + unit = units[variant.product.variant_unit].select { |scale, unit_name| + variant.unit_value / scale >= 1 + }.to_a.last + unit = units[variant.product.variant_unit].first if unit.nil? + + unit + end + end +end diff --git a/spec/lib/open_food_network/option_value_namer_spec.rb b/spec/lib/open_food_network/option_value_namer_spec.rb new file mode 100644 index 0000000000..22f9c54c3c --- /dev/null +++ b/spec/lib/open_food_network/option_value_namer_spec.rb @@ -0,0 +1,137 @@ +require 'spec_helper' + +module OpenFoodNetwork + describe OptionValueNamer do + describe "generating option value name" do + it "when description is blank" do + v = Spree::Variant.new unit_description: nil + subject = OptionValueNamer.new v + subject.stub(:value_scaled?) { true } + subject.stub(:option_value_value_unit) { %w(value unit) } + subject.name.should == "valueunit" + end + + it "when description is present" do + v = Spree::Variant.new unit_description: 'desc' + subject = OptionValueNamer.new v + subject.stub(:option_value_value_unit) { %w(value unit) } + subject.stub(:value_scaled?) { true } + subject.name.should == "valueunit desc" + end + + it "when value is blank and description is present" do + v = Spree::Variant.new unit_description: 'desc' + subject = OptionValueNamer.new v + subject.stub(:option_value_value_unit) { [nil, nil] } + subject.stub(:value_scaled?) { true } + subject.name.should == "desc" + end + + it "spaces value and unit when value is unscaled" do + v = Spree::Variant.new unit_description: nil + subject = OptionValueNamer.new v + subject.stub(:option_value_value_unit) { %w(value unit) } + subject.stub(:value_scaled?) { false } + subject.name.should == "value unit" + end + end + + describe "determining if a variant's value is scaled" do + it "returns true when the product has a scale" do + p = Spree::Product.new variant_unit_scale: 1000 + v = Spree::Variant.new + v.stub(:product) { p } + subject = OptionValueNamer.new v + + subject.value_scaled?.should be_true + end + + it "returns false otherwise" do + p = Spree::Product.new + v = Spree::Variant.new + v.stub(:product) { p } + subject = OptionValueNamer.new v + + subject.value_scaled?.should be_false + end + end + + describe "generating option value's value and unit" do + let(:v) { Spree::Variant.new } + let(:subject) { OptionValueNamer.new v } + + it "generates simple values" do + p = double(:product, variant_unit: 'weight', variant_unit_scale: 1.0) + v.stub(:product) { p } + v.stub(:unit_value) { 100 } + + + subject.option_value_value_unit.should == [100, 'g'] + end + + it "generates values when unit value is non-integer" do + p = double(:product, variant_unit: 'weight', variant_unit_scale: 1.0) + v.stub(:product) { p } + v.stub(:unit_value) { 123.45 } + + subject.option_value_value_unit.should == [123.45, 'g'] + end + + it "returns a value of 1 when unit value equals the scale" do + p = double(:product, variant_unit: 'weight', variant_unit_scale: 1000.0) + v.stub(:product) { p } + v.stub(:unit_value) { 1000.0 } + + subject.option_value_value_unit.should == [1, 'kg'] + end + + it "generates values for all weight scales" do + [[1.0, 'g'], [1000.0, 'kg'], [1000000.0, 'T']].each do |scale, unit| + p = double(:product, variant_unit: 'weight', variant_unit_scale: scale) + v.stub(:product) { p } + v.stub(:unit_value) { 100 * scale } + subject.option_value_value_unit.should == [100, unit] + end + end + + it "generates values for all volume scales" do + [[0.001, 'mL'], [1.0, 'L'], [1000000.0, 'ML']].each do |scale, unit| + p = double(:product, variant_unit: 'volume', variant_unit_scale: scale) + v.stub(:product) { p } + v.stub(:unit_value) { 100 * scale } + subject.option_value_value_unit.should == [100, unit] + end + end + + it "chooses the correct scale when value is very small" do + p = double(:product, variant_unit: 'volume', variant_unit_scale: 0.001) + v.stub(:product) { p } + v.stub(:unit_value) { 0.0001 } + subject.option_value_value_unit.should == [0.1, 'mL'] + end + + it "generates values for item units" do + %w(packet box).each do |unit| + p = double(:product, variant_unit: 'items', variant_unit_scale: nil, variant_unit_name: unit) + v.stub(:product) { p } + v.stub(:unit_value) { 100 } + subject.option_value_value_unit.should == [100, unit.pluralize] + end + end + + it "generates singular values for item units when value is 1" do + p = double(:product, variant_unit: 'items', variant_unit_scale: nil, variant_unit_name: 'packet') + v.stub(:product) { p } + v.stub(:unit_value) { 1 } + subject.option_value_value_unit.should == [1, 'packet'] + end + + it "returns [nil, nil] when unit value is not set" do + p = double(:product, variant_unit: 'items', variant_unit_scale: nil, variant_unit_name: 'foo') + v.stub(:product) { p } + v.stub(:unit_value) { nil } + subject.option_value_value_unit.should == [nil, nil] + end + end + end +end \ No newline at end of file diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index 4b9e1fab0a..3d0f069f3a 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -228,130 +228,6 @@ module Spree end end end - - describe "generating option value name" do - it "when description is blank" do - v = Spree::Variant.new unit_description: nil - v.stub(:option_value_value_unit) { %w(value unit) } - v.stub(:value_scaled?) { true } - v.send(:option_value_name).should == "valueunit" - end - - it "when description is present" do - v = Spree::Variant.new unit_description: 'desc' - v.stub(:option_value_value_unit) { %w(value unit) } - v.stub(:value_scaled?) { true } - v.send(:option_value_name).should == "valueunit desc" - end - - it "when value is blank and description is present" do - v = Spree::Variant.new unit_description: 'desc' - v.stub(:option_value_value_unit) { [nil, nil] } - v.stub(:value_scaled?) { true } - v.send(:option_value_name).should == "desc" - end - - it "spaces value and unit when value is unscaled" do - v = Spree::Variant.new unit_description: nil - v.stub(:option_value_value_unit) { %w(value unit) } - v.stub(:value_scaled?) { false } - v.send(:option_value_name).should == "value unit" - end - end - - describe "determining if a variant's value is scaled" do - it "returns true when the product has a scale" do - p = Spree::Product.new variant_unit_scale: 1000 - v = Spree::Variant.new - v.stub(:product) { p } - - v.send(:value_scaled?).should be_true - end - - it "returns false otherwise" do - p = Spree::Product.new - v = Spree::Variant.new - v.stub(:product) { p } - - v.send(:value_scaled?).should be_false - end - end - - describe "generating option value's value and unit" do - let(:v) { Spree::Variant.new } - - it "generates simple values" do - p = double(:product, variant_unit: 'weight', variant_unit_scale: 1.0) - v.stub(:product) { p } - v.stub(:unit_value) { 100 } - - v.send(:option_value_value_unit).should == [100, 'g'] - end - - it "generates values when unit value is non-integer" do - p = double(:product, variant_unit: 'weight', variant_unit_scale: 1.0) - v.stub(:product) { p } - v.stub(:unit_value) { 123.45 } - - v.send(:option_value_value_unit).should == [123.45, 'g'] - end - - it "returns a value of 1 when unit value equals the scale" do - p = double(:product, variant_unit: 'weight', variant_unit_scale: 1000.0) - v.stub(:product) { p } - v.stub(:unit_value) { 1000.0 } - - v.send(:option_value_value_unit).should == [1, 'kg'] - end - - it "generates values for all weight scales" do - [[1.0, 'g'], [1000.0, 'kg'], [1000000.0, 'T']].each do |scale, unit| - p = double(:product, variant_unit: 'weight', variant_unit_scale: scale) - v.stub(:product) { p } - v.stub(:unit_value) { 100 * scale } - v.send(:option_value_value_unit).should == [100, unit] - end - end - - it "generates values for all volume scales" do - [[0.001, 'mL'], [1.0, 'L'], [1000000.0, 'ML']].each do |scale, unit| - p = double(:product, variant_unit: 'volume', variant_unit_scale: scale) - v.stub(:product) { p } - v.stub(:unit_value) { 100 * scale } - v.send(:option_value_value_unit).should == [100, unit] - end - end - - it "chooses the correct scale when value is very small" do - p = double(:product, variant_unit: 'volume', variant_unit_scale: 0.001) - v.stub(:product) { p } - v.stub(:unit_value) { 0.0001 } - v.send(:option_value_value_unit).should == [0.1, 'mL'] - end - - it "generates values for item units" do - %w(packet box).each do |unit| - p = double(:product, variant_unit: 'items', variant_unit_scale: nil, variant_unit_name: unit) - v.stub(:product) { p } - v.stub(:unit_value) { 100 } - v.send(:option_value_value_unit).should == [100, unit.pluralize] - end - end - - it "generates singular values for item units when value is 1" do - p = double(:product, variant_unit: 'items', variant_unit_scale: nil, variant_unit_name: 'packet') - v.stub(:product) { p } - v.stub(:unit_value) { 1 } - v.send(:option_value_value_unit).should == [1, 'packet'] - end - - it "returns [nil, nil] when unit value is not set" do - p = double(:product, variant_unit: 'items', variant_unit_scale: nil, variant_unit_name: 'foo') - v.stub(:product) { p } - v.stub(:unit_value) { nil } - v.send(:option_value_value_unit).should == [nil, nil] - end - end end describe "deleting unit option values" do From f4c15bfc4861203c546c52788c1623ab6f968aca Mon Sep 17 00:00:00 2001 From: Rob H Date: Wed, 4 Jun 2014 15:21:31 +1000 Subject: [PATCH 03/30] Adding name and display_as fields to variants --- ...40604051248_add_name_and_display_as_to_spree_variants.rb | 6 ++++++ db/schema.rb | 6 ++++-- 2 files changed, 10 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20140604051248_add_name_and_display_as_to_spree_variants.rb diff --git a/db/migrate/20140604051248_add_name_and_display_as_to_spree_variants.rb b/db/migrate/20140604051248_add_name_and_display_as_to_spree_variants.rb new file mode 100644 index 0000000000..5955ec0967 --- /dev/null +++ b/db/migrate/20140604051248_add_name_and_display_as_to_spree_variants.rb @@ -0,0 +1,6 @@ +class AddNameAndDisplayAsToSpreeVariants < ActiveRecord::Migration + def change + add_column :spree_variants, :name, :string + add_column :spree_variants, :display_as, :string + end +end diff --git a/db/schema.rb b/db/schema.rb index 999ff55705..dc08f3ac2b 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20140522044009) do +ActiveRecord::Schema.define(:version => 20140604051248) do create_table "adjustment_metadata", :force => true do |t| t.integer "adjustment_id" @@ -681,7 +681,7 @@ ActiveRecord::Schema.define(:version => 20140522044009) do t.float "variant_unit_scale" t.string "variant_unit_name" t.text "notes" - t.integer "primary_taxon_id", :null => false + t.integer "primary_taxon_id" end add_index "spree_products", ["available_on"], :name => "index_products_on_available_on" @@ -962,6 +962,8 @@ ActiveRecord::Schema.define(:version => 20140522044009) do t.string "cost_currency" t.float "unit_value" t.string "unit_description", :default => "" + t.string "name" + t.string "display_as" end add_index "spree_variants", ["product_id"], :name => "index_variants_on_product_id" From 7f2b3d62f42183e2050189264e5d108ada9bea89 Mon Sep 17 00:00:00 2001 From: Rob H Date: Wed, 4 Jun 2014 15:39:04 +1000 Subject: [PATCH 04/30] OptionValueNamer spec refactor --- .../open_food_network/option_value_namer_spec.rb | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/spec/lib/open_food_network/option_value_namer_spec.rb b/spec/lib/open_food_network/option_value_namer_spec.rb index 22f9c54c3c..c0758d2a37 100644 --- a/spec/lib/open_food_network/option_value_namer_spec.rb +++ b/spec/lib/open_food_network/option_value_namer_spec.rb @@ -3,33 +3,32 @@ require 'spec_helper' module OpenFoodNetwork describe OptionValueNamer do describe "generating option value name" do + let(:v) { Spree::Variant.new } + let(:subject) { OptionValueNamer.new v } + it "when description is blank" do - v = Spree::Variant.new unit_description: nil - subject = OptionValueNamer.new v + v.stub(:unit_description) { nil } subject.stub(:value_scaled?) { true } subject.stub(:option_value_value_unit) { %w(value unit) } subject.name.should == "valueunit" end it "when description is present" do - v = Spree::Variant.new unit_description: 'desc' - subject = OptionValueNamer.new v + v.stub(:unit_description) { 'desc' } subject.stub(:option_value_value_unit) { %w(value unit) } subject.stub(:value_scaled?) { true } subject.name.should == "valueunit desc" end it "when value is blank and description is present" do - v = Spree::Variant.new unit_description: 'desc' - subject = OptionValueNamer.new v + v.stub(:unit_description) { 'desc' } subject.stub(:option_value_value_unit) { [nil, nil] } subject.stub(:value_scaled?) { true } subject.name.should == "desc" end it "spaces value and unit when value is unscaled" do - v = Spree::Variant.new unit_description: nil - subject = OptionValueNamer.new v + v.stub(:unit_description) { nil } subject.stub(:option_value_value_unit) { %w(value unit) } subject.stub(:value_scaled?) { false } subject.name.should == "value unit" From 3240a4e08eb3bcfb90e1e8a9760df6791e40cbd2 Mon Sep 17 00:00:00 2001 From: Rob H Date: Wed, 4 Jun 2014 17:23:11 +1000 Subject: [PATCH 05/30] Use display_as as option value for variant when it is present --- app/models/spree/variant_decorator.rb | 12 +++++- spec/models/spree/variant_spec.rb | 53 ++++++++++++++++++++------- 2 files changed, 49 insertions(+), 16 deletions(-) diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index 4ec19cf6ad..bded64b9e8 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -56,11 +56,19 @@ Spree::Variant.class_eval do delete_unit_option_values option_type = self.product.variant_unit_option_type - option_value_namer = OpenFoodNetwork::OptionValueNamer.new self if option_type - name = option_value_namer.name + name = option_value_name ov = Spree::OptionValue.where(option_type_id: option_type, name: name, presentation: name).first || Spree::OptionValue.create!({option_type: option_type, name: name, presentation: name}, without_protection: true) option_values << ov end end + + def option_value_name + if display_as.present? + display_as + else + option_value_namer = OpenFoodNetwork::OptionValueNamer.new self + option_value_namer.name + end + end end diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index 3d0f069f3a..535508ebe2 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -1,4 +1,5 @@ require 'spec_helper' +require 'open_food_network/option_value_namer' module Spree describe Variant do @@ -207,25 +208,49 @@ module Spree v_orig.option_values.should include ov end end + end - context "when the variant already has a value set (and all required option values exist)" do - let!(:p0) { create(:simple_product, variant_unit: 'weight', variant_unit_scale: 1) } - let!(:v0) { create(:variant, product: p0, unit_value: 10, unit_description: 'foo') } + context "when the variant already has a value set (and all required option values exist)" do + let!(:p0) { create(:simple_product, variant_unit: 'weight', variant_unit_scale: 1) } + let!(:v0) { create(:variant, product: p0, unit_value: 10, unit_description: 'foo') } - let!(:p) { create(:simple_product, variant_unit: 'weight', variant_unit_scale: 1) } - let!(:v) { create(:variant, product: p, unit_value: 5, unit_description: 'bar') } + let!(:p) { create(:simple_product, variant_unit: 'weight', variant_unit_scale: 1) } + let!(:v) { create(:variant, product: p, unit_value: 5, unit_description: 'bar') } - it "removes the old option value and assigns the new one" do - ov_orig = v.option_values.last - ov_new = v0.option_values.last + it "removes the old option value and assigns the new one" do + ov_orig = v.option_values.last + ov_new = v0.option_values.last - expect { - v.update_attributes!(unit_value: 10, unit_description: 'foo') - }.to change(Spree::OptionValue, :count).by(0) + expect { + v.update_attributes!(unit_value: 10, unit_description: 'foo') + }.to change(Spree::OptionValue, :count).by(0) - v.option_values.should_not include ov_orig - v.option_values.should include ov_new - end + v.option_values.should_not include ov_orig + v.option_values.should include ov_new + end + end + + context "when the variant does not have a display_as value set" do + let!(:p) { create(:simple_product, variant_unit: 'weight', variant_unit_scale: 1) } + let!(:v) { create(:variant, product: p, unit_value: 5, unit_description: 'bar', display_as: '') } + + it "requests the name of the new option_value from OptionValueName" do + OpenFoodNetwork::OptionValueNamer.any_instance.should_receive(:name).exactly(1).times.and_call_original + v.update_attributes(unit_value: 10, unit_description: 'foo') + ov = v.option_values.last + ov.name.should == "10g foo" + end + end + + context "when the variant has a display_as value set" do + let!(:p) { create(:simple_product, variant_unit: 'weight', variant_unit_scale: 1) } + let!(:v) { create(:variant, product: p, unit_value: 5, unit_description: 'bar', display_as: 'FOOS!') } + + it "requests the name of the new option_value from OptionValueName" do + OpenFoodNetwork::OptionValueNamer.any_instance.should_not_receive(:name) + v.update_attributes!(unit_value: 10, unit_description: 'foo') + ov = v.option_values.last + ov.name.should == "FOOS!" end end end From 1eac76fbbadb772e1021d8e8a7de5b094a5d25af Mon Sep 17 00:00:00 2001 From: Rob H Date: Thu, 5 Jun 2014 13:39:53 +1000 Subject: [PATCH 06/30] Change variant name to display name --- ...248_add_display_name_and_display_as_to_spree_variants.rb | 6 ++++++ ...40604051248_add_name_and_display_as_to_spree_variants.rb | 6 ------ db/schema.rb | 2 +- 3 files changed, 7 insertions(+), 7 deletions(-) create mode 100644 db/migrate/20140604051248_add_display_name_and_display_as_to_spree_variants.rb delete mode 100644 db/migrate/20140604051248_add_name_and_display_as_to_spree_variants.rb diff --git a/db/migrate/20140604051248_add_display_name_and_display_as_to_spree_variants.rb b/db/migrate/20140604051248_add_display_name_and_display_as_to_spree_variants.rb new file mode 100644 index 0000000000..a2c1080b31 --- /dev/null +++ b/db/migrate/20140604051248_add_display_name_and_display_as_to_spree_variants.rb @@ -0,0 +1,6 @@ +class AddDisplayNameAndDisplayAsToSpreeVariants < ActiveRecord::Migration + def change + add_column :spree_variants, :display_name, :string + add_column :spree_variants, :display_as, :string + end +end diff --git a/db/migrate/20140604051248_add_name_and_display_as_to_spree_variants.rb b/db/migrate/20140604051248_add_name_and_display_as_to_spree_variants.rb deleted file mode 100644 index 5955ec0967..0000000000 --- a/db/migrate/20140604051248_add_name_and_display_as_to_spree_variants.rb +++ /dev/null @@ -1,6 +0,0 @@ -class AddNameAndDisplayAsToSpreeVariants < ActiveRecord::Migration - def change - add_column :spree_variants, :name, :string - add_column :spree_variants, :display_as, :string - end -end diff --git a/db/schema.rb b/db/schema.rb index dc08f3ac2b..2b3c2cbf62 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -962,7 +962,7 @@ ActiveRecord::Schema.define(:version => 20140604051248) do t.string "cost_currency" t.float "unit_value" t.string "unit_description", :default => "" - t.string "name" + t.string "display_name" t.string "display_as" end From 43c5c373263f1caae096bf2111928e2236e395be Mon Sep 17 00:00:00 2001 From: Rob H Date: Fri, 6 Jun 2014 10:10:41 +1000 Subject: [PATCH 07/30] Add fields for variant display name and display as to bpe --- .../admin/bulk_product_update.js.coffee | 23 +++++++++++++++---- .../admin/directives/track_master.js.coffee | 9 ++++++++ .../admin/services/dirty_products.js.coffee | 5 ++++ app/models/spree/product_set.rb | 20 ++++++++++------ app/models/spree/variant_decorator.rb | 2 +- .../spree/admin/products/bulk_edit.html.haml | 12 +++++++--- .../spree/api/variants/bulk_show.v1.rabl | 2 +- .../spree/api/variants_controller_spec.rb | 2 +- .../admin/bulk_product_update_spec.rb | 21 ++++++++++++----- 9 files changed, 73 insertions(+), 23 deletions(-) create mode 100644 app/assets/javascripts/admin/directives/track_master.js.coffee diff --git a/app/assets/javascripts/admin/bulk_product_update.js.coffee b/app/assets/javascripts/admin/bulk_product_update.js.coffee index 8d50a3dc1f..93cf4b430b 100644 --- a/app/assets/javascripts/admin/bulk_product_update.js.coffee +++ b/app/assets/javascripts/admin/bulk_product_update.js.coffee @@ -196,6 +196,8 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", [ unit_value: null unit_description: null on_demand: false + display_as: null + display_name: null on_hand: null price: null $scope.displayProperties[product.id].showVariants = true @@ -425,6 +427,7 @@ filterSubmitProducts = (productsToFilter) -> if product.hasOwnProperty("id") filteredProduct = {id: product.id} filteredVariants = [] + filteredMaster = null hasUpdatableProperty = false if product.hasOwnProperty("variants") @@ -435,11 +438,14 @@ filterSubmitProducts = (productsToFilter) -> filteredVariants.push filteredVariant if variantHasUpdatableProperty if product.master?.hasOwnProperty("unit_value") - filteredProduct.unit_value = product.master.unit_value - hasUpdatableProperty = true + filteredMaster ?= { id: product.master.id } + filteredMaster.unit_value = product.master.unit_value if product.master?.hasOwnProperty("unit_description") - filteredProduct.unit_description = product.master.unit_description - hasUpdatableProperty = true + 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("name") filteredProduct.name = product.name @@ -466,6 +472,9 @@ filterSubmitProducts = (productsToFilter) -> if product.hasOwnProperty("available_on") filteredProduct.available_on = product.available_on 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 @@ -491,6 +500,12 @@ filterSubmitVariant = (variant) -> if variant.hasOwnProperty("unit_description") filteredVariant.unit_description = variant.unit_description hasUpdatableProperty = true + if variant.hasOwnProperty("display_name") + filteredVariant.display_name = variant.display_name + hasUpdatableProperty = true + if variant.hasOwnProperty("display_as") + filteredVariant.display_as = variant.display_as + hasUpdatableProperty = true {filteredVariant: filteredVariant, hasUpdatableProperty: hasUpdatableProperty} diff --git a/app/assets/javascripts/admin/directives/track_master.js.coffee b/app/assets/javascripts/admin/directives/track_master.js.coffee new file mode 100644 index 0000000000..ce26a4aa32 --- /dev/null +++ b/app/assets/javascripts/admin/directives/track_master.js.coffee @@ -0,0 +1,9 @@ +angular.module("ofn.admin").directive "ofnTrackMaster", ["DirtyProducts", (DirtyProducts) -> + require: "ngModel" + link: (scope, element, attrs, ngModel) -> + ngModel.$parsers.push (viewValue) -> + if ngModel.$dirty + DirtyProducts.addMasterProperty scope.product.id, scope.product.master.id, attrs.ofnTrackMaster, viewValue + scope.displayDirtyProducts() + viewValue + ] \ No newline at end of file diff --git a/app/assets/javascripts/admin/services/dirty_products.js.coffee b/app/assets/javascripts/admin/services/dirty_products.js.coffee index 16c10e1e34..1c8d987dbc 100644 --- a/app/assets/javascripts/admin/services/dirty_products.js.coffee +++ b/app/assets/javascripts/admin/services/dirty_products.js.coffee @@ -12,6 +12,11 @@ angular.module("ofn.admin").factory "DirtyProducts", ($parse) -> addProductProperty: (productID, propertyName, propertyValue) -> addDirtyProperty dirtyProducts, productID, propertyName, propertyValue + + addMasterProperty: (productID, masterID, propertyName, propertyValue) -> + if !dirtyProducts.hasOwnProperty(productID) or !dirtyProducts[productID].hasOwnProperty("master") + addDirtyProperty dirtyProducts, productID, "master", { id: masterID } + $parse(propertyName).assign(dirtyProducts[productID]["master"], propertyValue) addVariantProperty: (productID, variantID, propertyName, propertyValue) -> if !dirtyProducts.hasOwnProperty(productID) or !dirtyProducts[productID].hasOwnProperty("variants") diff --git a/app/models/spree/product_set.rb b/app/models/spree/product_set.rb index 0f2ebd511e..d6f361c82e 100644 --- a/app/models/spree/product_set.rb +++ b/app/models/spree/product_set.rb @@ -14,18 +14,24 @@ class Spree::ProductSet < ModelSet if e.nil? @klass.new(attributes).save unless @reject_if.andand.call(attributes) else - e.update_attributes(attributes.except(:id, :variants_attributes)) and (attributes[:variants_attributes] ? update_variants_attributes(e, attributes[:variants_attributes]) : true ) + e.update_attributes(attributes.except(:id, :variants_attributes, :master_attributes)) and + (attributes[:variants_attributes] ? update_variants_attributes(e, attributes[:variants_attributes]) : true ) and + (attributes[:master_attributes] ? update_variant(e, attributes[:master_attributes]) : true ) end end def update_variants_attributes(product, variants_attributes) variants_attributes.each do |attributes| - e = product.variants.detect { |e| e.id.to_s == attributes[:id].to_s && !e.id.nil? } - if e.present? - e.update_attributes(attributes.except(:id)) - else - product.variants.create attributes - end + update_variant(product, attributes) + end + end + + def update_variant(product, variant_attributes) + e = product.variants_including_master.detect { |e| e.id.to_s == variant_attributes[:id].to_s && !e.id.nil? } + if e.present? + e.update_attributes(variant_attributes.except(:id)) + else + product.variants.create variant_attributes end end diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index bded64b9e8..2ffb07d4e1 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -4,7 +4,7 @@ Spree::Variant.class_eval do has_many :exchange_variants, dependent: :destroy has_many :exchanges, through: :exchange_variants - attr_accessible :unit_value, :unit_description, :images_attributes + attr_accessible :unit_value, :unit_description, :images_attributes, :display_as, :display_name accepts_nested_attributes_for :images validates_presence_of :unit_value, diff --git a/app/views/spree/admin/products/bulk_edit.html.haml b/app/views/spree/admin/products/bulk_edit.html.haml index fa295adb47..8d943adb8e 100644 --- a/app/views/spree/admin/products/bulk_edit.html.haml +++ b/app/views/spree/admin/products/bulk_edit.html.haml @@ -93,6 +93,7 @@ %col{'style' => 'width: 20%;'} %col{'style' => 'width: 12%;'} %col{'style' => 'width: 12%;'} + %col{'style' => 'width: 12%;'} %col %col %col @@ -106,6 +107,7 @@ %th{ 'ng-show' => 'columns.supplier.visible' } Supplier %th{ 'ng-show' => 'columns.name.visible' } Name %th{ 'ng-show' => 'columns.unit.visible' } Unit / Value + %th{ 'ng-show' => 'columns.unit.visible' } Display As %th{ 'ng-show' => 'columns.price.visible' } Price %th{ 'ng-show' => 'columns.on_hand.visible' } On Hand %th{ 'ng-show' => 'columns.taxons.visible' } Taxons @@ -125,8 +127,10 @@ %td.unit{ 'ng-show' => 'columns.unit.visible' } %select.select2{ 'ng-model' => 'product.variant_unit_with_scale', :name => 'variant_unit_with_scale', 'ofn-track-product' => 'variant_unit_with_scale', 'ng-options' => 'unit[1] as unit[0] for unit in variant_unit_options' } %option{'value' => '', 'ng-hide' => "hasVariants(product) && hasUnit(product)"} - %input{ 'ng-model' => 'product.master.unit_value_with_description', :name => 'master_unit_value_with_description', 'ofn-track-product' => 'master.unit_value_with_description', :type => 'text', :placeholder => 'value', 'ng-show' => "!hasVariants(product) && hasUnit(product)" } + %input{ 'ng-model' => 'product.master.unit_value_with_description', :name => 'master_unit_value_with_description', 'ofn-track-master' => 'unit_value_with_description', :type => 'text', :placeholder => 'value', 'ng-show' => "!hasVariants(product) && hasUnit(product)" } %input{ 'ng-model' => 'product.variant_unit_name', :name => 'variant_unit_name', 'ofn-track-product' => 'variant_unit_name', :placeholder => 'unit', 'ng-show' => "product.variant_unit_with_scale == 'items'", :type => 'text' } + %td.display_as{ 'ng-show' => 'columns.unit.visible' } + %input{ name: 'display_as', 'ofn-track-master' => 'display_as', :type => 'text', placeholder: "{{ product.master.options_text }}", ng: { hide: 'hasVariants(product)', model: 'product.master.display_as' } } %td{ 'ng-show' => 'columns.price.visible' } %input{ 'ng-model' => 'product.price', 'ofn-decimal' => :true, :name => 'price', 'ofn-track-product' => 'price', :type => 'text', 'ng-hide' => 'hasVariants(product)' } %td{ 'ng-show' => 'columns.on_hand.visible' } @@ -148,9 +152,11 @@ %a{ :class => "add-variant icon-plus-sign", 'ng-click' => "addVariant(product)", 'ng-show' => "$last" } %td{ 'ng-show' => 'columns.supplier.visible' } %td{ 'ng-show' => 'columns.name.visible' } - {{ variant.options_text }} - %td{ 'ng-show' => 'columns.unit.visible' } + %input{ 'ng-model' => 'variant.display_name', :name => 'variant_display_name', 'ofn-track-variant' => 'display_name', :type => 'text', placeholder: "{{ product.name }}" } + %td.unit_value{ 'ng-show' => 'columns.unit.visible' } %input{ 'ng-model' => 'variant.unit_value_with_description', :name => 'variant_unit_value_with_description', 'ofn-track-variant' => 'unit_value_with_description', :type => 'text' } + %td.display_as{ 'ng-show' => 'columns.unit.visible' } + %input{ 'ng-model' => 'variant.display_as', :name => 'variant_display_as', 'ofn-track-variant' => 'display_as', :type => 'text', placeholder: "{{ variant.options_text }}" } %td{ 'ng-show' => 'columns.price.visible' } %input{ 'ng-model' => 'variant.price', 'ofn-decimal' => :true, :name => 'variant_price', 'ofn-track-variant' => 'price', :type => 'text' } %td{ 'ng-show' => 'columns.on_hand.visible' } diff --git a/app/views/spree/api/variants/bulk_show.v1.rabl b/app/views/spree/api/variants/bulk_show.v1.rabl index 52e293a890..4a2a6bae9c 100644 --- a/app/views/spree/api/variants/bulk_show.v1.rabl +++ b/app/views/spree/api/variants/bulk_show.v1.rabl @@ -1,6 +1,6 @@ object @variant -attributes :id, :options_text, :unit_value, :unit_description, :on_demand +attributes :id, :options_text, :unit_value, :unit_description, :on_demand, :display_as, :display_name # Infinity is not a valid JSON object, but Rails encodes it anyway node( :on_hand ) { |v| v.on_hand.nil? ? 0 : ( v.on_hand.to_f.finite? ? v.on_hand : "On demand" ) } diff --git a/spec/controllers/spree/api/variants_controller_spec.rb b/spec/controllers/spree/api/variants_controller_spec.rb index 818e10457a..5df1a40dde 100644 --- a/spec/controllers/spree/api/variants_controller_spec.rb +++ b/spec/controllers/spree/api/variants_controller_spec.rb @@ -8,7 +8,7 @@ module Spree let!(:variant1) { FactoryGirl.create(:variant) } let!(:variant2) { FactoryGirl.create(:variant) } let!(:variant3) { FactoryGirl.create(:variant) } - let(:attributes) { [:id, :options_text, :price, :on_hand] } + let(:attributes) { [:id, :options_text, :price, :on_hand, :unit_value, :unit_description, :on_demand, :display_as, :display_name] } let(:unit_attributes) { [:id, :unit_text, :unit_value] } before do diff --git a/spec/features/admin/bulk_product_update_spec.rb b/spec/features/admin/bulk_product_update_spec.rb index 0e15fbd2f4..1e0bcb3c03 100644 --- a/spec/features/admin/bulk_product_update_spec.rb +++ b/spec/features/admin/bulk_product_update_spec.rb @@ -166,8 +166,8 @@ feature %q{ end it "displays a list of variants for each product" do - v1 = FactoryGirl.create(:variant) - v2 = FactoryGirl.create(:variant) + v1 = FactoryGirl.create(:variant, display_name: "something1" ) + v2 = FactoryGirl.create(:variant, display_name: "something2" ) visit '/admin/products/bulk_edit' page.should have_selector "a.view-variants" @@ -175,8 +175,8 @@ feature %q{ page.should have_field "product_name", with: v1.product.name page.should have_field "product_name", with: v2.product.name - page.should have_selector "td", text: v1.options_text - page.should have_selector "td", text: v2.options_text + page.should have_field "variant_display_name", with: v1.display_name + page.should have_field "variant_display_name", with: v2.display_name end it "displays an on_hand input (for each variant) for each product" do @@ -208,14 +208,16 @@ feature %q{ it "displays a unit value field (for each variant) for each product" do p1 = FactoryGirl.create(:product, price: 2.0, variant_unit: "weight", variant_unit_scale: "1000") - v1 = FactoryGirl.create(:variant, product: p1, is_master: false, price: 12.75, unit_value: 1200, unit_description: "(small bag)") - v2 = FactoryGirl.create(:variant, product: p1, is_master: false, price: 2.50, unit_value: 4800, unit_description: "(large bag)") + v1 = FactoryGirl.create(:variant, product: p1, is_master: false, price: 12.75, unit_value: 1200, unit_description: "(small bag)", display_as: "bag") + v2 = FactoryGirl.create(:variant, product: p1, is_master: false, price: 2.50, unit_value: 4800, unit_description: "(large bag)", display_as: "bin") visit '/admin/products/bulk_edit' all("a.view-variants").each{ |e| e.click } page.should have_field "variant_unit_value_with_description", with: "1.2 (small bag)" page.should have_field "variant_unit_value_with_description", with: "4.8 (large bag)" + page.should have_field "variant_display_as", with: "bag" + page.should have_field "variant_display_as", with: "bin" end end @@ -274,10 +276,13 @@ feature %q{ page.all("tr.variant").count.should == 1 # When I fill out variant details and hit update + fill_in "variant_display_name", with: "Case of 12 Bottles" fill_in "variant_unit_value_with_description", with: "4000 (12x250 mL bottles)" + fill_in "variant_display_as", with: "Case" fill_in "variant_price", with: "4.0" fill_in "variant_on_hand", with: "10" click_button 'Update' + page.find("span#update-status-message").should have_content "Update complete" # Then I should see edit buttons for the new variant @@ -288,7 +293,9 @@ feature %q{ page.should have_selector "a.view-variants" first("a.view-variants").click + page.should have_field "variant_display_name", with: "Case of 12 Bottles" page.should have_field "variant_unit_value_with_description", with: "4000 (12x250 mL bottles)" + page.should have_field "variant_display_as", with: "Case" page.should have_field "variant_price", with: "4.0" page.should have_field "variant_on_hand", with: "10" end @@ -322,6 +329,7 @@ feature %q{ fill_in "price", with: "20" select "Weight (kg)", from: "variant_unit_with_scale" fill_in "on_hand", with: "18" + fill_in "display_as", with: "Big Bag" click_button 'Update' page.find("span#update-status-message").should have_content "Update complete" @@ -334,6 +342,7 @@ feature %q{ page.should have_field "price", with: "20.0" page.should have_select "variant_unit_with_scale", selected: "Weight (kg)" page.should have_field "on_hand", with: "18" + page.should have_field "display_as", with: "Big Bag" end scenario "updating a product with a variant unit of 'items'" do From 6e04eeba0704f3e665823143ccf23ffa7d850994 Mon Sep 17 00:00:00 2001 From: Rob H Date: Fri, 6 Jun 2014 15:43:05 +1000 Subject: [PATCH 08/30] Adding dynamic text to display as placeholder --- .../variant_units_controller.js.coffee | 42 ++++++++++++ .../services/option_value_namer.js.coffee | 66 +++++++++++++++++++ .../spree/admin/products/bulk_edit.html.haml | 2 +- 3 files changed, 109 insertions(+), 1 deletion(-) create mode 100644 app/assets/javascripts/admin/controllers/variant_units_controller.js.coffee create mode 100644 app/assets/javascripts/admin/services/option_value_namer.js.coffee diff --git a/app/assets/javascripts/admin/controllers/variant_units_controller.js.coffee b/app/assets/javascripts/admin/controllers/variant_units_controller.js.coffee new file mode 100644 index 0000000000..41d87113a1 --- /dev/null +++ b/app/assets/javascripts/admin/controllers/variant_units_controller.js.coffee @@ -0,0 +1,42 @@ +angular.module("ofn.admin") + .controller "VariantUnitsCtrl", ($scope, optionValueNamer) -> + $scope.$watchCollection '[variant.unit_value_with_description, product.variant_unit_name, product.variant_unit_with_scale]', -> + [variant_unit, variant_unit_scale] = $scope.productUnitProperties() + [unit_value, unit_description] = $scope.variantUnitProperties(variant_unit_scale) + variant_object = + unit_value: unit_value + unit_description: unit_description + product: + variant_unit_scale: variant_unit_scale + variant_unit: variant_unit + variant_unit_name: $scope.product.variant_unit_name + + $scope.variant.options_text = new optionValueNamer(variant_object).name() + + $scope.productUnitProperties = -> + # get relevant product properties + if $scope.product.variant_unit_with_scale + match = $scope.product.variant_unit_with_scale.match(/^([^_]+)_([\d\.]+)$/) + if match + variant_unit = match[1] + variant_unit_scale = parseFloat(match[2]) + else + variant_unit = $scope.product.variant_unit_with_scale + variant_unit_scale = null + else + variant_unit = variant_unit_scale = null + + [variant_unit, variant_unit_scale] + + $scope.variantUnitProperties = (variant_unit_scale)-> + # get relevant variant properties + if $scope.variant.hasOwnProperty("unit_value_with_description") + match = $scope.variant.unit_value_with_description.match(/^([\d\.]+(?= |$)|)( |)(.*)$/) + if match + unit_value = parseFloat(match[1]) + unit_value = null if isNaN(unit_value) + unit_value *= variant_unit_scale if unit_value && variant_unit_scale + unit_description = match[3] + [unit_value, unit_description] + + \ No newline at end of file diff --git a/app/assets/javascripts/admin/services/option_value_namer.js.coffee b/app/assets/javascripts/admin/services/option_value_namer.js.coffee new file mode 100644 index 0000000000..8d127f01c7 --- /dev/null +++ b/app/assets/javascripts/admin/services/option_value_namer.js.coffee @@ -0,0 +1,66 @@ +angular.module("ofn.admin").factory "optionValueNamer", ($resource) -> + class OptionValueNamer + constructor: (@variant) -> + + name: -> + [value, unit] = @option_value_value_unit() + separator = if @value_scaled() then '' else ' ' + + name_fields = [] + name_fields.push "#{value}#{separator}#{unit}" if value? && unit? + name_fields.push @variant.unit_description if @variant.unit_description? + name_fields.join ' ' + + value_scaled: -> + @variant.product.variant_unit_scale? + + option_value_value_unit: -> + if @variant.unit_value? + if @variant.product.variant_unit in ["weight", "volume"] + [value, unit_name] = @option_value_value_unit_scaled() + + else + value = @variant.unit_value + unit_name = @variant.product.variant_unit_name + # TODO needs to add pluralize to line below + # unit_name = unit_name if value > 1 + + value = parseInt(value, 10) if value == parseInt(value, 10) + + else + value = unit_name = null + + [value, unit_name] + + option_value_value_unit_scaled: -> + [unit_scale, unit_name] = @scale_for_unit_value() + + value = @variant.unit_value / unit_scale + + [value, unit_name] + + scale_for_unit_value: -> + units = + 'weight': + 1.0: 'g' + 1000.0: 'kg' + 1000000.0: 'T' + 'volume': + 0.001: 'mL' + 1.0: 'L' + 1000000.0: 'ML' + + # Find the largest available unit where unit_value comes to >= 1 when expressed in it. + # If there is none available where this is true, use the smallest available unit. + unit = ([scale, unit_name] for scale, unit_name of units[@variant.product.variant_unit] when @variant.unit_value / scale >= 1).reduce (unit, [scale, unit_name]) -> + if (unit && scale > unit[0]) || !unit? + [scale, unit_name] + else + unit + , null + if !unit? + unit = ([scale, unit_name] for scale, unit_name of units[@variant.product.variant_unit]).reduce (unit, [scale, unit_name]) -> + if scale < unit[0] then [scale, unit_name] else unit + , [Infinity,""] + + unit \ No newline at end of file diff --git a/app/views/spree/admin/products/bulk_edit.html.haml b/app/views/spree/admin/products/bulk_edit.html.haml index 8d943adb8e..2b636af09d 100644 --- a/app/views/spree/admin/products/bulk_edit.html.haml +++ b/app/views/spree/admin/products/bulk_edit.html.haml @@ -155,7 +155,7 @@ %input{ 'ng-model' => 'variant.display_name', :name => 'variant_display_name', 'ofn-track-variant' => 'display_name', :type => 'text', placeholder: "{{ product.name }}" } %td.unit_value{ 'ng-show' => 'columns.unit.visible' } %input{ 'ng-model' => 'variant.unit_value_with_description', :name => 'variant_unit_value_with_description', 'ofn-track-variant' => 'unit_value_with_description', :type => 'text' } - %td.display_as{ 'ng-show' => 'columns.unit.visible' } + %td.display_as{ 'ng-show' => 'columns.unit.visible', 'ng-controller' => "VariantUnitsCtrl" } %input{ 'ng-model' => 'variant.display_as', :name => 'variant_display_as', 'ofn-track-variant' => 'display_as', :type => 'text', placeholder: "{{ variant.options_text }}" } %td{ 'ng-show' => 'columns.price.visible' } %input{ 'ng-model' => 'variant.price', 'ofn-decimal' => :true, :name => 'variant_price', 'ofn-track-variant' => 'price', :type => 'text' } From 12809438acbaf124bb110c0f36ca17d9c6dc77c3 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 11 Jun 2014 10:33:59 +1000 Subject: [PATCH 09/30] Orders and fulfilment report handles order cycles with nil opening or closing times --- app/helpers/spree/reports_helper.rb | 11 +++ .../reports/orders_and_fulfillment.html.haml | 3 +- spec/features/admin/reports_spec.rb | 74 +++++++++++-------- 3 files changed, 56 insertions(+), 32 deletions(-) create mode 100644 app/helpers/spree/reports_helper.rb diff --git a/app/helpers/spree/reports_helper.rb b/app/helpers/spree/reports_helper.rb new file mode 100644 index 0000000000..573b790051 --- /dev/null +++ b/app/helpers/spree/reports_helper.rb @@ -0,0 +1,11 @@ +module Spree + module ReportsHelper + def report_order_cycle_options(order_cycles) + order_cycles.map do |oc| + orders_open_at = oc.orders_open_at.andand.to_s(:short) || 'NA' + orders_close_at = oc.orders_close_at.andand.to_s(:short) || 'NA' + [ "#{oc.name}   (#{orders_open_at} - #{orders_close_at})".html_safe, oc.id ] + end + end + end +end diff --git a/app/views/spree/admin/reports/orders_and_fulfillment.html.haml b/app/views/spree/admin/reports/orders_and_fulfillment.html.haml index 0935c24cc7..522fdfdf39 100644 --- a/app/views/spree/admin/reports/orders_and_fulfillment.html.haml +++ b/app/views/spree/admin/reports/orders_and_fulfillment.html.haml @@ -20,8 +20,7 @@ .row .alpha.two.columns= label_tag nil, "Order Cycles: " .omega.fourteen.columns - - order_cycles_select = @order_cycles.collect {|oc| [ "#{oc.name}   (#{oc.orders_open_at.to_s(:short)} - #{oc.orders_close_at.to_s(:short)})".html_safe, oc.id ] } - = f.select(:order_cycle_id_in, order_cycles_select, {selected: params[:q][:order_cycle_id_in]}, {class: "select2 fullwidth", multiple: true}) + = f.select(:order_cycle_id_in, report_order_cycle_options(@order_cycles), {selected: params[:q][:order_cycle_id_in]}, {class: "select2 fullwidth", multiple: true}) .row .alpha.two.columns= label_tag nil, "Report Type: " diff --git a/spec/features/admin/reports_spec.rb b/spec/features/admin/reports_spec.rb index 51c9f83e07..e880f25d23 100644 --- a/spec/features/admin/reports_spec.rb +++ b/spec/features/admin/reports_spec.rb @@ -81,43 +81,57 @@ feature %q{ page.should have_content 'Payment State' end - scenario "orders & fulfillment reports" do - login_to_admin_section - click_link 'Reports' - click_link 'Orders & Fulfillment Reports' + describe "orders & fulfilment reports" do + it "loads the report page" do + login_to_admin_section + click_link 'Reports' + click_link 'Orders & Fulfillment Reports' - page.should have_content 'Supplier' - end + page.should have_content 'Supplier' + end - scenario "orders & fulfillment reports are precise to time of day, not just date" do - # Given two orders on the same day at different times - @bill_address = create(:address) - @distributor_address = create(:address, :address1 => "distributor address", :city => 'The Shire', :zipcode => "1234") - @distributor = create(:distributor_enterprise, :address => @distributor_address) - product = create(:product) - product_distribution = create(:product_distribution, :product => product, :distributor => @distributor) - @shipping_instructions = "pick up on thursday please!" - @order1 = create(:order, :distributor => @distributor, :bill_address => @bill_address, :special_instructions => @shipping_instructions) - @order2 = create(:order, :distributor => @distributor, :bill_address => @bill_address, :special_instructions => @shipping_instructions) + context "with two orders on the same day at different times" do + let(:bill_address) { create(:address) } + let(:distributor_address) { create(:address, :address1 => "distributor address", :city => 'The Shire', :zipcode => "1234") } + let(:distributor) { create(:distributor_enterprise, :address => distributor_address) } + let(:product) { create(:product) } + let(:product_distribution) { create(:product_distribution, :product => product, :distributor => distributor) } + let(:shipping_instructions) { "pick up on thursday please!" } + let(:order1) { create(:order, :distributor => distributor, :bill_address => bill_address, :special_instructions => shipping_instructions) } + let(:order2) { create(:order, :distributor => distributor, :bill_address => bill_address, :special_instructions => shipping_instructions) } + + before do + Timecop.travel(Time.zone.local(2013, 4, 25, 14, 0, 0)) { order1.finalize! } + Timecop.travel(Time.zone.local(2013, 4, 25, 16, 0, 0)) { order2.finalize! } - Timecop.travel(Time.zone.local(2013, 4, 25, 14, 0, 0)) { @order1.finalize! } - Timecop.travel(Time.zone.local(2013, 4, 25, 16, 0, 0)) { @order2.finalize! } + create(:line_item, :product => product, :order => order1) + create(:line_item, :product => product, :order => order2) + end - create(:line_item, :product => product, :order => @order1) - create(:line_item, :product => product, :order => @order2) + it "is precise to time of day, not just date" do + # When I generate a customer report with a timeframe that includes one order but not the other + login_to_admin_section + visit spree.orders_and_fulfillment_admin_reports_path - # When I generate a customer report with a timeframe that includes one order but not the other - login_to_admin_section - click_link 'Reports' - click_link 'Orders & Fulfillment Reports' + fill_in 'q_completed_at_gt', with: '2013-04-25 13:00:00' + fill_in 'q_completed_at_lt', with: '2013-04-25 15:00:00' + select 'Order Cycle Customer Totals', from: 'report_type' + click_button 'Search' - fill_in 'q_completed_at_gt', with: '2013-04-25 13:00:00' - fill_in 'q_completed_at_lt', with: '2013-04-25 15:00:00' - select 'Order Cycle Customer Totals', from: 'report_type' - click_button 'Search' + # Then I should see the rows for the first order but not the second + all('table#listing_orders tbody tr').count.should == 2 # Two rows per order + end + end - # Then I should see the rows for the first order but not the second - all('table#listing_orders tbody tr').count.should == 2 # Two rows per order + it "handles order cycles with nil opening or closing times" do + oc = create(:simple_order_cycle, name: "My Order Cycle", orders_open_at: Time.now, orders_close_at: nil) + o = create(:order, order_cycle: oc) + + login_to_admin_section + visit spree.orders_and_fulfillment_admin_reports_path + + page.should have_content "My Order Cycle" + end end describe "products and inventory report" do From 44f4ee822c9ce7e2d3727cb463d0410cf99a3aa2 Mon Sep 17 00:00:00 2001 From: Rob H Date: Wed, 11 Jun 2014 12:41:51 +1000 Subject: [PATCH 10/30] Move dynamic placeholder logic to directive so that it can be used for master variants too --- .../variant_units_controller.js.coffee | 42 ---------------- .../admin/directives/display_as.js.coffee | 48 +++++++++++++++++++ .../directives/maintain_unit_scale.js.coffee | 8 ++++ .../spree/admin/products/bulk_edit.html.haml | 10 ++-- .../unit/bulk_product_update_spec.js.coffee | 14 ++++-- 5 files changed, 71 insertions(+), 51 deletions(-) delete mode 100644 app/assets/javascripts/admin/controllers/variant_units_controller.js.coffee create mode 100644 app/assets/javascripts/admin/directives/display_as.js.coffee create mode 100644 app/assets/javascripts/admin/directives/maintain_unit_scale.js.coffee diff --git a/app/assets/javascripts/admin/controllers/variant_units_controller.js.coffee b/app/assets/javascripts/admin/controllers/variant_units_controller.js.coffee deleted file mode 100644 index 41d87113a1..0000000000 --- a/app/assets/javascripts/admin/controllers/variant_units_controller.js.coffee +++ /dev/null @@ -1,42 +0,0 @@ -angular.module("ofn.admin") - .controller "VariantUnitsCtrl", ($scope, optionValueNamer) -> - $scope.$watchCollection '[variant.unit_value_with_description, product.variant_unit_name, product.variant_unit_with_scale]', -> - [variant_unit, variant_unit_scale] = $scope.productUnitProperties() - [unit_value, unit_description] = $scope.variantUnitProperties(variant_unit_scale) - variant_object = - unit_value: unit_value - unit_description: unit_description - product: - variant_unit_scale: variant_unit_scale - variant_unit: variant_unit - variant_unit_name: $scope.product.variant_unit_name - - $scope.variant.options_text = new optionValueNamer(variant_object).name() - - $scope.productUnitProperties = -> - # get relevant product properties - if $scope.product.variant_unit_with_scale - match = $scope.product.variant_unit_with_scale.match(/^([^_]+)_([\d\.]+)$/) - if match - variant_unit = match[1] - variant_unit_scale = parseFloat(match[2]) - else - variant_unit = $scope.product.variant_unit_with_scale - variant_unit_scale = null - else - variant_unit = variant_unit_scale = null - - [variant_unit, variant_unit_scale] - - $scope.variantUnitProperties = (variant_unit_scale)-> - # get relevant variant properties - if $scope.variant.hasOwnProperty("unit_value_with_description") - match = $scope.variant.unit_value_with_description.match(/^([\d\.]+(?= |$)|)( |)(.*)$/) - if match - unit_value = parseFloat(match[1]) - unit_value = null if isNaN(unit_value) - unit_value *= variant_unit_scale if unit_value && variant_unit_scale - unit_description = match[3] - [unit_value, unit_description] - - \ No newline at end of file diff --git a/app/assets/javascripts/admin/directives/display_as.js.coffee b/app/assets/javascripts/admin/directives/display_as.js.coffee new file mode 100644 index 0000000000..a13d65bc5e --- /dev/null +++ b/app/assets/javascripts/admin/directives/display_as.js.coffee @@ -0,0 +1,48 @@ +angular.module("ofn.admin").directive "ofnDisplayAs", (optionValueNamer) -> + link: (scope, element, attrs) -> + + scope.$watchCollection -> + return [ + scope.$eval(attrs.ofnDisplayAs).unit_value_with_description + scope.product.variant_unit_name + scope.product.variant_unit_with_scale + ] + , -> + [variant_unit, variant_unit_scale] = productUnitProperties() + [unit_value, unit_description] = variantUnitProperties(variant_unit_scale) + variant_object = + unit_value: unit_value + unit_description: unit_description + product: + variant_unit_scale: variant_unit_scale + variant_unit: variant_unit + variant_unit_name: scope.product.variant_unit_name + + scope.placeholder_text = new optionValueNamer(variant_object).name() + + productUnitProperties = -> + # get relevant product properties + if scope.product.variant_unit_with_scale? + match = scope.product.variant_unit_with_scale.match(/^([^_]+)_([\d\.]+)$/) + if match + variant_unit = match[1] + variant_unit_scale = parseFloat(match[2]) + else + variant_unit = scope.product.variant_unit_with_scale + variant_unit_scale = null + else + variant_unit = variant_unit_scale = null + + [variant_unit, variant_unit_scale] + + variantUnitProperties = (variant_unit_scale)-> + # get relevant variant properties + variant = scope.$eval(attrs.ofnDisplayAs) # Like this so we can switch between 'master' and 'variant' + if variant.unit_value_with_description? + match = variant.unit_value_with_description.match(/^([\d\.]+(?= |$)|)( |)(.*)$/) + if match + unit_value = parseFloat(match[1]) + unit_value = null if isNaN(unit_value) + unit_value *= variant_unit_scale if unit_value && variant_unit_scale + unit_description = match[3] + [unit_value, unit_description] \ No newline at end of file diff --git a/app/assets/javascripts/admin/directives/maintain_unit_scale.js.coffee b/app/assets/javascripts/admin/directives/maintain_unit_scale.js.coffee new file mode 100644 index 0000000000..55306f6d77 --- /dev/null +++ b/app/assets/javascripts/admin/directives/maintain_unit_scale.js.coffee @@ -0,0 +1,8 @@ +angular.module("ofn.admin").directive "ofnMaintainUnitScale", -> + require: "ngModel" + link: (scope, element, attrs, ngModel) -> + scope.$watch 'product.variant_unit_with_scale', (newValue, oldValue) -> + if not (oldValue == newValue) + # Triggers track-variant directive to track the unit_value, so that changes to the unit are passed to the server + ngModel.$setViewValue ngModel.$viewValue + \ No newline at end of file diff --git a/app/views/spree/admin/products/bulk_edit.html.haml b/app/views/spree/admin/products/bulk_edit.html.haml index 2b636af09d..d21235cee6 100644 --- a/app/views/spree/admin/products/bulk_edit.html.haml +++ b/app/views/spree/admin/products/bulk_edit.html.haml @@ -127,10 +127,10 @@ %td.unit{ 'ng-show' => 'columns.unit.visible' } %select.select2{ 'ng-model' => 'product.variant_unit_with_scale', :name => 'variant_unit_with_scale', 'ofn-track-product' => 'variant_unit_with_scale', 'ng-options' => 'unit[1] as unit[0] for unit in variant_unit_options' } %option{'value' => '', 'ng-hide' => "hasVariants(product) && hasUnit(product)"} - %input{ 'ng-model' => 'product.master.unit_value_with_description', :name => 'master_unit_value_with_description', 'ofn-track-master' => 'unit_value_with_description', :type => 'text', :placeholder => 'value', 'ng-show' => "!hasVariants(product) && hasUnit(product)" } + %input{ 'ng-model' => 'product.master.unit_value_with_description', :name => 'master_unit_value_with_description', 'ofn-track-master' => 'unit_value_with_description', :type => 'text', :placeholder => 'value', 'ng-show' => "!hasVariants(product) && hasUnit(product)", 'ofn-maintain-unit-scale' => true } %input{ 'ng-model' => 'product.variant_unit_name', :name => 'variant_unit_name', 'ofn-track-product' => 'variant_unit_name', :placeholder => 'unit', 'ng-show' => "product.variant_unit_with_scale == 'items'", :type => 'text' } %td.display_as{ 'ng-show' => 'columns.unit.visible' } - %input{ name: 'display_as', 'ofn-track-master' => 'display_as', :type => 'text', placeholder: "{{ product.master.options_text }}", ng: { hide: 'hasVariants(product)', model: 'product.master.display_as' } } + %input{ 'ofn-display-as' => 'product.master', name: 'display_as', 'ofn-track-master' => 'display_as', type: 'text', placeholder: '{{ placeholder_text }}', ng: { hide: 'hasVariants(product)', model: 'product.master.display_as' } } %td{ 'ng-show' => 'columns.price.visible' } %input{ 'ng-model' => 'product.price', 'ofn-decimal' => :true, :name => 'price', 'ofn-track-product' => 'price', :type => 'text', 'ng-hide' => 'hasVariants(product)' } %td{ 'ng-show' => 'columns.on_hand.visible' } @@ -154,9 +154,9 @@ %td{ 'ng-show' => 'columns.name.visible' } %input{ 'ng-model' => 'variant.display_name', :name => 'variant_display_name', 'ofn-track-variant' => 'display_name', :type => 'text', placeholder: "{{ product.name }}" } %td.unit_value{ 'ng-show' => 'columns.unit.visible' } - %input{ 'ng-model' => 'variant.unit_value_with_description', :name => 'variant_unit_value_with_description', 'ofn-track-variant' => 'unit_value_with_description', :type => 'text' } - %td.display_as{ 'ng-show' => 'columns.unit.visible', 'ng-controller' => "VariantUnitsCtrl" } - %input{ 'ng-model' => 'variant.display_as', :name => 'variant_display_as', 'ofn-track-variant' => 'display_as', :type => 'text', placeholder: "{{ variant.options_text }}" } + %input{ 'ng-model' => 'variant.unit_value_with_description', :name => 'variant_unit_value_with_description', 'ofn-track-variant' => 'unit_value_with_description', :type => 'text', 'ofn-maintain-unit-scale' => true } + %td.display_as{ 'ng-show' => 'columns.unit.visible' } + %input{ 'ofn-display-as' => 'variant', 'ng-model' => 'variant.display_as', name: 'variant_display_as', 'ofn-track-variant' => 'display_as', type: 'text', placeholder: '{{ placeholder_text }}' } %td{ 'ng-show' => 'columns.price.visible' } %input{ 'ng-model' => 'variant.price', 'ofn-decimal' => :true, :name => 'variant_price', 'ofn-track-variant' => 'price', :type => 'text' } %td{ 'ng-show' => 'columns.on_hand.visible' } diff --git a/spec/javascripts/unit/bulk_product_update_spec.js.coffee b/spec/javascripts/unit/bulk_product_update_spec.js.coffee index 0e45220b4e..a1ca2d4b83 100644 --- a/spec/javascripts/unit/bulk_product_update_spec.js.coffee +++ b/spec/javascripts/unit/bulk_product_update_spec.js.coffee @@ -202,6 +202,8 @@ describe "filtering products for submission to database", -> price: 10.0 unit_value: 250 unit_description: "(bottle)" + display_as: "bottle" + display_name: "nothing" ] variant_unit: 'volume' variant_unit_scale: 1 @@ -215,15 +217,19 @@ describe "filtering products for submission to database", -> variant_unit: 'volume' variant_unit_scale: 1 variant_unit_name: 'loaf' - unit_value: 250 - unit_description: "foo" available_on: available_on + master_attributes: + id: 2 + unit_value: 250 + unit_description: "foo" variants_attributes: [ id: 1 on_hand: 2 price: 10.0 unit_value: 250 unit_description: "(bottle)" + display_as: "bottle" + display_name: "nothing" ] ] @@ -956,8 +962,8 @@ describe "AdminProductEditCtrl", -> expect(product).toEqual id: 123 variants: [ - {id: -1, price: null, unit_value: null, unit_description: null, on_demand: false, on_hand: null} - {id: -2, price: null, unit_value: null, unit_description: null, on_demand: false, on_hand: null} + {id: -1, price: null, unit_value: null, unit_description: null, on_demand: false, on_hand: null, display_as: null, display_name: null} + {id: -2, price: null, unit_value: null, unit_description: null, on_demand: false, on_hand: null, display_as: null, display_name: null} ] it "shows the variant(s)", -> From 177e03eae232c737108298d76023cf0e6ac020d8 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 11 Jun 2014 12:41:08 +1000 Subject: [PATCH 11/30] Fix payments coming up as $0, credit card charges failing for the same reason --- app/controllers/checkout_controller.rb | 17 ++++++++++++++++- .../features/consumer/shopping/checkout_spec.rb | 4 ++++ spec/support/request/shop_workflow.rb | 1 + 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/app/controllers/checkout_controller.rb b/app/controllers/checkout_controller.rb index 998f0ceeea..049280b7b0 100644 --- a/app/controllers/checkout_controller.rb +++ b/app/controllers/checkout_controller.rb @@ -15,7 +15,7 @@ class CheckoutController < Spree::CheckoutController end def update - if @order.update_attributes(params[:order]) + if @order.update_attributes(object_params) fire_event('spree.checkout.update') while @order.state != "complete" if @order.state == "payment" @@ -52,8 +52,23 @@ class CheckoutController < Spree::CheckoutController end end + private + # Copied and modified from spree. Remove check for order state, since the state machine is + # progressed all the way in one go with the one page checkout. + def object_params + # For payment step, filter order parameters to produce the expected nested attributes for a single payment and its source, discarding attributes for payment methods other than the one selected + if params[:payment_source].present? && source_params = params.delete(:payment_source)[params[:order][:payments_attributes].first[:payment_method_id].underscore] + params[:order][:payments_attributes].first[:source_attributes] = source_params + end + if (params[:order][:payments_attributes]) + params[:order][:payments_attributes].first[:amount] = @order.total + end + params[:order] + end + + def update_failed clear_ship_address respond_to do |format| diff --git a/spec/features/consumer/shopping/checkout_spec.rb b/spec/features/consumer/shopping/checkout_spec.rb index 8b0efea314..54cfd6fa61 100644 --- a/spec/features/consumer/shopping/checkout_spec.rb +++ b/spec/features/consumer/shopping/checkout_spec.rb @@ -145,6 +145,10 @@ feature "As a consumer I want to check out my cart", js: true do place_order page.should have_content "Your order has been processed successfully" + + # Order should have a payment with the correct amount + o = Spree::Order.complete.first + o.payments.first.amount.should == 10 end it "shows the payment processing failed message when submitted with an invalid credit card" do diff --git a/spec/support/request/shop_workflow.rb b/spec/support/request/shop_workflow.rb index c87d33149a..e8fd6e3b17 100644 --- a/spec/support/request/shop_workflow.rb +++ b/spec/support/request/shop_workflow.rb @@ -13,6 +13,7 @@ module ShopWorkflow def add_product_to_cart create(:line_item, variant: product.master, order: order) + order.reload.save! # Recalculate totals end def toggle_accordion(name) From d8f1153817550a1953e494f45a2c8a10344ff392 Mon Sep 17 00:00:00 2001 From: Rob H Date: Wed, 11 Jun 2014 14:21:13 +1000 Subject: [PATCH 12/30] Add js spec for option value namer --- .../option_value_namer_spec.js.coffee | 123 ++++++++++++++++++ 1 file changed, 123 insertions(+) create mode 100644 spec/javascripts/unit/admin/services/option_value_namer_spec.js.coffee diff --git a/spec/javascripts/unit/admin/services/option_value_namer_spec.js.coffee b/spec/javascripts/unit/admin/services/option_value_namer_spec.js.coffee new file mode 100644 index 0000000000..f58061ae27 --- /dev/null +++ b/spec/javascripts/unit/admin/services/option_value_namer_spec.js.coffee @@ -0,0 +1,123 @@ +describe "Option Value Namer", -> + optionValueNamer = null + + beforeEach -> + module "ofn.admin" + + beforeEach inject (_optionValueNamer_) -> + optionValueNamer = _optionValueNamer_ + + describe "generating option value name", -> + v = namer = null + beforeEach -> + v = {} + namer = new optionValueNamer(v) + + it "when description is blank", -> + v.unit_description = null + spyOn(namer, "value_scaled").andReturn true + spyOn(namer, "option_value_value_unit").andReturn ["value", "unit"] + expect(namer.name()).toBe "valueunit" + + it "when description is present", -> + v.unit_description = 'desc' + spyOn(namer, "option_value_value_unit").andReturn ["value", "unit"] + spyOn(namer, "value_scaled").andReturn true + expect(namer.name()).toBe "valueunit desc" + + it "when value is blank and description is present", -> + v.unit_description = 'desc' + spyOn(namer, "option_value_value_unit").andReturn [null, null] + spyOn(namer, "value_scaled").andReturn true + expect(namer.name()).toBe "desc" + + it "spaces value and unit when value is unscaled", -> + v.unit_description = null + spyOn(namer, "option_value_value_unit").andReturn ["value", "unit"] + spyOn(namer, "value_scaled").andReturn false + expect(namer.name()).toBe "value unit" + + describe "determining if a variant's value is scaled", -> + v = p = namer = null + + beforeEach -> + p = {} + v = { product: p } + namer = new optionValueNamer(v) + + it "returns true when the product has a scale", -> + p.variant_unit_scale = 1000 + expect(namer.value_scaled()).toBe true + + it "returns false otherwise", -> + expect(namer.value_scaled()).toBe false + + describe "generating option value's value and unit", -> + v = p = namer = null + + beforeEach -> + p = {} + v = { product: p } + namer = new optionValueNamer(v) + + it "generates simple values", -> + p.variant_unit = 'weight' + p.variant_unit_scale = 1.0 + v.unit_value = 100 + expect(namer.option_value_value_unit()).toEqual [100, 'g'] + + it "generates values when unit value is non-integer", -> + p.variant_unit = 'weight' + p.variant_unit_scale = 1.0 + v.unit_value = 123.45 + expect(namer.option_value_value_unit()).toEqual [123.45, 'g'] + + it "returns a value of 1 when unit value equals the scale", -> + p.variant_unit = 'weight' + p.variant_unit_scale = 1000.0 + v.unit_value = 1000.0 + expect(namer.option_value_value_unit()).toEqual [1, 'kg'] + + it "generates values for all weight scales", -> + for units in [[1.0, 'g'], [1000.0, 'kg'], [1000000.0, 'T']] + [scale, unit] = units + p.variant_unit = 'weight' + p.variant_unit_scale = scale + v.unit_value = 100 * scale + expect(namer.option_value_value_unit()).toEqual [100, unit] + + it "generates values for all volume scales", -> + for units in [[0.001, 'mL'], [1.0, 'L'], [1000000.0, 'ML']] + [scale, unit] = units + p.variant_unit = 'volume' + p.variant_unit_scale = scale + v.unit_value = 100 * scale + expect(namer.option_value_value_unit()).toEqual [100, unit] + + it "chooses the correct scale when value is very small", -> + p.variant_unit = 'volume' + p.variant_unit_scale = 0.001 + v.unit_value = 0.0001 + expect(namer.option_value_value_unit()).toEqual [0.1, 'mL'] + + it "generates values for item units", -> + #TODO + # %w(packet box).each do |unit| + # p = double(:product, variant_unit: 'items', variant_unit_scale: nil, variant_unit_name: unit) + # v.stub(:product) { p } + # v.stub(:unit_value) { 100 } + # subject.option_value_value_unit.should == [100, unit.pluralize] + + it "generates singular values for item units when value is 1", -> + p.variant_unit = 'items' + p.variant_unit_scale = null + p.variant_unit_name = 'packet' + v.unit_value = 1 + expect(namer.option_value_value_unit()).toEqual [1, 'packet'] + + it "returns [nil, nil] when unit value is not set", -> + p.variant_unit = 'items' + p.variant_unit_scale = null + p.variant_unit_name = 'foo' + v.unit_value = null + expect(namer.option_value_value_unit()).toEqual [null, null] \ No newline at end of file From a6e3dd65fe717f8e4e5fa196a803401a5df16f8b Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Wed, 11 Jun 2014 20:36:27 +1000 Subject: [PATCH 13/30] WIP (tests reqd): Customers and products+inventory reports handle nil order cycle times --- app/views/spree/admin/reports/customers.html.haml | 3 +-- app/views/spree/admin/reports/products_and_inventory.html.haml | 3 +-- 2 files changed, 2 insertions(+), 4 deletions(-) diff --git a/app/views/spree/admin/reports/customers.html.haml b/app/views/spree/admin/reports/customers.html.haml index 726d068010..ebe1d31492 100644 --- a/app/views/spree/admin/reports/customers.html.haml +++ b/app/views/spree/admin/reports/customers.html.haml @@ -13,9 +13,8 @@ %br = label_tag nil, "Order Cycle: " - - order_cycles_select = @order_cycles.collect {|oc| [ "#{oc.name}   (#{oc.orders_open_at.to_s(:short)} - #{oc.orders_close_at.to_s(:short)})".html_safe, oc.id ] } = select_tag(:order_cycle_id, - options_for_select(order_cycles_select, params[:order_cycle_id]), + options_for_select(report_order_cycle_options(@order_cycles), params[:order_cycle_id]), include_blank: true) %br diff --git a/app/views/spree/admin/reports/products_and_inventory.html.haml b/app/views/spree/admin/reports/products_and_inventory.html.haml index 93859d0c58..f5e0b9ce6b 100644 --- a/app/views/spree/admin/reports/products_and_inventory.html.haml +++ b/app/views/spree/admin/reports/products_and_inventory.html.haml @@ -13,9 +13,8 @@ %br = label_tag nil, "Order Cycle: " - - order_cycles_select = @order_cycles.collect {|oc| [ "#{oc.name}   (#{oc.orders_open_at.to_s(:short)} - #{oc.orders_close_at.to_s(:short)})".html_safe, oc.id ] } = select_tag(:order_cycle_id, - options_for_select(order_cycles_select, params[:order_cycle_id]), + options_for_select(report_order_cycle_options(@order_cycles), params[:order_cycle_id]), include_blank: true) %br From c09aeeee8fe42f46a70024c505cd0835f620b838 Mon Sep 17 00:00:00 2001 From: Rob H Date: Thu, 12 Jun 2014 12:11:20 +1000 Subject: [PATCH 14/30] Recalculate option values on variants when product variant unit is changed --- app/models/spree/product_decorator.rb | 2 +- app/models/spree/variant_decorator.rb | 23 +++++++++++------------ spec/features/admin/products_spec.rb | 2 ++ spec/models/spree/product_spec.rb | 24 +++++++++++++++++------- spec/models/spree/variant_spec.rb | 4 ++-- 5 files changed, 33 insertions(+), 22 deletions(-) diff --git a/app/models/spree/product_decorator.rb b/app/models/spree/product_decorator.rb index 36473d4e24..f257e55c1b 100644 --- a/app/models/spree/product_decorator.rb +++ b/app/models/spree/product_decorator.rb @@ -158,7 +158,7 @@ Spree::Product.class_eval do if variant_unit_changed? option_types.delete self.class.all_variant_unit_option_types option_types << variant_unit_option_type if variant_unit.present? - variants_including_master.each { |v| v.delete_unit_option_values } + variants_including_master.each { |v| v.update_units } end end diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index 2ffb07d4e1..25d09ae097 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -40,18 +40,6 @@ Spree::Variant.class_eval do values.to_sentence({ :words_connector => ", ", :two_words_connector => ", " }) end - def delete_unit_option_values - ovs = self.option_values.where(option_type_id: Spree::Product.all_variant_unit_option_types) - self.option_values.destroy ovs - end - - - private - - def update_weight_from_unit_value - self.weight = unit_value / 1000 if self.product.variant_unit == 'weight' && unit_value.present? - end - def update_units delete_unit_option_values @@ -63,6 +51,17 @@ Spree::Variant.class_eval do end end + private + + def update_weight_from_unit_value + self.weight = unit_value / 1000 if self.product.variant_unit == 'weight' && unit_value.present? + end + + def delete_unit_option_values + ovs = self.option_values.where(option_type_id: Spree::Product.all_variant_unit_option_types) + self.option_values.destroy ovs + end + def option_value_name if display_as.present? display_as diff --git a/spec/features/admin/products_spec.rb b/spec/features/admin/products_spec.rb index 35c6bc10cd..18aed5bf52 100644 --- a/spec/features/admin/products_spec.rb +++ b/spec/features/admin/products_spec.rb @@ -45,6 +45,8 @@ feature %q{ product.on_hand.should == 5 product.description.should == "A description..." product.group_buy.should be_false + product.master.option_values.map(&:name).should == ['5kg'] + product.master.options_text.should == "5kg" # Distributors visit spree.product_distributions_admin_product_path(product) diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index 08939532f6..3b21d425eb 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -442,24 +442,34 @@ module Spree p.update_attributes!(name: 'foo') end - it "removes the related option values from all its variants" do + it "removes the related option values from all its variants and replaces them" do ot = Spree::OptionType.find_by_name 'unit_weight' - v = create(:variant, product: p) + v = create(:variant, unit_value: 1, product: p) p.reload - expect { + v.option_values.map(&:name).include?("1L").should == false + v.option_values.map(&:name).include?("1g").should == true + expect { p.update_attributes!(variant_unit: 'volume', variant_unit_scale: 0.001) - }.to change(v.option_values(true), :count).by(-1) + }.to change(p.master.option_values(true), :count).by(0) + v.reload + v.option_values.map(&:name).include?("1L").should == true + v.option_values.map(&:name).include?("1g").should == false end - it "removes the related option values from its master variant" do + it "removes the related option values from its master variant and replaces them" do ot = Spree::OptionType.find_by_name 'unit_weight' p.master.update_attributes!(unit_value: 1) p.reload - expect { + p.master.option_values.map(&:name).include?("1L").should == false + p.master.option_values.map(&:name).include?("1g").should == true + expect { p.update_attributes!(variant_unit: 'volume', variant_unit_scale: 0.001) - }.to change(p.master.option_values(true), :count).by(-1) + }.to change(p.master.option_values(true), :count).by(0) + p.reload + p.master.option_values.map(&:name).include?("1L").should == true + p.master.option_values.map(&:name).include?("1g").should == false end end diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index 535508ebe2..f5a3dd146d 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -264,13 +264,13 @@ module Spree it "removes option value associations for unit option types" do expect { - @v.delete_unit_option_values + @v.send(:delete_unit_option_values) }.to change(@v.option_values, :count).by(-1) end it "does not delete option values" do expect { - @v.delete_unit_option_values + @v.send(:delete_unit_option_values) }.to change(Spree::OptionValue, :count).by(0) end end From e6c7acdff356d02233d5f993cb93c2d1af824548 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 12 Jun 2014 11:16:52 +1000 Subject: [PATCH 15/30] Add soft-delete method to variant --- .../spree/api/variants_controller_decorator.rb | 8 ++------ app/models/spree/variant_decorator.rb | 5 +++++ spec/models/spree/variant_spec.rb | 9 +++++++++ 3 files changed, 16 insertions(+), 6 deletions(-) diff --git a/app/controllers/spree/api/variants_controller_decorator.rb b/app/controllers/spree/api/variants_controller_decorator.rb index a225c4c401..3bc8b1c511 100644 --- a/app/controllers/spree/api/variants_controller_decorator.rb +++ b/app/controllers/spree/api/variants_controller_decorator.rb @@ -3,11 +3,7 @@ Spree::Api::VariantsController.class_eval do @variant = scope.find(params[:variant_id]) authorize! :delete, @variant - @variant.deleted_at = Time.now() - if @variant.save - respond_with(@variant, :status => 204) - else - invalid_resource!(@variant) - end + @variant.delete + respond_with @variant, status: 204 end end diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index 25d09ae097..c7aed600ea 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -51,6 +51,11 @@ Spree::Variant.class_eval do end end + def delete + self.update_column(:deleted_at, Time.now) + end + + private def update_weight_from_unit_value diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index f5a3dd146d..1c87397aec 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -276,6 +276,15 @@ module Spree end end + describe "deletion" do + it "marks the variant as deleted" do + v = create(:variant) + v.deleted_at.should be_nil + v.delete + v.deleted_at.should_not be_nil + end + end + describe "destruction" do it "destroys exchange variants" do v = create(:variant) From 9dc02e5eac7e1dfc3cc491997cdd2329387a5c5f Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 12 Jun 2014 11:20:07 +1000 Subject: [PATCH 16/30] Add product soft-delete API action --- .../api/products_controller_decorator.rb | 9 ++++++ config/routes.rb | 1 + .../spree/api/products_controller_spec.rb | 30 ++++++++++++++++--- 3 files changed, 36 insertions(+), 4 deletions(-) diff --git a/app/controllers/spree/api/products_controller_decorator.rb b/app/controllers/spree/api/products_controller_decorator.rb index 0dbecaa9fd..77c1aa6632 100644 --- a/app/controllers/spree/api/products_controller_decorator.rb +++ b/app/controllers/spree/api/products_controller_decorator.rb @@ -8,6 +8,15 @@ Spree::Api::ProductsController.class_eval do end + def soft_delete + authorize! :delete, Spree::Product + @product = find_product(params[:product_id]) + authorize! :delete, @product + @product.delete + respond_with(@product, :status => 204) + end + + private # Copied and modified from Spree::Api::BaseController to allow diff --git a/config/routes.rb b/config/routes.rb index be74e71469..7c4a02b4d1 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -120,6 +120,7 @@ Spree::Core::Engine.routes.prepend do resources :products do get :managed, on: :collection + delete :soft_delete resources :variants do delete :soft_delete diff --git a/spec/controllers/spree/api/products_controller_spec.rb b/spec/controllers/spree/api/products_controller_spec.rb index c64b95853f..eb56859365 100644 --- a/spec/controllers/spree/api/products_controller_spec.rb +++ b/spec/controllers/spree/api/products_controller_spec.rb @@ -7,9 +7,11 @@ module Spree render_views let(:supplier) { FactoryGirl.create(:supplier_enterprise) } + let(:supplier2) { FactoryGirl.create(:supplier_enterprise) } let!(:product1) { FactoryGirl.create(:product, supplier: supplier) } let!(:product2) { FactoryGirl.create(:product, supplier: supplier) } let!(:product3) { FactoryGirl.create(:product, supplier: supplier) } + let(:product_other_supplier) { FactoryGirl.create(:product, supplier: supplier2) } let(:attributes) { [:id, :name, :supplier, :price, :on_hand, :available_on, :permalink_live] } let(:unit_attributes) { [:id, :name, :group_buy_unit_size, :variant_unit] } @@ -39,6 +41,20 @@ module Spree keys = json_response.first.keys.map{ |key| key.to_sym } attributes.all?{ |attr| keys.include? attr }.should == true end + + it "soft deletes my products" do + spree_delete :soft_delete, {product_id: product1.to_param, format: :json} + response.status.should == 204 + lambda { product1.reload }.should_not raise_error + product1.deleted_at.should_not be_nil + end + + it "is denied access to soft deleting another enterprises' product" do + spree_delete :soft_delete, {product_id: product_other_supplier.to_param, format: :json} + assert_unauthorized! + lambda { product_other_supplier.reload }.should_not raise_error + product_other_supplier.deleted_at.should be_nil + end end context "as an administrator" do @@ -80,17 +96,23 @@ module Spree end it "should allow available_on to be nil" do - spree_get :index, { :template => 'bulk_index', :format => :json } json_response.size.should == 3 - product4 = FactoryGirl.create(:product) - product4.available_on = nil - product4.save! + product5 = FactoryGirl.create(:product) + product5.available_on = nil + product5.save! spree_get :index, { :template => 'bulk_index', :format => :json } json_response.size.should == 4 end + + it "soft deletes a product" do + spree_delete :soft_delete, {product_id: product1.to_param, format: :json} + response.status.should == 204 + lambda { product1.reload }.should_not raise_error + product1.deleted_at.should_not be_nil + end end end end From b970f54f5355c88ae02d0ceda7b1849a40c31ad4 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 12 Jun 2014 11:20:25 +1000 Subject: [PATCH 17/30] Admin UI soft-deletes variants, not hard delete --- .../spree/admin/variants_controller_decorator.rb | 11 +++++++++++ spec/features/admin/variants_spec.rb | 14 ++++++++++++++ 2 files changed, 25 insertions(+) diff --git a/app/controllers/spree/admin/variants_controller_decorator.rb b/app/controllers/spree/admin/variants_controller_decorator.rb index 16a37d5286..7c66f1f08a 100644 --- a/app/controllers/spree/admin/variants_controller_decorator.rb +++ b/app/controllers/spree/admin/variants_controller_decorator.rb @@ -1,6 +1,17 @@ Spree::Admin::VariantsController.class_eval do helper 'spree/products' + def destroy + @variant = Spree::Variant.find(params[:id]) + @variant.delete # This line changed, as well as removal of following conditional + flash[:success] = I18n.t('notice_messages.variant_deleted') + + respond_with(@variant) do |format| + format.html { redirect_to admin_product_variants_url(params[:product_id]) } + format.js { render_js_for_destroy } + end + end + protected diff --git a/spec/features/admin/variants_spec.rb b/spec/features/admin/variants_spec.rb index d05aa75ef8..07a8580022 100644 --- a/spec/features/admin/variants_spec.rb +++ b/spec/features/admin/variants_spec.rb @@ -83,4 +83,18 @@ feature %q{ page.should_not have_field "variant_unit_value" page.should_not have_field "variant_unit_description" end + + it "soft-deletes variants", js: true do + p = create(:simple_product) + v = create(:variant, product: p) + + login_to_admin_section + visit spree.admin_product_variants_path p + + page.find('a.delete-resource').click + page.should_not have_content v.options_text + + v.reload + v.deleted_at.should_not be_nil + end end From 88c41df201d97af28c147611a0e1621692d79921 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 12 Jun 2014 11:29:03 +1000 Subject: [PATCH 18/30] BPE uses soft-delete for products --- .../javascripts/admin/bulk_product_update.js.coffee | 2 +- spec/javascripts/unit/bulk_product_update_spec.js.coffee | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/assets/javascripts/admin/bulk_product_update.js.coffee b/app/assets/javascripts/admin/bulk_product_update.js.coffee index 93cf4b430b..ba9c1fa18d 100644 --- a/app/assets/javascripts/admin/bulk_product_update.js.coffee +++ b/app/assets/javascripts/admin/bulk_product_update.js.coffee @@ -213,7 +213,7 @@ angular.module("ofn.admin").controller "AdminProductEditCtrl", [ if confirm("Are you sure?") $http( method: "DELETE" - url: "/api/products/" + product.id + url: "/api/products/" + product.id + "/soft_delete" ).success (data) -> $scope.products.splice $scope.products.indexOf(product), 1 DirtyProducts.deleteProduct product.id diff --git a/spec/javascripts/unit/bulk_product_update_spec.js.coffee b/spec/javascripts/unit/bulk_product_update_spec.js.coffee index a1ca2d4b83..44466b5d10 100644 --- a/spec/javascripts/unit/bulk_product_update_spec.js.coffee +++ b/spec/javascripts/unit/bulk_product_update_spec.js.coffee @@ -973,7 +973,7 @@ describe "AdminProductEditCtrl", -> describe "deleting products", -> - it "deletes products with a http delete request to /api/products/id", -> + it "deletes products with a http delete request to /api/products/id/soft_delete", -> spyOn(window, "confirm").andReturn true $scope.products = [ { @@ -986,7 +986,7 @@ describe "AdminProductEditCtrl", -> } ] $scope.dirtyProducts = {} - $httpBackend.expectDELETE("/api/products/13").respond 200, "data" + $httpBackend.expectDELETE("/api/products/13/soft_delete").respond 200, "data" $scope.deleteProduct $scope.products[1] $httpBackend.flush() @@ -1005,7 +1005,7 @@ describe "AdminProductEditCtrl", -> DirtyProducts.addProductProperty 9, "someProperty", "something" DirtyProducts.addProductProperty 13, "name", "P1" - $httpBackend.expectDELETE("/api/products/13").respond 200, "data" + $httpBackend.expectDELETE("/api/products/13/soft_delete").respond 200, "data" $scope.deleteProduct $scope.products[1] $httpBackend.flush() expect($scope.products).toEqual [ @@ -1036,7 +1036,7 @@ describe "AdminProductEditCtrl", -> describe "when the variant has been saved", -> - it "deletes variants with a http delete request to /api/products/product_permalink/variants/(variant_id)", -> + it "deletes variants with a http delete request to /api/products/product_permalink/variants/(variant_id)/soft_delete", -> spyOn(window, "confirm").andReturn true $scope.products = [ { From c21d5cc3dc9f6786a770640e55df99e39567dce6 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 6 Jun 2014 12:26:30 +1000 Subject: [PATCH 19/30] Remove redirect to certified hostname - this is now performed by nginx. Fixes SSL errors. --- app/controllers/application_controller.rb | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/app/controllers/application_controller.rb b/app/controllers/application_controller.rb index 07e1029991..decc48ac7e 100644 --- a/app/controllers/application_controller.rb +++ b/app/controllers/application_controller.rb @@ -1,6 +1,5 @@ class ApplicationController < ActionController::Base protect_from_forgery - before_filter :require_certified_hostname include EnterprisesHelper @@ -17,6 +16,7 @@ class ApplicationController < ActionController::Base end end + private def require_distributor_chosen @@ -42,17 +42,6 @@ class ApplicationController < ActionController::Base end end - # There are several domains that point to the production server, but only one - # (vic.openfoodnetwork.org) that has the SSL certificate. Redirect all requests to this - # domain to avoid showing customers a scary invalid certificate error. - def require_certified_hostname - certified_host = "openfoodnetwork.org.au" - - if Rails.env.production? && request.host != certified_host - redirect_to "http://#{certified_host}#{request.fullpath}" - end - end - # All render calls within the block will be performed with the specified format # Useful for rendering html within a JSON response, particularly if the specified From 083220089ff84007fa3497521935487dfa409c25 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 6 Jun 2014 14:52:26 +1000 Subject: [PATCH 20/30] WIP: Delete ExchangeVariants when product is soft-deleted --- app/models/spree/product_decorator.rb | 7 +++++++ spec/models/spree/product_spec.rb | 22 +++++++++++++++++++++- 2 files changed, 28 insertions(+), 1 deletion(-) diff --git a/app/models/spree/product_decorator.rb b/app/models/spree/product_decorator.rb index f257e55c1b..a1cdc4b543 100644 --- a/app/models/spree/product_decorator.rb +++ b/app/models/spree/product_decorator.rb @@ -147,6 +147,13 @@ Spree::Product.class_eval do end end + def delete_with_delete_from_order_cycles + delete_without_delete_from_order_cycles + + ExchangeVariant.where('exchange_variants.variant_id IN (?)', self.variants_including_master_and_deleted).destroy_all + end + alias_method_chain :delete, :delete_from_order_cycles + private diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index 3b21d425eb..1311314e0c 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -580,7 +580,7 @@ module Spree end end - describe "Taxons" do + describe "taxons" do let(:taxon1) { create(:taxon) } let(:taxon2) { create(:taxon) } let(:product) { create(:simple_product) } @@ -589,5 +589,25 @@ module Spree product.taxons.should == [product.primary_taxon] end end + + describe "deletion" do + let(:p) { create(:simple_product) } + let(:v) { create(:variant, product: p) } + let(:oc) { create(:simple_order_cycle) } + let(:s) { create(:supplier_enterprise) } + let(:e) { create(:exchange, order_cycle: oc, incoming: true, sender: s, receiver: oc.coordinator) } + + it "removes the master variant from all order cycles" do + e.variants << p.master + p.delete + e.variants(true).should be_empty + end + + it "removes all other variants from order cycles" do + e.variants << v + p.delete + e.variants(true).should be_empty + end + end end end From 836a08606c1f2a57ce5c450b665b3cef128703ce Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 12 Jun 2014 12:00:16 +1000 Subject: [PATCH 21/30] Product and variant deletion removes the product or variant from any order cycles --- app/models/spree/product_decorator.rb | 6 ++++-- app/models/spree/variant_decorator.rb | 5 ++++- spec/models/spree/product_spec.rb | 8 ++++---- spec/models/spree/variant_spec.rb | 12 ++++++++++-- 4 files changed, 22 insertions(+), 9 deletions(-) diff --git a/app/models/spree/product_decorator.rb b/app/models/spree/product_decorator.rb index a1cdc4b543..37aeb5e3f1 100644 --- a/app/models/spree/product_decorator.rb +++ b/app/models/spree/product_decorator.rb @@ -148,9 +148,11 @@ Spree::Product.class_eval do end def delete_with_delete_from_order_cycles - delete_without_delete_from_order_cycles + transaction do + delete_without_delete_from_order_cycles - ExchangeVariant.where('exchange_variants.variant_id IN (?)', self.variants_including_master_and_deleted).destroy_all + ExchangeVariant.where('exchange_variants.variant_id IN (?)', self.variants_including_master_and_deleted).destroy_all + end end alias_method_chain :delete, :delete_from_order_cycles diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index c7aed600ea..a4f8d6d305 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -52,7 +52,10 @@ Spree::Variant.class_eval do end def delete - self.update_column(:deleted_at, Time.now) + transaction do + self.update_column(:deleted_at, Time.now) + ExchangeVariant.where(variant_id: self).destroy_all + end end diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index 1311314e0c..594dfa5d29 100644 --- a/spec/models/spree/product_spec.rb +++ b/spec/models/spree/product_spec.rb @@ -591,11 +591,11 @@ module Spree end describe "deletion" do - let(:p) { create(:simple_product) } - let(:v) { create(:variant, product: p) } + let(:p) { create(:simple_product) } + let(:v) { create(:variant, product: p) } let(:oc) { create(:simple_order_cycle) } - let(:s) { create(:supplier_enterprise) } - let(:e) { create(:exchange, order_cycle: oc, incoming: true, sender: s, receiver: oc.coordinator) } + let(:s) { create(:supplier_enterprise) } + let(:e) { create(:exchange, order_cycle: oc, incoming: true, sender: s, receiver: oc.coordinator) } it "removes the master variant from all order cycles" do e.variants << p.master diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index 1c87397aec..56797f03fd 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -277,16 +277,24 @@ module Spree end describe "deletion" do + let(:v) { create(:variant) } + let(:e) { create(:exchange, variants: [v]) } + it "marks the variant as deleted" do - v = create(:variant) v.deleted_at.should be_nil v.delete v.deleted_at.should_not be_nil end + + it "removes the variant from all order cycles" do + e + v.delete + e.variants(true).should be_empty + end end describe "destruction" do - it "destroys exchange variants" do + it "removes the variant from all order cycles" do v = create(:variant) e = create(:exchange, variants: [v]) From 0061caa8dfa86fb84a0f44156e0f3a1b6f45e4fb Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Tue, 3 Jun 2014 11:29:07 +1000 Subject: [PATCH 22/30] Do not show deleted products in order cycle admin --- .../admin/enterprises/_supplied_product.rabl | 10 ++++++++++ app/views/admin/enterprises/index.rabl | 11 +++-------- spec/views/admin/enterprises/index.rabl_spec.rb | 15 +++++++++++++++ 3 files changed, 28 insertions(+), 8 deletions(-) create mode 100644 app/views/admin/enterprises/_supplied_product.rabl create mode 100644 spec/views/admin/enterprises/index.rabl_spec.rb diff --git a/app/views/admin/enterprises/_supplied_product.rabl b/app/views/admin/enterprises/_supplied_product.rabl new file mode 100644 index 0000000000..268ff302d5 --- /dev/null +++ b/app/views/admin/enterprises/_supplied_product.rabl @@ -0,0 +1,10 @@ +object @product + +attributes :name +node(:supplier_name) { |p| p.supplier.andand.name } +node(:image_url) { |p| p.images.present? ? p.images.first.attachment.url(:mini) : nil } +node(:master_id) { |p| p.master.id } +child variants: :variants do |variant| + attributes :id + node(:label) { |v| v.options_text } +end diff --git a/app/views/admin/enterprises/index.rabl b/app/views/admin/enterprises/index.rabl index 9e1ee893d1..1342d6eb1a 100644 --- a/app/views/admin/enterprises/index.rabl +++ b/app/views/admin/enterprises/index.rabl @@ -2,13 +2,8 @@ collection @collection attributes :id, :name -child supplied_products: :supplied_products do |product| - attributes :name - node(:supplier_name) { |p| p.supplier.andand.name } - node(:image_url) { |p| p.images.present? ? p.images.first.attachment.url(:mini) : nil } - node(:master_id) { |p| p.master.id } - child variants: :variants do |variant| - attributes :id - node(:label) { |v| v.options_text } +node(:supplied_products) do |enterprise| + enterprise.supplied_products.not_deleted.map do |product| + partial 'admin/enterprises/supplied_product', object: product end end diff --git a/spec/views/admin/enterprises/index.rabl_spec.rb b/spec/views/admin/enterprises/index.rabl_spec.rb new file mode 100644 index 0000000000..e92388da61 --- /dev/null +++ b/spec/views/admin/enterprises/index.rabl_spec.rb @@ -0,0 +1,15 @@ +require 'spec_helper' + +describe "admin/enterprises/index.rabl" do + let(:enterprise) { create(:distributor_enterprise) } + let!(:product) { create(:simple_product, supplier: enterprise) } + let!(:deleted_product) { create(:simple_product, supplier: enterprise, deleted_at: Time.now) } + let(:render) { Rabl.render([enterprise], 'admin/enterprises/index', view_path: 'app/views', scope: RablHelper::FakeContext.instance) } + + describe "supplied products" do + it "does not render deleted products" do + render.should have_json_size(1).at_path '0/supplied_products' + render.should be_json_eql(product.master.id).at_path '0/supplied_products/0/master_id' + end + end +end From b23430375ca736c3eacc76aa2e100a668f1b7e0e Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 12 Jun 2014 12:15:37 +1000 Subject: [PATCH 23/30] Add migration to remove deleted variants from order cycles --- ...020206_remove_deleted_variants_from_order_cycles.rb | 10 ++++++++++ db/schema.rb | 4 ++-- 2 files changed, 12 insertions(+), 2 deletions(-) create mode 100644 db/migrate/20140612020206_remove_deleted_variants_from_order_cycles.rb diff --git a/db/migrate/20140612020206_remove_deleted_variants_from_order_cycles.rb b/db/migrate/20140612020206_remove_deleted_variants_from_order_cycles.rb new file mode 100644 index 0000000000..cb03abe09a --- /dev/null +++ b/db/migrate/20140612020206_remove_deleted_variants_from_order_cycles.rb @@ -0,0 +1,10 @@ +class RemoveDeletedVariantsFromOrderCycles < ActiveRecord::Migration + def up + evs = ExchangeVariant.joins(:variant).where('spree_variants.deleted_at IS NOT NULL') + say "Removing #{evs.count} deleted variants from order cycles..." + evs.destroy_all + end + + def down + end +end diff --git a/db/schema.rb b/db/schema.rb index 2b3c2cbf62..66205112e0 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -11,7 +11,7 @@ # # It's strongly recommended to check this file into your version control system. -ActiveRecord::Schema.define(:version => 20140604051248) do +ActiveRecord::Schema.define(:version => 20140612020206) do create_table "adjustment_metadata", :force => true do |t| t.integer "adjustment_id" @@ -547,9 +547,9 @@ ActiveRecord::Schema.define(:version => 20140604051248) do t.string "email" t.text "special_instructions" t.integer "distributor_id" + t.integer "order_cycle_id" t.string "currency" t.string "last_ip_address" - t.integer "order_cycle_id" t.integer "cart_id" end From 8cea6d53edfd3272960dcb03c92365cd29673784 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 12 Jun 2014 12:15:51 +1000 Subject: [PATCH 24/30] Correctly designate primary_taxon_id as NOT NULL --- db/schema.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/db/schema.rb b/db/schema.rb index 66205112e0..55246ee838 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -681,7 +681,7 @@ ActiveRecord::Schema.define(:version => 20140612020206) do t.float "variant_unit_scale" t.string "variant_unit_name" t.text "notes" - t.integer "primary_taxon_id" + t.integer "primary_taxon_id", :null => false end add_index "spree_products", ["available_on"], :name => "index_products_on_available_on" From 9ccdd3b4e0a3919e9e4f16595682868012cca631 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 12 Jun 2014 14:55:57 +1000 Subject: [PATCH 25/30] Add reactive integrity test that soft-deleted variants are removed from order cycles --- Gemfile | 2 ++ Gemfile.lock | 5 +++++ config/schedule.rb | 12 +++++++++++ lib/open_food_network/integrity_checker.rb | 23 ++++++++++++++++++++++ 4 files changed, 42 insertions(+) create mode 100644 config/schedule.rb create mode 100644 lib/open_food_network/integrity_checker.rb diff --git a/Gemfile b/Gemfile index 87566eebfa..e958a9389e 100644 --- a/Gemfile +++ b/Gemfile @@ -40,6 +40,8 @@ gem 'custom_error_message', :github => 'jeremydurham/custom-err-msg' gem 'foreigner' gem 'immigrant' +gem 'whenever', require: false + # Gems used only for assets and not required # in production environments by default. group :assets do diff --git a/Gemfile.lock b/Gemfile.lock index 733ef43572..dd09513503 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -175,6 +175,7 @@ GEM xpath (~> 2.0) celluloid (0.15.2) timers (~> 1.1.0) + chronic (0.10.2) chunky_png (1.3.0) climate_control (0.0.3) activesupport (>= 3.0) @@ -479,6 +480,9 @@ GEM addressable (>= 2.2.7) crack (>= 0.3.2) websocket-driver (0.3.2) + whenever (0.9.2) + activesupport (>= 2.3.4) + chronic (>= 0.6.3) xpath (2.0.0) nokogiri (~> 1.3) zeus (0.13.3) @@ -552,3 +556,4 @@ DEPENDENCIES unicorn unicorn-rails webmock + whenever diff --git a/config/schedule.rb b/config/schedule.rb new file mode 100644 index 0000000000..36cec1f172 --- /dev/null +++ b/config/schedule.rb @@ -0,0 +1,12 @@ +require 'whenever' + +# Learn more: http://github.com/javan/whenever + +env "MAILTO", "rohan@rohanmitchell.com" + +# If we use -e with a file containing specs, rspec interprets it and filters out our examples +job_type :run_file, "cd :path; :environment_variable=:environment bundle exec script/rails runner :task :output" + +every 1.day, at: '12:05am' do + run_file "lib/open_food_network/integrity_checker.rb" +end diff --git a/lib/open_food_network/integrity_checker.rb b/lib/open_food_network/integrity_checker.rb new file mode 100644 index 0000000000..ff34f661dc --- /dev/null +++ b/lib/open_food_network/integrity_checker.rb @@ -0,0 +1,23 @@ +require 'rspec/rails' +require 'rspec/autorun' + +# This spec file is one part of a two-part strategy to maintain data integrity. The first part +# is to proactively protect data integrity using database constraints (not null, foreign keys, +# etc) and ActiveRecord validations. As a backup to those two techniques, and particularly in +# the cases where it's not possible to model an integrity concern with database constraints, +# we can add a reactive integrity test here. + +# These tests are run nightly and the results are emailed to the MAILTO address in +# config/schedule.rb if any failures occur. + +# Ref: http://pluralsight.com/training/Courses/TableOfContents/database-your-friend + + +describe "data integrity" do + it "has no deleted variants in order cycles" do + # When a variant is soft deleted, it should be removed from all order cycles + # via Spree::Product#delete or Spree::Variant#delete. + evs = ExchangeVariant.joins(:variant).where('spree_variants.deleted_at IS NOT NULL') + evs.count.should == 0 + end +end From 98611c3672cfaa27fb0500e18e28f852e246948e Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 12 Jun 2014 15:21:35 +1000 Subject: [PATCH 26/30] Fix timing error in enterprise supplied products rabl spec --- spec/views/admin/enterprises/index.rabl_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/views/admin/enterprises/index.rabl_spec.rb b/spec/views/admin/enterprises/index.rabl_spec.rb index e92388da61..91463c2212 100644 --- a/spec/views/admin/enterprises/index.rabl_spec.rb +++ b/spec/views/admin/enterprises/index.rabl_spec.rb @@ -3,7 +3,7 @@ require 'spec_helper' describe "admin/enterprises/index.rabl" do let(:enterprise) { create(:distributor_enterprise) } let!(:product) { create(:simple_product, supplier: enterprise) } - let!(:deleted_product) { create(:simple_product, supplier: enterprise, deleted_at: Time.now) } + let!(:deleted_product) { create(:simple_product, supplier: enterprise, deleted_at: 1.day.ago) } let(:render) { Rabl.render([enterprise], 'admin/enterprises/index', view_path: 'app/views', scope: RablHelper::FakeContext.instance) } describe "supplied products" do From f6690cb8ddf3a77fb9ef82c8d6a4dbfae639530b Mon Sep 17 00:00:00 2001 From: Will Marshall Date: Tue, 10 Jun 2014 15:21:41 +1000 Subject: [PATCH 27/30] Patching a minor bug in cart emptying --- .../darkswarm/directives/empties_cart.js.coffee | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/darkswarm/directives/empties_cart.js.coffee b/app/assets/javascripts/darkswarm/directives/empties_cart.js.coffee index 0b29180b57..4691d9b830 100644 --- a/app/assets/javascripts/darkswarm/directives/empties_cart.js.coffee +++ b/app/assets/javascripts/darkswarm/directives/empties_cart.js.coffee @@ -5,12 +5,13 @@ Darkswarm.directive "ofnEmptiesCart", (CurrentHub, CurrentOrder, Navigation, sto template: "{{action}} {{hub.name}}" link: (scope, elm, attr)-> # A hub is selected, we're changing to a different hub, and the cart isn't empty - if CurrentHub.id and CurrentHub.id isnt scope.hub.id and not CurrentOrder.empty() + if CurrentHub.id and CurrentHub.id isnt scope.hub.id scope.action = attr.change - elm.bind 'click', (ev)-> - ev.preventDefault() - if confirm "Are you sure? This will change your selected Hub and remove any items in you shopping cart." - storage.clearAll() # One day this will have to be moar GRANULAR - Navigation.go scope.hub.path + unless CurrentOrder.empty() + elm.bind 'click', (ev)-> + ev.preventDefault() + if confirm "Are you sure? This will change your selected Hub and remove any items in you shopping cart." + storage.clearAll() # One day this will have to be moar GRANULAR + Navigation.go scope.hub.path else scope.action = attr.shop From 3aa9501480a7e1d7a6d1331f5f426d6784151066 Mon Sep 17 00:00:00 2001 From: Will Marshall Date: Fri, 13 Jun 2014 14:13:16 +1000 Subject: [PATCH 28/30] Adding main app --- app/views/json/_producer.rabl | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/views/json/_producer.rabl b/app/views/json/_producer.rabl index 5c76befec1..467011bb7a 100644 --- a/app/views/json/_producer.rabl +++ b/app/views/json/_producer.rabl @@ -8,7 +8,7 @@ node :logo do |producer| end node :path do |producer| - producer_path(producer) + main_app.producer_path(producer) end node :hash do |producer| From d6098ec2de7b77d49f40b38d7454fad5b9b3228b Mon Sep 17 00:00:00 2001 From: Will Marshall Date: Fri, 13 Jun 2014 14:23:44 +1000 Subject: [PATCH 29/30] Fixing a private method --- app/models/spree/variant_decorator.rb | 5 ----- 1 file changed, 5 deletions(-) diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index 8c0b2acf81..9410c14902 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -79,11 +79,6 @@ Spree::Variant.class_eval do self.weight = unit_value / 1000 if self.product.variant_unit == 'weight' && unit_value.present? end - def delete_unit_option_values - ovs = self.option_values.where(option_type_id: Spree::Product.all_variant_unit_option_types) - self.option_values.destroy ovs - end - def option_value_name if display_as.present? display_as From 17682dbc580926438b8d9a0f9d30fdda1c520581 Mon Sep 17 00:00:00 2001 From: Will Marshall Date: Fri, 13 Jun 2014 14:31:55 +1000 Subject: [PATCH 30/30] fixing issue 750 --- .../checkout/payment_controller.js.coffee | 16 +++++++++++++++- .../spree/checkout/payment/_gateway.html.haml | 2 +- 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/app/assets/javascripts/darkswarm/controllers/checkout/payment_controller.js.coffee b/app/assets/javascripts/darkswarm/controllers/checkout/payment_controller.js.coffee index 33a06e5d1f..9ae7c14b90 100644 --- a/app/assets/javascripts/darkswarm/controllers/checkout/payment_controller.js.coffee +++ b/app/assets/javascripts/darkswarm/controllers/checkout/payment_controller.js.coffee @@ -2,7 +2,21 @@ Darkswarm.controller "PaymentCtrl", ($scope, $timeout) -> angular.extend(this, new FieldsetMixin($scope)) $scope.name = "payment" - $scope.months = {1: "January", 2: "February", 3: "March", 4: "April", 5: "May", 6: "June", 7: "July", 8: "August", 9: "September", 10: "October", 11: "November", 12: "December"} + $scope.months = [ + {key: "January", value: "1"}, + {key: "February", value: "2"}, + {key: "March", value: "3"}, + {key: "April", value: "4"}, + {key: "May", value: "5"}, + {key: "June", value: "6"}, + {key: "July", value: "7"}, + {key: "August", value: "8"}, + {key: "September", value: "9"}, + {key: "October", value: "10"}, + {key: "November", value: "11"}, + {key: "December", value: "12"}, + ] + $scope.years = [moment().year()..(moment().year()+15)] $scope.secrets.card_month = "1" $scope.secrets.card_year = moment().year() diff --git a/app/views/spree/checkout/payment/_gateway.html.haml b/app/views/spree/checkout/payment/_gateway.html.haml index 66b189f39b..0b801cb420 100644 --- a/app/views/spree/checkout/payment/_gateway.html.haml +++ b/app/views/spree/checkout/payment/_gateway.html.haml @@ -20,6 +20,6 @@ .row .small-6.columns - %select{"ng-model" => "secrets.card_month", "ng-options" => "number as name for (number, name) in months", name: "secrets.card_month", required: true} + %select{"ng-model" => "secrets.card_month", "ng-options" => "currMonth.value as currMonth.key for currMonth in months", name: "secrets.card_month", required: true} .small-6.columns %select{"ng-model" => "secrets.card_year", "ng-options" => "year for year in years", name: "secrets.card_year", required: true}