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

Add info about dependencies when deleting collection #17961

Merged
merged 9 commits into from Apr 12, 2023
Merged

Conversation

Nitwel
Copy link
Member

@Nitwel Nitwel commented Mar 27, 2023

This PR introduces a warning showing the user in advance why the deletion process might fail.

Fixes #17536
Fixes ENG-701

grafik

@Nitwel Nitwel requested a review from a team March 27, 2023 15:31
@Nitwel Nitwel self-assigned this Mar 27, 2023
@Nitwel Nitwel requested review from paescuj, br41nslug and ConnorSimply and removed request for a team March 27, 2023 15:31
@rijkvanzanten
Copy link
Member

@Nitwel Do we know for a fact that the deletion will fail? Cause if that's the case, I'd rather just say "You can't delete collection this as other items are relying on it. Please delete the other collection first"

@Nitwel
Copy link
Member Author

Nitwel commented Mar 27, 2023

Good point, I went with that wording for now as I had the suspicion that DB vendors like SQLite might not support foreign key constraints.

Copy link
Member

@br41nslug br41nslug left a comment

Choose a reason for hiding this comment

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

It's working as advertised but I find the double popup on postgres a bit counter intuitive 😬 It feels a bit redundant to get a "confirmation" popup where you agree and then it doesnt delete it and you have to go back delete it manually anyway.

firefox_c9TG9fkCsQ.mp4

In sqlite it makes a lot more sense as it doesnt get constraint blocked. However both postgres and sqlite have issues with circular dependencies, that may be a separate issue.

firefox_U4NNA231a2.mp4
firefox_zre1R907X9.mp4

@Nitwel
Copy link
Member Author

Nitwel commented Mar 28, 2023

@rijkvanzanten Tim and I discussed that it might make more sense to not only warn the user that you might not be able to delete something but instead to delete the fields that the collection depends on first and then delete the collection, so you don't have to do that yourself manually. Thoughts?

@rijkvanzanten
Copy link
Member

but instead to delete the fields that the collection depends on first and then delete the collection, so you don't have to do that yourself manually. Thoughts?

Yes 💯

@Zack-Heisnberg
Copy link

@rijkvanzanten Tim and I discussed that it might make more sense to not only warn the user that you might not be able to delete something but instead to delete the fields that the collection depends on first and then delete the collection, so you don't have to do that yourself manually. Thoughts?

it's should have a warning though that the data of that field will be lost

This collection " a " depends on the collection you want to delete " b " ,
do you want to delete the field and all it's data "x" in items in collection "a" to delete the collection "b"

@Nitwel
Copy link
Member Author

Nitwel commented Mar 29, 2023

Updated it to show a danger banner and to delete all fields it depends on first.

Copy link
Member

@paescuj paescuj left a comment

Choose a reason for hiding this comment

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

Works as expected for collections with M2O fields 👍

Not sure about other types of relations, though. For example when deleting a collection included in a M2A relation it now deletes the item field in the junction table - this wasn't the case before:

app/src/lang/translations/en-US.yaml Outdated Show resolved Hide resolved
rijkvanzanten and others added 2 commits April 11, 2023 19:39
@rijkvanzanten
Copy link
Member

@br41nslug @paescuj Y'all happy?!!!!!

Copy link
Member

@rijkvanzanten rijkvanzanten left a comment

Choose a reason for hiding this comment

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

This is one of those PRs that should've been there from the beginning. Thanks @Nitwel!

@rijkvanzanten rijkvanzanten enabled auto-merge (squash) April 11, 2023 23:40
@paescuj
Copy link
Member

paescuj commented Apr 12, 2023

@br41nslug @paescuj Y'all happy?!!!!!

Not yet, as outlined in #17961 (review).
I really think the item field shouldn't get deleted! Not sure if there are other cases like this, we need to test!

@paescuj paescuj disabled auto-merge April 12, 2023 05:26
@Nitwel
Copy link
Member Author

Nitwel commented Apr 12, 2023

@paescuj I updated to only delete the item field in an a2o, when there is only 1 collection left, thus it is fine to delete it. Otherwise it won't be deleted.

Copy link
Member

@br41nslug br41nslug left a comment

Choose a reason for hiding this comment

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

Looks to be working as expected now on sqlite and postgres 😄

@paescuj
Copy link
Member

paescuj commented Apr 12, 2023

@paescuj I updated to only delete the item field in an a2o, when there is only 1 collection left, thus it is fine to delete it. Otherwise it won't be deleted.

Okay, that already makes me much happier 👍 But I still think that there's no need to deleted this field at all. So the M2A field would remain usable in that you can simply connect a new collection (as it is the case now):
Screenshot 2023-04-12 at 17 20 21

@Nitwel
Copy link
Member Author

Nitwel commented Apr 12, 2023

Agreed 👍
Updated it to only delete m2o relations.

@rijkvanzanten rijkvanzanten merged commit d954172 into main Apr 12, 2023
5 checks passed
@rijkvanzanten rijkvanzanten deleted the fix-17536 branch April 12, 2023 17:56
@rijkvanzanten rijkvanzanten added this to the Next Release milestone Apr 13, 2023
hanneskuettner pushed a commit to hanneskuettner/directus that referenced this pull request Apr 18, 2023
* add info about dependencies when deleting collection

* Delete dependent fields

* Update app/src/lang/translations/en-US.yaml

Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>

* add m2a edgecase

* run linter

* run linter

* Update app/src/lang/translations/en-US.yaml

* only delete m2o relations

---------

Co-authored-by: Rijk van Zanten <rijkvanzanten@me.com>
Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>
meditadvisors pushed a commit to ciso360ai/directus-mod that referenced this pull request May 13, 2023
* add info about dependencies when deleting collection

* Delete dependent fields

* Update app/src/lang/translations/en-US.yaml

Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>

* add m2a edgecase

* run linter

* run linter

* Update app/src/lang/translations/en-US.yaml

* only delete m2o relations

---------

Co-authored-by: Rijk van Zanten <rijkvanzanten@me.com>
Co-authored-by: Pascal Jufer <pascal-jufer@bluewin.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

App: Trying to remove a collection with dependencies throws error which is not clear
5 participants