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 deleting datasets on disk #4696

Merged
merged 14 commits into from
Jul 27, 2020
Merged

Allow deleting datasets on disk #4696

merged 14 commits into from
Jul 27, 2020

Conversation

fm3
Copy link
Member

@fm3 fm3 commented Jul 6, 2020

Adds new datastore route DELETE /datasets/:organizationName/:dataSetName/deleteOnDisk (use with care)
Adds a tab to dataset edit view, where this route can be triggered

URL of deployed dev instance (used for testing):

  • https://___.webknossos.xyz

Steps to test:

  • backend: test route DELETE /datasets/:organizationName/:dataSetName/deleteOnDisk (with admin token)
  • frontend: check out new view, hit the button, dataset should be moved to .trash directory within organization datasets directory. Should be one after refresh

Open Questions:

  • how to hide this for non-admins? (how to pass user to view as prop?)
  • how to trigger dataset list refresh on dashboard after redirect

Issues:


@fm3 fm3 added the backend label Jul 6, 2020
@fm3 fm3 self-assigned this Jul 6, 2020
@fm3 fm3 marked this pull request as draft July 8, 2020 09:32
@fm3 fm3 changed the title [WIP] Allow deleting datasets on disk Allow deleting datasets on disk Jul 9, 2020
@fm3 fm3 marked this pull request as ready for review July 9, 2020 11:22
Copy link
Contributor

@MichaelBuessemeyer MichaelBuessemeyer left a comment

Choose a reason for hiding this comment

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

@fm3 Thanks a lot for also doing the frontend part 🚀 -> I did not check the backend part :)

I found only a few minor things to improve and while trying out my suggestions I just pushed them. I hope this is fine 🙈:

  • We generally spell "dataset" in lowercase without capitalizing the s. Could you please correct this to have a consistent spelling?

And I noticed one additional thing:
When a dataset is deleted, it is still listed on the dashboard, which shouldn't be the case. Reloading wk does only solve this issue after some time. Maybe the backend is still deleting the dataset or the dataset is still cached or something like that but it is only removed from the dataset list after some time. :/
@fm3 could you please check whether this is due to the backend?

@fm3
Copy link
Member Author

fm3 commented Jul 12, 2020

Thanks a lot for looking at this and even implementing the suggestions! 🎉
Yes, the backend will only notice the now-missing dataset upon the next inbox check (runs once per minute).
The Refresh button above the dataset list would also cause an inbox check, leading to the list being refreshed. Maybe we can trigger this action also during the deletion before redirecting.
But even then, the front-end shows a cached version of the dataset list until the next refresh, compare #4640
Do you have any ideas how we could solve this for the deletion?

@fm3 fm3 requested a review from youri-k July 13, 2020 08:17
@daniel-wer
Copy link
Member

@fm3 Yes, it should be possible to trigger a refresh before redirecting and there should also be a way to show the newest set of datasets, because that's what happens when the "Refresh" button is clicked by the user. I'll have a look at the code :)

Copy link
Contributor

@youri-k youri-k left a comment

Choose a reason for hiding this comment

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

Tested and works :)
Backend Code LGTM

@daniel-wer
Copy link
Member

@fm3 The dataset list should show updated results after deleting a dataset now :)

Regarding the admin-only ToDo - Who should actually have access to the Import/Edit view at all? At a quick glance I didn't see any restrictions (apart from that the user needs access to the dataset).

@fm3
Copy link
Member Author

fm3 commented Jul 16, 2020

dataset managers should see the links pointing there for all datasets; team managers should see them for the datasets their teams have access to, admins should see them for all datasets. I think this is already the case.
only admins should be allowed to delete

@MichaelBuessemeyer
Copy link
Contributor

MichaelBuessemeyer commented Jul 16, 2020

ToDos for me:

dataset managers should see the links pointing there for all datasets; team managers should see them for the datasets their teams have access to, admins should see them for all datasets. I think this is already the case.

  • check this

only admins should be allowed to delete

  • implement this

@MichaelBuessemeyer
Copy link
Contributor

@fm3 The dataset list should show updated results after deleting a dataset now :)

works perfectly 👌

@MichaelBuessemeyer
Copy link
Contributor

@daniel-wer Could you please check my latest frontend changes?

After this, the PR should be ready to go 🚢

@MichaelBuessemeyer
Copy link
Contributor

I just had a look at the description.

@fm3 is updating wk-connect still a thing to do? I'll have a look at the docs

@daniel-wer
Copy link
Member

Frontend changes look good, thanks for taking this over 👍

@fm3
Copy link
Member Author

fm3 commented Jul 16, 2020

I added it to wk-connect’s issue tracker at scalableminds/webknossos-connect#22
I don’t think it has to be implemented there right away.

@fm3 fm3 merged commit 322b79e into master Jul 27, 2020
@fm3 fm3 deleted the delete-datasets branch July 27, 2020 11:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

enable deleting datasets from UI
4 participants