handle no deleted_at col

[jd-solanki]

Pull Request Template for FastCRUD

🚨 PR fails in some tests related to soft delete & I can't figure out why 🤔

Can someone help me fix it?

Description

closes #148

Changes

I've conditionally checked if the deleted_at col exist or not.

Tests

None. I don't know much about pytest

Checklist

• I have read the CONTRIBUTING document.
• My code follows the code style of this project.
• I have added necessary documentation (if appropriate).
• I have added tests that cover my changes (if applicable).
• All new and existing tests passed.

Additional Notes

1. Instead of checking it in code, Why don't we allow setting those cols to None so we can be sure that this cols doesn't exist instead of dynamically checking.
2. Also I noticed, In delete func above we use hasattr(db_row, self.is_deleted_column) for checking and after that we use self.is_deleted_column in self.model_col_names, why?

---

This is my first time contribution to your repo so I might now know the structure and all the conditions.

[igorbenav]

Here we got:

FAILED tests/sqlalchemy/crud/test_delete.py::test_delete_soft_delete - assert None is not None
 +  where None = <tests.sqlalchemy.conftest.ModelTest object at 0x7fec745cf410>.deleted_at
FAILED tests/sqlalchemy/crud/test_delete.py::test_soft_delete_with_custom_columns - AssertionError: Record should have a deletion timestamp
assert None is not None
 +  where None = getattr(<tests.sqlalchemy.conftest.ModelTest object at 0x7fec74246010>, 'deleted_at')
FAILED tests/sqlmodel/crud/test_delete.py::test_delete_soft_delete - AssertionError: assert None is not None
 +  where None = ModelTest(name='Charlie', category_id=1, is_deleted=True, deleted_at=None, id=1, tier_id=1).deleted_at
FAILED tests/sqlmodel/crud/test_delete.py::test_soft_delete_with_custom_columns - AssertionError: Record should have a deletion timestamp
assert None is not None
 +  where None = getattr(ModelTest(name='Charlie', category_id=1, is_deleted=True, deleted_at=None, id=1, tier_id=1), 'deleted_at')

(Twice because of sqlalchemy and sqlmodel)
So let's check them one at a time:

test_delete_soft_delete

FAILED tests/sqlalchemy/crud/test_delete.py::test_delete_soft_delete - assert None is not None
 +  where None = <tests.sqlalchemy.conftest.ModelTest object at 0x7fec745cf410>.deleted_at

If we actually go to the test, we can see:

@pytest.mark.asyncio
async def test_delete_soft_delete(async_session, test_data, test_model):
    for item in test_data:
        async_session.add(test_model(**item))
    await async_session.commit()

    crud = FastCRUD(test_model)
    some_existing_id = test_data[0]["id"]
    await crud.delete(db=async_session, id=some_existing_id)

    soft_deleted_record = await async_session.execute(
        select(test_model).where(test_model.id == some_existing_id)
    )
    soft_deleted = soft_deleted_record.scalar_one()
    assert soft_deleted.is_deleted is True 

    # here it's checking whether there is a deleted_at column that was also changed
    assert soft_deleted.deleted_at is not None 

test_soft_delete_with_custom_columns

FAILED tests/sqlalchemy/crud/test_delete.py::test_soft_delete_with_custom_columns - AssertionError: Record should have a deletion timestamp
assert None is not None
 +  where None = getattr(<tests.sqlalchemy.conftest.ModelTest object at 0x7fec74246010>, 'deleted_at')

The test:

@pytest.mark.asyncio
async def test_soft_delete_with_custom_columns(async_session, test_data, test_model):
    crud = FastCRUD(
        test_model, is_deleted_column="is_deleted", deleted_at_column="deleted_at"
    )
    some_existing_id = test_data[0]["id"]

    for item in test_data:
        async_session.add(test_model(**item))
    await async_session.commit()

    await crud.delete(db=async_session, id=some_existing_id, allow_multiple=False)

    deleted_record = await async_session.execute(
        select(test_model)
        .where(test_model.id == some_existing_id)
        .where(getattr(test_model, "is_deleted") == True)  # noqa
    )
    deleted_record = deleted_record.scalar_one_or_none()

    assert deleted_record is not None, "Record should exist after soft delete"
    assert (
        getattr(deleted_record, "is_deleted") == True  # noqa
    ), "Record should be marked as soft deleted"

    # Here's the part that's failing, checking that there's a deletion timestamp
    assert (
        getattr(deleted_record, "deleted_at") is not None
    ), "Record should have a deletion timestamp"

So basically removing the deleted_at column makes the tests fail because the tests are checking whether there is a deleted_at column that's being updated at some point

[igorbenav]

Btw this is related to #88, so you may want to fix this one as well (being necessary to have either is_deleted or deleted_at). Plus, this change is pretty big, so new tests and docs updates are necessary

[jd-solanki]

Thanks for your review. I will let you know once I finish it!

[igorbenav]

Fixed by #152