Skip to content

Conversation

ambrussimon
Copy link
Contributor

@ambrussimon ambrussimon commented Jul 11, 2017

Added dataexplorerhandler unit test with mocked ElasticSearch.
@nagem @hkethi002:

  • gathered the source fields into module level vars
  • found unreachable lines (added #NOTE unreachable)
  • didn't get to _handle_properties recursion yet
  • didn't get to facet_status @ indexing yet

Review Checklist

  • Tests were added to cover all code changes
  • Documentation was added / updated
  • Code and tests follow standards in CONTRIBUTING.md

@ambrussimon ambrussimon self-assigned this Jul 11, 2017
@ambrussimon ambrussimon requested a review from nagem July 11, 2017 20:11
@ambrussimon ambrussimon requested a review from hkethi002 July 12, 2017 15:01
}
}
if not filters:
# NOTE unreachable
Copy link
Contributor

Choose a reason for hiding this comment

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

The code will need to get updated to support device behavior for completeness. That's where an empty filter could come from (no uid for device auth), but it doesn't currently support it. I'll open a ticket about supporting device requests in search.

field_query = str(field_query)
except ValueError:
# NOTE unreachable
self.abort(400, 'Must specify string for field query')
Copy link
Contributor

Choose a reason for hiding this comment

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

This can get removed

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you mean the whole except or just the comment? :)

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant the try/except around the str cast.

}


SOURCE_COMMON = [
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for cleaning this up 👍

@nagem
Copy link
Contributor

nagem commented Jul 20, 2017

@ambrussimon-invenshure feel free to merge this one.

@ambrussimon ambrussimon merged commit 49b6019 into master Jul 20, 2017
@ambrussimon ambrussimon deleted the dataexplorer-tests branch July 20, 2017 19:23
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.

3 participants