From 76f05d13d734a079f2551774012f89a2f7556878 Mon Sep 17 00:00:00 2001 From: Rob H Date: Thu, 24 Apr 2014 10:50:35 +1000 Subject: [PATCH 01/22] Slow specs on CI --- spec/features/admin/bulk_order_management_spec.rb | 2 +- spec/features/admin/cms_spec.rb | 8 ++++++++ 2 files changed, 9 insertions(+), 1 deletion(-) diff --git a/spec/features/admin/bulk_order_management_spec.rb b/spec/features/admin/bulk_order_management_spec.rb index 1a227bc6bd..a56f669756 100644 --- a/spec/features/admin/bulk_order_management_spec.rb +++ b/spec/features/admin/bulk_order_management_spec.rb @@ -9,7 +9,7 @@ feature %q{ before :all do @default_wait_time = Capybara.default_wait_time - Capybara.default_wait_time = 5 + Capybara.default_wait_time = 10 end after :all do diff --git a/spec/features/admin/cms_spec.rb b/spec/features/admin/cms_spec.rb index f032994a9d..27fc57582a 100644 --- a/spec/features/admin/cms_spec.rb +++ b/spec/features/admin/cms_spec.rb @@ -8,6 +8,14 @@ feature %q{ include AuthenticationWorkflow include WebHelper + before :all do + @default_wait_time = Capybara.default_wait_time + Capybara.default_wait_time = 5 + end + + after :all do + Capybara.default_wait_time = @default_wait_time + end scenario "admin can access CMS admin and return to Spree admin" do login_to_admin_section From 2e51518b5b78e7223503ab43fc59cd1231029f38 Mon Sep 17 00:00:00 2001 From: Rob H Date: Thu, 24 Apr 2014 11:28:53 +1000 Subject: [PATCH 02/22] CMS spec being weird --- spec/features/admin/cms_spec.rb | 11 +---------- 1 file changed, 1 insertion(+), 10 deletions(-) diff --git a/spec/features/admin/cms_spec.rb b/spec/features/admin/cms_spec.rb index 27fc57582a..a01ce78101 100644 --- a/spec/features/admin/cms_spec.rb +++ b/spec/features/admin/cms_spec.rb @@ -8,15 +8,6 @@ feature %q{ include AuthenticationWorkflow include WebHelper - before :all do - @default_wait_time = Capybara.default_wait_time - Capybara.default_wait_time = 5 - end - - after :all do - Capybara.default_wait_time = @default_wait_time - end - scenario "admin can access CMS admin and return to Spree admin" do login_to_admin_section click_link 'Configuration' @@ -24,7 +15,7 @@ feature %q{ page.should have_content "ComfortableMexicanSofa" click_link 'Spree Admin' - current_path.should == spree.admin_orders_path + current_path.should match(/^\/admin/) end scenario "anonymous user can't access CMS admin" do From f7658ad250f2a4fd005a8675896ae5ce6a189e35 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 3 Apr 2014 15:32:41 +1100 Subject: [PATCH 03/22] Add foreign keys, sanitising data first --- Gemfile | 3 + Gemfile.lock | 7 + db/migrate/20140402033428_add_foreign_keys.rb | 178 ++++++++++++++++++ db/schema.rb | 149 ++++++++++++++- 4 files changed, 336 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20140402033428_add_foreign_keys.rb diff --git a/Gemfile b/Gemfile index dec618a871..eb95c1b209 100644 --- a/Gemfile +++ b/Gemfile @@ -37,6 +37,9 @@ gem 'gmaps4rails' gem 'spinjs-rails' gem 'rack-ssl', :require => 'rack/ssl' +gem 'foreigner' +gem 'immigrant' + # 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 971e536da5..382ffcc1e2 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -261,6 +261,8 @@ GEM net-ssh (>= 2.1.3) nokogiri (~> 1.5) ruby-hmac + foreigner (1.6.1) + activerecord (>= 3.0.0) formatador (0.2.4) foundation-icons-sass-rails (3.0.0) railties (>= 3.1.1) @@ -298,6 +300,9 @@ GEM multi_json (~> 1.0) multi_xml (>= 0.5.2) i18n (0.6.9) + immigrant (0.1.6) + activerecord (>= 3.0) + foreigner (>= 1.2.1) journey (1.0.4) jquery-rails (2.2.2) railties (>= 3.0, < 5.0) @@ -512,6 +517,7 @@ DEPENDENCIES eaterprises_feature! factory_girl_rails faker + foreigner foundation-icons-sass-rails foundation-rails foundation_rails_helper! @@ -523,6 +529,7 @@ DEPENDENCIES guard-rspec guard-zeus haml + immigrant jquery-rails json_spec letter_opener diff --git a/db/migrate/20140402033428_add_foreign_keys.rb b/db/migrate/20140402033428_add_foreign_keys.rb new file mode 100644 index 0000000000..9f4349d485 --- /dev/null +++ b/db/migrate/20140402033428_add_foreign_keys.rb @@ -0,0 +1,178 @@ +class AddForeignKeys < ActiveRecord::Migration + class AdjustmentMetadata < ActiveRecord::Base; end + class ExchangeVariant < ActiveRecord::Base; end + class Spree::InventoryUnit < ActiveRecord::Base; end + class Spree::LineItem < ActiveRecord::Base; end + class Spree::Address < ActiveRecord::Base; end + class Spree::Order < ActiveRecord::Base; end + class Spree::Taxon < ActiveRecord::Base; end + class CoordinatorFee < ActiveRecord::Base; end + + def change + setup_foreign_keys + end + + # http://stackoverflow.com/a/7679513/2720566 + def migrate(direction) + sanitise_data if direction == :up + super + end + + + private + + def sanitise_data + # Remove orphaned AdjustmentMetadata records + orphaned_adjustment_metadata = AdjustmentMetadata.joins('LEFT OUTER JOIN spree_adjustments ON spree_adjustments.id = adjustment_metadata.adjustment_id').where('spree_adjustments.id IS NULL') + say "Destroying #{orphaned_adjustment_metadata.count} orphaned AdjustmentMetadata (of total #{AdjustmentMetadata.count})" + orphaned_adjustment_metadata.destroy_all + + # Remove orphaned ExchangeVariants + orphaned_exchange_variants = ExchangeVariant.joins('LEFT OUTER JOIN spree_variants ON spree_variants.id=exchange_variants.variant_id').where('spree_variants.id IS NULL') + say "Destroying #{orphaned_exchange_variants.count} orphaned ExchangeVariants (of total #{ExchangeVariant.count})" + orphaned_exchange_variants.destroy_all + + # Remove orphaned Spree::InventoryUnits + orphaned_inventory_units = Spree::InventoryUnit.joins('LEFT OUTER JOIN spree_variants ON spree_variants.id=spree_inventory_units.variant_id').where('spree_variants.id IS NULL') + say "Destroying #{orphaned_inventory_units.count} orphaned InventoryUnits (of total #{Spree::InventoryUnit.count})" + orphaned_inventory_units.destroy_all + # TODO: Need a dependent destroy for variant -> inventory unit? + + # Remove orphaned Spree::LineItems + orphaned_line_items = Spree::LineItem. + joins('LEFT OUTER JOIN spree_variants ON spree_variants.id=spree_line_items.variant_id'). + joins('LEFT OUTER JOIN spree_orders ON spree_orders.id=spree_line_items.order_id'). + where('spree_variants.id IS NULL OR spree_orders.id IS NULL') + say "Destroying #{orphaned_line_items.count} orphaned LineItems (of total #{Spree::LineItem.count})" + orphaned_line_items.each { |li| li.delete } + + # Give all orders a distributor + address = Spree::Address.create!(firstname: 'Dummy distributor', lastname: 'Dummy distributor', phone: '12345678', state: Spree::State.first, + address1: 'Dummy distributor', city: 'Dummy distributor', zipcode: '1234', country: Spree::State.first.country) + no_distributor = Enterprise.create!(name: "No distributor", address: address) + deleted_distributor = Enterprise.create!(name: "Deleted distributor", address: address) + + orders = Spree::Order.where(distributor_id: nil) + say "Assigning a dummy distributor to #{orders.count} orders which lack one (of total #{Spree::Order.count})" + orders.update_all distributor_id: no_distributor.id + + orphaned_orders = Spree::Order.joins('LEFT OUTER JOIN enterprises ON enterprises.id=spree_orders.distributor_id').where('enterprises.id IS NULL') + say "Assigning a dummy distributor to #{orphaned_orders.count} orders with a deleted distributor (of total #{Spree::Order.count})" + orphaned_orders.update_all distributor_id: deleted_distributor.id + + # Remove orphaned Spree::Taxons + orphaned_taxons = Spree::Taxon.joins('LEFT OUTER JOIN spree_taxonomies ON spree_taxonomies.id=spree_taxons.taxonomy_id').where('spree_taxonomies.id IS NULL') + say "Destroying #{orphaned_taxons.count} orphaned Taxons (of total #{Spree::Taxon.count})" + orphaned_taxons.destroy_all + + # Remove orphaned CoordinatorFee records + orphaned_coordinator_fees = CoordinatorFee.joins('LEFT OUTER JOIN enterprise_fees ON enterprise_fees.id = coordinator_fees.enterprise_fee_id').where('enterprise_fees.id IS NULL') + say "Destroying #{orphaned_coordinator_fees.count} orphaned CoordinatorFees (of total #{CoordinatorFee.count})" + orphaned_coordinator_fees.each do |cf| + CoordinatorFee.connection.execute("DELETE FROM coordinator_fees WHERE coordinator_fees.order_cycle_id=#{cf.order_cycle_id} AND coordinator_fees.enterprise_fee_id=#{cf.enterprise_fee_id}") + end + end + + + def setup_foreign_keys + add_foreign_key "adjustment_metadata", "spree_adjustments", name: "adjustment_metadata_adjustment_id_fk", column: "adjustment_id" + add_foreign_key "adjustment_metadata", "enterprises", name: "adjustment_metadata_enterprise_id_fk" + add_foreign_key "carts", "spree_users", name: "carts_user_id_fk", column: "user_id" + add_foreign_key "cms_blocks", "cms_pages", name: "cms_blocks_page_id_fk", column: "page_id" + add_foreign_key "cms_categories", "cms_sites", name: "cms_categories_site_id_fk", column: "site_id", dependent: :delete + add_foreign_key "cms_categorizations", "cms_categories", name: "cms_categorizations_category_id_fk", column: "category_id" + add_foreign_key "cms_files", "cms_blocks", name: "cms_files_block_id_fk", column: "block_id" + add_foreign_key "cms_files", "cms_sites", name: "cms_files_site_id_fk", column: "site_id" + add_foreign_key "cms_layouts", "cms_layouts", name: "cms_layouts_parent_id_fk", column: "parent_id" + add_foreign_key "cms_layouts", "cms_sites", name: "cms_layouts_site_id_fk", column: "site_id", dependent: :delete + add_foreign_key "cms_pages", "cms_layouts", name: "cms_pages_layout_id_fk", column: "layout_id" + add_foreign_key "cms_pages", "cms_pages", name: "cms_pages_parent_id_fk", column: "parent_id" + add_foreign_key "cms_pages", "cms_sites", name: "cms_pages_site_id_fk", column: "site_id", dependent: :delete + add_foreign_key "cms_pages", "cms_pages", name: "cms_pages_target_page_id_fk", column: "target_page_id" + add_foreign_key "cms_snippets", "cms_sites", name: "cms_snippets_site_id_fk", column: "site_id", dependent: :delete + add_foreign_key "coordinator_fees", "enterprise_fees", name: "coordinator_fees_enterprise_fee_id_fk" + add_foreign_key "coordinator_fees", "order_cycles", name: "coordinator_fees_order_cycle_id_fk" + add_foreign_key "distributors_payment_methods", "enterprises", name: "distributors_payment_methods_distributor_id_fk", column: "distributor_id" + add_foreign_key "distributors_payment_methods", "spree_payment_methods", name: "distributors_payment_methods_payment_method_id_fk", column: "payment_method_id" + add_foreign_key "distributors_shipping_methods", "enterprises", name: "distributors_shipping_methods_distributor_id_fk", column: "distributor_id" + add_foreign_key "distributors_shipping_methods", "spree_shipping_methods", name: "distributors_shipping_methods_shipping_method_id_fk", column: "shipping_method_id" + add_foreign_key "enterprise_fees", "enterprises", name: "enterprise_fees_enterprise_id_fk" + add_foreign_key "enterprise_groups_enterprises", "enterprise_groups", name: "enterprise_groups_enterprises_enterprise_group_id_fk" + add_foreign_key "enterprise_groups_enterprises", "enterprises", name: "enterprise_groups_enterprises_enterprise_id_fk" + add_foreign_key "enterprise_roles", "enterprises", name: "enterprise_roles_enterprise_id_fk" + add_foreign_key "enterprise_roles", "spree_users", name: "enterprise_roles_user_id_fk", column: "user_id" + add_foreign_key "enterprises", "spree_addresses", name: "enterprises_address_id_fk", column: "address_id" + add_foreign_key "exchange_fees", "enterprise_fees", name: "exchange_fees_enterprise_fee_id_fk" + add_foreign_key "exchange_fees", "exchanges", name: "exchange_fees_exchange_id_fk" + add_foreign_key "exchange_variants", "exchanges", name: "exchange_variants_exchange_id_fk" + add_foreign_key "exchange_variants", "spree_variants", name: "exchange_variants_variant_id_fk", column: "variant_id" + add_foreign_key "exchanges", "order_cycles", name: "exchanges_order_cycle_id_fk" + add_foreign_key "exchanges", "enterprises", name: "exchanges_payment_enterprise_id_fk", column: "payment_enterprise_id" + add_foreign_key "exchanges", "enterprises", name: "exchanges_receiver_id_fk", column: "receiver_id" + add_foreign_key "exchanges", "enterprises", name: "exchanges_sender_id_fk", column: "sender_id" + add_foreign_key "order_cycles", "enterprises", name: "order_cycles_coordinator_id_fk", column: "coordinator_id" + add_foreign_key "product_distributions", "enterprises", name: "product_distributions_distributor_id_fk", column: "distributor_id" + add_foreign_key "product_distributions", "enterprise_fees", name: "product_distributions_enterprise_fee_id_fk" + add_foreign_key "product_distributions", "spree_products", name: "product_distributions_product_id_fk", column: "product_id" + add_foreign_key "spree_addresses", "spree_countries", name: "spree_addresses_country_id_fk", column: "country_id" + add_foreign_key "spree_addresses", "spree_states", name: "spree_addresses_state_id_fk", column: "state_id" + add_foreign_key "spree_inventory_units", "spree_orders", name: "spree_inventory_units_order_id_fk", column: "order_id" + add_foreign_key "spree_inventory_units", "spree_return_authorizations", name: "spree_inventory_units_return_authorization_id_fk", column: "return_authorization_id" + add_foreign_key "spree_inventory_units", "spree_shipments", name: "spree_inventory_units_shipment_id_fk", column: "shipment_id" + add_foreign_key "spree_inventory_units", "spree_variants", name: "spree_inventory_units_variant_id_fk", column: "variant_id" + add_foreign_key "spree_line_items", "spree_orders", name: "spree_line_items_order_id_fk", column: "order_id" + add_foreign_key "spree_line_items", "spree_variants", name: "spree_line_items_variant_id_fk", column: "variant_id" + add_foreign_key "spree_option_types_prototypes", "spree_option_types", name: "spree_option_types_prototypes_option_type_id_fk", column: "option_type_id" + add_foreign_key "spree_option_types_prototypes", "spree_prototypes", name: "spree_option_types_prototypes_prototype_id_fk", column: "prototype_id" + add_foreign_key "spree_option_values", "spree_option_types", name: "spree_option_values_option_type_id_fk", column: "option_type_id" + add_foreign_key "spree_option_values_variants", "spree_option_values", name: "spree_option_values_variants_option_value_id_fk", column: "option_value_id" + add_foreign_key "spree_option_values_variants", "spree_variants", name: "spree_option_values_variants_variant_id_fk", column: "variant_id" + add_foreign_key "spree_orders", "spree_addresses", name: "spree_orders_bill_address_id_fk", column: "bill_address_id" + add_foreign_key "spree_orders", "carts", name: "spree_orders_cart_id_fk" + add_foreign_key "spree_orders", "enterprises", name: "spree_orders_distributor_id_fk", column: "distributor_id" + add_foreign_key "spree_orders", "order_cycles", name: "spree_orders_order_cycle_id_fk" + add_foreign_key "spree_orders", "spree_addresses", name: "spree_orders_ship_address_id_fk", column: "ship_address_id" + #add_foreign_key "spree_orders", "spree_shipping_methods", name: "spree_orders_shipping_method_id_fk", column: "shipping_method_id" + add_foreign_key "spree_orders", "spree_users", name: "spree_orders_user_id_fk", column: "user_id" + add_foreign_key "spree_payments", "spree_orders", name: "spree_payments_order_id_fk", column: "order_id" + add_foreign_key "spree_payments", "spree_payment_methods", name: "spree_payments_payment_method_id_fk", column: "payment_method_id" + #add_foreign_key "spree_payments", "spree_payments", name: "spree_payments_source_id_fk", column: "source_id" + add_foreign_key "spree_prices", "spree_variants", name: "spree_prices_variant_id_fk", column: "variant_id" + add_foreign_key "spree_product_option_types", "spree_option_types", name: "spree_product_option_types_option_type_id_fk", column: "option_type_id" + add_foreign_key "spree_product_option_types", "spree_products", name: "spree_product_option_types_product_id_fk", column: "product_id" + add_foreign_key "spree_product_properties", "spree_products", name: "spree_product_properties_product_id_fk", column: "product_id" + add_foreign_key "spree_product_properties", "spree_properties", name: "spree_product_properties_property_id_fk", column: "property_id" + add_foreign_key "spree_products_promotion_rules", "spree_products", name: "spree_products_promotion_rules_product_id_fk", column: "product_id" + add_foreign_key "spree_products_promotion_rules", "spree_promotion_rules", name: "spree_products_promotion_rules_promotion_rule_id_fk", column: "promotion_rule_id" + add_foreign_key "spree_products", "spree_shipping_categories", name: "spree_products_shipping_category_id_fk", column: "shipping_category_id" + add_foreign_key "spree_products", "enterprises", name: "spree_products_supplier_id_fk", column: "supplier_id" + add_foreign_key "spree_products", "spree_tax_categories", name: "spree_products_tax_category_id_fk", column: "tax_category_id" + add_foreign_key "spree_products_taxons", "spree_products", name: "spree_products_taxons_product_id_fk", column: "product_id", dependent: :delete + add_foreign_key "spree_products_taxons", "spree_taxons", name: "spree_products_taxons_taxon_id_fk", column: "taxon_id", dependent: :delete + add_foreign_key "spree_promotion_action_line_items", "spree_promotion_actions", name: "spree_promotion_action_line_items_promotion_action_id_fk", column: "promotion_action_id" + add_foreign_key "spree_promotion_action_line_items", "spree_variants", name: "spree_promotion_action_line_items_variant_id_fk", column: "variant_id" + add_foreign_key "spree_promotion_actions", "spree_activators", name: "spree_promotion_actions_activator_id_fk", column: "activator_id" + add_foreign_key "spree_promotion_rules", "spree_activators", name: "spree_promotion_rules_activator_id_fk", column: "activator_id" + add_foreign_key "spree_properties_prototypes", "spree_properties", name: "spree_properties_prototypes_property_id_fk", column: "property_id" + add_foreign_key "spree_properties_prototypes", "spree_prototypes", name: "spree_properties_prototypes_prototype_id_fk", column: "prototype_id" + add_foreign_key "spree_return_authorizations", "spree_orders", name: "spree_return_authorizations_order_id_fk", column: "order_id" + add_foreign_key "spree_roles_users", "spree_roles", name: "spree_roles_users_role_id_fk", column: "role_id" + add_foreign_key "spree_roles_users", "spree_users", name: "spree_roles_users_user_id_fk", column: "user_id" + add_foreign_key "spree_shipments", "spree_addresses", name: "spree_shipments_address_id_fk", column: "address_id" + add_foreign_key "spree_shipments", "spree_orders", name: "spree_shipments_order_id_fk", column: "order_id" + #add_foreign_key "spree_shipments", "spree_shipping_methods", name: "spree_shipments_shipping_method_id_fk", column: "shipping_method_id" + add_foreign_key "spree_shipping_methods", "spree_shipping_categories", name: "spree_shipping_methods_shipping_category_id_fk", column: "shipping_category_id" + add_foreign_key "spree_shipping_methods", "spree_zones", name: "spree_shipping_methods_zone_id_fk", column: "zone_id" + add_foreign_key "spree_state_changes", "spree_users", name: "spree_state_changes_user_id_fk", column: "user_id" + add_foreign_key "spree_states", "spree_countries", name: "spree_states_country_id_fk", column: "country_id" + add_foreign_key "spree_tax_rates", "spree_tax_categories", name: "spree_tax_rates_tax_category_id_fk", column: "tax_category_id" + add_foreign_key "spree_tax_rates", "spree_zones", name: "spree_tax_rates_zone_id_fk", column: "zone_id" + add_foreign_key "spree_taxons", "spree_taxons", name: "spree_taxons_parent_id_fk", column: "parent_id" + add_foreign_key "spree_taxons", "spree_taxonomies", name: "spree_taxons_taxonomy_id_fk", column: "taxonomy_id" + add_foreign_key "spree_users", "spree_addresses", name: "spree_users_bill_address_id_fk", column: "bill_address_id" + add_foreign_key "spree_users", "spree_addresses", name: "spree_users_ship_address_id_fk", column: "ship_address_id" + add_foreign_key "spree_variants", "spree_products", name: "spree_variants_product_id_fk", column: "product_id" + add_foreign_key "spree_zone_members", "spree_zones", name: "spree_zone_members_zone_id_fk", column: "zone_id" + add_foreign_key "suburbs", "spree_states", name: "suburbs_state_id_fk", column: "state_id" + end +end diff --git a/db/schema.rb b/db/schema.rb index 83688ff525..3137940846 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 => 20140402032034) do +ActiveRecord::Schema.define(:version => 20140402033428) do create_table "adjustment_metadata", :force => true do |t| t.integer "adjustment_id" @@ -966,4 +966,151 @@ ActiveRecord::Schema.define(:version => 20140402032034) do t.integer "state_id" end + add_foreign_key "adjustment_metadata", "enterprises", name: "adjustment_metadata_enterprise_id_fk" + add_foreign_key "adjustment_metadata", "spree_adjustments", name: "adjustment_metadata_adjustment_id_fk", column: "adjustment_id" + + add_foreign_key "carts", "spree_users", name: "carts_user_id_fk", column: "user_id" + + add_foreign_key "cms_blocks", "cms_pages", name: "cms_blocks_page_id_fk", column: "page_id" + + add_foreign_key "cms_categories", "cms_sites", name: "cms_categories_site_id_fk", column: "site_id", dependent: :delete + + add_foreign_key "cms_categorizations", "cms_categories", name: "cms_categorizations_category_id_fk", column: "category_id" + + add_foreign_key "cms_files", "cms_blocks", name: "cms_files_block_id_fk", column: "block_id" + add_foreign_key "cms_files", "cms_sites", name: "cms_files_site_id_fk", column: "site_id" + + add_foreign_key "cms_layouts", "cms_layouts", name: "cms_layouts_parent_id_fk", column: "parent_id" + add_foreign_key "cms_layouts", "cms_sites", name: "cms_layouts_site_id_fk", column: "site_id", dependent: :delete + + add_foreign_key "cms_pages", "cms_layouts", name: "cms_pages_layout_id_fk", column: "layout_id" + add_foreign_key "cms_pages", "cms_pages", name: "cms_pages_parent_id_fk", column: "parent_id" + add_foreign_key "cms_pages", "cms_pages", name: "cms_pages_target_page_id_fk", column: "target_page_id" + add_foreign_key "cms_pages", "cms_sites", name: "cms_pages_site_id_fk", column: "site_id", dependent: :delete + + add_foreign_key "cms_snippets", "cms_sites", name: "cms_snippets_site_id_fk", column: "site_id", dependent: :delete + + add_foreign_key "coordinator_fees", "enterprise_fees", name: "coordinator_fees_enterprise_fee_id_fk" + add_foreign_key "coordinator_fees", "order_cycles", name: "coordinator_fees_order_cycle_id_fk" + + add_foreign_key "distributors_payment_methods", "enterprises", name: "distributors_payment_methods_distributor_id_fk", column: "distributor_id" + add_foreign_key "distributors_payment_methods", "spree_payment_methods", name: "distributors_payment_methods_payment_method_id_fk", column: "payment_method_id" + + add_foreign_key "distributors_shipping_methods", "enterprises", name: "distributors_shipping_methods_distributor_id_fk", column: "distributor_id" + add_foreign_key "distributors_shipping_methods", "spree_shipping_methods", name: "distributors_shipping_methods_shipping_method_id_fk", column: "shipping_method_id" + + add_foreign_key "enterprise_fees", "enterprises", name: "enterprise_fees_enterprise_id_fk" + + add_foreign_key "enterprise_groups_enterprises", "enterprise_groups", name: "enterprise_groups_enterprises_enterprise_group_id_fk" + add_foreign_key "enterprise_groups_enterprises", "enterprises", name: "enterprise_groups_enterprises_enterprise_id_fk" + + add_foreign_key "enterprise_roles", "enterprises", name: "enterprise_roles_enterprise_id_fk" + add_foreign_key "enterprise_roles", "spree_users", name: "enterprise_roles_user_id_fk", column: "user_id" + + add_foreign_key "enterprises", "spree_addresses", name: "enterprises_address_id_fk", column: "address_id" + + add_foreign_key "exchange_fees", "enterprise_fees", name: "exchange_fees_enterprise_fee_id_fk" + add_foreign_key "exchange_fees", "exchanges", name: "exchange_fees_exchange_id_fk" + + add_foreign_key "exchange_variants", "exchanges", name: "exchange_variants_exchange_id_fk" + add_foreign_key "exchange_variants", "spree_variants", name: "exchange_variants_variant_id_fk", column: "variant_id" + + add_foreign_key "exchanges", "enterprises", name: "exchanges_payment_enterprise_id_fk", column: "payment_enterprise_id" + add_foreign_key "exchanges", "enterprises", name: "exchanges_receiver_id_fk", column: "receiver_id" + add_foreign_key "exchanges", "enterprises", name: "exchanges_sender_id_fk", column: "sender_id" + add_foreign_key "exchanges", "order_cycles", name: "exchanges_order_cycle_id_fk" + + add_foreign_key "order_cycles", "enterprises", name: "order_cycles_coordinator_id_fk", column: "coordinator_id" + + add_foreign_key "product_distributions", "enterprise_fees", name: "product_distributions_enterprise_fee_id_fk" + add_foreign_key "product_distributions", "enterprises", name: "product_distributions_distributor_id_fk", column: "distributor_id" + add_foreign_key "product_distributions", "spree_products", name: "product_distributions_product_id_fk", column: "product_id" + + add_foreign_key "spree_addresses", "spree_countries", name: "spree_addresses_country_id_fk", column: "country_id" + add_foreign_key "spree_addresses", "spree_states", name: "spree_addresses_state_id_fk", column: "state_id" + + add_foreign_key "spree_inventory_units", "spree_orders", name: "spree_inventory_units_order_id_fk", column: "order_id" + add_foreign_key "spree_inventory_units", "spree_return_authorizations", name: "spree_inventory_units_return_authorization_id_fk", column: "return_authorization_id" + add_foreign_key "spree_inventory_units", "spree_shipments", name: "spree_inventory_units_shipment_id_fk", column: "shipment_id" + add_foreign_key "spree_inventory_units", "spree_variants", name: "spree_inventory_units_variant_id_fk", column: "variant_id" + + add_foreign_key "spree_line_items", "spree_orders", name: "spree_line_items_order_id_fk", column: "order_id" + add_foreign_key "spree_line_items", "spree_variants", name: "spree_line_items_variant_id_fk", column: "variant_id" + + add_foreign_key "spree_option_types_prototypes", "spree_option_types", name: "spree_option_types_prototypes_option_type_id_fk", column: "option_type_id" + add_foreign_key "spree_option_types_prototypes", "spree_prototypes", name: "spree_option_types_prototypes_prototype_id_fk", column: "prototype_id" + + add_foreign_key "spree_option_values", "spree_option_types", name: "spree_option_values_option_type_id_fk", column: "option_type_id" + + add_foreign_key "spree_option_values_variants", "spree_option_values", name: "spree_option_values_variants_option_value_id_fk", column: "option_value_id" + add_foreign_key "spree_option_values_variants", "spree_variants", name: "spree_option_values_variants_variant_id_fk", column: "variant_id" + + add_foreign_key "spree_orders", "carts", name: "spree_orders_cart_id_fk" + add_foreign_key "spree_orders", "enterprises", name: "spree_orders_distributor_id_fk", column: "distributor_id" + add_foreign_key "spree_orders", "order_cycles", name: "spree_orders_order_cycle_id_fk" + add_foreign_key "spree_orders", "spree_addresses", name: "spree_orders_bill_address_id_fk", column: "bill_address_id" + add_foreign_key "spree_orders", "spree_addresses", name: "spree_orders_ship_address_id_fk", column: "ship_address_id" + add_foreign_key "spree_orders", "spree_users", name: "spree_orders_user_id_fk", column: "user_id" + + add_foreign_key "spree_payments", "spree_orders", name: "spree_payments_order_id_fk", column: "order_id" + add_foreign_key "spree_payments", "spree_payment_methods", name: "spree_payments_payment_method_id_fk", column: "payment_method_id" + + add_foreign_key "spree_prices", "spree_variants", name: "spree_prices_variant_id_fk", column: "variant_id" + + add_foreign_key "spree_product_option_types", "spree_option_types", name: "spree_product_option_types_option_type_id_fk", column: "option_type_id" + add_foreign_key "spree_product_option_types", "spree_products", name: "spree_product_option_types_product_id_fk", column: "product_id" + + add_foreign_key "spree_product_properties", "spree_products", name: "spree_product_properties_product_id_fk", column: "product_id" + add_foreign_key "spree_product_properties", "spree_properties", name: "spree_product_properties_property_id_fk", column: "property_id" + + add_foreign_key "spree_products", "enterprises", name: "spree_products_supplier_id_fk", column: "supplier_id" + add_foreign_key "spree_products", "spree_shipping_categories", name: "spree_products_shipping_category_id_fk", column: "shipping_category_id" + add_foreign_key "spree_products", "spree_tax_categories", name: "spree_products_tax_category_id_fk", column: "tax_category_id" + + add_foreign_key "spree_products_promotion_rules", "spree_products", name: "spree_products_promotion_rules_product_id_fk", column: "product_id" + add_foreign_key "spree_products_promotion_rules", "spree_promotion_rules", name: "spree_products_promotion_rules_promotion_rule_id_fk", column: "promotion_rule_id" + + add_foreign_key "spree_products_taxons", "spree_products", name: "spree_products_taxons_product_id_fk", column: "product_id", dependent: :delete + add_foreign_key "spree_products_taxons", "spree_taxons", name: "spree_products_taxons_taxon_id_fk", column: "taxon_id", dependent: :delete + + add_foreign_key "spree_promotion_action_line_items", "spree_promotion_actions", name: "spree_promotion_action_line_items_promotion_action_id_fk", column: "promotion_action_id" + add_foreign_key "spree_promotion_action_line_items", "spree_variants", name: "spree_promotion_action_line_items_variant_id_fk", column: "variant_id" + + add_foreign_key "spree_promotion_actions", "spree_activators", name: "spree_promotion_actions_activator_id_fk", column: "activator_id" + + add_foreign_key "spree_promotion_rules", "spree_activators", name: "spree_promotion_rules_activator_id_fk", column: "activator_id" + + add_foreign_key "spree_properties_prototypes", "spree_properties", name: "spree_properties_prototypes_property_id_fk", column: "property_id" + add_foreign_key "spree_properties_prototypes", "spree_prototypes", name: "spree_properties_prototypes_prototype_id_fk", column: "prototype_id" + + add_foreign_key "spree_return_authorizations", "spree_orders", name: "spree_return_authorizations_order_id_fk", column: "order_id" + + add_foreign_key "spree_roles_users", "spree_roles", name: "spree_roles_users_role_id_fk", column: "role_id" + add_foreign_key "spree_roles_users", "spree_users", name: "spree_roles_users_user_id_fk", column: "user_id" + + add_foreign_key "spree_shipments", "spree_addresses", name: "spree_shipments_address_id_fk", column: "address_id" + add_foreign_key "spree_shipments", "spree_orders", name: "spree_shipments_order_id_fk", column: "order_id" + + add_foreign_key "spree_shipping_methods", "spree_shipping_categories", name: "spree_shipping_methods_shipping_category_id_fk", column: "shipping_category_id" + add_foreign_key "spree_shipping_methods", "spree_zones", name: "spree_shipping_methods_zone_id_fk", column: "zone_id" + + add_foreign_key "spree_state_changes", "spree_users", name: "spree_state_changes_user_id_fk", column: "user_id" + + add_foreign_key "spree_states", "spree_countries", name: "spree_states_country_id_fk", column: "country_id" + + add_foreign_key "spree_tax_rates", "spree_tax_categories", name: "spree_tax_rates_tax_category_id_fk", column: "tax_category_id" + add_foreign_key "spree_tax_rates", "spree_zones", name: "spree_tax_rates_zone_id_fk", column: "zone_id" + + add_foreign_key "spree_taxons", "spree_taxonomies", name: "spree_taxons_taxonomy_id_fk", column: "taxonomy_id" + add_foreign_key "spree_taxons", "spree_taxons", name: "spree_taxons_parent_id_fk", column: "parent_id" + + add_foreign_key "spree_users", "spree_addresses", name: "spree_users_bill_address_id_fk", column: "bill_address_id" + add_foreign_key "spree_users", "spree_addresses", name: "spree_users_ship_address_id_fk", column: "ship_address_id" + + add_foreign_key "spree_variants", "spree_products", name: "spree_variants_product_id_fk", column: "product_id" + + add_foreign_key "spree_zone_members", "spree_zones", name: "spree_zone_members_zone_id_fk", column: "zone_id" + + add_foreign_key "suburbs", "spree_states", name: "suburbs_state_id_fk", column: "state_id" + end From 4cbe43457332363b233d84c47b580d6ce766be33 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 4 Apr 2014 10:50:39 +1100 Subject: [PATCH 04/22] Remove trailing relations on destruction for EnterpriseFee and Variant --- app/models/enterprise_fee.rb | 5 +++++ app/models/spree/variant_decorator.rb | 2 +- spec/models/enterprise_fee_spec.rb | 19 +++++++++++++++++++ spec/models/spree/variant_spec.rb | 10 ++++++++++ 4 files changed, 35 insertions(+), 1 deletion(-) diff --git a/app/models/enterprise_fee.rb b/app/models/enterprise_fee.rb index 148c18d367..f41ad76c96 100644 --- a/app/models/enterprise_fee.rb +++ b/app/models/enterprise_fee.rb @@ -1,5 +1,10 @@ class EnterpriseFee < ActiveRecord::Base belongs_to :enterprise + has_and_belongs_to_many :order_cycles, join_table: 'coordinator_fees' + has_many :exchange_fees, dependent: :destroy + has_many :exchanges, through: :exchange_fees + + before_destroy { order_cycles.clear } calculated_adjustments diff --git a/app/models/spree/variant_decorator.rb b/app/models/spree/variant_decorator.rb index 23f88b3f35..165c2d34f6 100644 --- a/app/models/spree/variant_decorator.rb +++ b/app/models/spree/variant_decorator.rb @@ -1,5 +1,5 @@ Spree::Variant.class_eval do - has_many :exchange_variants + has_many :exchange_variants, dependent: :destroy has_many :exchanges, through: :exchange_variants attr_accessible :unit_value, :unit_description diff --git a/spec/models/enterprise_fee_spec.rb b/spec/models/enterprise_fee_spec.rb index 806eb7759c..fc72f383d2 100644 --- a/spec/models/enterprise_fee_spec.rb +++ b/spec/models/enterprise_fee_spec.rb @@ -9,6 +9,25 @@ describe EnterpriseFee do it { should validate_presence_of(:name) } end + describe "callbacks" do + it "removes itself from order cycle coordinator fees when destroyed" do + ef = create(:enterprise_fee) + oc = create(:simple_order_cycle, coordinator_fees: [ef]) + + ef.destroy + oc.reload.coordinator_fee_ids.should be_empty + end + + it "removes itself from order cycle exchange fees when destroyed" do + ef = create(:enterprise_fee) + oc = create(:simple_order_cycle) + ex = create(:exchange, order_cycle: oc, enterprise_fees: [ef]) + + ef.destroy + ex.reload.exchange_fee_ids.should be_empty + end + end + describe "scopes" do describe "finding per-item enterprise fees" do it "does not return fees with FlatRate and FlexiRate calculators" do diff --git a/spec/models/spree/variant_spec.rb b/spec/models/spree/variant_spec.rb index 9c37663e1e..4b9e1fab0a 100644 --- a/spec/models/spree/variant_spec.rb +++ b/spec/models/spree/variant_spec.rb @@ -374,4 +374,14 @@ module Spree end end end + + describe "destruction" do + it "destroys exchange variants" do + v = create(:variant) + e = create(:exchange, variants: [v]) + + v.destroy + e.reload.variant_ids.should be_empty + end + end end From e28a4508759facdf668b17ec740da0c15413848c Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 4 Apr 2014 13:28:46 +1100 Subject: [PATCH 05/22] Add API call to soft-delete a variant --- .../api/variants_controller_decorator.rb | 13 ++++++++++++ config/routes.rb | 4 ++++ lib/spree/api/testing_support/setup.rb | 19 +++++++++++++++++ .../spree/api/variants_controller_spec.rb | 21 +++++++++++++++---- spec/spec_helper.rb | 4 ++++ 5 files changed, 57 insertions(+), 4 deletions(-) create mode 100644 app/controllers/spree/api/variants_controller_decorator.rb create mode 100644 lib/spree/api/testing_support/setup.rb diff --git a/app/controllers/spree/api/variants_controller_decorator.rb b/app/controllers/spree/api/variants_controller_decorator.rb new file mode 100644 index 0000000000..476d3c1543 --- /dev/null +++ b/app/controllers/spree/api/variants_controller_decorator.rb @@ -0,0 +1,13 @@ +Spree::Api::VariantsController.class_eval do + def soft_delete + @variant = scope.find(params[:id]) + authorize! :delete, @variant + + @variant.deleted_at = Time.now() + if @variant.save + respond_with(@variant, :status => 204) + else + invalid_resource!(@variant) + end + end +end diff --git a/config/routes.rb b/config/routes.rb index b2ad6b0eeb..4ca341db37 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -110,6 +110,10 @@ Spree::Core::Engine.routes.prepend do get :managed, on: :collection end + resources :variants do + delete :soft_delete + end + resources :orders do get :managed, on: :collection end diff --git a/lib/spree/api/testing_support/setup.rb b/lib/spree/api/testing_support/setup.rb new file mode 100644 index 0000000000..a54e45a46b --- /dev/null +++ b/lib/spree/api/testing_support/setup.rb @@ -0,0 +1,19 @@ +module Spree + module Api + module TestingSupport + module Setup + def sign_in_as_admin! + let!(:current_api_user) do + user = stub_model(Spree::LegacyUser) + user.should_receive(:has_spree_role?).any_number_of_times.with("admin").and_return(true) + + # Stub enterprises, needed for cancan ability checks + user.stub(:enterprises) { [] } + + user + end + end + end + end + end +end diff --git a/spec/controllers/spree/api/variants_controller_spec.rb b/spec/controllers/spree/api/variants_controller_spec.rb index c61928e1bb..66806798fc 100644 --- a/spec/controllers/spree/api/variants_controller_spec.rb +++ b/spec/controllers/spree/api/variants_controller_spec.rb @@ -1,11 +1,9 @@ require 'spec_helper' -require 'spree/api/testing_support/helpers' module Spree describe Spree::Api::VariantsController do - include Spree::Api::TestingSupport::Helpers render_views - + let!(:variant1) { FactoryGirl.create(:variant) } let!(:variant2) { FactoryGirl.create(:variant) } let!(:variant3) { FactoryGirl.create(:variant) } @@ -29,6 +27,7 @@ module Spree keys = json_response.keys.map{ |key| key.to_sym } unit_attributes.all?{ |attr| keys.include? attr }.should == true end + #it "sorts variants in ascending id order" do # spree_get :index, { :template => 'bulk_index', :format => :json } # ids = json_response.map{ |variant| variant['id'] } @@ -36,5 +35,19 @@ module Spree # ids[1].should < ids[2] #end end + + context "as an administrator" do + sign_in_as_admin! + + it "soft deletes a variant" do + product = create(:product) + variant = product.master + + spree_delete :soft_delete, {id: variant.to_param, product_id: product.to_param, format: :json} + response.status.should == 204 + lambda { variant.reload }.should_not raise_error(ActiveRecord::RecordNotFound) + variant.deleted_at.should_not be_nil + end + end end -end \ No newline at end of file +end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 76d72d7291..121e827169 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -23,6 +23,8 @@ WebMock.disable_net_connect!(:allow_localhost => true) Dir[Rails.root.join("spec/support/**/*.rb")].each {|f| require f} require 'spree/core/testing_support/controller_requests' require 'spree/core/testing_support/capybara_ext' +require 'spree/api/testing_support/setup' +require 'spree/api/testing_support/helpers' require 'active_record/fixtures' fixtures_dir = File.expand_path('../../db/default', __FILE__) @@ -92,6 +94,8 @@ RSpec.configure do |config| config.include Spree::CheckoutHelpers config.include Spree::Core::TestingSupport::ControllerRequests, :type => :controller config.include Devise::TestHelpers, :type => :controller + config.extend Spree::Api::TestingSupport::Setup, :type => :controller + config.include Spree::Api::TestingSupport::Helpers, :type => :controller config.include OpenFoodNetwork::FeatureToggleHelper config.include OpenFoodNetwork::EnterpriseGroupsHelper config.include OpenFoodNetwork::DistributionHelper From e6d988ff34935bd0a0f4dbcb1b4c7dbe82f85998 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 4 Apr 2014 14:06:12 +1100 Subject: [PATCH 06/22] Nest variant soft delete route under product resource --- app/controllers/spree/api/variants_controller_decorator.rb | 2 +- config/routes.rb | 6 +++--- spec/controllers/spree/api/variants_controller_spec.rb | 2 +- 3 files changed, 5 insertions(+), 5 deletions(-) diff --git a/app/controllers/spree/api/variants_controller_decorator.rb b/app/controllers/spree/api/variants_controller_decorator.rb index 476d3c1543..a225c4c401 100644 --- a/app/controllers/spree/api/variants_controller_decorator.rb +++ b/app/controllers/spree/api/variants_controller_decorator.rb @@ -1,6 +1,6 @@ Spree::Api::VariantsController.class_eval do def soft_delete - @variant = scope.find(params[:id]) + @variant = scope.find(params[:variant_id]) authorize! :delete, @variant @variant.deleted_at = Time.now() diff --git a/config/routes.rb b/config/routes.rb index 4ca341db37..63d313aa09 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -108,10 +108,10 @@ Spree::Core::Engine.routes.prepend do resources :products do get :managed, on: :collection - end - resources :variants do - delete :soft_delete + resources :variants do + delete :soft_delete + end end resources :orders do diff --git a/spec/controllers/spree/api/variants_controller_spec.rb b/spec/controllers/spree/api/variants_controller_spec.rb index 66806798fc..6177344b7c 100644 --- a/spec/controllers/spree/api/variants_controller_spec.rb +++ b/spec/controllers/spree/api/variants_controller_spec.rb @@ -43,7 +43,7 @@ module Spree product = create(:product) variant = product.master - spree_delete :soft_delete, {id: variant.to_param, product_id: product.to_param, format: :json} + spree_delete :soft_delete, {variant_id: variant.to_param, product_id: product.to_param, format: :json} response.status.should == 204 lambda { variant.reload }.should_not raise_error(ActiveRecord::RecordNotFound) variant.deleted_at.should_not be_nil From d16d97095213cd7bbba788553ad5a2d0d7a49c2b Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 4 Apr 2014 14:12:27 +1100 Subject: [PATCH 07/22] BPE uses soft-delete --- app/assets/javascripts/admin/bulk_product_update.js.coffee | 2 +- spec/features/admin/bulk_product_update_spec.rb | 4 ++-- spec/javascripts/unit/bulk_product_update_spec.js.coffee | 6 +++--- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/admin/bulk_product_update.js.coffee b/app/assets/javascripts/admin/bulk_product_update.js.coffee index c2641615e2..35e66c49ad 100644 --- a/app/assets/javascripts/admin/bulk_product_update.js.coffee +++ b/app/assets/javascripts/admin/bulk_product_update.js.coffee @@ -291,7 +291,7 @@ productEditModule.controller "AdminProductEditCtrl", [ if confirm("Are you sure?") $http( method: "DELETE" - url: "/api/products/" + product.id + "/variants/" + variant.id + url: "/api/products/" + product.permalink_live + "/variants/" + variant.id + "/soft_delete" ).success (data) -> $scope.removeVariant(product, variant) diff --git a/spec/features/admin/bulk_product_update_spec.rb b/spec/features/admin/bulk_product_update_spec.rb index 62d9b53cee..0c8d81bd5c 100644 --- a/spec/features/admin/bulk_product_update_spec.rb +++ b/spec/features/admin/bulk_product_update_spec.rb @@ -609,7 +609,7 @@ feature %q{ visit '/admin/products/bulk_edit' page.should have_selector "a.view-variants" - all("a.view-variants").each{ |e| e.click } + all("a.view-variants").each { |e| e.click } page.should have_selector "a.delete-variant", :count => 3 @@ -620,7 +620,7 @@ feature %q{ visit '/admin/products/bulk_edit' page.should have_selector "a.view-variants" - all("a.view-variants").select{ |e| e.visible? }.each{ |e| e.click } + all("a.view-variants").select { |e| e.visible? }.each { |e| e.click } page.should have_selector "a.delete-variant", :count => 2 end diff --git a/spec/javascripts/unit/bulk_product_update_spec.js.coffee b/spec/javascripts/unit/bulk_product_update_spec.js.coffee index 58a5ce1941..7a452b213a 100644 --- a/spec/javascripts/unit/bulk_product_update_spec.js.coffee +++ b/spec/javascripts/unit/bulk_product_update_spec.js.coffee @@ -1100,7 +1100,7 @@ describe "AdminProductEditCtrl", -> describe "when the variant has been saved", -> - it "deletes variants with a http delete request to /api/products/product_id/variants/(variant_id)", -> + it "deletes variants with a http delete request to /api/products/product_permalink/variants/(variant_id)", -> spyOn(window, "confirm").andReturn true scope.products = [ { @@ -1117,7 +1117,7 @@ describe "AdminProductEditCtrl", -> } ] scope.dirtyProducts = {} - httpBackend.expectDELETE("/api/products/9/variants/3").respond 200, "data" + httpBackend.expectDELETE("/api/products/apples/variants/3/soft_delete").respond 200, "data" scope.deleteVariant scope.products[0], scope.products[0].variants[0] httpBackend.flush() @@ -1159,7 +1159,7 @@ describe "AdminProductEditCtrl", -> id: 13 name: "P1" - httpBackend.expectDELETE("/api/products/9/variants/3").respond 200, "data" + httpBackend.expectDELETE("/api/products/apples/variants/3/soft_delete").respond 200, "data" scope.deleteVariant scope.products[0], scope.products[0].variants[0] httpBackend.flush() expect(scope.products[0].variants).toEqual [ From 95a6e34523a9d4df5ff5a0f2c6b33c40d2cef52e Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 11 Apr 2014 11:21:10 +1000 Subject: [PATCH 08/22] Spec access denied to regular user when soft-deleting variants --- .../api/testing_support/helpers_decorator.rb | 7 +++++++ lib/spree/api/testing_support/setup.rb | 2 +- .../spree/api/variants_controller_spec.rb | 19 ++++++++++++++++++- spec/spec_helper.rb | 1 + 4 files changed, 27 insertions(+), 2 deletions(-) create mode 100644 lib/spree/api/testing_support/helpers_decorator.rb diff --git a/lib/spree/api/testing_support/helpers_decorator.rb b/lib/spree/api/testing_support/helpers_decorator.rb new file mode 100644 index 0000000000..0ade798960 --- /dev/null +++ b/lib/spree/api/testing_support/helpers_decorator.rb @@ -0,0 +1,7 @@ +require 'spree/api/testing_support/helpers' + +Spree::Api::TestingSupport::Helpers.class_eval do + def current_api_user + @current_api_user ||= stub_model(Spree::LegacyUser, :email => "spree@example.com", :enterprises => []) + end +end diff --git a/lib/spree/api/testing_support/setup.rb b/lib/spree/api/testing_support/setup.rb index a54e45a46b..d7ee226b8f 100644 --- a/lib/spree/api/testing_support/setup.rb +++ b/lib/spree/api/testing_support/setup.rb @@ -5,7 +5,7 @@ module Spree def sign_in_as_admin! let!(:current_api_user) do user = stub_model(Spree::LegacyUser) - user.should_receive(:has_spree_role?).any_number_of_times.with("admin").and_return(true) + user.stub(:has_spree_role?).with("admin").and_return(true) # Stub enterprises, needed for cancan ability checks user.stub(:enterprises) { [] } diff --git a/spec/controllers/spree/api/variants_controller_spec.rb b/spec/controllers/spree/api/variants_controller_spec.rb index 6177344b7c..66ddf93c57 100644 --- a/spec/controllers/spree/api/variants_controller_spec.rb +++ b/spec/controllers/spree/api/variants_controller_spec.rb @@ -16,6 +16,13 @@ module Spree end context "as a normal user" do + let!(:current_api_user) do + user = stub_model(Spree::LegacyUser) + user.stub(:has_spree_role?).with("admin").and_return(false) + user.stub(:enterprises) { [] } + user + end + it "retrieves a list of variants with appropriate attributes" do spree_get :index, { :template => 'bulk_index', :format => :json } keys = json_response.first.keys.map{ |key| key.to_sym } @@ -28,6 +35,16 @@ module Spree unit_attributes.all?{ |attr| keys.include? attr }.should == true end + it "is denied access when trying to delete a variant" do + product = create(:product) + variant = product.master + + spree_delete :soft_delete, {variant_id: variant.to_param, product_id: product.to_param, format: :json} + assert_unauthorized! + lambda { variant.reload }.should_not raise_error + variant.deleted_at.should be_nil + end + #it "sorts variants in ascending id order" do # spree_get :index, { :template => 'bulk_index', :format => :json } # ids = json_response.map{ |variant| variant['id'] } @@ -45,7 +62,7 @@ module Spree spree_delete :soft_delete, {variant_id: variant.to_param, product_id: product.to_param, format: :json} response.status.should == 204 - lambda { variant.reload }.should_not raise_error(ActiveRecord::RecordNotFound) + lambda { variant.reload }.should_not raise_error variant.deleted_at.should_not be_nil end end diff --git a/spec/spec_helper.rb b/spec/spec_helper.rb index 121e827169..6cf9229252 100644 --- a/spec/spec_helper.rb +++ b/spec/spec_helper.rb @@ -25,6 +25,7 @@ require 'spree/core/testing_support/controller_requests' require 'spree/core/testing_support/capybara_ext' require 'spree/api/testing_support/setup' require 'spree/api/testing_support/helpers' +require 'spree/api/testing_support/helpers_decorator' require 'active_record/fixtures' fixtures_dir = File.expand_path('../../db/default', __FILE__) From 6c86254a900bdd5f79f3a930326b9ddddb1609ec Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 11 Apr 2014 12:05:05 +1000 Subject: [PATCH 09/22] Extract sign_in_as_user method, test managed orders API access for normal user and enterprise user --- .../spree/api/orders_controller_decorator.rb | 4 +- lib/spree/api/testing_support/setup.rb | 24 +++++++ .../spree/api/orders_controller_spec.rb | 70 ++++++++++--------- .../spree/api/variants_controller_spec.rb | 7 +- 4 files changed, 64 insertions(+), 41 deletions(-) diff --git a/app/controllers/spree/api/orders_controller_decorator.rb b/app/controllers/spree/api/orders_controller_decorator.rb index 53e815a5f6..ca1fc4a570 100644 --- a/app/controllers/spree/api/orders_controller_decorator.rb +++ b/app/controllers/spree/api/orders_controller_decorator.rb @@ -7,7 +7,9 @@ Spree::Api::OrdersController.class_eval do before_filter :authorize_read!, :except => [:managed] def managed + authorize! :admin, Spree::Order + authorize! :read, Spree::Order @orders = Spree::Order.ransack(params[:q]).result.distributed_by_user(current_api_user).page(params[:page]).per(params[:per_page]) respond_with(@orders, default_template: :index) end -end \ No newline at end of file +end diff --git a/lib/spree/api/testing_support/setup.rb b/lib/spree/api/testing_support/setup.rb index d7ee226b8f..cbf8393e33 100644 --- a/lib/spree/api/testing_support/setup.rb +++ b/lib/spree/api/testing_support/setup.rb @@ -2,6 +2,30 @@ module Spree module Api module TestingSupport module Setup + def sign_in_as_user! + let!(:current_api_user) do + user = stub_model(Spree::LegacyUser) + user.stub(:has_spree_role?).with("admin").and_return(false) + user.stub(:enterprises) { [] } + user + end + end + + # enterprises is an array of variable names of let defines + # eg. + # let(:enterprise) { ... } + # sign_in_as_enterprise_user! [:enterprise] + def sign_in_as_enterprise_user!(enterprises) + let!(:current_api_user) do + user = create(:user) + user.spree_roles = [] + enterprises.each { |e| user.enterprise_roles.create(enterprise: send(e)) } + user.save! + user + end + end + + def sign_in_as_admin! let!(:current_api_user) do user = stub_model(Spree::LegacyUser) diff --git a/spec/controllers/spree/api/orders_controller_spec.rb b/spec/controllers/spree/api/orders_controller_spec.rb index bc34ba5f39..ec87169f76 100644 --- a/spec/controllers/spree/api/orders_controller_spec.rb +++ b/spec/controllers/spree/api/orders_controller_spec.rb @@ -5,7 +5,15 @@ module Spree describe Spree::Api::OrdersController do include Spree::Api::TestingSupport::Helpers render_views - context "as a normal user" do + + before do + stub_authentication! + Spree.user_class.stub :find_by_spree_api_key => current_api_user + end + + let(:order_attributes) { [:id, :full_name, :email, :phone, :completed_at, :line_items, :distributor, :order_cycle, :number] } + + def self.make_simple_data! let!(:dist1) { FactoryGirl.create(:distributor_enterprise) } let!(:order1) { FactoryGirl.create(:order, state: 'complete', completed_at: Time.now, distributor: dist1, billing_address: FactoryGirl.create(:address) ) } let!(:order2) { FactoryGirl.create(:order, state: 'complete', completed_at: Time.now, distributor: dist1, billing_address: FactoryGirl.create(:address) ) } @@ -14,13 +22,23 @@ module Spree let!(:line_item2) { FactoryGirl.create(:line_item, order: order2) } let!(:line_item3) { FactoryGirl.create(:line_item, order: order2) } let!(:line_item4) { FactoryGirl.create(:line_item, order: order3) } - let(:order_attributes) { [:id, :full_name, :email, :phone, :completed_at, :line_items, :distributor, :order_cycle, :number] } let(:line_item_attributes) { [:id, :quantity, :max_quantity, :supplier, :units_product, :units_variant] } - - before do - stub_authentication! - Spree.user_class.stub :find_by_spree_api_key => current_api_user + end + + + context "as a normal user" do + sign_in_as_user! + make_simple_data! + + it "should deny me access to managed orders" do + spree_get :managed, { :template => 'bulk_index', :format => :json } + assert_unauthorized! end + end + + context "as an administrator" do + sign_in_as_admin! + make_simple_data! before :each do spree_get :managed, { :template => 'bulk_index', :format => :json } @@ -68,7 +86,7 @@ module Spree end end - context "As an enterprise user" do + context "as an enterprise user" do let(:supplier) { create(:supplier_enterprise) } let(:distributor1) { create(:distributor_enterprise) } let(:distributor2) { create(:distributor_enterprise) } @@ -77,51 +95,35 @@ module Spree let!(:line_item2) { FactoryGirl.create(:line_item, order: order1, product: FactoryGirl.create(:product, supplier: supplier)) } let!(:order2) { FactoryGirl.create(:order, state: 'complete', completed_at: Time.now, distributor: distributor2, billing_address: FactoryGirl.create(:address) ) } let!(:line_item3) { FactoryGirl.create(:line_item, order: order2, product: FactoryGirl.create(:product, supplier: supplier)) } - let(:supplier_user) do - user = create(:user) - user.spree_roles = [] - user.enterprise_roles.create(enterprise: supplier) - user.save! - user - end - let(:distributor1_user) do - user = create(:user) - user.spree_roles = [] - user.enterprise_roles.create(enterprise: distributor1) - user.save! - user - end - let(:distributor2_user) do - user = create(:user) - user.spree_roles = [] - user.enterprise_roles.create(enterprise: distributor2) - user.save! - user - end context "producer enterprise" do + sign_in_as_enterprise_user! [:supplier] + before :each do - stub_authentication! - Spree.user_class.stub :find_by_spree_api_key => supplier_user spree_get :managed, { :template => 'bulk_index', :format => :json } end it "does not display line item for which my enteprise is a supplier" do - json_response.map{ |order| order['line_items'] }.flatten.length.should == 0 + response.status.should == 401 end end context "hub enterprise" do + sign_in_as_enterprise_user! [:distributor1] + before :each do - stub_authentication! - Spree.user_class.stub :find_by_spree_api_key => distributor1_user spree_get :managed, { :template => 'bulk_index', :format => :json } end + it "retrieves a list of orders" do + keys = json_response.first.keys.map{ |key| key.to_sym } + order_attributes.all?{ |attr| keys.include? attr }.should == true + end + it "only displays line items from orders for which my enterprise is a distributor" do json_response.map{ |order| order['line_items'] }.flatten.map{ |line_item| line_item["id"] }.should == [line_item1.id, line_item2.id] end end end end -end \ No newline at end of file +end diff --git a/spec/controllers/spree/api/variants_controller_spec.rb b/spec/controllers/spree/api/variants_controller_spec.rb index 66ddf93c57..2eb0145271 100644 --- a/spec/controllers/spree/api/variants_controller_spec.rb +++ b/spec/controllers/spree/api/variants_controller_spec.rb @@ -16,12 +16,7 @@ module Spree end context "as a normal user" do - let!(:current_api_user) do - user = stub_model(Spree::LegacyUser) - user.stub(:has_spree_role?).with("admin").and_return(false) - user.stub(:enterprises) { [] } - user - end + sign_in_as_user! it "retrieves a list of variants with appropriate attributes" do spree_get :index, { :template => 'bulk_index', :format => :json } From 7931a2e8da28ad3e986a7f43018586937b6a4579 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 11 Apr 2014 12:31:23 +1000 Subject: [PATCH 10/22] Test managed products API access for all user types --- .../api/products_controller_decorator.rb | 3 ++ .../spree/api/products_controller_spec.rb | 40 +++++++++++++++++-- 2 files changed, 39 insertions(+), 4 deletions(-) diff --git a/app/controllers/spree/api/products_controller_decorator.rb b/app/controllers/spree/api/products_controller_decorator.rb index 7175ded8c1..0dbecaa9fd 100644 --- a/app/controllers/spree/api/products_controller_decorator.rb +++ b/app/controllers/spree/api/products_controller_decorator.rb @@ -1,5 +1,8 @@ Spree::Api::ProductsController.class_eval do def managed + authorize! :admin, Spree::Product + authorize! :read, Spree::Product + @products = product_scope.ransack(params[:q]).result.managed_by(current_api_user).page(params[:page]).per(params[:per_page]) respond_with(@products, default_template: :index) end diff --git a/spec/controllers/spree/api/products_controller_spec.rb b/spec/controllers/spree/api/products_controller_spec.rb index 0d7560aa1b..c64b95853f 100644 --- a/spec/controllers/spree/api/products_controller_spec.rb +++ b/spec/controllers/spree/api/products_controller_spec.rb @@ -6,9 +6,10 @@ module Spree include Spree::Api::TestingSupport::Helpers render_views - let!(:product1) { FactoryGirl.create(:product) } - let!(:product2) { FactoryGirl.create(:product) } - let!(:product3) { FactoryGirl.create(:product) } + let(:supplier) { 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(:attributes) { [:id, :name, :supplier, :price, :on_hand, :available_on, :permalink_live] } let(:unit_attributes) { [:id, :name, :group_buy_unit_size, :variant_unit] } @@ -18,6 +19,37 @@ module Spree end context "as a normal user" do + sign_in_as_user! + + it "should deny me access to managed products" do + spree_get :managed, { :template => 'bulk_index', :format => :json } + assert_unauthorized! + end + end + + context "as an enterprise user" do + sign_in_as_enterprise_user! [:supplier] + + before :each do + spree_get :index, { :template => 'bulk_index', :format => :json } + end + + it "retrieves a list of managed products" do + spree_get :managed, { :template => 'bulk_index', :format => :json } + keys = json_response.first.keys.map{ |key| key.to_sym } + attributes.all?{ |attr| keys.include? attr }.should == true + end + end + + context "as an administrator" do + sign_in_as_admin! + + it "retrieves a list of managed products" do + spree_get :managed, { :template => 'bulk_index', :format => :json } + keys = json_response.first.keys.map{ |key| key.to_sym } + attributes.all?{ |attr| keys.include? attr }.should == true + end + it "retrieves a list of products with appropriate attributes" do spree_get :index, { :template => 'bulk_index', :format => :json } keys = json_response.first.keys.map{ |key| key.to_sym } @@ -61,4 +93,4 @@ module Spree end end end -end \ No newline at end of file +end From 8715b71151e9bed3d9fabbb54e9fc305bb764b1e Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 11 Apr 2014 12:34:11 +1000 Subject: [PATCH 11/22] Do not show rspec profiling by default --- .rspec | 1 - 1 file changed, 1 deletion(-) diff --git a/.rspec b/.rspec index 333fedd86c..53607ea52b 100644 --- a/.rspec +++ b/.rspec @@ -1,2 +1 @@ --colour ---profile From 3c8757034a1b409ccfdb207896c90c0fbfd3a3ae Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 11 Apr 2014 13:18:28 +1000 Subject: [PATCH 12/22] Enterprise user can soft-delete a variant. BUT, only its own variants. --- app/models/spree/ability_decorator.rb | 8 +++-- .../spree/api/variants_controller_spec.rb | 29 +++++++++++++++---- spec/models/spree/ability_spec.rb | 7 ++++- 3 files changed, 35 insertions(+), 9 deletions(-) diff --git a/app/models/spree/ability_decorator.rb b/app/models/spree/ability_decorator.rb index 795a818693..ee83e85bf7 100644 --- a/app/models/spree/ability_decorator.rb +++ b/app/models/spree/ability_decorator.rb @@ -12,11 +12,15 @@ class AbilityDecorator # Enterprise User can only access products that they are a supplier for can [:create], Spree::Product - can [:admin, :read, :update, :product_distributions, :bulk_edit, :bulk_update, :clone, :destroy], Spree::Product do |product| + can [:admin, :read, :update, :product_distributions, :bulk_edit, :bulk_update, :clone, :destroy], Spree::Product do |product| user.enterprises.include? product.supplier end - can [:admin, :index, :read, :create, :edit, :update, :search, :destroy], Spree::Variant + can [:create], Spree::Variant + can [:admin, :index, :read, :edit, :update, :search, :destroy], Spree::Variant do |variant| + user.enterprises.include? variant.product.supplier + end + can [:admin, :index, :read, :create, :edit, :update_positions, :destroy], Spree::ProductProperty can [:admin, :index, :read, :create, :edit, :update, :destroy], Spree::Image diff --git a/spec/controllers/spree/api/variants_controller_spec.rb b/spec/controllers/spree/api/variants_controller_spec.rb index 2eb0145271..818e10457a 100644 --- a/spec/controllers/spree/api/variants_controller_spec.rb +++ b/spec/controllers/spree/api/variants_controller_spec.rb @@ -4,6 +4,7 @@ module Spree describe Spree::Api::VariantsController do render_views + let(:supplier) { FactoryGirl.create(:supplier_enterprise) } let!(:variant1) { FactoryGirl.create(:variant) } let!(:variant2) { FactoryGirl.create(:variant) } let!(:variant3) { FactoryGirl.create(:variant) } @@ -39,13 +40,29 @@ module Spree lambda { variant.reload }.should_not raise_error variant.deleted_at.should be_nil end + end - #it "sorts variants in ascending id order" do - # spree_get :index, { :template => 'bulk_index', :format => :json } - # ids = json_response.map{ |variant| variant['id'] } - # ids[0].should < ids[1] - # ids[1].should < ids[2] - #end + context "as an enterprise user" do + sign_in_as_enterprise_user! [:supplier] + let(:supplier_other) { create(:supplier_enterprise) } + let(:product) { create(:product, supplier: supplier) } + let(:variant) { product.master } + let(:product_other) { create(:product, supplier: supplier_other) } + let(:variant_other) { product_other.master } + + it "soft deletes a variant" do + spree_delete :soft_delete, {variant_id: variant.to_param, product_id: product.to_param, format: :json} + response.status.should == 204 + lambda { variant.reload }.should_not raise_error + variant.deleted_at.should_not be_nil + end + + it "is denied access to soft deleting another enterprises' variant" do + spree_delete :soft_delete, {variant_id: variant_other.to_param, product_id: product_other.to_param, format: :json} + assert_unauthorized! + lambda { variant.reload }.should_not raise_error + variant.deleted_at.should be_nil + end end context "as an administrator" do diff --git a/spec/models/spree/ability_spec.rb b/spec/models/spree/ability_spec.rb index e3d7581727..fc13925bae 100644 --- a/spec/models/spree/ability_spec.rb +++ b/spec/models/spree/ability_spec.rb @@ -48,7 +48,12 @@ module Spree end it "should be able to read/write their enterprises' product variants" do - should have_ability([:admin, :index, :read, :create, :edit, :search, :update, :destroy], for: Spree::Variant) + should have_ability([:create], for: Spree::Variant) + should have_ability([:admin, :index, :read, :create, :edit, :search, :update, :destroy], for: p1.master) + end + + it "should not be able to read/write other enterprises' product variants" do + should_not have_ability([:admin, :index, :read, :create, :edit, :search, :update, :destroy], for: p2.master) end it "should be able to read/write their enterprises' product properties" do From b649d6ef6913b3af83d46ed1690f28e12a5f584a Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 11 Apr 2014 13:36:26 +1000 Subject: [PATCH 13/22] Authorise access to OrderCycles API --- .../api/order_cycles_controller.rb | 3 +- .../api/order_cycles_controller_spec.rb | 38 +++++++++++++++---- 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/app/controllers/api/order_cycles_controller.rb b/app/controllers/api/order_cycles_controller.rb index c32568ef92..b4b3486778 100644 --- a/app/controllers/api/order_cycles_controller.rb +++ b/app/controllers/api/order_cycles_controller.rb @@ -2,6 +2,8 @@ module Api class OrderCyclesController < Spree::Api::BaseController respond_to :json def managed + authorize! :admin, OrderCycle + authorize! :read, OrderCycle @order_cycles = OrderCycle.ransack(params[:q]).result.managed_by(current_api_user) render params[:template] || :bulk_index end @@ -12,4 +14,3 @@ module Api end end end - \ No newline at end of file diff --git a/spec/controllers/api/order_cycles_controller_spec.rb b/spec/controllers/api/order_cycles_controller_spec.rb index 55a6ea096e..7f8bbec7af 100644 --- a/spec/controllers/api/order_cycles_controller_spec.rb +++ b/spec/controllers/api/order_cycles_controller_spec.rb @@ -6,15 +6,37 @@ module Api include Spree::Api::TestingSupport::Helpers render_views - context "as a normal user" do - let!(:oc1) { FactoryGirl.create(:order_cycle) } - let!(:oc2) { FactoryGirl.create(:order_cycle) } - let(:attributes) { [:id, :name, :suppliers, :distributors] } + let!(:oc1) { FactoryGirl.create(:order_cycle) } + let!(:oc2) { FactoryGirl.create(:order_cycle) } + let(:coordinator) { oc1.coordinator } + let(:attributes) { [:id, :name, :suppliers, :distributors] } - before do - stub_authentication! - Spree.user_class.stub :find_by_spree_api_key => current_api_user + before do + stub_authentication! + Spree.user_class.stub :find_by_spree_api_key => current_api_user + end + + context "as a normal user" do + sign_in_as_user! + + it "should deny me access to managed order cycles" do + spree_get :managed, { :format => :json } + assert_unauthorized! end + end + + context "as an enterprise user" do + sign_in_as_enterprise_user! [:coordinator] + + it "retrieves a list of variants with appropriate attributes" do + get :managed, { :format => :json } + keys = json_response.first.keys.map{ |key| key.to_sym } + attributes.all?{ |attr| keys.include? attr }.should == true + end + end + + context "as an administrator" do + sign_in_as_admin! it "retrieves a list of variants with appropriate attributes" do get :managed, { :format => :json } @@ -89,4 +111,4 @@ module Api end end end -end \ No newline at end of file +end From 8dd9260163204b016f7bf1499a43bf982e420497 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 11 Apr 2014 16:55:54 +1000 Subject: [PATCH 14/22] Do not add dummy distributor to orders without one - they don't need it --- db/migrate/20140402033428_add_foreign_keys.rb | 6 ------ 1 file changed, 6 deletions(-) diff --git a/db/migrate/20140402033428_add_foreign_keys.rb b/db/migrate/20140402033428_add_foreign_keys.rb index 9f4349d485..03d384c244 100644 --- a/db/migrate/20140402033428_add_foreign_keys.rb +++ b/db/migrate/20140402033428_add_foreign_keys.rb @@ -36,7 +36,6 @@ class AddForeignKeys < ActiveRecord::Migration orphaned_inventory_units = Spree::InventoryUnit.joins('LEFT OUTER JOIN spree_variants ON spree_variants.id=spree_inventory_units.variant_id').where('spree_variants.id IS NULL') say "Destroying #{orphaned_inventory_units.count} orphaned InventoryUnits (of total #{Spree::InventoryUnit.count})" orphaned_inventory_units.destroy_all - # TODO: Need a dependent destroy for variant -> inventory unit? # Remove orphaned Spree::LineItems orphaned_line_items = Spree::LineItem. @@ -49,13 +48,8 @@ class AddForeignKeys < ActiveRecord::Migration # Give all orders a distributor address = Spree::Address.create!(firstname: 'Dummy distributor', lastname: 'Dummy distributor', phone: '12345678', state: Spree::State.first, address1: 'Dummy distributor', city: 'Dummy distributor', zipcode: '1234', country: Spree::State.first.country) - no_distributor = Enterprise.create!(name: "No distributor", address: address) deleted_distributor = Enterprise.create!(name: "Deleted distributor", address: address) - orders = Spree::Order.where(distributor_id: nil) - say "Assigning a dummy distributor to #{orders.count} orders which lack one (of total #{Spree::Order.count})" - orders.update_all distributor_id: no_distributor.id - orphaned_orders = Spree::Order.joins('LEFT OUTER JOIN enterprises ON enterprises.id=spree_orders.distributor_id').where('enterprises.id IS NULL') say "Assigning a dummy distributor to #{orphaned_orders.count} orders with a deleted distributor (of total #{Spree::Order.count})" orphaned_orders.update_all distributor_id: deleted_distributor.id From d0585b4d0532a9c252faa8e1eae076283df7efc7 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 24 Apr 2014 14:43:33 +1000 Subject: [PATCH 15/22] Admin order cycle listing does not show order cycles that enterprise users don't have access to --- app/models/order_cycle.rb | 6 ++++-- app/views/admin/order_cycles/index.html.haml | 4 ++-- spec/features/admin/order_cycles_spec.rb | 11 ++++++++--- 3 files changed, 14 insertions(+), 7 deletions(-) diff --git a/app/models/order_cycle.rb b/app/models/order_cycle.rb index 317c092375..915580d7d8 100644 --- a/app/models/order_cycle.rb +++ b/app/models/order_cycle.rb @@ -79,11 +79,13 @@ class OrderCycle < ActiveRecord::Base end def suppliers - self.exchanges.incoming.map(&:sender).uniq + enterprise_ids = self.exchanges.incoming.pluck :sender_id + Enterprise.where('enterprises.id IN (?)', enterprise_ids) end def distributors - self.exchanges.outgoing.map(&:receiver).uniq + enterprise_ids = self.exchanges.outgoing.pluck :receiver_id + Enterprise.where('enterprises.id IN (?)', enterprise_ids) end def variants diff --git a/app/views/admin/order_cycles/index.html.haml b/app/views/admin/order_cycles/index.html.haml index 328d56d5aa..0e829e025c 100644 --- a/app/views/admin/order_cycles/index.html.haml +++ b/app/views/admin/order_cycles/index.html.haml @@ -37,12 +37,12 @@ %td= order_cycle_form.text_field :orders_open_at, :class => 'datetimepicker', :value => order_cycle.orders_open_at %td= order_cycle_form.text_field :orders_close_at, :class => 'datetimepicker', :value => order_cycle.orders_close_at %td.suppliers - - order_cycle.suppliers.each do |s| + - order_cycle.suppliers.managed_by(spree_current_user).each do |s| = s.name %br/ %td= order_cycle.coordinator.name %td.distributors - - order_cycle.distributors.each do |d| + - order_cycle.distributors.managed_by(spree_current_user).each do |d| = d.name %br/ diff --git a/spec/features/admin/order_cycles_spec.rb b/spec/features/admin/order_cycles_spec.rb index fdd5c009d4..0244bc434e 100644 --- a/spec/features/admin/order_cycles_spec.rb +++ b/spec/features/admin/order_cycles_spec.rb @@ -462,17 +462,22 @@ feature %q{ login_to_admin_as @new_user end - scenario "can view products I am coordinating" do - oc_user_coordinating = create(:simple_order_cycle, { coordinator: supplier1, name: 'Order Cycle 1' } ) + scenario "viewing a list of order cycles I am coordinating" do + oc_user_coordinating = create(:simple_order_cycle, { suppliers: [supplier1, supplier2], coordinator: supplier1, distributors: [distributor1, distributor2], name: 'Order Cycle 1' } ) oc_for_other_user = create(:simple_order_cycle, { coordinator: supplier2, name: 'Order Cycle 2' } ) click_link "Order Cycles" + # I should see only the order cycle I am coordinating page.should have_content oc_user_coordinating.name page.should_not have_content oc_for_other_user.name + + # The order cycle should not show enterprises that I don't manage + page.should_not have_selector 'td.suppliers', text: supplier2.name + page.should_not have_selector 'td.distributors', text: distributor2.name end - scenario "can create a new order cycle" do + scenario "creating a new order cycle" do click_link "Order Cycles" click_link 'New Order Cycle' From 2603256a17152d487ea0ecdef8a0ce34009db154 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 24 Apr 2014 14:53:32 +1000 Subject: [PATCH 16/22] Extract admin order cycle index row into partial --- app/views/admin/order_cycles/_row.html.haml | 25 ++++++++++++++++++ app/views/admin/order_cycles/index.html.haml | 27 +++----------------- 2 files changed, 28 insertions(+), 24 deletions(-) create mode 100644 app/views/admin/order_cycles/_row.html.haml diff --git a/app/views/admin/order_cycles/_row.html.haml b/app/views/admin/order_cycles/_row.html.haml new file mode 100644 index 0000000000..4396fabcba --- /dev/null +++ b/app/views/admin/order_cycles/_row.html.haml @@ -0,0 +1,25 @@ +- order_cycle = order_cycle_form.object +- klass = "order-cycle-#{order_cycle.id} #{order_cycle_status_class order_cycle}" +%tr{class: klass} + %td= link_to order_cycle.name, main_app.edit_admin_order_cycle_path(order_cycle) + %td= order_cycle_form.text_field :orders_open_at, :class => 'datetimepicker', :value => order_cycle.orders_open_at + %td= order_cycle_form.text_field :orders_close_at, :class => 'datetimepicker', :value => order_cycle.orders_close_at + %td.suppliers + - order_cycle.suppliers.managed_by(spree_current_user).each do |s| + = s.name + %br/ + %td= order_cycle.coordinator.name + %td.distributors + - order_cycle.distributors.managed_by(spree_current_user).each do |d| + = d.name + %br/ + + %td.products + - variant_images = capture do + - order_cycle.variants.each do |v| + = image_tag(v.images.first.attachment.url(:mini)) if v.images.present? + %br/ + %span.with-tip{'data-powertip' => variant_images}= "#{order_cycle.variants.count} variants" + + %td.actions + = link_to '', main_app.clone_admin_order_cycle_path(order_cycle), class: 'clone-order-cycle icon-copy no-text' diff --git a/app/views/admin/order_cycles/index.html.haml b/app/views/admin/order_cycles/index.html.haml index 0e829e025c..c3f4bc0b8f 100644 --- a/app/views/admin/order_cycles/index.html.haml +++ b/app/views/admin/order_cycles/index.html.haml @@ -18,6 +18,7 @@ %col %col %col + %thead %tr %th Name @@ -28,31 +29,9 @@ %th Distributors %th Products %th.actions + %tbody = f.fields_for :collection do |order_cycle_form| - - order_cycle = order_cycle_form.object - - klass = "order-cycle-#{order_cycle.id} #{order_cycle_status_class order_cycle}" - %tr{class: klass} - %td= link_to order_cycle.name, main_app.edit_admin_order_cycle_path(order_cycle) - %td= order_cycle_form.text_field :orders_open_at, :class => 'datetimepicker', :value => order_cycle.orders_open_at - %td= order_cycle_form.text_field :orders_close_at, :class => 'datetimepicker', :value => order_cycle.orders_close_at - %td.suppliers - - order_cycle.suppliers.managed_by(spree_current_user).each do |s| - = s.name - %br/ - %td= order_cycle.coordinator.name - %td.distributors - - order_cycle.distributors.managed_by(spree_current_user).each do |d| - = d.name - %br/ + = render 'admin/order_cycles/row', order_cycle_form: order_cycle_form - %td.products - - variant_images = capture do - - order_cycle.variants.each do |v| - = image_tag(v.images.first.attachment.url(:mini)) if v.images.present? - %br/ - %span.with-tip{'data-powertip' => variant_images}= "#{order_cycle.variants.count} variants" - - %td.actions - = link_to '', main_app.clone_admin_order_cycle_path(order_cycle), class: 'clone-order-cycle icon-copy no-text' = f.submit 'Update' From 85db8859bbab3886760578f5dcbc55a9c457b7f3 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Thu, 24 Apr 2014 15:51:39 +1000 Subject: [PATCH 17/22] On admin order cycle edit page, do not show exchanges for enterprises the user doesn't manage --- app/models/exchange.rb | 13 +++++++++ app/views/admin/order_cycles/show.rep | 2 +- spec/features/admin/order_cycles_spec.rb | 14 +++++++-- spec/models/exchange_spec.rb | 37 ++++++++++++++++++++++++ 4 files changed, 63 insertions(+), 3 deletions(-) diff --git a/app/models/exchange.rb b/app/models/exchange.rb index 122586ba96..18c3f81447 100644 --- a/app/models/exchange.rb +++ b/app/models/exchange.rb @@ -28,6 +28,19 @@ class Exchange < ActiveRecord::Base scope :with_product, lambda { |product| joins(:exchange_variants).where('exchange_variants.variant_id IN (?)', product.variants_including_master) } + scope :managed_by, lambda { |user| + if user.has_spree_role?('admin') + scoped + else + joins('LEFT JOIN enterprises senders ON senders.id = exchanges.sender_id'). + joins('LEFT JOIN enterprises receivers ON receivers.id = exchanges.receiver_id'). + joins('LEFT JOIN enterprise_roles sender_roles ON sender_roles.enterprise_id = senders.id'). + joins('LEFT JOIN enterprise_roles receiver_roles ON receiver_roles.enterprise_id = receivers.id'). + where('sender_roles.user_id = ? AND receiver_roles.user_id = ?', user.id, user.id) + end + } + + def clone!(new_order_cycle) exchange = self.dup exchange.order_cycle = new_order_cycle diff --git a/app/views/admin/order_cycles/show.rep b/app/views/admin/order_cycles/show.rep index 1f91cbe3e9..04fc659813 100644 --- a/app/views/admin/order_cycles/show.rep +++ b/app/views/admin/order_cycles/show.rep @@ -9,7 +9,7 @@ r.element :order_cycle, @order_cycle do r.element :id end - r.list_of :exchanges, @order_cycle.exchanges.order('id ASC') do |exchange| + r.list_of :exchanges, @order_cycle.exchanges.managed_by(spree_current_user).order('id ASC') do |exchange| r.element :id r.element :sender_id r.element :receiver_id diff --git a/spec/features/admin/order_cycles_spec.rb b/spec/features/admin/order_cycles_spec.rb index 0244bc434e..7d5ad9a4ec 100644 --- a/spec/features/admin/order_cycles_spec.rb +++ b/spec/features/admin/order_cycles_spec.rb @@ -217,11 +217,11 @@ feature %q{ end # And the distributors should have fees - distributor = oc.distributors.sort_by(&:name).first + distributor = oc.distributors.sort_by(&:id).first page.should have_select 'order_cycle_outgoing_exchange_0_enterprise_fees_0_enterprise_id', selected: distributor.name page.should have_select 'order_cycle_outgoing_exchange_0_enterprise_fees_0_enterprise_fee_id', selected: distributor.enterprise_fees.first.name - distributor = oc.distributors.sort_by(&:name).last + distributor = oc.distributors.sort_by(&:id).last page.should have_select 'order_cycle_outgoing_exchange_1_enterprise_fees_0_enterprise_id', selected: distributor.name page.should have_select 'order_cycle_outgoing_exchange_1_enterprise_fees_0_enterprise_fee_id', selected: distributor.enterprise_fees.first.name end @@ -515,6 +515,16 @@ feature %q{ order_cycle.coordinator.should == distributor1 end + scenario "editing an order cycle" do + oc = create(:simple_order_cycle, { suppliers: [supplier1, supplier2], coordinator: supplier1, distributors: [distributor1, distributor2], name: 'Order Cycle 1' } ) + + visit edit_admin_order_cycle_path(oc) + + # I should not see exchanges for supplier2 or distributor2 + page.all('tr.supplier').count.should == 1 + page.all('tr.distributor').count.should == 1 + end + scenario "cloning an order cycle" do oc = create(:simple_order_cycle) diff --git a/spec/models/exchange_spec.rb b/spec/models/exchange_spec.rb index 9614a4d357..ed6cdbfc93 100644 --- a/spec/models/exchange_spec.rb +++ b/spec/models/exchange_spec.rb @@ -85,6 +85,43 @@ describe Exchange do let(:distributor) { create(:distributor_enterprise) } let(:oc) { create(:simple_order_cycle, coordinator: coordinator) } + describe "finding exchanges managed by a particular user" do + let(:user) do + user = create(:user) + user.spree_roles = [] + user + end + + before { Exchange.destroy_all } + + it "returns exchanges where the user manages both the sender and the receiver" do + exchange = create(:exchange, order_cycle: oc) + exchange.sender.users << user + exchange.receiver.users << user + + Exchange.managed_by(user).should == [exchange] + end + + it "does not return exchanges where the user manages only the sender" do + exchange = create(:exchange, order_cycle: oc) + exchange.sender.users << user + + Exchange.managed_by(user).should be_empty + end + + it "does not return exchanges where the user manages only the receiver" do + exchange = create(:exchange, order_cycle: oc) + exchange.receiver.users << user + + Exchange.managed_by(user).should be_empty + end + + it "does not return exchanges where the user manages neither enterprise" do + exchange = create(:exchange, order_cycle: oc) + Exchange.managed_by(user).should be_empty + end + end + it "finds exchanges in a particular order cycle" do ex = create(:exchange, order_cycle: oc) Exchange.in_order_cycle(oc).should == [ex] From c35112a40c793ad68bcd10a48a6c9afddbc73d4b Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 25 Apr 2014 16:18:30 +1000 Subject: [PATCH 18/22] Migration works without countries or states (ie. in CI) --- db/migrate/20140402033428_add_foreign_keys.rb | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/db/migrate/20140402033428_add_foreign_keys.rb b/db/migrate/20140402033428_add_foreign_keys.rb index 03d384c244..f072127158 100644 --- a/db/migrate/20140402033428_add_foreign_keys.rb +++ b/db/migrate/20140402033428_add_foreign_keys.rb @@ -45,9 +45,16 @@ class AddForeignKeys < ActiveRecord::Migration say "Destroying #{orphaned_line_items.count} orphaned LineItems (of total #{Spree::LineItem.count})" orphaned_line_items.each { |li| li.delete } - # Give all orders a distributor - address = Spree::Address.create!(firstname: 'Dummy distributor', lastname: 'Dummy distributor', phone: '12345678', state: Spree::State.first, - address1: 'Dummy distributor', city: 'Dummy distributor', zipcode: '1234', country: Spree::State.first.country) + # Update orders without a distributor with a dummy distributor + state = Spree::State.first + country = state.andand.country + unless country && state + country = Spree::Country.create! name: 'Australia', iso_name: 'AU' + state = country.states.create! name: 'Victoria' + end + + address = Spree::Address.create!(firstname: 'Dummy distributor', lastname: 'Dummy distributor', phone: '12345678', state: state, + address1: 'Dummy distributor', city: 'Dummy distributor', zipcode: '1234', country: country) deleted_distributor = Enterprise.create!(name: "Deleted distributor", address: address) orphaned_orders = Spree::Order.joins('LEFT OUTER JOIN enterprises ON enterprises.id=spree_orders.distributor_id').where('enterprises.id IS NULL') From 1610e1448a9ad659064e0d151911a71dfe98d849 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 25 Apr 2014 16:58:25 +1000 Subject: [PATCH 19/22] In specs, correctly disable referential integrity for postgres --- config/environments/test.rb | 2 ++ .../foreign_keys_postgresql.rb | 22 +++++++++++++++++++ 2 files changed, 24 insertions(+) create mode 100644 lib/open_food_network/foreign_keys_postgresql.rb diff --git a/config/environments/test.rb b/config/environments/test.rb index e1291ab4b4..5848cbc22e 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -1,3 +1,5 @@ +require 'open_food_network/foreign_keys_postgresql' + Openfoodnetwork::Application.configure do # Settings specified here will take precedence over those in config/application.rb diff --git a/lib/open_food_network/foreign_keys_postgresql.rb b/lib/open_food_network/foreign_keys_postgresql.rb new file mode 100644 index 0000000000..4a4d06fd3d --- /dev/null +++ b/lib/open_food_network/foreign_keys_postgresql.rb @@ -0,0 +1,22 @@ +# http://kopongo.com/2008/7/25/postgres-ri_constrainttrigger-error + +if Rails::VERSION::MAJOR < 4 + # Fix fixtures with foreign keys, fixed in Rails 4 + + module ActiveRecord + module ConnectionAdapters + class PostgreSQLAdapter < AbstractAdapter + def disable_referential_integrity(&block) + transaction { + begin + execute "SET CONSTRAINTS ALL DEFERRED" + yield + ensure + execute "SET CONSTRAINTS ALL IMMEDIATE" + end + } + end + end + end + end +end From ad6021a11677e1f8e2b3be45392fa3a78ae0ef9f Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 25 Apr 2014 17:14:25 +1000 Subject: [PATCH 20/22] Revert "In specs, correctly disable referential integrity for postgres" This reverts commit 1610e1448a9ad659064e0d151911a71dfe98d849. --- config/environments/test.rb | 2 -- .../foreign_keys_postgresql.rb | 22 ------------------- 2 files changed, 24 deletions(-) delete mode 100644 lib/open_food_network/foreign_keys_postgresql.rb diff --git a/config/environments/test.rb b/config/environments/test.rb index 5848cbc22e..e1291ab4b4 100644 --- a/config/environments/test.rb +++ b/config/environments/test.rb @@ -1,5 +1,3 @@ -require 'open_food_network/foreign_keys_postgresql' - Openfoodnetwork::Application.configure do # Settings specified here will take precedence over those in config/application.rb diff --git a/lib/open_food_network/foreign_keys_postgresql.rb b/lib/open_food_network/foreign_keys_postgresql.rb deleted file mode 100644 index 4a4d06fd3d..0000000000 --- a/lib/open_food_network/foreign_keys_postgresql.rb +++ /dev/null @@ -1,22 +0,0 @@ -# http://kopongo.com/2008/7/25/postgres-ri_constrainttrigger-error - -if Rails::VERSION::MAJOR < 4 - # Fix fixtures with foreign keys, fixed in Rails 4 - - module ActiveRecord - module ConnectionAdapters - class PostgreSQLAdapter < AbstractAdapter - def disable_referential_integrity(&block) - transaction { - begin - execute "SET CONSTRAINTS ALL DEFERRED" - yield - ensure - execute "SET CONSTRAINTS ALL IMMEDIATE" - end - } - end - end - end - end -end From c8f50216ab92da086500c696bae878063a523982 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 25 Apr 2014 18:06:09 +1000 Subject: [PATCH 21/22] Increase default_wait_time on order_cycles_spec in an attempt to prevent inconsistent failures in CI --- spec/features/admin/order_cycles_spec.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/spec/features/admin/order_cycles_spec.rb b/spec/features/admin/order_cycles_spec.rb index 7d5ad9a4ec..bc9be6c298 100644 --- a/spec/features/admin/order_cycles_spec.rb +++ b/spec/features/admin/order_cycles_spec.rb @@ -9,7 +9,7 @@ feature %q{ before :all do @orig_default_wait_time = Capybara.default_wait_time - Capybara.default_wait_time = 5 + Capybara.default_wait_time = 10 end after :all do From 126ddd0a7580ea6f655b33837ce409e66b63e4b2 Mon Sep 17 00:00:00 2001 From: Rohan Mitchell Date: Fri, 25 Apr 2014 18:59:33 +1000 Subject: [PATCH 22/22] Add explicit wait before inconsistently failing lookup --- spec/features/admin/order_cycles_spec.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/spec/features/admin/order_cycles_spec.rb b/spec/features/admin/order_cycles_spec.rb index bc9be6c298..2eff8508c7 100644 --- a/spec/features/admin/order_cycles_spec.rb +++ b/spec/features/admin/order_cycles_spec.rb @@ -295,6 +295,7 @@ feature %q{ click_button 'Add supplier' page.all("table.exchanges tr.supplier td.products input").each { |e| e.click } + wait_until { page.find("#order_cycle_incoming_exchange_1_variants_#{initial_variants.last.id}", visible: true).present? } page.find("#order_cycle_incoming_exchange_1_variants_#{initial_variants.last.id}", visible: true).click # uncheck (with visible:true filter) check "order_cycle_incoming_exchange_2_variants_#{v1.id}" check "order_cycle_incoming_exchange_2_variants_#{v2.id}"