Skip to content
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

Merged
merged 40 commits into from
Feb 12, 2018
Merged
Show file tree
Hide file tree
Changes from 32 commits
Commits
Show all changes
40 commits
Select commit Hold shift + click to select a range
5f06e02
Update workflow_manager httpReponse to jsonReponse.
jkmarx Jan 24, 2018
ebda65d
Add allowed_hosts to dev setting.
jkmarx Jan 24, 2018
a3daf10
Update comment.
jkmarx Jan 24, 2018
39e198e
Remove conflicting settings, model can only have one of add, add_on, …
jkmarx Jan 24, 2018
c84fb24
Update analysis_manager httpReponse to jsonResponse.
jkmarx Jan 24, 2018
b68e726
Add migrations.
jkmarx Jan 24, 2018
d6812a4
Update module_name with model name.
jkmarx Jan 24, 2018
4d289fe
Refactor.
jkmarx Jan 24, 2018
4464b77
Upgrade some dependencies for django 1.8
jkmarx Jan 25, 2018
83a6f91
Revert to minimum dependency upgrade to resolve migration issue.
jkmarx Jan 31, 2018
15f1d98
Update annotation server with jsonResponse.
jkmarx Jan 31, 2018
90ae2da
Update core and data_set_manager apps.
jkmarx Jan 31, 2018
ecd9849
Fix typo.
jkmarx Jan 31, 2018
5528d8f
Update requirements to fix migration issue.
jkmarx Jan 31, 2018
26ccf36
Try release which seperates dependency on User module.
jkmarx Jan 31, 2018
46ba4db
Check to see if chunked-upload is the problem.
jkmarx Jan 31, 2018
7a854bb
Fix bug related to abstract model causing migration to fail for library.
jkmarx Jan 31, 2018
721dfe6
Remove duplicate setting.
jkmarx Jan 31, 2018
740531e
Track migration until library updates
jkmarx Jan 31, 2018
5f79419
Reorder installed apps per redux docs.
jkmarx Jan 31, 2018
5d42882
Update activate_user with parser.
jkmarx Feb 1, 2018
ff912e4
Fix pep8 in migration.
jkmarx Feb 1, 2018
e11c1f8
Remove unit test due to inaccurate scope.
jkmarx Feb 2, 2018
bee52c4
Add mocks to unit test.
jkmarx Feb 5, 2018
9ff9032
Remove unneccessary deletion.
jkmarx Feb 5, 2018
3e1c4eb
Fix selium unit test errors.
jkmarx Feb 5, 2018
2f4316f
Remove accidently added json
jkmarx Feb 6, 2018
8166553
Jkmarx/registration redux upgrade (#2562)
jkmarx Feb 6, 2018
d1e00b1
Refactor, return_value to inline.
jkmarx Feb 6, 2018
39ba7e9
Fix unit test.
jkmarx Feb 6, 2018
404345c
Fix mistake using jsonresponse with a str.
jkmarx Feb 6, 2018
5e67bd2
Remove typo.
jkmarx Feb 6, 2018
428839c
Remove unneeded field.
jkmarx Feb 7, 2018
7a5fc77
Chunked_upload migration.
jkmarx Feb 7, 2018
6eeeb75
Opt for safer option for jsonResponse.
jkmarx Feb 7, 2018
f712ca4
Fix syntax.
jkmarx Feb 7, 2018
6801aa0
Fix pep8.
jkmarx Feb 7, 2018
ca22e4c
Swap deprecated method out. (#2566)
jkmarx Feb 9, 2018
2f91446
Remove unnessary flake8 ignore.
jkmarx Feb 12, 2018
d8e90cd
Add json.load exception.
jkmarx Feb 12, 2018
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
12 changes: 0 additions & 12 deletions refinery/analysis_manager/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -176,18 +176,6 @@ def test_fetch_objects_required_for_analyses_bad_user(self):
)
self.assertIn("Couldn't fetch User", context.exception.message)

def test_fetch_objects_required_for_analyses_bad_dataset(self):
Copy link
Member Author

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

self.dataset.delete()
with self.assertRaises(RuntimeError) as context:
fetch_objects_required_for_analysis(
{
"study_uuid": self.study.uuid,
"user_id": self.user.id,
"workflow_uuid": self.workflow.uuid
}
)
self.assertIn("Couldn't fetch DataSet", context.exception.message)


class AnalysisViewsTests(AnalysisManagerTestBase, ToolManagerTestBase):
"""
Expand Down
18 changes: 7 additions & 11 deletions refinery/analysis_manager/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down Expand Up @@ -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')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

JsonResponse docs say:

Its default Content-Type header is set to application/json

Would the default not work here?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's application/javascript

Copy link
Member

Choose a reason for hiding this comment

The 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},
Expand Down Expand Up @@ -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')
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ditto?

return HttpResponse(status=400)


Expand All @@ -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())
Copy link
Member

Choose a reason for hiding this comment

The 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):
Expand Down
22 changes: 11 additions & 11 deletions refinery/annotation_server/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@

from django.db import connection
from django.db.models import Q
from django.http import HttpResponse
from django.http import HttpResponse, JsonResponse

from .utils import GAP_REGIONS, MAPPABILITY_THEORETICAL, SUPPORTED_GENOMES

Expand All @@ -27,7 +27,7 @@ def search_genes(request, genome, search_string):
'cdsEnd', 'exonCount', 'exonStarts', 'exonEnds')

data = ValuesQuerySetToDict(curr_vals)
return HttpResponse(data, 'application/json')
return JsonResponse(data)
return HttpResponse(status=400)


Expand Down Expand Up @@ -64,7 +64,7 @@ def get_length(request, genome):
current_table = globals()[genome + "_ChromInfo"]
data = ValuesQuerySetToDict(
current_table.objects.values('chrom', 'size'))
return HttpResponse(data, 'application/json')
return JsonResponse(data)
return HttpResponse(status=400)


Expand All @@ -78,7 +78,7 @@ def get_chrom_length(request, genome, chrom):
curr_vals = current_table.objects.filter(chrom__iexact=chrom).values(
'chrom', 'size')
data = ValuesQuerySetToDict(curr_vals)
return HttpResponse(data, 'application/json')
return JsonResponse(data)
return HttpResponse(status=400)

# TODO: return genome lengths according to chrom order i.e. 1,2,3 etc.
Expand All @@ -94,7 +94,7 @@ def get_cytoband(request, genome, chrom):
curr_vals = current_table.objects.filter(chrom__iexact=chrom).values(
'chrom', 'chromStart', 'chromEnd', 'name', 'gieStain')
data = ValuesQuerySetToDict(curr_vals)
return HttpResponse(data, 'application/json')
return JsonResponse(data)
return HttpResponse(status=400)


Expand All @@ -111,7 +111,7 @@ def get_genes(request, genome, chrom, start, end):
).values('name', 'chrom', 'strand', 'txStart', 'txEnd', 'cdsStart',
'cdsEnd', 'exonCount', 'exonStarts', 'exonEnds')
data = ValuesQuerySetToDict(curr_vals)
return HttpResponse(data, 'application/json')
return JsonResponse(data)
return HttpResponse(status=400)


Expand All @@ -127,7 +127,7 @@ def get_gc(request, genome, chrom, start, end):
Q(position__range=(start, end)),
).values('chrom', 'position', 'value')
data = ValuesQuerySetToDict(curr_vals)
return HttpResponse(data, 'application/json')
return JsonResponse(data)
return HttpResponse(status=400)


Expand All @@ -145,7 +145,7 @@ def get_maptheo(request, genome, chrom, start, end):
Q(chromStart__range=(start, end)) | Q(chromEnd__range=(start, end))
).values('chrom', 'chromStart', 'chromEnd')
data = ValuesQuerySetToDict(curr_vals)
return HttpResponse(data, 'application/json')
return JsonResponse(data)
return HttpResponse(status=400)


Expand All @@ -162,7 +162,7 @@ def get_mapemp(request, genome, chrom, start, end):
Q(chromStart__range=(start, end)) | Q(chromEnd__range=(start, end))
).values('chrom', 'chromStart', 'chromEnd')
data = ValuesQuerySetToDict(curr_vals)
return HttpResponse(data, 'application/json')
return JsonResponse(data)
return HttpResponse(status=400)


Expand All @@ -178,7 +178,7 @@ def get_conservation(request, genome, chrom, start, end):
Q(position__range=(start, end)),
).values('chrom', 'position', 'value')
data = ValuesQuerySetToDict(curr_vals)
return HttpResponse(data, 'application/json')
return JsonResponse(data)
return HttpResponse(status=400)


Expand All @@ -197,7 +197,7 @@ def get_gapregion(request, genome, chrom, start, end):
).values('bin', 'chromStart', 'chromEnd', 'ix', 'n', 'size', 'type',
'bridge')
data = ValuesQuerySetToDict(curr_vals)
return HttpResponse(data, 'application/json')
return JsonResponse(data)
return HttpResponse(status=400)


Expand Down
13 changes: 10 additions & 3 deletions refinery/config/settings/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -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',
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

registration doc recommendations

Copy link
Member

Choose a reason for hiding this comment

The 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:
Expand Down Expand Up @@ -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
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

required both for dev and prod, so moved to base

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think # NOQA: F405 is necessary here (http://flake8.pycqa.org/en/3.1.1/user/error-codes.html)


MIGRATION_MODULES = {
'chunked_upload': 'dependency_migrations.chunked_upload'
Copy link
Member Author

Choose a reason for hiding this comment

The 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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

}
2 changes: 1 addition & 1 deletion refinery/config/settings/dev.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,6 @@

EMAIL_BACKEND = "django.core.mail.backends.console.EmailBackend"

INSTALLED_APPS += (
INSTALLED_APPS += ( # NOQA: F405
Copy link
Member

Choose a reason for hiding this comment

The 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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes. I edited it before I refactored into base.

'django_extensions',
)
3 changes: 0 additions & 3 deletions refinery/config/settings/prod.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,4 @@
DEBUG = False
TEMPLATE_DEBUG = DEBUG

# Required when DEBUG = False
ALLOWED_HOSTS = get_setting("ALLOWED_HOSTS") # NOQA: F405

EMAIL_BACKEND = "django.core.mail.backends.smtp.EmailBackend"
7 changes: 5 additions & 2 deletions refinery/core/management/commands/activate_user.py
Original file line number Diff line number Diff line change
Expand Up @@ -9,8 +9,11 @@
class Command(BaseCommand):
help = "Activate user account"

def handle(self, username, **options):
def add_arguments(self, parser):
Copy link
Member Author

Choose a reason for hiding this comment

The 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)
1 change: 1 addition & 0 deletions refinery/core/migrations/0001_initial.py
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ class Migration(migrations.Migration):
('data_set_manager', '0001_initial'),
('auth', '0001_initial'),
('file_store', '0001_initial'),
('registration', '0001_initial')
Copy link
Member

Choose a reason for hiding this comment

The 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

Copy link
Member Author

Choose a reason for hiding this comment

The 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 = [
Expand Down
22 changes: 15 additions & 7 deletions refinery/core/tests.py
Original file line number Diff line number Diff line change
Expand Up @@ -651,13 +651,19 @@ def setUp(self):
)
self.analysis.set_owner(self.user)

def test_verify_dataset_deletion_if_no_analysis_run_upon_it(self):
@mock.patch('core.models.DataSet.get_investigation_links', return_value=[])
def test_verify_dataset_deletion_if_no_analysis_run_upon_it(
self, mock_get_links
):
self.assertIsNotNone(
DataSet.objects.get(name="dataset_without_analysis"))
DataSet.objects.get(name="dataset_without_analysis")
)
self.dataset_without_analysis.delete()
self.assertRaises(DataSet.DoesNotExist,
DataSet.objects.get,
name="dataset_without_analysis")
self.assertRaises(
DataSet.DoesNotExist,
DataSet.objects.get,
name="dataset_without_analysis"
)

def test_verify_dataset_deletion_if_analysis_run_upon_it(self):
self.assertIsNotNone(
Expand All @@ -667,12 +673,14 @@ def test_verify_dataset_deletion_if_analysis_run_upon_it(self):
DataSet.objects.get,
name="dataset_with_analysis")

def test_isa_archive_deletion(self):
@mock.patch('core.models.DataSet.get_investigation_links', return_value=[])
def test_isa_archive_deletion(self, mock_get_links):
self.assertIsNotNone(self.dataset_without_analysis.get_isa_archive())
self.dataset_without_analysis.delete()
self.assertIsNone(self.dataset_without_analysis.get_isa_archive())

def test_pre_isa_archive_deletion(self):
@mock.patch('core.models.DataSet.get_investigation_links', return_value=[])
def test_pre_isa_archive_deletion(self, mock_get_links):
self.assertIsNotNone(
self.dataset_without_analysis.get_pre_isa_archive())
self.dataset_without_analysis.delete()
Expand Down
37 changes: 16 additions & 21 deletions refinery/core/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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):
Expand All @@ -575,10 +574,10 @@ def solr_select(request, core):
full_response.raise_for_status()
except HTTPError as e:
logger.error(e)
response = json.dumps({})
return JsonResponse({})
else:
response = full_response.content
return HttpResponse(response, content_type='application/json')
return HttpResponse(full_response.content,
content_type='application/json')


def solr_igv(request):
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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):
Expand Down Expand Up @@ -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
Expand All @@ -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()
Expand All @@ -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.
Expand Down
19 changes: 19 additions & 0 deletions refinery/data_set_manager/migrations/0006_auto_20180124_1042.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# -*- coding: utf-8 -*-
from __future__ import unicode_literals

from django.db import migrations, models


class Migration(migrations.Migration):

dependencies = [
('data_set_manager', '0005_update_attribute_orders'),
]

operations = [
migrations.AlterField(
model_name='contact',
name='email',
field=models.EmailField(max_length=254, null=True, blank=True),
),
]