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

[SPACES] Support file and folders operations with Spaces in DocumentsProvider #3941

Merged
merged 2 commits into from
Mar 7, 2023

Conversation

JuancaG05
Copy link
Collaborator

Related Issues

App: #3881


QA

@JuancaG05 JuancaG05 linked an issue Feb 28, 2023 that may be closed by this pull request
11 tasks
@JuancaG05 JuancaG05 changed the base branch from master to spaces/uploads February 28, 2023 14:43
@JuancaG05 JuancaG05 marked this pull request as ready for review March 1, 2023 12:50
@JuancaG05 JuancaG05 requested a review from abelgardep March 1, 2023 12:50
Base automatically changed from spaces/uploads to spaces/main March 2, 2023 13:04
@JuancaG05 JuancaG05 self-assigned this Mar 2, 2023
@jesmrec
Copy link
Collaborator

jesmrec commented Mar 6, 2023

One open question here:

"Move to other space in same account" operation is banned in the app, but available in doc provider.

i guess that it's not straightforward to prevent this... but it is a shortcut to jump over the app restriction. Is this OK?

CC @michaelstingl

@JuancaG05
Copy link
Collaborator Author

In Android, since we use the native documents provider, we cannot restrict this behaviour.

In fact, the move operation is not performed as it is, since we invoke the move usecase and an exception is thrown if we try to move to a different space. When this happens, the documents provider catches internally the exception and performs an "artificial" move, which means downloading the file from the source, upload it to the destination and deleting it from the source. This artificial move is performed as well when trying to move between accounts through the documents provider -> not allowed in the app but from the document provider.

In conclusion, a remote move is not performed in server, but we cannot restrict it since it is a native and internal behaviour from Android. We'll see if this is doable in future versions of documents provider.

/CC @michaelstingl

@jesmrec
Copy link
Collaborator

jesmrec commented Mar 7, 2023

A complete sanity test was done over the feature, including operations over oCIS accounts with spaces, oC10 accounts and other providers.

Some open issues are still reproducible:

#3675
#2659
#2658

These three, related with Copy and Move operations (read, upload and delete under the hood) are still there. The most unstable scenario is copying/moving tree structures with more than 2-deep levels. Sometimes works, sometimes fails, sometimes is only copied or moved a part of the files. If the operation is done over two different oC accounts/providers (no matter if oC10 or oCIS), it's unstable even with a few amount of items. My guess: some internal memory restrictions (did not find anything in official documentation about this)

Keep in mind also this issue with improvements that point to the mentioned issues: #2336

Since those are known issues and do not affect the oCIS' spaces implementation, i do not have objections to move this forward, keeping in mind that some scenarios are not working fine. If any other idea/suggestion/solution to improve the known bad scenarios, is totally welcome.

@JuancaG05 JuancaG05 force-pushed the spaces/doc_provider_operations branch from 6373f31 to 9f25263 Compare March 7, 2023 16:18
@abelgardep abelgardep enabled auto-merge March 7, 2023 16:41
@abelgardep abelgardep added this pull request to the merge queue Mar 7, 2023
@abelgardep abelgardep removed this pull request from the merge queue due to a manual request Mar 7, 2023
@abelgardep abelgardep merged commit 9935e66 into spaces/main Mar 7, 2023
@abelgardep abelgardep deleted the spaces/doc_provider_operations branch March 7, 2023 17:39
@JuancaG05 JuancaG05 mentioned this pull request Mar 8, 2023
1 task
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.

[SPACES] Support file and folders operations with Spaces in DocumentsProvider
3 participants