Skip to content

Conversation

amymok
Copy link
Contributor

@amymok amymok commented Jun 22, 2017

This is to add aggregations as part of the search.

Additions:

  • Added aggregation object creation logic
  • Updated tests

Notes:

This PR does not include the following, but the following items will be worked on in later PRs:

  • Add an options to disable aggregation for search
  • Refactor/clean up code

Reviews:

@JeffreyMFarley @AdamZarger
cc @sephcoster

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling e8366a7 on amymok:aggs into 0a00e69 on cfpb:master.

@amymok amymok changed the title [WIP] Add aggregations with filters Add aggregations with filters Jun 25, 2017
Field('product', 'Product / Subproduct', 10000, True),
Field('issue', 'Issue / Subissue', 10000, True),
Field('state', 'State', 50, False),
Field('zip_code', 'Zip code', 1000, False),
Copy link
Contributor

Choose a reason for hiding this comment

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

Is zip code really 1000 characters?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think the short field names are more appropriate to exchange in the aggs rather than the longer titles

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@JeffreyMFarley This is the size of the results being returned, not the # of characters in the zip code field. I will also take a look at what makes sense for each field as well.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay I will use the short name and leave it to the front-end to handle the title.

@@ -36,11 +37,307 @@ def test_search_no_param__valid(self, mock_rget, mock_search):
"fragment_size": 500
},
"sort": [{"_score": {"order": "desc"}}],
"post_filter": {"and": {"filters": []}}
"post_filter": {"and": {"filters": []}},
"aggs": {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking, how in the world is the test file 9K lines? I see now it is due to lots of embedded JSON to validate the responses.

It's fine, but you could always store them as static files in the same directory and keep the test file focused on the test, not the expected data. Here's my version of that pattern

    # -------------------------------------------------------------------------
    # Helper Methods
    # -------------------------------------------------------------------------

    def to_absolute(self, fileName):
        import os.path
        # where is this module?
        thisDir = os.path.dirname(__file__)
        return os.path.join(thisDir, fileName)

    def load(self, shortName):
        import json
        fileName = self.to_absolute(shortName + '.json')
        with open(fileName, 'r') as f:
            return json.load(f)

    # -------------------------------------------------------------------------
    # Test Methods
    # -------------------------------------------------------------------------

    def test_search_no_param_valid(self):
        expected = self.load('search_no_param_valid')

        actual = ...

        self.assertEqual(actual, expected)

"sub_product": "sub_product.raw",
"issue": "issue.raw",
"sub_issue": "sub_issue.raw"
}
Copy link
Contributor

@JeffreyMFarley JeffreyMFarley Jun 26, 2017

Choose a reason for hiding this comment

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

add company_public_response.raw and consumer_consent_provided.raw

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Currently company_public_response.raw and consumer_consent_provided.raw are currently not in the mappings, so it may not work at this moment. @AdamZarger Can you add them to the mapping and I will do the same here for the API

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@AdamZarger AdamZarger requested review from AdamZarger and removed request for AdamZarger June 28, 2017 22:10
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling c77711a on amymok:aggs into 0a00e69 on cfpb:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 47fabc2 on amymok:aggs into 0a00e69 on cfpb:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4bb23b0 on amymok:aggs into 0a00e69 on cfpb:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 17067af on amymok:aggs into 0a00e69 on cfpb:master.

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 4a01a3c on amymok:aggs into 0a00e69 on cfpb:master.

@amymok
Copy link
Contributor Author

amymok commented Jun 29, 2017

Will merge this in for now.

@amymok amymok merged commit d4ca32c into cfpb:master Jun 29, 2017
JeffreyMFarley added a commit to JeffreyMFarley/ccdb5-ui that referenced this pull request Jun 30, 2017
AdamZarger pushed a commit to AdamZarger/ccdb5-api that referenced this pull request Sep 22, 2017
Use the latest CSS markup for pagination controls
Scotchester pushed a commit to cfpb/ccdb5-ui that referenced this pull request Jul 20, 2020
imuchnik added a commit that referenced this pull request Apr 30, 2021
higs4281 pushed a commit that referenced this pull request Jul 14, 2021
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