Make new aggregations searchable #131
Conversation
Pull Request Test Coverage Report for Build 8192124817Details
💛 - Coveralls |
|
I'm treating this as a hotfix and merging it prior to review. I welcome a retroactive review and any suggestions/opinions on the testing front. |
Why these changes are being introduced: We added some more aggregations as part of the ongoing work, but did not add them to the query string or corresponding models. Relevant ticket(s): * (GDT-128)[https://mitlibraries.atlassian.net/browse/GDT-128] How this addresses that need: This adds the new aggregations to the Enhancer and QueryBuilder models, and adds them to the query in the TimdexSearch model. Side effects of this change: * VCR cassettes have been regenerated. * In the filter partial, `to_sym` calls on the filter category have been removed. These are no longer needed as all filter categories are cast as symbols in the search controller. * This is another instance of something that has come up recently: the need to develop an E2E testing strategy for our Rails apps, particularly those that are API consumers. We can test this using our standard VCR process, but that is likely to break when cassettes are regenerated and the search results change.
There was a problem hiding this comment.
There's one change in here that I'm not sure I'm following (not because I anticipate a problem, I'm just not sure why it is happening) - I've noted that inline.
More generally, am I correct in thinking that these are things I should have included as part of #128 ?
| @@ -1,17 +1,17 @@ | |||
| <% return if values.blank? %> | |||
|
|
|||
| <div class="category"> | |||
| <button class="filter-label <%= 'expanded' if @enhanced_query[category.to_sym].present? || first == true %>" | |||
| <button class="filter-label <%= 'expanded' if @enhanced_query[category].present? || first == true %>" | |||
There was a problem hiding this comment.
I see us dropping the .to_sym method here, and I'm not sure why. I'm sure it makes sense - my first assumption is because the values aren't convertible to symbols now, but were before. I'm just not sure I'm following what changed.
There was a problem hiding this comment.
Sorry, I should've been clearer in the commit message. That to_sym call was there because SearchController#extract_filters didn't always cast the filter keys as symbols. I refactored that method to do so a while back, but noticed while debugging this that there were still a couple of to_sym calls. I removed them to make this less confusing for our future selves (hopefully), but it's unrelated to the additional/customizable filters changeset.
There was a problem hiding this comment.
Ah, gotcha - that makes sense. Thanks!
|
@matt-bernhardt Yes, this is related to #128. I ought to have mentioned it during code review but missed it at the time. I suspect that I was confirming functionality by click-testing |
|
No worries - I should have caught it as well. Thank you for catching it now, and for the hotfix :-) |
** Why are these changes being introduced: * The TIMDEX API has added support for a Places filter and aggregation, so the UI should be extended to take advantage of this. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/post-128 ** How does this address that need: * This builds on the work from #128 and #131, to include the Places feature in our displayed filters and aggregations. ** Document any side effects to this change: * These changes will require that our test cassettes are re-generated, which will happen in the next commit.
Why these changes are being introduced:
We added some more aggregations as part of the ongoing work, but did
not add them to the query string or corresponding models.
Relevant ticket(s):
How this addresses that need:
This adds the new aggregations to the Enhancer and QueryBuilder models, and adds them to the query in the TimdexSearch model.
Side effects of this change:
Developer
our guide and
all issues introduced by these changes have been resolved or opened as new
issues (link to those issues in the Pull Request details above)
Code Reviewer
(not just this pull request message)
Requires database migrations?
NO
Includes new or updated dependencies?
NO