Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Migration: Upload migration files from profile settings and start processing #8274

Open
wants to merge 12 commits into
base: develop
Choose a base branch
from
1 change: 1 addition & 0 deletions app/assets/config/manifest.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,3 +8,4 @@
//= link error_pages.css
//= link admin.css
//= link rtl.css
//= link archive-uploader
24 changes: 24 additions & 0 deletions app/assets/javascripts/archive-uploader.js
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
const checkProfileUploadButton = function() {
let photoFiles = $("#profile-file-btn")[0].files;
let profileFiles = $("#photos-file-btn")[0].files;
if ((photoFiles.size + profileFiles.size) === 0) {
$("#upload_profile_files").attr("disabled", "disabled");
} else {
$("#upload_profile_files").removeAttr("disabled");
}
};
const profileFileButton = document.getElementById("profile-file-btn");
const profileFileChosen = document.getElementById("profile-file-chosen");

const photosFileButton = document.getElementById("photos-file-btn");
const photosFileChosen = document.getElementById("photos-file-chosen");

profileFileButton.addEventListener("change", function() {
profileFileChosen.textContent = this.files[0].name;
checkProfileUploadButton();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'checkProfileUploadButton' was used before it was defined

});

photosFileButton.addEventListener("change", function() {
photosFileChosen.textContent = this.files[0].name;
checkProfileUploadButton();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

'checkProfileUploadButton' was used before it was defined

});
14 changes: 14 additions & 0 deletions app/assets/stylesheets/forms.scss
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,20 @@ textarea {
box-shadow: none;
}
}

.upload-btn-wrapper {
display: inline-block;
overflow: hidden;
position: relative;

input[type=file] {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Avoid qualifying attribute selectors with an element.

font-size: 100px;
left: 0;
opacity: 0;
position: absolute;
top: 0;
}
}
// scss-lint:enable QualifyingElement

textarea {
Expand Down
34 changes: 27 additions & 7 deletions app/controllers/users_controller.rb
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,10 @@ def confirm_email
redirect_to edit_user_path
end

def import_parameter?(import_parameters)
import_parameters[:profile_path] || import_parameters[:photos_path]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we support importing just photos without the profile?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A user could first import his/her account and in another step the photos. These files are huge, that to separate them still makes sense.
After the profile is imported the user could import the photos.
I wouldn't put these files into on (very) big file.

end

private

def user_params
Expand Down Expand Up @@ -229,15 +233,31 @@ def change_email(user_data)

def upload_export_files(user_data)
logger.info "Start importing account"
@user.export = user_data[:export] if user_data[:export]
@user.exported_photos_file = user_data[:exported_photos_file] if user_data[:exported_photos_file]
if @user.save
flash.now[:notice] = "Your account migration has been scheduled"

import_parameters = copy_import_files(user_data)

if import_parameter?(import_parameters)
flash.now[:notice] = t("users.import.import_has_been_scheduled")
else
flash.now[:error] = "Your account migration could not be scheduled for the following reason:"\
" #{@user.errors.full_messages}"
flash.now[:error] = t("users.import.import_has_no_files_received")
end
Workers::ImportUser.perform_async(@user.id)
Workers::ImportUser.perform_async(@user.id, import_parameters)
end

def copy_import_files(user_data)
{
profile_path: copy_import_file(user_data[:export]),
photos_path: copy_import_file(user_data[:exported_photos_file])
}
end

def copy_import_file(tmp_file)
return if tmp_file.empty?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I tried uploading archive locally I've got the following error raised on this line:

image

What do you think about adding automated tests for this scenario? Tests could catch issues like that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you need to use blank? here instead

Suggested change
return if tmp_file.empty?
return if tmp_file.blank?


file_path_to_save_to = Rails.root.join("public", "uploads", "users",
"#{current_user.username}_#{tmp_file.original_filename}")
FileUtils.cp tmp_file.path, file_path_to_save_to
file_path_to_save_to
end

def change_settings(user_data, successful="users.update.settings_updated", error="users.update.settings_not_updated")
Expand Down
25 changes: 15 additions & 10 deletions app/services/import_service.rb
Original file line number Diff line number Diff line change
Expand Up @@ -3,24 +3,27 @@
class ImportService
include Diaspora::Logging

def import_by_user(user, opts={})
import_by_files(user.export.current_path, user.exported_photos_file.current_path, user.username, opts)
def import_by_user(user_name, import_parameters)
profile_path = import_parameters["profile_path"]
photos_path = import_parameters["photos_path"]

import_by_files(profile_path, photos_path, user_name)
end

def import_by_files(path_to_profile, path_to_photos, username, opts={})
user = User.find_by(username: username)
raise ArgumentError, "Username #{username} should exist before uploading photos." if user.nil?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think raising exception from the sidekiq job would schedule this job to retry. If this error is irrecoverable we should instead finish the job without errors, possibly logging some error messages.


if path_to_profile.present?
logger.info "Import for profile #{username} at path #{path_to_profile} requested"
import_user_profile(path_to_profile, username, opts.merge(photo_migration: path_to_photos.present?))
end

user = User.find_by(username: username)
raise ArgumentError, "Username #{username} should exist before uploading photos." if user.nil?

if path_to_photos.present?
logger.info("Importing photos from import file for '#{username}' from #{path_to_photos}")
import_user_photos(user, path_to_photos)
end
remove_file_references(user)
remove_import_files(path_to_profile, path_to_photos)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cleaning up invalid data -- if the archive is in wrong format. If we don't clean up, then someone could abuse the pod by uploading invalid archives indefinitely.

I guess we could handle at least certain known errors when the archive import fails, and clean up the files. But not in every case, because in some cases we may want to retry the import.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you mean? The 'import_user_profile' and 'import_user_photos' handle (known) errors. Whenever execution comes here, the uploaded files (I guess the zipped ones) will be removed.
Could you describe your idea or give a suggestion?

end

private
Expand Down Expand Up @@ -101,9 +104,11 @@ def create_folder(compressed_file_name)
folder
end

def remove_file_references(user)
user.remove_exported_photos_file
user.remove_export
user.save
# Removes import files after processing
# @param [*String] files
def remove_import_files(*files)
files.each do |file|
File.delete(file) if file && File.exist?(file)
end
end
end
21 changes: 19 additions & 2 deletions app/views/users/_edit.haml
Original file line number Diff line number Diff line change
Expand Up @@ -193,8 +193,11 @@
%hr

.row
.col-md-6.account-data
.col-md-12
%h3= t(".export_data")
%p= t(".export_data_subline")
.row
.col-md-6.account-data
%h4= t("profile")
.form-group
- if current_user.exporting
Expand All @@ -210,6 +213,7 @@
= link_to t(".request_export"), export_profile_user_path, method: :post,
class: "btn btn-default"

.col-md-6
%h4= t("javascripts.profile.photos")
.form-group
- if current_user.exporting_photos
Expand All @@ -224,7 +228,20 @@
= link_to t(".request_export_photos"), export_photos_user_path, method: :post,
class: "btn btn-default"

.col-md-6
.row
.col-md-12
%h3
= t(".import.headline", {accountname: current_user.diaspora_handle})
%p
= t(".import.body")
%strong
= t(".import.warning")
.form-group
.btn.btn-primary{id: "import_account", data: {toggle: "modal", target: "#importAccountModal"}}
Import another account...
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this needs to be translatable -- wrap into t method call

= render "import_account_modal"
.row
.col-md-12
%h3
= t(".close_account_text")
.form-group
Expand Down
65 changes: 65 additions & 0 deletions app/views/users/_import_account_modal.haml
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
.modal.fade{id: "importAccountModal",
tabindex: "-1",
role: "dialog",
aria: {labelledby: "importAccountModalLabel", hidden: "true"}}
.modal-dialog
.modal-content
.modal-header
%button.close{type: "button", data: {dismiss: "modal"}, aria: {hidden: "true"}}
×
%h3.modal-title{id: "importAccountModalLabel"}
= t("users.import_modal.title")
.modal-body
%p
= simple_format(t("users.import_modal.subtext", {accountname: current_user.diaspora_handle}))
%ol
%li
= t("users.import_modal.list1")
%li
= t("users.import_modal.list2")
%li
= t("users.import_modal.list3")
%li
= t("users.import_modal.list4")
%strong
= t("users.import_modal.closing_text")
%p
= t("users.import_modal.closing_recommendation")
%p
= t("users.import_modal.old_pod_offline")

= form_for current_user,
url: edit_user_path,
html: {method: :put, multipart: true, class: "form-horizontal"} do |f|
.row
.col-md-12
.upload-btn-wrapper
.btn.btn-primary
= t("users.import_modal.select_profile_archive")
= f.file_field :export, accept: "application/json, application/zip, application/gzip",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

id: "profile-file-btn"
%span#profile-file-chosen
= t("users.import_modal.no_profile_file_set")
.row
.col-md-12
.upload-btn-wrapper
.btn.btn-primary
= t("users.import_modal.select_photo_archive")
= f.file_field :exported_photos_file, accept: "application/zip",

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Layout/SpaceAfterColon: Space missing after colon.
  • Style/StringLiterals: Prefer double-quoted strings unless you need single quotes to avoid extra backslashes for escaping.

id: "photos-file-btn"
%span#photos-file-chosen
= t("users.import_modal.no_photos_file_set")
%h4
= t("users.import_modal.import_starts")
%strong
= t("users.import_modal.original_account_will_be_deleted")
%p
.clearfix
.btn.btn-default{data: {dismiss: "modal"}}
= t("users.import_modal.cancel")
= f.submit t("users.import_modal.start_import"),
class: "btn btn-primary.pull-right",
id: "upload_profile_files",
disabled: true

= javascript_include_tag "archive-uploader"
4 changes: 2 additions & 2 deletions app/workers/import_user.rb
Original file line number Diff line number Diff line change
Expand Up @@ -4,9 +4,9 @@ module Workers
class ImportUser < ArchiveBase
private

def perform_archive_job(user_id)
def perform_archive_job(user_id, import_parameters)
user = User.find(user_id)
ImportService.new.import_by_user(user)
ImportService.new.import_by_user(user.username, import_parameters)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we have a user object, and then we get it's username just to look up the user again inside the ImportService. Maybe we can still pass the user object here?

Instead of changing this signature to username, maybe we could change import_by_files to accepting user object and in the rake task we could call import_by_files passing the user object that we find outside of the service?

end
end
end
38 changes: 35 additions & 3 deletions config/locales/diaspora/en.yml
Original file line number Diff line number Diff line change
Expand Up @@ -287,11 +287,10 @@ en:
comments:
create:
error: "Failed to comment."
fail: "Comment creation has failed"
new_comment:
comment: "Comment"
commenting: "Commenting..."
create:
fail: "Comment creation has failed"
destroy:
success: "Comment %{id} has been successfully deleted"
fail: "Comment deletion has failed"
Expand Down Expand Up @@ -1307,13 +1306,21 @@ en:
request_export: "Request my profile data"
request_export_update: "Refresh my profile data"
export_data: "Export data"
export_data_subline: "If you want to migrate this account to another pod, start by exporting your data and photos using the buttons below."
export_in_progress: "We are currently processing your data. Please check back in a few moments."
last_exported_html: "(Last updated %{timeago})"
download_export_photos: "Download my photos"
request_export_photos: "Request my photos"
request_export_photos_update: "Refresh my photos"
export_photos_in_progress: "We are currently processing your photos. Please check back in a few moments."


import:
headline: "Import another account into %{accountname}"
body: "diaspora* allows you to migrate from one pod to another.
To do this, first export your data and photos from the pod you want to move from. Then, import those data and photo archives into this account.
Your old account on the other pod will be deleted once this process has completed."
warning: "There is no way back; that account can’t be restored if you change your mind."

close_account:
dont_go: "Hey, please don’t go!"
make_diaspora_better: "We’d love you to stay and help us make diaspora* better instead of leaving. If you really do want to leave, however, here’s what will happen next:"
Expand All @@ -1328,6 +1335,28 @@ en:
description: "web+diaspora:// is a new web protocol that we have introduced. Any link to a diaspora* page on an external website that uses this protocol can be opened in the pod on which your diaspora* account is registered. Click the button below to set your browser to use %{pod_url} to recognise external web+diaspora:// links."
browser: "This protocol is currently in the experimental stage and the success of interactions using it will depend on your browser. If you want to manage or remove this handler, you will do this via your browser settings. The button below will always be enabled, and you need to set the handler separately in each browser you use."
register: "Register web+diaspora:// handler on this browser"

import_modal:
title: "Import another account"
subtext: "You are about to import and merge another account into %{accountname} <br>
Here is what will happen:"
list1: "Your profile (name, bio, birthday, gender, etc.) will be transferred from the imported account"
list2: "Your settings (theme, default post visibility, etc.) will be transferred from the imported account"
list3: "All your aspects and contacts from that account will be added to this account"
list4: "All posts and comments from that account will be added to this account"
closing_text: "The imported account and all its linked data will be deleted from the previous pod."
closing_recommendation: "Your contacts do not need to do anything; they will automatically be connected to this account."
old_pod_offline: "If your old pod is offline when you migrate, your contacts and posts will still be imported, but
your contacts will receive a notification that you have started sharing with them from
this account."
select_profile_archive: "Select profile archive"
select_photo_archive: "Select photo archive"
no_profile_file_set: "No profile set"
no_photos_file_set: "No photos set"
import_starts: "Your data will be imported."
original_account_will_be_deleted: "The original account will be deleted."
cancel: "Cancel"
start_import: "Start and Import"

privacy_settings:
title: "Privacy settings"
Expand Down Expand Up @@ -1372,6 +1401,9 @@ en:
confirm_email:
email_confirmed: "Email %{email} activated"
email_not_confirmed: "Email could not be activated. Wrong link?"
import:
import_has_been_scheduled: "Your account migration has been scheduled"
import_has_no_files_received: "No files for import received"

two_factor_auth:
title: "Two-factor authentication"
Expand Down
1 change: 1 addition & 0 deletions lib/archive_importer/post_importer.rb
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,7 @@ def substitute_author

entity_data["photos"].each do |photo|
photo["entity_data"]["author"] = user.diaspora_handle
photo["entity_data"]["remote_photo_path"] = "#{AppConfig.pod_uri}uploads\/images\/"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, so this is how we link uploaded photos. If there are files in the photos archive that weren't in the end linked to any photo record, should we detect and clean them up?

end
end

Expand Down
5 changes: 3 additions & 2 deletions spec/integration/migration_service_spec.rb
Original file line number Diff line number Diff line change
Expand Up @@ -271,10 +271,11 @@ def setup_validation_time_expectations
status_message = StatusMessage.find_by(guid: status_message_with_photos_entity.guid)
expect(status_message.author).to eq(user.person)
expect(
status_message.photos.pluck(:guid, :text, :remote_photo_path, :remote_photo_name, :width, :height)
status_message.photos.pluck(:guid, :text, :remote_photo_name, :width, :height)
).to match_array(
status_message_with_photos_entity.photos.map {|photo|
[photo.guid, photo.text, photo.remote_photo_path, photo.remote_photo_name, photo.width, photo.height]
# remote_photo_path is rewritten with new local path
[photo.guid, photo.text, photo.remote_photo_name, photo.width, photo.height]
}
)

Expand Down