Hard-coded uuids for drug order type and test order type

Development
,
1

Ref : TRUNK-5964 : Hard-coded uuids for drug order type and test order type by tendomart · Pull Request #3654 · openmrs/openmrs-core · GitHub

Hi all , Needs to be merged and closed .

Just inquiring to know if there is any other way of dealing with

public OrderType getOrderTypeByClassName(String javaClassName) {
		return sessionFactory.getCurrentSession().createQuery("from OrderType o where o.javaClassName = :javaClassName", OrderType.class)
			.setMaxResults(1).setString("javaClassName", javaClassName).uniqueResult();
}

So that .setMaxResults(1). is nolonger a limit to the size of the resultset . ?

Anyone Thoughts ?

2

If there’s the possibility of returning multiple order types by class name (i.e., if we’re not enforcing a business rule that says that there’s only one order type for each class name), then this would be correctly implemented as:

public List<OrderType> getOrderTypesByClassName(String javaClassName) {
		return sessionFactory.getCurrentSession().createQuery("from OrderType o where o.javaClassName = :javaClassName", OrderType.class)
			.setString("javaClassName", javaClassName).list();
}

That’s the up-shot of @dkayiwa’s comment. That does make things potentially more complex, but that’s the nature of the data model.

The alternative is to make the PR add a unique constraint to the order_type.java_class_name field, after we confirm with the community that that is indeed the expected behaviour and no one is relying on having multiple order types serviced by the same Java class. Then we can reliably use:

public OrderType getOrderTypeByClassName(String javaClassName) {
		return sessionFactory.getCurrentSession().createQuery("from OrderType o where o.javaClassName = :javaClassName", OrderType.class)
			.setString("javaClassName", javaClassName).uniqueResult();
}

Although we might need to fix some test data to reflect this data model change.

3

In general, though, instead of querying the database by javaClassName, it’s like better to just ensure we are using something like DrugOrder.class.isAssignableFrom(object), kind of like we already do in OrderService.

4

@ibacher agreed. #1 is what we need I believe (in our Rwanda distro we have multiple Order Types for org.openmrs.DrugOrder). See my comments on the ticket and let me know what you think:

1 Like
5

Thanks @ibacher @mseaton ,let me consider implementing a List and we see what happens .

6

@mseaton @ibacher for the Record, our standardTestDataset.xml has more OrderTypes other than DrugOrder , and TestOrder so just like @dkayiwa suggested , limiting the ResultSet to 1 is wrong business-wise

What would be the case if someone or some other Distro needed an OrderType say for X-Ray or Lab ? Won’t that wrongfully call for some other refactoring. Should we for now keep the scope around DrugOrder and TestOrder

I refactored the method to return a list instead , but this caused breaks elsewhere . OrderService @Authorized(PrivilegeConstants.GET_ORDER_TYPES) public List getOrderTypesByClassName(String className); **OrderServiceImpl**

@Transactional(readOnly = true)
	public List<OrderType> getOrderTypesByClassName(String className) {
		return dao.getOrderTypesByClassName(className);
	}

so am suggesting we do the following.

Have two methods to

  1. Get OrderType By Specific ClassName from OrderTypes table

    public OrderType getOrderTypeByClassName(String ordeType){}

  2. Get A List of OrderTypes given a generic Param like
    using getAllOrderTypesByClassName()

    public List<OrderType> getAllOrderTypesByClassName(String allOrderTypes){}

Is this the wrong way ? I believe it gives two options for the Distos and different use cases

7

In this case, wouldn’t the more appropriate name for the first method be

public OrderType  getRandomOrderTypeByClassName(String ordeType){}

What would the definition of the method returning a single order type when there are multiple types for the class? How do you know which one you will get?

1 Like
8

Ha ha good one .@burke I have no objection for that because the pool is actually bigger than i had initially anticipated . so yes it could makes more sense . Though @mseaton is having the idea that we have a method which returns any of these two conditions

  1. A list of similar OrderTypes or a List of different OrderTypes , none to limit the size.
9

@tendomart I’m not sure I see where @mseaton suggested that?

In his comment on the issue he says explicitly:

Neither of these should need us to return just a single Order type per classname. … So I’d change your service method to return a List rather than a single Order Type.

What he’s suggesting is that, e.g., here and here if getOrderTypesByClassName() returns more than one result, we throw an exception notifying the user that the must explicitly set an order type.

I would also change things here so that instead of loading the order type by class name, we just check something like:

&& firstOrder instance of DrugOrder
10

There was some development [TRUNK-5964] - OpenMRS Issues

Considering this , thanks @ibacher

11

@ibacher did the implementation of the List , with alot of breaks, do you have sometime to explore the implementation ?

12

Hey folks do you want to look at the new changes to see wether they satisfy the needs of the use case ?