Contact Us: Modifies api to fetch name and email for logged in users. #922

RishabhJain2018 · 2017-04-09T20:57:23Z

fixes #921 . @deshraj Please review the PR.

taranjeet · 2017-04-10T03:26:47Z

apps/web/views.py

+            return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
+
+        elif request.method == 'GET':
+            user = User.objects.get(username=request.user)


Whenever using get operation, please make sure that it is properly catched. Same goes for post method.

@trojan Thank you for guiding. But I think in this case we don't need to catch it separately as it is already being cached in the try and except case.
The function works as follows:

If the user is logged in:
a) On get request the username and email is being fetched from the database.
b) On post request the name, email and message is posted to the database.

If the user is not logged in then name, email and message is posted to the database.

Please review and tell me if I am missing somewhere ?

We would probably remove the try statement from line 41 and make it more specific to the particular statement. also, if try-except block is required for some other piece of code, then you should add a separate try-catch for that statement.

deshraj · 2017-04-10T19:13:37Z

apps/web/views.py

+            return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST)
+
+        elif request.method == 'GET':
+            user = User.objects.get(username=request.user)


We would probably remove the try statement from line 41 and make it more specific to the particular statement. also, if try-except block is required for some other piece of code, then you should add a separate try-catch for that statement.

RishabhJain2018 · 2017-04-18T17:29:08Z

@deshraj @trojan Please review the PR.

taranjeet · 2017-05-07T17:36:16Z

apps/web/views.py

+    if request.method == 'POST':
+        request_data = {"name": name, "email": email}
+        request_data['message'] = request.data['message']
+        serializer = ContactSerializer(data=request_data)
        if serializer.is_valid():
            serializer.save()
            response_data = {'message': 'Your message has been successfully recorded. We will contact you shortly.'}


Can we make it "We have received your request and will contact you shortly" ?

taranjeet · 2017-05-07T17:37:12Z

apps/web/views.py

@@ -35,29 +35,35 @@ def internal_server_error(request):


 @throttle_classes([AnonRateThrottle, ])
-@api_view(['POST', ])
+@api_view(['POST', 'GET'])


Can we make it alphabetically ? GET, POST

taranjeet · 2017-05-07T17:45:15Z

apps/web/views.py

+
+    if request.method == 'POST':
+        request_data = {"name": name, "email": email}
+        request_data['message'] = request.data['message']


Is message mandatory on frontend. If not, then I will prefer to use .get here.

@trojan I think we should keep a check on front-end only as it will prevent unnecessary api calls.
What are your views about it?

Yeah, we should keep a check on the frontend for this. Also, we should throttle this endpoint to 10 requests/minute. Does that sound reasonable to you guys? @trojan @RishabhJain2018 ?

On front end, there should be a check that message is mandatory, because if a user is trying to contact then he or she should definitely leave a message.

Also as far as throttling is concerned, I am not sure of what exactly should be the limit.

Anyways it will be nice if you use .get here.

@deshraj Can you please tell why we should change the limit from 100 requests/minute to 10 requests/minute ?

Sorry I missed this part. IMO: a user won't submit the contact form 100 times in a minute. No one will have that many problems.

Anyways, this way we can restrict the users from abusing the system.

@deshraj I agree that we can restrict the user from abusing the system.

taranjeet · 2017-05-11T03:25:47Z

apps/web/views.py

-        request_data['message'] = request.data['message']
-        serializer = ContactSerializer(data=request_data)
+    except:
+        serializer = ContactSerializer(data=request.data)


This is redundant. Same thing is happening in POST check one just that data is different. I think it can be easily handled with a simple if condition. @RishabhJain2018

@trojan Please explain how can we remove the redundancy ?

I think something like this can. Let me know your concerns if any

user_does_not_exist = False try: user = User.objects.get(username=request.user) name = user.username email = user.email request_data = {'name': name, 'email': email} except: request_data = request.data user_does_not_exist = True if request.method == 'POST' or user_does_not_exist: if request.POST.get('message'): request_data['message'] = request.POST.get('message') serializer = ContactSerializer(data=request_data) if serializer.is_valid(): serializer.save() response_data = {'message': 'We have received your request and will contact you shortly.'} return Response(response_data, status=status.HTTP_201_CREATED) return Response(serializer.errors, status=status.HTTP_400_BAD_REQUEST) elif request.method == 'GET': response_data = {"name": name, "email": email} return Response(response_data, status=status.HTTP_200_OK)

deshraj · 2017-05-26T00:26:36Z

@RishabhJain2018 any update on this?

RishabhJain2018 · 2017-05-30T19:36:16Z

@deshraj @trojan Please review the PR.

taranjeet · 2017-05-31T18:18:26Z

LGTM. Let @deshraj review it once.

RishabhJain2018 · 2017-06-03T04:50:10Z

@deshraj @trojan Please review the PR.

deshraj · 2017-06-03T18:19:06Z

Looks good. Merging this. :)

Good job Rishabh.

taranjeet reviewed Apr 10, 2017

View reviewed changes

deshraj requested changes Apr 10, 2017

View reviewed changes

RishabhJain2018 added 3 commits April 18, 2017 21:56

Contact Us: Modifies api to fetch name and email for logged in users.

520ded6

flake8 fixes

3feb558

fixes review changes.

ae2fd0d

RishabhJain2018 force-pushed the api branch from 9275e03 to ae2fd0d Compare April 18, 2017 17:28

fixes test cases.

e2cd11a

taranjeet reviewed May 7, 2017

View reviewed changes

RishabhJain2018 added 2 commits May 8, 2017 01:01

Review changes fixed.

58ebcea

Fixes review changes.

e24aeef

taranjeet reviewed May 11, 2017

View reviewed changes

Fix review changes.

ebcb2b2

Add test cases.

1f1ecfb

RishabhJain2018 force-pushed the api branch from 0ee6cf6 to 1f1ecfb Compare June 3, 2017 04:14

fix flake8 changes.

2c8d60d

Merge branch 'master' into api

e4dcd32

deshraj approved these changes Jun 3, 2017

View reviewed changes

deshraj merged commit 01581d8 into Cloud-CV:master Jun 3, 2017

RishabhJain2018 deleted the api branch July 13, 2017 19:34

deshraj pushed a commit that referenced this pull request Oct 24, 2018

Contact: Modify API to fetch name and email for logged in users (#922)

1526f10

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Contact Us: Modifies api to fetch name and email for logged in users. #922

Contact Us: Modifies api to fetch name and email for logged in users. #922

RishabhJain2018 commented Apr 9, 2017

taranjeet Apr 10, 2017

RishabhJain2018 Apr 10, 2017

deshraj Apr 10, 2017

deshraj Apr 10, 2017

RishabhJain2018 commented Apr 18, 2017

taranjeet May 7, 2017

taranjeet May 7, 2017

taranjeet May 7, 2017

RishabhJain2018 May 7, 2017

deshraj May 7, 2017

taranjeet May 8, 2017

RishabhJain2018 May 9, 2017

deshraj May 10, 2017

deshraj May 10, 2017

RishabhJain2018 May 11, 2017

taranjeet May 11, 2017

RishabhJain2018 May 21, 2017

taranjeet May 24, 2017

deshraj commented May 26, 2017

RishabhJain2018 commented May 30, 2017

taranjeet commented May 31, 2017

RishabhJain2018 commented Jun 3, 2017

deshraj commented Jun 3, 2017

Contact Us: Modifies api to fetch name and email for logged in users. #922

Contact Us: Modifies api to fetch name and email for logged in users. #922

Conversation

RishabhJain2018 commented Apr 9, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RishabhJain2018 commented Apr 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deshraj commented May 26, 2017

RishabhJain2018 commented May 30, 2017

taranjeet commented May 31, 2017

RishabhJain2018 commented Jun 3, 2017

deshraj commented Jun 3, 2017