Fix bug in remote thumbnail search

[richvdh]

#7124 changed the behaviour of remote thumbnails so that the thumbnailing method was included in the filename of the thumbnail. To support existing files, it included a fallback so that we would check the old filename if the new filename didn't exist.

Unfortunately, it didn't apply this logic to storage providers, so any thumbnails stored on such a storage provider was broken.

[richvdh]

this is just moved down by a few lines.

[clokep]

In general I wonder if a bunch of this code could be de-duplicated and simplified by generating a set of paths to check (the "correct" path and maybe the legacy one) something like:

paths = [local_path]
if legacy_path:
    paths.append(legacy_path)

for path in paths:
    # Check if exists in local media dir

for path in paths:
    for provider in self.storage_providers:
        # Check if can fetch from provider

🤷 There's potentially a couple ways to organize those loops and such too.

Overall this seems like a fine change though I do wonder if it makes sense to check each provider for the "correct" file name first, then the legacy one or to iterate through each provider (as the PR has now). I think we only expect one match ever, so it shouldn't really matter?

[richvdh]

I do wonder if it makes sense to check each provider for the "correct" file name first, then the legacy one or to iterate through each provider (as the PR has now).

The current implementation means that we can check all local stores first, before making any S3 requests. Flipping the order around would mean adding the delay of an S3 lookup for a file that is stored in the local system with a legacy name.

[richvdh]

I've deduplicated the code a bit.

[clokep]

I find the logic much easier to follow now! Thanks. 👍

[clokep]

Looks good, assuming CI passes.