Skip to content
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

[FEATURE REQUEST] Improve the removed from original folder option in Auto Uploads #4437

Merged

Conversation

Aitorbp
Copy link
Contributor

@Aitorbp Aitorbp commented Jul 16, 2024

Related Issues

App:

  • Add changelog files for the fixed issues in folder changelog/unreleased. More info here
  • Add feature to Release Notes in ReleaseNotesViewModel.kt creating a new ReleaseNote() with String resources (if required)

QA

Checks:

#4437 (comment)

Reports:

@Aitorbp Aitorbp self-assigned this Jul 16, 2024
@Aitorbp Aitorbp linked an issue Jul 16, 2024 that may be closed by this pull request
11 tasks
@Aitorbp Aitorbp requested a review from JuancaG05 July 16, 2024 09:40
@JuancaG05 JuancaG05 changed the title [FEATURE REQUEST] Improve the removed from original folder option in Auto Uploads [FEATURE REQUEST] Improve the removed from original folder option in Auto Uploads Jul 19, 2024
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some changes requested here @Aitorbp

@Aitorbp Aitorbp force-pushed the feature/improve_removed_from_original_folder_option_auto_upload branch 3 times, most recently from dc40855 to 84ec62b Compare July 22, 2024 10:25
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment here @Aitorbp

@Aitorbp Aitorbp requested a review from JuancaG05 July 24, 2024 07:05
@Aitorbp Aitorbp force-pushed the feature/improve_removed_from_original_folder_option_auto_upload branch from 3b981b6 to 41c2016 Compare July 29, 2024 15:20
@Aitorbp Aitorbp requested a review from JuancaG05 July 29, 2024 15:23
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Aitorbp
Copy link
Contributor Author

Aitorbp commented Jul 30, 2024

The solution that has been attempted in this issue is the following:
We need the file deletion to be done after that file has been uploaded to the server.
For this we have a worker UploadFileFromContentUriWorker, which performs the upload task. So we must create another worker in the background that runs when UploadFileFromContentUriWorker has been successful. If it has not been successful, the file will not be deleted, avoiding data loss.
Technically, to do this, the workManager's then() function has been used. The content of this function is executed ONLY when the first worker has been successful.

@jesmrec
Copy link
Collaborator

jesmrec commented Jul 30, 2024

Let's QA this

move behaviour:

  • Upload with no errors -> removed from Photo gallery and /tmp
  • Upload with connection error + success retry -> removed from Photo gallery and /tmp
  • Upload with server error + success 1st retry -> removed from Photo gallery and /tmp
  • Upload with server error + fail 1st retry + success 2nd retry -> removed from Photo gallery and /tmp
  • Upload interrupted with other error (server down, f. ex.) + retry -> removed from Photo gallery and /tmp
  • Upload with connection error + pictures removed from gallery after the error + retry -> Folder error ⚠️ (expected) + /tmp clean
  • Upload with server error + pictures removed from gallery after the error + retry -> pics uploaded + /tmp clean

copy behaviour (regression)

  • Upload with no errors -> stay in Photo gallery, removed from /tmp
  • Upload with connection error + retry -> stay in Photo gallery, removed from /tmp
  • Upload with server error + success 1st retry -> stay from Photo gallery, removed from /tmp
  • Upload with server error + fail 1st retry + success 2nd retry -> stay from Photo gallery, removed from /tmp
  • Upload interrupted with other error (server down, f. ex.) + retry -> stay from Photo gallery, removed from /tmp

@jesmrec
Copy link
Collaborator

jesmrec commented Jul 30, 2024

(1) [FIXED]

i did the following steps:

  1. Enable Auto uploads for pictures with removed from original folder behaviour
  2. Take some pictures
  3. Switch server off (it will make the uploads fail)
  4. Wait till pictures are enqueued/failed
  5. Switch server on
  6. Retry failed uploads

Current: uploads are finally uploaded, but they are still in local
Expected: uploads are finally uploaded, but they are removed from local because the selected behaviour in Settings

NOTE: with connection loss, it works fine.

Pixel 2, Android11
41c2016a0

@Aitorbp
Copy link
Contributor Author

Aitorbp commented Aug 6, 2024

(1)

i did the following steps:

  1. Enable Auto uploads for pictures with removed from original folder behaviour
  2. Take some pictures
  3. Switch server off (it will make the uploads fail)
  4. Wait till pictures are enqueued/failed
  5. Switch server on
  6. Retry failed uploads

Current: uploads are finally uploaded, but they are still in local Expected: uploads are finally uploaded, but they are removed from local because the selected behaviour in Settings

NOTE: with connection loss, it works fine.

Pixel 2, Android11 41c2016a0

To put this problem into context, first of all we need to know the different behaviors between how the worker works when there is no connection and when the server is off.

No connection: The worker is not executed, therefore the upload remains queued until the connection is recovered.

Server off: The worker is executed, but the upload ends up failing because the server is not operational and the upload gives a Connection error. We will need to click Retry to try the upload again.

The problem is that within the UploadFileFromContentUriWorker worker the following copyFileToLocalStorage() function is being executed. This method updates the localPath to tmp. Therefore when we do the Retry it will delete the file that we have in the tmp folder and not the file that is on the device.

As a solution, we have created a new column (sourcePath) in the Transfers table, where we save the path of the file on the device, since we have no way to recover it once the upload is ready to do a Retry. In this way, when we click on Retry, the file we have on the device will be deleted when the upload is finished.

@Aitorbp Aitorbp force-pushed the feature/improve_removed_from_original_folder_option_auto_upload branch 2 times, most recently from 8a346d8 to b18aeb7 Compare August 13, 2024 08:35
Copy link

@mplazaspalacio mplazaspalacio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments

@Aitorbp Aitorbp requested a review from mplazaspalacio August 14, 2024 10:49
Copy link

@mplazaspalacio mplazaspalacio left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Aitorbp Aitorbp force-pushed the feature/improve_removed_from_original_folder_option_auto_upload branch from 0f2c8b0 to 57a3476 Compare August 14, 2024 13:40
@jesmrec
Copy link
Collaborator

jesmrec commented Aug 19, 2024

Let's move this one. Now, with move behaviour, original files are removed just when the file is uploaded and submitted to the server, preventing data loss caused by non-recoverable failures.

Good one, improving our reliability 💯

@Aitorbp Aitorbp merged commit 254388d into master Aug 19, 2024
5 checks passed
@Aitorbp Aitorbp deleted the feature/improve_removed_from_original_folder_option_auto_upload branch August 19, 2024 10:04
@jesmrec jesmrec mentioned this pull request Oct 24, 2024
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[FEATURE REQUEST] Improve the removed from original folder option in Auto Uploads
4 participants