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

HTML-777: Validate that encounter dates times are within visit start and end date. #48

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

icrc-toliveira
Copy link
Contributor

Background
The encounter date can be changed to be prior to the visit start date

Issue:
https://issues.openmrs.org/browse/HTML-777

Change:
Edit mode will now be validated

if (validationErrors.size() > 0) {
return returnHelper(validationErrors, fes, null);
}
} else if(visit != null) {

Choose a reason for hiding this comment

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

thanks @icrc-toliveira we can put this on the same line

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @HerbertYiga
I´m not sure if this was you mean, but I change the indentation to be on the same line.
However, I noticed that if I let the project auto-ident, the entire file will have changes

@mks-d mks-d changed the title HTML-777 - Validate date in Edit mode HTML-777: Validate that encounter dates times are within visit start and end date. Jun 24, 2021
Copy link
Member

@mks-d mks-d left a comment

Choose a reason for hiding this comment

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

@icrc-toliveira please add appropriate unit tests for this.

…mlformentryui into HTML-777

� Conflicts:
�	omod/src/test/java/org/openmrs/module/htmlformentryui/fragment/controller/htmlform/EnterHtmlFormFragmentControllerComponentTest.java
@icrc-toliveira icrc-toliveira requested a review from mks-d July 1, 2021 15:53
@icrc-toliveira
Copy link
Contributor Author

Hi @mks-d, @HerbertYiga

I just created the requested test,
If possible, could you have a look?

@dkayiwa
Copy link
Member

dkayiwa commented Jul 6, 2021

@icrc-toliveira the new test passes even without your fix/changes

…houldNotChangeTimeOfEncounterDateIfNewDateIsPriorToDateOfVisit
@icrc-toliveira
Copy link
Contributor Author

icrc-toliveira commented Jul 8, 2021

@icrc-toliveira the new test passes even without your fix/changes

@dkayiwa Yes thx for noticing

Unfortunately I had mistyped the second request.
It had two times the value "w5" instead of one "w3" and one "w5".
Resulting always in at least one error message.

And since the test was validating the existence of error messages, the test would always pass

@dkayiwa
Copy link
Member

dkayiwa commented Jul 9, 2021

@icrc-toliveira isn't this already covered by? https://github.com/openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/validator/EncounterValidator.java

@HerbertYiga
Copy link

isn't this already covered by? https://github.com/openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/validator/EncounterValidator.java

hi @icrc-toliveira,thanks too for also working on this,we will also need this could you kindly attend to @dkayiwa 's comments above

@HerbertYiga
Copy link

hi @icrc-toliveira trying to re ping you on this

@icrc-toliveira
Copy link
Contributor Author

Hi @HerbertYiga, @dkayiwa

Will have a look

@HerbertYiga
Copy link

hi @dkayiwa does this look good enough to be merged now so that we release htmlformentryui?

@dkayiwa
Copy link
Member

dkayiwa commented Aug 2, 2021

@HerbertYiga did you check the previous comments?

@HerbertYiga
Copy link

Will have a look

hi @icrc-toliveira do you still have some time to complete this so that we release htmlformentryui as soon as possible?

@icrc-toliveira
Copy link
Contributor Author

Hi @HerbertYiga, @dkayiwa

I have been looking into the validation method mention by @dkayiwa, and yes indeed the Validation is called.
However, the exception is never handled, causing the FragmentFactory to throw and error and the form to be saved with the wrong date.

It is possible to handled the error, but the message that we get from it its far from perfect and not translatable.
image

Example on how to handle the exception:

try {
	// Do actual encounter creation/updating
	fes.applyActions();
}catch (ValidationException e) {
	validationErrors.add(new FormSubmissionError("general-form-error", e.getLocalizedMessage()));
	return returnHelper(validationErrors, fes, null);
}

@dkayiwa
Copy link
Member

dkayiwa commented Aug 14, 2021

@icrc-toliveira are you able to reproduce this from here? https://qa-refapp.openmrs.org/

@HerbertYiga
Copy link

are you able to reproduce this from here? https://qa-refapp.openmrs.org/

hi @icrc-toliveira ,@dkayiwa left some comment for you!!

@icrc-toliveira
Copy link
Contributor Author

Hi @dkayiwa @HerbertYiga
Currently I'm trying to replicate this using the https://qa-refapp.openmrs.org/

For now, I´m able to get the error but I´m unable to save the form.
Some further test is needed to be sure why I'm able to save it in our distro

image

@HerbertYiga
Copy link

HerbertYiga commented Aug 19, 2021

Currently I'm trying to replicate this using the https://qa-refapp.openmrs.org/

For now, I´m able to get the error but I´m unable to save the form.
Some further test is needed to be sure why I'm able to save it in our distro

hi @dkayiwa could we be having any next steps here basing on these comments? cc @sherrif10

@dkayiwa
Copy link
Member

dkayiwa commented Aug 19, 2021

@icrc-toliveira is still investigating.

@HerbertYiga
Copy link

hi @icrc-toliveira any progress on investigating this?

@icrc-toliveira
Copy link
Contributor Author

hi @icrc-toliveira any progress on investigating this?

Hi @HerbertYiga

Not, much

I tried to replicate the forms causing the error in https://qa-refapp.openmrs.org/,
But for now, only was able to get the error message mentioned above.

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.

4 participants