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: support vfolder trash bin #2204

Merged
merged 29 commits into from
Apr 1, 2024
Merged

feat: support vfolder trash bin #2204

merged 29 commits into from
Apr 1, 2024

Conversation

agatha197
Copy link
Contributor

@agatha197 agatha197 commented Feb 6, 2024

Final screenshots:
image
image

resolves #2202
core related PR: lablup/backend.ai#1892

Checklist for reviewers.

  • Only delete-pending status folders are controllable.
    image
  • Other folders whose status is one of the DEAD_VFOLDER_STATUSES except for delete-pending are disabled. But you can check the info.
    image
  • Vfolders that belong to the DEAD_VFOLDER_STATUSES are listed only in the trash bin tab.
  • If you delete the folder, it will be moved to the trash bin tab. And the status is delete-pending.
  • If you click the restore button, the folder status will be changed to ready.
  • If you click the delete_forever button, the folder status will be changed to delete-ongoing -> delete-complete / delete-error.
  • If the folder status is delete-complete, you cannot see the folder info in UI. (you can check with DB).
  • Can you create a new folder with the same name as the deleted forever folder?

How to test

move this line to line number 677.

this._features['vfolder-trash-bin'] = true;

Checklist: (if applicable)

  • Mention to the original issue
  • Documentation
  • Minimum required manager version: 24.03.0*
  • Specific setting for review (eg., KB link, endpoint or how to setup)
  • Minimum requirements to check during review
  • Test case(s) to demonstrate the difference of before/after

@agatha197 agatha197 self-assigned this Feb 6, 2024
@agatha197 agatha197 added type:enhance Add new features effort:normal Need to understand a few modules / some extent of contextual or historical information. area:ux UI / UX issue. urgency:4 As soon as feasible, implementation is essential. platform:general impact:visible This change is visible to users. status:ongoing Currently working on it. labels Feb 6, 2024
@agatha197 agatha197 added this to the 24.03 milestone Feb 6, 2024
Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-2204.d3g9cs6u59b8lw.amplifyapp.com

@github-actions github-actions bot added the urgency:3 Must be finished within a certain time frame. label Feb 6, 2024
@agatha197 agatha197 marked this pull request as ready for review February 7, 2024 04:46
@agatha197 agatha197 removed the urgency:4 As soon as feasible, implementation is essential. label Feb 7, 2024
@yomybaby
Copy link
Member

yomybaby commented Feb 8, 2024

I'll review this PR after merging lablup/backend.ai#1892 @agatha197

@github-actions github-actions bot added size:XL 500~ LoC and removed size:L 100~500 LoC labels Feb 15, 2024
@agatha197
Copy link
Contributor Author

image image
Since supporting the trash bin, the 'delete from trash bin' action needs to be confirmed by the user.

Thank you for your feedback! I modified as below :)
image

@yomybaby
Copy link
Member

@agatha197 Please resolve conflicts.

@agatha197
Copy link
Contributor Author

kimsujin (@agatha197) Please resolve conflicts.

Done!

Copy link

github-actions bot commented Feb 28, 2024

Coverage report for ./react

St.
Category Percentage Covered / Total
🔴 Statements
3.13% (-0% 🔻)
117/3738
🔴 Branches
3.56% (-0.01% 🔻)
87/2447
🔴 Functions
1.53% (-0% 🔻)
19/1240
🔴 Lines
3.08% (-0% 🔻)
113/3665

Test suite run success

32 tests passing in 4 suites.

Report generated by 🧪jest coverage report action from 98bca3b

Copy link
Member

@yomybaby yomybaby left a comment

Choose a reason for hiding this comment

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

If manager supports vfolder-trash-bin:

  • Tooltip: "Delete" to "Move to trash bin"
    image

  • The confirmation modal below is not needed. However, the notification message should describe "휴지통으로 이동되었습니다."
    image
    image

Copy link
Member

@yomybaby yomybaby left a comment

Choose a reason for hiding this comment

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

  • How about changing the trash bin icon color to the normal color since "move to trash bin" is not a dangerous action?
image

- fix: remove duplicated code (`this._triggerFolderListChanged();` is called twice because it is included in `this._refreshFolderList()`)
@yomybaby
Copy link
Member

yomybaby commented Apr 1, 2024

@agatha197 Thank you for the update. It's better than before. Could you improve the messages to include the folder name?

  • "휴지통으로 이동되었습니다." to "OOO 폴더가 휴지통으로 이동되었습니다."
  • "폴더가 복원되었습니다." to " OOO 폴더가 복원되었습니다."
  • "폴더가 완전히 삭제되었습니다." to "OOO 폴더가 완전히 삭제되었습니다."

Copy link
Member

@yomybaby yomybaby left a comment

Choose a reason for hiding this comment

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

LGTM

@agatha197 agatha197 merged commit 4f2c725 into main Apr 1, 2024
8 checks passed
@agatha197 agatha197 deleted the feat/vfolder-trash-bin branch April 1, 2024 08:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area:ux UI / UX issue. effort:normal Need to understand a few modules / some extent of contextual or historical information. field:localization field:UI / UX impact:visible This change is visible to users. platform:general size:XL 500~ LoC status:ongoing Currently working on it. type:enhance Add new features urgency:3 Must be finished within a certain time frame.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Let's introduce trash bin to the vfolder list page.
3 participants