Conversation
| """Return the top 100 publishers ordered by dataset count.""" | ||
| publishers = self.opensearch.get_publisher_counts(size=100) | ||
|
|
||
| return sorted( |
There was a problem hiding this comment.
this seems complicated. opensearch agg should return non-empty publishers with count > 0. if that's less than 100 so what. we don't need to sort by name only dataset count. also, each item should always contain the necessary keys so no need to check if one exists. why can't this be...
return sorted( publishers, key=lambda item: item["count"], reverse=True)or better yet why can the OS result be sorted already?
| org_id=None, | ||
| org_types=None, | ||
| keywords: list[str] = None, | ||
| publisher: str | None = None, |
There was a problem hiding this comment.
where is get_contextual_aggregations called?
| publisher_size=100, | ||
| ) -> dict: | ||
| """ | ||
| Get keyword and organization aggregations based on current search context. |
There was a problem hiding this comment.
update docstring to include publishers?
| publisher_size=100, | ||
| ) -> dict: | ||
| """ | ||
| Get keyword and organization aggregations based on current search context. |
There was a problem hiding this comment.
update docstring for publisher
| } | ||
|
|
||
| class PublisherAutocomplete { | ||
| constructor(options) { |
There was a problem hiding this comment.
at this point we now have 3 classes sharing identical functionality. publisher, keyword, and organization. seems like a good opportunity to extract a base class so we can avoid having to review as much content.
| _get_publisher_counts, | ||
| ) | ||
|
|
||
| publishers = interface_with_dataset.get_top_publishers() |
There was a problem hiding this comment.
this sorts by multiple fields (count then name) and checks for key presence. can you update the fixture data with more examples to reflect the expected sorting behavior?
| assert geography_panel is not None | ||
|
|
||
|
|
||
| class TestPublisherSearch: |
There was a problem hiding this comment.
does this work for organization? how about multiple publishers? do the chips look they way we expect them to? (only the selected ones are present and the counts are correct), do aggregates update after filtering by publisher?
3 participants
For
About:
add publisher as a new filter, next to keywords, organizations...
Changes:
get_top_publishers()to get top 100 publishers/api/publishersTODO
this need a new OpenSearch index.