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) O3-4286: Fix error when starting a visit immediately after ending one #2175

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
+7 −3
Open

(fix) O3-4286: Fix error when starting a visit immediately after ending one #2175

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
bcae102
(fix) O3-4286 Fix error when starting a visit immediately after endin…
Muppasanipraneeth Jan 10, 2025
23e502d
updated the stopDatetime
Muppasanipraneeth Jan 10, 2025
111e603
modified the validation for future start time
Muppasanipraneeth Jan 10, 2025
f528fdd
corrected end payload
Muppasanipraneeth Jan 11, 2025
8e35f04
removed miliseconds in payload
Muppasanipraneeth Jan 11, 2025
3ea84ab
renamed vist-form.component to workspace
Muppasanipraneeth Jan 20, 2025
b02565b
Merge branch 'main' into O3-4286-fix-error-when-starting-a-visit-imme…
Muppasanipraneeth Jan 20, 2025
52ef729
Update packages/esm-patient-chart-app/src/visit/visit-form/visit-form…
ibacher Jan 21, 2025
53a05e4
Merge branch 'openmrs:main' into O3-4286-fix-error-when-starting-a-vi…
Muppasanipraneeth Jan 25, 2025
22caafa
Merge branch 'openmrs:main' into O3-4286-fix-error-when-starting-a-vi…
Muppasanipraneeth Jan 31, 2025
747eee4
use current seconds for visit start time payload
Muppasanipraneeth Jan 31, 2025
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 packages/esm-patient-chart-app/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@ import patientChartPageComponent from './root.component';
import patientDetailsTileComponent from './patient-details-tile/patient-details-tile.component';
import startVisitActionButtonComponent from './actions-buttons/start-visit.component';
import startVisitActionButtonOnPatientSearch from './visit/start-visit-button.component';
import startVisitFormComponent from './visit/visit-form/visit-form.workspace';
Copy link
Member

Choose a reason for hiding this comment

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

Why are we adding this import?

Copy link
Author

@Muppasanipraneeth Muppasanipraneeth Jan 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Screenshot 2025-01-22 at 3 01 18 PM

import stopVisitActionButtonComponent from './actions-buttons/stop-visit.component';
import visitAttributeTagsComponent from './patient-banner-tags/visit-attribute-tags.component';

Expand Down
9 changes: 6 additions & 3 deletions packages/esm-patient-chart-app/src/visit/visit-form/visit-form.workspace.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,9 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({
// Validates that the start time is not in the future
const validateStartTime = (data: z.infer<typeof visitFormSchema>) => {
const [visitStartHours, visitStartMinutes] = convertTime12to24(data.visitStartTime, data.visitStartTimeFormat);
const visitStartDatetime = new Date(data.visitStartDate).setHours(visitStartHours, visitStartMinutes);
return new Date(visitStartDatetime) <= new Date();
const visitStartDatetime = new Date(data.visitStartDate);
visitStartDatetime.setHours(visitStartHours, visitStartMinutes, 0, 0);
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure that zeroing out seconds is always the right thing to do here. Can you explain why we're doing so?

Copy link
Author

Choose a reason for hiding this comment

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

Explanation of Validation Logic

This change only affects the comparison logic, not the datetime storage format.

  • If you observe the attached screenshot, even though the time appears correct, a validation error is thrown:
    "Visit start time cannot be in the future."

  • After investigation, it was found that even when the current time is correctly set, the error occurs due to how seconds are handled in the comparison logic.

Example Scenario

  1. Form-Filling Delay:

    • Assume it took 1 minute to fill out the form.
    • The start visit time is set to 7:21:30.
    • By the time the form is submitted, the current time has advanced to 7:22:10.

  2. Time Adjustment:

    • If the user updates the minute to 7:22:30, the seconds component does not automatically adjust.

  3. Validation Failure:

    • Validation logic compares:
      • 7:22:30 (start visit) <= 7:22:10 (current time) → False
      • This results in the error: "Visit start time cannot be in the future."

  4. Correct Validation After Zeroing Seconds:

    • When seconds are zeroed out:
      • 7:22:0 (start visit) <= 7:22:10 (current time) → True
    • This ensures that the validation focuses on hours and minutes, which is more aligned with user input expectations.

Screenshot

Screenshot 2025-01-22 at 7 17 52 AM

Key Takeaway

Zeroing out the seconds ensures that only the hours and minutes are considered during validation, avoiding errors caused by minor mismatches in time precision. This approach simplifies the logic and aligns it with user expectations.

Let me know sir if you’d like further modifications!

Copy link
Author

Choose a reason for hiding this comment

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

"@ibacher sir, please correct me if I am wrong. Was I thinking in the wrong way (approach)?"

Copy link
Member

Choose a reason for hiding this comment

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

I see. So the limiting factor here is the time picker component? I guess that makes sense.

Copy link
Member

Choose a reason for hiding this comment

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

But, then, shouldn't we either zero out the seconds component when sending this to the backend or use the seconds (and milliseconds) from the current timestamp? Otherwise, we're effectively sending data the user didn't supply to the backend and that's what gets stored (not to mention, we have a similar validation check on the backend that might fail with a quick enough connection).

Copy link
Author

@Muppasanipraneeth Muppasanipraneeth Jan 31, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

But, then, shouldn't we either zero out the seconds component when sending this to the backend or use the seconds (and milliseconds) from the current timestamp? Otherwise, we're effectively sending data the user didn't supply to the backend and that's what gets stored (not to mention, we have a similar validation check on the backend that might fail with a quick enough connection).
Here's a clearer, more organized version of your reply:

@ibacher Sir, I agree with your assessment. To elaborate:

  1. Regarding zeroing out seconds:

    • We should not zero out seconds when sending to the backend
    • This would cause errors in a specific scenario:
      • When a user starts a visit
      • Ends that visit
      • Then starts another visit within the same minute
    • In this case, if seconds were zeroed, only the minute would change but here for immediate start visit with in minute that will also won't change
    • The backend would correctly throw an overlap error

  2. Regarding timestamp handling:

    • Using the current timestamp is the correct approach
    • Based on your input, I understand we should:
      • Apply this change only to the start visit payload
      • Keep the existing behavior for stop visit payload
      • Let the stop visit continue using its current seconds handling

This solution will prevent overlap errors while maintaining accurate visit timing records.

return visitStartDatetime <= new Date();
};

const hadPreviousStopDateTime = Boolean(visitToEdit?.stopDatetime);
Expand Down Expand Up @@ -449,6 +450,7 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({
dayjs(visitStartDate).date(),
hours,
minutes,
dayjs(visitStartDate).second(),
),
),
),
Expand All @@ -472,6 +474,7 @@ const StartVisitForm: React.FC<StartVisitFormProps> = ({
dayjs(visitStopDate).date(),
visitStopHours,
visitStopMinutes,
dayjs(visitStopDate).second(),
),
),
);
Expand Down Expand Up @@ -876,4 +879,4 @@ const VisitFormExtensionSlot: React.FC<VisitFormExtensionSlotProps> = React.memo
},
);

export default StartVisitForm;
export default StartVisitForm;
ibacher marked this conversation as resolved.
Show resolved Hide resolved
Loading