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

Support case-insensitive search for all views #161

Merged

Conversation

qu8n
Copy link
Collaborator

@qu8n qu8n commented Sep 18, 2024

For card #1263. This PR enables case-insensitive and partial string search for all views.

Example:
demo

@qu8n qu8n force-pushed the requests-case-insensitive-search branch from 8ef9ac3 to fcefb0d Compare September 18, 2024 20:57
@qu8n qu8n changed the title Support case-insensitive and partial search for Patients page Support case-insensitive and partial search for Requests page Sep 18, 2024
@qu8n qu8n requested a review from ao508 September 18, 2024 21:01
@qu8n qu8n closed this Sep 19, 2024
@qu8n qu8n deleted the requests-case-insensitive-search branch September 19, 2024 14:07
@qu8n qu8n restored the requests-case-insensitive-search branch September 19, 2024 14:08
@qu8n qu8n reopened this Sep 19, 2024
ao508

This comment was marked as resolved.

@qu8n qu8n changed the title Support case-insensitive and partial search for Requests page Support case-insensitive and partial search Sep 19, 2024
@qu8n qu8n dismissed ao508’s stale review September 19, 2024 16:53

Removing this change request as we have discussed over Zoom. The new regex handles partial string matching and eliminates our need for _CONTAINS.

@qu8n qu8n force-pushed the requests-case-insensitive-search branch from fcefb0d to 3847c4c Compare September 19, 2024 20:40
@qu8n qu8n changed the title Support case-insensitive and partial search Support case-insensitive search for all views Sep 20, 2024
@qu8n qu8n force-pushed the requests-case-insensitive-search branch from 6e9a893 to 49f5c92 Compare October 9, 2024 14:52
@qu8n qu8n changed the base branch from dev-ignore-case-searching to master October 9, 2024 14:53
@qu8n qu8n force-pushed the requests-case-insensitive-search branch from 49f5c92 to a6024b3 Compare October 9, 2024 19:11
@qu8n qu8n changed the base branch from master to dev-ignore-case-searching October 9, 2024 19:12
@qu8n qu8n changed the base branch from dev-ignore-case-searching to dev-case-insensitive-search October 9, 2024 19:13
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Replacing all instances of predicates _IN and _CONTAINS with _MATCHES. This lets us pass in a regex that enables both case-insensitive and partial string searches.

graphql-server/src/schemas/custom.ts Show resolved Hide resolved
Comment on lines -27 to -36
const httpLink = createHttpLink({
uri: "https://localhost:4000/graphql",
fetch: fetch,
});

const client = new ApolloClient({
link: httpLink,
cache: new InMemoryCache(),
});

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Removed as these are not being used. (These were used previously to query some samples data as part of the old sample mutation workflow.)

Comment on lines +54 to +59
filters: {
String: {
MATCHES: true,
},
},
};
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

FYI I removed ID: true inside of String: { ... }. That config would let us perform RegEx matching on values of type ID (source), which is a use case that we don't have.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't ID something we were interested in accessing?

Copy link
Collaborator Author

@qu8n qu8n Oct 10, 2024

Choose a reason for hiding this comment

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

Yes, we are interested in accessing the ID, but setting ID: true here does something else. The docs link I shared above says it enables RegEx matching for value of type ID. For example, if we wanted to search for internal ID containing value "abc". Maybe I missed something there?

@qu8n qu8n marked this pull request as ready for review October 9, 2024 19:56
@qu8n qu8n requested a review from ao508 October 9, 2024 19:56
Copy link
Collaborator

@ao508 ao508 left a comment

Choose a reason for hiding this comment

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

Just a little confused about adding the ID support. Does that not also expose that field for us?

Comment on lines +54 to +59
filters: {
String: {
MATCHES: true,
},
},
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't ID something we were interested in accessing?

@ao508 ao508 merged commit 4b8524a into mskcc:dev-case-insensitive-search Oct 10, 2024
2 checks passed
@qu8n qu8n deleted the requests-case-insensitive-search branch November 14, 2024 21:52
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