From 1570e3fb9afb6d312c94118f5bae5463021d9bae Mon Sep 17 00:00:00 2001
From: Benjamin Neff <benjamin@coding4coffee.ch>
Date: Fri, 29 Oct 2021 03:08:01 +0200
Subject: [PATCH] Migrate remote_photo_path and cleanup old photo uploads
If the migration contains a new remote_photo_path migrate all photos of
the old person to this path. If the person was local before, cleanup old
uploaded files of the photos.
closes #8314
---
Changelog.md | 1 +
app/models/account_migration.rb | 19 +++++++++
..._remote_photo_path_to_account_migration.rb | 7 ++++
lib/diaspora/federation/receive.rb | 6 ++-
spec/integration/account_migration_spec.rb | 41 +++++++++++++++----
.../receive_federation_messages_spec.rb | 4 +-
spec/models/account_migration_spec.rb | 32 ++++++++++++++-
7 files changed, 100 insertions(+), 10 deletions(-)
create mode 100644 db/migrate/20211027230348_add_remote_photo_path_to_account_migration.rb
diff --git a/Changelog.md b/Changelog.md
index b4f8e0e2af..7a7d5eecb5 100644
--- a/Changelog.md
+++ b/Changelog.md
@@ -22,6 +22,7 @@
* Add username to password-reset mail [#8037](https://github.com/diaspora/diaspora/pull/8037)
* Resend account migration and deletion for closed recipients [#8309](https://github.com/diaspora/diaspora/pull/8309)
* Add sharing status to hovercards [#8317](https://github.com/diaspora/diaspora/pull/8317)
+* Migrate photo URLs and cleanup old uploaded photos [#8314](https://github.com/diaspora/diaspora/pull/8314)
# 0.7.15.0
diff --git a/app/models/account_migration.rb b/app/models/account_migration.rb
index 8a3c42cabb..274ecdecfb 100644
--- a/app/models/account_migration.rb
+++ b/app/models/account_migration.rb
@@ -90,6 +90,10 @@ class AccountMigration < ApplicationRecord
old_user && new_user
end
+ def includes_photo_migration?
+ remote_photo_path.present?
+ end
+
# We need to resend contacts of users of our pod for the remote new person so that the remote pod received this
# contact information from the authoritative source.
def dispatch_contacts
@@ -122,6 +126,7 @@ class AccountMigration < ApplicationRecord
end
def update_all_references
+ update_remote_photo_path if remotely_initiated? && includes_photo_migration?
update_person_references
update_user_references if user_changed_id_locally?
end
@@ -200,6 +205,20 @@ class AccountMigration < ApplicationRecord
.destroy_all
end
+ def update_remote_photo_path
+ Photo.where(author: old_person)
+ .update_all(remote_photo_path: remote_photo_path) # rubocop:disable Rails/SkipsModelValidations
+ return unless user_left_our_pod?
+
+ Photo.where(author: old_person).find_in_batches do |batch|
+ batch.each do |photo|
+ photo.processed_image = nil
+ photo.unprocessed_image = nil
+ logger.warn "Error cleaning up photo #{photo.id}" unless photo.save
+ end
+ end
+ end
+
def update_person_references
logger.debug "Updating references from person id=#{old_person.id} to person id=#{new_person.id}"
eliminate_person_duplicates
diff --git a/db/migrate/20211027230348_add_remote_photo_path_to_account_migration.rb b/db/migrate/20211027230348_add_remote_photo_path_to_account_migration.rb
new file mode 100644
index 0000000000..291d88c956
--- /dev/null
+++ b/db/migrate/20211027230348_add_remote_photo_path_to_account_migration.rb
@@ -0,0 +1,7 @@
+# frozen_string_literal: true
+
+class AddRemotePhotoPathToAccountMigration < ActiveRecord::Migration[5.2]
+ def change
+ add_column :account_migrations, :remote_photo_path, :text
+ end
+end
diff --git a/lib/diaspora/federation/receive.rb b/lib/diaspora/federation/receive.rb
index edbdf2b9f7..dd136ac424 100644
--- a/lib/diaspora/federation/receive.rb
+++ b/lib/diaspora/federation/receive.rb
@@ -31,7 +31,11 @@ module Diaspora
profile = profile(entity.profile)
return if AccountMigration.exists?(old_person: old_person, new_person: profile.person)
- AccountMigration.create!(old_person: old_person, new_person: profile.person).tap do |migration|
+ AccountMigration.create!(
+ old_person: old_person,
+ new_person: profile.person,
+ remote_photo_path: entity.remote_photo_path
+ ).tap do |migration|
migration.signature = entity.signature if old_person.local?
migration.save!
end
diff --git a/spec/integration/account_migration_spec.rb b/spec/integration/account_migration_spec.rb
index 35597edad6..17beba1994 100644
--- a/spec/integration/account_migration_spec.rb
+++ b/spec/integration/account_migration_spec.rb
@@ -97,9 +97,29 @@ shared_examples_for "migration scenarios initiated locally" do
[]
end
- inlined_jobs do
- run_migration
- end
+ inlined_jobs { run_migration }
+ end
+
+ it "does not change the remote paths" do
+ photo = FactoryGirl.create(:photo, author: old_person)
+ remote_photo_path = photo.remote_photo_path
+
+ run_migration
+
+ expect(photo.reload.remote_photo_path).to eq(remote_photo_path)
+ end
+end
+
+shared_examples_for "remote photo migration" do
+ it "changes the remote paths of photos of the old person" do
+ old_photo = FactoryGirl.create(:photo, author: old_person)
+ new_photo = FactoryGirl.create(:photo, author: new_person)
+ new_remote_photo_path = old_photo.remote_photo_path
+
+ run_migration
+
+ expect(old_photo.reload.remote_photo_path).to eq("https://diaspora.example.tld/uploads/images/")
+ expect(new_photo.reload.remote_photo_path).to eq(new_remote_photo_path)
end
end
@@ -125,6 +145,8 @@ describe "account migration" do
include_examples "every migration scenario"
include_examples "migration scenarios initiated remotely"
+
+ include_examples "remote photo migration"
end
# this is the case when we're a pod, which was left by a person in favor of remote one
@@ -136,6 +158,8 @@ describe "account migration" do
include_examples "migration scenarios initiated remotely"
+ include_examples "remote photo migration"
+
it_behaves_like "migration scenarios with local old user"
it_behaves_like "deletes all of the user data" do
@@ -165,9 +189,10 @@ describe "account migration" do
def run_migration
AccountMigration.create!(
- old_person: old_user.person,
- new_person: new_user.person,
- old_private_key: old_user.serialized_private_key
+ old_person: old_user.person,
+ new_person: new_user.person,
+ old_private_key: old_user.serialized_private_key,
+ remote_photo_path: "https://diaspora.example.tld/uploads/images/"
).perform!
end
@@ -184,7 +209,9 @@ describe "account migration" do
include_context "with local new user"
def run_migration
- AccountMigration.create!(old_person: old_user.person, new_person: new_user.person).perform!
+ AccountMigration.create!(old_person: old_user.person,
+ new_person: new_user.person,
+ remote_photo_path: "https://diaspora.example.tld/uploads/images/").perform!
end
include_examples "every migration scenario"
diff --git a/spec/integration/federation/receive_federation_messages_spec.rb b/spec/integration/federation/receive_federation_messages_spec.rb
index b53bc97c18..7b29c5738b 100644
--- a/spec/integration/federation/receive_federation_messages_spec.rb
+++ b/spec/integration/federation/receive_federation_messages_spec.rb
@@ -55,7 +55,9 @@ describe "Receive federation messages feature" do
it "receives account migration correctly" do
run_migration
expect(AccountMigration.where(old_person: sender.person, new_person: new_user.person)).to exist
- expect(AccountMigration.find_by(old_person: sender.person, new_person: new_user.person)).to be_performed
+ account_migration = AccountMigration.find_by(old_person: sender.person, new_person: new_user.person)
+ expect(account_migration).to be_performed
+ expect(account_migration.remote_photo_path).to eq("https://diaspora.example.tld/uploads/images/")
end
it "doesn't run the same migration for the second time" do
diff --git a/spec/models/account_migration_spec.rb b/spec/models/account_migration_spec.rb
index a5fd24f0a6..91a12ba470 100644
--- a/spec/models/account_migration_spec.rb
+++ b/spec/models/account_migration_spec.rb
@@ -16,7 +16,9 @@ describe AccountMigration, type: :model do
let(:old_person) { FactoryGirl.create(:person) }
let(:new_person) { FactoryGirl.create(:person) }
let(:account_migration) {
- AccountMigration.create!(old_person: old_person, new_person: new_person)
+ AccountMigration.create!(old_person: old_person,
+ new_person: new_person,
+ remote_photo_path: "https://diaspora.example.tld/uploads/images/")
}
describe "receive" do
@@ -125,6 +127,34 @@ describe AccountMigration, type: :model do
expect(Diaspora::Federation::Dispatcher).to receive(:defer_dispatch).with(contact.user, contact)
account_migration.perform!
end
+
+ it "cleans up old local photos" do
+ photo = FactoryGirl.create(:photo, author: old_person)
+ photo.processed_image.store!(photo.unprocessed_image)
+ photo.save!
+
+ account_migration.perform!
+
+ updated_photo = photo.reload
+ expect(updated_photo.remote_photo_path).to eq("https://diaspora.example.tld/uploads/images/")
+ expect(updated_photo.processed_image.path).to be_nil
+ expect(updated_photo.unprocessed_image.path).to be_nil
+ end
+
+ it "does nothing if migration doesn't contain a new remote_photo_path" do
+ photo = FactoryGirl.create(:photo, author: old_person)
+ photo.processed_image.store!(photo.unprocessed_image)
+ photo.save!
+
+ remote_photo_path = photo.remote_photo_path
+
+ AccountMigration.create!(old_person: old_person, new_person: new_person).perform!
+
+ updated_photo = photo.reload
+ expect(updated_photo.remote_photo_path).to eq(remote_photo_path)
+ expect(updated_photo.processed_image.path).not_to be_nil
+ expect(updated_photo.unprocessed_image.path).not_to be_nil
+ end
end
context "with local new and remote old users" do
--
GitLab