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

Jkmarx/solr user files bug #2950

Merged
merged 20 commits into from Aug 24, 2018
Merged

Jkmarx/solr user files bug #2950

merged 20 commits into from Aug 24, 2018

Conversation

jkmarx
Copy link
Member

@jkmarx jkmarx commented Aug 23, 2018

Resolves #2623
Resolves #2931
Resolves #2729 (Remove insert_facet_field method. No longer needed.)

@codecov
Copy link

codecov bot commented Aug 23, 2018

Codecov Report

Merging #2950 into develop will decrease coverage by 1.17%.
The diff coverage is 89.92%.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop    #2950      +/-   ##
===========================================
- Coverage    60.37%   59.19%   -1.18%     
===========================================
  Files          433      434       +1     
  Lines        27924    27144     -780     
  Branches      1274     1274              
===========================================
- Hits         16860    16069     -791     
- Misses       11064    11075      +11
Impacted Files Coverage Δ
refinery/user_files_manager/tests.py 100% <100%> (ø) ⬆️
refinery/data_set_manager/utils.py 74.77% <80%> (-6.06%) ⬇️
refinery/data_set_manager/tests.py 99.28% <94.73%> (-0.08%) ⬇️
...e/js/data-set-import/controllers/isa-tab-import.js 40.9% <0%> (-5.15%) ⬇️
...ery/tool_manager/management/commands/load_tools.py 85.23% <0%> (-0.77%) ⬇️
refinery/tool_manager/tests.py 99.82% <0%> (-0.01%) ⬇️
refinery/selenium_testing/utils.py 0% <0%> (ø)

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 5917397...6f7cdbe. Read the comment docs.

@jkmarx jkmarx requested a review from mccalluc August 23, 2018 16:28
@jkmarx jkmarx self-assigned this Aug 23, 2018
@jkmarx jkmarx added this to the Release 1.6.6 milestone Aug 23, 2018
Copy link
Member

@mccalluc mccalluc left a comment

Choose a reason for hiding this comment

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

As far as I follow it, looks fine. I can't remember why all the gets are necessary... Not really dicts under the hood?

insert_facet_field_filter, is_field_in_hidden_list,
objectify_facet_field_counts, update_annotated_nodes,
is_field_in_hidden_list, update_annotated_nodes,
create_facet_field_counts,
Copy link
Member

Choose a reason for hiding this comment

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

I can't remember if we decided whether to worry about alphabetical order or not.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes to worry. Thanks

))

def test_generate_solr_params_for_assay_with_params(self):
self.assertItemsEqual(query.keys(), ['json', 'params'])
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you sort keys below: worth doing it here, too?


def test_generate_solr_params_no_params_returns_json_filter(self):
query = generate_solr_params_for_assay(QueryDict({}), self.valid_uuid)
self.assertListEqual(query.get('json').get('filter'),
Copy link
Member

Choose a reason for hiding this comment

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

Would regular query['json']['filter'] not work here and elsewhere?

if not facet_field:
facet_field = ','.join([char_str, factor_str])
else:
facet_field = ','.join([facet_field, char_str, factor_str])
Copy link
Member

Choose a reason for hiding this comment

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

Maybe build up a list and join it once at the end instead?

Copy link
Member Author

@jkmarx jkmarx Aug 23, 2018

Choose a reason for hiding this comment

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

I was doing other things here before. I'll just revert to the previous code now.

})
{'name': field_obj.get('val'),
'count': field_obj.get('count')}
)
Copy link
Member

Choose a reason for hiding this comment

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

List comprehension clearer?

@jkmarx jkmarx merged commit 0134753 into develop Aug 24, 2018
@jkmarx jkmarx deleted the jkmarx/solr-user-files-bug branch August 24, 2018 10:34
@jkmarx jkmarx mentioned this pull request Aug 24, 2018
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

2 participants