TRUNK-4697 Get all Orders in Visit

Dear @darius

I wrote a first draft of how I would solve:

you can check out the code here:

Its not totally ready/tested. I just wanted you and anybody else give there opinion on the OrderSearchCriteria implementation. I used the builder pattern. Now I only support visit, but if you like it I will add the other parameters you want to query for. So I think this makes it read nicely and extendable (plus we can put any checks we need in the build method).

Client code would then look like this:

OrderSearchCriteria orderSearchCriteria = new OrderSearchCriteria.Builder().addVisit(visit).addCareSetting(careSetting).build();
List<Order> orders = orderService.getOrders(orderSearchCriteria);

What do you say? (if yes, I will claim the issue :wink:

On second thought maybe factory methods will do just fine since we do not need to support all kinds of combination’s. Not sure if for ex. Getting orders for patient and visits will be needed.

All orders for a visit should be all orders linked to an encounter within the given visit, right? I was thrown off by mention of care setting in your example client code, since the care setting should be implied (i.e., any given visit should occur within a single care setting). Maybe this is an artifact of us introducing care settings with orders and not yet having them throughout the model.

you are totally right, that was my mistake.

darius just mentioned possible search parameters

In the first pass, I only care about supporting “visit”, but we might as well support: patient visits (list) encounters (list) orderType careSetting active (or else status=“active”)

he didnt specify if client code should be able to combine them or not. My prototype of OrderSearch using builder pattern and mistake of combining Visit and CareSetting just showed me that I think factory methods like these

OrderSearchCriteria OrderSearchCriteria.createOrderSearchForVisits(List<Visit> visit)
OrderSearchCriteria OrderSearchCriteria.createOrderSearchForCareSetting(CareSetting careSetting)
OrderSearchCriteria OrderSearchCriteria.createOrderSearchForPatient(Patient patient)
OrderSearchCriteria OrderSearchCriteria.createOrderSearchForPatientAndCareSetting(Patient patient, CareSetting careSetting)

are probably better, since its easier to ensure combinations that actually make sense :smile: @darius what do you think? Should I pursue this issue

We may want separate methods for the likely use cases, since visits, encounters, and care settings are not independent. For example: a visit (and all of its encounters) must be within a single care setting, specifying both visits & encounters (which may or may not be within the visit) would be strange, asking for status and for specific visits and/or encounters is hard to interpret.

The common searches I’d imagine are:

  • Active orders for patient X within care setting Y
  • All orders (full history of orders regardless of whether they are active or not) for patient X within care setting Y
  • All orders (or all orders of one or more types) for a given Visit

So maybe a few methods like:

getActiveOrders(Patient, CareSetting);
getOrders(Patient, CareSetting);
getOrdersByVisitAndOrderType(Visit, OrderType[]);

@teleivo sorry I haven’t had a chance to peek at the code yet, but generally, yes, I like the idea of OrderSearchCriteria with a Builder pattern for this. And you should ignore @burke suggesting an explosion of different methods. :slight_smile:

Stylistically, you need to either (a) follow the pattern set by EncounterSearchCriteria and its builder, or else (b) propose a change to that pattern and also refactor EncounterSearchCriteria to match it.

I would actually suggest using @Builder from Project Lombok. OpenMRS hasn’t used it before, but I’ve heard very good things about this tool (if used carefully) and I’d love to have us explore it. (I haven’t checked whether it would require IDE plugins to handle, which might be a deal breaker.)

2 Likes

I agree that an explosion of finder methods is not desirable, but neither is a single “God method” finder. It’s a matter of finding a balance between both extremes. If the methods Burke proposes are used very often maybe they deserve to be specific finders.

Speaking as the person who wrote the ticket… :smile:

I specifically asked for “all orders in a given visit (of any status or care setting)” so that I could display an “order sheet” of all orders that were placed during any encounter in the visit that the user is looking at now.

So at a minimum, I would want getOrders(Visit)

However our experience has shown us that, since we’re building a generic platform and not just a specific application, if we go down the path of implementing each query we want, we quickly hit the combinatorial explosion. (Just search for “getEncounters” in EncounterService to see that explosion.) That’s why we specifically took the initiative to get in a SearchCriteria pattern, so we could start moving in that direction.

Accordingly, when I wrote the ticket, I asked for the QueryParameter pattern, and I’m surprised that this is controversial! (Btw, as a result of peeking at the EncounterService I just updated TRUNK-4697 to also ask for an includeVoided parameter.)

(I agree that its nice to have convenience queries beyond just the “God method” query, but personally I think we should be writing the “God method” first, and other things later once we have real-world insight into usage patterns.)

When we started, we creating search methods as needed and, in too many cases, ended up with an explosion of methods. That led us to a convention of creating “God methods” for searching. For a consumer of the API, neither dozens of use-case-specific search methods nor calling something like getFoo(null, null, null, null, null, null, null, 7, null) smell too good.

In this case, the “God method” has many cases that would produce unpredictable results, since the parameters are far from independent from each other (i.e., it’s not just a matter of filtering a set of objects based on parameters) to the point that only clients with a deep knowledge of our model could use it and, unless we’re careful, it would be easy for us to give invalid results for invalid combinations of parameters. For example, “get active orders for visit” or “get orders for unrelated encounters and visits.”

But, to avoid confusion of too many cooks in the kitchen, feel free to ignore me and follow @darius’ suggestions (especially if he can convince @lluismf). :smiley:

I’m already convinced :smile:

Agreed. What’s the convention for handling non-sensical combinations of parameters? For example:

  • If any visits or encounters are for a different patient than the one specified, the result is uninterpretable.
  • If visit(s) and encounter(s) are both specified, then the result is uninterpretable (i.e., we shouldn’t be inventing ways of interpreting answering such a query).
  • If asking for active orders without specifying the care setting or when specifying visit(s) and/or encounter(s), the result is uninterpretable.

The reason for my suggestion of separate queries in this case was not that I disagree with our “start with a God method” convention; rather, there are so many dependencies between these parameters, I was worried that (1) we would fail to handle them properly in the “God method” and (2) it’s not fair to expect clients of the API to know them.

Asking for orders by visit makes sense. Asking for orders by encounter makes sense, but may be unnecessary (if Encounter.getOrders() does this. Asking for orders for a care setting makes sense. Trying to do all of these in one single “God method” creates all the complexities I mention above.

My presumption would be that the client should expect that we AND together all criteria. This is a developer API, not something that should be exposed directly to a doctor. And keep in mind that the client may be a REST client that is trying to fetch a bunch of data to build its own multi-screen UI.

E.g. you are suggesting that a client should not be allowed to ask for orders across active orders across care settings (because a clinician should never see those orders mixed together as a list), but I think we must allow it (so an API client can get all these and do its own grouping).

Therefore I would expect:

  • If any visits or encounters are for a different patient than the one specified, the result is the empty list
  • order.patient = $patient AND order.encounter.visit in ($visits)
  • If visit(s) and encounter(s) are both specified, then the result is the same as the intersection of querying once with visit(s) and once with encounter(s) and intersecting the results of the two queries.
  • order.encounter.visit in ($visits) AND order.encounter in ($encounters)
  • If asking for active orders without specifying the care setting the result is all active orders across all care settings
    • but I think this should not be allowed unless you specify patient, visit(s), or encounter(s), e.g. we don’t allow querying for all active visits in the system.
  • If asking for active orders when specifying visit(s) and/or encounter(s), the result is all orders that are currently active and were placed during the specified visit(s) and/or encounter(s)
  • order.encounter.visit in ($visits) AND order.encounter in ($encounters) AND /* logic for isActive() */

Actually @teleivo, in the first pass at this ticket you should skip the ‘active’ functionality, because we need to decide whether we want a model of active (true/false) or status (maybe scheduled/active/discontinued/expired), or if we want both of these.