From 490100fc819646b07d7071c370b7b3f8eb78df22 Mon Sep 17 00:00:00 2001 From: pabois Date: Thu, 3 Sep 2026 20:47:45 +0200 Subject: [PATCH 1/5] Mise ne place d'un lock git - partie 1 --- app/models/communication/website.rb | 9 ++++--- .../website/with_git_repository.rb | 17 ++++++++++++ app/services/git/providers/abstract.rb | 4 +++ app/services/git/providers/github.rb | 3 ++- app/services/git/providers/gitlab.rb | 3 ++- app/services/git/repository.rb | 8 ++++++ ...ion_locked_by_to_communication_websites.rb | 11 ++++++++ db/schema.rb | 5 +++- test/services/git_repository_test.rb | 26 +++++++++++++++++++ 9 files changed, 80 insertions(+), 6 deletions(-) create mode 100644 db/migrate/20260903090000_add_synchronization_locked_by_to_communication_websites.rb diff --git a/app/models/communication/website.rb b/app/models/communication/website.rb index 436e64114f..6750e73a72 100644 --- a/app/models/communication/website.rb +++ b/app/models/communication/website.rb @@ -45,17 +45,20 @@ # default_language_id :uuid not null, indexed # deuxfleurs_access_key_id :string # locked_by_job_id :uuid +# synchronization_locked_by_id :uuid indexed # university_id :uuid not null, indexed # # Indexes # -# index_communication_websites_on_about (about_type,about_id) -# index_communication_websites_on_default_language_id (default_language_id) -# index_communication_websites_on_university_id (university_id) +# index_communication_websites_on_about (about_type,about_id) +# index_communication_websites_on_default_language_id (default_language_id) +# index_communication_websites_on_synchronization_locked_by_id (synchronization_locked_by_id) +# index_communication_websites_on_university_id (university_id) # # Foreign Keys # # fk_rails_2b6d929310 (default_language_id => languages.id) +# fk_rails_49c2afa13d (synchronization_locked_by_id => users.id) ON DELETE => nullify # fk_rails_bb6a496c08 (university_id => universities.id) # class Communication::Website < ApplicationRecord diff --git a/app/models/communication/website/with_git_repository.rb b/app/models/communication/website/with_git_repository.rb index fa7d240495..b65a16b91c 100644 --- a/app/models/communication/website/with_git_repository.rb +++ b/app/models/communication/website/with_git_repository.rb @@ -2,6 +2,10 @@ module Communication::Website::WithGitRepository extend ActiveSupport::Concern included do + belongs_to :synchronization_locked_by, + class_name: 'User', + optional: true + has_many :website_git_files, class_name: 'Communication::Website::GitFile', dependent: :destroy @@ -31,6 +35,19 @@ def git_repository @git_repository ||= Git::Repository.new self end + def synchronization_locked? + synchronization_locked_by_id.present? + end + + def lock_synchronization!(user) + update_column :synchronization_locked_by_id, user.id + end + + def unlock_synchronization! + update_column :synchronization_locked_by_id, nil + # TODO website.sync_with_git ? + end + def repository_url git_repository.url end diff --git a/app/services/git/providers/abstract.rb b/app/services/git/providers/abstract.rb index f0a9802e53..825f423bb2 100644 --- a/app/services/git/providers/abstract.rb +++ b/app/services/git/providers/abstract.rb @@ -13,6 +13,10 @@ def valid? repository.present? && access_token.present? end + def synchronization_locked? + git_repository.synchronization_locked? + end + def url raise NoMethodError, "You must implement the `url` method in #{self.class.name}" end diff --git a/app/services/git/providers/github.rb b/app/services/git/providers/github.rb index 50cf8526d2..6c369cea5d 100644 --- a/app/services/git/providers/github.rb +++ b/app/services/git/providers/github.rb @@ -56,6 +56,7 @@ def destroy_file(path) end def update_theme! + return if synchronization_locked? return unless should_update_theme? batch << { path: ENV["GITHUB_WEBSITE_THEME_PATH"], @@ -78,7 +79,7 @@ def init_from_template(name) end def push(commit_message) - return if !valid? || batch.empty? + return if synchronization_locked? || !valid? || batch.empty? check_batch_integrity! commit = create_commit_from_batch(batch, commit_message) client.update_branch repository, default_branch, commit[:sha] diff --git a/app/services/git/providers/gitlab.rb b/app/services/git/providers/gitlab.rb index 64a0a7ba53..53e44d8e7a 100644 --- a/app/services/git/providers/gitlab.rb +++ b/app/services/git/providers/gitlab.rb @@ -42,6 +42,7 @@ def destroy_file(path) end def update_theme! + return if synchronization_locked? return unless should_update_theme? client.edit_submodule repository, ENV["GITHUB_WEBSITE_THEME_PATH"], @@ -61,7 +62,7 @@ def update_secrets(secrets) end def push(commit_message) - return if !valid? || batch.empty? + return if synchronization_locked? || !valid? || batch.empty? check_batch_integrity! client.create_commit repository, branch, diff --git a/app/services/git/repository.rb b/app/services/git/repository.rb index c363172f3f..1285c17a6c 100644 --- a/app/services/git/repository.rb +++ b/app/services/git/repository.rb @@ -15,8 +15,13 @@ def batch_size provider.class::COMMIT_BATCH_SIZE end + def synchronization_locked? + website.synchronization_locked? + end + def sync! return if git_files.empty? + return if synchronization_locked? puts "Start sync" synchronize_git_files provider.push('Sync from osuny') @@ -24,6 +29,7 @@ def sync! end def update_theme_version! + return if synchronization_locked? provider.update_theme! end @@ -45,10 +51,12 @@ def valid? end def init_from_template(name) + return if synchronization_locked? provider.init_from_template(name) end def update_secrets(secrets) + return if synchronization_locked? provider.update_secrets(secrets) end diff --git a/db/migrate/20260903090000_add_synchronization_locked_by_to_communication_websites.rb b/db/migrate/20260903090000_add_synchronization_locked_by_to_communication_websites.rb new file mode 100644 index 0000000000..e803e63533 --- /dev/null +++ b/db/migrate/20260903090000_add_synchronization_locked_by_to_communication_websites.rb @@ -0,0 +1,11 @@ +class AddSynchronizationLockedByToCommunicationWebsites < ActiveRecord::Migration[8.1] + def change + add_reference :communication_websites, + :synchronization_locked_by, + type: :uuid, + foreign_key: { + to_table: :users, + on_delete: :nullify + } + end +end diff --git a/db/schema.rb b/db/schema.rb index a22bb880b2..d897fc0234 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,7 +10,7 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.1].define(version: 2026_08_29_043355) do +ActiveRecord::Schema[8.1].define(version: 2026_09_03_090000) do # These are extensions that must be enabled in order to support this database enable_extension "pg_catalog.plpgsql" enable_extension "pg_stat_statements" @@ -1661,6 +1661,7 @@ t.string "repository" t.text "style" t.date "style_updated_at" + t.uuid "synchronization_locked_by_id" t.string "theme_version", default: "NA" t.uuid "university_id", null: false t.datetime "updated_at", null: false @@ -1668,6 +1669,7 @@ t.integer "years_before_archive_content", default: 3 t.index ["about_type", "about_id"], name: "index_communication_websites_on_about" t.index ["default_language_id"], name: "index_communication_websites_on_default_language_id" + t.index ["synchronization_locked_by_id"], name: "index_communication_websites_on_synchronization_locked_by_id" t.index ["university_id"], name: "index_communication_websites_on_university_id" end @@ -3016,6 +3018,7 @@ add_foreign_key "communication_website_posts", "universities" add_foreign_key "communication_websites", "languages", column: "default_language_id" add_foreign_key "communication_websites", "universities" + add_foreign_key "communication_websites", "users", column: "synchronization_locked_by_id", on_delete: :nullify add_foreign_key "education_diploma_localizations", "education_diplomas", column: "about_id" add_foreign_key "education_diploma_localizations", "languages" add_foreign_key "education_diploma_localizations", "universities" diff --git a/test/services/git_repository_test.rb b/test/services/git_repository_test.rb index fc2eb5fb2d..57c9cccc8c 100644 --- a/test/services/git_repository_test.rb +++ b/test/services/git_repository_test.rb @@ -55,6 +55,23 @@ class GitRepositoryTest < ActiveSupport::TestCase end end + test "no push on github when synchronization is locked" do + # Note: no VCR needed as no real HTTP requests should be made + website_with_github.lock_synchronization! users(:admin) + provider = website_with_github.git_repository.send(:provider) + provider.create_file 'test.txt', 'content' + assert_nil provider.push('Creating test.txt file') + end + + test "git files stay desynchronized when synchronization is locked" do + git_file = communication_website_git_files(:git_file_2) + website_with_github.lock_synchronization! users(:admin) + repository = website_with_github.git_repository + repository.git_files = [git_file] + assert_nil repository.sync! + assert git_file.reload.desynchronized + end + test "incorrect credentials for gitlab" do VCR.use_cassette(location) do assert_enqueued_emails 1 do @@ -105,4 +122,13 @@ class GitRepositoryTest < ActiveSupport::TestCase end end end + + test "no push on gitlab when synchronization is locked" do + # Note: no VCR needed as no real HTTP requests should be made + website_with_gitlab.lock_synchronization! users(:admin) + provider = website_with_gitlab.git_repository.send(:provider) + provider.create_file 'test.txt', 'content' + assert_nil provider.push('Creating test.txt file') + end + end From ace495dc9521c6dd6cb53219b749c847c9233465 Mon Sep 17 00:00:00 2001 From: pabois Date: Fri, 4 Sep 2026 09:39:14 +0200 Subject: [PATCH 2/5] adjust --- app/models/communication/website/with_git_repository.rb | 2 ++ test/services/git_repository_test.rb | 8 ++++++++ 2 files changed, 10 insertions(+) diff --git a/app/models/communication/website/with_git_repository.rb b/app/models/communication/website/with_git_repository.rb index b65a16b91c..72622196f7 100644 --- a/app/models/communication/website/with_git_repository.rb +++ b/app/models/communication/website/with_git_repository.rb @@ -61,11 +61,13 @@ def identify_git_files_safely end def sync_with_git + return if synchronization_locked? update_column(:last_sync_at, Time.now) Communication::Website::SyncWithGitJob.perform_later(id) end def sync_with_git_safely + return if synchronization_locked? return unless git_repository.valid? git_repository.git_files = git_files.generated .desynchronized_until(last_sync_at) diff --git a/test/services/git_repository_test.rb b/test/services/git_repository_test.rb index 57c9cccc8c..771e39dde1 100644 --- a/test/services/git_repository_test.rb +++ b/test/services/git_repository_test.rb @@ -131,4 +131,12 @@ class GitRepositoryTest < ActiveSupport::TestCase assert_nil provider.push('Creating test.txt file') end + test "no sync job enqueued when synchronization is locked" do + website_with_github.lock_synchronization! users(:admin) + assert_no_enqueued_jobs only: Communication::Website::SyncWithGitJob do + website_with_github.sync_with_git + # git_file_2 stays desynchronized, but the job must not requeue itself + website_with_github.sync_with_git_safely + end + end end From 705d9ca7231f6e81562b31b96cab4dd66138d1ea Mon Sep 17 00:00:00 2001 From: pabois Date: Mon, 7 Sep 2026 11:49:59 +0200 Subject: [PATCH 3/5] adjust --- app/models/communication/website/with_git_repository.rb | 2 ++ app/services/git/providers/abstract.rb | 4 ---- app/services/git/providers/github.rb | 3 +-- app/services/git/providers/gitlab.rb | 3 +-- 4 files changed, 4 insertions(+), 8 deletions(-) diff --git a/app/models/communication/website/with_git_repository.rb b/app/models/communication/website/with_git_repository.rb index 72622196f7..c836de8349 100644 --- a/app/models/communication/website/with_git_repository.rb +++ b/app/models/communication/website/with_git_repository.rb @@ -121,10 +121,12 @@ def should_clean_on_git? end def update_theme_version + return if synchronization_locked? Communication::Website::UpdateThemeVersionJob.perform_later(id) end def update_theme_version_safely + return if synchronization_locked? return unless git_repository.valid? git_repository.update_theme_version! end diff --git a/app/services/git/providers/abstract.rb b/app/services/git/providers/abstract.rb index 825f423bb2..f0a9802e53 100644 --- a/app/services/git/providers/abstract.rb +++ b/app/services/git/providers/abstract.rb @@ -13,10 +13,6 @@ def valid? repository.present? && access_token.present? end - def synchronization_locked? - git_repository.synchronization_locked? - end - def url raise NoMethodError, "You must implement the `url` method in #{self.class.name}" end diff --git a/app/services/git/providers/github.rb b/app/services/git/providers/github.rb index 6c369cea5d..50cf8526d2 100644 --- a/app/services/git/providers/github.rb +++ b/app/services/git/providers/github.rb @@ -56,7 +56,6 @@ def destroy_file(path) end def update_theme! - return if synchronization_locked? return unless should_update_theme? batch << { path: ENV["GITHUB_WEBSITE_THEME_PATH"], @@ -79,7 +78,7 @@ def init_from_template(name) end def push(commit_message) - return if synchronization_locked? || !valid? || batch.empty? + return if !valid? || batch.empty? check_batch_integrity! commit = create_commit_from_batch(batch, commit_message) client.update_branch repository, default_branch, commit[:sha] diff --git a/app/services/git/providers/gitlab.rb b/app/services/git/providers/gitlab.rb index 53e44d8e7a..64a0a7ba53 100644 --- a/app/services/git/providers/gitlab.rb +++ b/app/services/git/providers/gitlab.rb @@ -42,7 +42,6 @@ def destroy_file(path) end def update_theme! - return if synchronization_locked? return unless should_update_theme? client.edit_submodule repository, ENV["GITHUB_WEBSITE_THEME_PATH"], @@ -62,7 +61,7 @@ def update_secrets(secrets) end def push(commit_message) - return if synchronization_locked? || !valid? || batch.empty? + return if !valid? || batch.empty? check_batch_integrity! client.create_commit repository, branch, From 9841ef0a9c2660e0f8f461d7f2b737d0f03b1804 Mon Sep 17 00:00:00 2001 From: pabois Date: Mon, 7 Sep 2026 11:59:43 +0200 Subject: [PATCH 4/5] clean --- app/services/git/repository.rb | 8 -------- 1 file changed, 8 deletions(-) diff --git a/app/services/git/repository.rb b/app/services/git/repository.rb index 1285c17a6c..c363172f3f 100644 --- a/app/services/git/repository.rb +++ b/app/services/git/repository.rb @@ -15,13 +15,8 @@ def batch_size provider.class::COMMIT_BATCH_SIZE end - def synchronization_locked? - website.synchronization_locked? - end - def sync! return if git_files.empty? - return if synchronization_locked? puts "Start sync" synchronize_git_files provider.push('Sync from osuny') @@ -29,7 +24,6 @@ def sync! end def update_theme_version! - return if synchronization_locked? provider.update_theme! end @@ -51,12 +45,10 @@ def valid? end def init_from_template(name) - return if synchronization_locked? provider.init_from_template(name) end def update_secrets(secrets) - return if synchronization_locked? provider.update_secrets(secrets) end From 7dc793193e5dbe143dff2f9e944349e938e8b922 Mon Sep 17 00:00:00 2001 From: pabois Date: Mon, 7 Sep 2026 12:27:20 +0200 Subject: [PATCH 5/5] adjust tests --- .../website/with_git_repository.rb | 2 +- .../website/with_git_repository_test.rb | 35 +++++++++++++++++++ test/services/git_repository_test.rb | 34 ------------------ 3 files changed, 36 insertions(+), 35 deletions(-) create mode 100644 test/models/communication/website/with_git_repository_test.rb diff --git a/app/models/communication/website/with_git_repository.rb b/app/models/communication/website/with_git_repository.rb index c836de8349..ec455952af 100644 --- a/app/models/communication/website/with_git_repository.rb +++ b/app/models/communication/website/with_git_repository.rb @@ -45,7 +45,7 @@ def lock_synchronization!(user) def unlock_synchronization! update_column :synchronization_locked_by_id, nil - # TODO website.sync_with_git ? + sync_with_git if desynchronized_generated_git_files.any? end def repository_url diff --git a/test/models/communication/website/with_git_repository_test.rb b/test/models/communication/website/with_git_repository_test.rb new file mode 100644 index 0000000000..04dfe8d6d5 --- /dev/null +++ b/test/models/communication/website/with_git_repository_test.rb @@ -0,0 +1,35 @@ +require "test_helper" + +# rails test test/models/communication/website/with_git_repository_test.rb +class Communication::Website::WithGitRepositoryTest < ActiveSupport::TestCase + include ActiveJob::TestHelper + + test "lock and unlock synchronization" do + refute website_with_github.synchronization_locked? + website_with_github.lock_synchronization! users(:admin) + assert website_with_github.synchronization_locked? + website_with_github.unlock_synchronization! + refute website_with_github.synchronization_locked? + end + + test "jobs are enqueued only when unlocked" do + # Locked: nothing is enqueued + website_with_github.lock_synchronization! users(:admin) + assert_no_enqueued_jobs do + website_with_github.sync_with_git + website_with_github.sync_with_git_safely + website_with_github.update_theme_version + end + + # Unlocking re-triggers a sync for desynchronized generated git files + assert_enqueued_jobs 1, only: Communication::Website::SyncWithGitJob do + website_with_github.unlock_synchronization! + end + + # Unlocked: jobs run normally + assert_enqueued_jobs 1, only: Communication::Website::UpdateThemeVersionJob do + website_with_github.update_theme_version + end + + end +end diff --git a/test/services/git_repository_test.rb b/test/services/git_repository_test.rb index 771e39dde1..fc2eb5fb2d 100644 --- a/test/services/git_repository_test.rb +++ b/test/services/git_repository_test.rb @@ -55,23 +55,6 @@ class GitRepositoryTest < ActiveSupport::TestCase end end - test "no push on github when synchronization is locked" do - # Note: no VCR needed as no real HTTP requests should be made - website_with_github.lock_synchronization! users(:admin) - provider = website_with_github.git_repository.send(:provider) - provider.create_file 'test.txt', 'content' - assert_nil provider.push('Creating test.txt file') - end - - test "git files stay desynchronized when synchronization is locked" do - git_file = communication_website_git_files(:git_file_2) - website_with_github.lock_synchronization! users(:admin) - repository = website_with_github.git_repository - repository.git_files = [git_file] - assert_nil repository.sync! - assert git_file.reload.desynchronized - end - test "incorrect credentials for gitlab" do VCR.use_cassette(location) do assert_enqueued_emails 1 do @@ -122,21 +105,4 @@ class GitRepositoryTest < ActiveSupport::TestCase end end end - - test "no push on gitlab when synchronization is locked" do - # Note: no VCR needed as no real HTTP requests should be made - website_with_gitlab.lock_synchronization! users(:admin) - provider = website_with_gitlab.git_repository.send(:provider) - provider.create_file 'test.txt', 'content' - assert_nil provider.push('Creating test.txt file') - end - - test "no sync job enqueued when synchronization is locked" do - website_with_github.lock_synchronization! users(:admin) - assert_no_enqueued_jobs only: Communication::Website::SyncWithGitJob do - website_with_github.sync_with_git - # git_file_2 stays desynchronized, but the job must not requeue itself - website_with_github.sync_with_git_safely - end - end end