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

Es item -- Fourfront. NOT READY #1225

Merged
merged 8 commits into from
Feb 3, 2020
Merged

Es item -- Fourfront. NOT READY #1225

merged 8 commits into from
Feb 3, 2020

Conversation

carlvitzthum
Copy link
Collaborator

@carlvitzthum carlvitzthum commented Dec 17, 2019

DO NOT FORGET TO UPDATE SNOVAULT BRANCH IN BUILDOUT.CFG

This branch is the Fourfront complement to 4dn-dcic/snovault#111. It adds some functionality specific to the new Elasticsearch items and a few other misc things.

Summary

  • dev_servers.py changed to run create_mapping before and after loading test data. This is done to initialize any ES-item indices before loading, as well as deduplicate the queue before serving the application (and thus indexing)
  • Refactored User._update to make more sense. Now does not does gets to multiple labs to get display titles. This code should be fully deprecated in the future for a more full-featured subscriptions
  • /counts refactored a bit. May be slightly less performant, but is now much more maintainable
  • Refactored some indexing-related test features. This previously in .features.conftest have been moved to workbook_fixtures
  • Removed last reference to linkFrom in the testing types
  • Other random test fix

@carlvitzthum carlvitzthum added the Not Ready This branch is not yet ready for code review. label Dec 17, 2019
@coveralls
Copy link

coveralls commented Dec 17, 2019

Coverage Status

Coverage decreased (-14.6%) to 47.286% when pulling 58b9d02 on es_item into 288af11 on master.

Copy link
Collaborator

@netsettler netsettler left a comment

Choose a reason for hiding this comment

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

This PR is very similar to one in the cgap-portal. I accidentally fell into this one and it read through parts of it before I realized I was in the wrong system. Anyway, I am AGAIN up to date on changes here and still happy with it.

@@ -71,7 +71,7 @@ def test_document_display_title_wo_attachment(testapp, protocol_data):
from datetime import datetime
del(protocol_data['protocol_type'])
res = testapp.post_json('/document', protocol_data).json['@graph'][0]
assert res.get('display_title') == 'Document from ' + str(datetime.now())[:10]
assert res.get('display_title') == 'Document from ' + str(datetime.utcnow())[:10]
Copy link
Collaborator

Choose a reason for hiding this comment

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

At some point maybe put it in utils but for now just duplicating the utc_today_str() we created for fourfront makes sense here. I don't like these string surgeries.

# run _update on a new user with already formed subscriptions.
# Ensure they're not duplicated in the process
user_data = {
'first_name': 'User',
'last_name': 'McUser',
'email': '[email protected]',
'status': 'current',
'lab': lab['uuid'],
'subscriptions': [{'url': '?lab.uuid=' + lab['uuid'] + '&sort=-date_created',
Copy link
Collaborator

Choose a reason for hiding this comment

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

This URL is null, just has a query string? Is that even legal?

Copy link
Member

Choose a reason for hiding this comment

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

Lab in this case I believe is the result of the post so it will always have uuid.

conn = request.registry[CONNECTION]
return [request.resource_path(conn[uuid]) for uuid in
self.get_filtered_rev_links(request, 'reverse')]
return self.rev_link_atids(request, "reverse")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Very minor, but I had to go look up what this second argument was and whether it was something semantic. Seems it's just a name. I'd write this as:

return self.rev_link_atids(request, rev_name="reverse")

@willronchetti willronchetti merged commit f2c300f into master Feb 3, 2020
@willronchetti willronchetti deleted the es_item branch February 3, 2020 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Not Ready This branch is not yet ready for code review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants