8000 Delete library button added to library settings by matthewbadeau · Pull Request #3627 · manyfold3d/manyfold · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Delete library button added to library settings #3627

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.

Sign up for GitHub

By clicking &ld 8000 quo;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

Jump to bottom
Merged
merged 7 commits into from
Mar 18, 2025
Merged

Delete library button added to library settings #3627

merged 7 commits into from
Mar 18, 2025

Conversation

matthewbadeau
Copy link
Collaborator

Checklist

🚨 Please review the guidelines for contributing to this repository. 🚨

  • Make sure you are making a pull request against our main branch (left side)
  • Check that that your branch is up to date with our main.
  • Make sure you are requesting to pull a topic/feature/bugfix branch (right side). Don't request your main!
  • Check that the tests and code linter both pass.
  • If you're a new contributor, please sign our contributor license agreement.

Warnings

  • This PR will change existing database contents.
  • This PR introduces a breaking change to existing installations.

Summary

This adds a delete button to the library edit page.

Linked issues

This should resolve #3595

Description of changes

This change adds a button to the library edit page.

It uses if policy(@library).destroy? similarly to how users can be deleted.

@matthewbadeau matthewbadeau requested a review from Floppy February 20, 2025 13:08
Floppy
Floppy requested changes Feb 20, 2025
View reviewed changes
Copy link
Collaborator
@Floppy Floppy left a comment

Choose a reason for hiding this comment

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

Thanks! This a great start.

I've been thinking about this though, and we might need something more; I think this will delete models inside the library as well, it'll cascade. I think that's probably not what we want?

There are two possibilities for solving that:

  1. Only allow empty libraries to be deleted. This probably fixes most use cases for this where someone's put in the wrong path.
  2. Allow the models to be deleted along with the library, but prevent any filesystem-level changes. I think we'd want some tests to make sure that works.

What do you reckon?

@matthewbadeau
Copy link
Collaborator Author

Thank you for the feedback! Yes, you're right, we definitely need something more.

I think this will delete models inside the library as well, it'll cascade.

Yes, you're right. We don't want to remove the models inside the library and instead want to remove the models without a delete file operation. I think I'll need to make changes to the Model controller too to not delete the files if the library has been marked for deletion.
So yes, I think both approaches are the way to go. I will insta-delete empty libraries. Plus mark models for deletion but without filesystem changes.

Give me a few more days and I'll throw something together.

I also need to write tests -- which I'm not very good at but I'll try.

@matthewbadeau matthewbadeau marked this pull request as draft February 27, 2025 12:58
@Floppy Floppy added the feature User-facing features and product enhancements label Mar 2, 2025
@Floppy
Copy link
Collaborator
Floppy commented Mar 18, 2025

Update on this: #3786 and #3789 make improvements to file removal so that destroying database records never removes files, unless delete_from_disk_and_destroy (on Model or ModelFile) is called, which only eveer happens with explicit user input. So, calling Library#destroy is now safe, and won't remove any actual files. I still think it's worth having a confirmation dialog of "this library contains 27 models, all data will be lost, do you want to go ahead" or something, but we can allow removal of non-empty libraries.

I'm going to pick up this PR now and finish it off, if you don't mind @matthewbadeau!

@Floppy Floppy force-pushed the feature/delete_library_button branch from 1ba4330 to fef08b8 Compare March 18, 2025 12:00
@Floppy Floppy force-pushed the feature/delete_library_button branch from fef08b8 to 0dff08c Compare March 18, 2025 13:08
9F25 @Floppy Floppy marked this pull request as ready for review March 18, 2025 14:45
@Floppy Floppy merged commit a5274e0 into main Mar 18, 2025
11 checks passed
@Floppy Floppy deleted the feature/delete_library_button branch March 18, 2025 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Floppy Floppy Floppy approved these changes

Assignees
No one assigned
Labels
feature User-facing features and product enhancements
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

No way to delete libraries
2 participants
@matthewbadeau @Floppy
0