FIX: Handle non-AWS S3 to local migration for assets that do not use upload: pseudo-protocl (PR #9809)

GitHub

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Michael K Johnson seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

This pull request has been mentioned on Discourse Meta. There might be relevant details there:

https://meta.discourse.org/t/migrating-s3-spaces-non-image-uploads-to-local/132484/3

@martin-brennan can you review this one please? Thanks!

@johnsonm thanks for submitting; I read your meta post and I have some idea of the context, but it would be good if you add a description of what you are trying to achieve here to the PR. I think your change here is sound and won’t break anything.

It would be great if you could make a minimal test to demonstrate what you are solving here. Take a look at spec/tasks/uploads_spec.rb which is a spec that shows how to test a rake task. To invoke the task you do this:

    def invoke_task
      capture_stdout do
        Rake::Task['uploads:disable_secure_media'].invoke
      end
    end

I would feel more confident about merging once there is a test and a description.

Discourse is my introduction to ruby development, so I very much appreciate the pointer. I hate writing code without automated tests.

I don’t think a minimal test is really meaningful without adding some baseline coverage though. I’ll see what I can do!

I think that all the video uploads in my google+ import might have been broken when I tried to do it; it’s not at all clear to me that this PR is meaningful.

I have found examples that do need to be fixed in my migration, and the tests I added in the batch limit PR will help here.

There is an additional problem that UploadCreator is doing strange things with videos; with this broken, it happens that it will ignore them; with this problem fixed, it actually breaks the posts. It modified the post to:

https:/uploads/default/original/3X/b/a/ba9e06ebc2f4397f26793bb5cd4e169308dd371d.mp4

It looks like new_upload.url isn’t right for a video. It’s definitely giving me /uploads/default/original/3X/b/a/ba9e06ebc2f4397f26793bb5cd4e169308dd371d.mp4 which is not a base URL.

This is replaced by #10093 which has many other fixes.