HTML-722: Encounter edit should not overwrite "unrelated" workflow state. #171
Conversation
… an encounter should not overwrite previous states not related to the encounter
mks-d
changed the title
There was a problem hiding this comment.
I think this looks good, but just want to make sure we are getting everything right, can you review my question when you get a chance @kml27 ?
| Assert.assertNull(patientProgram.getDateCompleted()); | ||
| Assert.assertNull(currentPatientState.getEndDate()); | ||
|
|
||
| //illustrate that this second state has overwritten the existing state |
There was a problem hiding this comment.
this comment is now incorrect, I believe? This was when it was showing the error case?
There was a problem hiding this comment.
Yes, I thought as a regression test it would still make sense. I can update or remove it if you prefer?
There was a problem hiding this comment.
Update or remove, whatever works for you!
|
|
||
| //since there is no direct connection between encounters and states | ||
| //the best that can be done, for now, is to check | ||
| //if the encounterDatetime matches the startDate |
There was a problem hiding this comment.
Trying to remember back all those years ago when I worked on this... this whole logic is rather muddled unfortunately...
I think I'm following your logic, but want to make sure: in this case "oldPatientState" means "the old patient state we assume was created via this form"? And if we find an "old state we assume was created via the form" we want to replace that state with the new state... but if we find a state for on the previous encounter date, but it wasn't started on the previous encounter date, we assume it wasn't created by the form as so we don't want to replace the existing state, but instead just transition to the new state... is this correct?
There was a problem hiding this comment.
Also, can you confirm that you agree with the logic in this ticket is not bug: https://tickets.openmrs.org/browse/HTML-321 (If you feel it is a bug we can debate, but just want to make sure we aren't changing assumptins or behavior without realizing it.
There was a problem hiding this comment.
In response to your first comment, yes.
Re: HTML-321
I think the behavior of changing the start date to the encounter date when submitting an edit regardless of whether the state was "edited" is unexpected. The behavior of when in enter mode when 1) submitting the "blank" option and 2) when submitting the same state as the current state, is to "do nothing". I believe "do nothing" would be the expected behavior, as it seems the reporter of the ticket did.
Primarily, in HTML-321 the encounter datetime was not changed during edit. In my reading of the Notes section, this would fall under "Otherwise, the state selected on the form is compared to the current patient state at the time of the original encounter date. If these two states differ, change the existing patient state on the original encounter date is changed to the selected state." Since 1) there is an existing state, 2) there is no "new" encounter datetime to be before or after the "original", it is still the "original" encounter datetime.
There was a problem hiding this comment.
I think I agree @kml27 ... do your tickets change this behavior, or is this not affected for now?
There was a problem hiding this comment.
I believe this still follows the existing spec as written. It's just a matter of if/which state will be edited. Having a full set of unit tests (if there are any that haven't already been implemented) to verify the described behaviors is probably the best way to answer the question though.
There was a problem hiding this comment.
Thanks @kml27 ! this looks good, but can you add a slight tweak your comment and say something like "... if th encounterDate matches the startDate, and, if so, assume that the state was originally set via this form an tha dtherefore we need to modify it"... or something like that, I realize it's getting to be a bit of a mouthful!
…k between a state and an encounter based on the datetime
|
This issue can't be completely resolved since program workflow states do not have a time of day component, even though encounterDatetime can (though its time component is often cleared). The time of day component could be cleared before the compare to widen the window for matches. Unfortunately, the opposite isn't quite possible right now. Created datetime for the encounter and state are not based on a single db transaction datetime, so they cannot reliably be used to match the encounter and state. |
Setting a new patient program workflow state while editing an encounter should not overwrite previous states not related to the encounter
HTML-722: Setting a new program workflow state when editing an existing encounter overwrites previous state
https://issues.openmrs.org/browse/HTML-722
Description of what I changed
In element impl. in handleSubmission during EDIT mode context, there must be an exact datetime match of the encounter (or previous if it was changed during the edit) with the start date of the most recent workflow state.
Issue I worked on
see https://issues.openmrs.org/browse/HTML-722
Checklist: I completed these to help reviewers :)
My pull request only contains ONE single commit
(the number above, next to the 'Commits' tab is 1).
No? -> read here on how to squash multiple commits into one
My IDE is configured to follow the code style of this project.
No? Unsure? -> configure your IDE, format the code and add the changes with
git add . && git commit --amendI have added tests to cover my changes. (If you refactored
existing code that was well tested you do not have to add tests)
No? -> write tests and add them to this commit
git add . && git commit --amendI ran
mvn clean packageright before creating this pull request andadded all formatting changes to my commit.
No? -> execute above command
All new and existing tests passed.
No? -> figure out why and add the fix to your commit. It is your responsibility to make sure your code works.
My pull request is based on the latest changes of the master branch.
No? Unsure? -> execute command
git pull --rebase upstream master