New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversations #333

Merged
merged 32 commits into from Aug 10, 2017

Conversation

Projects
None yet
3 participants
@nicksellen
Copy link
Member

nicksellen commented Aug 5, 2017

Ok, I added conversations!

I rejigged the architecture a bit from the hackathon. First I'll define a few concepts:

Name From? Meaning
Channel django channels a single persistent connection from a client to the backend, basically, a websocket connection ...
reply_channel django channels the id for a Channel
ChannelGroup django channels you can add reply_channels into a group, then send messages to the group, we do not use them right now
ChannelSubscription fs tool db model in foodsaving.subscriptions app has reply_channel, user, lastseen fields - represents an intention to receive some messages to a particular channel, currently just holds the user but could also store more detailed user intentions

The relationship between Conversation and is now a GenericForeignKey on the Conversation. You can include ConversationMixin into some other model which will give you aninstance.conversation to create/load a conversation.

The previous implementation I did at the hackathon would populate ChannelGroups for all of a users conversations when they connected. But I decided it doesn't seem necessary to do so much stuff, most of the users will not do much, and most of the conversations will remain empty (remember a "conversation" might end up being on many many things - there could be a lot).

So for the connection flow (implemented in the foodsaving.subscriptions app to keep it seperate):

  • user connects to websocket
  • if they are not authenticated we leave it there (although leave them connected)
  • we create a ChannelSubscription entry for them
  • periodically the client should send a heartbeat/keepalive/ping message, we will update the lastseen timestamp on the ChannelSubscription, this is because you cannot rely on getting a nice clean "disconnect" message on the channel, and we need to cleanup
  • if we do get a nice clean disconnect, we delete the ChannelSubscription

Then the bridge between conversations and subscriptions (currently in subscriptions/receivers, but arguably should be slightly abstracted and put under conversations/receivers):

  • we listen to two signals from the conversations:
    1. on new message
    • we find all the ChannelSubscription objects relating to users in the conversation...
    • ..., then send them the message
    1. on user leaving a conversation
    • same as above, but we send them a message telling them they were removed from the conversation

Then, for the conversations part (pretty simple);

  • Conversation, ConversationParticipant, and ConversationMessage
  • has API endpoints for listing conversations and messages, and creating messages

Then, Group is the only model that has a conversation now and has to obey the contract to handle the logic for how to add/remove participants from the conversation. In this case kind of simple, as if you are in the group you should be in the conversation. it's handled with signals in groups/receivers.

Currently it includes the logic about creating/deleting the conversation too, but I'd like to add that into ConversationMixin too. But not right now.

I added tests for about half of it so far (has a nice way of handling the websocket tests, was very nice! - https://channels.readthedocs.io/en/stable/testing.html).

You can try it out too!

Adding this subscriptions concept actually means if you want to send other stuff to the users browsers you can - would have to flesh it out a bit more, but it would support a concept where users are subscribed to changes to the current item they are on (e.g. a store page), and can get notified when things happen (e.g. "store updated" - with some payload data, or trigger a reload etc...).

TODO:

  • add tests for the remaining parts (mostly conversations)
  • maybe switch to use class based consumers
  • maybe add serializers for the messages we send to the clients
  • consider moving the conversation signal receivers -> subscriptions logic into conversations
  • slightly abstract the subscriptions to provide a simple send_message_to_user() type interface
  • perhaps get slightly fancier with serializers to return slightly more/useful data in the API endpoints (I guess need to work out which data is useful first...)
  • work out which API endpoints returning which data will be useful in the frontend
  • maybe work on adding a frontend angular service to hit the endpoints and do the websocket stuff
  • make sure the way I did the migrations works with existing databases (I tried... I had to remove the old conversations/messages/etc)
  • remove the frontendchat app
  • squash some of my older commits out...

Ok, thats enough words for now :)

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented Aug 5, 2017

Codecov Report

Merging #333 into master will increase coverage by 0.32%.
The diff coverage is 98.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #333      +/-   ##
==========================================
+ Coverage   97.89%   98.21%   +0.32%     
==========================================
  Files         140      153      +13     
  Lines        3891     4157     +266     
  Branches      174      181       +7     
==========================================
+ Hits         3809     4083     +274     
+ Misses         63       54       -9     
- Partials       19       20       +1
Impacted Files Coverage Δ
...oodsaving/conversations/migrations/0001_initial.py 100% <ø> (ø) ⬆️
foodsaving/base/base_models.py 100% <ø> (ø) ⬆️
foodsaving/groups/serializers.py 100% <ø> (ø) ⬆️
foodsaving/conversations/api.py 100% <100%> (+6.89%) ⬆️
...saving/subscriptions/tests/test_cleanup_command.py 100% <100%> (ø)
foodsaving/conversations/tests/test_models.py 100% <100%> (ø)
foodsaving/conversations/factories.py 100% <100%> (+10%) ⬆️
foodsaving/subscriptions/__init__.py 100% <100%> (ø)
...onversations/migrations/0004_auto_20170805_1653.py 100% <100%> (ø)
foodsaving/subscriptions/tests/test_consumers.py 100% <100%> (ø)
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 918a5b8...f1e93ec. Read the comment docs.

@nicksellen

This comment has been minimized.

Copy link
Member

nicksellen commented Aug 5, 2017

Just a bit more detail on the reason I stopped using the ChannelGroup - as they are actually pretty nice, it delegates the actual sending to all the subscribers to redis (or other backends) instead of sending out multiple messages from the backend.

... the problem is managing the adding and removing of users to and from groups. As conversations are designed to be lightweight and added to almost anything there is likely to be a much larger number of things to receive messages about compared to the number of connected users).

If each of the things you might receive a message about had a channel group, it makes sending the updates easy (ChannelGroup('conversation:10').send(...)), but it means you have to make sure each user is in all the correct groups immediately after they connect, and then if any of their access (added/removed from group etc) or intentions change (navigate to a page and don't want messages about some stuff).

So the answer here is to store a very broad user intention in the ChannelSubscription table (currently just "I am a connected user and I might want to receive some stuff"), and then choose who should receive messages when the event itself happens (e.g. a new message). Having it in the database also means it's available to do db joins on to limit the results to connected users. With ChannelGroups it's not possible (afaik) to easily get a list of the users in a group anyway.

I'm mostly writing this stuff whilst it's in my mind, should probably transfer it to a documentation area.

nicksellen added some commits Jul 3, 2017

Add subscriptions between conversations & channels
Instead of adding users to a ChannelGroup for each conversation
they are part of (most of them will be inactive anyway) now it:
- for each user connection we save a ChannelSubscription entry
- when a new ConversationMessage is created it sends a message
  to the intersection of users in the conversation and have
  a channel subscription

It avoids having to manage the ChannelGroup members as people leave
/join groups (nothing to do with sending messages).

Also, added tests!
Conversation tests and api mixin
I also removed the conversations api and decided for now you
should only access the conversation on the entities that they
are for, so: `/api/groups/{id}/conversation/` will get or create
the conversation.

The `/messages/` endpoint is not quite ready for primetime yet, but
sort of works :)
Add pytesting utilities
Execute with:

```
pytest
```

Or with watching, and only-changed-files:

```
ptw -- --testmon
```
Remove redundant ws accept replies
It's only relevent on connection
@tiltec
Copy link
Member

tiltec left a comment

Had another read of the code, looks good to me. Some of the TODOs are more important than others ;)


def get_queryset(self):
# TODO: should return an error if the user is not in the conversation, not just filter messages

This comment has been minimized.

@tiltec

tiltec Aug 7, 2017

Member

Would do this with a custom permission class.

This comment has been minimized.

@nicksellen
group = kwargs.get('instance')
user_ids = kwargs.get('pk_set')

if action == 'post_add':

This comment has been minimized.

@tiltec

tiltec Aug 7, 2017

Member

Would it be sufficient to use sync_users for add and remove as well?

This comment has been minimized.

@nicksellen

nicksellen Aug 8, 2017

Member

Yes, but it would reload all the users each time. Would work, but so does this :)

manage.py Outdated
@@ -6,5 +6,30 @@
os.environ.setdefault("DJANGO_SETTINGS_MODULE", "config.settings")

from django.core.management import execute_from_command_line
from django.conf import settings

if 'test' in sys.argv:

This comment has been minimized.

@nicksellen

nicksellen Aug 8, 2017

Member

Should probably put all this logic inside config.test_settings or something, then switch the DJANGO_SETTINGS_MODULE when in test.

This comment has been minimized.

@nicksellen
@@ -0,0 +1,6 @@
ipython
pytest

This comment has been minimized.

@nicksellen

nicksellen Aug 8, 2017

Member

Actually I would get rid of pytest from this now...

This comment has been minimized.

@nicksellen

nicksellen added some commits Aug 8, 2017

Add test for 403 error
Should be same error if the conversation does not exist or if you
don't have permission. We hide the existence of the conversation.

@nicksellen nicksellen changed the title WIP: Conversations Conversations Aug 8, 2017

@tiltec
Copy link
Member

tiltec left a comment

127.0.0.1:8000 doesn't open in the latest version, it just hangs and no response comes.

When I reset manage.py (git checkout master -- manage.py) then it works as expected. Was this introduced in b4a98b0?

(using this fancy review thing!)

@nicksellen

This comment has been minimized.

Copy link
Member

nicksellen commented Aug 8, 2017

@tiltec hmm odd, it's not very different from normal manage.py now, see https://github.com/yunity/foodsaving-backend/pull/333/files#diff-632b651e5579af0e928ed63482e9bc77R7

Basically:

if 'test' in sys.argv:
    os.environ.setdefault("DJANGO_SETTINGS_MODULE", "config.test_settings")
else:
    os.environ.setdefault("DJANGO_SETTINGS_MODULE", "config.settings")

Instead of:

os.environ.setdefault("DJANGO_SETTINGS_MODULE", "config.settings")
@nicksellen

This comment has been minimized.

Copy link
Member

nicksellen commented Aug 8, 2017

If I run manage.py with test_settings then it doesn't even start:

(env) ada/nick: ./manage.py runserver
CommandError: You must set settings.ALLOWED_HOSTS if DEBUG is False.

I can't see how the version of manage.py in master is executing anything differently from the one in here (unless running the test command).

@tiltec

This comment has been minimized.

Copy link
Member

tiltec commented Aug 8, 2017

config.test_settings would need to be extended by some things (e.g. all installed apps) or inherit from config.settings.

I would find it better if you put those config-related changes into a separate PR.

# See https://brobin.me/blog/2016/08/7-ways-to-speed-up-your-django-test-suite/

# noinspection PyUnresolvedReferences
from .settings import * # noqa: F401,F403

This comment has been minimized.

@tiltec

tiltec Aug 8, 2017

Member

Just saw this. Well then it should theoretically work, as it also loads local_settings.

This comment has been minimized.

@nicksellen

nicksellen Aug 9, 2017

Member

I removed this stuff now for this PR

@nicksellen

This comment has been minimized.

Copy link
Member

nicksellen commented Aug 9, 2017


ChannelSubscription.objects.filter(lastseen_at__lt=timezone.now() - relativedelta(minutes=5)).delete()

print('done')

This comment has been minimized.

@tiltec

tiltec Aug 9, 2017

Member

This makes the test output a tad more confusing:

............................................................................................................................................................................................done
.done
.................................................................
----------------------------------------------------------------------
Ran 254 tests in 12.275s

I would either get rid of the 'done' or redirect it during testing.

This comment has been minimized.

@nicksellen

@tiltec tiltec referenced this pull request Aug 9, 2017

Closed

Add pytest and friends #334

@nicksellen nicksellen referenced this pull request Aug 9, 2017

Merged

Speed up test execution #338

I removed these changes now, and put them in another PR

@tiltec

tiltec approved these changes Aug 10, 2017

@tiltec tiltec merged commit cf24833 into master Aug 10, 2017

5 checks passed

ci/circleci Your tests passed on CircleCI!
Details
ci/circleci-e2e end-to-end test suite
Details
codecov/patch 98.53% of diff hit (target 97.89%)
Details
codecov/project 98.21% (+0.32%) compared to 918a5b8
Details
pyup.io/safety-ci No dependencies with known security vulnerabilities.
Details

@tiltec tiltec deleted the conversations branch Aug 10, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment