What is the allowDecimal field in ConceptNumeric used for?

ysachinta108 · May 4, 2016, 7:30am

public class ConceptNumeric extends Concept implements java.io.Serializable { private Boolean allowDecimal = false;

allowDecimal is a field in ConceptNumeric which stores a default value as false. Earlier this field was named as precision.

Going by the name allowDecimal = true/false it means that we are allowing/not-allowing a decimal value to be stored.

But then I do not see any validation for this field.

Is the field meant to do what I am understanding or is there a different purpose for this field?

Thank You!

dkayiwa · May 4, 2016, 7:46am

@ysachinta108 are you looking for this? https://github.com/openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/validator/ObsValidator.java#L173

ysachinta108 · May 4, 2016, 11:21am

Thanks @dkayiwa for pointing to the link.

Looking through the code, it seems that when we save Observations through ObsService, then the ObsValidator gets triggered.

But if we create an Encounter and put the Observations in the encounter. Then we save the Encounter through EncounterService, the Obs also gets saved becuase of the hibernate cascade. But the validation (ObsValidator) doesn’t get triggered in this case

Most of our observations gets saved through EncounterService and therefore this validation doesn’t get triggered. We needed this validation to get triggered also when we saved Obs directly through and encounter save

dkayiwa · May 4, 2016, 11:40am

@ysachinta108 that is strange because the encounter service saves all obs one by one using the ObsService which in turn calls the ObsValidator as you can see starting from this line: https://github.com/openmrs/openmrs-core/blob/master/api/src/main/java/org/openmrs/api/impl/EncounterServiceImpl.java#L205

Do you have a unit test reproducing your problem?

ysachinta108 · May 4, 2016, 11:53am

@dkayiwa Looks like that piece of code was added newly. It is just present in 2.0.x and master.

Whereas the older version 1.12.x doesn’t have that piece of implementation.

github.com

openmrs/openmrs-core/blob/1.12.x/api/src/main/java/org/openmrs/api/impl/EncounterServiceImpl.java#L202


		// save the new orderGroups
		for (OrderGroup orderGroup : encounter.getOrderGroups()) {
			Context.getOrderService().saveOrderGroup(orderGroup);
		}
		//save the new orders which do not have order groups
		for (Order o : encounter.getOrdersWithoutOrderGroups()) {
			if (o.getOrderId() == null) {
				Context.getOrderService().saveOrder(o, null);
			}
		}
		return encounter;
	}
	
	/**
	 * @see org.openmrs.api.EncounterService#getEncounter(java.lang.Integer)
	 */
	@Transactional(readOnly = true)
	public Encounter getEncounter(Integer encounterId) throws APIException {
		Encounter encounter = dao.getEncounter(encounterId);
		if (encounter == null) {
			return null;

After the encounter save there is just order save. The obs saving which is happening after that is newly added as in your link

We are using 1.12 of Openmrs. Therefore we are facing that issue

Can this implementation be back-ported to 1.12.x so we can get that validation?

dkayiwa · May 4, 2016, 12:15pm

@ysachinta108 you are correct. It was fixed with TRUNK-50 but never got back ported. I personally feel that back porting to 1.12.x is fine. What do others think? @wyclif @darius @burke It is a huge commit though!

darius · May 4, 2016, 7:25pm

It is up to @bharatak (as release manager for Platform 1.12) to decide whether he wants this bugfix backported to the 1.12.x branch.

Since Platform 1.12.0 has not been released yet, it’s still okay to backport new features to that branch (as long as it’s also in Platform 2.0). The issue in question is a bugfix, not a new feature, so it is technically okay to backport even in a maintenance release. Though, with the size of the change I would definitely prefer to do it in a .0 vs a .1.

So, @ysachinta108, if Bahmni wants/needs this bugfix backported, you should ask @bharatak (wearing his OpenMRS Release Manager hat) to approve that backporting.

bharatak · May 5, 2016, 3:25pm

I dont think we should go with the bugfix backported at this time when we are about to release 1.12 considering the size of the change. Spoke with @ysachinta108 and he is fine with it. So, we are not doing it in 1.12.0. If it is required, we may consider doing it in .1 as @darius suggested.