-
Notifications
You must be signed in to change notification settings - Fork 256
TICKET-614: Add case sensitivy to group search #7938
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
base: master
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -110,6 +110,63 @@ export function textFilter({filter, onChange, column}) { | |
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Locale-aware substring match with optional case sensitivity. Both arguments | ||
| * are coerced to strings, so callers do not have to guard against null/undefined. | ||
| */ | ||
| export function caseSensitiveIncludes(haystack, needle, caseSensitive) { | ||
| const a = String(haystack); | ||
| const b = String(needle); | ||
| if (caseSensitive) return a.includes(b); | ||
| return a.toLocaleLowerCase().includes(b.toLocaleLowerCase()); | ||
| } | ||
|
|
||
| /** | ||
| * Builds a filterMethod that matches `row[filter.id]` against `filter.value` | ||
| * with the given case sensitivity. Empty filters match every row. | ||
| */ | ||
| export function caseSensitiveStringFilterMethod(caseSensitive) { | ||
| return (filter, row) => { | ||
| if (!filter.value) return true; | ||
| return caseSensitiveIncludes(row[filter.id], filter.value, caseSensitive); | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
| * Builds a Filter component that pairs a text input with an "Aa" checkbox | ||
| * toggle for case-sensitive matching. `getCaseSensitive` is read on every | ||
| * render so the checkbox reflects the latest value without rebuilding the | ||
| * Filter — this keeps the rendered <input> elements stable across re-renders. | ||
| */ | ||
| export function caseSensitiveTextFilter({getCaseSensitive, onToggle}) { | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Okay so looking at the overall code changes, I think we can simplify greatly by keeping the toggled " So you should be able to do something like export function caseSensitiveTextFilter({onChange, column}) => {
const { filterValue, setFilterValue } = React.useState("");
const { caseSensitive, setCaseSensitive } = React.useState(false);
React.useEffect(() => {
// Key line
onChange({filterValue: filterValue, caseSensitive: caseSensitive});
}, [filterValue, caseSensitive]);
return (
// The same, except the input element onChange callbacks call setFilterValue/setCaseSensitive
...
);
});Then you can modify the |
||
| return ({filter, onChange, column}) => ( | ||
| <div style={{display: "flex", alignItems: "center", gap: "4px"}}> | ||
| <input | ||
| type="text" | ||
| style={{flex: 1, minWidth: 0}} | ||
| value={filter ? filter.value : ""} | ||
| aria-label={`${I18n.t("search")} ${column.Header || ""}`} | ||
| onChange={event => onChange(event.target.value)} | ||
| /> | ||
| <label | ||
| title={I18n.t("table.case_sensitive_search")} | ||
| style={{display: "flex", alignItems: "center", cursor: "pointer"}} | ||
| > | ||
| <input | ||
| type="checkbox" | ||
| checked={getCaseSensitive()} | ||
| onChange={onToggle} | ||
| aria-label={I18n.t("table.case_sensitive_search")} | ||
| data-testid={`${column.id}_case_sensitive`} | ||
| /> | ||
| <span style={{fontSize: "1.05em", marginLeft: "2px"}}> | ||
| {I18n.t("table.case_sensitive_indicator")} | ||
| </span> | ||
| </label> | ||
| </div> | ||
| ); | ||
| } | ||
|
|
||
| /** | ||
| * Select-based search filter. Options are generated from the custom column attribute | ||
| * filterOptions, which is a list of objects with keys "value" and "text". | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,75 @@ | ||
| /*** | ||
| * Tests for AnnotationUsagePanel Component | ||
| */ | ||
|
|
||
| import {AnnotationUsagePanel} from "../annotation_usage_panel"; | ||
| import {render, screen, fireEvent} from "@testing-library/react"; | ||
|
|
||
| jest.mock("@fortawesome/react-fontawesome", () => ({ | ||
| FontAwesomeIcon: () => null, | ||
| })); | ||
|
|
||
| describe("For the AnnotationUsagePanel's group name search", () => { | ||
| beforeEach(async () => { | ||
| const applications = [ | ||
| { | ||
| result_id: 1, | ||
| user_name: "alice", | ||
| first_name: "Alice", | ||
| last_name: "First", | ||
| group_name: "Alpha_001", | ||
| count: 1, | ||
| }, | ||
| { | ||
| result_id: 2, | ||
| user_name: "bob", | ||
| first_name: "Bob", | ||
| last_name: "Second", | ||
| group_name: "alpha_002", | ||
| count: 1, | ||
| }, | ||
| { | ||
| result_id: 3, | ||
| user_name: "carol", | ||
| first_name: "Carol", | ||
| last_name: "Third", | ||
| group_name: "Beta_003", | ||
| count: 1, | ||
| }, | ||
| ]; | ||
| fetch.mockReset(); | ||
| fetch.mockResolvedValueOnce({ | ||
| ok: true, | ||
| json: jest.fn().mockResolvedValueOnce(applications), | ||
| }); | ||
|
|
||
| render(<AnnotationUsagePanel course_id={1} assignment_id={1} annotation_id={1} num_used={3} />); | ||
|
|
||
| fireEvent.click(screen.getByText(I18n.t("annotations.usage"))); | ||
| await screen.findByText("(alice) Alice First"); | ||
| }); | ||
|
|
||
| it("is case-sensitive by default", () => { | ||
| const groupSearch = screen.getByRole("textbox", { | ||
| name: `${I18n.t("search")} ${I18n.t("activerecord.models.submission.one")}`, | ||
| }); | ||
| fireEvent.change(groupSearch, {target: {value: "Alpha"}}); | ||
|
|
||
| expect(screen.getByText("(alice) Alice First")).toBeInTheDocument(); | ||
| expect(screen.queryByText("(bob) Bob Second")).not.toBeInTheDocument(); | ||
| expect(screen.queryByText("(carol) Carol Third")).not.toBeInTheDocument(); | ||
| }); | ||
|
|
||
| it("becomes case-insensitive when the toggle is unchecked", () => { | ||
| fireEvent.click(screen.getByTestId("group_name_case_sensitive")); | ||
|
|
||
| const groupSearch = screen.getByRole("textbox", { | ||
| name: `${I18n.t("search")} ${I18n.t("activerecord.models.submission.one")}`, | ||
| }); | ||
| fireEvent.change(groupSearch, {target: {value: "alpha"}}); | ||
|
|
||
| expect(screen.getByText("(alice) Alice First")).toBeInTheDocument(); | ||
| expect(screen.getByText("(bob) Bob Second")).toBeInTheDocument(); | ||
| expect(screen.queryByText("(carol) Carol Third")).not.toBeInTheDocument(); | ||
| }); | ||
| }); |
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.
I appreciate the coercion, but for
nullandundefined:String(null) === "null"andString(undefined) === "undefined", which I do not think are appropriate values for this function. I'd be happy to have them "coerced" into the empty string"".