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.

Sign up for GitHub

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

fix: fixes datepicker issue within dataviz (M2-7348) #1987

Draft
wants to merge 4 commits into
base: develop
Choose a base branch
from
Draft

fix: fixes datepicker issue within dataviz (M2-7348) #1987

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
2455d45
fix: fixes calendar issue with utc conversion within dataviz
Dec 4, 2024
49d6a2d
fix: add optional activityId to AppletSubmitDateList and update date …
Dec 10, 2024
2574eef
fix: rename activityId to activityOrFlowId in AppletSubmitDateList an…
Dec 10, 2024
c7fd245
fix: refactor useEffect to handle selectedActivity changes in Respond…
Dec 11, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/modules/Dashboard/api/api.types.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -505,6 +505,7 @@ export type AppletSubmitDateList = AppletId &
TargetSubjectId & {
fromDate: string;
toDate: string;
activityOrFlowId?: string;
};

export type EventId = { eventId: string };
Expand Down
7 changes: 5 additions & 2 deletions src/modules/Dashboard/features/RespondentData/RespondentDataReview/RespondentDataReview.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,12 +143,14 @@ export const RespondentDataReview = () => {
const datesResult = data?.result;
if (!datesResult) return;

// Map each date string to a Date object, appending 'T00:00:00' to remove timezone offset
const submitDates = datesResult.dates.map((date: string) => new Date(`${date}T00:00:00`));

setResponseDates(submitDates);
});

const handleGetSubmitDates = (date: Date) => {
if (!appletId || !subjectId) return;
if (!appletId || !subjectId || !selectedActivity?.id) return;
Copy link
Contributor

@farmerpaul farmerpaul Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We don't actually need activity ID to be defined as it's an optional parameter to getAppletSubmitDateList (but open to hearing your thoughts).

Suggested change
if (!appletId || !subjectId || !selectedActivity?.id) return;
if (!appletId || !subjectId) return;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was necessary to make sure the method in the api is being called with a non-empty value for activityOrFlowId @farmerpaul

Copy link
Contributor

@farmerpaul farmerpaul Dec 11, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I'm not sure we should be restricting the handleGetSubmitDates to only instances of this screen where an activity has been selected. The RespondentDataReview screen is accessible in the app from several routes, including a route that doesn't include either an activity ID or flow ID as a parameter:

mindlogger-admin/src/modules/Dashboard/routes/routes.tsx

Lines 136 to 147 in c1ba765

<Route path={page.appletParticipantDataReview} element={<RespondentDataReview />}>
<Route
path={page.appletParticipantDataReview}
element={
<PrivateRoute>
<ErrorBoundary FallbackComponent={ErrorFallback}>
<RespondentDataReview />
</ErrorBoundary>
</PrivateRoute>
}
/>
</Route>

mindlogger-admin/src/resources/pages.json

Line 27 in a55547a

"appletParticipantDataReview": "/dashboard/:appletId/participants/:subjectId/dataviz/responses",

So it's still possible to invoke this component where activity ID or activity flow ID have not been defined. I was able to navigate to this URL after checking out your branch:

https://localhost:3000/dashboard/8e9bd4f9-7ea3-4cfd-b6b1-040108774370/participants/83b7ff36-2175-4fd0-80c7-bc9f1d08ddee/dataviz/responses

And because no default activity was selected, the handleGetSubmitDates callback was bypassed due to this condition, and resulted in a datepicker showing all dates as being available:

Now, whether the app is even supposed to still be calling this route is a separate question (I think it's deprecated), one which I think I need to bring up with product. But I think it's still accessible from a few spots, so we should probably make sure not to affect the datepicker when those routes are still being called.

So for now, I think I stand by my original suggestion of not changing this line:

Suggested change
if (!appletId || !subjectId || !selectedActivity?.id) return;
if (!appletId || !subjectId) return;


const fromDate = startOfMonth(date).getTime().toString();
const toDate = endOfMonth(date).getTime().toString();
Expand All @@ -158,6 +160,7 @@ export const RespondentDataReview = () => {
targetSubjectId: subjectId,
fromDate,
toDate,
activityOrFlowId: selectedActivity?.id || selectedFlow?.id,
});
};

Expand Down Expand Up @@ -251,7 +254,7 @@ export const RespondentDataReview = () => {

handleSetInitialDate(new Date());
// eslint-disable-next-line react-hooks/exhaustive-deps
}, [lastActivityCompleted]);
}, [lastActivityCompleted, selectedActivity?.id]);
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm assuming you discovered you needed to trigger the effect when the activity ID becomes available, but shouldn't that also be true for the flow ID?

Suggested change
}, [lastActivityCompleted, selectedActivity?.id]);
}, [lastActivityCompleted, selectedActivity?.id, selectedFlow?.id]);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🎯 @farmerpaul


/**
* Determines the source subject based on the presence of activity responses.
Expand Down
Loading