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

Mccalluc/add fields to datasetmanager index #1824

Merged
merged 9 commits into from Jun 26, 2017

Conversation

mccalluc
Copy link
Member

@mccalluc mccalluc commented Jun 22, 2017

Towards #1815: I want facets that will work across datasets. I'm not sure who has the most experience here, so listing everyone.

The test needs to be stronger, and after that the next step would be to use it in the UI.

@mccalluc
Copy link
Member Author

(Tests still pass, which indicates that we need better coverage here.)

@refinery-platform refinery-platform deleted a comment from codecov-io Jun 22, 2017
name)

uniq_key = name + "_" + uuid + "_s"
generic_key = name + "_generic_s"
Copy link
Member Author

Choose a reason for hiding this comment

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

Adding "generic" fields to solr is the reason for this PR. Is this a reasonable approach?

Copy link
Member

Choose a reason for hiding this comment

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

@mccalluc I wouldn't know myself without doing more research about solr.
I think @flekschas would have more of an idea than any other team member.

Copy link
Member Author

Choose a reason for hiding this comment

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

Talked this over with Nils and he thinks its reasonable.


test_file = StringIO()
test_file.write('Coffee is great.\n')
file_store_item = FileStoreItem.objects.create(
Copy link
Member

Choose a reason for hiding this comment

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

I've found that something in our codebase is still writing temp files to disk for FileStoreItems regardless of the class you use for the datafile param

We have a pre_delete signal for FileStoreItems that deletes these datafiles, so I've been doing this in my tests:

def tearDown(self):
    FileStoreItem.objects.all().delete()

name)

uniq_key = name + "_" + uuid + "_s"
generic_key = name + "_generic_s"
Copy link
Member

Choose a reason for hiding this comment

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

@mccalluc I wouldn't know myself without doing more research about solr.
I think @flekschas would have more of an idea than any other team member.

@scottx611x
Copy link
Member

@mccalluc Also regarding the travis test failures I found overriding the maxDiff attribute of TestCases to be helpful:

# Some members in assertions are truncated if they are too long, but we
# want to test them in their entirety
maxDiff = None

Copy link
Member

@hackdna hackdna left a comment

Choose a reason for hiding this comment

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

Unfortunately, I don't know enough about this code to provide meaningful feedback.

from rest_framework.test import APIClient, APIRequestFactory, APITestCase

from .models import Assay, AttributeOrder, Study, Investigation, Node
from core.models import DataSet, ExtendedGroup, InvestigationLink
Copy link
Member

@jkmarx jkmarx Jun 23, 2017

Choose a reason for hiding this comment

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

code style guide:

from __future__ import absolute_import

from .models import WaffleCone
from .forms import WaffleConeForm
from core.views import FoodMixin```

Copy link
Member

Choose a reason for hiding this comment

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

Which needs to be updated?

Copy link
Member Author

Choose a reason for hiding this comment

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

isort wants to put relative imports at the end, which doesn't seem bad to me, but I'll look into overriding that: #1830.

@codecov-io
Copy link

Codecov Report

Merging #1824 into develop will increase coverage by 0.17%.
The diff coverage is 65.9%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #1824      +/-   ##
===========================================
+ Coverage    41.04%   41.21%   +0.17%     
===========================================
  Files          394      394              
  Lines        25758    26360     +602     
  Branches      1278     1297      +19     
===========================================
+ Hits         10572    10864     +292     
- Misses       15186    15496     +310
Impacted Files Coverage Δ
refinery/data_set_manager/tests.py 99.56% <100%> (+0.02%) ⬆️
refinery/data_set_manager/search_indexes.py 67.1% <11.76%> (+29.6%) ⬆️
.../tool-launch/services/file-relationship-service.js 88.42% <0%> (-6.78%) ⬇️
.../file-browser/ctrls/node-selection-popover-ctrl.js 100% <0%> (ø) ⬆️
refinery/core/utils.py 32.56% <0%> (+0.11%) ⬆️
...ui/source/js/tool-launch/ctrls/input-group-ctrl.js 88.6% <0%> (+0.37%) ⬆️
refinery/core/views.py 24.96% <0%> (+0.65%) ⬆️
.../file-browser/directives/node-selection-popover.js 92.59% <0%> (+1.28%) ⬆️
.../source/js/tool-launch/ctrls/input-control-ctrl.js 93.33% <0%> (+1.66%) ⬆️
...ce/js/file-browser/services/active-node-service.js 80% <0%> (+10%) ⬆️
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4bcd8d...8cacffe. Read the comment docs.

@mccalluc mccalluc merged commit fb6d99b into develop Jun 26, 2017
@mccalluc mccalluc deleted the mccalluc/add-fields-to-datasetmanager-index branch June 26, 2017 15:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants