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

feat(files): Import/export files to textile bucket AP-799 #1616

Merged
merged 19 commits into from
Feb 25, 2022
Merged

Conversation

josephmcg
Copy link
Contributor

@josephmcg josephmcg commented Feb 22, 2022

What this PR does 📖

  • Upload/delete files in 'personal-files' textile bucket.
  • After file/folder update, upload a JSON file to bucket which keeps track of directory structure
    • this is done with a recursive import & export. May encounter performance issues eventually, but maybe warp will ease this concern
  • context menu allowing for like/share/delete of items in grid view
    • rename is currently todo
    • sharing folder is currently todo, toast will indicate that
    • I made a ticket about this, but the context menu will need a rework to be more dynamic (showing Favorite/un-favorite depending on current file status)
  • filesystem.hasChild was only checking the current directory. Now it will check a flattened list of all files which is recursively generated
    • let me know if this algorithm can be optimized

Which issue(s) this PR fixes 🔨
AP-799

Special notes for reviewers 🗒️

  • While the directory structure index is being updated, all the files UI will be disabled. If not, it allows for another update request before the current request finishes, which throws an error. We can update the loading ui if it makes the app feel slow
  • I want to improve our nsfw check later. I made a ticket about it already, but it will currently reject certain image types as nsfw if they're unable to be scanned (tiff).

Additional comments 🎤

  • Ticket mentions removing multiple files at once, this is still todo
  • if you delete a folder which has child files, the files will still exist in the bucket. the reference however will be deleted from the index. maybe we should delete everything nested inside, would need more recursion

@github-actions github-actions bot added the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Feb 22, 2022
@josephmcg josephmcg changed the title Import/export files to textile bucket AP-799 feat(files): Import/export files to textile bucket AP-799 Feb 22, 2022
@Satellite-im Satellite-im deleted a comment from github-actions bot Feb 22, 2022
@netlify
Copy link

netlify bot commented Feb 22, 2022

✔️ Yeeeehaw, deploy preview is ready!

🔨 Explore the source changes: 4c65024

🔍 Inspect the deploy log: https://app.netlify.com/sites/adoring-edison-dbcef8/deploys/621821c9f1fb4c00085bc3e9

😎 Browse the preview: https://deploy-preview-1616--adoring-edison-dbcef8.netlify.app

@josephmcg josephmcg added the draft A developer wants eyes on this PR, but they don't think it's ready to merge. label Feb 22, 2022
@josephmcg josephmcg removed the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Feb 22, 2022
@josephmcg josephmcg added Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA draft A developer wants eyes on this PR, but they don't think it's ready to merge. and removed draft A developer wants eyes on this PR, but they don't think it's ready to merge. Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA labels Feb 24, 2022
@josephmcg josephmcg added Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA and removed draft A developer wants eyes on this PR, but they don't think it's ready to merge. labels Feb 24, 2022
@phillsatellite
Copy link
Contributor

Tested: Worked good only issue I came across was I would get a crash whenever trying to upload .heic images
Screen Shot 2022-02-24 at 4 40 41 PM

@phillsatellite phillsatellite added the Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. label Feb 24, 2022
@WanderingHogan WanderingHogan removed the Missing Dev Review A Dev and a Dev Approver need to review the PR, then mark as Ready for QA label Feb 24, 2022
@josephmcg josephmcg added Ready for QA Ready for QA team to test, Devs approved. and removed Changes Requested Changes have been requested by Dev or QA, once the changes have been made, remove this label. labels Feb 25, 2022
@phillsatellite phillsatellite added QA tested One QA Person has tested - Needs QA Lead review still and removed Ready for QA Ready for QA team to test, Devs approved. labels Feb 25, 2022
@stavares843
Copy link
Member

Captura de ecrã 2022-02-25, às 00 52 57

not sure if this is already on your list for other PR but this view is missing a scroll if you upload too many files

also a couple of questions

  • are we gonna have a limit of file size to upload? I tried with 100MB and worked
  • are we gonna have a limit on the number of files to upload at the same time? I tried 9 and worked

I noticed we don't allow uploading a file with the same name even on different folders, is that on purpose?

Gravacao.do.ecra.2022-02-25.as.00.56.10.mov

also if you have an error and you switch folders, the error is still there

Captura de ecrã 2022-02-25, às 00 57 30

@stavares843
Copy link
Member

merging, all of the above can be addressed separately

@stavares843 stavares843 merged commit 2ae1128 into dev Feb 25, 2022
@stavares843 stavares843 deleted the AP799 branch February 25, 2022 00:58
@github-actions github-actions bot removed the QA tested One QA Person has tested - Needs QA Lead review still label Feb 25, 2022
@josephmcg
Copy link
Contributor Author

Thanks Sara

  • this was focused mostly on grid view, I'll make a ticket for list view scroll.
  • idk about file size limit or file number limit. I recall Matt saying 8gb, but that figure may have changed.
  • Yes, disallowing the same name in different folders was intentional. The folders are for organization on the client end. They don't exist in the textile bucket, all the files are at the root. If we use the same name, it would overwrite other files
  • we can clear errors on a folder change if you like. we can consult ui to see if that's better

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.

4 participants