Merge pull request openfoodfoundation#2893 from luisramos0/deleted_pr…

…oducts_break_inventory

Fix bug in inventory management page

app/models/enterprise_relationship.rb

@@ -6,7 +6,8 @@ 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
+  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').
@@ -26,7 +27,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 +76,24 @@ def has_permission?(name)
 
   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

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.select(:hub_id).uniq)
+
+    variant_override_hubs.find_each do |hub|
+      permitting_producer_ids = hub.relationships_as_child
+        .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)
+
+      variant_overrides_with_revoked_permissions.update_all(permission_revoked_at: Time.now)
+    end
+  end
+end

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

spec/models/enterprise_relationship_spec.rb

@@ -140,7 +140,7 @@
   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 @@
         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