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
Jkmarx/update httpresponse #2533
Conversation
@@ -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): |
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
refinery/config/settings/base.py
Outdated
@@ -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 = ( | |||
'django.contrib.sites', |
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.
registration doc recommendations
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.
Maybe comments to distinguish the lines where order matters (and explain why) from those where we don't think that it does?
refinery/config/settings/base.py
Outdated
@@ -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 comment
The 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 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)
refinery/config/settings/base.py
Outdated
ALLOWED_HOSTS = get_setting("ALLOWED_HOSTS") # NOQA: F405 | ||
|
||
MIGRATION_MODULES = { | ||
'chunked_upload': 'dependency_migrations.chunked_upload' |
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -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 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.
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.
Some questions about what's going on, but it may just be confusion on my end.
@@ -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 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?
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.
it's application/javascript
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.
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 comment
The reason will be displayed to describe this comment to others. Learn more.
ditto?
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 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.
refinery/config/settings/base.py
Outdated
@@ -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 = ( | |||
'django.contrib.sites', |
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.
Maybe comments to distinguish the lines where order matters (and explain why) from those where we don't think that it does?
refinery/core/views.py
Outdated
@@ -578,7 +577,7 @@ def solr_select(request, core): | |||
response = json.dumps({}) | |||
else: | |||
response = full_response.content | |||
return HttpResponse(response, content_type='application/json') | |||
return JsonResponse(response) |
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.
response
may be string rather than a dict here... Change the code above?
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.
Good catch.
return HttpResponseRedirect( | ||
reverse(self.success_view_name, args=[dataset_uuid]) | ||
) | ||
else: | ||
error = 'Problem parsing ISA-Tab file' | ||
if request.is_ajax(): | ||
return HttpResponse(json.dumps({'error': error}), | ||
content_type='application/json') | ||
return JsonResponse({'error': error}) |
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.
Would an HTTP error response be appropriate here? (But that's not something you've changed, so perhaps better not to wade into that.)
refinery/workflow_manager/views.py
Outdated
@@ -27,10 +27,10 @@ def import_workflows(request): | |||
logger.debug("Engine: " + engine.name + " - " + | |||
str(new_workflow_count) + ' workflows after.') | |||
workflows += new_workflow_count | |||
return HttpResponse(str(workflows) + ' workflows imported.') | |||
return HttpResponse(str(workflows) + ' workflows imported.', ) |
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.
I only know about trailing commas when you want a 1-element tuple: Not sure what's going on here?
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.
That's a typo. :)
|
||
|
||
def download_workflow(request, workflow_uuid): | ||
"""Returns a specified workflow_id as a dictionary object.""" | ||
workflow = get_object_or_404(Workflow, uuid=workflow_uuid) | ||
return HttpResponse(workflow.graph) | ||
return JsonResponse(workflow.graph) |
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.
Will this return the right thing? In the other examples, you had to remove a json.dumps
.
refinery/workflow_manager/views.py
Outdated
|
||
|
||
def download_workflow(request, workflow_uuid): | ||
"""Returns a specified workflow_id as a dictionary object.""" | ||
workflow = get_object_or_404(Workflow, uuid=workflow_uuid) | ||
return HttpResponse(workflow.graph) |
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.
Do we know that JSON was expected downstream from here?
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.
I was trusting the comment, but will double check.
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.
A unit test would have helped to disambiguate.
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.
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.
Good point. I'm removing a lot of stale code as I implement support for S3 to avoid unnecessary work.
refinery/workflow_manager/views.py
Outdated
@@ -27,10 +27,10 @@ def import_workflows(request): | |||
logger.debug("Engine: " + engine.name + " - " + | |||
str(new_workflow_count) + ' workflows after.') | |||
workflows += new_workflow_count | |||
return HttpResponse(str(workflows) + ' workflows imported.') | |||
return HttpResponse(str(workflows) + ' workflows imported.', ) |
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.
New trailing comma?
# recommended solution to an auth_permission error, though doc says | ||
# we probably won't need to call it since django will call it | ||
# automatically when needed | ||
ContentType.objects.clear_cache() |
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.
Could we get a link to the recommended solution?
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.
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.
It'd be great to add this URL to the comment for future reference.
refinery/core/tests.py
Outdated
def test_pre_isa_archive_deletion(self): | ||
@mock.patch('core.models.DataSet.get_investigation_links') | ||
def test_pre_isa_archive_deletion(self, mock_get_links): | ||
mock_get_links.return_value = [] |
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.
Would the mocked return_value
be an empty list or None
? Also could put the return_value
as a named arg in the @mock.patch
and save a line
refinery/core/tests.py
Outdated
@@ -667,12 +673,16 @@ 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') |
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.
If we have to mock this many times it may be good to think about adding a reference in a common setUp()
and just .start()
the mock in these three tests.
Something like:
def setUp(self):
self.get_investigation_links_mock = mock.patch('core.models.DataSet.get_investigation_links', return_value=None)
def testA(self):
self.get_investigation_links_mock.start()
@@ -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 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
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.
Yes. Perhaps, we can discuss in dev. Feel free to mess around with this branch locally if you want to try anything.
@@ -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 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?
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.
Yes. I edited it before I refactored into base.
@@ -0,0 +1,69 @@ | |||
# -*- coding: utf-8 -*- |
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.
How was this migration created?
Did ./manage.py makemigrations chunked_upload
not work?
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.
Btw, we exclude migrations from flake8: https://github.com/refinery-platform/refinery-platform/pull/2568/files
FWIW while checking out and upgrading my local Django 1.7 instance to this branch I recieved this error during the migration step:
|
* update redux. * Update method params changed from upgrade * Update flockblock params due to upgrade.
Codecov Report
@@ Coverage Diff @@
## jkmarx/django-upgrade-1.8 #2533 +/- ##
=============================================================
+ Coverage 53.16% 53.18% +0.01%
=============================================================
Files 415 413 -2
Lines 28158 27102 -1056
Branches 1240 1240
=============================================================
- Hits 14969 14413 -556
+ Misses 13189 12689 -500
Continue to review full report at Codecov.
|
@scottx611x Yes, you'd already have a migration locally. The previous versions of django: |
refinery/file_store/models.py
Outdated
@@ -240,11 +239,9 @@ class FileStoreItem(models.Model): | |||
import_task_id = UUIDField(auto=False, blank=True) | |||
# Date created | |||
created = models.DateTimeField(auto_now_add=True, | |||
default=timezone.now, | |||
blank=True) |
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.
blank=True
is not needed either:
"As currently implemented, setting auto_now or auto_now_add to True will cause the field to have editable=False and blank=True set"
https://docs.djangoproject.com/en/1.8/ref/models/fields/#datefield
refinery/file_store/models.py
Outdated
blank=True) | ||
# Date updated | ||
updated = models.DateTimeField(auto_now=True, | ||
default=timezone.now, | ||
blank=True) |
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.
blank=True
is not needed either:
"As currently implemented, setting auto_now or auto_now_add to True will cause the field to have editable=False and blank=True set"
https://docs.djangoproject.com/en/1.8/ref/models/fields/#datefield
@jkmarx Okay, I was more so worried about how this will work on existing instances that we'll need to upgrade. If I understand correctly, we could only use this code if we created a fresh vm? We'd see this error while bringing up an AWS stack as well since the snapshots we have all encapsulate these faked migrations. Probably good to think about for the overall 1.8 upgrade, but probably not crucial for this PR. |
refinery/core/views.py
Outdated
@@ -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 comment
The 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 comment
The 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 comment
The 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 comment
The reason will be displayed to describe this comment to others. Learn more.
- Fix JSON decode error. We need to retain this method until we update data set search feature
* Swap deprecated method out. * Remove solr_igv dependencies. (#2570) * Remove solr_igv dependencies. * Remove visualization_manager. * Upgrade django storage. (#2571) * Upgrade django storage. * Remove post-css library, unused. (#2574) * Update user/file facet/columns per user request. (#2577) * Remove post-css library, unused. * Update user/file facet/columns per user request. * Remove typo , * Fix typo, double quotes. * Remove space. * Add cell_type to facets. * Fix unit test. * Add missing facets. * Fix cypress test. * Revert to 1.6.0 due to django 1.9 support.
* Upgrade django version to 1.8 * Update workflow_manager httpReponse to jsonReponse. * Add allowed_hosts to dev setting. * Update comment. * Remove conflicting settings, model can only have one of add, add_on, or default. * Update analysis_manager httpReponse to jsonResponse. * Add migrations. * Update module_name with model name. * Refactor. * Upgrade some dependencies for django 1.8 * Revert to minimum dependency upgrade to resolve migration issue. * Update annotation server with jsonResponse. * Update core and data_set_manager apps. * Fix typo. * Update requirements to fix migration issue. * Try release which seperates dependency on User module. * Check to see if chunked-upload is the problem. * Fix bug related to abstract model causing migration to fail for library. * Remove duplicate setting. * Track migration until library updates * Reorder installed apps per redux docs. * Update activate_user with parser. * Fix pep8 in migration. * Remove unit test due to inaccurate scope. * Add mocks to unit test. * Remove unneccessary deletion. * Fix selium unit test errors. * Remove accidently added json * Jkmarx/registration redux upgrade (#2562) * update redux. * Update method params changed from upgrade * Update flockblock params due to upgrade. * Refactor, return_value to inline. * Fix unit test. * Fix mistake using jsonresponse with a str. * Remove typo. * Remove unneeded field. * Chunked_upload migration. * Opt for safer option for jsonResponse. * Fix syntax. * Fix pep8. * Swap deprecated method out. (#2566) * Swap deprecated method out. * Remove solr_igv dependencies. (#2570) * Remove solr_igv dependencies. * Remove visualization_manager. * Upgrade django storage. (#2571) * Upgrade django storage. * Remove post-css library, unused. (#2574) * Update user/file facet/columns per user request. (#2577) * Remove post-css library, unused. * Update user/file facet/columns per user request. * Remove typo , * Fix typo, double quotes. * Remove space. * Add cell_type to facets. * Fix unit test. * Add missing facets. * Fix cypress test. * Revert to 1.6.0 due to django 1.9 support. * Remove unnessary flake8 ignore. * Add json.load exception. * Jkmarx/update httpresponse (#2533) * Update workflow_manager httpReponse to jsonReponse. * Add allowed_hosts to dev setting. * Update comment. * Remove conflicting settings, model can only have one of add, add_on, or default. * Update analysis_manager httpReponse to jsonResponse. * Add migrations. * Update module_name with model name. * Refactor. * Upgrade some dependencies for django 1.8 * Revert to minimum dependency upgrade to resolve migration issue. * Update annotation server with jsonResponse. * Update core and data_set_manager apps. * Fix typo. * Update requirements to fix migration issue. * Try release which seperates dependency on User module. * Check to see if chunked-upload is the problem. * Fix bug related to abstract model causing migration to fail for library. * Remove duplicate setting. * Track migration until library updates * Reorder installed apps per redux docs. * Update activate_user with parser. * Fix pep8 in migration. * Remove unit test due to inaccurate scope. * Add mocks to unit test. * Remove unneccessary deletion. * Fix selium unit test errors. * Remove accidently added json * Jkmarx/registration redux upgrade (#2562) * update redux. * Update method params changed from upgrade * Update flockblock params due to upgrade. * Refactor, return_value to inline. * Fix unit test. * Fix mistake using jsonresponse with a str. * Remove typo. * Remove unneeded field. * Chunked_upload migration. * Opt for safer option for jsonResponse. * Fix syntax. * Fix pep8. * Swap deprecated method out. (#2566) * Swap deprecated method out. * Remove solr_igv dependencies. (#2570) * Remove solr_igv dependencies. * Remove visualization_manager. * Upgrade django storage. (#2571) * Upgrade django storage. * Remove post-css library, unused. (#2574) * Update user/file facet/columns per user request. (#2577) * Remove post-css library, unused. * Update user/file facet/columns per user request. * Remove typo , * Fix typo, double quotes. * Remove space. * Add cell_type to facets. * Fix unit test. * Add missing facets. * Fix cypress test. * Revert to 1.6.0 due to django 1.9 support. * Remove unnessary flake8 ignore. * Add json.load exception. * Jkmarx/remove unused m commands (#2583) * Remove unused management commands. * Remove unused method. * Jkmarx/outparse to argparse (#2584) * Update make_options to add_arguments for optparse/argparse upgrade. * Refactor for easier read. * Jkmarx update deprecated 1 8 items (#2588) * Update config with the new 1.8 setting. * Add comment. * Remove unused template. * Update template related settings. * Update get_template per docs, Context&RequestContext unneeded. * Fix typo in context_processors location update. * Remove duplicate. * Fix added setting. * Add `--fake-initial` to our migrate commands to provide the behavior that Django 1.7 gave by default (#2585) * Update management command, argparse. * Scottx611x/fix management commands (#2606) * Update `set_uo_site_name` to use add_arguments() * Add test coverage * Add test for `add_users_to_public_group` * Add test for user activation * Add test for `import_annotations` * Move for consistency. * Jkmarx/update management command argparse (#2607) * Update management command, argparse. * Move for consistency. * Add tests for `create_workflow_engine` * Add tests for `create_galaxy_instance` * Fix bug related to non-obj.
Ref #2537
Ref #449