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

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

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

Conversation

ramirlm
Copy link
Contributor

@ramirlm ramirlm commented Dec 5, 2024

📝 Description

🔗 Jira Ticket M2-7348

Dates without responses were being auto-selected, and the actual response day could not be selected. This behavior is being fixed and now the view loads the days correctly.

Changes include:

  • Fixes for the calendar picker on Dataviz, interpolating a UTC 00:00 time into the date in order to avoid gaps

🪤 Peer Testing

  • Create an applet, and add responses to it

  • Open the participant view, click on responses

  • Check if the responses are showing on the correct day

  • Click on any response, and check if it opens correctly

    Expected outcome: The responses are loading on the correct days, and the days without response are not being able to be selected

    The updated code only shows selectable options where the limited user has inputted answers for that activity, for that subject:

CleanShot 2024-12-10 at 08 58 57@2x

✏️ Notes

  • Backend changes for this task were not necessary

Copy link

This pull request is automatically being deployed by Amplify Hosting (learn more).

Access this pull request here: https://pr-1987.d19gtpld8yi51u.amplifyapp.com

@farmerpaul farmerpaul self-requested a review December 5, 2024 12:29
@ramirlm ramirlm marked this pull request as draft December 6, 2024 16:21
@ramirlm ramirlm requested a review from ChaconC December 10, 2024 12:01
Ramir Mesquita added 2 commits December 10, 2024 14:10
setResponseDates(submitDates);
});

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

@farmerpaul farmerpaul Dec 11, 2024

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

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:

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

"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;

@@ -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.

@ramirlm
Copy link
Contributor Author

ramirlm commented Dec 11, 2024

@farmerpaul FYI still ongoing fixing this – I appreciate the insights though!

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.

3 participants