From 5ae0ad87a7812d6b99dbd0fae457f2b4ad8ca1a1 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Sat, 20 Oct 2018 11:12:00 +0100 Subject: [PATCH 1/4] Refactor EnterpriseRelationship before save hook: some renames and extract methods --- app/models/enterprise_relationship.rb | 25 +++++++++++++++++-------- 1 file changed, 17 insertions(+), 8 deletions(-) diff --git a/app/models/enterprise_relationship.rb b/app/models/enterprise_relationship.rb index 1f87b8737b..c5806cae8c 100644 --- a/app/models/enterprise_relationship.rb +++ b/app/models/enterprise_relationship.rb @@ -6,7 +6,7 @@ class EnterpriseRelationship < ActiveRecord::Base validates_presence_of :parent_id, :child_id validates_uniqueness_of :child_id, scope: :parent_id, message: I18n.t('validation_msg_relationship_already_established') - after_save :apply_variant_override_permissions + after_save :update_permissions_of_child_variant_overrides scope :with_enterprises, joins('LEFT JOIN enterprises AS parent_enterprises ON parent_enterprises.id = enterprise_relationships.parent_id'). @@ -26,7 +26,6 @@ class EnterpriseRelationship < ActiveRecord::Base scope :by_name, with_enterprises.order('child_enterprises.name, parent_enterprises.name') - # Load an array of the relatives of each enterprise (ie. any enterprise related to it in # either direction). This array is split into distributors and producers, and has the format: # {enterprise_id => {distributors: [id, ...], producers: [id, ...]} } @@ -76,14 +75,24 @@ class EnterpriseRelationship < ActiveRecord::Base private - def apply_variant_override_permissions - variant_overrides = VariantOverride.unscoped.for_hubs(child) - .joins(variant: :product).where("spree_products.supplier_id IN (?)", parent) - + def update_permissions_of_child_variant_overrides if has_permission?(:create_variant_overrides) - variant_overrides.update_all(permission_revoked_at: nil) + allow_all_child_variant_overrides else - variant_overrides.update_all(permission_revoked_at: Time.now) + revoke_all_child_variant_overrides end end + + def allow_all_child_variant_overrides + child_variant_overrides.update_all(permission_revoked_at: nil) + end + + def revoke_all_child_variant_overrides + child_variant_overrides.update_all(permission_revoked_at: Time.now) + end + + def child_variant_overrides + VariantOverride.unscoped.for_hubs(child) + .joins(variant: :product).where("spree_products.supplier_id IN (?)", parent) + end end From 9079437284aab3b83895b8c5625dca0189b00118 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Sat, 20 Oct 2018 11:12:53 +0100 Subject: [PATCH 2/4] Add before_destroy to enterprise_relationship so that variant overrides are revoked when permission is deleted --- app/models/enterprise_relationship.rb | 1 + spec/models/enterprise_relationship_spec.rb | 13 ++++++++++++- 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/app/models/enterprise_relationship.rb b/app/models/enterprise_relationship.rb index c5806cae8c..7af3d180a3 100644 --- a/app/models/enterprise_relationship.rb +++ b/app/models/enterprise_relationship.rb @@ -7,6 +7,7 @@ class EnterpriseRelationship < ActiveRecord::Base validates_uniqueness_of :child_id, scope: :parent_id, message: I18n.t('validation_msg_relationship_already_established') after_save :update_permissions_of_child_variant_overrides + before_destroy :revoke_all_child_variant_overrides scope :with_enterprises, joins('LEFT JOIN enterprises AS parent_enterprises ON parent_enterprises.id = enterprise_relationships.parent_id'). diff --git a/spec/models/enterprise_relationship_spec.rb b/spec/models/enterprise_relationship_spec.rb index 5dbdf1f3be..650e4f1fa3 100644 --- a/spec/models/enterprise_relationship_spec.rb +++ b/spec/models/enterprise_relationship_spec.rb @@ -140,7 +140,7 @@ describe EnterpriseRelationship do end describe "callbacks" do - context "applying variant override permissions" do + context "updating variant override permissions" do let(:hub) { create(:distributor_enterprise) } let(:producer) { create(:supplier_enterprise) } let(:some_other_producer) { create(:supplier_enterprise) } @@ -152,6 +152,17 @@ describe EnterpriseRelationship do let!(:vo2) { create(:variant_override, hub: hub, variant: create(:variant, product: create(:product, supplier: producer))) } let!(:vo3) { create(:variant_override, hub: hub, variant: create(:variant, product: create(:product, supplier: some_other_producer))) } + context "revoking variant override permissions" do + context "when the enterprise relationship is destroyed" do + before { er.destroy } + it "should set permission_revoked_at to the current time for all variant overrides of the relationship" do + expect(vo1.reload.permission_revoked_at).to_not be_nil + expect(vo2.reload.permission_revoked_at).to_not be_nil + expect(vo2.reload.permission_revoked_at).to_not be_nil + end + end + end + context "and is then removed" do before { er.permissions_list = [:add_to_order_cycles]; er.save! } it "should set permission_revoked_at to the current time for all relevant variant overrides" do From d375bb8c556950cdf56ebb704dc374e579331613 Mon Sep 17 00:00:00 2001 From: luisramos0 Date: Sat, 20 Oct 2018 11:48:23 +0100 Subject: [PATCH 3/4] Migration: Revoke variant overrides without permissions --- ...voke_variant_overrideswithout_permissions.rb | 17 +++++++++++++++++ db/schema.rb | 2 +- 2 files changed, 18 insertions(+), 1 deletion(-) create mode 100644 db/migrate/20181020103501_revoke_variant_overrideswithout_permissions.rb diff --git a/db/migrate/20181020103501_revoke_variant_overrideswithout_permissions.rb b/db/migrate/20181020103501_revoke_variant_overrideswithout_permissions.rb new file mode 100644 index 0000000000..bd2d0e750b --- /dev/null +++ b/db/migrate/20181020103501_revoke_variant_overrideswithout_permissions.rb @@ -0,0 +1,17 @@ +class RevokeVariantOverrideswithoutPermissions < ActiveRecord::Migration + def up + # This process was executed when the permission_revoked_at colum was created (see AddPermissionRevokedAtToVariantOverrides) + # It needs to be repeated due to #2739 + variant_override_hubs = Enterprise.where(id: VariantOverride.all.map(&:hub_id).uniq) + + variant_override_hubs.each do |hub| + permitting_producer_ids = hub.relationships_as_child + .with_permission(:create_variant_overrides).map(&:parent_id) + + variant_overrides_with_revoked_permissions = VariantOverride.for_hubs(hub) + .joins(variant: :product).where("spree_products.supplier_id NOT IN (?)", permitting_producer_ids) + + variant_overrides_with_revoked_permissions.update_all(permission_revoked_at: Time.now) + end + end +end diff --git a/db/schema.rb b/db/schema.rb index c29fa95b9e..3e60568a4a 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 => 20181010093850) do +ActiveRecord::Schema.define(:version => 20181020103501) do create_table "account_invoices", :force => true do |t| t.integer "user_id", :null => false From dc5302ca0896394c4020b46f82102d6465f4ca54 Mon Sep 17 00:00:00 2001 From: Maikel Linke Date: Thu, 25 Oct 2018 11:33:14 +1100 Subject: [PATCH 4/4] Speed up database queries and make them scale This commit makes use of three ActiveRecord features: 1. Using `select` instead of `all.map` enables ActiveRecord to nest one select into the other, resulting in one more efficient query instead of two. 2. Using `find_each` saves memory by loading records in batches. https://api.rubyonrails.org/classes/ActiveRecord/Batches.html#method-i-find_each 3. Using `pluck` creates only an array, avoiding loading all the other columns of the records into objects. Running this on the current Canadian database, fixes the following variant overrides: ``` [] [] [] [] [] [] [925, 924, 966, 965] [] [] [] [] [462, 863, 464, 822, 949, 947, 944, 939, 942, 946, 945, 943, 438, 937, 938, 941, 940, 467, 952, 875, 453, 953, 454, 951, 487, 460, 457, 528, 527, 486, 459, 458, 461, 529, 530, 950, 642, 384, 380, 643, 385, 381, 644, 386, 382, 960, 959, 379, 640, 377, 375, 532, 639, 376, 374, 646, 390, 389, 637, 406, 408, 647, 391, 393, 633, 396, 400, 398, 645, 388, 387, 648, 394, 392, 536, 632, 399, 397, 395, 634, 403, 401, 635, 404, 402, 636, 407, 405, 535, 534, 638, 410, 409, 948, 533, 537, 531, 877, 880, 894, 893, 672, 671, 673, 674, 703, 714, 715, 716, 717, 862, 864, 879, 876, 865, 881, 878, 463, 954, 866, 823, 957, 958, 955, 956, 899, 897] [] [969] ``` --- ...020103501_revoke_variant_overrideswithout_permissions.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/db/migrate/20181020103501_revoke_variant_overrideswithout_permissions.rb b/db/migrate/20181020103501_revoke_variant_overrideswithout_permissions.rb index bd2d0e750b..d8140c9bf3 100644 --- a/db/migrate/20181020103501_revoke_variant_overrideswithout_permissions.rb +++ b/db/migrate/20181020103501_revoke_variant_overrideswithout_permissions.rb @@ -2,11 +2,11 @@ class RevokeVariantOverrideswithoutPermissions < ActiveRecord::Migration def up # This process was executed when the permission_revoked_at colum was created (see AddPermissionRevokedAtToVariantOverrides) # It needs to be repeated due to #2739 - variant_override_hubs = Enterprise.where(id: VariantOverride.all.map(&:hub_id).uniq) + variant_override_hubs = Enterprise.where(id: VariantOverride.select(:hub_id).uniq) - variant_override_hubs.each do |hub| + variant_override_hubs.find_each do |hub| permitting_producer_ids = hub.relationships_as_child - .with_permission(:create_variant_overrides).map(&:parent_id) + .with_permission(:create_variant_overrides).pluck(:parent_id) variant_overrides_with_revoked_permissions = VariantOverride.for_hubs(hub) .joins(variant: :product).where("spree_products.supplier_id NOT IN (?)", permitting_producer_ids)