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/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/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/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 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/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/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/product_decorator.rb b/app/models/spree/product_decorator.rb index 36473d4e24..37aeb5e3f1 100644 --- a/app/models/spree/product_decorator.rb +++ b/app/models/spree/product_decorator.rb @@ -147,6 +147,15 @@ Spree::Product.class_eval do end end + def delete_with_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 + end + end + alias_method_chain :delete, :delete_from_order_cycles + private @@ -158,7 +167,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 f2b641b680..9410c14902 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -54,12 +54,6 @@ Spree::Variant.class_eval do 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 @@ -71,6 +65,20 @@ Spree::Variant.class_eval do end end + def delete + transaction do + self.update_column(:deleted_at, Time.now) + ExchangeVariant.where(variant_id: self).destroy_all + 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 option_value_name if display_as.present? display_as 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/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| 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} diff --git a/config/routes.rb b/config/routes.rb index 250bed1a5b..0622acb050 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -119,6 +119,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/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/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/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 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 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/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 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 = [ { diff --git a/spec/models/spree/product_spec.rb b/spec/models/spree/product_spec.rb index 47b7fa165e..594dfa5d29 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 @@ -570,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) } @@ -580,5 +590,24 @@ module Spree 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 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..91463c2212 --- /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: 1.day.ago) } + 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