Do not delete the original file if it's not a temporary file when sending it to a room. #3819

bmarty · 2024-11-06T14:50:29Z

Content

When sending file using Element X the original file may be deleted by the application. This is an error of course and this PR fixes it.

This PR also ensure that temporary files that are created when taking a photo or a video are correctly deleted if the process is cancelled. Else such file were living in the application cache folder until this one get deleted.

Motivation and context

Fixes #3800

Screenshots / GIFs

Tests

You may use apps from Sharing to Element X from certain apps deletes original files #3800 to test that the PR fixes the issue CC @vidyava
Send a file from the camera and observe that even if the operation is cancelled, there is no remaining file in the cache folder of the app.

Tested devices

Physical
Emulator
OS version(s):

Checklist

Changes have been tested on an Android device or Android emulator with API 23
UI change has been tested on both light and dark themes
Accessibility has been taken into account. See https://github.com/element-hq/element-x-android/blob/develop/CONTRIBUTING.md#accessibility
Pull request is based on the develop branch
Pull request title will be used in the release note, it clearly define what will change for the user
Pull request includes screenshots or videos if containing UI changes
Pull request includes a sign off
You've made a self review of your PR

github-actions · 2024-11-06T14:55:49Z

📱 Scan the QR code below to install the build (arm64 only) for this PR.

If you can't scan the QR code you can install the build via this link: https://i.diawi.com/3CUFXa

ganfra · 2024-11-06T16:37:09Z

...testutils/src/main/kotlin/io/element/android/tests/testutils/fake/FakeTemporaryUriDeleter.kt

+import io.element.android.tests.testutils.lambda.lambdaError
+
+class FakeTemporaryUriDeleter(
+    val deleteCallback: (uri: Uri?) -> Unit = { lambdaError() }


name it deleteLambda like we do elsewhere?

ganfra · 2024-11-06T16:38:48Z

...io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewPresenter.kt

@@ -57,6 +60,20 @@ class AttachmentsPreviewPresenter @AssistedInject constructor(

        val ongoingSendAttachmentJob = remember { mutableStateOf<Job?>(null) }

+        DisposableEffect(Unit) {


What if it's disposed for some reason (new screen on the backstack, config changes not handled)?

Yes good point, so I guess I have to handle the cancellation with an event.

bmarty · 2024-11-07T08:37:22Z

@ganfra thanks for the review, I have taken your remarks in to account. Let me know if it's fine for you!

codecov · 2024-11-07T09:01:57Z

Codecov Report

Attention: Patch coverage is 80.88235% with 13 lines in your changes missing coverage. Please review.

Project coverage is 82.98%. Comparing base (64b1f30) to head (8dfe530).
Report is 7 commits behind head on develop.

Files with missing lines	Patch %	Lines
...libraries/androidutils/file/TemporaryUriDeleter.kt	0.00%	7 Missing ⚠️
...impl/attachments/preview/AttachmentsPreviewView.kt	50.00%	2 Missing ⚠️
.../impl/user/editprofile/EditUserProfilePresenter.kt	88.88%	0 Missing and 2 partials ⚠️
.../roomdetails/impl/edit/RoomDetailsEditPresenter.kt	88.88%	0 Missing and 2 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3819      +/-   ##
===========================================
+ Coverage    82.97%   82.98%   +0.01%     
===========================================
  Files         1782     1783       +1     
  Lines        44911    44961      +50     
  Branches      5287     5288       +1     
===========================================
+ Hits         37265    37311      +46     
- Misses        5799     5805       +6     
+ Partials      1847     1845       -2

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

ganfra

I've just realised this might be be incompatible with the new async media upload?

ganfra · 2024-11-07T13:57:33Z

...tlin/io/element/android/features/messages/impl/attachments/preview/AttachmentsPreviewView.kt

-        LaunchedEffect(state.sendActionState) {
-            latestOnDismiss()
-        }
+    BackHandler(enabled = state.sendActionState !is SendActionState.Sending) {


Do we really need this check?

The cancellation during the sending state is emitting a ClearSendState event, so I did not want to change this behavior.

ganfra · 2024-11-07T14:58:47Z

I've just realised this might be be incompatible with the new async media upload?

Lets rebase on develop with the latest changes and see how it reacts?

vidyava · 2024-11-07T23:00:06Z

I can confirm that the issue is fixed on my physical device with the debug build posted earlier. I'll give it another try with the rebased build when one is available. Thanks for the quick turnaround on this!

bmarty · 2024-11-08T08:50:01Z

Thanks a lot @vidyava . I need to rebase the branch to test again with the new way we send media, introduced in #3827

…. No functional change here.

…load.

sonarcloud · 2024-11-08T09:20:13Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

bmarty · 2024-11-08T09:31:39Z

It seems to work as expected 🎉

bmarty added the PR-Bugfix For bug fix label Nov 6, 2024

bmarty requested a review from a team as a code owner November 6, 2024 14:50

bmarty requested review from ganfra and removed request for a team November 6, 2024 14:50

bmarty changed the title ~~Feature/bma/fix delete original file~~ Do not delete the original file if it's not a temporary file when sending it to a room. Nov 6, 2024

bmarty added the Z-NextRelease For issues and PRs which should be included in the NextRelease. label Nov 6, 2024

ganfra reviewed Nov 6, 2024

View reviewed changes

ganfra reviewed Nov 7, 2024

View reviewed changes

bmarty added 6 commits November 8, 2024 10:11

MediaPreProcessor: remove default value of parameter deleteOriginal…

13ec183

…. No functional change here.

Do not delete the original file when sending a media. Fixes #3800.

58a0875

Add a log when deleting a file.

0c84144

Delete temporary created files.

585b6a9

Delete the temporary file only when the user explicitly cancel the up…

43053de

…load.

Rename parameter.

8dfe530

bmarty force-pushed the feature/bma/fixDeleteOriginalFile branch from b13605d to 8dfe530 Compare November 8, 2024 09:13

ganfra approved these changes Nov 8, 2024

View reviewed changes

bmarty merged commit a0fb244 into develop Nov 8, 2024
25 of 26 checks passed

bmarty deleted the feature/bma/fixDeleteOriginalFile branch November 8, 2024 10:45

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Do not delete the original file if it's not a temporary file when sending it to a room. #3819

Do not delete the original file if it's not a temporary file when sending it to a room. #3819

bmarty commented Nov 6, 2024

github-actions bot commented Nov 6, 2024 •

edited

Loading

ganfra Nov 6, 2024

bmarty Nov 7, 2024

ganfra Nov 6, 2024

bmarty Nov 6, 2024

bmarty Nov 7, 2024

bmarty commented Nov 7, 2024

codecov bot commented Nov 7, 2024 •

edited

Loading

ganfra left a comment

ganfra Nov 7, 2024

bmarty Nov 7, 2024

ganfra commented Nov 7, 2024

vidyava commented Nov 7, 2024

bmarty commented Nov 8, 2024

sonarcloud bot commented Nov 8, 2024

bmarty commented Nov 8, 2024

		@@ -57,6 +60,20 @@ class AttachmentsPreviewPresenter @AssistedInject constructor(

		val ongoingSendAttachmentJob = remember { mutableStateOf<Job?>(null) }

		DisposableEffect(Unit) {

Do not delete the original file if it's not a temporary file when sending it to a room. #3819

Do not delete the original file if it's not a temporary file when sending it to a room. #3819

Conversation

bmarty commented Nov 6, 2024

Content

Motivation and context

Screenshots / GIFs

Tests

Tested devices

Checklist

github-actions bot commented Nov 6, 2024 • edited Loading

ganfra Nov 6, 2024

Choose a reason for hiding this comment

bmarty Nov 7, 2024

Choose a reason for hiding this comment

ganfra Nov 6, 2024

Choose a reason for hiding this comment

bmarty Nov 6, 2024

Choose a reason for hiding this comment

bmarty Nov 7, 2024

Choose a reason for hiding this comment

bmarty commented Nov 7, 2024

codecov bot commented Nov 7, 2024 • edited Loading

Codecov Report

ganfra left a comment

Choose a reason for hiding this comment

ganfra Nov 7, 2024

Choose a reason for hiding this comment

bmarty Nov 7, 2024

Choose a reason for hiding this comment

ganfra commented Nov 7, 2024

vidyava commented Nov 7, 2024

bmarty commented Nov 8, 2024

sonarcloud bot commented Nov 8, 2024

Quality Gate passed

bmarty commented Nov 8, 2024

github-actions bot commented Nov 6, 2024 •

edited

Loading

codecov bot commented Nov 7, 2024 •

edited

Loading