-
Notifications
You must be signed in to change notification settings - Fork 80
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
Let's get redbiom-ing #2118
Let's get redbiom-ing #2118
Conversation
I think this is ready for review. Sorry for the amount of changes but as you can imagine, had to do a bunch of tests/changes based on these. Note that this goes to it's own branch so it should be fine to merge once is reviewed and changes are approved. At this point, we are using redbiom to only get the sample_ids that match a giving query (metadata or sequence), to then use the database to find the artifacts (based on the preparations) and their information where those samples exist. The ToDo list is as follows, this list will be moved to the PR against master once this one is merged:
|
qiita_pet/handlers/qiita_redbiom.py
Outdated
@coroutine | ||
@execute_as_transaction | ||
def get(self, search): | ||
contexts = yield Task(self._get_context) |
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.
just curious, how annoying was it to deal with these Task
s?
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.
Once you get the drift, pretty easy.
qiita_pet/handlers/qiita_redbiom.py
Outdated
callback(([], 'The given context is not valid: %s - %s' % ( | ||
context, contexts))) | ||
else: | ||
if search_on == 'metadata': |
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.
we may want an additional elif
here for searching over categories
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.
yup, let's chat this afternoon
qiita_pet/handlers/qiita_redbiom.py
Outdated
context, contexts))) | ||
else: | ||
if search_on == 'metadata': | ||
samples = redbiom.search.metadata_full(query, categories=None) |
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.
categories
is defined as a bool
, can this be set to categories=False
?
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.
k
qiita_pet/handlers/qiita_redbiom.py
Outdated
if search_on == 'metadata': | ||
samples = redbiom.search.metadata_full(query, categories=None) | ||
elif search_on == 'observations': | ||
# from_or_nargs first parameter is the file handler so it uses |
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 understand this comment. from_or_nargs
handles the case of passing in a list of things via a file or from the command line
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 one of the previous versions I was using from_or_nargs but then replaced with current code and forgot to remove that comment, removing.
qiita_pet/handlers/qiita_redbiom.py
Outdated
# the values from query | ||
samples = [s.split('_', 1)[1] | ||
for s in redbiom.util.samples_from_observations( | ||
query.split(' '), True, context)] |
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.
the True
here may not be always desirable but that can be deferred
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.
yup, also within our discussion today.
qiita_pet/handlers/qiita_redbiom.py
Outdated
# from_or_nargs first parameter is the file handler so it uses | ||
# that as the query input. None basically will force to take | ||
# the values from query | ||
samples = [s.split('_', 1)[1] |
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.
What this is doing is converting from a redbiom ID to an ambiguous ID, and it does not seem to be enforcing that the ID is unique through a set
comprehension. You may want to use redbiom.util.resolve_ambiguities
instead
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.
ok, this is one of the other things we should discuss this afternoon.
qiita_pet/handlers/qiita_redbiom.py
Outdated
callback(([], 'Incorrect search by: you can use observations ' | ||
'or metadata and you passed: %s' % search_on)) | ||
|
||
if bool(samples): |
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.
why cast to bool
?
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 read somewhere that was the safest way to test for an empty set ...
@@ -117,3 +117,90 @@ function show_hide_process_list() { | |||
$("#qiita-processing").hide(); | |||
} | |||
} | |||
|
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 may not be the best person for review of changes to this file
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 mainly moving code around so the libraries are available everywhere vs. just in the study listing
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.
Can you add documentation to the JavaScript functions? I know at the beginning we were not doing it but I started doing it based on the Emperor documentation and it is really useful (i.e. it serves the same goal as the numpy doc).
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.
tried to add as many as possible
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!
// render Artifact Processing | ||
{"render": function ( data, type, row, meta ) { | ||
var text = ''; | ||
if (!(!row.command)) { |
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.
Isn't a double negative a positive?
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.
yup, again read somewhere that to test is a variable is not undefined or null the easiest is to do !row.command but didn't want to add an else so just negated that boolean ...
qiita_pet/handlers/qiita_redbiom.py
Outdated
@execute_as_transaction | ||
def _redbiom_search(self, context, query, search_on, callback): | ||
df = redbiom.summarize.contexts() | ||
contexts = df.ContextName.values |
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.
per gchat, we may want to filter these. And one other possibility in addition to what we discussed was is to filter on load of the cache as well.
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.
yup, let's chat this afternoon
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 reviewing, this should make our conversation go faster this afternoon and we are getting closer
INSTALL.md
Outdated
@@ -87,6 +88,26 @@ brew update | |||
brew install homebrew/versions/redis28 | |||
``` | |||
|
|||
### webdis | |||
|
|||
Note that this is the only package that assumes that Qiita is already installed (due to library dependencies). Also, that the general suggestion is to have 2 redis servers running, one for webdis/redbiom and the other for Qiita. The default configuration has webdis/redbiom running on the default redis port and Qiita's on 7777. |
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.
k
INSTALL.md
Outdated
@@ -163,6 +184,12 @@ Next, make a test environment: | |||
qiita-env make --no-load-ontologies | |||
``` | |||
|
|||
Finally, redbiom rely on the REDBIOM_HOST environment variable to set the URL to query. By default is set to http://127.0.0.1:7379, which is the webdis default. For example you could: |
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.
k, makes sense for default installations but it might be better to leave as is as the idea of these instructions is to have local installations for development.
qiita_pet/handlers/qiita_redbiom.py
Outdated
@coroutine | ||
@execute_as_transaction | ||
def get(self, search): | ||
contexts = yield Task(self._get_context) |
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.
Once you get the drift, pretty easy.
qiita_pet/handlers/qiita_redbiom.py
Outdated
@execute_as_transaction | ||
def _redbiom_search(self, context, query, search_on, callback): | ||
df = redbiom.summarize.contexts() | ||
contexts = df.ContextName.values |
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.
yup, let's chat this afternoon
qiita_pet/handlers/qiita_redbiom.py
Outdated
callback(([], 'The given context is not valid: %s - %s' % ( | ||
context, contexts))) | ||
else: | ||
if search_on == 'metadata': |
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.
yup, let's chat this afternoon
qiita_pet/handlers/qiita_redbiom.py
Outdated
# from_or_nargs first parameter is the file handler so it uses | ||
# that as the query input. None basically will force to take | ||
# the values from query | ||
samples = [s.split('_', 1)[1] |
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.
ok, this is one of the other things we should discuss this afternoon.
qiita_pet/handlers/qiita_redbiom.py
Outdated
# the values from query | ||
samples = [s.split('_', 1)[1] | ||
for s in redbiom.util.samples_from_observations( | ||
query.split(' '), True, context)] |
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.
yup, also within our discussion today.
qiita_pet/handlers/qiita_redbiom.py
Outdated
callback(([], 'Incorrect search by: you can use observations ' | ||
'or metadata and you passed: %s' % search_on)) | ||
|
||
if bool(samples): |
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 read somewhere that was the safest way to test for an empty set ...
@@ -117,3 +117,90 @@ function show_hide_process_list() { | |||
$("#qiita-processing").hide(); | |||
} | |||
} | |||
|
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 mainly moving code around so the libraries are available everywhere vs. just in the study listing
// render Artifact Processing | ||
{"render": function ( data, type, row, meta ) { | ||
var text = ''; | ||
if (!(!row.command)) { |
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.
yup, again read somewhere that to test is a variable is not undefined or null the easiest is to do !row.command but didn't want to add an else so just negated that boolean ...
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 few comments.
.travis.yml
Outdated
# first let's make sure redis is empty | ||
- curl -s http://127.0.0.1:7379/FLUSHALL > /dev/null | ||
- redbiom admin create-context --name "qiita-test" --description "qiita-test context" | ||
- fp=`python -c 'import qiita_db; print qiita_db.__file__'` |
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 line and the one below can be removed. Then instead of using ${qdbd}
on the other lines you can simply use `pwd`/qiita_db/. According to lines 40, 42, and 43 that should work w/o a problem
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.
k
INSTALL.md
Outdated
@@ -60,6 +60,7 @@ Install the non-python dependencies | |||
|
|||
* [PostgreSQL](http://www.postgresql.org/download/) (minimum required version 9.3.5, we have tested most extensively with 9.3.6) | |||
* [redis-server](http://redis.io) (we have tested most extensively with 2.8.17) | |||
* [webdis] (https://github.com/nicolasff/webdis) (latest version should be fine) |
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.
Can you make a comment similar to the one in redis? i.e. Latest version should be fine but we have tested most extensively with XXXX.
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.
k
INSTALL.md
Outdated
@@ -87,6 +88,26 @@ brew update | |||
brew install homebrew/versions/redis28 | |||
``` | |||
|
|||
### webdis | |||
|
|||
Note that this is the only package that assumes that Qiita is already installed (due to library dependencies). Also, that the general suggestion is to have 2 redis servers running, one for webdis/redbiom and the other for Qiita. The default configuration. The reason for multiple redis servers is so that the redbiom cache can be flushed without impacting the operation of the qiita server itself. |
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.
Remove The default configuration
or complete the sentence.
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.
rm
INSTALL.md
Outdated
make | ||
./webdis & | ||
popd | ||
wget https://raw.githubusercontent.com/wasade/redbiom/master/Makefile |
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.
The instructions outlined here looks like they're for setting up a test environment, but I don't believe that is the goal of this documentation. I think it should list the commands used to generate the release from Qiita, populate the redbiom DB from that file and then how to set up a job so that the cache gets updated periodically.
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.
yup, this is a copy of the first version with loaded the redbiom db; the new one actually loads the qiita data so changing
@@ -0,0 +1,163 @@ | |||
from tornado.gen import coroutine, Task |
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 file is missing the copyright header.
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.
k
@@ -0,0 +1,161 @@ | |||
from unittest import main |
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 file is missing the copyright notice
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.
k
response = self.post('/redbiom/', post_args) | ||
self.assertEqual(response.code, 200) | ||
exp = {'status': 'success', | ||
'message': ('Not a valid search: "4353076", are you sure this ' |
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.
Why is this not a valid metadata 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.
cause it's not a stem ...
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.
not exactly. we intentionally do not index things which look like numbers as i could not determine a reasonable strategy for determining when a number was or was not a good thing to index. in general, it is not and it would have very large impact on the key space. however, as a downside, important numbers like host taxon ID are not index. but, if a user knows they want to search for host taxon ID, they can still do that with where host_taxid=foo
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.
OK, thanks for the explanation. I'll coordinate with you to write some of these explanations and add them to the instructions.
qiita_pet/test/test_qiita_redbiom.py
Outdated
|
||
def test_post_observations(self): | ||
post_args = { | ||
'search': ('4479944'), |
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.
Not sure why these values are in parenthesis? They are not needed. (Applies to 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.
you are right but they don't hurt or aren't flake8 complaint ... so?
response = self.post('/redbiom/', post_args) | ||
self.assertEqual(response.code, 200) | ||
exp = {'status': 'success', | ||
'message': ('Not a valid search: "4353076", are you sure this ' |
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.
Same question as above - why it is not valid?
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.
also not a stem ... I guess that it will become valid if a metadata category or value is 4353076, @wasade?
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 order to allow this to move forward, can this be added as a TODO item (even if it is only a discussion)? I don't understand why a number will not be a valid stem, or why searching over numerical values is not allowed. However, I don't want to block this PR for this discussion.
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.
k, will do, now numerical values are fine but have to be inclosed in a where, like where age_cat > 40
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.
@josenavas, that would mean we'd index every single pH value, elevation, lat/long, etc in the absence of context.
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 the explanation @wasade , that makes sense.
@@ -395,6 +430,8 @@ def main(): | |||
ipython_running = False | |||
extra_info = ('IPython test failed, perhaps due to an old MOI ' | |||
'version, please review') | |||
|
|||
print qiita_config_header |
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.
Remove print?
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 actually needed so it's displayed, see line https://github.com/biocore/qiita/pull/2118/files#diff-bb7154348182dace785aedbad03c3a2bL341
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
.travis.yml
Outdated
# first let's make sure redis is empty | ||
- curl -s http://127.0.0.1:7379/FLUSHALL > /dev/null | ||
- redbiom admin create-context --name "qiita-test" --description "qiita-test context" | ||
- fp=`python -c 'import qiita_db; print qiita_db.__file__'` |
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.
k
INSTALL.md
Outdated
@@ -60,6 +60,7 @@ Install the non-python dependencies | |||
|
|||
* [PostgreSQL](http://www.postgresql.org/download/) (minimum required version 9.3.5, we have tested most extensively with 9.3.6) | |||
* [redis-server](http://redis.io) (we have tested most extensively with 2.8.17) | |||
* [webdis] (https://github.com/nicolasff/webdis) (latest version should be fine) |
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.
k
INSTALL.md
Outdated
@@ -87,6 +88,26 @@ brew update | |||
brew install homebrew/versions/redis28 | |||
``` | |||
|
|||
### webdis | |||
|
|||
Note that this is the only package that assumes that Qiita is already installed (due to library dependencies). Also, that the general suggestion is to have 2 redis servers running, one for webdis/redbiom and the other for Qiita. The default configuration. The reason for multiple redis servers is so that the redbiom cache can be flushed without impacting the operation of the qiita server itself. |
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.
rm
INSTALL.md
Outdated
make | ||
./webdis & | ||
popd | ||
wget https://raw.githubusercontent.com/wasade/redbiom/master/Makefile |
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.
yup, this is a copy of the first version with loaded the redbiom db; the new one actually loads the qiita data so changing
@@ -0,0 +1,163 @@ | |||
from tornado.gen import coroutine, Task |
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.
k
@@ -0,0 +1,161 @@ | |||
from unittest import main |
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.
k
response = self.post('/redbiom/', post_args) | ||
self.assertEqual(response.code, 200) | ||
exp = {'status': 'success', | ||
'message': ('Not a valid search: "4353076", are you sure this ' |
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.
cause it's not a stem ...
qiita_pet/test/test_qiita_redbiom.py
Outdated
|
||
def test_post_observations(self): | ||
post_args = { | ||
'search': ('4479944'), |
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.
you are right but they don't hurt or aren't flake8 complaint ... so?
response = self.post('/redbiom/', post_args) | ||
self.assertEqual(response.code, 200) | ||
exp = {'status': 'success', | ||
'message': ('Not a valid search: "4353076", are you sure this ' |
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.
also not a stem ... I guess that it will become valid if a metadata category or value is 4353076, @wasade?
@@ -395,6 +430,8 @@ def main(): | |||
ipython_running = False | |||
extra_info = ('IPython test failed, perhaps due to an old MOI ' | |||
'version, please review') | |||
|
|||
print qiita_config_header |
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 actually needed so it's displayed, see line https://github.com/biocore/qiita/pull/2118/files#diff-bb7154348182dace785aedbad03c3a2bL341
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.
Only a couple of small comments regarding JS library imports.,
|
||
try: | ||
df = redbiom.summarize.contexts() | ||
except ConnectionError: |
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.
Ok, just wanted to make sure that this was the desired behavior.
@@ -117,3 +117,90 @@ function show_hide_process_list() { | |||
$("#qiita-processing").hide(); | |||
} | |||
} | |||
|
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!
qiita_pet/templates/redbiom.html
Outdated
<script type="text/javascript" src="{% raw qiita_config.portal_dir %}/static/js/sharing.js"></script> | ||
<script type="text/javascript"> | ||
$(document).ready(function() { | ||
moi.init(null, window.location.host + '{% raw qiita_config.portal_dir %}/study/list/socket/', function(){}, 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.
oh - thanks!
qiita_pet/templates/sitebase.html
Outdated
<script src="{% raw qiita_config.portal_dir %}/static/js/qiita.js"></script> | ||
|
||
<script src="{% raw qiita_config.portal_dir %}/static/vendor/js/moi.js"></script> |
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 one is added twice (see line 49)
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.
k
qiita_pet/templates/redbiom.html
Outdated
{% extends sitebase.html %} | ||
|
||
{%block head%} | ||
<link rel="stylesheet" href="{% raw qiita_config.portal_dir %}/static/vendor/css/select2.min.css" type="text/css"> |
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.
Just checked - select2.min.js
it is added in sitebase.html
but the css is not. I would suggest removing them from here and adding the css in the sitebase.html
file. Since the JS is added there, somewhere in the code it is expected to be available but currently is not, which I can generate some problems down the line.
On the other hand, sharing.js
is ok to leave here since it is only used here.
response = self.post('/redbiom/', post_args) | ||
self.assertEqual(response.code, 200) | ||
exp = {'status': 'success', | ||
'message': ('Not a valid search: "4353076", are you sure this ' |
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 order to allow this to move forward, can this be added as a TODO item (even if it is only a discussion)? I don't understand why a number will not be a valid stem, or why searching over numerical values is not allowed. However, I don't want to block this PR for this discussion.
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, changes soon!
qiita_pet/templates/redbiom.html
Outdated
{% extends sitebase.html %} | ||
|
||
{%block head%} | ||
<link rel="stylesheet" href="{% raw qiita_config.portal_dir %}/static/vendor/css/select2.min.css" type="text/css"> |
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.
k, removing all duplicated imports.
qiita_pet/templates/sitebase.html
Outdated
<script src="{% raw qiita_config.portal_dir %}/static/js/qiita.js"></script> | ||
|
||
<script src="{% raw qiita_config.portal_dir %}/static/vendor/js/moi.js"></script> |
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.
k
response = self.post('/redbiom/', post_args) | ||
self.assertEqual(response.code, 200) | ||
exp = {'status': 'success', | ||
'message': ('Not a valid search: "4353076", are you sure this ' |
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.
k, will do, now numerical values are fine but have to be inclosed in a where, like where age_cat > 40
Just installing redbiom, once the test pass I'll add a handler