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

Allow the upload of empty folder on drop #2164

Merged
merged 16 commits into from
Jun 10, 2022
Merged

Allow the upload of empty folder on drop #2164

merged 16 commits into from
Jun 10, 2022

Conversation

Tbaut
Copy link
Collaborator

@Tbaut Tbaut commented Jun 2, 2022

closes #2104

it's for Files for now, I'll do it for Storage as well if all goes well.

@render
Copy link

render bot commented Jun 2, 2022

@render
Copy link

render bot commented Jun 2, 2022

@render
Copy link

render bot commented Jun 2, 2022

@Tbaut Tbaut added the Type: Feature Added to PRs to identify that the change is a new feature. label Jun 2, 2022
Copy link
Contributor

@juans-chainsafe juans-chainsafe left a comment

Choose a reason for hiding this comment

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

So, the test related with upload a file are failing, because the file is not being uploaded.
Here is a video of what is happening:

Screen.Recording.2022-06-02.at.15.59.00.mov

Basically, after you select a file nothing happens, the file is not added in the modal.
Drag and drop a file is working correctly, the problem is when you try to select a file from your machine.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Jun 2, 2022

Oh, I completely forgot this case, I didn't expect to break it actually, I focused on the drag/drop. Thanks, I'll check it out.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Jun 3, 2022

Upload by clicking and selecting files should work as well now too.
I tested with a mix between drag (with empty folders) and manual selection, and it also working.
We def. need to test this thoroughly because there's a lot going on here.

Copy link
Contributor

@tanmoyAtb tanmoyAtb left a comment

Choose a reason for hiding this comment

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

Amazing work, Its working well with a combination of cases.

Two things we can improve.

  1. If we simply add an empty folder, the toast says encrypting and uploading 0 files, better not to show toast if we are not uploading files and simply adding folders

  2. We need a refresh bucket call after the folder creation API calls, otherwise after the uploads call, there will be no refresh after the folder creation calls and we wouldn't see the folders created.

Adding a recording here.

Screen.Recording.2022-06-03.at.3.44.13.PM.mov

@tanmoyAtb
Copy link
Contributor

BTW if we want to add this into storage in this PR, we'd better merge in #2165 before, Folder uploads have been added in that PR.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Jun 3, 2022

Yup, saw it yesterday, I'd add it after

@juans-chainsafe
Copy link
Contributor

Nice, now is working on files (upload file by selecting it), but... is not working on Storage, but that was working fine 4 hours ago, seems that has to be with the merge maybe? but well this is the error:
"After you select a file nothing happens, the file is not added in the modal."

Also, I agree with Tanmoy, will be great to see the folder added at the moment (if you import an empty folder), so you don't need to refresh the page

@Tbaut
Copy link
Collaborator Author

Tbaut commented Jun 3, 2022

Thanks for the tests y'all. @tanmoyAtb great suggestions, it should be in now, a refresh happens after the folder creation now and we don't show the dialog if all there is, is an empty directory.

@Tbaut
Copy link
Collaborator Author

Tbaut commented Jun 3, 2022

@juans-chainsafe I guess it's related to what we merged indeed, it uses the same components. Also this PR will conflict most probably with the NFT upload one, it's touching the same components.

@Tbaut Tbaut requested a review from tanmoyAtb June 3, 2022 14:42
@juans-chainsafe
Copy link
Contributor

@Tbaut now is refreshing correctly the empty folder when is imported, nice!

As well, the error in Storage was fixed, but we have a new error in Storage, when we upload files from the file upload modal, is not automatically refreshing the table, so the files doesn't appear instantly (you need to refresh the page to see the files). Video below:

Screen.Recording.2022-06-03.at.11.58.47.mov

@Tbaut
Copy link
Collaborator Author

Tbaut commented Jun 3, 2022

Yup, saw it, working on it

@Tbaut
Copy link
Collaborator Author

Tbaut commented Jun 3, 2022

let's see how much the NFT will break stuff. We should merge it before this one.

@juans-chainsafe
Copy link
Contributor

Seems to be working as expected now in Storage! let's wait for the NFT PR to be merged and I found something:

Something that I noticed testing this (is not from this PR) is that:

  1. Create new folder and delete it
  2. Create new folder with the same name and add some files
  3. Delete that folder and go to Bin
    In Bin, the content will go to the first folder that was deleted (because the second one that was deleted has the name {folderName} (1) ) so if I keep creating folders with the same name and delete them, the content inside the folder will go to the older folder.

I just wanted to comment on this behavior, maybe you've seen it before @Tbaut @asnaith @FSM1

@Tbaut
Copy link
Collaborator Author

Tbaut commented Jun 6, 2022

Ah good one Juan, I'll open an issue for that, it's a bug

@asnaith
Copy link
Member

asnaith commented Jun 6, 2022

In Bin, the content will go to the first folder that was deleted (because the second one that was deleted has the name {folderName} (1) ) so if I keep creating folders with the same name and delete them, the content inside the folder will go to the older folder.

@juans-chainsafe I wasn't aware of this bug, good discovery!

Copy link
Contributor

@tanmoyAtb tanmoyAtb left a comment

Choose a reason for hiding this comment

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

Looks amazing on both storage and files !

@Tbaut
Copy link
Collaborator Author

Tbaut commented Jun 9, 2022

I wouldn't be against a last test on file upload since the conflicts weren't super small. It still looks good from my test with drag or manual upload.

Copy link
Contributor

@juans-chainsafe juans-chainsafe left a comment

Choose a reason for hiding this comment

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

I reviewed again and all seems to be working perfect! I didn't see anything strange or failing, so nice work!

@Tbaut Tbaut merged commit e27e367 into dev Jun 10, 2022
@Tbaut Tbaut deleted the tbaut-empty-folders-2104 branch June 10, 2022 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Type: Feature Added to PRs to identify that the change is a new feature.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Empty directories are not uploaded.
5 participants