(fix) O3-4260 fix error when starting a visit immediately after ending one #2147
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
chibongho
requested review from
denniskigen,
jayasanka-sack,
vasharma05 and
ibacher
|
Size Change: +35 B (0%) Total Size: 16 MB ℹ️ View Unchanged
|
ibacher
approved these changes
Dec 11, 2024
|
Sorry, I take this back. This still doesn't fix the issue, as long as the stop time of the previous visit and the start time of the new visit are within the same minute. (The backend does both The reason we truncate the seconds in the start visit form in the first place is that we don't have a form element to ask for the seconds. I think if people want to modify the visit start time (for RDE), defaults the seconds to 0 is reasonable. However, when they want "current time" to be the visit start time, I think we shouldn't truncate. The queues app has a similar problem as well when editing a queue entry's start time. With input from UX designers, we came up with a checkbox to enable editing of the time field. I propose we can use the same pattern here: if checkbox not enabled, we use Thoughts?
|
I agree with this... I don't see the need to truncate. That being said, I don't think we store milliseconds in the DB, so this might not fix things? |
I agree with the optional time thing. We've run into issues before with time truncation in non-RDE contexts, e.g., needing to wait 1 minute between actions, which is why I'm not a huge fan. I agree that in a RDE context, truncating to 0 seconds makes complete sense, though if we do that, I would also advocate that we truncate the start date/time as well in an RDE context. For the non-RDE use case, I still like the idea of defaulting to the current date - 1 second. That is, as @mogoodrich points out, the smallest difference that can be stored in the database and should ensure that (at least for the same user) the "End Visit" is always before the "Start Visit". |
kdaud
changed the title
|
Thanks for the input. Closing this PR for now. I'll update the corresponding ticket with our discussion. |
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
…g one
Requirements
Summary
In O3, when we start a visit, we truncate the start time’s seconds field to 0. However, when we end a visit, we do not do the same truncation. Hence, when we try to start another visit within the same minute, we get a This visit overlaps with another visit of the same patient error.
This PR fix this by making the end time truncate the seconds field to 0 as well when ending a visit,
Note that, however, there is still an issue with starting a visit, then ending the visit, and then starting a new visit, all within the same minute, as the backend will still think that the new visit conflicts with the old one. We’ll have to change the backend logic (and probably also remove the seconds truncation logic when we start and stop visits) if we want to fix that as well.
Screenshots
Related Issue
https://openmrs.atlassian.net/browse/O3-4260
Other