-
-
Notifications
You must be signed in to change notification settings - Fork 95
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
Implement Feedback submission #558
base: main
Are you sure you want to change the base?
Changes from all commits
3e7889c
523a1f7
eb4adea
64f0eb8
900f497
bfc63dc
4921628
dc27d25
21c1e35
f008ef3
15f242f
ce32827
80331a4
cf43d92
911f314
5738e4f
0b3996a
e888187
1245417
3678db4
acf9cd2
6d727af
4d8c455
2846e66
f4f14f3
15a155b
ed32542
3e65a27
268d57f
8baac60
b70185e
d2dae82
ebc35b7
7ae97d3
72e788b
663e731
ce5db3a
66e68fa
1e23459
93ba308
c0eb02d
fc77079
bd10806
20b5060
740ddfa
c3db979
f49f08a
636a6ff
0360316
04213f1
cfe1690
1978b89
65e60c1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
# Register your models here. |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
@@ -0,0 +1,95 @@ | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"""Handle feedback submission, saving to local DB and uploading to S3.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import csv | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from datetime import datetime, timezone | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from io import StringIO | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import boto3 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
import botocore | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from django.conf import settings | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from django.core.exceptions import ValidationError | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from rest_framework import status, viewsets | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from rest_framework.permissions import AllowAny | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
from rest_framework.response import Response | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
class FeedbackViewSet(viewsets.ModelViewSet): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"""ViewSet for handling feedback.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
permission_classes = [AllowAny] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def create(self, request): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"""Handle POST request for feedback submission.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
s3_client = self._get_s3_client() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
output, writer = self._get_or_create_tsv(s3_client) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self._write_feedback_to_tsv(writer, request.data) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self._upload_tsv_to_s3(s3_client, output) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return Response(status=status.HTTP_201_CREATED) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
except ValidationError: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return Response({"error": "Invalid Credentials"}, status=status.HTTP_400_BAD_REQUEST) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def _get_s3_client(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"""Initialize and returns the S3 client.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return boto3.client( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"s3", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
aws_access_key_id=settings.AWS_ACCESS_KEY_ID, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
aws_secret_access_key=settings.AWS_SECRET_ACCESS_KEY, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
region_name=settings.AWS_S3_REGION_NAME, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def _get_or_create_tsv(self, s3_client, tsv_key="feedbacks.tsv"): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"""Get the existing TSV file or creates a new one if it doesn't exist.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
output = StringIO() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
writer = csv.writer(output, delimiter="\t") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
try: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
response = s3_client.get_object( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Bucket=settings.AWS_STORAGE_BUCKET_NAME, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Key=tsv_key, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# read the content from the body of the response | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
existing_content = response["Body"].read() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# decode the content to utf-8 format | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
decoded_content = existing_content.decode("utf-8") | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# write the decoded content to the output file | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
output.write(decoded_content) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
# move the cursor to the end of the file | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
output.seek(0, 2) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What's going on here and can we make it a bit more readable please? |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
except botocore.exceptions.ClientError as e: | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
if e.response["Error"]["Code"] == "NoSuchKey": | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
writer.writerow( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
["Name", "Email", "Message", "is_anonymous", "is_nestbot", "created_at"] | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return output, writer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def _write_feedback_to_tsv(self, writer, feedback_data): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"""Write the new feedback data to the TSV file.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
writer.writerow( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
feedback_data["name"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
feedback_data["email"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
feedback_data["message"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
feedback_data["is_anonymous"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
feedback_data["is_nestbot"], | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
datetime.now(timezone.utc).isoformat(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Comment on lines
+66
to
+77
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Refactor suggestion Consider input sanitization for TSV data The feedback data is directly written to the TSV without sanitizing inputs. This could cause issues if user inputs contain tab characters, newlines, or other special characters that might break the TSV format. Implement input sanitization: def _write_feedback_to_tsv(self, writer, feedback_data):
"""Write the new feedback data to the TSV file."""
+ # Sanitize inputs to prevent TSV injection
+ sanitized_data = {
+ key: str(value).replace('\t', ' ').replace('\n', ' ').replace('\r', ' ')
+ if isinstance(value, str) else value
+ for key, value in feedback_data.items()
+ }
+
writer.writerow(
(
- feedback_data["name"],
- feedback_data["email"],
- feedback_data["message"],
- feedback_data["is_anonymous"],
- feedback_data["is_nestbot"],
+ sanitized_data["name"],
+ sanitized_data["email"],
+ sanitized_data["message"],
+ sanitized_data["is_anonymous"],
+ sanitized_data["is_nestbot"],
datetime.now(timezone.utc).isoformat(),
)
) 📝 Committable suggestion
Suggested change
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def _upload_tsv_to_s3(self, s3_client, output): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"""Upload the updated TSV file back to S3.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
output.seek(0) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
s3_client.put_object( | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Bucket=settings.AWS_STORAGE_BUCKET_NAME, | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Key="feedbacks.tsv", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
Body=output.getvalue(), | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
ContentType="text/tab-separated-values", | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def write_feedback_to_tsv(self, writer, feedback_data): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"""Public method to write feedback data to TSV format.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
self._write_feedback_to_tsv(writer, feedback_data) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
def get_s3_client(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
"""Public method to get the S3 client.""" | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
return self._get_s3_client() |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,9 @@ | ||
"""Feedback API URLs.""" | ||
|
||
from rest_framework import routers | ||
|
||
from apps.feedback.api.feedback import FeedbackViewSet | ||
|
||
router = routers.SimpleRouter() | ||
|
||
router.register(r"feedback", FeedbackViewSet, basename="feedback") |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
from django.apps import AppConfig | ||
|
||
|
||
class FeedbackConfig(AppConfig): | ||
default_auto_field = "django.db.models.BigAutoField" | ||
name = "apps.feedback" |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1 @@ | ||
"""Module contains the models for the feedback app.""" |
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.
Error handling could be more specific
The catch block only handles
ValidationError
, but other exceptions (like network issues or S3 failures) are not caught, which could expose internal errors to users.Implement more comprehensive error handling:
📝 Committable suggestion