Refactor ModelObserver and Fix Test Warnings for Consistency and Safety

[tumblingman]

This PR includes two key updates to improve the functionality and reliability of the ModelObserver and the test suite:

1. Refactor ModelObserver for transaction safety:

• Messages for database events (CREATE, UPDATE, DELETE) are now prepared before the transaction is committed.
• Added new methods (prepare_messages, generate_messages, send_prepared_messages) to enhance code clarity and consistency.
• Ensures DELETE actions retain access to the primary key (pk) and other instance data when messages are sent.

2. Fix warnings and improve test suite:

• Resolved warnings like Task was destroyed but it is pending! caused by improper cleanup in tests.
• Added __test__ = False to test models to prevent pytest from misinterpreting them as test cases.
• Overhauled communicator management in tests:
  • Introduced a context manager for the communicator to ensure proper cleanup.
  • Prevented dangling communicators from causing test instability.

Benefits:

1. Improved transaction handling in ModelObserver for DELETE actions.
2. Ensured all tests run cleanly without warnings or residual side effects.
3. Simplified and standardized test communicator usage, reducing the risk of errors in future tests.

This PR significantly improves the reliability and maintainability of both the ModelObserver implementation and the associated test suite.

[hishnash]

What about making some larger changes to the class:

Do all the message building (for all actions) before the commit. And then on commit send them all.

This way the messages for delete will not be sent if the transaction is rolled back, but they will still have access to the pk.

You could have send_message yield or return the messages rather than calling https://github.com/NilCoalescing/djangochannelsrestframework/pull/208/files#diff-5d27ab5ea78b0069ed90a381b278d801e1079561c390d57aaaab47d9ba4eb7e3L166

Then have post_change_receiver do the same, then you can always call post_change_receiver before https://github.com/NilCoalescing/djangochannelsrestframework/pull/208/files#diff-5d27ab5ea78b0069ed90a381b278d801e1079561c390d57aaaab47d9ba4eb7e3R120 and then within connection.on_commit you can send them messages.

[tumblingman]

What about making some larger changes to the class:

Do all the message building (for all actions) before the commit. And then on commit send them all.

This way the messages for delete will not be sent if the transaction is rolled back, but they will still have access to the pk.

You could have send_message yield or return the messages rather than calling https://github.com/NilCoalescing/djangochannelsrestframework/pull/208/files#diff-5d27ab5ea78b0069ed90a381b278d801e1079561c390d57aaaab47d9ba4eb7e3L166

Then have post_change_receiver do the same, then you can always call post_change_receiver before https://github.com/NilCoalescing/djangochannelsrestframework/pull/208/files#diff-5d27ab5ea78b0069ed90a381b278d801e1079561c390d57aaaab47d9ba4eb7e3R120 and then within connection.on_commit you can send them messages.

Thank you for your suggestion! I really like the idea of handling all message building before the commit and then sending them on commit. It makes a lot of sense to ensure consistency and proper rollback handling. I'll work on implementing this approach tomorrow.

[tumblingman]

What about making some larger changes to the class:

Do all the message building (for all actions) before the commit. And then on commit send them all.

This way the messages for delete will not be sent if the transaction is rolled back, but they will still have access to the pk.

You could have send_message yield or return the messages rather than calling https://github.com/NilCoalescing/djangochannelsrestframework/pull/208/files#diff-5d27ab5ea78b0069ed90a381b278d801e1079561c390d57aaaab47d9ba4eb7e3L166

Then have post_change_receiver do the same, then you can always call post_change_receiver before https://github.com/NilCoalescing/djangochannelsrestframework/pull/208/files#diff-5d27ab5ea78b0069ed90a381b278d801e1079561c390d57aaaab47d9ba4eb7e3R120 and then within connection.on_commit you can send them messages.

hishnash, I'm done!

Pytest results:

[hishnash]

Looks like the tests are all timing out, you might need to bump some package versions to match your local setup.

https://github.com/NilCoalescing/djangochannelsrestframework/blob/master/setup.py. and in requirements.txt maybe would be good to pin these to a new version.

[tumblingman]

Looks like the tests are all timing out, you might need to bump some package versions to match your local setup.

https://github.com/NilCoalescing/djangochannelsrestframework/blob/master/setup.py. and in requirements.txt maybe would be good to pin these to a new version.

I’ve fixed everything and tested it again. I updated the list of dependencies and ran the tests once more. There were two errors in the tests: in two cases, after subscribing to database changes, instances of the model we are observing were immediately created, and the communicator was requested for a message. As a result, a TimeoutError occurred because the subscription process was not completed before the model creation.

Could you please let me know if the solution with a timeout works for you? Alternatively, in my code, I handle this differently: in every "subscribe" action, I always send a response to the client confirming that the subscription is established. This way, even in the tests, we can wait for a response before inserting data into the database to ensure that the observer works as expected. I believe this approach would be more deterministic.

Also, I want to note that during the first test run, before starting development, I encountered an error stating that the daphne package was missing in the virtual environment, and I had to install it manually. If you face a similar issue on the first run, maybe it makes sense to add daphne to requirements.txt?

[tumblingman]

In this latest commit, I refactored two test cases where multiple websocket connections are established, and one of them is terminated during execution.

Previously, the code used nested async with statements:

async with connected_communicator(TestOtherConsumer()) as communicator1:
    async with connected_communicator(TestUserConsumer()) as communicator2:

This was replaced with a more concise and flexible approach using AsyncExitStack:

async with AsyncExitStack() as stack:
    communicator1 = await stack.enter_async_context(connected_communicator(TestOtherConsumer()))
    communicator2 = await stack.enter_async_context(connected_communicator(TestUserConsumer()))

This change improves readability and offers greater flexibility for managing multiple connections, making it easier to modify or extend in future test scenarios.

@hishnash I think I’m done with all the updates and fixes. Tests are now passing in both the djangochannelsrestframework library and my own project where I use it. Please take a look at these latest changes, and let me know if there’s anything else to address. Thanks again for your time and patience!

Pytest results: