FEATURE: Initial implementation of direct S3 uploads with uppy and stubs (PR #13787)

Still very much in flux and in bad need of tests, please don’t review yet.

dontloolk

GitHub

I’m not sure I like all these checks for !external_upload_too_big, may refactor this in future. There is just so little we can do for these huge external uploads that we haven’t downloaded

Not quite sure whether I should put a lifecycle rule in this same commit; no one is using this yet and it’s all behind experimental site settings. When I do add one it will be done in a similar way to the tombstone lifecycle rule, but for the temp folder this time:

This can be done in a later PR, this one still only accepts image uploads for the user card background

This check for existing uploads works for direct S3 uploads that are not too big as well.

The title of this pull request changed from “WIP: Initial implementation of direct S3 uploads” to "FEATURE: Initial implementation of direct S3 uploads with uppy and stubs

This is a breaking change - I noticed at least one plugin is using it:

Could we add the new parameter on the end, and have it default to data.files[0].name if not present?

This looks good - I only made minor comments. The usual warning applies: large commits like this should be merged with caution when you are around to support them.

Any reason why you couldn’t use render_json_error(message) here?

I wonder if these are of much use to an end user? Most users will only want to know their upload failed and to try again, rather than if there’s a download failure or checksum mismatch. Those are very advanced messages that I suspect will only confuse people. They might be good for admins, though.

I wonder if the put verb would be better here, considering it even has presigned_put in the URL!

Not super important. Just a thought!

      URI.parse(url).path.delete_prefix('/')

Again this is minor but I find delete_prefix clearer with regards to intent.

Ah damn I only searched inside discourse-org and discourse organizations. Sure I will do that, makes more sense to put the new param on the end.

The reason is because I forgot that existed, will fix :slight_smile:

Yeah that is fair enough. When you say good for admins do you mean I should show these error messages to admin users and the generic “upload failed” one to everyone else?

Yeah it’s a weird one because this endpoint creates an ExternalUploadStub record at the same time and is not changing an existing resource like PUT is usually used for

Much better thanks, I forget about these nice little ruby helpers a lot

Oh also I don’t even use this method anyway! Will just get rid of it

Added a deprecation warning too

    opts = opts || {};
    super(uppy, opts);

Should the default value be assigned before the super function call?