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

tclaus · 2021-07-23T19:50:12Z

Currently a import from Settings, while a user already exists.

ToDo:

Photos upload and processing compressed file
Photos processing
Rake file adaptions
Use translation for texts

diaspora-code-review · 2021-07-23T19:50:24Z

app/views/users/_import_account_modal.haml

+          %p
+            If the pod of the account you are importing is offline, your contacts and posts will still be imported,
+            but your contacts will receive a notification as if you started sharing with them.
+          = form_for current_user, url: edit_user_path, html: {method: :put, multipart: true, class: "form-horizontal"} do |f|


Line is too long. [126/120]

diaspora-code-review · 2021-07-23T19:50:24Z

app/views/users/_import_account_modal.haml

+              .clearfix
+                .btn.btn-default{data: {dismiss: "modal"}}
+                  Cancel
+                = f.submit 'Import and merge', class: "btn btn-primary.pull-right", id: "change_email_preferences"


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

diaspora-code-review · 2021-07-23T19:50:24Z

app/views/users/_import_account_modal.haml

+                .btn.btn-default{data: {dismiss: "modal"}}
+                  Cancel
+                = f.submit 'Import and merge', class: "btn btn-primary.pull-right", id: "change_email_preferences"
+


Line contains trailing whitespace

diaspora-code-review · 2021-07-23T19:50:24Z

app/workers/import_profile.rb

+      end
+    end
+
+private


Layout/AccessModifierIndentation: Indent access modifiers like private.

Layout/EmptyLinesAroundAccessModifier: Keep a blank line before and after private.

diaspora-code-review · 2021-07-23T19:50:24Z

app/workers/import_profile.rb

+        photo.remote_photo_name = nil # migration sets this attribute, but file needs to be procesed
+        photo.unprocessed_image = photo_filename
+        photo.save(touch: false)
+


Layout/EmptyLinesAroundBlockBody: Extra empty line detected at block body end.

diaspora-code-review · 2021-07-23T19:50:24Z

app/workers/import_profile.rb

+    def unzip_photos_file(photo_file_path)
+      system("unzip", photo_file_path)
+    end
+


Layout/EmptyLinesAroundClassBody: Extra empty line detected at class body end.

diaspora-code-review · 2021-09-03T22:12:39Z

app/uploaders/exported_photos.rb

  def filename
-    "#{model.username}_photos_#{secure_token}.zip" if original_filename.present?
+    if original_filename.present?


Style/GuardClause: Use a guard clause (return unless original_filename.present?) instead of wrapping the code inside a conditional expression.

diaspora-code-review · 2021-09-03T22:12:39Z

app/uploaders/exported_user.rb

  end

  def filename
-    "#{model.username}_diaspora_data_#{secure_token}.json.gz" if original_filename.present?
+    if original_filename.present?


Style/GuardClause: Use a guard clause (return unless original_filename.present?) instead of wrapping the code inside a conditional expression.

diaspora-code-review · 2021-09-04T13:38:40Z

app/models/photo.rb

-  def url(name = nil)
-    if remote_photo_path
-      name = name.to_s + '_' if name
+  def url(name=nil)


Metrics/AbcSize: Assignment Branch Condition size for url is too high. [<2, 19, 7> 20.35/20]

diaspora-code-review · 2021-09-04T13:38:40Z

app/models/photo.rb

+    # During migration in bad cases this might be happen
+    # If this happens, stream loading stops. Better dont show photos as stop loading stream
+    if remote_photo_path.present? && remote_photo_name.present?
+      name = "#{name.to_s}_" if name


Lint/RedundantStringCoercion: Redundant use of Object#to_s in interpolation.

goobertron · 2021-09-04T19:19:39Z

I'd like chance to review the text (including code comments) at some point, as Diaspora's (sort of) 'chief proof-reader'. I'll try to do this soon.

goobertron

I've reviewed most of the strings and made suggested changes to some of them. I think I've seen everything, but it's not easy with the text in code files rather than in a separate config file. I'll check again once the text has been extracted.

There are a few comments where I'm not sure exactly what they are supposed to communicate, so I've asked for clarification.

I'll have to think about the two large text blocks at a later date, so I'll come back to them.

Thanks!

goobertron · 2021-09-04T19:22:29Z

app/controllers/users_controller.rb

+    @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] = "A profile migration is scheduled"


Assuming this includes data and photos, I suggest using 'account' rather than 'profile'. How about either:

Your account migration has been scheduled
or

Your account migration is in progress

goobertron · 2021-09-04T19:23:58Z

app/controllers/users_controller.rb

+    if @user.save
+      flash.now[:notice] = "A profile migration is scheduled"
+    else
+      flash.now[:error] = "An error occured scheduling a migration: #{@user.errors.full_messages}"


It looks as though a specific error message is appended to this warning, so I suggest:

The following error occurred in scheduling your account migration:
or

Your account migration could not be scheduled for the following reason:
(note: two Rs in occurred)

goobertron · 2021-09-04T19:25:08Z

app/models/account_migration.rb

@@ -29,6 +29,7 @@ def sender
  # executes a migration plan according to this AccountMigration object


Could this be clearer? Or is it clear to any coder?

goobertron · 2021-09-04T19:25:54Z

app/models/photo.rb

    else
      true
    end
  end

  def ownership_of_status_message
-    message = StatusMessage.find_by_guid(self.status_message_guid)
+    message = StatusMessage.find_by(guid: status_message_guid)
    return unless status_message_guid && message && diaspora_handle != message.diaspora_handle

    errors.add(:base, "Photo must have the same owner as status message")


If this is text in the UI, I'll need to know context in order to be able to polish this.

This is internally only. I think, a photo must have a reference to a post ('status message' internally) to be valid. This text goes out to an error log.

Got it. That is probably clear enough to a coder or admin.

goobertron · 2021-09-04T19:26:30Z

app/models/photo.rb

-    if remote_photo_path
-      name = name.to_s + '_' if name
+  def url(name=nil)
+    # During migration in bad cases this might be happen


Again, these two lines could be clearer, but context would be helpful.

A message to a coder. The if clause the next line was changed, because in rare cases a photo can not be processed. Then the stream will not load. Diaspora stops (crashes). Would be nice to use clear words for this.

How about 'If a photo cannot be processed during migration, the migration will fail and the stream will stop loading. If this happens, don't show the photos' ???

goobertron · 2021-09-04T19:28:18Z

app/models/user.rb

-    logger.error "Unexpected error while exporting user '#{username}': #{error.class}: #{error.message}\n" \
-                 "#{error.backtrace.first(15).join("\n")}"
+  rescue StandardError => e
+    logger.error "Unexpected error while exporting user '#{username}': #{e.class}: #{e.message}\n" \


Unexpected error while exporting data for '#{username}':
This mirrors the error message for photos.

goobertron · 2021-09-04T19:29:34Z

app/views/users/_edit.haml

        %h3= t(".export_data")
+        %p If you want to migrate this account to another pod, start by exporting your data using the buttons below.


... start by exporting your data and photos ...

goobertron · 2021-09-04T19:30:31Z

app/views/users/_edit.haml

+          Import another account into
+          = current_user.diaspora_handle
+        %p
+          diaspora* allows you to merge an account into another, even if the accounts are in two different pods.


I'll have to come back to this text block. Do remind me.

Suggested some modified texts here, for discussion: https://discourse.diasporafoundation.org/t/backup-and-restore-account-migration/544/173

goobertron · 2021-09-04T19:37:46Z

app/views/users/_import_account_modal.haml

+                %label Select the data archive:
+                = f.file_field :export, accept: "application/json, application/zip, application/gzip"
+              .col-md-6
+                %label Select the photos archive:


'photo archive'

goobertron · 2021-09-04T19:38:27Z

app/views/users/_import_account_modal.haml

+                %label Select the photos archive:
+                = f.file_field :exported_photos_file, accept: "application/zip"
+            %h4
+              The original account will be imported


You data will be imported %strong and the original account deleted.

This needs to be transferred to config/locales/diaspora/en.yml. Also, I still think it's better not to repeat 'the original account' in this line, so change the first one to 'Your data will be imported'.

And maybe add 'This cannot be undone.' after this sentence?

diaspora-code-review · 2021-09-05T07:44:48Z

app/views/users/_edit.haml

        %h3= t(".export_data")
+        %p If you want to migrate this account to another pod, start by exporting your data and photos using the buttons below.


Line is too long. [127/120]

diaspora-code-review · 2021-09-08T21:17:36Z

app/assets/javascripts/archive-uploader.js

@@ -0,0 +1,15 @@
+const profileFileBtn = document.getElementById('profile-file-btn');


Strings must use doublequote.

diaspora-code-review · 2021-09-08T21:17:36Z

app/assets/javascripts/archive-uploader.js

@@ -0,0 +1,15 @@
+const profileFileBtn = document.getElementById('profile-file-btn');
+
+const profileFileChosen = document.getElementById('profile-file-chosen');


Strings must use doublequote.

diaspora-code-review · 2021-09-08T21:17:36Z

app/assets/javascripts/archive-uploader.js

+
+const profileFileChosen = document.getElementById('profile-file-chosen');
+
+profileFileBtn.addEventListener('change', function(){


Strings must use doublequote.

Missing space before opening brace.

diaspora-code-review · 2021-09-08T21:17:36Z

app/assets/javascripts/archive-uploader.js

+const profileFileChosen = document.getElementById('profile-file-chosen');
+
+profileFileBtn.addEventListener('change', function(){
+  profileFileChosen.textContent = this.files[0].name


Missing semicolon.

diaspora-code-review · 2021-09-08T21:17:36Z

app/assets/javascripts/archive-uploader.js

+
+profileFileBtn.addEventListener('change', function(){
+  profileFileChosen.textContent = this.files[0].name
+})


Missing semicolon.

diaspora-code-review · 2021-09-08T21:17:37Z

app/views/users/_import_account_modal.haml

+                .upload-btn-wrapper
+                  .btn.btn-primary
+                    Select profile archive
+                  = f.file_field :export, accept: "application/json, application/zip, application/gzip",


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

diaspora-code-review · 2021-09-08T21:17:37Z

app/views/users/_import_account_modal.haml

+                .upload-btn-wrapper
+                  .btn.btn-primary
+                    Select photo archive
+                  = f.file_field :exported_photos_file, accept: "application/zip",


Layout/SpaceAfterColon: Space missing after colon.

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

diaspora-code-review · 2021-09-08T21:17:37Z

app/assets/stylesheets/forms.scss

+  display: inline-block;
+}
+
+.upload-btn-wrapper input[type=file] {


Merge rule .upload-btn-wrapper input[type=file] with rule on line 21

Avoid qualifying attribute selectors with an element.

diaspora-code-review · 2021-09-08T21:17:37Z

app/assets/stylesheets/forms.scss

@@ -18,6 +18,20 @@ textarea {
 }
 // scss-lint:enable QualifyingElement

+.upload-btn-wrapper {
+  position: relative;


Properties should be ordered display, overflow, position

diaspora-code-review · 2021-09-08T21:17:38Z

app/assets/stylesheets/forms.scss

+
+.upload-btn-wrapper input[type=file] {
+  font-size: 100px;
+  position: absolute;


Properties should be ordered font-size, left, opacity, position, top

diaspora-code-review · 2021-09-10T05:06:08Z

lib/archive_importer.rb

@@ -36,7 +36,8 @@ def create_user(attr)
        profile_attributes: profile_attributes
      }
    )
-    self.user = User.build(data)
+    self.user = User.find_or_build(data)
+    self.user.getting_started = false


Style/RedundantSelf: Redundant self detected.

diaspora-code-review · 2021-09-10T05:06:09Z

app/assets/stylesheets/forms.scss

+  overflow: hidden;
+  position: relative;
+
+  input[type=file] {


Avoid qualifying attribute selectors with an element.

tclaus · 2021-09-12T08:20:51Z

Current status of the import dialog is:

Rake file allows import of photo archives:

goobertron

Thanks. I can only comment on the wording (and the fact that all the text needs to be in config/locales/diaspora/en.yml.

Fla has said in this Discourse comment that the text is not that accurate, so we might have to rewrite some of it. But with the two minor changes I've just suggested, I think it reads well enough and is clear enough, even if some of it is not yet accurate!

goobertron · 2021-09-12T18:15:36Z

config/locales/diaspora/en.yml

+      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.<br>
+                     Your contacts do not need to do anything; they will automatically be connected to this account.<br>


I would not have the rest of this text in bold – only 'The imported account and all its linked data will be deleted from the previous pod.'

I also suggest separating this text block into three strings, one for each paragraph, rather than using <br>, which I think is not favoured.

goobertron · 2021-09-12T18:17:25Z

app/views/users/_import_account_modal.haml

+                %label Select the photos archive:
+                = f.file_field :exported_photos_file, accept: "application/zip"
+            %h4
+              The original account will be imported


This needs to be transferred to config/locales/diaspora/en.yml. Also, I still think it's better not to repeat 'the original account' in this line, so change the first one to 'Your data will be imported'.

And maybe add 'This cannot be undone.' after this sentence?

Flaburgan · 2021-09-13T09:31:05Z

Hey, could you please make the rake test to fail when it doesn't find the archive?

diaspora-code-review · 2021-09-13T18:42:43Z

app/views/users/_import_account_modal.haml

+              .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: "change_email_preferences"


Line is too long. [132/120]

diaspora-code-review · 2021-09-14T18:29:18Z

app/assets/javascripts/archive-uploader.js

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


'checkProfileUploadButton' was used before it was defined

diaspora-code-review · 2021-09-14T18:29:18Z

app/assets/javascripts/archive-uploader.js

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


'checkProfileUploadButton' was used before it was defined

diaspora-code-review · 2021-09-14T18:29:18Z

app/assets/javascripts/archive-uploader.js

+});
+
+const checkProfileUploadButton = function() {
+  let photo_files = $("#profile-file-btn")[0].files;


Identifier 'photo_files' is not in camel case.

diaspora-code-review · 2021-09-14T18:29:19Z

app/assets/javascripts/archive-uploader.js

+
+const checkProfileUploadButton = function() {
+  let photo_files = $("#profile-file-btn")[0].files;
+  let profile_files = $("#photos-file-btn")[0].files;


Identifier 'profile_files' is not in camel case.

diaspora-code-review · 2021-09-14T18:29:19Z

app/assets/javascripts/archive-uploader.js

+  let photo_files = $("#profile-file-btn")[0].files;
+  let profile_files = $("#photos-file-btn")[0].files;
+
+  if ((photo_files.size + profile_files.size) === 0)


Opening curly brace does not appear on the same line as controlling statement.

Identifier 'photo_files' is not in camel case.

Identifier 'profile_files' is not in camel case.

diaspora-code-review · 2021-09-14T18:29:19Z

app/assets/javascripts/archive-uploader.js

+
+  if ((photo_files.size + profile_files.size) === 0)
+  {
+    $("#upload_profile_files").attr('disabled', 'disabled');


Strings must use doublequote.

diaspora-code-review · 2021-09-14T18:29:19Z

app/assets/javascripts/archive-uploader.js

+  {
+    $("#upload_profile_files").attr('disabled', 'disabled');
+  } else {
+    $("#upload_profile_files").removeAttr('disabled');


Strings must use doublequote.

diaspora-code-review · 2021-09-14T18:29:19Z

app/assets/javascripts/archive-uploader.js

+    $("#upload_profile_files").removeAttr('disabled');
+  }
+
+}


Block must not be padded by blank lines.

Missing semicolon.

The datamodel and implementation supports multiple imports from different locations This Commit removes a test and fixes the validation, which is not supported in the datamodel

- Updates (most) texts to goobs proposal - Moved texts to localization file - Fixed file-upload buttons

Flaburgan · 2021-09-19T11:48:40Z

Hey @tclaus could you please extract the rake refactor and the photos import (basically everything only linked to the backend) in a smaller pull request? This would allow me to start doing more tests even if the UI isn't ready yet

Flaburgan · 2021-09-26T20:49:18Z

So, I tried the UI locally and it didn't work, the validation took too much time and the HTTP connection "has been reinitialized" (says Firefox). I think we need to split this again. Maybe we could have a different setting page so it's actually easier to find (and to link to) for the user?
So we would add an "Export / Import" item in the settings menu. There, we have more space to actually put the import buttons, and allow the user to come back later even for the validation step.

tclaus · 2021-09-27T16:36:43Z

So, I tried the UI locally and it didn't work, the validation took too much time and the HTTP connection "has been reinitialized" (says Firefox). I think we need to split this again. Maybe we could have a different setting page so it's actually easier to find (and to link to) for the user?
So we would add an "Export / Import" item in the settings menu. There, we have more space to actually put the import buttons, and allow the user to come back later even for the validation step.

An extra menu is nice.
There is no "wait" for the validation. Instantly after uploading the files, a job should start with validation and processing.
Did you test it locally.. without a running sidekiq for background processing? I have experienced something similar.

Flaburgan · 2021-09-27T19:29:50Z

There is no "wait" for the validation. Instantly after uploading the files, a job should start with validation and processing.

But what happens for you when you click the button in the modal then? For me, it does launch the validation, but the modal stays open waiting for it to finish, and with real data, that takes too much time and the http connection expires.

SuperTux88 · 2021-09-27T19:45:05Z

If you run diaspora locally in dev-mode there is no sidekiq by default (the jobs happen in the request then, which works for most things needed for dev-mode), for certain things like migration (takes too long for being handled in request) and also federation with other local pods (with only 1 unicorn worker that is already busy handling the federation job it doesn't have another worker free to handle a webfinger request from a second pod, so it deadlocks), you need a sidekiq, so you need to enable that in the diaspora.toml.

Flaburgan · 2021-09-28T21:10:51Z

Ok, I was able to enable sidekiq by setting single_process_mode = false. Thank you.
Indeed, the modal then close, but when the import fails, there is no message in the settings page. The button to import should disappear and there should be a feedback to the user.
After submitting the modal: "Processing archive XXX"
If the validation failed: "Archive is invalid, ERROR_MESSAGE"
If the import failed: "Import failed, ERROR_MESSAGE, please warn your podmin"
If the import succeed: "Archive XXX imported successfully" and the button to import should reappear.

If you like the idea of having all this on a separated page I can create it.

…_files # Conflicts: # app/controllers/users_controller.rb # app/models/photo.rb # app/models/user.rb # app/services/migration_service.rb # app/uploaders/exported_photos.rb # app/uploaders/exported_user.rb # app/uploaders/secure_uploader.rb # lib/archive_importer.rb # lib/tasks/accounts.rake # spec/models/user_spec.rb

On profile file upload the same fields in the user table are used than for generated exporting files. This conflicts if any export files exists and user wants to import something

diaspora-code-review · 2022-06-11T18:26:42Z

app/controllers/users_controller.rb

@@ -122,6 +122,10 @@ def confirm_email
    redirect_to edit_user_path
  end

+  def has_import_parameter?(import_parameters)


Naming/PredicateName: Rename has_import_parameter? to import_parameter?.

diaspora-code-review · 2022-06-11T18:26:42Z

app/controllers/users_controller.rb

+
+  def copy_import_files(user_data)
+    {
+            profile_path: copy_import_file(user_data[:export]),


Layout/FirstHashElementIndentation: Use 2 spaces for indentation in a hash, relative to the start of the line where the left curly brace is.

diaspora-code-review · 2022-06-11T18:26:42Z

app/controllers/users_controller.rb

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


Layout/HashAlignment: Align the keys and values of a hash literal if they span more than one line.

diaspora-code-review · 2022-06-11T18:26:42Z

app/controllers/users_controller.rb

+  end
+
+  def copy_import_file(tmp_file)
+    if tmp_file.present?


Style/GuardClause: Use a guard clause (return unless tmp_file.present?) instead of wrapping the code inside a conditional expression.

diaspora-code-review · 2022-06-11T18:26:42Z

app/controllers/users_controller.rb

+
+  def copy_import_file(tmp_file)
+    if tmp_file.present?
+      file_path_to_save_to = Rails.root.join("public","uploads", "users", "#{current_user.username}_#{tmp_file.original_filename}")


Layout/SpaceAfterComma: Space missing after comma.

Layout/LineLength: Line is too long. [131/120]

diaspora-code-review · 2022-06-11T18:26:42Z

app/services/import_service.rb

+  # @param [*String] files
+  def remove_import_files(*files)
+    files.each do |file|
+      if file && File.exist?(file)


Style/IfUnlessModifier: Favor modifier if usage when having a single-line body. Another good alternative is the usage of control flow &&/||.

diaspora-code-review · 2022-06-11T18:26:42Z

app/services/import_service.rb

+  def remove_import_files(*files)
+    files.each do |file|
+      if file && File.exist?(file)
+        File.delete(file) rescue nil


Style/RescueModifier: Avoid using rescue in its modifier form.

tclaus · 2022-06-11T19:39:19Z

I merged latest development into it to fix merge errors.
This adds a Import UI to the user settings page.

There might be a problem when uploading a json.gz - when unzipped, the resulting file is just a messed up binary. Can someone check this?

cmrd-senya · 2022-07-19T20:17:02Z

There might be a problem when uploading a json.gz - when unzipped, the resulting file is just a messed up binary. Can someone check this?

This may be an issue with the development environment, I've seen something like that happened while downloading the archive on the dev env.

SuperTux88 · 2022-07-19T20:19:07Z

Oh yes, if you download the file in the dev env, it is gzipped twice, because rails again gzips it on download, while in production it will be served by your reverse proxy which doesn't do stuff like that.

tclaus · 2022-07-19T20:20:58Z

Oh yes, if you download the file in the dev env, it is gzipped twice, because rails again gzips it on download, while in production it will be served by your reverse proxy which doesn't do stuff like that.

Good to be on a team that can help.

cmrd-senya

So apart from my comments, a few thoughts:

This PR doesn't add new tests, do we want to add tests here, or we don't consider tests mandatory for everything?
Do we want to give podmins a configuration option to hide/show import functionality? Is there a reason why a podmin may want to turn off this feature on their pods?
Could we combine the two archives -- profile and photos -- in one? We can reduce potential issues if we just needed to upload one file.

cmrd-senya · 2022-07-19T21:06:56Z

app/views/users/_edit.haml

+            = t(".import.warning")
+        .form-group
+          .btn.btn-primary{id: "import_account", data: {toggle: "modal", target: "#importAccountModal"}}
+            Import another account...


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

cmrd-senya · 2022-07-19T21:21:40Z

app/controllers/users_controller.rb

+  end
+
+  def copy_import_file(tmp_file)
+    return if tmp_file.empty?


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

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

I think you need to use blank? here instead

Suggested change

return if tmp_file.empty?

return if tmp_file.blank?

cmrd-senya · 2022-07-19T21:33:14Z

app/controllers/users_controller.rb

@@ -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]


Do we support importing just photos without the profile?

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.

cmrd-senya · 2022-07-19T21:56:52Z

app/services/import_service.rb

    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)


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.

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?

cmrd-senya · 2022-07-19T22:01:26Z

app/services/import_service.rb

  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?


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.

cmrd-senya · 2022-07-19T22:06:10Z

app/workers/import_user.rb

      user = User.find(user_id)
-      ImportService.new.import_by_user(user)
+      ImportService.new.import_by_user(user.username, import_parameters)


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?

cmrd-senya · 2022-07-19T22:11:41Z

lib/archive_importer/post_importer.rb

@@ -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\/"


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?

diaspora-code-review · 2022-07-29T08:14:56Z

app/services/import_service.rb

  end

-  private
+  def import_profile_if_present(opts, path_to_photos, path_to_profile, username)
+    if path_to_profile.present?


Style/GuardClause: Use a guard clause (return unless path_to_profile.present?) instead of wrapping the code inside a conditional expression.

tclaus changed the title ~~Upload migration files from profile settings and start processing~~ Migration: Upload migration files from profile settings and start processing Jul 23, 2021

diaspora-code-review reviewed Jul 23, 2021

View reviewed changes

tclaus force-pushed the upload_migration_files branch from afa3462 to 92e6d5c Compare September 3, 2021 22:12

diaspora-code-review reviewed Sep 3, 2021

View reviewed changes

tclaus force-pushed the upload_migration_files branch from 92e6d5c to 68fad50 Compare September 4, 2021 08:04

tclaus mentioned this pull request Sep 4, 2021

User data export / (partial) import metaissue #5343

Open

7 tasks

tclaus force-pushed the upload_migration_files branch from 68fad50 to 8ce090e Compare September 4, 2021 08:32

tclaus marked this pull request as ready for review September 4, 2021 08:35

diaspora-code-review reviewed Sep 4, 2021

View reviewed changes

goobertron reviewed Sep 4, 2021

View reviewed changes

diaspora-code-review reviewed Sep 5, 2021

View reviewed changes

diaspora-code-review reviewed Sep 8, 2021

View reviewed changes

diaspora-code-review reviewed Sep 10, 2021

View reviewed changes

tclaus force-pushed the upload_migration_files branch from 09c7b0d to a144219 Compare September 12, 2021 08:18

tclaus requested a review from goobertron September 12, 2021 08:20

goobertron reviewed Sep 12, 2021

View reviewed changes

goobertron added the 🚚 migration label Sep 12, 2021

diaspora-code-review reviewed Sep 13, 2021

View reviewed changes

diaspora-code-review reviewed Sep 14, 2021

View reviewed changes

tclaus force-pushed the upload_migration_files branch from e1c57ca to f4a3e09 Compare September 14, 2021 18:33

This was referenced Sep 18, 2021

Migration: Import compressed archive file handling #8260

Merged

Migration: Fixing import issues / Speed up repeated unknown pod detection #8257

Closed

tclaus added 5 commits September 19, 2021 12:53

Upload migration files from profile settings and start processing

9fb0909

Multiple_migrations_are_allowed

c6c19b7

The datamodel and implementation supports multiple imports from different locations This Commit removes a test and fixes the validation, which is not supported in the datamodel

Moving Texts to localization file

c539d8c

- Updates (most) texts to goobs proposal - Moved texts to localization file - Fixed file-upload buttons

Find or create a user on import by Rake Task

7fa8057

Fixed style errors

c82f6c3

tclaus added 3 commits September 19, 2021 12:57

Addad photos import to rake file

26995bd

changes in texts

1eec8a4

No submit if no photo is selected

5255767

tclaus force-pushed the upload_migration_files branch from f4a3e09 to 5255767 Compare September 19, 2021 11:11

tclaus mentioned this pull request Sep 19, 2021

Migration: Backend, Rake file, Photos import #8298

Merged

Flaburgan mentioned this pull request Sep 28, 2021

Import shouldn't fail when a pod certificate is invalid #8300

Closed

tclaus added 2 commits June 8, 2022 09:21

Fixing merge errors

6927c21

On profile file upload the same fields in the user table are used than for generated exporting files. This conflicts if any export files exists and user wants to import something

diaspora-code-review reviewed Jun 11, 2022

View reviewed changes

Fixing styles

14743e1

tclaus requested a review from SuperTux88 July 2, 2022 13:26

cmrd-senya reviewed Jul 19, 2022

View reviewed changes

diaspora-code-review reviewed Jul 29, 2022

View reviewed changes

Fixing review issues

2e7dbb5

tclaus force-pushed the upload_migration_files branch from cd1cfa3 to 2e7dbb5 Compare July 30, 2022 19:40

+                    end
+                  end
+              private

		@@ -29,6 +29,7 @@ def sender
		# executes a migration plan according to this AccountMigration object

		%h3= t(".export_data")
		%p If you want to migrate this account to another pod, start by exporting your data using the buttons below.

		@@ -0,0 +1,15 @@
		const profileFileBtn = document.getElementById('profile-file-btn');

		@@ -0,0 +1,15 @@
		const profileFileBtn = document.getElementById('profile-file-btn');

		const profileFileChosen = document.getElementById('profile-file-chosen');


		const profileFileChosen = document.getElementById('profile-file-chosen');

		profileFileBtn.addEventListener('change', function(){

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

Are you sure you want to change the base?

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

Conversation

tclaus commented Jul 23, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goobertron commented Sep 4, 2021

goobertron left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

goobertron Sep 12, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tclaus commented Sep 12, 2021

goobertron left a comment

Choose a reason for hiding this comment

goobertron Sep 12, 2021 • edited

Choose a reason for hiding this comment

goobertron Sep 12, 2021 • edited

Choose a reason for hiding this comment

Flaburgan commented Sep 13, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Flaburgan commented Sep 19, 2021

Flaburgan commented Sep 26, 2021

tclaus commented Sep 27, 2021

Flaburgan commented Sep 27, 2021

SuperTux88 commented Sep 27, 2021

Flaburgan commented Sep 28, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tclaus commented Jun 11, 2022

cmrd-senya commented Jul 19, 2022

tclaus commented Jul 23, 2021 •

edited

goobertron Sep 12, 2021 •

edited

goobertron Sep 12, 2021 •

edited

goobertron Sep 12, 2021 •

edited