Adding Order Context to Order Group

Hi folks , revisiting https://issues.openmrs.org/browse/TRUNK-5963 , i noticed I overloaded OrderService.saveOrderGroup(OrderGroup orderGroup) with OrderService.saveOrderGroup(OrderGroup orderGroup, OrderContext orderContext) ,

What would have been the right path ? Is it to completely get replace the former, or proceed with the overloading?

any thoughts @mseaton @dkayiwa @ibacher

I would’ve thought that overloading was fine. The problem I see in the PR is there’s too much going on. Why add a CareSetting and OrderType fields to OrderGroup (especially when these aren’t saved anywhere)? Why do we have two almost identical functions (saveOrderGroup(OrderGroup) and saveOrderGroup(OrderGroup, OrderContext))? (Can saveOrderGroup(OrderGroup) just be saveOrderGroup(orderGroup, null?).

@ibacher CareSetting and OrderType are part of the OrderContext which help refining the criteria for saving the OrderGroup , i actually added a changeset for them and they were created in the database . Something similar was done for orderService.saveOrder()

You mean making the second option nullable ?

Yeah.

I saw that; but it doesn’t appear there are any actual calls to, e.g., orderGroup.setOrderContext(), etc. (Not that we necessarily need those calls).

Over all, my question is what are we missing out on if the entire implementation for this ticket is this:

/**
 * @see org.openmrs.api.OrderService#saveOrderGroup(org.openmrs.OrderGroup)
 */
@Override
public OrderGroup saveOrderGroup(OrderGroup orderGroup) throws APIException {
	saveOrderGroup(orderGroup, null);
}

/**
 * @see org.openmrs.api.OrderService#saveOrderGroup(org.openmrs.OrderGroup, org.openmrs.api.OrderContext)
 */
@Override
public OrderGroup saveOrderGroup(OrderGroup orderGroup, OrderContext orderContext) throws APIException {
	if (orderGroup.getId() == null) {
		// an OrderGroup requires an encounter, which has a patient, so it
		// is odd that OrderGroup has a patient field. There is no obvious
		// reason why they should ever be different.
		orderGroup.setPatient(orderGroup.getEncounter().getPatient());
		CustomDatatypeUtil.saveAttributesIfNecessary(orderGroup);
		dao.saveOrderGroup(orderGroup);
	}
	List<Order> orders = orderGroup.getOrders();
	for (Order order : orders) {
		if (order.getId() == null) {
			order.setEncounter(orderGroup.getEncounter());
			Context.getOrderService().saveOrder(order, orderContext);
		}
	}
	Set<OrderGroup> nestedGroups = orderGroup.getNestedOrderGroups();
	if (nestedGroups != null) {
		for (OrderGroup nestedGroup : nestedGroups) {
			Context.getOrderService().saveOrderGroup(nestedGroup);
		}
	}
	if (orderGroup.getId() == null) {
		// an OrderGroup requires an encounter, which has a patient, so it
		// is odd that OrderGroup has a patient field. There is no obvious
		// reason why they should ever be different.
		orderGroup.setPatient(orderGroup.getEncounter().getPatient());
		CustomDatatypeUtil.saveAttributesIfNecessary(orderGroup);
		dao.saveOrderGroup(orderGroup);
	}
	return orderGroup;
}

Plus actually adding the saveOrderGroup(OrderGroup orderGroup, OrderContext orderContext) function to the OrderService interface.

You’re right

I think tis right to implement it this way.

Added earlier.