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) 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

Conversation

Muppasanipraneeth
Copy link

@Muppasanipraneeth Muppasanipraneeth commented Jan 10, 2025

Requirements Checklist

  • PR title includes ticket number and follows conventional commit format
  • Code follows OpenMRS 3.0 Styleguide and design documentation
  • Changes are covered by existing or new tests

Summary

Fixed an error that occurs when starting a new visit immediately after ending another one.

Issue Description

The problem occurs when two visits are created within the same minute. Here's the scenario:

  1. First Visit (Visit A)

    • Start time: 2025-01-11T02:33:00.000Z
    • End time: 2025-01-11T02:33:33.497Z
  2. Second Visit (Visit B)

    • Start time: 2025-01-11T02:33:00.000Z

The system was throwing a "visitCannotOverlapAnotherVisitOfTheSamePatient" error because:

  • The payload only included hours and minutes in the datetime
  • Even though the visits had different seconds, this precision was lost
  • This made the system think the visits were overlapping

Solution

Include seconds in the datetime payload. Example of working times:

  1. First Visit (Visit A)

    • Start time: 2025-01-11T02:43:26.000Z
    • End time: 2025-01-11T02:43:45.330Z
  2. Second Visit (Visit B)

    • Start time: 2025-01-11T02:43:51.000Z

Additional Fix:Future Time Validation

Fixed an issue where the start visit form would incorrectly validate times example when form completion takes about a minute. so you want enter the current time the it validates future time for current time also

Previous Implementation

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();
};

Current Implementation

const validateStartTime = (data: z.infer<typeof visitFormSchema>) => {
  const [visitStartHours, visitStartMinutes] = convertTime12to24(data.visitStartTime, data.visitStartTimeFormat);
  const visitStartDatetime = new Date(data.visitStartDate);
  visitStartDatetime.setHours(visitStartHours, visitStartMinutes, 0, 0);
  return visitStartDatetime <= new Date();
};

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

###Exmaple
Let me explain it more simply:

Problem: When you try to start a visit at "2:30 PM" while it's currently "2:30:45 PM":

Old way: Creates "2:30:00 PM" and fails because it thinks this exact second overlaps with current time.

New way: Just checks if your "2:30 PM" minute matches, ignoring seconds, which works as expected.

It's like telling someone "Meet at 2:30" vs "Meet at exactly 2:30:00" - the new way uses the more natural interpretation.

Screenshots

Visit Overlap Error

Before the fix

Screen.Recording.2025-01-10.at.10.05.47.AM.mov

After the fix

Screen.Recording.2025-01-10.at.10.07.58.AM.mov

Time Validation

Before correcting validation

Screen.Recording.2025-01-10.at.10.37.14.AM.mov

After correcting the validation

Screen.Recording.2025-01-10.at.10.28.04.AM.mov

Related Items

@Muppasanipraneeth
Copy link
Author

Muppasanipraneeth commented Jan 10, 2025

@denniskigen and @ibacher

Copy link
Member

@ibacher ibacher left a comment

Choose a reason for hiding this comment

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

Thanks @Muppasanipraneeth. I'm not sure I understand from the code or the explanation in the PR how this addresses the issue in the ticket. This seems to add unnecessary complexity.

The issue occurs when I stop Visit 1 and then immediately try to start Visit 2, Visit 2 gets rejected for overlapping with Visit 1.

Comment on lines 148 to 150
const visitStartDatetime = new Date(data.visitStartDate);
visitStartDatetime.setHours(visitStartHours, visitStartMinutes, 0, 0);
return visitStartDatetime <= new Date();
Copy link
Member

Choose a reason for hiding this comment

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

Doesn't this just make things worse, since we've zeroed out the seconds and milliseconds?

Copy link
Author

@Muppasanipraneeth Muppasanipraneeth Jan 13, 2025

Choose a reason for hiding this comment

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

Doesn't this just make things worse, since we've zeroed out the seconds and milliseconds?

no sir it just used for logical comparison only without including seconds and milliseconds we set seconds and milliseconds to zero

@Muppasanipraneeth
Copy link
Author

updated pr summary

@Muppasanipraneeth
Copy link
Author

@ibacher sir

@NethmiRodrigo
Copy link
Collaborator

@Muppasanipraneeth please update your branch and resolve the conflicts

@Muppasanipraneeth Muppasanipraneeth force-pushed the O3-4286-fix-error-when-starting-a-visit-immediately-after-ending-one branch from 7da684a to 3ea84ab Compare January 20, 2025 12:37
@Muppasanipraneeth
Copy link
Author

Muppasanipraneeth commented Jan 20, 2025

@Muppasanipraneeth please update your branch and resolve the conflicts

@NethmiRodrigo updated branch and resolved conflicts

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

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

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

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.

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