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

[BUG] Av. offline files removed locally with the Local only option #4399

Merged

Conversation

Aitorbp
Copy link
Contributor

@Aitorbp Aitorbp commented May 7, 2024

Related Issues

App:#4353

  • 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

Reports:

@Aitorbp Aitorbp self-assigned this May 7, 2024
@Aitorbp Aitorbp linked an issue May 7, 2024 that may be closed by this pull request
@Aitorbp Aitorbp force-pushed the fix/av_offline_files_removed_locally_with_local_only_option branch 2 times, most recently from 4664694 to abd2de1 Compare May 8, 2024 06:44
@Aitorbp Aitorbp requested a review from JuancaG05 May 8, 2024 07:07
@Aitorbp Aitorbp changed the base branch from master to fix/av_offline_operation_does_not_work_when_run_from_parent_folder May 14, 2024 07:24
@Aitorbp Aitorbp force-pushed the fix/av_offline_files_removed_locally_with_local_only_option branch 4 times, most recently from 9f4d4f4 to c753572 Compare May 14, 2024 13:30
@Aitorbp Aitorbp changed the base branch from fix/av_offline_operation_does_not_work_when_run_from_parent_folder to master June 3, 2024 15:46
@Aitorbp Aitorbp force-pushed the fix/av_offline_files_removed_locally_with_local_only_option branch 2 times, most recently from c3f0242 to 9d25437 Compare June 4, 2024 11:56
@JuancaG05 JuancaG05 changed the title [BUG] Av. offline files removed locally with the Local only option [BUG] Av. offline files removed locally with the Local only option Jun 5, 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 and comments here for you to review @Aitorbp!

@Aitorbp Aitorbp force-pushed the fix/av_offline_files_removed_locally_with_local_only_option branch from 527fbcd to f1716f8 Compare June 6, 2024 08:24
@Aitorbp
Copy link
Contributor Author

Aitorbp commented Jun 6, 2024

Checks functionalities of deleteLocalFolderRecursively:

Case Delete files only local:

  1. Create a folder with an available offline file and another downloaded file inside.
  2. Delete the folder only local.
  3. Check in Device Explorer that only the downloaded file has been deleted and both the parent folder and the available offline file are kept.

Case refresh on a folder that no longer exists:

  1. Create a folder with available offline and downloaded files inside.
  2. Delete the folder from the web.
  3. Do a refresh in the main list.
  4. Check that all files and also the available offline files have been deleted on that folder in Device Explorer

Case move folder with a source folder that no longer exists:

  1. Create a folder with downloaded and available offline files.
  2. Start the process of moving the folder.
  3. Go to the web and delete the source folder.
  4. End the move process.
  5. Check Device Explorer to see if the entire source folder has been deleted.

If you move a folder to a destination folder that no longer exists:

  1. Move a folder or file to a folder with available offline and downloaded files inside.
  2. In the process of moving it, we delete the folder from the web.
  3. We finish the process of moving the folder.
  4. Check Device Explorer to see if the entire destination folder has been deleted

If you copy a folder from a source folder that no longer exists:

  1. Create a folder with download and available offline files.
  2. Start the process of copying the folder.
  3. Go to the web and delete the source folder.
  4. Check Device Explorer to see if the entire source folder has been deleted when the process is finished

If you copy a folder to a destination folder that no longer exists:

  1. Copy a folder or file to a folder with available offline and downloaded files inside.
  2. In the process of moving it, we delete the folder from the web.
  3. We finish the process of moving the folder.
  4. Check in Device Explorer if the entire destination folder has been deleted.

I leave you these test cases in case you want to try them when doing QA @jesmrec

@Aitorbp Aitorbp requested a review from JuancaG05 June 6, 2024 12:22
@Aitorbp Aitorbp force-pushed the fix/av_offline_files_removed_locally_with_local_only_option branch from 5e63372 to 29cb5f9 Compare June 7, 2024 12:39
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 more things to check here @Aitorbp 👀

@Aitorbp Aitorbp force-pushed the fix/av_offline_files_removed_locally_with_local_only_option branch from 62592e4 to abf4f9e Compare June 9, 2024 14:27
@Aitorbp Aitorbp requested a review from JuancaG05 June 10, 2024 18:09
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 more change, and review as well the non-resolved comment of the previous CR round @Aitorbp

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.

@Aitorbp an explanation would be nice here, so that we can check it in the future if we forget

@Aitorbp Aitorbp requested a review from JuancaG05 June 12, 2024 08:17
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.

Let's move this to QA 🚀

@Aitorbp Aitorbp force-pushed the fix/av_offline_files_removed_locally_with_local_only_option branch from 2e97003 to 62a76c5 Compare June 12, 2024 15:24
@jesmrec
Copy link
Collaborator

jesmrec commented Jun 12, 2024

First, checks over #4402, that is merged into here

Folder with some files and a subfolders that also contains some files

Folder1
|_ File1
|_ File2
|_ Folder2
    |_ File3
    |_ File4

  • Nothing downloaded + Av. offline + Unset av. offline -> Everything downloaded
  • File1 & File2 downloaded + remove locally File2 + Av. offline + Unset av. offline -> Everything downloaded
  • File3 & File4 downloaded + remove locally File3 + Av. offline + Unset av. offline -> Everything downloaded
  • File1 & File3 downloaded + remove locally File3 + Av. offline + Unset av. offline -> Everything downloaded
  • File1 & File3 downloaded + remove locally File3 & File1 + Av. offline + Unset av. offline -> Everything downloaded
  • Everything downloaded + remove everything locally + Av. offline + Unset av. offline -> Everything downloaded

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 13, 2024

Basic behaviour:

Remove folder dialog when...:

  • Nothing is downloaded inside -> Local only not displayed in dialog
  • Some files are dowloaded and others are not downloaded -> Local only displayed in dialog
  • Some files are downloaded and others are av. offline -> Local only displayed in dialog
  • Some files are av. offline and others are not downloaded -> Local only not displayed in dialog
  • Everything is downloaded -> Local only displayed in dialog
  • Everything is av. offline -> Local only not displayed in dialog

All those tests were repeated with files inside a subfolder.

Local copies of the items removed externally:

  • Remove file from server -> locally removed
  • Remove folder from server -> locally removed including local copies
  • Remove folder during copy/move operations -> locally removed including local copies

@jesmrec
Copy link
Collaborator

jesmrec commented Jun 13, 2024

(1) [FIXED]

I noticed the bad behaviour when a sync is performed (not an av. offline);

With the following structure:

Folder with some files and a subfolders that also contains some files

Folder1
|_ File1
|_ File2
|_ Folder2
    |_ File3
    |_ File4

  1. Download File3
  2. Remove locally File3
  3. Sync Folder1
  4. Browse to Folder2

Current:

File3 and File 4 are not downloaded.

Expected:

Both File3 and File4 are downloaded, because their parent folder was synced in the previous step.

That was the problem fixed in #4402 from the av. offline files, that seems to be reproducible with sync.

Pixel 2, Android 11
62a76c55

@Aitorbp Aitorbp force-pushed the fix/av_offline_files_removed_locally_with_local_only_option branch from bb9cacf to b6c01f1 Compare June 13, 2024 10:34
@Aitorbp Aitorbp force-pushed the fix/av_offline_files_removed_locally_with_local_only_option branch from b6c01f1 to 080a201 Compare June 13, 2024 10:36
@jesmrec
Copy link
Collaborator

jesmrec commented Jun 14, 2024

Approved on my side.

@Aitorbp Aitorbp merged commit 8af346d into master Jun 14, 2024
5 checks passed
@Aitorbp Aitorbp deleted the fix/av_offline_files_removed_locally_with_local_only_option branch June 14, 2024 08:26
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.

[BUG] Av. offline files removed locally with the Local only option
4 participants