diff --git a/shopdb/cli/__init__.py b/shopdb/cli/__init__.py index 01b652e..8885eb0 100644 --- a/shopdb/cli/__init__.py +++ b/shopdb/cli/__init__.py @@ -738,6 +738,13 @@ def fix_controls_direction(): relationshiptypeid=controls.relationshiptypeid, ).first() if duplicate: + # The correctly-directed row exists - but it may itself be soft + # deleted, and deletion here IS soft. Retiring this row without + # looking left the pair with NO live link, while the command + # reported a successful clean-up. Reactivate the one pointing the + # right way before retiring the one pointing the wrong way. + if not duplicate.isactive: + duplicate.isactive = True row.isactive = False # flipped row already exists, retire this one deactivated += 1 else: diff --git a/shopdb/core/api/assets.py b/shopdb/core/api/assets.py index 9e07966..c958e9e 100644 --- a/shopdb/core/api/assets.py +++ b/shopdb/core/api/assets.py @@ -768,14 +768,23 @@ def create_asset_relationship(): 'An asset cannot be related to itself' ) - # Check for duplicate relationship + # Check for duplicate relationship. + # + # Deleting a relationship is SOFT (isactive = False, see + # delete_asset_relationship). The row therefore survives, and this lookup + # found it - so deleting a link and adding it back answered 409 "this + # relationship already exists" about a link the page no longer shows, with + # no way forward from the UI at all. The row cannot simply be inserted + # again either: assetrelationships is unique on (source, target, type). + # + # Reactivating IS the create, for an inactive row. existing = AssetRelationship.query.filter_by( sourceassetid=source_id, targetassetid=target_id, relationshiptypeid=type_id ).first() - if existing: + if existing and existing.isactive: return error_response( ErrorCodes.CONFLICT, 'This relationship already exists', @@ -798,7 +807,11 @@ def create_asset_relationship(): targetassetid=source_id, relationshiptypeid=type_id ).first() - if inverse is not None: + # isactive, for the same reason as the duplicate check above: a soft + # deleted inverse is a link somebody already removed, and blocking on it + # made "remove the existing one first" - the instruction in this very + # message - fail to unblock anything. + if inverse is not None and inverse.isactive: return error_response( ErrorCodes.CONFLICT, f"The reverse of this relationship already exists " @@ -808,14 +821,20 @@ def create_asset_relationship(): http_code=409 ) - rel = AssetRelationship( - sourceassetid=source_id, - targetassetid=target_id, - relationshiptypeid=type_id, - notes=data.get('notes') - ) - - db.session.add(rel) + if existing is not None: + # The soft-deleted row from the duplicate check. Reuse it: the unique + # index on (source, target, type) means there is no second row to add. + rel = existing + rel.isactive = True + rel.notes = data.get('notes') + else: + rel = AssetRelationship( + sourceassetid=source_id, + targetassetid=target_id, + relationshiptypeid=type_id, + notes=data.get('notes') + ) + db.session.add(rel) apply_import_timestamps(rel, data) db.session.flush() diff --git a/tests/test_core/test_relationship_soft_delete.py b/tests/test_core/test_relationship_soft_delete.py new file mode 100644 index 0000000..d8f9270 --- /dev/null +++ b/tests/test_core/test_relationship_soft_delete.py @@ -0,0 +1,76 @@ +"""Deleting a relationship is soft, and everything that reads them must know. + +Rows are retired with isactive = False, never removed, and assetrelationships is +unique on (source, target, type). Anything that looks a relationship up without +filtering on isactive therefore sees links the UI does not show - and refuses to +create one it cannot create any other way, or retires the last live row believing +another one covers it. +""" + +import pytest + +from shopdb.core.models import Asset, AssetType +from shopdb.core.models.relationship import RelationshipType, AssetRelationship + + +@pytest.fixture +def rig(db): + if not AssetType.query.filter_by(assettype='computer').first(): + db.session.add(AssetType(assettype='computer')) + if not AssetType.query.filter_by(assettype='machine').first(): + db.session.add(AssetType(assettype='machine')) + controls = RelationshipType.query.filter_by(relationshiptype='controls').first() + if not controls: + controls = RelationshipType(relationshiptype='controls', isdirectional=True) + db.session.add(controls) + controls.isdirectional = True + db.session.commit() + computer = AssetType.query.filter_by(assettype='computer').first() + machine = AssetType.query.filter_by(assettype='machine').first() + pc = Asset(assetnumber='PCSOFT01', assettypeid=computer.assettypeid) + bay = Asset(assetnumber='BAYSOFT01', assettypeid=machine.assettypeid) + db.session.add_all([pc, bay]) + db.session.commit() + return {'pc': pc, 'bay': bay, 'controls': controls} + + +def _create(client, headers, source, target, typeid): + return client.post('/api/assets/relationships', + json={'sourceassetid': source, 'targetassetid': target, + 'relationshiptypeid': typeid}, + headers=headers) + + +def test_a_deleted_relationship_can_be_created_again(client, db, rig, auth_headers): + """Soft delete then re-add answered 409 about a link nothing displayed.""" + created = _create(client, auth_headers, rig['pc'].assetid, rig['bay'].assetid, + rig['controls'].relationshiptypeid) + assert created.status_code == 201, created.get_json() + relid = created.get_json()['data']['relationshipid'] + + assert client.delete('/api/assets/relationships/%d' % relid, + headers=auth_headers).status_code == 200 + + again = _create(client, auth_headers, rig['pc'].assetid, rig['bay'].assetid, + rig['controls'].relationshiptypeid) + assert again.status_code in (200, 201), again.get_json() + row = db.session.get(AssetRelationship, relid) + assert row.isactive is True + assert AssetRelationship.query.filter_by( + sourceassetid=rig['pc'].assetid, + targetassetid=rig['bay'].assetid).count() == 1 + + +def test_a_deleted_inverse_does_not_block_the_right_direction( + client, db, rig, auth_headers): + """'Remove the existing one first' is the advice the guard itself gives.""" + wrong = _create(client, auth_headers, rig['bay'].assetid, rig['pc'].assetid, + rig['controls'].relationshiptypeid) + assert wrong.status_code == 201 + relid = wrong.get_json()['data']['relationshipid'] + assert client.delete('/api/assets/relationships/%d' % relid, + headers=auth_headers).status_code == 200 + + right = _create(client, auth_headers, rig['pc'].assetid, rig['bay'].assetid, + rig['controls'].relationshiptypeid) + assert right.status_code in (200, 201), right.get_json()