-
Notifications
You must be signed in to change notification settings - Fork 24
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
Jkmarx/update httpresponse #2533
Changes from 37 commits
5f06e02
ebda65d
a3daf10
39e198e
c84fb24
b68e726
d6812a4
4d289fe
4464b77
83a6f91
15f1d98
90ae2da
ecd9849
5528d8f
26ccf36
46ba4db
7a854bb
721dfe6
740531e
5f79419
5d42882
ff912e4
e11c1f8
bee52c4
9ff9032
3e1c4eb
2f4316f
8166553
d1e00b1
39ba7e9
404345c
5e67bd2
428839c
7a5fc77
6eeeb75
f712ca4
6801aa0
ca22e4c
2f91446
d8e90cd
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 |
---|---|---|
|
@@ -4,11 +4,10 @@ | |
|
||
from django.contrib import messages | ||
from django.contrib.auth.decorators import login_required | ||
from django.core import serializers | ||
from django.core.urlresolvers import reverse | ||
from django.http import (HttpResponse, HttpResponseBadRequest, | ||
HttpResponseForbidden, HttpResponseNotAllowed, | ||
HttpResponseServerError) | ||
HttpResponseServerError, JsonResponse) | ||
from django.shortcuts import render_to_response | ||
from django.template import RequestContext | ||
|
||
|
@@ -75,8 +74,7 @@ def analysis_status(request, uuid): | |
} | ||
logger.debug("Analysis status for '%s': %s", | ||
analysis.name, json.dumps(ret_json)) | ||
return HttpResponse(json.dumps(ret_json, indent=4), | ||
content_type='application/javascript') | ||
return JsonResponse(ret_json, content_type='application/javascript') | ||
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.
Would the default not work here? 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. it's application/javascript 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. |
||
return render_to_response( | ||
'analysis_manager/analysis_status.html', | ||
{'uuid': uuid, 'status': status, 'analysis': analysis}, | ||
|
@@ -133,10 +131,8 @@ def update_workflows(request): | |
workflows += new_workflow_count | ||
# getting updated available workflows | ||
workflows = Workflow.objects.all() | ||
json_serializer = serializers.get_serializer("json")() | ||
return HttpResponse( | ||
json_serializer.serialize(workflows, ensure_ascii=False), | ||
content_type='application/javascript') | ||
|
||
return JsonResponse(workflows, content_type='application/javascript') | ||
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. ditto? |
||
return HttpResponse(status=400) | ||
|
||
|
||
|
@@ -145,11 +141,11 @@ def get_workflow_data_input_map(request, workflow_uuid): | |
workflow_uuid | ||
""" | ||
curr_workflow = Workflow.objects.filter(uuid=workflow_uuid)[0] | ||
data = serializers.serialize('json', curr_workflow.data_inputs.all()) | ||
if request.is_ajax(): | ||
return HttpResponse(data, content_type='application/javascript') | ||
return JsonResponse(curr_workflow.data_inputs.all(), | ||
content_type='application/javascript') | ||
else: | ||
return HttpResponse(data, content_type='application/json') | ||
return JsonResponse(curr_workflow.data_inputs.all()) | ||
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. Oh! It does make a difference here? I didn't realize this mattered. |
||
|
||
|
||
def run(request): | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -178,14 +178,14 @@ def get_setting(name, settings=local_settings, default=None): | |
|
||
# NOTE: the order of INSTALLED_APPS matters in some instances. | ||
INSTALLED_APPS = ( | ||
'registration', | ||
'django.contrib.sites', | ||
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. registration doc recommendations 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. Maybe comments to distinguish the lines where order matters (and explain why) from those where we don't think that it does? |
||
'registration', # docs: should be immediately above 'django.contrib.auth' | ||
'django.contrib.auth', | ||
'core', | ||
'data_set_manager', | ||
'guardian', | ||
'django.contrib.contenttypes', | ||
'django.contrib.auth', | ||
'django.contrib.sessions', | ||
'django.contrib.sites', | ||
'django.contrib.messages', | ||
'django.contrib.staticfiles', | ||
# Uncomment the next line to enable the admin: | ||
|
@@ -615,3 +615,10 @@ def get_setting(name, settings=local_settings, default=None): | |
|
||
# temporary feature toggle for using S3 as user data file storage backend | ||
REFINERY_S3_USER_DATA = get_setting('REFINERY_S3_USER_DATA', default=False) | ||
|
||
# ALLOWED_HOSTS required in 1.8.16 to prevent a DNS rebinding attack. | ||
ALLOWED_HOSTS = get_setting("ALLOWED_HOSTS") # NOQA: F405 | ||
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. required both for dev and prod, so moved to base 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. I don't think |
||
|
||
MIGRATION_MODULES = { | ||
'chunked_upload': 'dependency_migrations.chunked_upload' | ||
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. 3rd party app has not added their own migration, so we needed to build and include one. Author said they are getting around to it soon. 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. |
||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,6 @@ | |
|
||
EMAIL_BACKEND = "django.core.mail.backends.console.EmailBackend" | ||
|
||
INSTALLED_APPS += ( | ||
INSTALLED_APPS += ( # NOQA: F405 | ||
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. This change must be due to the fact that this file hasn't been edited in ages? 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. Yes. I edited it before I refactored into base. |
||
'django_extensions', | ||
) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -9,8 +9,11 @@ | |
class Command(BaseCommand): | ||
help = "Activate user account" | ||
|
||
def handle(self, username, **options): | ||
def add_arguments(self, parser): | ||
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. changes from outparse argparse change. Writing separate issue to deal with other base commands, but changed this one for unit tests to pass. |
||
parser.add_argument('username') | ||
|
||
def handle(self, *args, **options): | ||
"""Activate a user account. | ||
""" | ||
|
||
username = options['username'] | ||
User.objects.filter(username__exact=username).update(is_active=True) |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -15,6 +15,7 @@ class Migration(migrations.Migration): | |
('data_set_manager', '0001_initial'), | ||
('auth', '0001_initial'), | ||
('file_store', '0001_initial'), | ||
('registration', '0001_initial') | ||
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. I haven't had much time to think about this yet, but it still seems scary 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. Yes. Perhaps, we can discuss in dev. Feel free to mess around with this branch locally if you want to try anything. |
||
] | ||
|
||
operations = [ | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -14,7 +14,8 @@ | |
from django.core.urlresolvers import reverse | ||
from django.http import (HttpResponse, HttpResponseBadRequest, | ||
HttpResponseForbidden, HttpResponseNotFound, | ||
HttpResponseRedirect, HttpResponseServerError) | ||
HttpResponseRedirect, HttpResponseServerError, | ||
JsonResponse) | ||
from django.shortcuts import get_object_or_404, redirect, render_to_response | ||
from django.template import RequestContext, loader | ||
from django.views.decorators.gzip import gzip_page | ||
|
@@ -552,9 +553,7 @@ def solr_core_search(request): | |
if annotations: | ||
response['response']['annotations'] = annotation_data | ||
|
||
response = json.dumps(response) | ||
|
||
return HttpResponse(response, content_type='application/json') | ||
return JsonResponse(response) | ||
|
||
|
||
def solr_select(request, core): | ||
|
@@ -564,6 +563,7 @@ def solr_select(request, core): | |
data = request.GET.urlencode() | ||
try: | ||
full_response = requests.get(url, params=data) | ||
response_dict = json.loads(full_response.content) | ||
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. Missing error handling for JSON decode errors. 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. in further inquiry, "is this code even used anymore?" I'll write an issue up. 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. Ref #2567 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.
|
||
# FIXME: | ||
# Solr sends back an additional 400 here in the data_sets 1 filebrowser | ||
# when there is only one row defined in the metadata since | ||
|
@@ -573,12 +573,11 @@ def solr_select(request, core): | |
if ("Pivot Facet needs at least one field name" | ||
not in full_response.content): | ||
full_response.raise_for_status() | ||
except HTTPError as e: | ||
except (HTTPError, ValueError) as e: | ||
logger.error(e) | ||
response = json.dumps({}) | ||
return JsonResponse({}) | ||
else: | ||
response = full_response.content | ||
return HttpResponse(response, content_type='application/json') | ||
return JsonResponse(response_dict) | ||
|
||
|
||
def solr_igv(request): | ||
|
@@ -634,8 +633,7 @@ def solr_igv(request): | |
logger.debug("session_urls") | ||
logger.debug(json.dumps(session_urls, indent=4)) | ||
|
||
return HttpResponse(json.dumps(session_urls), | ||
content_type='application/json') | ||
return JsonResponse(session_urls) | ||
|
||
|
||
def samples_solr(request, ds_uuid, study_uuid, assay_uuid): | ||
|
@@ -705,10 +703,7 @@ def pubmed_abstract(request, id): | |
except ExpatError: | ||
return HttpResponse('Service currently unavailable', status=503) | ||
|
||
return HttpResponse( | ||
json.dumps(response_dict), | ||
content_type='application/json' | ||
) | ||
return JsonResponse(response_dict) | ||
|
||
|
||
def pubmed_search(request, term): | ||
|
@@ -931,7 +926,7 @@ def delete(self, request, uuid): | |
|
||
class CustomRegistrationView(RegistrationView): | ||
|
||
def register(self, request, **cleaned_data): | ||
def register(self, request): | ||
""" | ||
Given a username, email address, password, first name, last name, | ||
and affiliation, register a new user account, which will initially | ||
|
@@ -956,12 +951,12 @@ def register(self, request, **cleaned_data): | |
class of this backend as the sender. | ||
|
||
""" | ||
username = cleaned_data['username'] | ||
email = cleaned_data['email'] | ||
password = cleaned_data['password1'] | ||
first_name = cleaned_data['first_name'] | ||
last_name = cleaned_data['last_name'] | ||
affiliation = cleaned_data['affiliation'] | ||
username = request.cleaned_data['username'] | ||
email = request.cleaned_data['email'] | ||
password = request.cleaned_data['password1'] | ||
first_name = request.cleaned_data['first_name'] | ||
last_name = request.cleaned_data['last_name'] | ||
affiliation = request.cleaned_data['affiliation'] | ||
|
||
if Site._meta.installed: | ||
site = Site.objects.get_current() | ||
|
@@ -979,7 +974,7 @@ class of this backend as the sender. | |
request=request) | ||
return new_user | ||
|
||
def get_success_url(self, request, user): | ||
def get_success_url(self, user): | ||
""" | ||
Return the name of the URL to redirect to after successful | ||
user registration. | ||
|
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.
discussed w/ Scott about removing this unit test since it tests for an error throw outside this function