-
Notifications
You must be signed in to change notification settings - Fork 6
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
Fix default value for sample_file_columns (#203). #204
Conversation
(I'll fix up the tests once I see the export work on our deployment) |
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.
Thanks for fixing these. To fix the tests you just need to add some value for EXPORT_URL_GCS_BUCKET
in test_export_url_controller.py
.
I'm happy to merge this now, these are both needed fixes and you can follow up with any additional PRs if necessary.
@@ -129,11 +129,12 @@ def _get_entities_dict(cohort_name, query, filter_arr): | |||
} | |||
}) | |||
|
|||
sample_id = current_app.config['SAMPLE_ID_COLUMN'] |
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.
nit: sample_id_column
sample_items = [] | ||
for doc in _get_doc_generator(filter_arr): | ||
sample_items.extend([{ | ||
'entityType': 'sample', | ||
'entityName': s['sample_id'] | ||
'entityName': s[sample_id] |
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.
Oops :)
@@ -237,6 +237,7 @@ def _process_facets(es): | |||
# Elasticsearch field type, optional UI facet description and Elasticsearch | |||
# facet. | |||
app.app.config['FACET_INFO'] = facets | |||
app.app.config['EXTRA_FACET_INFO'] = {} |
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 think it makes sense to have a proper default value here, but I actually would've thought this would be handled correctly here. Where are you seeing the key not found error thrown from?
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 think this is the line that was throwing (I didn't save the stack trace, but it was in the export controller and that's the first place where the key is accessed). I was hitting the error by:
- Opening the explorer
- Checking a few boxes in the pre-curated search facets
- Clicking the export button
If it matters, I didn't have a bigquery.json
in my explorer config at the time. I've noticed that some of the configuration code paths only get hit when it's present on deploy.
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.
Ah the missing biquery.json
was probably it, thanks for the explanation!
7eacdf6
to
1ef552f
Compare
1ef552f
to
3c155d6
Compare
* Change default value for sample_file_columns to a dictionary. * Read sample ID column from config on export. * Make sure EXTRA_FACET_INFO is initialized.
* update to get all * add scroll styling to card lists * update test for larger data sample * note for scaling > 1000 Will need further fleshed out UI design * switch to 500 <-- 600 * cleanup styling * remove unnecessary css file * Fix default values in API layer. (#204) * Change default value for sample_file_columns to a dictionary. * Read sample ID column from config on export. * Make sure EXTRA_FACET_INFO is initialized. * setup test
Fixes #203 ... eventually (another error popped up, will push again to this branch once I've got things working).