Expand available filters, give applications the ability to filter and order via ENV #128
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Pull Request Test Coverage Report for Build 8147243807Details
💛 - Coveralls |
matt-bernhardt
force-pushed
the
gdt-128-filters-list
branch
from
6544625 to
49d1d26
Compare
** Why are these changes being introduced: This app implements only two filter / aggregation categories: content type and source. However, others are now supported by the API: Contributors, format, languages, literary form, and subjects. Others are coming soon, but for now these are what is available. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/gdt-128 ** How does this address that need: This updates our search model, and filter helper, to add support for the new categories. ** Document any side effects to this change: Because all queries now include this request for more aggregations, we need to regenerate a number of the test cassettes.
** Why are these changes being introduced: While the API provides a large number of possible options for users to filter and aggregate their search results, not every instance of the application will want to use every category (GeoData and the basic Search application will likely use different sets, for example). One good option for enabling these differences is to leverage environment variables. ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/gdt-128 ** How does this address that need: This adds a new environment variable to the application, ACTIVE_FILTERS. This is meant to be a list of the filter categories which should be displayed in the application. The variable is optional - without it, the application should include every available category, in the order they appear in the query model. The env variable is loaded and processed in the search controller, which is then used to order and filter the categories returned from the API before sending the result to the user in the UI. The tests added in this commit demonstrate the impact of this change. ** Document any side effects to this change: Because the env var is optional, I'm not adding it to app.json. I'm leaning toward adding it to the Heroku pipeline, to preserve the current behavior (only content type and source being shown).
** Why are these changes being introduced: * The nice_labels method in the filter helper defines static names for each possible filter category, but the GDT project makes it likely that these names may need to vary by application due to the nature of the records available (i.e. "content type" versus "data type"). ** Relevant ticket(s): * https://mitlibraries.atlassian.net/browse/gdt-128 ** How does this address that need: * This adds support for loading the filter name from ENV, using a set of variables listed in the README. ** Document any side effects to this change: This is not a side effect, but a point of hesitation. I'm unsure if we need tests for _each_ ENV override in nice_labels. Right now I've written one to confirm that "Content type" is the default, and one to confirm that it can be overriden - for that category only. It feels like a bit of overkill to test each of the seven categories (and then need to write more tests as new categories are added to the API) - but this leaves open the possibility that the overrides - which are not likely to be heavily used - might break in a way that we do not realize initially. I take some solace in the fact that the defaults we provide are "okay", and that there will be stakeholders watching for the overridden values - so any future problem should be caught relatively quickly - but I'm open to contrasting perspectives here.
matt-bernhardt
force-pushed
the
gdt-128-filters-list
branch
from
2f9426c to
6363a55
Compare
jazairi
approved these changes
Mar 5, 2024
There was a problem hiding this comment.
Nice work on this! This will make it much easier to add the other filters when they become available. Abstracting the config to env is a nice touch that will help us manage multiple TIMDEX apps with different filters and labels.
I agree both that it would be tiresome to test everything in nice_labels, and that it opens us up to missing a potential breaking change. In this case, I think the logic in that method is so simple that testing one filter as a sample is fine for now. As you note, it's written to fall back on reasonable labels, anyway. It seems to be that the absolute worst case is that a new filter label is not defined at all, in which case it will fall back to the GraphQL field name, which doesn't feel particularly dangerous.
A minimally painful way to test everything could be to just add all the label to those two tests, so if something is off with anything in the nice_labels hash, the test fails and alerts us to the issue. That may be something to consider we've added the last of the filters. For now, this feels like adequate coverage.
Thanks for the excellent work, and for going above and beyond the requirements!
|
One more thing to add, in case you mention this detail in an amended commit message: GDT-128 can actually close without the access type filter. That one has its own ticket (GDT-140), as it's a bit different from the others. |
9 tasks
matt-bernhardt
added a commit
that referenced
this pull request
Mar 12, 2024
** 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.
11 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This expands the set of available filter / aggregation categories that are available for our search applications, and makes it possible for each application to customize which categories are shown, and how.
With this change, the full list of available categories are:
Individual applications can, via an
ACTIVE_FILTERSenvironment variable, choose which of these are shown, and in what order.Additionally, applications can override the name of any of these categories using a separate environment variable for each category.
This work will partially address ticket GDT-128, but it cannot close until the following filters are also supported:
Developer
Environment
Accessibility
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)
Stakeholders
Requires database migrations?
NO
Includes new or updated dependencies?
NO
Code Reviewer
(not just this pull request message)