-
Notifications
You must be signed in to change notification settings - Fork 2
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
feat!: Add substring search support for more fields #3028
Conversation
From the preview it looks like it's working. |
Cool - code looks nice to me. This is a config-breaking change so we should include |
Ah, we should potentially call this "substring" search, at least from the POV of the website |
I think @corneliusroemer will have stronger view than me on calling it substring search and might have suggestions for how he thinks the config should look (sometimes it is regex - i.e. on the LAPIS side) |
I edited the PR title |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As @theosanderson said, this is substring search not regex search (we're using the LAPIS regex feature but this feature here crafts a regex that does substring search only, not general regex)
a544419
to
d87f112
Compare
Ahh, I didn't understand it at first, but now I got it. I made a change, that should fix what you meant right? In the website config the property is now called 'substringSearch' (akin to rangeSearch) and in the values.yaml it's 'enableSubstringSearch' (like generateIndex, hideOnSequenceDetailsPage) |
Ignore, seems to be general instability of previews right now due to too much load |
d87f112
to
17c0f91
Compare
made the changes. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Works and looks good to me!
0a1909f
to
6a47777
Compare
If possible please do ping somewhere on slack when this goes live so I can be sure to add it to our running list of new items under technical improvements :) |
@emmahodcroft, this PR only adds the feature to support more substring fields to Loculus. Afterwards, we still have to define the fields for Pathoplexus: pathoplexus/pathoplexus#79 - You and Cornelius named already a few fields but do you have more in mind? Would be good to have a complete list. |
I think that the review is now outdated.
resolves #2684
preview URL: https://feat-2684-substring-searc.loculus.org
Summary
Add support for regex search for more fields, by making regex-searchability configurable in the Helm chart values.yaml.
Support is enabled for submissionId, authors, authorAffiliations, specimenCollectorSampleId
Screenshot
TODO
PR Checklist