Repository navigation
[PER-10780] Pick the file that has content, a size, not only the original format. - #1210
Open
aasandei-vsp wants to merge 2 commits into
Open
aasandei-vsp wants to merge 2 commits into
aasandei-vsp wants to merge 2 commits into
Conversation
…ontent Some files receive from stela multiple originals, one being an empty placeholder and the other one being the real content. On web, we were not checking whether the file we were picking had content, but now we make sure to filter the correct one only for preview-ing. Issue: PER-10780
…ne marked as original Stela is sending multiple files with the format original and up until now we would just take the first file marked as original, instead of checking it had content. Now in the data service and the file viewer, we are actually checking that the file that gets to be previewed has content. Issue: PER-10780
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1210 +/- ##
==========================================
+ Coverage 53.64% 53.73% +0.09%
==========================================
Files 368 368
Lines 12925 12928 +3
Branches 2363 2365 +2
==========================================
+ Hits 6933 6947 +14
+ Misses 5753 5744 -9
+ Partials 239 237 -2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
🟡 Changes recommended
File-selection fallbacks, unavailable-file handling, and incomplete fixture data have unresolved issues.
4 open findings
What changed in this PR
This PR updates preview and download file selection to prefer files with retrievable content instead of empty originals.
Changes:
- Adds file retrievability and prioritization helpers.
- Applies selection logic to downloads and viewer previews.
- Adds regression tests for duplicate and failed files.
| File | Summary |
|---|---|
src/app/shared/services/data/data.service.ts |
Updates download selection; unavailable typed or original files still need safe handling. |
src/app/shared/services/data/data.service.spec.ts |
Adds download-selection tests. |
src/app/models/get-access-file.ts |
Adds file-selection helpers; failed preferred entries can still be selected. |
src/app/models/get-access-file.spec.ts |
Tests prioritization behavior. |
src/app/file-browser/components/file-viewer/file-viewer.component.ts |
Uses original-file selection; empty originals can block fallback previews. |
src/app/file-browser/components/file-viewer/file-viewer.component.spec.ts |
Adds preview-selection tests and requires valid fixture sizes. |
🧠 Review effort: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| } | ||
|
|
||
| function hasRetrievableContent(file: PermanentFile): boolean { | ||
| return Boolean(file.fileURL) && file.size !== null && file.size !== undefined; |
Comment on lines
+34
to
39
| const files = prioritizeRetrievableFiles(record?.FileVOs ?? []); | ||
| return ( | ||
| getArchivematicaAccess(files) || | ||
| getPrioritizedConvertedFile(files) || | ||
| files[0] | ||
| ); |
Comment on lines
+608
to
+610
| return prioritizeRetrievableFiles(fileItem.FileVOs).find( | ||
| (file) => file.type === type, | ||
| ); |
| } | ||
|
|
||
| return find(fileItem.FileVOs, { format: 'file.format.original' }); | ||
| return GetOriginalFile(fileItem); |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stela can send multiple files per record with the original format. The web-app wouldn't check for content, but would pick the first file that has the original format, which sometimes is an empty one. This PR adds a check for the preview and data services to check if the file we are trying to preview or download actually has content. It seems we might need a follow-up with stela.
This is actually a fix(more of a gating) that is inferred, as I could not reproduce the bug locally.
Issue: PER-10780
Click here to view curated manual test cases
# Manual test cases — regression for picking files in the viewer and downloadsNothing should look different. This change only affects which file the app picks for a record, so every viewer and download should behave exactly as before.
Full-screen viewer ⭐
Images
Documents
Media
Download ⭐
From the full-screen viewer
From the file list
Shared and public views
(same viewer on other routes, cheap sanity checks)
Share link