- 
                Notifications
    You must be signed in to change notification settings 
- Fork 41
Add DO_NOTHING for on_delete on TreeDefItems #5024
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing instructions
- Go to any tree
- Add a new rank
- Select any parent node that is not the last one
- Try to delete the rank
- Make sure there is no error, and that the tree rank is no longer there
Works for taxon and geography trees but there's still a deletion blocker when trying to delete a storage tree rank

| The issue was that the specify models.py file was generated after the add_delete_ranks PR was merged. So the  | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing instructions
- Go to any tree
- Add a new rank by pressing the pencil button, then the plus button, select the parent, press continue, fill out fields, press save, wait for the page to reload.
- Select any parent node that is not the last one when adding a new rank.
- Try to delete the rank by pressing the pencil button, then press the pencil button next to your test rank, then press delete, confirm delete, wait for the page to reload.
- Make sure there is no error, and that the tree rank is no longer there
Looks good, can delete tree ranks in all trees!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing instructions
- Go to any tree
- Add a new rank by pressing the pencil button, then the plus button, select the parent, press continue, fill out fields, press save, wait for the page to reload.
- Select any parent node that is not the last one when adding a new rank.
- Try to delete the rank by pressing the pencil button, then press the pencil button next to your test rank, then press delete, confirm delete, wait for the page to reload.
- Make sure there is no error, and that the tree rank is no longer there
Seems to fix it on all trees now 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice!
I checked the other special deletion rules as well and they are now consistent with the Model definitions
specify7/specifyweb/specify/build_models.py
Lines 124 to 146 in bd0d01d
| SPECIAL_DELETION_RULES = { | |
| 'Agent.specifyuser': models.SET_NULL, | |
| 'Recordsetitem.recordset': models.CASCADE, | |
| # Handle workbench deletion using raw sql in business rules. | |
| 'Workbenchrow.workbench': models.DO_NOTHING, | |
| 'Workbenchdataitem.workbenchrow': models.DO_NOTHING, | |
| 'Workbenchrowimage.workbenchrow': models.DO_NOTHING, | |
| 'Workbenchrowexportedrelationship.workbenchrow': models.DO_NOTHING, | |
| 'Spappresourcedir.specifyuser': models.CASCADE, | |
| 'Spappresource.specifyuser': models.CASCADE, | |
| 'Spappresource.spappresourcedir': models.CASCADE, | |
| 'Spappresourcedata.spappresource': models.CASCADE, | |
| 'Spappresourcedata.spviewsetobj': models.CASCADE, | |
| 'Spreport.appresource': models.CASCADE, | |
| 'Geographytreedefitem.parent': models.DO_NOTHING, | |
| 'Geologictimeperiodtreedefitem.parent': models.DO_NOTHING, | |
| 'Lithostrattreedefitem.parent': models.DO_NOTHING, | |
| 'Storagetreedefitem.parent': models.DO_NOTHING, | |
| 'Taxontreedefitem.parent': models.DO_NOTHING, | |
| } | 
Not related to this PR, but I also noticed some unused code from #4257 and thought I'd mention it here so there's more of a paper trail.
specify7/specifyweb/specify/build_models.py
Lines 322 to 328 in bd0d01d
| tables_with_pre_constraints_delete = [ | |
| # 'Geographytreedefitem', | |
| # 'Geologictimeperiodtreedefitem', | |
| # 'Lithostrattreedefitem', | |
| # 'Storagetreedefitem', | |
| # 'Taxontreedefitem', | |
| ] | 
specify7/specifyweb/specify/build_models.py
Lines 97 to 98 in bd0d01d
| if table.django_name in tables_with_pre_constraints_delete: | |
| attrs['pre_constraints_delete'] = pre_constraints_delete | 
Code that functionally does nothing currently could lead other developers (or yourself in the future!) to look into this red herring.
Could this be either removed or commented?
At the very least a comment explaining the envisioned use for this code would go a long way!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
| @melton-jason thanks, I checked that all the special cases are represented in the models.py file. the specify models.py file was generated after the add_delete_ranks PR was merged, so it was the only one not to get their on_delete function set correctly. I added a comment for pre_constraints_delete. | 
Fixes #5021
The issue was that the specify models.py file was generated after the add_deete_ranks PR was merged. So the
on_delete=models.DO_NOTHINGwasn't applied in the re-definition.Old:
Add a filter to delete_blockers, so that blockers that aren't really actually blockers and filtered out. This will fix the delete_blockers issue we are having with deleting tree ranks. Delete a Taxontreedefitem should only be blocked by a Taxon, not another Taxontreedefitem, this dependency is handled in the api call for tree rank deletion. Let me know if someone has a better idea, this just seemed like a simple solution. Is there a case where we would want these blockers not to be filtered?Checklist
and self-explanatory (or properly documented)
Testing instructions
#5021 (comment)