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

Easier folder creation and configurable target folder for dataset upload #6704

Merged
merged 34 commits into from
Jan 9, 2023

Conversation

philippotto
Copy link
Member

@philippotto philippotto commented Dec 19, 2022

  • add "Add Folder" button next to "Add Dataset" button
    image
  • dummy context menu when rightclicking into the blank space of the sidebar:
    image
  • clicking "add dataset" will encode the active folder id in the url so that the import view can use that to initial the new target-folder selection
    image
    image

The target folder functionality is currently front-end only (it simply uses the API to move the DS after the upload). This won't work if the dataset needs to be converted first. Not sure whether this should block the PR. The backend part probably won't happen in this year. Merging this now could be confusing to users when the DS doesn't pop up in the desired folder. However, this can be confusing right now, too

URL of deployed dev instance (used for testing):

Steps to test:

  • use context menu in sidebar on folders and on blank space
  • use new add-folder button
    • use the target folder UI to change the folder (should have initialized to the folder that was active in the dashboard)
    • upload something (zarr or normal)
    • dataset should appear there

Issues:


(Please delete unneeded items, merge only when none are left open)

@philippotto philippotto self-assigned this Dec 19, 2022
Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Nicely done! Worked well during my testing, both for dataset upload and remote zarr import 👍

The backend part probably won't happen in this year. Merging this now could be confusing to users when the DS doesn't pop up in the desired folder. However, this can be confusing right now, too 🤔

Since the frontend gets to know whether the dataset needs to be converted, a simple toast/message could help to avoid confusion. It could state that this is not supported yet and that the dataset needs to be moved after the conversion.

frontend/javascripts/dashboard/dataset_view.tsx Outdated Show resolved Hide resolved
philippotto and others added 2 commits December 21, 2022 16:08
@philippotto
Copy link
Member Author

Since the frontend gets to know whether the dataset needs to be converted, a simple toast/message could help to avoid confusion. It could state that this is not supported yet and that the dataset needs to be moved after the conversion.

Good idea! Instead of showing a prompt, I used the antd form validation which also works nicely, I think.

image

and the normal case (as before)
image

@philippotto philippotto enabled auto-merge (squash) January 3, 2023 09:21
@philippotto philippotto disabled auto-merge January 3, 2023 09:24
@philippotto
Copy link
Member Author

@daniel-wer I updated the front-end to incorporate the newest changes by @fm3 so that the target folder also works with datasets which are converted after upload. The target folder must be specified now and the front-end ensures that the user may upload the DS to the folder by using the disabled property of the antd tree component. The "Allowed Teams", on the other hand, are not required anymore during upload. Also, the "Allowed Teams" warning/hint in the dataset settings were removed because there are not really necessary, anymore (due to the cumulative permission concept, users should be far more likely to have the necessary permissions).

@philippotto philippotto requested a review from daniel-wer January 4, 2023 15:41
Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

Very nice, that datasets that need a conversion can be uploaded to a specific folder now, too.

During testing I noticed

  • When selecting "Add Dataset" from a folder, the target folder is preselected which is nice. However, when trying to upload, webknossos complains that no target folder is selected (manually selecting the same folder fixes the issue). See screenshot
    image
  • The target folder preselection is also performed for folders the user doesn't have edit permissions for and which, therefore, couldn't be selected manually, because their entries are disabled
  • [Optional] When selecting "Go to Dashboard" after a successful dataset upload, it would be cool to auto-activate the target folder so the user immediately sees the uploaded dataset

frontend/javascripts/admin/dataset/dataset_upload_view.tsx Outdated Show resolved Hide resolved
@philippotto
Copy link
Member Author

@daniel-wer Thank you for your testing feedback! I incorporated the first two points. I agree, the third point would be nice, but I decided against it for now due to the priority/cost ratio.

@philippotto philippotto requested a review from daniel-wer January 9, 2023 15:14
Copy link
Member

@daniel-wer daniel-wer left a comment

Choose a reason for hiding this comment

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

LGTM, works as advertised 👍

@philippotto philippotto enabled auto-merge (squash) January 9, 2023 20:06
@philippotto philippotto merged commit ab5d192 into master Jan 9, 2023
@philippotto philippotto deleted the folder-follow-up branch January 9, 2023 20:25
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.

3 participants