specify / specify7

Specify 7
https://www.specifysoftware.org/products/specify-7/
GNU General Public License v2.0
66 stars 36 forks source link

Attachments: Faulty assumption that all collections use the same storage folder #1056

Closed FedorSteeman closed 2 years ago

FedorSteeman commented 3 years ago

For many years now, we've had issues with viewing attachments in Specify7 for our major museum's installation (NHMD).

After some digging, it now turns out a single commit long ago is apparently the cause of our woes: https://github.com/specify/specify7/commit/0050c8fd6a9c931a3712595e717a8190acd6e0b8#commitcomment-59690296

The way our app server is setup for NHMD is that we use multiple folders for each of our collections. This can be seen in the settings.py file for the web asset server, where each collection identifier string points to different folders for the NHMD collections at least. In the same settings file, we made sure to set WEB_ATTACHMENT_COLLECTION to 'None', because, again, we're using multiple folders and not a single one.

Instead of Specify passing the collection name string in the URL, depending on what collection one is logged on to, it defaults to the collection with the lowest ordinal, which in our case is “NHMD Vertebrate Paleontology”. So for instance, when I’m logged in to Specify under the NHMD Entomology collection, instead of putting “NHMD+Entomology” in the URL, it puts “NHMD+Vertebrate+Paleontology” in the URL, causing not only the thumbnails to break, but any further linkage.

For example, Specify7 would produce this link for the thumbnail (which won’t work for you, because you’re not logged in):

https://specify-attachments.science.ku.dk/fileget?coll=NHMD+Vertebrate+Paleontology&type=T&filename=sp63631302825063967879.att.jpg&token=2c13f25f87bb6887e059866b33e0f009%3A1632475049&scale=1024

But it should be the following, since we’re logged in under NHMD Entomology, and this (manually fixed) URL works:

https://specify-attachments.science.ku.dk/fileget?coll=NHMD+Entomology&type=T&filename=sp63631302825063967879.att.jpg&token=2c13f25f87bb6887e059866b33e0f009%3A1632475049&scale=1024

It’s a mystery to me why Specify7 is no longer set up to use the current collection for constructing the URL, and instead defaults to the collection with the lowest ordinal in the set. Seeing the source code, it's clear that it's doing it, but we don't understand why this was changed from the original code 8 years ago.

FedorSteeman commented 2 years ago

This is apparently revamped in v7.6.1, but since we're still stuck with v7.5.0, I added a quick fix to a fork of that version that will do for the time being.

FedorSteeman commented 2 years ago

To our utter surprise, this bug has resurfaced in the latest version, so we still need to hotfix our installation in order to get it to work properly. Why has this simple change not been added to the main branch!?

Our hotfix: https://github.com/specify/specify7/commit/e4d899ea7e657199cab5f0f0a8f3903ca5801cf2#diff-688a4b186646bf0680896b3dc52d2c8c4c1f0fa7c510b482cc7671ed170def7e

FedorSteeman commented 2 years ago

Doesn't look like this simple fix is included in your latest version of Specify 7.7.2, or at least it's not mentioned. This would mean I'd have to apply my hotfix again after upgrading. What's up with that?

FedorSteeman commented 2 years ago

We upgraded to the latest version of Specify7 (7.7.4) and love the new design and features.

However, we are also very disappointed that our issue with viewing attachments across multiple collections is still being ignored and I still need to apply our hotfix after updating.

These are just a few lines of code, so why not take a few moments to proof and include it???

grantfitzsimmons commented 2 years ago

@benanhalt @maxpatiiuk Have you added this to 7.8.0 as a preference? Will this be available upon release?

benanhalt commented 2 years ago

We can't add this hotfix as it stands because it adds request as a parameter to the get_collection function, but that function is called in another context where the request object is not available.

FedorSteeman commented 2 years ago

@benanhalt What other context and how do you propose we otherwise can get a hold of the collection name?

Alternatively, we could allow for overloading the function by changing the parameter to a request=None and then circumvent the lines using the request object when None.

grantfitzsimmons commented 2 years ago

@FedorSteeman Have you tested/evaluated the PR that Ben put together here?

PR https://github.com/specify/specify7/pull/2375

FedorSteeman commented 2 years ago

Yes! I installed it and everything works perfectly now. Thanks for the assistance!

maxpatiiuk commented 1 year ago

@FedorSteeman this change is included in the 7.8.2 release. Note however, for backwards compatibility with existing installations, it is not enabled by default. You can enable it by setting SEPARATE_WEB_ATTACHMENT_FOLDERS to True in the specify settings file (or add - SEPARATE_WEB_ATTACHMENT_FOLDERS=True to the environment variables section of the docker-compose.yml file)

specifysoftware commented 1 year ago

This issue has been mentioned on Specify Community Forum. There might be relevant details there:

https://discourse.specifysoftware.org/t/specify-7-8-2-announcement/963/1