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
Basic support for broadcast messages #66
Changes from all commits
51cd0cc
2913cb4
8437dba
c0d66ab
8987b95
10f05e9
c60ddab
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.
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.
NO_CONTENT is a way to signify success but that no the response contains no other data and that no action is expected on the part of the client. The practical differences between this and a 200 are pretty small so it's just a nicety. What a POST would return depends on the context, in the case of a user submitting a form it's usually a redirect, in the case of an API call it would likely return the json representation of the created object.