FIX: Do not prefix temp/ S3 keys with s3_bucket_folder_path in S3Helper (PR #14145)

This is unnecessary, as when the temporary key is created in S3Store we already include the s3_bucket_folder_path, and the key will always start with temp/ to assist with lifecycle rules for multipart uploads.

This was affecting Discourse.store.object_from_path, Discourse.store.signed_url_for_path, and possibly others.

See also: e0102a5

GitHub

I think starts_with is a lot more readable here than regex and does the same thing, existing tests pass fine.

When running this test singly I got an error saying .minutes wasn’t an integer method, so rails gubbins hadn’t been loaded yet :slight_smile:

    it "does not prefix the s3_bucket_folder_path onto #{FileStore::BaseStore::TEMPORARY_UPLOAD_PREFIX} keys" do
  it "should not prefix the bucket folder path if the key begins with #{FileStore::BaseStore::TEMPORARY_UPLOAD_PREFIX}" do
    object2 = s3_helper.object("#{FileStore::BaseStore::TEMPORARY_UPLOAD_PREFIX}folder_path/uploads/default/blah/def.xyz")
    expect(object2.key).to eq("#{FileStore::BaseStore::TEMPORARY_UPLOAD_PREFIX}folder_path/uploads/default/blah/def.xyz")

Hmm this is a bit weird, do we usually interpolate strings into spec descriptions???

Hmm maybe just say temporary upload prefix instead of duplicating the value of the constant in multiple places.

Yep cool that makes more sense, I think the spec descriptions should be as static as possible :+1: