Refactor and refine cascade deletion rules #2806

melton-jason · 2023-01-12T22:05:01Z

Fixes #1694, #2784, #1069
Additionally test #2511 to see if the error still occurs. If so, we can easily add AddressofRecord to Borrow deletion blockers.

In addition to refactoring the code, the following rules were added:

While deleting any of the following:

Picklist should delete Picklist Item
Spquery should delete Spqueryfield
Spauditlog should delete Spauditlogfield
Specifyuser should delete Spprincipal

In addition, we now have the ability to extend deletion blockers as we see fit on the backend.
Added:

Specifyuser added to the Agent delete blockers (You should not be able to delete an Agent when associated with Specifyuser)

Fixes #1694, #2784, #1069

This was originally in commit 56cb36f, but is being moved here as this is more fitting for the functionality and easier to find in the future

For more information, view the commit in #2806 here: c25ab91

maxpatiiuk · 2023-01-13T00:34:37Z

specifyweb/specify/build_models.py

@@ -112,11 +96,13 @@ def make_relationship(modelname, rel, datamodel):
        return None

    try:
-        on_delete = SPECIAL_DELETION_RULES["%s.%s" % (modelname.capitalize(), rel.name.lower())]
+        on_delete = SPECIAL_DELETION_RULES[f"{rel.name.capitalize()}"][f"{modelname.lower()}"]


It looks like you swapped the relationship and model name. Is this intentional?

I think I should change the variable names...

This was confusing even in production.

Take this for example.

specify7/specifyweb/specify/build_models.py

Line 77 in 741a5e3

'Agent.specifyuser': models.SET_NULL,

This operates as "When a Specifyuser is deleted, set its relationship with Agent to null"
Where the on_delete functionality for these rules get defined, the modelname would be Agent, and rel.name would be specifyuser.

specify7/specifyweb/specify/build_models.py

Line 115 in 741a5e3

on_delete = SPECIAL_DELETION_RULES["%s.%s" % (modelname.capitalize(), rel.name.lower())]

I thought it might make more sense to have the table being deleted be the 'primary' key of the dictonary, so I changed it to 'Specifyuser' : {'agent': models.SET_NULL}

As a result of this flip, I flipped the variables where on_delete was being defined to ensure the same functionality.

I can change it to 'Agent': {'specifyuser': models.SET_NULL} if that makes more sense. It just didn't read as I would expect it to.
Even now, I'm second guessing my changes because of how everything is worded (even though I have tested it many times)

I think adding an example breakdown you provided here as a comment on top of SPECIAL_DELETION_RULES (in addition to what you already have there) would be great

specifyweb/specify/deletion_rules.py

grantfitzsimmons · 2023-01-13T18:26:38Z

Add a block for

spdataset

melton-jason · 2023-01-13T18:51:20Z

Add a block for

spdataset

For more context, this is regarding adding a deletion blocker to SpecifyUser.
This operation is not currently supported with how I extended deletion blockers, but I will add a way to support adding reverse relationships so this can be added.

melton-jason · 2023-01-16T17:29:53Z

Add a block for

spdataset

For more context, this is regarding adding a deletion blocker to SpecifyUser. This operation is not currently supported with how I extended deletion blockers, but I will add a way to support adding reverse relationships so this can be added.

Spatasets will more complicated than any other table(s) to work with on the backend. Many of Specify's internal helper functions rely on the datamodel being loaded when Specify starts up (this happens in load_datamodel.py).
The issue is that to get the list of tables and fields in the database, Specify 7 reads the specify_datamodel.xml in the Specify 6 config directory (as seen below), and as far as I know the spdataset table is not included within that file.

specify7/specifyweb/specify/load_datamodel.py

Lines 222 to 231 in 4877f75

    
           def load_datamodel() -> Datamodel: 
        
               datamodeldef = ElementTree.parse(os.path.join(settings.SPECIFY_CONFIG_DIR, 'specify_datamodel.xml')) 
        
               datamodel = Datamodel() 
        
               datamodel.tables = [make_table(tabledef) for tabledef in datamodeldef.findall('table')] 
        
               add_collectingevents_to_locality(datamodel) 
        
               flag_dependent_fields(datamodel) 
        
               flag_system_tables(datamodel) 
        
               return datamodel

We could manually use SQL to get the correct information, but that will only be a short-term solution (which may be fine for now). Preferably, we would want to engineer a way to load/register these Specify 7 specific tables in the internal specify datamodel. (If such a way already exists and I am overlooking it, then feel free to point it out. I have checked all attributes on specify.models and spdataset does not exist)

melton-jason · 2023-01-17T16:05:17Z

Additionally requested by @grantfitzsimmons:
Add a Report/Label deletion blocker to Specifyuser

melton-jason · 2023-01-19T19:53:44Z

I have went through and cleaned up all of the deletion rules as they stand, so this is ready for testing.

Adding reverse-relationship deletion blockers (i.e. the relationship we want to protect does not exist on the table we want to delete) will require a little more engineering and time: there is no reason to further delay the changes that have already been made on this branch.

These deletion rules can probably be addressed/changed when #2847 and #2813 are being developed (once those two issues are being developed, #2584 can be fixed as well), but this branch should probably be merged before work on those issues begin.

Also, along with testing #2511, can issues #1385, #1431, #88, and #150 additionally be tested to see if they still occur? If so, the fixes for those can be added in this PR.

Additionally remove lines from agent_rules.py which were used for testing, and should not have gotten pushed to origin!

grantfitzsimmons · 2023-01-20T16:25:02Z

While deleting any of the following:

Picklist should delete Picklist Item

After deletion:

Looks good! ✅

Spquery should delete Spqueryfield

Deleted correctly! ✅

Spauditlog should delete Spauditlogfield

https://ptrm11923-issue-1694.test.specifysystems.org/specify/view/spauditlog/2165/

Trying to delete a spauditlog is still blocked by the spauditlogfield. ⚠️

Specifyuser should delete Spprincipal

✅

In addition, we now have the ability to extend deletion blockers as we see fit on the backend. Added:
* Specifyuser added to the Agent delete blockers (You should not be able to delete an Agent when associated with Specifyuser)

✅

grantfitzsimmons · 2023-01-20T16:28:56Z

It looks like #1385 is not solved

https://fwri2023-issue-1694.test.specifysystems.org/specify/query/48/

#2806 does not seem to help this

grantfitzsimmons · 2023-01-20T16:32:31Z

Does not solve #1431 either

Still able to recreate

https://fwri2023-issue-1694.test.specifysystems.org/specify/view/collectionobject/367315/

grantfitzsimmons · 2023-01-20T16:37:31Z

I couldn't recreate #88 (likely because I do not understand the issue) and #150 (seems to be invalid/already solved).

grantfitzsimmons · 2023-01-20T16:41:25Z

#2511 is not solved either

https://herbrbge6802-issue-1694.test.specifysystems.org/specify/view/borrow/1365/

grantfitzsimmons

Please let me know if everything I discovered is intentional. I can formally aprove once I hear from you again

Turns out the models contain the attribute name as plural, to signify this is a to-many relationship. For deletion rules, the table name is needed and NOT the name of the attribute on the base_table model

Fixes #2511, #1431

melton-jason · 2023-01-29T05:35:04Z

These remaining issues should be fixed now.

Spauditlogfield should no longer block the deletion of Spauditlog
Error message when trying to delete a preparation linked to an interaction #1431
- This will require some discussion. How should we handle LoanPreparation when it's associated Preparation is deleted? For comparison purposes to other branches I have set the relationship to null between Preparation and LoanPreparation. (The data seems to be preserved within the LoanPreparation, but it is 'orphaned' )
- On other branches, LoanPreparations are a deletion blocker for Preparations, but the 'Trash' icon can still be clicked from the CollectionObject form and the user can 'attempt' to remove the Preparation. If we want to preserve the deletion blocker, we would need an implementation in the front end

Some concerns:

ProtectedError at /api/specify/borrow/1365/ when deleting borrow #2511

Correctly solving this issue would be impossible with the current way the backend sets up its deletion rules (There is no Addressofrecord.borrows relationship). The code can be refactored to support handling cases like these, but I am not sure how bugprone the solution may be if not hardcoded.

There is another concerning side-effect with this issue (currently in production). When Borrow is deleted, it tries to cascade and delete its associated Addressofrecord. If there are multiple records referencing the Addressofrecord, the error is thrown. However, if the Borrow is the only record referencing the Addressofrecord, both the Addressofrecord and Borrow will be deleted.
This actually holds true for all of the following relationships:

Accession.addressofrecord
Borrow.addressofrecord
Exchangein.addressofrecord
Exchangeout.addressofrecord
Gift.addressofrecord
Loan.addressofrecord
Repositoryagreement.addressofrecord

Interactions are flagged as dependents of Addressofrecord (i.e., they require an Addressofrecord), but at a database level they do not require an Addressofrecord (similar to #2299).
@maxpatiiuk or @grantfitzsimmons do you know why there is this discrepancy?

IntegrityError when trying to delete a user #2784

Spprincipal and Specifyuser have a many-to-many relationship: a relationship type which is not created/supported on the backend. We would either need a proper implementation to handle creating Django many-to-many relationships (the on delete behavior would have to be handled differently than other fields) or use SQL directly to handle deletion rules on a case-by-case basis.

Modifying a schema mapping results in an error #1385

To maintain compatibility with Specify 6, I will not touch the schema mapping fields in this pull request. This can be addressed again when the frontend gets a 'proper' implementation of schema mappings.

grantfitzsimmons · 2023-02-14T03:07:02Z

@melton-jason I need to take a look at this again.

Interactions are flagged as dependents of Addressofrecord (i.e., they require an Addressofrecord), but at a database level they do not require an Addressofrecord (similar to #2299).
@maxpatiiuk or @grantfitzsimmons do you know why there is this discrepancy?

I would remove that requirement– that's bizarre. What do you think @maxpatiiuk?

maxpatiiuk · 2023-02-14T04:14:02Z

I believe address of record is marked as dependent so that it can be displayed as a sub view on the form (otherwise it would only be available as a query combo box and all existing forms would have to be redone)

maxpatiiuk · 2023-02-14T04:14:50Z

Fixing #114 would allow for editing independent resources in sub-views

grantfitzsimmons · 2023-05-26T20:27:53Z

@melton-jason Is this something that just needs to be tested?

melton-jason · 2023-05-26T20:55:41Z

Yes, testing can probably be done on this. There are still 2 cases I need further clarification/discussion on.

There is another concerning side-effect with this issue (currently in production). When Borrow is deleted, it tries to cascade and delete its associated Addressofrecord. If there are multiple records referencing the Addressofrecord, the error is thrown. However, if the Borrow is the only record referencing the Addressofrecord, both the Addressofrecord and Borrow will be deleted.
This actually holds true for all of the following relationships:

Accession.addressofrecord
Borrow.addressofrecord
Exchangein.addressofrecord
Exchangeout.addressofrecord
Gift.addressofrecord
Loan.addressofrecord
Repositoryagreement.addressofrecord

Interactions are flagged as dependents of Addressofrecord ..., but at a database level they do not require an Addressofrecord (similar to #2299).
@maxpatiiuk or @grantfitzsimmons do you know why there is this discrepancy?

It has been awhile, so I would need to look into this specific issue again. Should deleting the interaction try and delete its address of record? I would immediately say no, because the address of record can be referenced by other interactions (or tables), hence the issue that caused #2511.

(#1431)

This will require some discussion. How should we handle LoanPreparation when it's associated Preparation is deleted? For comparison purposes to other branches I have set the relationship to null between Preparation and LoanPreparation. (The data seems to be preserved within the LoanPreparation, but it is 'orphaned' )

On other branches, LoanPreparations are a deletion blocker for Preparations, but the 'Trash' icon can still be clicked from the CollectionObject form and the user can 'attempt' to remove the Preparation. If we want to preserve the deletion blocker, we would need an implementation in the front end

We need to identify what really needs to be done and what the issue is here. I would lean more towards this being a frontend issue with -to-many relationships. The issue is caused because a Preparation is removed from the Collection Object form, but that Preparation references a Loan Preparation, which causes the error to be thrown.

@grantfitzsimmons For that view, I envision making any -to-many relationship like this display the warning symbol next to the trash can, which displays that resource's deletion blockers when clicked and prevents the deletion
We can probably do a similar approach to the minus button in a subview representation of the resource.
Would this be an acceptable solution?

carlosmbe · 2024-01-11T17:52:37Z

I'm going to request both UX and Dev Testing on this PR to conform with our new policy on needing 3 Reviews, Consisting of both Devs and UX people.

Triggered by 7d3ccca on branch refs/heads/issue-1694

melton-jason · 2024-01-27T21:57:42Z

There were changes to the delete blocker dialog and the corresponding backend endpoint in v7.9.0 (specifically in the following commits: dd27101, 80087a2), which resulted in the Additional Delete Blockers not being resolved correctly.
This issue has since been resolved.

To test this fix:

Identify an Agent which is associated with a Specifyuser
- Here is a Query which identifies all Agents in the lsumz mammals database: https://lsumzmammals-issue1694.test.specifysystems.org/specify/query/53/
The SpecifyUser should appear in the Agent's delete blocker dialog

pashiav

#1385 could not reproduce error on edge with https://fwri2023-edge.test.specifysystems.org/specify/query/48/

#1431

Deletes preparation linked to interaction without error!
When I clicked Save for the loan, a new tab opened with this: ReportException at report_runner_run.pdf. Is this related or a separate issue?

1431.mov

#2511 not fixed
Specify 7 Crash Report - 2024-01-29T21_16_00.444Z.txt

https://herbrbge71423-issue1694.test.specifysystems.org/specify/view/borrow/1364/

#2806 (comment) looks good!

melton-jason added 2 commits January 12, 2023 14:00

Refactor and fine tune deletion rules

48763cd

Fixes #1694, #2784, #1069

Add ability to extend deletion blockers

c25ab91

This was originally in commit 56cb36f, but is being moved here as this is more fitting for the functionality and easier to find in the future

melton-jason requested a review from a team January 12, 2023 22:05

melton-jason added a commit that referenced this pull request Jan 12, 2023

Move add deletion blockers to #2806

0387638

For more information, view the commit in #2806 here: c25ab91

maxpatiiuk reviewed Jan 13, 2023

View reviewed changes

specifyweb/specify/deletion_rules.py Outdated Show resolved Hide resolved

maxpatiiuk approved these changes Jan 13, 2023

View reviewed changes

melton-jason mentioned this pull request Jan 13, 2023

Remove additional deletion blocker from import #2807

Merged

Merge branch 'production' into issue-1694

c7374d4

This was referenced Jan 17, 2023

Load Specify 7 specific tables into datamodel #2835

Closed

Front-end has no direct access to sp7-exclusive tables #2813

Closed

Correct field names

789e692

Delete 'names' rather than 'name' in splocalecontaineritem

e054a5d

Additionally remove lines from agent_rules.py which were used for testing, and should not have gotten pushed to origin!

grantfitzsimmons mentioned this pull request Jan 20, 2023

Modifying a schema mapping results in an error #1385

Open

grantfitzsimmons mentioned this pull request Jan 20, 2023

ProtectedError at /api/specify/borrow/1365/ when deleting borrow #2511

Open

grantfitzsimmons reviewed Jan 20, 2023

View reviewed changes

melton-jason added 4 commits January 20, 2023 16:49

Fix to-manys cascade deletion rules

c4371b7

Turns out the models contain the attribute name as plural, to signify this is a to-many relationship. For deletion rules, the table name is needed and NOT the name of the attribute on the base_table model

Add typing to build_models.py

212be10

Reformat special deletion rules

5322caf

Add remaining deletion rules

eb07a3b

Fixes #2511, #1431

grantfitzsimmons self-assigned this Feb 14, 2023

carlosmbe requested review from a team January 11, 2024 17:52

CarolineDenis approved these changes Jan 11, 2024

View reviewed changes

melton-jason added 3 commits January 26, 2024 11:12

Merge remote-tracking branch 'origin/production' into issue-1694

7d3ccca

Lint code with ESLint and Prettier

99432c6

Triggered by 7d3ccca on branch refs/heads/issue-1694

Return all instances of Additional Delete Blockers

94fe4e2

pashiav requested changes Jan 29, 2024

View reviewed changes

Merge branch 'production' into issue-1694

1092471

melton-jason mentioned this pull request May 31, 2024

Delete blocker does not show up initially #2231

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor and refine cascade deletion rules #2806

Refactor and refine cascade deletion rules #2806

melton-jason commented Jan 12, 2023 •

edited

Loading

maxpatiiuk Jan 13, 2023

melton-jason Jan 13, 2023 •

edited

Loading

melton-jason Jan 13, 2023 •

edited

Loading

maxpatiiuk Jan 13, 2023

grantfitzsimmons commented Jan 13, 2023 •

edited

Loading

melton-jason commented Jan 13, 2023

melton-jason commented Jan 16, 2023

melton-jason commented Jan 17, 2023

melton-jason commented Jan 19, 2023 •

edited

Loading

grantfitzsimmons commented Jan 20, 2023 •

edited

Loading

grantfitzsimmons commented Jan 20, 2023

grantfitzsimmons commented Jan 20, 2023

grantfitzsimmons commented Jan 20, 2023

grantfitzsimmons commented Jan 20, 2023

grantfitzsimmons left a comment

melton-jason commented Jan 29, 2023 •

edited by maxpatiiuk

Loading

grantfitzsimmons commented Feb 14, 2023 •

edited by maxpatiiuk

Loading

maxpatiiuk commented Feb 14, 2023

maxpatiiuk commented Feb 14, 2023

grantfitzsimmons commented May 26, 2023

melton-jason commented May 26, 2023 •

edited by maxpatiiuk

Loading

carlosmbe commented Jan 11, 2024

melton-jason commented Jan 27, 2024

pashiav left a comment

Refactor and refine cascade deletion rules #2806

Are you sure you want to change the base?

Refactor and refine cascade deletion rules #2806

Conversation

melton-jason commented Jan 12, 2023 • edited Loading

maxpatiiuk Jan 13, 2023

Choose a reason for hiding this comment

melton-jason Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

melton-jason Jan 13, 2023 • edited Loading

Choose a reason for hiding this comment

maxpatiiuk Jan 13, 2023

Choose a reason for hiding this comment

grantfitzsimmons commented Jan 13, 2023 • edited Loading

melton-jason commented Jan 13, 2023

melton-jason commented Jan 16, 2023

melton-jason commented Jan 17, 2023

melton-jason commented Jan 19, 2023 • edited Loading

grantfitzsimmons commented Jan 20, 2023 • edited Loading

grantfitzsimmons commented Jan 20, 2023

grantfitzsimmons commented Jan 20, 2023

grantfitzsimmons commented Jan 20, 2023

grantfitzsimmons commented Jan 20, 2023

grantfitzsimmons left a comment

Choose a reason for hiding this comment

melton-jason commented Jan 29, 2023 • edited by maxpatiiuk Loading

grantfitzsimmons commented Feb 14, 2023 • edited by maxpatiiuk Loading

maxpatiiuk commented Feb 14, 2023

maxpatiiuk commented Feb 14, 2023

grantfitzsimmons commented May 26, 2023

melton-jason commented May 26, 2023 • edited by maxpatiiuk Loading

carlosmbe commented Jan 11, 2024

melton-jason commented Jan 27, 2024

pashiav left a comment

Choose a reason for hiding this comment

melton-jason commented Jan 12, 2023 •

edited

Loading

melton-jason Jan 13, 2023 •

edited

Loading

melton-jason Jan 13, 2023 •

edited

Loading

grantfitzsimmons commented Jan 13, 2023 •

edited

Loading

melton-jason commented Jan 19, 2023 •

edited

Loading

grantfitzsimmons commented Jan 20, 2023 •

edited

Loading

melton-jason commented Jan 29, 2023 •

edited by maxpatiiuk

Loading

grantfitzsimmons commented Feb 14, 2023 •

edited by maxpatiiuk

Loading

melton-jason commented May 26, 2023 •

edited by maxpatiiuk

Loading