Skip to content

Conversation

Treeed
Copy link
Contributor

@Treeed Treeed commented Feb 2, 2025

Since there is a tendency for online data to be lost it usually makes sense to generally download any external attachments. However, with the current configuration, the original link is lost, so there is no easy way to check e.g. if there was a change to the original file. Since I don't like throwing out data in bulk I needed a mechanism to retain both copies.

This PR splits the single "path" property into "internal_path" and "external_path" to achieve that. To keep the naming and handling of internal and external files consistent that spreads into quite a few files.

Sorry for the single huge commit, but there's really no in-between state where everything still works.

Some translations are required again. Since I still don't know how to do these I just added TODO tags. There's also some changes to the English translation file which would need to be propagated to the other languages.

I considered keeping the "media_url" key in the normalizer for compatibility, but it didn't work for external URLs in the first place, so I ended up just removing it.

Depends on PR #847

@Treeed Treeed force-pushed the SplitAttachmentPaths branch 2 times, most recently from e6dcf69 to 628b68b Compare February 2, 2025 04:48
Copy link

codecov bot commented Feb 2, 2025

Codecov Report

Attention: Patch coverage is 66.24204% with 53 lines in your changes missing coverage. Please review.

Project coverage is 60.24%. Comparing base (ebb977e) to head (524900c).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
src/DataTables/AttachmentDataTable.php 70.58% 10 Missing ⚠️
src/Services/EntityURLGenerator.php 25.00% 9 Missing ⚠️
src/Controller/AttachmentFileController.php 0.00% 8 Missing ⚠️
...rc/Services/Attachments/AttachmentURLGenerator.php 22.22% 7 Missing ⚠️
...c/Services/Attachments/AttachmentSubmitHandler.php 45.45% 6 Missing ⚠️
src/Entity/Attachments/Attachment.php 92.59% 4 Missing ⚠️
src/Services/Attachments/AttachmentManager.php 72.72% 3 Missing ⚠️
src/EntityListeners/AttachmentDeleteListener.php 0.00% 2 Missing ⚠️
src/DataFixtures/PartFixtures.php 0.00% 1 Missing ⚠️
...rc/Services/Attachments/AttachmentPathResolver.php 66.66% 1 Missing ⚠️
... and 2 more
Additional details and impacted files
@@             Coverage Diff              @@
##             master     #848      +/-   ##
============================================
- Coverage     60.28%   60.24%   -0.04%     
- Complexity     5972     5982      +10     
============================================
  Files           525      525              
  Lines         20348    20383      +35     
============================================
+ Hits          12266    12280      +14     
- Misses         8082     8103      +21     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@Treeed Treeed force-pushed the SplitAttachmentPaths branch 2 times, most recently from 7202f7f to c8e6612 Compare February 2, 2025 05:25
…l source URL can be retained after a file is downloaded
@Treeed Treeed force-pushed the SplitAttachmentPaths branch from c8e6612 to ceb7c7b Compare February 2, 2025 05:42
@jbtronics
Copy link
Member

This is a useful and reasonable change, I think. Thank you for the PR.

I am currently going through it, add the missing translations and change some smaller things.
I made the external_path and internal_path columns nullable in the DB and use null as empty value. In my opinion, this is more logical and consistent with other empty value definitions.

Also, I think we should keep the media_url property in the serialized API output for backward compatibility (while removing its documentation, so that it is not used for newer software). That should be an easy addition.

@jbtronics jbtronics merged commit 29f92d9 into Part-DB:master Feb 22, 2025
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants