No deleted model tracking in deserialization

[rtibbles]

Summary

• Adds a test to ensure that objects that are created, synced, deleted, synced, and then recreated properly sync
• Creates a fix for this by muting DeletedModel creation during deserialization

TODO

• Have tests been written for the new code?
• Has documentation been written/updated?
• New dependencies (if any) added to requirements file

Reviewer guidance

This seems to not break any existing tests, either here or in the Kolibri integration tests (including an addition I made to test the issue this is intended to address).

What I don't know is if the DeletedModel creation during deserialization is desirable - one possible additional test would be to add an integration test on Kolibri for deletion on A, update on B, and ensure that deletion persists on C when both are synced to it.

Issues addressed

This is intended to resolve learningequality/kolibri#11208

[jamalex]

An example of where creation of DeletedModel during deserialization could come into play:

• Class Q is create on device A, and synced to Device B
• Group X is created inside Class Q on Device B, and is then synced to Device C
• Class Q is deleted on Device A
• Device B pulls in data from Device A, which brings in the deletion of Class Q. Since Group X depends via foreign key on Class Q, Group X gets cascade deleted, and we then need to mark it as a DeletedModel so that the deletion gets reflected into the Store (and synced onwards e.g. to Device C).

Here's the code that handles that. ^^

So I'd recommend not doing a blanket muting of that deletion -- and instead figure out why it's getting triggered on the LoD (e.g. maybe even the deserialization of the synced deleted model triggers generation of a DeletedModel? For deletions that weren't initiated locally but rather synced in, it should skip that, as it's already marked as deleted in the Store -- but for things deleted as a result of them foreignkeying onto a deleted record, like in my scenario above, we do still want it to generate a DeletedModel so it gets written back to the Store).

[jamalex]

I think this would be the more focal place to wrap with the signal-skipper instead -- as it's handling the case where the deletion was synced in from elsewhere.

[rtibbles]

I have made the change more focal, and also added this section to the Kolibri ecosystem tests:

        # assert deletion takes priority over update
        alk_user = FacilityUser.objects.using(s0.db_alias).create(username="Antemblowind", facility=facility)
        # Sync to both devices
        s1.sync(s0, facility)
        s2.sync(s0, facility)

        # delete on s0
        s0.delete_model(FacilityUser, id=alk_user.id)
        # update on s1
        s1.update_model(FacilityUser, alk_user.id, username="Antemblowind2")
        # Sync deletion to s2
        s2.sync(s0, facility)
        self.assertFalse(
            FacilityUser.objects.using(s2.db_alias)
            .filter(id=alk_user.id)
            .exists()
        )
        # Sync update to s2
        s2.sync(s1, facility)
        self.assertFalse(
            FacilityUser.objects.using(s2.db_alias)
            .filter(id=alk_user.id)
            .exists()
        )

This seems to confirm that making this change doesn't affect the desired behaviour of deletion taking precedence over updates. I am going to attempt to add similar tests to this PR as well.

[bjester]

Makes sense to mute the delete signal in the deserialization stage. While unlikely to run into the same issue that this fixes, I think it also makes sense to mute the signal for the hard deletion since the core problem is that deserializing shouldn't generate a DeletedModels record.

Could you also increment the morango version and add a changelog entry so we can publish after merge?

[rtibbles]

I think it also makes sense to mute the signal for the hard deletion since the core problem is that deserializing shouldn't generate a DeletedModels record.

I did wonder about that - can do.

Could you also increment the morango version and add a changelog entry so we can publish after merge?

Sure.

[rtibbles]

Updated - I also added a couple of extra conditions/assertions to the Kolibri ecosystem tests to ensure this doesn't mess with transitive propagation of deletions, and it did not seem to.

[bjester]

I think your change to not differentiate between hard and soft deletes makes sense. In deserialization, the appropriate tracking records should already be created and the purpose of the hard delete functionality should be satisfied by that point.

One broken test it seems, but hopefully that just isn't matching the appropriate expectation for hard deletes