-
Notifications
You must be signed in to change notification settings - Fork 13
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
Fixed getSamplingFeatureDatasets issue #130 #131
Conversation
@Elijahwalkerwest Please fix the test also. I won't merge until we have passing Travis CI. Thanks. Let me know if you need my help. =================================== FAILURES ===================================
_______________ TestReadService.test_getSamplingFeatureDataSets ________________
self = <tests.test_odm2.test_readservice.TestReadService instance at 0x7fd087c41170>
def test_getSamplingFeatureDataSets(self):
try:
#find a sampling feature that is associated with a dataset
sf = self.engine.execute(
'SELECT * from SamplingFeatures as sf '
'inner join FeatureActions as fa on fa.SamplingFeatureID == sf.SamplingFeatureID '
'inner join Results as r on fa.FeatureActionID == r.FeatureActionID '
'inner join DataSetsResults as ds on r.ResultID == ds.ResultID '
).fetchone()
assert len(sf) > 0
#get the dataset associated with the sampling feature
ds = self.engine.execute(
'SELECT * from DataSetsResults as ds '
'inner join Results as r on r.ResultID == ds.ResultID '
'inner join FeatureActions as fa on fa.FeatureActionID == r.FeatureActionID '
'where fa.SamplingFeatureID = ' + str(sf[0])
).fetchone()
assert len(ds) > 0
print (sf[0])
# get the dataset associated with the sampling feature using hte api
dsapi = self.reader.getSamplingFeatureDatasets(ids=[sf[0]])
assert dsapi is not None
assert len(dsapi) > 0
assert dsapi[0].datasets is not None
assert dsapi[0].SamplingFeatureID == sf[0]
# assert ds[0] == dsapi[0]
except Exception as ex:
> assert False
E assert False
tests/test_odm2/test_readservice.py:162: AssertionError
----------------------------- Captured stdout call -----------------------------
1
Error running Query: 'NoneType' object is not iterable
=============== 1 failed, 57 passed, 34 skipped in 4.79 seconds ================ |
@Elijahwalkerwest What is the status of this? |
odm2api/ODM2/services/readService.py
Outdated
@@ -875,7 +887,7 @@ def getDataSetsValues(self, ids=None, codes=None, uuids=None, dstype=None): | |||
return None | |||
|
|||
|
|||
def getSamplingFeatureDatasets(self, ids=None, codes=None, uuids=None, dstype=None): | |||
def getSamplingFeatureDatasets(self, ids=None, codes=None, uuids=None, dstype=None, type=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.
type
is a reserved python word. Please use sftype
to be clear, and avoid using reserved word.
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, the doc string doesn't seem to reflect this additional argument. Please add.
|
||
def assignRelatedFeatures(self, relatedfeatures): | ||
self.related_features = {} | ||
if relatedfeatures: |
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 added this line to handle cases that there are no relatedfeatures.
# res.FeatureActionObj = None | ||
self.datasets[dsr.DataSetObj].append(res) | ||
self.datasets = {} | ||
if datasetresults: |
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 added this line to handle cases that there are no datasets associated.
Seems like the failure reported on #131 (comment) wasn't because the test needed to be changed, but the actual |
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.
Should be good to merge now, once the Travis CI checks are complete.
I've tested the behavior of these changes. They all look good. Thanks @Elijahwalkerwest. Merging now. This resolves #130. |
Additionally added ability to filter by SamplingFeatureTypeCV as per Mauriel's request.