Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove expanded search result fields configuration #3125

Merged
merged 3 commits into from
Jan 22, 2025

Conversation

ryanb-gds
Copy link
Contributor

@ryanb-gds ryanb-gds commented Jan 20, 2025

Search result field expansion was used exclusively to enable human-friendly labels for metadata in Finder Frontend.

However, Finder Frontend already has access to the human friendly labels from the facet options for each field. Therefore it made sense to map the search result value to the facet option label in Finder Frontend and remove the expanded search result configuration from Search API. This enables us to reduce the amount of changes we need to make to Search API configuration when changing specialist finders.

Relevant finder frontend PR for context: alphagov/finder-frontend#3627

Trello: https://trello.com/c/kFdQhAGr

@kevindew
Copy link
Member

Sounds great to remove this information 👍 Does seem such a headache/dubious technical decision to hardcode all of these into Search API so fantastic work pulling them out.

We as the AI team aren't going to be in a great position to point out if these have any usages outside the specialist finders so we'll have to fall back to your testing to verify they're not a problem for non-specialist finders or API responses from Search API.

Only other thing is that this PR is going to be pretty hard to review with all of the config removals all in the same commit. What I think might be an easier way to do this (and potentially easier to revert) would be to have this PR remove the use of the config and then have a second PR that removes the config?

@ryanb-gds
Copy link
Contributor Author

Only other thing is that this PR is going to be pretty hard to review with all of the config removals all in the same commit. What I think might be an easier way to do this (and potentially easier to revert) would be to have this PR remove the use of the config and then have a second PR that removes the config?

I did try that, but the config parser failed because of an unexpected key in the file.

@kevindew
Copy link
Member

Only other thing is that this PR is going to be pretty hard to review with all of the config removals all in the same commit. What I think might be an easier way to do this (and potentially easier to revert) would be to have this PR remove the use of the config and then have a second PR that removes the config?

I did try that, but the config parser failed because of an unexpected key in the file.

That's no worry, any chance you could break it out so the removal is a separate commit at the end so other commits can be reviewed distinctly?

@ryanb-gds ryanb-gds force-pushed the remove-expanded-search-result-fields branch 2 times, most recently from 0ef90e9 to 60147f2 Compare January 20, 2025 15:22
Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

Looks a great improvement to me, just a couple of comments on the code in the first commit to look at

lib/schema/elasticsearch_type.rb Outdated Show resolved Hide resolved
lib/schema/combined_index_schema.rb Show resolved Hide resolved
@@ -81,34 +81,11 @@ Document types are defined in the `elasticsearch_types` directory, with the
additional configuration for each type being defined by a JSON file in that
directory.

The files contain JSON object with the following keys:
The files contain a JSON object with the following key:
Copy link
Member

Choose a reason for hiding this comment

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

I guess now this file being a JSON object is probably overkill though it seems too big a can of worms to try change as part of this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so, though I must confess I haven't looked into it too closely.

Search result field expansion was used exclusively to enable human-friendly labels for metadata in Finder Frontend.

However, Finder Frontend already has access to the human friendly labels from the facet options for each field. Therefore it made sense to map the search result value to the facet option label in Finder Frontend and remove the expanded search result configuration from Search API. This enables us to reduce the amount of changes we need to make to Search API configuration when changing specialist finders.
@ryanb-gds ryanb-gds force-pushed the remove-expanded-search-result-fields branch from 60147f2 to d385c57 Compare January 21, 2025 09:54
@ryanb-gds ryanb-gds force-pushed the remove-expanded-search-result-fields branch from d385c57 to 89f4cca Compare January 21, 2025 09:59
@ryanb-gds ryanb-gds requested a review from kevindew January 21, 2025 10:02
Copy link
Member

@kevindew kevindew left a comment

Choose a reason for hiding this comment

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

Nice one

@ryanb-gds ryanb-gds merged commit 282e97d into main Jan 22, 2025
6 checks passed
@ryanb-gds ryanb-gds deleted the remove-expanded-search-result-fields branch January 22, 2025 10:07
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.

2 participants