Change warning message when users try to export private silo as JSON - #540
Merged
Conversation
jefmoura
reviewed
Jul 18, 2018
jefmoura
left a comment
Member
There was a problem hiding this comment.
- Why did you decide to use the DRF token?
- I don't think it's a good idea to use it because the token doesn't expire and if somebody gets the user's token, the can access some other endpoints. It's better to add the Django OAuth Toolkit to Track and use the Bearer token.
Author
|
I thought that if users use API call then that means they are making request from code (If they are not using postman or similar tools to get data). So, if they are working with code I think token shouldn't change. |
Member
|
ybekdemir
force-pushed
the
issue-530
branch
4 times, most recently
from
July 19, 2018 15:25
5ba68e6 to
c9483b9
Compare
jefmoura
reviewed
Jul 19, 2018
| silo = Silo.objects.get(pk=id) | ||
| if silo.public == False: | ||
| return HttpResponse("This table is not public. You must use the private API.") | ||
| url = "http://{}/api/silo/{}/data?query={}&shown_cols={}" \ |
Member
There was a problem hiding this comment.
Why don't you use the function reverse to generate the URL?
| from urlparse import urljoin | ||
| from datetime import datetime | ||
|
|
||
| from django.contrib.sites.shortcuts import get_current_site |
| from django.shortcuts import get_object_or_404, redirect | ||
|
|
||
|
|
||
| from rest_framework.authtoken.models import Token |
ybekdemir
force-pushed
the
issue-530
branch
2 times, most recently
from
July 20, 2018 08:21
c443899 to
cdb0756
Compare
jefmoura
reviewed
Jul 20, 2018
| if silo.public == False: | ||
| return HttpResponse("This table is not public. You must use the private API.") | ||
| url = reverse('silos-data', kwargs={'id': silo.pk}) | ||
| params = '?query={}&shown_cols={}'\ |
Member
There was a problem hiding this comment.
Do you know that you can use the function urlencode from the lib urllib with the request.GET as a parameter to create the URL query?
jefmoura
reviewed
Jul 20, 2018
| @@ -106,29 +108,32 @@ def data(self, request, id): | |||
|
|
|||
| silo = Silo.objects.get(pk=id) | |||
| if silo.public == False: | |||
Member
There was a problem hiding this comment.
Could you change this condition as well?
jefmoura
approved these changes
Jul 20, 2018
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
Purpose
Currently, when user tries to export private silo as JSON feed, we are warning to redirect them to API. However, we need more clear message with simple instruction.
Approach
Warning message was changed and user token added to message to inform users about how can they use API.
_Related ticket: #530