add the option to mark users as deleted instead of actually deleting them from the db

[crosspolar]

Resolves #328

[Theophile-Madet]

The class NonDeleted(models.Manager) is nice for default filtering.

I think we want to keep the default delete() as it is, there's too much uncertainty about what happens when we override it.

There are many tests failing, do you want to look into them or do you need help?

[Theophile-Madet]

Looks better, I think the next big thing is tests for ShareOwnerDeleteView.

[Theophile-Madet]

Should this be a FormView since we're not actually deleting the object?

[crosspolar]

as far as I understand, deleteview inherits from a formview and provides this nice confirmation form, otherwise I would need to create the logic of conformation, right?

[Theophile-Madet]

You already have a custom template and a custom form_valid(), so there's nothing left from DeleteView that we are using.

[crosspolar]

Wouldn't we need to create extra form for a FormView? Also, I like the DeleteView so make it more clear that we delete ShareOwners here. If you don't have a strong opinion, I'd like to leave it here

[Theophile-Madet]

I still think DeleteView is not correct since we don't delete the objects. Using DeleteView is but overriding everything that is specific to delete view doesn't make sense. The intention is already clear from the class name (although that name should probably be ShareOwnerSoftDeleteView instead of ShareOwnerDeleteView).

You can use UpdateView as parent class, it's 2 lines more:

class ShareOwnerDeleteView(
    LoginRequiredMixin,
    PermissionRequiredMixin,
    generic.UpdateView,
):
    permission_required = PERMISSION_GROUP_MANAGE
    model = ShareOwner
    template_name = "coop/shareowner_confirm_delete.html"
    fields = []

[crosspolar]

Edit: Solved, 1. was because DeleteView was actually deleting users with calling super() of course
What confuses me is that there are different behavior:

1. If I implement a soft_delete() method, I still can open the page of the user and it shows some message top-right that this user is no share-owner, but all other entries are still there. However, test_ShareOwnerDeleteView_shareOwnerdeletedAt_hasDate fails since it struggles to find the share-owner. Also, I can't find the Shareowner in ShareOwner.everything.all()...
2. If I override delete() my tests are working but I can't open the Tapir-user's page, it ends with some db-error. The user seems to be in ShareOwner.everything.all()

Have to investigate more...

[crosspolar]

It seems to work now. However, when going to a deleted user's page, it fails with

web-1          |   File "/app/tapir/coop/templatetags/coop.py", line 26, in member_status_colored_text
web-1          |     status = share_owner.get_member_status(at_datetime)
web-1          |   File "/app/tapir/coop/models.py", line 276, in get_member_status
web-1          |     if InvestingStatusService.is_investing(self, at_datetime):
web-1          |        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^
web-1          |   File "/app/tapir/coop/services/investing_status_service.py", line 34, in is_investing
web-1          |     annotated_date = getattr(share_owner, cls.ANNOTATION_WAS_INVESTING_AT_DATE)
web-1          | AttributeError: 'NoneType' object has no attribute 'was_investing_date_check'

[Theophile-Madet]

It seems to work now. However, when going to a deleted user's page, it fails with

You'll need to change this line to use ShareOwner.everything

cls.annotate_share_owner_queryset_with_investing_status_at_datetime(
    ShareOwner.objects.filter(id=share_owner.id), at_datetime
).first()

[crosspolar]

It seems to work now. However, when going to a deleted user's page, it fails with

You'll need to change this line to use ShareOwner.everything

cls.annotate_share_owner_queryset_with_investing_status_at_datetime(
    ShareOwner.objects.filter(id=share_owner.id), at_datetime
).first()

I think that would make things more complicated, since all the calls of is_investing would return the deleted members included. Maybe it's easier to disable the shareowner-card

[Theophile-Madet]

is_investing takes a single ShareOwner as argument so I think it makes sense for it to work even if that user is deleted. I don't see what gets more complex using .everything

[Theophile-Madet]

Most comments are suggestions for test names and other small things. The main thing is still the usage of DeleteView vs another generic view.

[Theophile-Madet]

I think we should either:

• keep hard_delete and override delete to always throw an error and give the hint that either soft or hard delete should be used
• remove hard_delete and keep the normal delete

Otherwise hard_delete doesn't do anything.

[Theophile-Madet]

I still think DeleteView is not correct since we don't delete the objects. Using DeleteView is but overriding everything that is specific to delete view doesn't make sense. The intention is already clear from the class name (although that name should probably be ShareOwnerSoftDeleteView instead of ShareOwnerDeleteView).

You can use UpdateView as parent class, it's 2 lines more:

class ShareOwnerDeleteView(
    LoginRequiredMixin,
    PermissionRequiredMixin,
    generic.UpdateView,
):
    permission_required = PERMISSION_GROUP_MANAGE
    model = ShareOwner
    template_name = "coop/shareowner_confirm_delete.html"
    fields = []

[Theophile-Madet]

Could we just use a ShareOwnerFactory?

[Theophile-Madet]

This is already done in tapir/coop/tests/test_shareowner_delete_view.py, right? Then we can remove this test