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

feature/toggle-modal #311

Merged
merged 13 commits into from
Nov 4, 2024
Merged

Conversation

BrianWhitneyAI
Copy link
Contributor

@BrianWhitneyAI BrianWhitneyAI commented Oct 25, 2024

Description

We want to present to users the option to move files on and off NAS Cache. In order to do this and convey the significance of the action we want to add the following functionality:

  • Add Move menu item to dropdown with options “Off NAS Cache” & “Onto NAS Cache”
  • Modal component to report effect of move ( total # of file and total size )

Relevant Issues

@BrianWhitneyAI
Copy link
Contributor Author

Move option on file selection.
Screenshot 2024-10-25 at 1 15 37 PM

@BrianWhitneyAI
Copy link
Contributor Author

BrianWhitneyAI commented Oct 25, 2024

Move Modal.

Screenshot 2024-10-25 at 1 02 15 PM

@BrianWhitneyAI
Copy link
Contributor Author

What still needs to be done.

  1. We need some semblance of what files are already in local or in cloud so we can convey more accurately how many files and the total file size is moving.

  2. We need the actual dispatch. right now confirming jus logs a message :)

@BrianWhitneyAI BrianWhitneyAI marked this pull request as ready for review October 25, 2024 20:20
@BrianWhitneyAI BrianWhitneyAI requested review from SeanLeRoy and aswallace and removed request for SeanLeRoy October 25, 2024 20:20
@BrianWhitneyAI BrianWhitneyAI self-assigned this Oct 25, 2024
@saeliddp
Copy link
Collaborator

General comment:

After a few rounds of discussion, we decided that we should not give BFF users the ability to move files off of the cache. Instead, once a file is moved onto the cache, it is given an expiration date (e.g. 6 months out). If a user later sends the request to cache the file again (while it's still in the cache), it will act as a 'refresh' on the expiration date.

Quick summary of why we made this decision:

  • Suppose user A decides to remove example.tif from the cache. How would user A know if another user is still using example.tif and still wants it on the cache? -> leads to unnecessary, costly (time and S3 $) 'cache battles'
  • We would need to prevent concurrency issues with 'remove from cache' and 'move to cache' actions
  • We do not expect space on the cache to be an issue once we make the transition to using cloud as primary storage, so there's not a lot of risk involved with setting a pretty distant expiration date
  • In the future (and if users want it), we could have expiration_date as an argument to the cache endpoint, giving users the option to say how long they need the file in local cache

What does this mean for BFF? (and this PR)

  • No need for Off NAS cache option
  • Eventually, probably want to surface expiration date if the file is already in the cache, give users the option to refresh that expiration date

packages/core/components/Modal/MoveFileManifest/index.tsx Outdated Show resolved Hide resolved
* Modal overlay for displaying details of selected files for NAS cache operations.
*/
export default function MoveFileManifest({ onDismiss }: ModalProps) {
// const dispatch = useDispatch(); //TODO: add onMove functionality
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this PR should/can include the action that will eventually contain the call to FSS. What I mean is that you can dispatch a "moveFIles" action that you can create in the interaction state branch & create an empty "logics" in the logic middleware file that will just log to the console rather than call FSS for now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved in e159438 and 8c810a6

/**
* Formats a file size to a human-readable string.
*/
const formatFileSize = (size: number) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

This should go after the imports, but I think we have a function for doing this sort of thing already. Look in the aggregate file box, that display already renders the appropriate byte unit.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved in ae06c75

<div className={styles.fileListContainer}>
<ul className={styles.fileList}>
{fileDetails.map((file) => (
<li key={file.id} className={styles.fileItem}>
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a table with the columns name & size would be better than this list for readability. Like so:

| File Name              | File Size  |
| my_file.txt             | 145 MB    |
| another_one.png | 206MB     |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved in 77a52bd

@@ -157,22 +157,26 @@ export default (filters?: FileFilter[], onDismiss?: () => void) => {
},
subMenuProps: {
items: [
{
Copy link
Contributor

Choose a reason for hiding this comment

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

The "move" option that contains these should only be rendered when the data source is "AICS FMS" (there is a constant for that value)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved in f52d31c

}),
[HIDE_VISIBLE_MODAL]: (state) => ({
...state,
visibleModal: undefined,
Copy link
Contributor

Choose a reason for hiding this comment

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

Confused about this. Was the visible modal otherwise never being set to undefined? Did you create this action in another PR?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops accidentally duplicated our existing. Resolved in 1e3c38b

@BrianWhitneyAI
Copy link
Contributor Author

New Screenshots
Screenshot 2024-10-28 at 1 47 44 PM
Screenshot 2024-10-28 at 1 48 02 PM

@BrianWhitneyAI
Copy link
Contributor Author

New menu with only ONTO NAS
Screenshot 2024-10-28 at 2 20 06 PM

Copy link
Collaborator

@saeliddp saeliddp left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Left a few comments related to users only being allowed to move files onto the cache

@BrianWhitneyAI
Copy link
Contributor Author

Heres what the menu looks like now
Screenshot 2024-10-29 at 12 09 22 PM

packages/core/components/Modal/MoveFileManifest/index.tsx Outdated Show resolved Hide resolved
],
},
},
...(isQueryingAicsFms
Copy link
Contributor

Choose a reason for hiding this comment

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

Lets move this option above "Download"

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved in 034549d

selection.selectors.getFileSelection,
FileSelection.selectionsAreEqual
);
const moveFileTarget = useSelector(interaction.selectors.getMoveFileTarget);
Copy link
Contributor

Choose a reason for hiding this comment

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

Now that we don't have a notion of moving on/off the cache it seems we can remove the idea of having a target. IMO this is likely a YAGNI since other than FMS we won't have control over moving files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

resolved in 034549d

@SeanLeRoy
Copy link
Contributor

The list of files should have a gradient at the bottom of it to visually signal to users that there are more options

@BrianWhitneyAI
Copy link
Contributor Author

The list of files should have a gradient at the bottom of it to visually signal to users that there are more options

resolved in a450c41
Screenshot 2024-10-31 at 4 58 48 PM

@BrianWhitneyAI BrianWhitneyAI merged commit dbb368e into feature/local_cloud_toggle Nov 4, 2024
3 checks passed
@BrianWhitneyAI BrianWhitneyAI deleted the feature/toggle-modal branch November 4, 2024 22:26
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.

5 participants