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

Draft: add new relations behavior on deletion of related objects (#343) #344

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

sennierer
Copy link
Collaborator

created a simple test that asserts that relations are deleted when entities are deleted. I think thats a foreseeable outcome (especially as the merge systematic takes care of copying relations).
However, relations are also deleted if the relation_type is deleted. And while it makes sense that a relation shouldn't exist without a relation_type I think we shouldn't delete relations when the relation type is deleted. Researchers clean up there vocabs in the admin interface and while they are warned that relations will be deleted I think we should nonetheless set the relation_type to a configurable default and keep the relation. Thus the second test is currently failing.
The behavior I suggest is:

  • relations are deleted if target or source are deleted (current behaviour, tested with the new test)
  • relations are rewritten to the new entity if source or target gets merged (current behavior, test needs to be extended to cover that)
  • relation_types are rewritten to a default relation type if the original gets deleted (covered by the test, but current behavior is the deletion of the relation)

@sennierer
Copy link
Collaborator Author

Can we have opinions on that? @gregorpirgie @steffres

@skurzinz
Copy link
Contributor

skurzinz commented Jul 21, 2022 via email

@gregorpirgie
Copy link
Contributor

I think @sennierer that the solution is very good. But I would also consider:

  • to set the default value to a value that has the same literal name across all relations (i.e. PersonPerson, PersonEvent, etc.)
  • something like ("no relationtype specified")
  • I mean we need one default type per RelationClass anyway, but it would be easier to handle them if all could be accessed by the same name.

And I think that it would be important than that editors never change that name; or, even better, to implement a save mechanism that disallows changing that name – so that relations with the default relationtype can only be assigned a new relationtype, but not changed by editing the default value itself. Which might happen if editors see that at a current state of a project all relations with the default value fall into a certain category and think of just renaming the relationtype.

@sennierer
Copy link
Collaborator Author

@gregorpirgie I think you are right, we should aim for one string across all relationtypes. However, I think it could be configurable. Something along the lines of getattr(settings, "APIS_DEFAULT_RELATION_TYPE", "no relationtype specified") and I wouldnt care about users changing the default relation type. If the system doesnt find a relation_type with the set lable it just creates one. Gives the users the opportunity to change in batch if they made a mistake by deleting a relation_type and relieves us from implementing a probably rather complex logic to prevent the change.

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