[BR-2136]:fix/upload unaccepted file deletes folder#2021
Conversation
Deploying drive-web with
|
| Latest commit: |
5614b24
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://0625e237.drive-web.pages.dev |
| Branch Preview URL: | https://fix-folder-upload-delete-whe.drive-web.pages.dev |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ithub.com/internxt/drive-web into fix/folder-upload-delete-when-a-file-fail
| isMaxSpaceError: boolean; | ||
| continueWithRemainingFiles: boolean; | ||
| }): void { | ||
| tasksService.updateTask({ |
There was a problem hiding this comment.
Although I haven't been too serious about this, the UploadManager is not the place to manage the logger tasks lifecycle, which is pure UI.
|
|
||
| const options = { withNotification: true, ...payloadOptions }; | ||
| options.onSuccess?.(); | ||
| referralService.trackFolderUpload(); |
There was a problem hiding this comment.
This is the kind of things that make the code dirty without any gain: the upload folder manager has the responsibility of managing the folder's upload, not the tracking system. If you want to track something, expose a listener on the manager (or a callback like onFinish. In fact, the onSuccess callback above this line is a good place to do that) and keep the manager clean of non-context-related tasks
| logNetworkInfoForUpload({ folderName: root.name }); | ||
|
|
||
| setTimeout(() => { | ||
| this.dispatch(planThunks.fetchUsageThunk()); |
There was a problem hiding this comment.
Same here, this is the global status of the app, it should not be coupled to how the upload is managed on the context of the network:
A upload folder manager on the network folder is the place where the codebase manages exclusively the upload of a folder: sending the content, confirming the upload, emitting status events (error, success, progress) to any interested consumer and that's all.
The incipient codebase here where half the things are Redux, trackers or logger tasks management is something whose prevention we should start to enforce @CandelR, as it is starting to be out of hands.
|
Am sorry you are involved here @jaaaaavier but this is part of maintaining the project and from time to time you may be involved in correcting something that could not be directly related to your changes, but it's a part of being a good professional, remember it any time you introduce technical debt as other teammate could be affected in a near future haha. |
No problem, understood |
|
2 participants
Description
A bug was detected whereby a user attempted to upload a prohibited file (0 bytes) within a folder, when this was detected, the entire folder was deleted and the upload never took place, yet the task was marked as successful. Consequently, the user could search for the folder but would never find anything.
The purpose of this PR is to change the behaviour of this error: essentially, we will no longer delete files that have already been uploaded, and we will continue uploading the remaining pending files. Once this process is complete, we will display the relevant error message and allow the user to see which files have failed, but they will not be able to re-upload them.
Related Issues
Related Pull Requests
Checklist
Testing Process
After creating a new free account, uploading a folder containing valid files and a 0-byte file, verifying that the 0-byte file does not upload whilst the rest do, confirming that the workflow in which we can see the failed file does not allow it to be uploaded, and testing the upload of a normal folder, which uploads correctly
Additional Notes