-
Notifications
You must be signed in to change notification settings - Fork 41
Fetch attachments on the backend when downloading from record sets #6625
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Triggered by bce323c on branch refs/heads/issue-6521
Fix missing parameter
Triggered by 28d620c on branch refs/heads/issue-6521
specifyweb/frontend/js_src/lib/components/FormSliders/RecordSelectorFromIds.tsx
Show resolved
Hide resolved
Triggered by 954ce9e on branch refs/heads/issue-6521
|
@specify/dev-testing Here's something I want to draw attention to: This meant that the true number of records was misrepresented on large queries. (Also, though expected, the undefined values weren't entirely handled correctly by the attachment gallery) I fixed this by both padding the ids in queries and sending the true number of records to the attachment gallery (aka not relying on the length of the array). Should I leave in the padding? Large record sets pad their arrays already and it seems to work fine, but it still feels weird to have potentially massive arrays. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Verify the "Download All" button actually downloaded all the record set's attachments.
- You should be able to scroll down and load more record set attachments.
- Verify the "Download All" button prompts you to create a record set from the query if not all the attachments were loaded
- You should be able to scroll down and load more query attachments.
- Make sure "Download All" downloads all of the query's attachments.
So everything does work, however downloading attachments takes forever even if there are not that many. I was testing on KUfish and most downloads seemed to take about 2 minutes but when checking on KUbirds a record set with about the same amount of attachments would be done in 10 seconds. I'm not sure if this is just a kufish problem or if it happens on more dbs but either way I think it should be looked into more.
Link to KUfish record set: https://kufish20250214-issue-6521.test.specifysystems.org/specify/view/collectionobject/35816/?recordsetid=214
Link to KUBirds record set: https://kubirds20240606-issue-6521.test.specifysystems.org/specify/view/collectionobject/129670/?recordsetid=231
Also I found a problem on ojsmnh where when I query for attachments and I open the attachment gallery it shows the wrong number of attachments. Not sure if this is on other dbs or not, it might have something to do with dowloading attachments as well but I'm not sure.
07-08_10.33.mp4
@emenslin Hm I have no idea what could be causing this but I optimized the backend asset server requests and queries to hopefully improve this. The PR is ready to test again with all issues resolved 👍👍 (except for the one below vv )
I recreated this on main so it seems to be an existing issue. If a record appears more than once in the query results it will appear multiple times in "browse in forms", so its attachments will be multiplied when viewed in the gallery. While I could remove duplicate records from the gallery, I think this should be written up as a separate issue to also address records showing up multiple times in browse in forms. chrome_mc54BJxI3y.mp4https://kubirds20240606-main.test.specifysystems.org/specify/query/76/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Verify the "Download All" button actually downloaded all the record set's attachments.
- You should be able to scroll down and load more record set attachments.
- Verify the "Download All" button prompts you to create a record set from the query if not all the attachments were loaded
- You should be able to scroll down and load more query attachments.
- Make sure "Download All" downloads all of the query's attachments.
Looks good, the only issue I ran into was when trying to download a large amount of attachments (<1000) I would either get a failed to fetch error or if it successfully downloaded when I tried to open the zip file it would be invalid. I'm going to approve because it seemed to work consistently with up to 500 attachments (not sure what the limit is for when it stops working consistently) and I don't know if users are ever going to want to download more than that, I just wanted to make note of this behavior.
Downloading attachments in kufish is still slow but faster than before (same record set is now done in about a minute and a half instead of two minutes)
Triggered by 7574f79 on branch refs/heads/issue-6521
|
I pushed a fix to correctly download files with paths in the name like "C:/path/to/file.jpg". I believe this could result in corrupted zip files sometimes. All attachment names will now be sanitized to be filename-safe. |
pashiav
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Verify the "Download All" button actually downloaded all the record set's attachments.
- You should be able to scroll down and load more record set attachments.
- Verify the "Download All" button prompts you to create a record set from the query if not all the attachments were loaded
- You should be able to scroll down and load more query attachments.
- Make sure "Download All" downloads all of the query's attachments.
Looks good!
Note: I also get Failed to Fetch messages (same behavior as #6625 (review)) with large downloads.
Triggered by 62dc727 on branch refs/heads/issue-6521
Fixes #6521, Fixes #4445, Fixes #6403, Fixes #6947
This PR makes the backend fetch all the attachments in a record set so the "Download All" button in the gallery dialog doesn't rely on the attachments fetched by the frontend. Kind of related to #4445, but fetching all the attachments on the frontend isn't a good idea anyway (This PR does fix the loading though!).
These are the changes:
Checklist
self-explanatory (or properly documented)
Testing instructions