diff --git a/app/controllers/admin/communication/websites/categories_controller.rb b/app/controllers/admin/communication/websites/categories_controller.rb index aa82e97e7e7300ab2bbdf0800dd6de0e15db758c..468fd4050675d78103bddcf5725794219e80bb4d 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 01c5d9df6d49a659b1b347e9d4f7c0c61fb50874..6294e26bc13a2902d6e4df8a0047af3e5137ba99 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 d3a69c0532792c84e3affc28eaf4a2eecd881e6a..3015ca457e236165427055111bc3417adfc68bdb 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 79993e0f8822920c788133bfaa29c23cdde69943..150442c0a3270e6c33408c104d8029ff9280c72c 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 48b81f6f075beb0ba389e1a849f53e7b697a1a03..f147b220e3356e2788f80230f5494a2430654ee2 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 b42580aaef7afa26f77381b63dcd3ec7ffd3734a..d8e99687e3a292195146148babb68e7896599e28 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 118bbe5ce4f0ba34bfc55ac67ddb0dc8fd72fa0e..42989fba26e0468eee0afd3d5cafea34cf9b25cd 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 03129bd162638dea6b7fab1ab4b51e19d1498d0c..a84da42596be128ced4ab26d90c19dc2c36824d5 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 0752db0bee2e77f970deef103dfa65f283ef4065..cc27ec35fdeebe0bfac08cf7075d5d009b1157c9 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 8d77e4a8a7b6c06b09d694677c1dec1018a17006..e4d5bd9b6c4b3ff332874a27074d2686084d40d0 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 80449c0742976fd9622cc591aa6fb0130b0db568..990cb6d0df1e7b6ee84641225960842636dda60d 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 750130fecf6e1b2dbe3a2478802fe418dbcf964a..7ecab59ed1031b3ed39753af3663515fdac24247 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)