destroy handled

Co-authored-by: Arnaud Levy <arnaud.levy@noesya.coop>
Co-authored-by: Pierre-André Boissinot <pierreandre.boissinot@noesya.coop>

app/controllers/admin/communication/websites/categories_controller.rb

@@ -77,7 +77,7 @@ class Admin::Communication::Websites::CategoriesController < Admin::Communicatio
   end
 
   def destroy
-    @category.destroy_and_sync
+    @category.destroy
     redirect_to admin_communication_website_categories_url, notice: t('admin.successfully_destroyed_html', model: @category.to_s)
   end
 

app/controllers/admin/communication/websites/menus_controller.rb

@@ -50,7 +50,7 @@ class Admin::Communication::Websites::MenusController < Admin::Communication::We
   end
 
   def destroy
-    @menu.destroy_and_sync
+    @menu.destroy
     redirect_to admin_communication_website_menus_url, notice: t('admin.successfully_destroyed_html', model: @menu.to_s)
   end
 

app/controllers/admin/communication/websites/pages_controller.rb

@@ -103,7 +103,7 @@ class Admin::Communication::Websites::PagesController < Admin::Communication::We
     if @page.is_special_page?
       redirect_back(fallback_location: admin_communication_website_page_path(@page), alert: t('admin.communication.website.pages.delete_special_page_notice'))
     else
-      @page.destroy_and_sync
+      @page.destroy
       redirect_to admin_communication_website_pages_url(@website), notice: t('admin.successfully_destroyed_html', model: @page.to_s)
     end
   end

app/controllers/admin/communication/websites/posts_controller.rb

@@ -92,7 +92,7 @@ class Admin::Communication::Websites::PostsController < Admin::Communication::We
   end
 
   def destroy
-    @post.destroy_and_sync
+    @post.destroy
     redirect_to admin_communication_website_posts_url, notice: t('admin.successfully_destroyed_html', model: @post.to_s)
   end
 

app/models/communication/website/git_file.rb

@@ -28,7 +28,7 @@ class Communication::Website::GitFile < ApplicationRecord
 
   attr_accessor :will_be_destroyed
 
-  def self.sync(website, object, destroy: false)
+  def self.sync(website, object)
     # All exportable objects must respond to this method
     # WithGitFiles defines it
     # AsDirectObject includes WithGitFiles, therefore all direct objects are exportable
@@ -44,8 +44,6 @@ class Communication::Website::GitFile < ApplicationRecord
     analyze_if_blob object
     # The git file might exist or not
     git_file = where(website: website, about: object).first_or_create
-    # Mark for destruction if necessary
-    git_file.will_be_destroyed = destroy
     # It is very important to go through this specific instance of the website,
     # and not through each git_file.website, which would be different instances.
     # Otherwise, we get 1 instance of git_repository per git_file,
@@ -53,8 +51,15 @@ class Communication::Website::GitFile < ApplicationRecord
     website.git_repository.add_git_file git_file
   end
 
+  # Simplified version of the sync method to simply delete an obsolete git_file
+  # Not an instance method because we need to share the website's instance, and thus pass it as an argument
+  def self.mark_for_destruction(website, git_file)
+    git_file.will_be_destroyed = true
+    website.git_repository.add_git_file git_file
+  end
+
   def path
-    @path ||= about.git_path(website)&.gsub(/\/+/, '/')
+    @path ||= about.nil? ? nil : about.git_path(website)&.gsub(/\/+/, '/')
   end
 
   def to_s

app/models/communication/website/with_git_repository.rb

@@ -15,10 +15,10 @@ module Communication::Website::WithGitRepository
   def destroy_obsolete_git_files
     website_git_files.find_each do |git_file|
       dependency = git_file.about
-      is_obsolete = !dependency.in?(recursive_dependencies_syncable)
+      # Here, dependency can be nil (object was previously destroyed)
+      is_obsolete = dependency.nil? || !dependency.in?(recursive_dependencies_syncable)
       if is_obsolete
-        # TODO git_file.destroy serait plus ActiveRecord
-        Communication::Website::GitFile.sync(self, dependency, destroy: true)
+        Communication::Website::GitFile.mark_for_destruction(self, git_file)
       end
     end
     self.git_repository.sync!

app/models/concerns/as_indirect_object.rb

@@ -47,28 +47,6 @@ module AsIndirectObject
     end
   end
 
-  def destroy
-    # On est obligés d'overwrite la méthode destroy pour éviter un problème d'œuf et de poule.
-    # On a besoin que les websites puissent recalculer leurs recursive_dependencies
-    # et on a besoin que ces recursive_dependencies n'incluent pas l'objet courant, puisqu'il est "en cours de destruction" (ni ses propres recursive_dependencies).
-    # Mais si on détruit juste l'objet et qu'on fait un `after_destroy :clean_website_connections`
-    # on ne peut plus accéder aux websites (puisque l'objet est déjà détruit et ses connexions en cascades).
-    # Donc :
-    # 1. on stocke les websites
-    # 2. PUIS on détruit les connexions
-    # 3. PUIS on détruit l'objet (la méthode destroy normale)
-    # 4. PUIS on demande aux websites stockés de nettoyer leurs connexions
-    self.transaction do
-      website_ids = websites.pluck(:id)
-      connections.destroy_all
-      super
-      Communication::Website.where(id: website_ids).each do |website|
-        website.destroy_obsolete_connections
-        website.save_and_sync
-      end
-    end
-  end
-
   protected
 
   def direct_sources_from_existing_connections

app/models/concerns/with_dependencies.rb

@@ -11,10 +11,28 @@ module WithDependencies
     if self < ActiveRecord::Base
       before_save :snapshot_dependencies
       after_save :clean_websites_if_necessary
-      after_destroy :clean_websites
     end
   end
 
+  def destroy
+    # On est obligés d'overwrite la méthode destroy pour éviter un problème d'œuf et de poule.
+    # On a besoin que les websites puissent recalculer leurs recursive_dependencies
+    # et on a besoin que ces recursive_dependencies n'incluent pas l'objet courant, puisqu'il est "en cours de destruction" (ni ses propres recursive_dependencies).
+    # Mais si on détruit juste l'objet et qu'on fait un `after_destroy :clean_website_connections`
+    # on ne peut plus accéder aux websites (puisque l'objet est déjà détruit et ses connexions en cascades).
+    # Donc :
+    # 1. on stocke les websites
+    # 2. on laisse la méthode destroy normale faire son travail
+    # 3. PUIS on demande aux websites stockés de nettoyer leurs connexions et leurs git files
+    self.transaction do
+      website_ids = websites_to_clean.pluck(:id)
+      super
+      clean_websites(Communication::Website.where(id: website_ids))
+      # TODO: Actuellement, on ne nettoie pas les références
+      # Exemple : Quand on supprime un auteur, il n'est pas nettoyé dans le static de ses anciens posts.
+      # Un save du website le fera en nocturne pour l'instant.
+    end
+  end
 
   # Cette méthode doit être définie dans chaque objet,
   # et renvoyer un tableau de ses références directes.
@@ -80,13 +98,13 @@ module WithDependencies
     # puts "  recursive_dependencies_syncable #{ reloaded_recursive_dependencies_syncable_filtered }"
     # puts "  missing_dependencies_after_save #{ missing_dependencies_after_save }"
     # puts
-    clean_websites if missing_dependencies_after_save.any?
+    clean_websites(websites_to_clean) if missing_dependencies_after_save.any?
   end
 
-  def clean_websites
+  def clean_websites(websites)
     # Les objets directs et les objets indirects (et les websites) répondent !
     return unless respond_to?(:is_direct_object?)
-    websites_to_clean.each do |website|
+    websites.each do |website|
       website.destroy_obsolete_connections
       website.destroy_obsolete_git_files
     end

app/models/concerns/with_git.rb

@@ -23,11 +23,6 @@ module WithGit
     end
   end
 
-  def destroy_and_sync
-    destroy_from_git
-    destroy
-  end
-
   def sync_with_git
     return unless website.git_repository.valid?
     if syncable?
@@ -43,10 +38,4 @@ module WithGit
   end
   handle_asynchronously :sync_with_git, queue: 'default'
 
-  def destroy_from_git
-    return unless website.git_repository.valid?
-    Communication::Website::GitFile.sync website, self, destroy: true
-    website.git_repository.sync!
-  end
-
 end

app/models/concerns/with_git_files.rb

@@ -4,8 +4,7 @@ module WithGitFiles
   included do
     has_many  :git_files,
               class_name: "Communication::Website::GitFile",
-              as: :about,
-              dependent: :destroy
+              as: :about
   end
 
   def git_path(website)

app/services/git/repository.rb

@@ -10,7 +10,8 @@ class Git::Repository
     if git_files.empty?
       analyzer.git_file = git_file
       action = analyzer.should_destroy? ? "Destroy" : "Save"
-      @commit_message = "[#{ git_file.about.class.name }] #{ action } #{ git_file.about }"
+      @commit_message = git_file.about.nil? ? "[#{ git_file.class.name }] #{ action } Git file"
+                                            : "[#{ git_file.about.class.name }] #{ action } #{ git_file.about }"
     end
     git_files << git_file
   end
@@ -79,15 +80,13 @@ class Git::Repository
     git_files.each do |git_file|
       analyzer.git_file = git_file
       if analyzer.should_destroy?
-        path = nil
-        sha = nil
+        puts "Destroying #{git_file.previous_path}"
+        git_file.destroy
       else
         path = git_file.path
-        sha = computed_sha(git_file.to_s)
+        puts "Marking #{path}"
+        git_file.update previous_path: path, previous_sha: computed_sha(git_file.to_s)
       end
-      puts "Marking #{path}"
-      git_file.update previous_path: path,
-                      previous_sha: sha
     end
   end
 end

config/initializers/active_storage.rb

@@ -40,7 +40,9 @@ Rails.application.config.to_prepare do
     extend ActiveSupport::Concern
 
     included do
-      has_many :git_files, class_name: "Communication::Website::GitFile", as: :about, dependent: :destroy
+      has_many  :git_files,
+                class_name: "Communication::Website::GitFile",
+                as: :about
     end
 
     def git_path(website)