From eaaeef30a80e2259bd560e877e8c281211e8006d Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?S=C3=A9bastien=20Gaya?= <sebastien.gaya@gmail.com>
Date: Fri, 28 Apr 2023 17:30:46 +0200
Subject: [PATCH] destroy handled
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
Co-authored-by: Arnaud Levy <arnaud.levy@noesya.coop>
Co-authored-by: Pierre-André Boissinot <pierreandre.boissinot@noesya.coop>
---
.../websites/categories_controller.rb | 2 +-
.../websites/menus_controller.rb | 2 +-
.../websites/pages_controller.rb | 2 +-
.../websites/posts_controller.rb | 2 +-
app/models/communication/website/git_file.rb | 13 +++++++---
.../website/with_git_repository.rb | 6 ++---
app/models/concerns/as_indirect_object.rb | 22 ----------------
app/models/concerns/with_dependencies.rb | 26 ++++++++++++++++---
app/models/concerns/with_git.rb | 11 --------
app/models/concerns/with_git_files.rb | 3 +--
app/services/git/repository.rb | 13 +++++-----
config/initializers/active_storage.rb | 4 ++-
12 files changed, 48 insertions(+), 58 deletions(-)
diff --git a/app/controllers/admin/communication/websites/categories_controller.rb b/app/controllers/admin/communication/websites/categories_controller.rb
index aa82e97e7..468fd4050 100644
--- a/app/controllers/admin/communication/websites/categories_controller.rb
+++ b/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
diff --git a/app/controllers/admin/communication/websites/menus_controller.rb b/app/controllers/admin/communication/websites/menus_controller.rb
index 01c5d9df6..6294e26bc 100644
--- a/app/controllers/admin/communication/websites/menus_controller.rb
+++ b/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
diff --git a/app/controllers/admin/communication/websites/pages_controller.rb b/app/controllers/admin/communication/websites/pages_controller.rb
index d3a69c053..3015ca457 100644
--- a/app/controllers/admin/communication/websites/pages_controller.rb
+++ b/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
diff --git a/app/controllers/admin/communication/websites/posts_controller.rb b/app/controllers/admin/communication/websites/posts_controller.rb
index 79993e0f8..150442c0a 100644
--- a/app/controllers/admin/communication/websites/posts_controller.rb
+++ b/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
diff --git a/app/models/communication/website/git_file.rb b/app/models/communication/website/git_file.rb
index 48b81f6f0..f147b220e 100644
--- a/app/models/communication/website/git_file.rb
+++ b/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
diff --git a/app/models/communication/website/with_git_repository.rb b/app/models/communication/website/with_git_repository.rb
index b42580aae..d8e99687e 100644
--- a/app/models/communication/website/with_git_repository.rb
+++ b/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!
diff --git a/app/models/concerns/as_indirect_object.rb b/app/models/concerns/as_indirect_object.rb
index 118bbe5ce..42989fba2 100644
--- a/app/models/concerns/as_indirect_object.rb
+++ b/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
diff --git a/app/models/concerns/with_dependencies.rb b/app/models/concerns/with_dependencies.rb
index 03129bd16..a84da4259 100644
--- a/app/models/concerns/with_dependencies.rb
+++ b/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
diff --git a/app/models/concerns/with_git.rb b/app/models/concerns/with_git.rb
index 0752db0be..cc27ec35f 100644
--- a/app/models/concerns/with_git.rb
+++ b/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
diff --git a/app/models/concerns/with_git_files.rb b/app/models/concerns/with_git_files.rb
index 8d77e4a8a..e4d5bd9b6 100644
--- a/app/models/concerns/with_git_files.rb
+++ b/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)
diff --git a/app/services/git/repository.rb b/app/services/git/repository.rb
index 80449c074..990cb6d0d 100644
--- a/app/services/git/repository.rb
+++ b/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
diff --git a/config/initializers/active_storage.rb b/config/initializers/active_storage.rb
index 750130fec..7ecab59ed 100644
--- a/config/initializers/active_storage.rb
+++ b/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)
--
GitLab