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

(1751) Multi-faceted search #1153

Merged
merged 2 commits into from
Apr 10, 2024
Merged

(1751) Multi-faceted search #1153

merged 2 commits into from
Apr 10, 2024

Conversation

adam-murray
Copy link
Contributor

@adam-murray adam-murray commented Mar 27, 2024

Depends on:

nationalarchives/ds-caselaw-marklogic#58
nationalarchives/ds-caselaw-custom-api-client#575
#1169

Changes in this PR:

This PR implements handling for court and year search facets.

Trello card / Rollbar error (etc)

https://trello.com/c/XMzUM1jX/1751-🏠🎛-homepage2-faceted-search-concept-on-staging-for-testing-courtyear

Screenshots of UI changes:

Before

image

After

image

  • Requires env variable(s) to be updated

Copy link
Contributor

@CristinaRO CristinaRO left a comment

Choose a reason for hiding this comment

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

A few nitpicky comments and typos, shouldn't be blocking.

@CristinaRO
Copy link
Contributor

@CristinaRO CristinaRO marked this pull request as ready for review April 8, 2024 15:58
@CristinaRO CristinaRO force-pushed the 1751-faceted-search branch from 53f9b68 to 2dc17e7 Compare April 8, 2024 16:16
Generate a list of valid years as strings.
"""
today = datetime.date.today()
valid_years = range(2003, today.year)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will probably bite us when we import older stuff.

"""
Sorts a dictionary by value where
the values contain numbers as strings
"""
Copy link
Collaborator

Choose a reason for hiding this comment

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

Does suggest we might just want to make those values ints early in the process.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree to do this in future, maybe alongside changes to preserve the facet name?

Copy link
Collaborator

Choose a reason for hiding this comment

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

yep, that's fine.

Separates facets dict into non-court facets,
and court facets.
"""
court_facets = {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Already discussed that we want to get court/year out of the original data, but we still want the validation. Okay for now.

@adam-murray adam-murray force-pushed the 1751-faceted-search branch from d98a168 to f21c0a7 Compare April 9, 2024 11:17
@CristinaRO CristinaRO force-pushed the 1751-faceted-search branch 3 times, most recently from 2d0df0c to 5646185 Compare April 9, 2024 15:03
@CristinaRO CristinaRO changed the title 1751 faceted search (1751) Multi-faceted search Apr 9, 2024
# Desired court_facet is present
assert response.context["context"]["court_facets"] == {eat_court_code: "3"}
# Blank keys are not present
assert {"": "3"}.items() not in response.context["context"][
Copy link
Collaborator

@dragon-dxw dragon-dxw Apr 9, 2024

Choose a reason for hiding this comment

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

I'd probably write assert "" not in complicated_dict.keys(); this currently will always pass because the value of "" is 5.

"court_facets"
].items()
# Keys that don't match existing courts are not present
assert {"invalid_court": "10"}.items() not in response.context["context"][
Copy link
Collaborator

Choose a reason for hiding this comment

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

ditto except the number is fine

# Desired year_facet is present
assert response.context["context"]["year_facets"] == {"2010": "103"}
# Keys that don't match valid years are not present
assert {"1900": "4"}.items() not in response.context["context"][
Copy link
Collaborator

Choose a reason for hiding this comment

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

similarly here.

adam-murray and others added 2 commits April 10, 2024 15:04
Get more court data into the facet context

Rather than assume a very generic, meta-programmed approach to multiple
facets in the future, I am encoding a more specific approach to the only
facet we have so far, the court facet.

Including data about the court will allow us to get its human-friendly
display name, as well as the code used to formulate the query.

Display relevant courts as links

We are making a fair number of assumptions here:
- the link only preserves the query text
- therefore any other filters that may have been selected are lost

fix: Separate processing of court facets into util, do not render
relevant facets template unless facets are provided!

Add year facets to interface

Replace year query with year facet

Years selected from the facets take precedence over any previously
applied filters.

feat: Added template for rendering facets

Put faceted search behind feature flag

Use latest release of the API client

Co-authored-by: adam-murray <[email protected]>
Co-authored-by: dragon-dxw <[email protected]>
Signed-off-by: Cristina <[email protected]>
Co-authored-by: adam-murray <[email protected]>
Signed-off-by: Cristina <[email protected]>
@CristinaRO CristinaRO force-pushed the 1751-faceted-search branch from 5646185 to a64be4a Compare April 10, 2024 14:05
@CristinaRO CristinaRO added this pull request to the merge queue Apr 10, 2024
Merged via the queue into main with commit 43465ed Apr 10, 2024
10 of 11 checks passed
@CristinaRO CristinaRO deleted the 1751-faceted-search branch April 10, 2024 14:12
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