-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
BugFix - Offline Operation Conflict Handling #15027
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
base: master
Are you sure you want to change the base?
Conversation
@@ -57,32 +56,6 @@ class FileUploaderIntents(private val context: Context) { | |||
) | |||
} | |||
|
|||
fun resultIntent(resultCode: ResultCode, operation: UploadFileOperation): PendingIntent { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unused function
8f0bc47
to
542a8b0
Compare
…tion if delete op successfully completed, change text delete offline folder to delete file or delete folder Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
542a8b0
to
7637906
Compare
Signed-off-by: alperozturk <[email protected]>
Signed-off-by: alperozturk <[email protected]>
<string name="offline_operations_worker_notification_remove_conflict_text">Offline operation cancelled. Could not delete %s file exists on server.</string> | ||
<string name="offline_operations_worker_notification_rename_conflict_text">Offline operation cancelled. Could not rename %s file exists on server.</string> | ||
<string name="offline_operations_worker_notification_create_file_conflict_text">Cannot create file: %s already exists on server.</string> | ||
<string name="offline_operations_worker_notification_create_folder_conflict_text">Cannot create folder: %s already exists on server.</string> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kra-mo Could you review the wording and let me know if it works?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hm, I'm not sure if "on server" is necessary. Ideally, the local and remote states are synced, so I don't think the user has to know where exactly the conflict happened to understand the error, maybe they could be simpler and more clear. I think "Offline operation cancelled" is also redundant/not relevant.
With regard to the first one, I don't particularly understand it either. If the file exists remotely, why can't you delete it locally? But I'm assuming it's the opposite, that the file doesn't exist?
<string name="offline_operations_worker_notification_remove_conflict_text">Offline operation cancelled. Could not delete %s file exists on server.</string> | |
<string name="offline_operations_worker_notification_rename_conflict_text">Offline operation cancelled. Could not rename %s file exists on server.</string> | |
<string name="offline_operations_worker_notification_create_file_conflict_text">Cannot create file: %s already exists on server.</string> | |
<string name="offline_operations_worker_notification_create_folder_conflict_text">Cannot create folder: %s already exists on server.</string> | |
<string name="offline_operations_worker_notification_remove_conflict_text">Could not delete %s, the file no longer exists.</string> | |
<string name="offline_operations_worker_notification_rename_conflict_text">Could not rename %s, a file with the same name already exists.</string> | |
<string name="offline_operations_worker_notification_create_file_conflict_text">Could not create %s, a file with the same name already exists.</string> | |
<string name="offline_operations_worker_notification_create_folder_conflict_text">Could not create %s, a folder with the same name already exists.</string> | |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the feedback. I think a bit of context might help clarify things.
We have an offline operations feature that allows users to queue actions like delete, rename, create file, or create folder while the device is offline. These operations are then executed once the connection is restored.
For delete, rename operation:
If a user queues a delete action while offline, but in the meantime, another user modifies that file or folder on the server, we end up with a conflict. The local operation wanted to "delete," but the remote version has changed, so which one is the source of truth we don't know at this moment.
In that case, we cannot proceed with the deletion silently, because doing so might override someone else's changes. That's why we cancel the offline operation and notify the user, instead of executing a potentially destructive action.
Regarding the message content:
I included “offline operation cancelled” so the user understands the operation won’t be executed later.
I kept “on the server” to clarify that the conflict is due to changes on the remote side, not a duplicate or issue on the local device.
Phrasing like “file with the same name already exists” might be misunderstood as a local duplicate, so specifying the server helps prevent confusion.
I hope that makes the intent behind the messages a bit clearer. I find the current version longer and wanted to give the same meaning with fewer words, actually.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see. In that case, providing the context does make sense. How about something like this?
In the case of a name conflict, I would want to specifically emphasize that, as the local file is not what exists on the server, but one with the same name.
<string name="offline_operations_worker_notification_remove_conflict_text">Offline operation cancelled. Could not delete %s file exists on server.</string> | |
<string name="offline_operations_worker_notification_rename_conflict_text">Offline operation cancelled. Could not rename %s file exists on server.</string> | |
<string name="offline_operations_worker_notification_create_file_conflict_text">Cannot create file: %s already exists on server.</string> | |
<string name="offline_operations_worker_notification_create_folder_conflict_text">Cannot create folder: %s already exists on server.</string> | |
<string name="offline_operations_worker_notification_remove_conflict_text">Cancelled deletion of %s. The file does not exist on the server.</string> | |
<string name="offline_operations_worker_notification_rename_conflict_text">Cancelled rename of %s. A file with the same name exists on the server.</string> | |
<string name="offline_operations_worker_notification_create_file_conflict_text">Could not create %s. A file with the same name exists on the server.</string> | |
<string name="offline_operations_worker_notification_create_folder_conflict_text">Could not create %s. A folder with the same name exists on the server.</string> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Delete same as rename (a file with the same name exists on the server that's why it's a conflict.) Thus should be like this.
<string name="offline_operations_worker_notification_remove_conflict_text">Cancelled deletion of %s. A file with the same name exists on the server.</string>
Suggestions are looks good to me. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still not sure I understand. If the file does exist, then why can't you delete it haha?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Although the file does exist, we cannot proceed with the deletion due to a conflict. e.g:
- User A (while offline) queued a request to delete Folder C (which, at that time, was 19 KB).
- While User A’s device was still offline, User C (via a web client) added a new file to Folder C, increasing its size to 1 MB.
- When User A’s device comes back online, it attempts to execute the previously queued deletion request. However, the folder has since been modified by another user (User C).
To prevent unintended data loss, we block User A's deletion request, executing it would override User C’s more recent changes.
/backport to stable-3.32 |
APK file: https://www.kaminsky.me/nc-dev/android-artifacts/15027.apk |
blue-Light-Screenshot test failed, but no output was generated. Maybe a preliminary stage failed. |
This PR improves the handling of file and folder conflicts during offline operations.
Changes:
Removed the HashMap previously used in
RefreshFolderOperation
andOfflineOperationActionReceiver
.Conflict detection is now handled directly by
OfflineOperationWorker
.Conflict detection logic checks if the target path already exists on the server. If it does, a conflict is assumed.
Handling Conflicts
Since offline operations do not have access to server-side metadata such as eTags, we cannot reliably compare local and remote versions of a file or folder. To prevent unintended overwrites: If the remote path exists, we treat it as a conflict.
Handling Create File & Create Folder
Users are prompted with a
ConflictResolveDialog
to choose how to proceed.Handling Remove & Rename
User interaction is not allowed via
ConflictResolveDialog:
These operations lack meaningful resolution options like "Keep Local."
For example, if a user modified a folder on the server and another offline operation attempted to delete the same folder, prompting the user to "keep local" would result in deleting the modified folder, which is not intuitive.
Supporting (Delete Folder, Delete File, Rename Folder, Rename File -- which files in the folder can be deleted which ones not etc.) these operations in the
ConflictResolveDialog
would introduce unnecessary complexity.Instead:
Users are notified of the conflict via a notification.
The corresponding offline operation is removed from the database.
If the user still intends to rename or delete the file/folder, they can manually retry the operation.