Filter by Date Pt. 2 - Custom date selection #3117

angelathe · 2025-01-09T16:22:48Z

PULL REQUEST

Summary

Add custom date selection options when filtering by date
- Max date set to today
- Start date time set to 00:00:00.000, and end date time set to 23:59:59.999
Add/update unit and snapshot tests

Related Issue

Additional Information

NOTE: Still looking to clean some things up (left a few comments), but would love to start the feedback process now.

Things that will probably get broken out into other tickets:

Apply Filter button disabled/enabled states - button should
Adding a custom error tooltip for when startDate > endDate. Right now, there's a placeholder Alert for this error

Acceptance Criteria

"Filter By Received Date" implemented for custom date range
When a user chooses "Custom date range", they must populate the start and end dates
When user applies the custom filter, only eCRs received in that date range are included
My selection is shown as the button text (e.g. "From 10/7/2024 to 10/10/2024")

Checklist

If this code affects the other scrum team, have they been notified? (In Slack, as reviewers, etc.)

…ate > end date

containers/ecr-viewer/src/app/components/BaseFilter.tsx

containers/ecr-viewer/src/app/components/Filters.tsx

angelathe · 2025-01-09T16:31:41Z

containers/ecr-viewer/src/app/components/Filters.tsx

+      if (new Date(startDate) > new Date(endDate)) {
+        alert(
+          "Invalid date range. Start date must be earlier or equal to the end date.",
+        );
+        resetFilterDate();
+        return;
+      }


Note: This is temporary -- a placeholder alert to safeguard against error where Start date > end date. Still working on how to make the custom error tooltip for this / might get brought out into a separate small ticket

codecov · 2025-01-09T17:58:25Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 85.67%. Comparing base (2055a30) to head (5604ed3).
Report is 1 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3117      +/-   ##
==========================================
- Coverage   87.05%   85.67%   -1.39%     
==========================================
  Files         219       25     -194     
  Lines       13652     1424   -12228     
  Branches      709        0     -709     
==========================================
- Hits        11885     1220   -10665     
+ Misses       1758      204    -1554     
+ Partials        9        0       -9

Flag	Coverage Δ
ecr-viewer	`?`
fhir-converter	`?`
ingestion	`?`
message-parser	`?`
message-refiner	`?`
orchestration	`85.67% <ø> (ø)`
record-linkage	`?`
trigger-code-reference	`?`
validation	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

see 194 files with indirect coverage changes

mcmcgrath13 · 2025-01-10T16:48:26Z

containers/ecr-viewer/src/app/components/Filters.tsx

@@ -77,7 +88,7 @@ const Filters = () => {
    }
  }, [filterBoxOpen]);

-  const paramKeys = ["condition", "dateRange"];
+  const paramKeys = [ParamName.Condition, ParamName.DateRange, ParamName.Dates];


can this instead be computed from the enum? (is a formal enum worth it or just use a plain object?)

Just made an update!

(Maybe an enum is overkill, but thought it would make things clearer in case we expand the scope of filters?)

mcmcgrath13 · 2025-01-10T16:50:16Z

containers/ecr-viewer/src/app/components/Filters.tsx

@@ -124,7 +135,8 @@ const Filters = () => {
 * - Updates the browser's query string when the filter is applied.
 */
 const FilterReportableConditions = () => {
-  const { searchParams, updateQueryParam } = useQueryParam();
+  const { searchParams, updateQueryParam, pushQueryUpdate } = useQueryParam();
+  const params = new URLSearchParams(searchParams.toString());


why do we need to do this copy?

Needed to pass params to updateQueryParam in the submitHandler (can't pass searchParams which is read-only)

Could move Line 139 into the submitHandler so it's more clear why that's needed?

I'd move this into updateQueryParams so it always returns a new params and doesn't mutate. Especially since mutating state can be such a footgun, best to avoid it entirely by default

mcmcgrath13 · 2025-01-10T16:58:22Z

containers/ecr-viewer/src/app/components/Filters.tsx

+
+  const submitHandler = () => {
+    const params = new URLSearchParams(searchParams.toString());
+    if (filterDateOption === "custom") {


what were the pros/cons/thinking on the two param approach vs a single param like custom(start,end)?

Sorry, missed this comment on Friday! My thoughts are below, and I'm definitely open to changing the approach.

I went with two URL parameters to keep things simpler. One shows that it’s a custom date, and the other holds the actual dates. Keeping these definitions separate means we don’t have to deal with extra parsing every time we just need one of them (felt cleaner and easier to work with this approach). Also, having two separate parameters helps keep the radio button option name and the URL param values in sync, which makes it simpler to match what the user picks to what’s in the query string.

Also, keeping custom dates in their own parameter seemed like it would make it easier to extend things in the future—like if we wanted to add time ranges. Saves us from having to dig through and parse a single dateRange parameter if we need to add more customizations down the line.

The main con with maintaining the two parameters is maintaining consistency between dateRange and dates, i.e. making sure that dates only exists and is processed when dateRange=custom, and that dates does not exist when dateRange is set to one of the other predefined date ranges. Using the single dateRange parameter also to house the custom dates would eliminate this risk.

Thoughts?

That matches what I see as well. We just need to make sure we've got robust handling of the keeping in sync and edge cases there

mcmcgrath13 · 2025-01-10T18:27:30Z

containers/ecr-viewer/src/app/components/BaseFilter.tsx

+          type="date"
+          className="usa-input width-card margin-top-0 border-base-dark custom-date"
+          defaultValue={endDate || ""}
+          required


should we require an end date? or should it be implicitly today if not selected?

Hm, I think this is a question for @ashton-skylight.

Do you prefer that 1) end date is always required, i.e. folks would have to manually set today as the end date, or 2) end date is not required, i.e. if users don't fill in the end date, the end date would implicitly be set as today?

@angelathe Since case investigation will usually take place with more recent cases, end date shouldn't necessarily be required. Let's go with 2, if the user does not manually set an end date, the end date is implicitly set as today, assuming that would reflect in the custom date range inputs after the user presses "apply filter".

Ok! Just making a note this slightly changes the acceptance criteria / scenario of the original ticket

…Cgov/phdi into angela/2753-filter-by-date-p2

mcmcgrath13 · 2025-01-13T16:52:00Z

containers/ecr-viewer/src/app/components/Filters.tsx

@@ -286,52 +309,108 @@ const FilterByDate = () => {
  };

  const resetFilterDate = () => {


I think there's a bug here where if you have a custom date range, then hit the reset button, then go back to the custom date range radio, the old custom dates are still there. Should those properly reset? Maybe just adding the setStartDate and end date to null in the default case?

mcmcgrath13 · 2025-01-13T16:53:39Z

containers/ecr-viewer/src/app/components/Filters.tsx

@@ -286,52 +309,108 @@ const FilterByDate = () => {
  };

  const resetFilterDate = () => {
-    const queryDateRange = searchParams.get("dateRange");
+    const queryDateRange = searchParams.get(ParamName.DateRange);


if a user manually deletes the dates param, it ends up looking like this

should we reset to the default instead at this point? cc @ashton-skylight

mcmcgrath13 · 2025-01-13T16:59:56Z

containers/ecr-viewer/src/app/components/Filters.tsx

+        updatedParams,
+        ParamName.Dates,
+        datesParam,
+        false,


maybe add false as the default on the function signature so we can skip this here?

mcmcgrath13 · 2025-01-13T17:00:29Z

containers/ecr-viewer/src/app/components/Filters.tsx

+      let updatedParams: URLSearchParams;
+      updatedParams = updateQueryParam(


Suggested change

let updatedParams: URLSearchParams;

updatedParams = updateQueryParam(

let updatedParams = updateQueryParam(

mcmcgrath13 · 2025-01-13T17:10:40Z

containers/ecr-viewer/src/app/components/BaseFilter.tsx

+  return (
+    <div className="padding-x-105">
+      <div>
+        <Label htmlFor="start-date" className="margin-top-1">


is it worth abstracting out these date inputs? there's a lot of overlap between the two

or maybe make the CustomDateInput just do one input, then have the CustomDateRange handle both or at that point just inline it into the date filter jsx

mcmcgrath13 · 2025-01-13T17:16:26Z

containers/ecr-viewer/src/app/components/Filters.tsx

    >
      <div className="display-flex flex-column">
        {Object.values(DateRangeOptions).map((option) => (
-          <div
-            className="checkbox-color usa-radio padding-bottom-1 padding-x-105"
+          <RadioDateOption


maybe move this abstraction up one level to RadioDateOptions? there's still a bit of "guts" in here that could have a nicer API if it handles the options bit. Then it could just pass what are all the options handler for option change and current option, which is pretty universal to any set of radio buttons

mcmcgrath13 · 2025-01-13T17:23:46Z

containers/ecr-viewer/src/app/page.tsx

@@ -68,7 +80,7 @@ const HomePage = async ({
              sortDirection={sortDirection}
              searchTerm={searchTerm}
              filterConditions={filterConditionsArr}
-              filterDate={filterDateRange}
+              filterDate={filterDates}


minor nit - pluralization doesn't match

mcmcgrath13 · 2025-01-13T17:26:10Z

containers/ecr-viewer/src/app/page.tsx

+  if (filterDateRange === "custom") {
+    const datesParam = searchParams?.dates as string | undefined;
+    if (datesParam) {
+      filterDates = buildCustomDateRange(datesParam);


if the dates param has junk content, we get a system internal error, can we harden this?

mcmcgrath13 · 2025-01-13T17:27:21Z

containers/ecr-viewer/src/app/page.tsx

+  const filterDateRange =
+    (searchParams?.dateRange as string) || DEFAULT_DATE_RANGE;
+  let filterDates;
+
+  if (filterDateRange === "custom") {


this logic feels a bit out of place here as it's likely something we'll want to sync with the component itself. Can we think about making this a bit more centralized then using that logic both here and in the UI component?

angelathe added 11 commits January 7, 2025 15:04

wip

2649c0d

Display custom dates in filter date button when custom date selected

c2a25ce

maintain state of custom dates by reading URL params

e611290

set max value for start/end dates to today

d92a516

move buildCustomDateRange logic to date-utils

5c1b5a7

fix reset state bugs when pressing clear on calendar and when start d…

64a27aa

…ate > end date

add JSdocs

016a23a

remove unused css, small fix in custom date button display

61cbc3c

Add key to remove console warnings

e718ac3

update snapshot tests

bcb4606

Merge branch 'main' into angela/2753-filter-by-date-p2

39fd3e8

angelathe self-assigned this Jan 9, 2025

angelathe commented Jan 9, 2025

View reviewed changes

containers/ecr-viewer/src/app/components/BaseFilter.tsx Outdated Show resolved Hide resolved

angelathe commented Jan 9, 2025

View reviewed changes

containers/ecr-viewer/src/app/components/Filters.tsx Outdated Show resolved Hide resolved

angelathe commented Jan 9, 2025

View reviewed changes

angelathe added 4 commits January 9, 2025 12:42

add dates to list of reset params

d634cc4

clean up deleteQueryParam

af0c329

add filter param names to an enum for safety/consistency

343c944

wip, refactoring param update functions

22d56d1

angelathe marked this pull request as ready for review January 10, 2025 15:28

angelathe requested review from akasper, austin-hall-skylight, BobanL, gordonfarrell, JNygaard-Skylight, lina-roth, mcmcgrath13 and robertmitchellv as code owners January 10, 2025 15:28

mock date for custom dates snapshot test

643400c

mcmcgrath13 reviewed Jan 10, 2025

View reviewed changes

angelathe added 3 commits January 10, 2025 10:59

replace submit with clicking apply button

ec9a5fa

small update to paramKeys

0b0792e

Merge branch 'main' into angela/2753-filter-by-date-p2

432bd1e

mcmcgrath13 reviewed Jan 10, 2025

View reviewed changes

angelathe added 2 commits January 10, 2025 12:52

abstract out RadioDateOption, and update snapshot tests

0902cc2

Merge branch 'angela/2753-filter-by-date-p2' of https://github.com/CD…

8f54b1a

…Cgov/phdi into angela/2753-filter-by-date-p2

mcmcgrath13 reviewed Jan 13, 2025

View reviewed changes

end date no longer required, defaults to today

5604ed3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Filter by Date Pt. 2 - Custom date selection #3117

Filter by Date Pt. 2 - Custom date selection #3117

angelathe commented Jan 9, 2025

angelathe Jan 9, 2025 •

edited

Loading

codecov bot commented Jan 9, 2025 •

edited

Loading

mcmcgrath13 Jan 10, 2025

angelathe Jan 10, 2025

mcmcgrath13 Jan 10, 2025

angelathe Jan 10, 2025

mcmcgrath13 Jan 13, 2025

mcmcgrath13 Jan 10, 2025

angelathe Jan 13, 2025

mcmcgrath13 Jan 13, 2025

mcmcgrath13 Jan 10, 2025

angelathe Jan 10, 2025

ashton-skylight Jan 10, 2025

angelathe Jan 10, 2025

mcmcgrath13 Jan 13, 2025

mcmcgrath13 Jan 13, 2025

mcmcgrath13 Jan 13, 2025

mcmcgrath13 Jan 13, 2025

mcmcgrath13 Jan 13, 2025

mcmcgrath13 Jan 13, 2025

mcmcgrath13 Jan 13, 2025

mcmcgrath13 Jan 13, 2025

mcmcgrath13 Jan 13, 2025

mcmcgrath13 Jan 13, 2025

		@@ -286,52 +309,108 @@ const FilterByDate = () => {
		};

		const resetFilterDate = () => {

		let updatedParams: URLSearchParams;
		updatedParams = updateQueryParam(

	let updatedParams: URLSearchParams;
	updatedParams = updateQueryParam(
	let updatedParams = updateQueryParam(

Filter by Date Pt. 2 - Custom date selection #3117

Are you sure you want to change the base?

Filter by Date Pt. 2 - Custom date selection #3117

Conversation

angelathe commented Jan 9, 2025

PULL REQUEST

Summary

Related Issue

Additional Information

Acceptance Criteria

Checklist

angelathe Jan 9, 2025 • edited Loading

Choose a reason for hiding this comment

codecov bot commented Jan 9, 2025 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

angelathe Jan 9, 2025 •

edited

Loading

codecov bot commented Jan 9, 2025 •

edited

Loading