Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Basic support for broadcast messages #66
base: main
Are you sure you want to change the base?
Basic support for broadcast messages #66
Changes from all commits
51cd0cc
2913cb4
8437dba
c0d66ab
8987b95
10f05e9
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Commenting as a reminder to open this as an issue to address later
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Add a
TODO
labelThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What these two do?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
require_POST
makes it so that the view only accepts POST requests.csrf_exempt
removes django's cross site request forgery protection which isn't relevant here.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see how this is hacky. Is the plan to add a
messages
field to the user model?Also, is there a desire to make this a REST API using the rest_framework?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why would a REST API endpoint be best here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whilst we could store messages under the user (or other model) I think the ephemeral nature of the data makes storage in user sessions a better fit - https://docs.djangoproject.com/en/5.1/topics/http/sessions/. Ultimately this does store data in the database but it provides a need way of making sure we only collect messages when a user needs to see them.
No desire to use rest_framework as it's a bit of a sledge hammer. After thinking about it more I don't think we actually need an endpoint if we write the Kafka consumer as a custom django-admin command it can populate data into sessions directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is
NO_CONTENT
returned here? Is it the same in general for a successful POST?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, is this all it is from the point of view of the consumer or it is just a minimal version to get going? Should this be running as some sort of daemon rather than a script?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As mentioned in a previous comment I suggest we turn this into a
manage.py
command. Could probably use a pass for robustness.