From 0771f013dcd0a31cad9088152ee2e06a8dde573b Mon Sep 17 00:00:00 2001 From: Cosmin Date: Mon, 21 Sep 2026 15:45:53 -0400 Subject: [PATCH 1/6] UHM-9514: migrate audit REST end points from pihcore --- .../module/pihapps/PihAppsService.java | 35 +++ .../module/pihapps/PihAppsServiceImpl.java | 204 ++++++++++++++- .../encounter/EncounterSearchCriteria.java | 65 +++++ .../encounter/EncounterSearchResult.java | 12 + .../module/pihapps/obs/ObsSearchCriteria.java | 27 ++ .../pihapps/EncounterAuditSearchTest.java | 241 ++++++++++++++++++ .../module/pihapps/ObsAuditSearchTest.java | 193 ++++++++++++++ .../resources/encounterAuditTestDataset.xml | 27 ++ .../test/resources/obsAuditTestDataset.xml | 22 ++ .../module/pihapps/rest/AuditRestSupport.java | 70 +++++ .../rest/EncounterAuditRestController.java | 186 ++++++++++++++ .../pihapps/rest/ObsAuditRestController.java | 148 +++++++++++ 12 files changed, 1229 insertions(+), 1 deletion(-) create mode 100644 api/src/main/java/org/openmrs/module/pihapps/encounter/EncounterSearchCriteria.java create mode 100644 api/src/main/java/org/openmrs/module/pihapps/encounter/EncounterSearchResult.java create mode 100644 api/src/test/java/org/openmrs/module/pihapps/EncounterAuditSearchTest.java create mode 100644 api/src/test/java/org/openmrs/module/pihapps/ObsAuditSearchTest.java create mode 100644 api/src/test/resources/encounterAuditTestDataset.xml create mode 100644 api/src/test/resources/obsAuditTestDataset.xml create mode 100644 omod/src/main/java/org/openmrs/module/pihapps/rest/AuditRestSupport.java create mode 100644 omod/src/main/java/org/openmrs/module/pihapps/rest/EncounterAuditRestController.java create mode 100644 omod/src/main/java/org/openmrs/module/pihapps/rest/ObsAuditRestController.java diff --git a/api/src/main/java/org/openmrs/module/pihapps/PihAppsService.java b/api/src/main/java/org/openmrs/module/pihapps/PihAppsService.java index 72bc1e0..7cd37d4 100644 --- a/api/src/main/java/org/openmrs/module/pihapps/PihAppsService.java +++ b/api/src/main/java/org/openmrs/module/pihapps/PihAppsService.java @@ -18,13 +18,17 @@ import org.openmrs.Location; import org.openmrs.Obs; import org.openmrs.Order; +import org.openmrs.annotation.Authorized; import org.openmrs.api.OpenmrsService; +import org.openmrs.module.pihapps.encounter.EncounterSearchCriteria; +import org.openmrs.module.pihapps.encounter.EncounterSearchResult; import org.openmrs.module.pihapps.obs.ObsSearchCriteria; import org.openmrs.module.pihapps.obs.ObsSearchResult; import org.openmrs.module.pihapps.orders.EncounterFulfillingOrders; import org.openmrs.module.pihapps.orders.OrderSearchCriteria; import org.openmrs.module.pihapps.orders.OrderSearchResult; import org.openmrs.module.pihapps.orders.PatientWithOrdersSearchResult; +import org.openmrs.util.PrivilegeConstants; import java.util.List; import java.util.Map; @@ -52,4 +56,35 @@ public interface PihAppsService extends OpenmrsService { void revertOrdersToOrdered(List orders); ObsSearchResult getObs(ObsSearchCriteria searchCriteria); + + /** + * Observations whose audit trail names the given user, most recent action first. Voided + * observations are included, since they are the whole point of a voidedBy search and are what + * an auditor most wants to see in a createdBy one. + * + *

The search is described by {@code createdBy}, {@code voidedBy} and the audit date bounds + * on {@link ObsSearchCriteria}; ordering defaults to the audit action the search named. At + * least one of the two users is required. + * + * @param searchCriteria what to search for, how to page it and how to sort it + * @return the matching observations and how many there are in total + * @throws org.openmrs.api.APIException if neither user is given + */ + @Authorized(PrivilegeConstants.GET_OBS) + ObsSearchResult getObsByAuditUser(ObsSearchCriteria searchCriteria); + + /** + * Encounters whose audit trail names the given user, or that name the given provider, most + * recent action first. Voided encounters are included, for the same reason voided observations + * are in {@link #getObsByAuditUser(ObsSearchCriteria)}. + * + *

The search is described by {@link EncounterSearchCriteria}; ordering defaults to the audit + * action the search named. At least one user or provider is required. + * + * @param searchCriteria what to search for, how to page it and how to sort it + * @return the matching encounters and how many there are in total + * @throws org.openmrs.api.APIException if no user and no provider is given + */ + @Authorized(PrivilegeConstants.GET_ENCOUNTERS) + EncounterSearchResult getEncountersByAuditUser(EncounterSearchCriteria searchCriteria); } diff --git a/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java b/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java index 125d0a2..25905d9 100644 --- a/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java +++ b/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java @@ -21,15 +21,22 @@ import org.hibernate.Criteria; import org.hibernate.FlushMode; import org.hibernate.criterion.Criterion; +import org.hibernate.criterion.DetachedCriteria; import org.hibernate.criterion.Projections; +import org.hibernate.criterion.Subqueries; import org.openmrs.Concept; import org.openmrs.Encounter; +import org.openmrs.EncounterProvider; +import org.openmrs.EncounterType; import org.openmrs.Location; import org.openmrs.LocationTag; import org.openmrs.Obs; import org.openmrs.Order; import org.openmrs.Patient; +import org.openmrs.Provider; +import org.openmrs.User; import org.openmrs.annotation.Authorized; +import org.openmrs.api.APIException; import org.openmrs.api.EncounterService; import org.openmrs.api.LocationService; import org.openmrs.api.ObsService; @@ -37,6 +44,8 @@ import org.openmrs.api.db.hibernate.DbSessionFactory; import org.openmrs.api.impl.BaseOpenmrsService; import org.openmrs.module.emrapi.EmrApiConstants; +import org.openmrs.module.pihapps.encounter.EncounterSearchCriteria; +import org.openmrs.module.pihapps.encounter.EncounterSearchResult; import org.openmrs.module.pihapps.obs.ObsSearchCriteria; import org.openmrs.module.pihapps.obs.ObsSearchResult; import org.openmrs.module.pihapps.orders.EncounterFulfillingOrders; @@ -576,6 +585,20 @@ public void revertOrdersToOrdered(List orders) { } } + /** + * Unlike the obsDatetime bounds, these are applied exactly as given rather than widened to whole + * days: the caller has already said which moment it means, and an audit range is often asked to + * the second. + */ + private void addAuditDateBounds(Criteria c, String property, ObsSearchCriteria searchCriteria) { + if (searchCriteria.getAuditOnOrAfter() != null) { + c.add(ge(property, searchCriteria.getAuditOnOrAfter())); + } + if (searchCriteria.getAuditOnOrBefore() != null) { + c.add(le(property, searchCriteria.getAuditOnOrBefore())); + } + } + @Override @Transactional(readOnly = true) @Authorized(PrivilegeConstants.GET_PATIENTS) @@ -605,7 +628,21 @@ public ObsSearchResult getObs(ObsSearchCriteria searchCriteria) { @SuppressWarnings({ "deprecation" }) private Criteria createHibernateObsSearchCriteria(ObsSearchCriteria searchCriteria, boolean applySortCriteria) { Criteria c = sessionFactory.getHibernateSessionFactory().getCurrentSession().createCriteria(Obs.class); - c.add(eq("voided", false)); + // An audit search leaves off the voided filter every other obs search applies: a voidedBy + // search would otherwise return nothing, and an auditor looking at what a user created wants + // to see the rows that have since been deleted just as much as the surviving ones. Callers + // can tell them apart by each observation's voided flag. + if (searchCriteria.getCreatedBy() == null && searchCriteria.getVoidedBy() == null) { + c.add(eq("voided", false)); + } + if (searchCriteria.getCreatedBy() != null) { + c.add(eq("creator", searchCriteria.getCreatedBy())); + addAuditDateBounds(c, "dateCreated", searchCriteria); + } + if (searchCriteria.getVoidedBy() != null) { + c.add(eq("voidedBy", searchCriteria.getVoidedBy())); + addAuditDateBounds(c, "dateVoided", searchCriteria); + } if (searchCriteria.getPatient() != null) { c.add(eq("person", searchCriteria.getPatient())); } @@ -634,4 +671,169 @@ private Criteria createHibernateObsSearchCriteria(ObsSearchCriteria searchCriter } return c; } + + @Override + @Transactional(readOnly = true) + @Authorized(PrivilegeConstants.GET_OBS) + public ObsSearchResult getObsByAuditUser(ObsSearchCriteria searchCriteria) { + if (searchCriteria.getCreatedBy() == null && searchCriteria.getVoidedBy() == null) { + throw new APIException("An observation audit search needs at least one of createdBy or voidedBy"); + } + // An audit has an order of its own, so fill it in unless the caller asked for another. + if (searchCriteria.getSortCriteria() == null || searchCriteria.getSortCriteria().isEmpty()) { + searchCriteria.setSortCriteria(auditSortCriteria(searchCriteria)); + } + return getObs(searchCriteria); + } + + /** + * "Most recent first" means the most recent audit action: when the row was created for a + * createdBy search, when it was voided for a voidedBy search. Ordering by the observation's own + * datetime would bury an obs backdated to last year but entered this morning, which is the + * opposite of what an audit needs. The obs id breaks ties so that paging cannot repeat or skip + * a row when several share a timestamp. + */ + private List auditSortCriteria(ObsSearchCriteria searchCriteria) { + String actionDate = searchCriteria.getVoidedBy() != null ? "dateVoided" : "dateCreated"; + List sortCriteria = new ArrayList<>(); + sortCriteria.add(new SortCriteria(actionDate, SortCriteria.Direction.DESC)); + sortCriteria.add(new SortCriteria("obsId", SortCriteria.Direction.DESC)); + return sortCriteria; + } + + @Override + @Transactional(readOnly = true) + @Authorized(PrivilegeConstants.GET_ENCOUNTERS) + @SuppressWarnings({ "unchecked" }) + public EncounterSearchResult getEncountersByAuditUser(EncounterSearchCriteria searchCriteria) { + if (searchCriteria.getCreatedBy() == null && searchCriteria.getChangedBy() == null + && searchCriteria.getVoidedBy() == null && searchCriteria.getProvider() == null) { + throw new APIException( + "An encounter audit search needs at least one of createdBy, changedBy, voidedBy or provider"); + } + // An audit has an order of its own, so fill it in unless the caller asked for another. + if (searchCriteria.getSortCriteria() == null || searchCriteria.getSortCriteria().isEmpty()) { + searchCriteria.setSortCriteria(encounterAuditSortCriteria(searchCriteria)); + } + + EncounterSearchResult result = new EncounterSearchResult(); + // First query to get total count + Criteria c = createHibernateEncounterSearchCriteria(searchCriteria, false); + c.setProjection(Projections.rowCount()); + Long totalCount = (Long) c.list().get(0); + result.setTotalCount(totalCount); + // Then query to get page of results + c = createHibernateEncounterSearchCriteria(searchCriteria, true); + c.setProjection(null); + Integer startIndex = searchCriteria.getStartIndex(); + Integer limit = searchCriteria.getLimit(); + if (limit != null) { + startIndex = startIndex == null ? 0 : startIndex; + c.setFirstResult(startIndex); + c.setMaxResults(limit); + } + result.setEncounters(c.list()); + return result; + } + + /** + * Orders by the most recent of the audit actions the search named, so that "most recent first" + * means the most recent thing the search is about, and matches whichever column the date bounds + * were applied to. A provider search names no action, so it orders by the encounter's own + * datetime. The encounter id breaks ties so that paging cannot repeat or skip a row when + * several share a timestamp. + */ + private List encounterAuditSortCriteria(EncounterSearchCriteria searchCriteria) { + List sortCriteria = new ArrayList<>(); + sortCriteria.add(new SortCriteria(encounterAuditSortProperty(searchCriteria), SortCriteria.Direction.DESC)); + sortCriteria.add(new SortCriteria("encounterId", SortCriteria.Direction.DESC)); + return sortCriteria; + } + + /** + * The column the results are ordered by: the named audit action, taking the most recent kind of + * action when a search named several, or the encounter's own datetime when only a provider was + * named. + */ + private String encounterAuditSortProperty(EncounterSearchCriteria searchCriteria) { + if (searchCriteria.getVoidedBy() != null) { + return "dateVoided"; + } + if (searchCriteria.getChangedBy() != null) { + return "dateChanged"; + } + if (searchCriteria.getCreatedBy() != null) { + return "dateCreated"; + } + return "encounterDatetime"; + } + + /** + * Unlike the obsDatetime bounds on an obs search, these are applied exactly as given rather than + * widened to whole days: the caller has already said which moment it means. + */ + private void addEncounterAuditDateBounds(Criteria c, String property, EncounterSearchCriteria searchCriteria) { + if (searchCriteria.getAuditOnOrAfter() != null) { + c.add(ge(property, searchCriteria.getAuditOnOrAfter())); + } + if (searchCriteria.getAuditOnOrBefore() != null) { + c.add(le(property, searchCriteria.getAuditOnOrBefore())); + } + } + + /** + * Voided encounters are deliberately left in, for the same reason voided observations are: a + * search for who voided something would otherwise return nothing, and an auditor looking at + * what a user entered wants to see what has since been deleted. Callers can tell them apart by + * each encounter's voided flag. + */ + @SuppressWarnings({ "deprecation" }) + private Criteria createHibernateEncounterSearchCriteria(EncounterSearchCriteria searchCriteria, + boolean applySortCriteria) { + Criteria c = sessionFactory.getHibernateSessionFactory().getCurrentSession().createCriteria(Encounter.class); + if (searchCriteria.getEncounterType() != null) { + c.add(eq("encounterType", searchCriteria.getEncounterType())); + } + if (searchCriteria.getCreatedBy() != null) { + c.add(eq("creator", searchCriteria.getCreatedBy())); + addEncounterAuditDateBounds(c, "dateCreated", searchCriteria); + } + if (searchCriteria.getChangedBy() != null) { + c.add(eq("changedBy", searchCriteria.getChangedBy())); + addEncounterAuditDateBounds(c, "dateChanged", searchCriteria); + } + if (searchCriteria.getVoidedBy() != null) { + c.add(eq("voidedBy", searchCriteria.getVoidedBy())); + addEncounterAuditDateBounds(c, "dateVoided", searchCriteria); + } + if (searchCriteria.getProvider() != null) { + // A subquery rather than a join, so that an encounter naming the provider more than + // once is still returned once — a join would need a distinct, and an in-memory distinct + // would be applied after paging had already counted the duplicate rows. + DetachedCriteria encountersNamingProvider = DetachedCriteria.forClass(EncounterProvider.class, "ep") + .createAlias("ep.encounter", "providerEncounter") + .setProjection(Projections.property("providerEncounter.encounterId")) + .add(eq("ep.provider", searchCriteria.getProvider())) + .add(eq("ep.voided", false)); + c.add(Subqueries.propertyIn("encounterId", encountersNamingProvider)); + // A provider search names no audit action, so on its own it takes the range against the + // encounter's own datetime. Alongside a user filter the range has already been applied + // to that user's action, which is the more specific thing to ask about. + if (searchCriteria.getCreatedBy() == null && searchCriteria.getChangedBy() == null + && searchCriteria.getVoidedBy() == null) { + addEncounterAuditDateBounds(c, "encounterDatetime", searchCriteria); + } + } + if (applySortCriteria && searchCriteria.getSortCriteria() != null) { + for (SortCriteria sortCriteria : searchCriteria.getSortCriteria()) { + if (sortCriteria.getDirection() == SortCriteria.Direction.DESC) { + c.addOrder(desc(sortCriteria.getField())); + } else { + c.addOrder(asc(sortCriteria.getField())); + } + } + } + return c; + } + } diff --git a/api/src/main/java/org/openmrs/module/pihapps/encounter/EncounterSearchCriteria.java b/api/src/main/java/org/openmrs/module/pihapps/encounter/EncounterSearchCriteria.java new file mode 100644 index 0000000..9603d8d --- /dev/null +++ b/api/src/main/java/org/openmrs/module/pihapps/encounter/EncounterSearchCriteria.java @@ -0,0 +1,65 @@ +package org.openmrs.module.pihapps.encounter; + +import lombok.Data; +import org.openmrs.EncounterType; +import org.openmrs.Provider; +import org.openmrs.User; +import org.openmrs.module.pihapps.SortCriteria; + +import java.util.Date; +import java.util.List; + +/** + * Describes an encounter search over the audit trail. Core's own + * {@link org.openmrs.parameter.EncounterSearchCriteria} carries a providers field that no search + * handler exposes, and has no creator, changedBy or voidedBy field at all, which is why this exists + * rather than the module reusing it. + * + *

Every filter narrows, so naming several asks for the encounters satisfying all of them. + */ +@Data +public class EncounterSearchCriteria { + + /** + * Restrict to encounters this user created. Naming any of {@link #createdBy}, {@link #changedBy}, + * {@link #voidedBy} or {@link #provider} makes the search an audit, which also means voided + * encounters are included: they are the whole point of a voidedBy search, and what an auditor + * most wants to see in the others. + */ + private User createdBy; + + /** Restrict to encounters this user changed. See {@link #createdBy}. */ + private User changedBy; + + /** Restrict to encounters this user voided. See {@link #createdBy}. */ + private User voidedBy; + + /** Restrict to encounters this provider is recorded on. See {@link #createdBy}. */ + private Provider provider; + + /** + * Restrict to encounters of this type. This narrows an audit but is not an audit of anything on + * its own, so it does not satisfy the requirement that one of the four above is given. + */ + private EncounterType encounterType; + + /** + * Bound whatever the search is about. Where an audit action is named these bound that action's + * column — an encounter backdated to last year but entered this morning was entered this + * morning, and core's own encounter search already covers encounterDatetime for the cases it + * can reach. A provider search names no action, so there they bound the encounter's own + * datetime, which is both what a provider's caseload is asked about and something core cannot + * filter by provider. + * + *

Both ends run inclusively and are applied as given, so a caller that means a whole day + * passes that day's last moment. + */ + private Date auditOnOrAfter; + + /** @see #auditOnOrAfter */ + private Date auditOnOrBefore; + + private List sortCriteria; + private Integer startIndex; + private Integer limit; +} diff --git a/api/src/main/java/org/openmrs/module/pihapps/encounter/EncounterSearchResult.java b/api/src/main/java/org/openmrs/module/pihapps/encounter/EncounterSearchResult.java new file mode 100644 index 0000000..eadb1b6 --- /dev/null +++ b/api/src/main/java/org/openmrs/module/pihapps/encounter/EncounterSearchResult.java @@ -0,0 +1,12 @@ +package org.openmrs.module.pihapps.encounter; + +import lombok.Data; +import org.openmrs.Encounter; + +import java.util.List; + +@Data +public class EncounterSearchResult { + Long totalCount; + List encounters; +} diff --git a/api/src/main/java/org/openmrs/module/pihapps/obs/ObsSearchCriteria.java b/api/src/main/java/org/openmrs/module/pihapps/obs/ObsSearchCriteria.java index 12b32a6..28979ff 100644 --- a/api/src/main/java/org/openmrs/module/pihapps/obs/ObsSearchCriteria.java +++ b/api/src/main/java/org/openmrs/module/pihapps/obs/ObsSearchCriteria.java @@ -3,6 +3,7 @@ import lombok.Data; import org.openmrs.Concept; import org.openmrs.Patient; +import org.openmrs.User; import org.openmrs.module.pihapps.SortCriteria; import java.util.Date; @@ -14,6 +15,32 @@ public class ObsSearchCriteria { private List concepts; private Date onOrBefore; private Date onOrAfter; + + /** + * Restrict to observations this user created. Naming either this or {@link #voidedBy} makes the + * search an audit, which also means voided observations are included: they are the whole point + * of a voidedBy search, and what an auditor most wants to see in a createdBy one. + */ + private User createdBy; + + /** Restrict to observations this user voided. See {@link #createdBy}. */ + private User voidedBy; + + /** + * Bound the audit action rather than the observation's own datetime, which {@link #onOrAfter} + * and {@link #onOrBefore} cover. An observation backdated to last year but entered this morning + * was modified this morning, which is what a search over a timeframe is asking about. + * + *

Each user filter is bounded by the column belonging to its action, so naming both users + * and a range asks for observations that one user created and the other voided, each within the + * window. Both ends run inclusively and are applied as given, so a caller that means a whole + * day passes that day's last moment. + */ + private Date auditOnOrAfter; + + /** @see #auditOnOrAfter */ + private Date auditOnOrBefore; + private List sortCriteria; private Integer startIndex; private Integer limit; diff --git a/api/src/test/java/org/openmrs/module/pihapps/EncounterAuditSearchTest.java b/api/src/test/java/org/openmrs/module/pihapps/EncounterAuditSearchTest.java new file mode 100644 index 0000000..190b6fa --- /dev/null +++ b/api/src/test/java/org/openmrs/module/pihapps/EncounterAuditSearchTest.java @@ -0,0 +1,241 @@ +package org.openmrs.module.pihapps; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.openmrs.Encounter; +import org.openmrs.EncounterType; +import org.openmrs.Provider; +import org.openmrs.User; +import org.openmrs.api.APIException; +import org.openmrs.api.context.Context; +import org.openmrs.module.pihapps.encounter.EncounterSearchCriteria; +import org.openmrs.module.pihapps.encounter.EncounterSearchResult; +import org.openmrs.test.jupiter.BaseModuleContextSensitiveTest; + +import java.util.Calendar; +import java.util.Collections; +import java.util.Date; +import java.util.List; +import java.util.stream.Collectors; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertThrows; + +/** + * Covers searching encounters by the users in their audit trail, by the provider recorded on them, + * and by encounter type and date. The fixture's audit dates run in a different order from its + * encounter datetimes, so an implementation that bounded or ordered by the wrong column would fail + * here rather than look plausible. + */ +public class EncounterAuditSearchTest extends BaseModuleContextSensitiveTest { + + private PihAppsService service; + + private User bruno; + + private User butch; + + private Provider provider; + + private EncounterType typeOne; + + @BeforeEach + public void setup() { + executeDataSet("encounterAuditTestDataset.xml"); + service = Context.getService(PihAppsService.class); + bruno = Context.getUserService().getUser(501); + butch = Context.getUserService().getUser(502); + provider = Context.getProviderService().getProvider(1); + typeOne = Context.getEncounterService().getEncounterType(1); + } + + /** + * The search under test. Building the criteria in one place means a test cannot silently set + * the wrong field, and keeps each case reading as the question it is asking. + */ + private EncounterSearchResult searchResult(User createdBy, User changedBy, User voidedBy, Provider byProvider, + EncounterType encounterType, Date fromDate, Date toDate, Integer startIndex, Integer limit) { + EncounterSearchCriteria searchCriteria = new EncounterSearchCriteria(); + searchCriteria.setCreatedBy(createdBy); + searchCriteria.setChangedBy(changedBy); + searchCriteria.setVoidedBy(voidedBy); + searchCriteria.setProvider(byProvider); + searchCriteria.setEncounterType(encounterType); + searchCriteria.setAuditOnOrAfter(fromDate); + searchCriteria.setAuditOnOrBefore(toDate); + searchCriteria.setStartIndex(startIndex); + searchCriteria.setLimit(limit); + return service.getEncountersByAuditUser(searchCriteria); + } + + /** The search under test, with the paging arguments left off. */ + private List search(User createdBy, User changedBy, User voidedBy, Provider byProvider, + EncounterType encounterType, Date fromDate, Date toDate) { + return searchResult(createdBy, changedBy, voidedBy, byProvider, encounterType, fromDate, toDate, null, null) + .getEncounters(); + } + + private Long count(User createdBy, User changedBy, User voidedBy, Provider byProvider, + EncounterType encounterType, Date fromDate, Date toDate) { + return searchResult(createdBy, changedBy, voidedBy, byProvider, encounterType, fromDate, toDate, null, null) + .getTotalCount(); + } + + private List encounterIds(List encounters) { + return encounters.stream().map(Encounter::getEncounterId).collect(Collectors.toList()); + } + + /** Only this fixture's encounters, so the standard test dataset's own rows do not interfere. */ + private List auditedIds(List encounters) { + return encounterIds(encounters).stream().filter(id -> id >= 3000).collect(Collectors.toList()); + } + + private Date day(int month, int dayOfMonth, int hourOfDay) { + Calendar calendar = Calendar.getInstance(); + calendar.clear(); + calendar.set(2026, month, dayOfMonth, hourOfDay, 0, 0); + return calendar.getTime(); + } + + private Date september(int dayOfMonth) { + return day(Calendar.SEPTEMBER, dayOfMonth, 0); + } + + @Test + public void shouldFindEncountersCreatedByAUserMostRecentlyCreatedFirst() { + assertThat(auditedIds(search(bruno, null, null, null, null, null, null)), contains(3001, 3002)); + } + + @Test + public void shouldFindEncountersChangedByAUserMostRecentlyChangedFirst() { + // 3004 was changed on 4 Sep by bruno, 3002 on 3 Sep by butch + assertThat(auditedIds(search(null, bruno, null, null, null, null, null)), contains(3004)); + assertThat(auditedIds(search(null, butch, null, null, null, null, null)), contains(3002)); + } + + @Test + public void shouldFindEncountersVoidedByAUser() { + assertThat(auditedIds(search(null, null, butch, null, null, null, null)), contains(3003)); + } + + @Test + public void shouldIncludeVoidedEncountersWhenSearchingByCreator() { + List results = search(butch, null, null, null, null, null, null); + + assertThat(auditedIds(results), contains(3005, 3004, 3003)); + assertThat(results.stream().anyMatch(Encounter::getVoided), is(true)); + } + + @Test + public void shouldNarrowByEveryUserGiven() { + // 3004 was created by butch and changed by bruno + assertThat(auditedIds(search(butch, bruno, null, null, null, null, null)), contains(3004)); + assertThat(search(bruno, bruno, null, null, null, null, null), is(Collections.emptyList())); + } + + @Test + public void shouldFindEncountersByProvider() { + // 3005 names the provider too, but on a voided row, so it does not count + assertThat(auditedIds(search(null, null, null, provider, null, null, null)), contains(3004)); + } + + @Test + public void shouldNarrowByProviderAndUserTogether() { + assertThat(auditedIds(search(butch, null, null, provider, null, null, null)), contains(3004)); + assertThat(search(bruno, null, null, provider, null, null, null), is(Collections.emptyList())); + } + + @Test + public void shouldNarrowByEncounterType() { + // every encounter in the fixture is of type one except 3005 + assertThat(auditedIds(search(butch, null, null, null, typeOne, null, null)), contains(3004, 3003)); + } + + @Test + public void shouldNarrowByEncounterTypeAndProviderTogether() { + assertThat(auditedIds(search(null, null, null, provider, typeOne, null, null)), contains(3004)); + + // 3005 is of the other type, but its only provider row is voided, so it still does not match + // (the standard dataset has encounters of that type on this provider, hence the scoping) + EncounterType otherType = Context.getEncounterService().getEncounterType(2); + assertThat(auditedIds(search(null, null, null, provider, otherType, null, null)), is(Collections.emptyList())); + } + + @Test + public void shouldRefuseATypeOnlySearch() { + assertThrows(APIException.class, () -> search(null, null, null, null, typeOne, null, null)); + } + + @Test + public void shouldBoundEachNamedActionByItsOwnDateColumn() { + // bruno created 3001 on 1 Sep and 3002 on 25 Aug + assertThat(auditedIds(search(bruno, null, null, null, null, september(1), null)), contains(3001)); + // bruno changed 3004 on 4 Sep, so nothing of his was changed from 5 Sep onwards + assertThat(search(null, bruno, null, null, null, september(5), null), is(Collections.emptyList())); + } + + /** + * 3003's encounter_datetime is 20 Aug but it was voided on 5 Sep, so a range applied to the + * encounter's own datetime would miss it. + */ + @Test + public void shouldBoundByTheAuditActionRatherThanTheEncounterDatetime() { + assertThat(auditedIds(search(null, null, butch, null, null, september(1), null)), contains(3003)); + } + + /** + * A provider search names no audit action, so its range bounds the encounter's own datetime — + * what a provider's caseload is asked about, and the one thing core cannot filter by provider. + */ + @Test + public void shouldBoundAProviderSearchByTheEncounterDatetime() { + // encounter 3004 happened on 30 Aug and was entered on 2 Sep + assertThat(auditedIds(search(null, null, null, provider, null, day(Calendar.AUGUST, 1, 0), null)), + contains(3004)); + assertThat(search(null, null, null, provider, null, september(1), null), is(Collections.emptyList())); + } + + @Test + public void shouldTreatBothEndsOfTheRangeAsInclusive() { + assertThat(auditedIds(search(bruno, null, null, null, null, day(Calendar.SEPTEMBER, 1, 8), + day(Calendar.SEPTEMBER, 1, 8))), contains(3001)); + } + + /** + * The standard test dataset has encounters of its own attributed to these users and this + * provider, so the counts are asserted against the unpaged result rather than against a number + * this fixture alone would explain. + */ + @Test + public void shouldCountTheWholeResultSetRatherThanThePage() { + List byCreator = search(butch, null, null, null, null, null, null); + assertThat(count(butch, null, null, null, null, null, null), is((long) byCreator.size())); + + List byProvider = search(null, null, null, provider, null, null, null); + assertThat(count(null, null, null, provider, null, null, null), is((long) byProvider.size())); + } + + @Test + public void shouldCountWithinAFilteredSearch() { + List filtered = search(butch, null, null, null, typeOne, null, null); + + assertThat(count(butch, null, null, null, typeOne, null, null), is((long) filtered.size())); + } + + @Test + public void shouldPageThroughTheResults() { + // this fixture's encounters were entered in 2026, so they sort ahead of the standard ones + assertThat(auditedIds(searchResult(butch, null, null, null, null, null, null, 0, 2).getEncounters()), + contains(3005, 3004)); + assertThat(auditedIds(searchResult(butch, null, null, null, null, null, null, 2, 2).getEncounters()), + contains(3003)); + } + + @Test + public void shouldRefuseASearchThatNamesNeitherUserNorProvider() { + assertThrows(APIException.class, () -> search(null, null, null, null, null, september(1), null)); + assertThrows(APIException.class, () -> count(null, null, null, null, null, null, null)); + } +} diff --git a/api/src/test/java/org/openmrs/module/pihapps/ObsAuditSearchTest.java b/api/src/test/java/org/openmrs/module/pihapps/ObsAuditSearchTest.java new file mode 100644 index 0000000..10ed9e7 --- /dev/null +++ b/api/src/test/java/org/openmrs/module/pihapps/ObsAuditSearchTest.java @@ -0,0 +1,193 @@ +package org.openmrs.module.pihapps; + +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.openmrs.Obs; +import org.openmrs.User; +import org.openmrs.api.APIException; +import org.openmrs.api.context.Context; +import org.openmrs.module.pihapps.obs.ObsSearchCriteria; +import org.openmrs.module.pihapps.obs.ObsSearchResult; +import org.openmrs.test.jupiter.BaseModuleContextSensitiveTest; + +import java.util.Calendar; +import java.util.Date; +import java.util.List; +import java.util.stream.Collectors; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertThrows; + +/** + * Covers searching observations by the user who created or voided them. The fixture's creation and + * voiding dates run in a different order from the observation datetimes, so an implementation that + * ordered by the wrong column would fail here rather than look plausible. + */ +public class ObsAuditSearchTest extends BaseModuleContextSensitiveTest { + + private PihAppsService service; + + private User bruno; + + private User butch; + + @BeforeEach + public void setup() { + executeDataSet("obsAuditTestDataset.xml"); + service = Context.getService(PihAppsService.class); + bruno = Context.getUserService().getUser(501); + butch = Context.getUserService().getUser(502); + } + + private List obsIds(List obs) { + return obs.stream().map(Obs::getObsId).collect(Collectors.toList()); + } + + /** + * The search under test. Building the criteria in one place means a test cannot silently set + * the wrong field, and keeps each case reading as the question it is asking. + */ + private ObsSearchResult search(User createdBy, User voidedBy, Date fromDate, Date toDate, Integer startIndex, + Integer limit) { + ObsSearchCriteria searchCriteria = new ObsSearchCriteria(); + searchCriteria.setCreatedBy(createdBy); + searchCriteria.setVoidedBy(voidedBy); + searchCriteria.setAuditOnOrAfter(fromDate); + searchCriteria.setAuditOnOrBefore(toDate); + searchCriteria.setStartIndex(startIndex); + searchCriteria.setLimit(limit); + return service.getObsByAuditUser(searchCriteria); + } + + private List obs(User createdBy, User voidedBy, Date fromDate, Date toDate, Integer startIndex, + Integer limit) { + return search(createdBy, voidedBy, fromDate, toDate, startIndex, limit).getObs(); + } + + private Long count(User createdBy, User voidedBy, Date fromDate, Date toDate) { + return search(createdBy, voidedBy, fromDate, toDate, null, null).getTotalCount(); + } + + /** A moment on a September 2026 day, matching the fixture's audit dates. */ + private Date september(int dayOfMonth, int hourOfDay) { + Calendar calendar = Calendar.getInstance(); + calendar.clear(); + calendar.set(2026, Calendar.SEPTEMBER, dayOfMonth, hourOfDay, 0, 0); + return calendar.getTime(); + } + + @Test + public void shouldFindObsCreatedByAUserMostRecentlyCreatedFirst() { + List results = obs(bruno, null, null, null, null, null); + + assertThat(obsIds(results), contains(2002, 2001, 2004)); + } + + @Test + public void shouldIncludeVoidedObsWhenSearchingByCreator() { + List results = obs(bruno, null, null, null, null, null); + + assertThat(results.stream().anyMatch(Obs::getVoided), is(true)); + } + + @Test + public void shouldFindObsVoidedByAUserMostRecentlyVoidedFirst() { + List results = obs(null, butch, null, null, null, null); + + assertThat(obsIds(results), contains(2005, 2004)); + } + + @Test + public void shouldNotFindObsVoidedByAnotherUser() { + assertThat(obs(null, bruno, null, null, null, null), is(java.util.Collections.emptyList())); + } + + @Test + public void shouldNarrowByBothUsersWhenBothAreGiven() { + List results = obs(bruno, butch, null, null, null, null); + + assertThat(obsIds(results), contains(2004)); + } + + @Test + public void shouldPageResults() { + assertThat(obsIds(obs(bruno, null, null, null, 0, 2)), contains(2002, 2001)); + assertThat(obsIds(obs(bruno, null, null, null, 2, 2)), contains(2004)); + assertThat(obsIds(obs(bruno, null, null, null, 1, 1)), contains(2001)); + } + + @Test + public void shouldCountTheWholeResultSetRatherThanThePage() { + assertThat(count(bruno, null, null, null), is(3L)); + assertThat(count(null, butch, null, null), is(2L)); + assertThat(count(bruno, butch, null, null), is(1L)); + } + + @Test + public void shouldBoundACreatedBySearchByWhenTheObsWasCreated() { + // obs 2001 was created on 1 Sep, 2002 on 3 Sep, 2004 on 28 Aug + List results = obs(bruno, null, september(1, 0), september(2, 0), null, null); + + assertThat(obsIds(results), contains(2001)); + } + + @Test + public void shouldBoundACreatedBySearchWithOnlyOneEndGiven() { + assertThat(obsIds(obs(bruno, null, september(2, 0), null, null, null)), contains(2002)); + assertThat(obsIds(obs(bruno, null, null, september(2, 0), null, null)), + contains(2001, 2004)); + } + + @Test + public void shouldBoundAVoidedBySearchByWhenTheObsWasVoided() { + // obs 2004 was voided on 4 Sep and 2005 on 5 Sep + List results = obs(null, butch, september(5, 0), null, null, null); + + assertThat(obsIds(results), contains(2005)); + } + + /** + * The fixture's creation dates deliberately run in a different order from its obs datetimes, so + * a range applied to the wrong column would pick different rows. + */ + @Test + public void shouldBoundByTheAuditActionRatherThanTheObsDatetime() { + // obs 2005 has an obs_datetime of 25 Aug but was created on 20 Aug and voided on 5 Sep + assertThat(obsIds(obs(butch, null, september(1, 0), null, null, null)), contains(2003)); + assertThat(obsIds(obs(null, butch, september(1, 0), null, null, null)), + contains(2005, 2004)); + } + + @Test + public void shouldTreatBothEndsOfTheRangeAsInclusive() { + // obs 2001 was created at 08:00 on 1 Sep, so a range of exactly that hour includes it + List results = obs(bruno, null, september(1, 8), september(1, 8), null, null); + + assertThat(obsIds(results), contains(2001)); + } + + @Test + public void shouldCountAndPageWithinTheRange() { + Date from = september(1, 0); + assertThat(count(bruno, null, from, null), is(2L)); + assertThat(obsIds(obs(bruno, null, from, null, 0, 1)), contains(2002)); + assertThat(obsIds(obs(bruno, null, from, null, 1, 1)), contains(2001)); + } + + @Test + public void shouldFindNothingWhenTheRangeExcludesEverything() { + List results = obs(bruno, null, september(20, 0), september(21, 0), null, null); + + assertThat(results, is(java.util.Collections.emptyList())); + assertThat(count(bruno, null, september(20, 0), september(21, 0)), is(0L)); + } + + @Test + public void shouldRefuseAnUnfilteredSearch() { + assertThrows(APIException.class, () -> obs(null, null, null, null, null, null)); + assertThrows(APIException.class, () -> count(null, null, null, null)); + } +} + diff --git a/api/src/test/resources/encounterAuditTestDataset.xml b/api/src/test/resources/encounterAuditTestDataset.xml new file mode 100644 index 0000000..6fbc1fd --- /dev/null +++ b/api/src/test/resources/encounterAuditTestDataset.xml @@ -0,0 +1,27 @@ + + + + + + + + + + + + + + + + + + + + diff --git a/api/src/test/resources/obsAuditTestDataset.xml b/api/src/test/resources/obsAuditTestDataset.xml new file mode 100644 index 0000000..05ccd6a --- /dev/null +++ b/api/src/test/resources/obsAuditTestDataset.xml @@ -0,0 +1,22 @@ + + + + + + + + + + + + + + + + diff --git a/omod/src/main/java/org/openmrs/module/pihapps/rest/AuditRestSupport.java b/omod/src/main/java/org/openmrs/module/pihapps/rest/AuditRestSupport.java new file mode 100644 index 0000000..742a207 --- /dev/null +++ b/omod/src/main/java/org/openmrs/module/pihapps/rest/AuditRestSupport.java @@ -0,0 +1,70 @@ +package org.openmrs.module.pihapps.rest; + +import org.apache.commons.lang.StringUtils; +import org.openmrs.Provider; +import org.openmrs.module.webservices.rest.SimpleObject; +import org.openmrs.module.webservices.rest.web.ConversionUtil; +import org.openmrs.module.webservices.rest.web.RestUtil; +import org.openmrs.module.webservices.rest.web.response.InvalidSearchException; +import org.openmrs.util.OpenmrsUtil; +import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException; + +import java.util.Date; + +/** + * The parts the audit endpoints in this package have in common: reading a date bound off a request + * parameter, and wording a rejected search the same way. Kept together so the two endpoints cannot + * drift apart on what `endDate=2026-09-30` means. + */ +final class AuditRestSupport { + + /** + * The context a rejected search is reported under. `RestUtil.wrapErrorResponse` prints it ahead + * of the exception's own message, so it says what kind of failure this is and leaves the + * particulars to the message. + */ + static final String INVALID_SEARCH_REASON = "Invalid search parameters"; + + private AuditRestSupport() { + } + + /** + * Reads one end of a date range, in any of the formats the REST API accepts elsewhere. + * + *

A bare date names the whole of that day: `endDate=2026-09-30` means through the end of the + * 30th, not its first instant, since a range given in dates is asking about days. Give a time + * to bound the range to the second instead. + * + * @param value the parameter as it arrived, or null or blank for no bound + * @param isUpperBound whether a date without a time should be stretched to the end of the day + * @return the bound, or null if none was given + */ + static Date parseBound(String value, boolean isUpperBound) { + if (StringUtils.isBlank(value)) { + return null; + } + + String trimmed = value.trim(); + Date date = (Date) ConversionUtil.convert(trimmed, Date.class); + return isUpperBound && isDateOnly(trimmed) ? OpenmrsUtil.getLastMomentOfDay(date) : date; + } + + private static boolean isDateOnly(String value) { + return value.matches("\\d{4}-\\d{2}-\\d{2}"); + } + + static String dateFormatMessage() { + return "startDate and endDate must be ISO 8601, e.g. 2026-09-01 or 2026-09-01T13:45:00.000+0000"; + } + + /** + * Answers a parameter core's property editors could not resolve into the object it names. Kept + * here so that both endpoints word the same failure the same way, and so that a bad `createdBy` + * reads the same to a client whichever one it asked. + */ + static SimpleObject unresolvedParameterResponse(MethodArgumentTypeMismatchException e) { + String noun = Provider.class.equals(e.getRequiredType()) ? "provider" : "user"; + return RestUtil.wrapErrorResponse(new InvalidSearchException("No " + noun + " found with id: " + e.getValue()), + INVALID_SEARCH_REASON); + } +} diff --git a/omod/src/main/java/org/openmrs/module/pihapps/rest/EncounterAuditRestController.java b/omod/src/main/java/org/openmrs/module/pihapps/rest/EncounterAuditRestController.java new file mode 100644 index 0000000..4d70ce2 --- /dev/null +++ b/omod/src/main/java/org/openmrs/module/pihapps/rest/EncounterAuditRestController.java @@ -0,0 +1,186 @@ +package org.openmrs.module.pihapps.rest; + +import org.apache.commons.lang.StringUtils; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.openmrs.EncounterType; +import org.openmrs.Provider; +import org.openmrs.User; +import org.openmrs.api.EncounterService; +import org.openmrs.api.context.Context; +import org.openmrs.module.pihapps.PihAppsService; +import org.openmrs.module.pihapps.encounter.EncounterSearchCriteria; +import org.openmrs.module.pihapps.encounter.EncounterSearchResult; +import org.openmrs.module.webservices.rest.SimpleObject; +import org.openmrs.module.webservices.rest.web.RequestContext; +import org.openmrs.module.webservices.rest.web.RestUtil; +import org.openmrs.module.webservices.rest.web.representation.CustomRepresentation; +import org.openmrs.module.webservices.rest.web.resource.impl.AlreadyPaged; +import org.openmrs.module.webservices.rest.web.response.InvalidSearchException; +import org.openmrs.module.webservices.rest.web.response.ResponseException; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.stereotype.Controller; +import org.springframework.web.bind.annotation.ExceptionHandler; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestMethod; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.ResponseBody; +import org.springframework.web.bind.annotation.ResponseStatus; +import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import java.util.Date; + +/** + * Searches encounters by the user who created, changed or voided them, and by the provider recorded + * on them — none of which the core REST API can do. Core's `EncounterSearchCriteria` carries a + * providers field but no search handler exposes it, and it has no creator, changedBy or voidedBy + * field at all, so those columns can be read off an encounter but not searched on. + * + *

Results are paged and ordered by the most recent of the audit actions the search named, and + * each encounter is rendered in the standard encounter representation, so a client can ask for + * whatever it needs with `v`: + * + * 

+ * GET /openmrs/ws/rest/v1/pihapps/encounteraudit?createdBy=<uuid>&limit=20&totalCount=true
+ * GET /openmrs/ws/rest/v1/pihapps/encounteraudit?changedBy=<uuid>&startDate=2026-09-01&endDate=2026-09-30
+ * GET /openmrs/ws/rest/v1/pihapps/encounteraudit?provider=<uuid>&v=custom:(uuid,encounterDatetime,auditInfo)
+ * GET /openmrs/ws/rest/v1/pihapps/encounteraudit?provider=<uuid>&encounterType=<uuid>
+ *
+ * + *

`createdBy`, `changedBy`, `voidedBy` and `provider` are bound by core's property editors, so + * each takes a uuid or a primary key. `encounterType` takes a uuid or its name. + * + *

Every filter narrows, so naming several asks for the encounters satisfying all of them. At + * least one user or provider is required; `encounterType` narrows an audit but is not an audit of + * anything on its own. `startDate` and `endDate` bound when the audit action + * happened rather than the encounter's own datetime, which is what core's encounter search already + * covers; they run inclusively, and a bare date names the whole of that day. + */ +@Controller +public class EncounterAuditRestController { + + protected Log log = LogFactory.getLog(getClass()); + + /** + * The same gate as this package's other administrative endpoints. An encounter audit reaches + * across every patient's record, so it is not something a clinical role should be able to run. + */ + private static final String REQUIRED_PRIVILEGE = "App: coreapps.systemAdministration"; + + /** + * Enough to say what the encounter was and who touched it. `auditInfo` is what makes this an + * audit result: it carries the creating, changing and voiding users with their timestamps. + */ + private static final String DEFAULT_REPRESENTATION = + "(uuid,display,encounterDatetime,voided,encounterType:(uuid,display),form:(uuid,display)," + + "location:(uuid,display),patient:(uuid,display)," + + "encounterProviders:(uuid,voided,provider:(uuid,display),encounterRole:(uuid,display)),auditInfo)"; + + @Autowired + private EncounterService encounterService; + + @Autowired + private PihAppsService pihAppsService; + + @RequestMapping(value = "/rest/v1/pihapps/encounteraudit", method = RequestMethod.GET) + @ResponseBody + public Object searchEncounters(HttpServletRequest request, HttpServletResponse response, + @RequestParam(value = "createdBy", required = false) User createdBy, + @RequestParam(value = "changedBy", required = false) User changedBy, + @RequestParam(value = "voidedBy", required = false) User voidedBy, + @RequestParam(value = "provider", required = false) Provider provider, + @RequestParam(value = "encounterType", required = false) String encounterType, + @RequestParam(value = "startDate", required = false) String startDate, + @RequestParam(value = "endDate", required = false) String endDate) + throws ResponseException { + + if (!Context.hasPrivilege(REQUIRED_PRIVILEGE)) { + return new ResponseEntity<>(HttpStatus.UNAUTHORIZED); + } + + try { + if (createdBy == null && changedBy == null && voidedBy == null && provider == null) { + throw new InvalidSearchException( + "Please specify at least one of createdBy, changedBy, voidedBy or provider."); + } + + EncounterType type = null; + if (StringUtils.isNotBlank(encounterType)) { + type = getEncounterType(encounterType); + if (type == null) { + throw new InvalidSearchException("No encounter type found with id: " + encounterType); + } + } + + Date fromDate; + Date toDate; + try { + fromDate = AuditRestSupport.parseBound(startDate, false); + toDate = AuditRestSupport.parseBound(endDate, true); + } + catch (Exception e) { + throw new InvalidSearchException(AuditRestSupport.dateFormatMessage(), e); + } + + if (fromDate != null && toDate != null && fromDate.after(toDate)) { + throw new InvalidSearchException("startDate must not be after endDate."); + } + + RequestContext context = RestUtil.getRequestContext(request, response, + new CustomRepresentation(DEFAULT_REPRESENTATION)); + + EncounterSearchCriteria searchCriteria = new EncounterSearchCriteria(); + searchCriteria.setCreatedBy(createdBy); + searchCriteria.setChangedBy(changedBy); + searchCriteria.setVoidedBy(voidedBy); + searchCriteria.setProvider(provider); + searchCriteria.setEncounterType(type); + searchCriteria.setAuditOnOrAfter(fromDate); + searchCriteria.setAuditOnOrBefore(toDate); + searchCriteria.setStartIndex(context.getStartIndex()); + searchCriteria.setLimit(context.getLimit()); + + EncounterSearchResult result = pihAppsService.getEncountersByAuditUser(searchCriteria); + Long totalCount = result.getTotalCount(); + boolean hasMore = totalCount > context.getStartIndex() + context.getLimit(); + + return new AlreadyPaged<>(context, result.getEncounters(), hasMore, totalCount).toSimpleObject(null); + } + catch (InvalidSearchException e) { + response.setStatus(HttpServletResponse.SC_BAD_REQUEST); + return RestUtil.wrapErrorResponse(e, AuditRestSupport.INVALID_SEARCH_REASON); + } + catch (Exception e) { + log.error("Failed to search encounters by audit user", e); + response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + return RestUtil.wrapErrorResponse(e, "Failed to search encounters by audit user"); + } + } + + /** + * A parameter core's editors could not resolve is still a bad request, so it is answered in the + * same shape as every other one this endpoint rejects rather than in Spring's own error format. + */ + @ExceptionHandler(MethodArgumentTypeMismatchException.class) + @ResponseStatus(HttpStatus.BAD_REQUEST) + @ResponseBody + public SimpleObject handleUnresolvedParameter(MethodArgumentTypeMismatchException e) { + return AuditRestSupport.unresolvedParameterResponse(e); + } + + /** + * There is no EncounterType Property Editor registered with core, so convert from a String + * uuid or name to an EncounterType. + */ + EncounterType getEncounterType(String uuidOrName) { + EncounterType encounterType = encounterService.getEncounterTypeByUuid(uuidOrName); + if (encounterType == null) { + encounterType = encounterService.getEncounterType(uuidOrName); + } + return encounterType; + } +} diff --git a/omod/src/main/java/org/openmrs/module/pihapps/rest/ObsAuditRestController.java b/omod/src/main/java/org/openmrs/module/pihapps/rest/ObsAuditRestController.java new file mode 100644 index 0000000..44af7be --- /dev/null +++ b/omod/src/main/java/org/openmrs/module/pihapps/rest/ObsAuditRestController.java @@ -0,0 +1,148 @@ +package org.openmrs.module.pihapps.rest; + +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; +import org.openmrs.User; +import org.openmrs.api.context.Context; +import org.openmrs.module.pihapps.PihAppsService; +import org.openmrs.module.pihapps.obs.ObsSearchCriteria; +import org.openmrs.module.pihapps.obs.ObsSearchResult; +import org.openmrs.module.webservices.rest.SimpleObject; +import org.openmrs.module.webservices.rest.web.RequestContext; +import org.openmrs.module.webservices.rest.web.RestUtil; +import org.openmrs.module.webservices.rest.web.representation.CustomRepresentation; +import org.openmrs.module.webservices.rest.web.resource.impl.AlreadyPaged; +import org.openmrs.module.webservices.rest.web.response.InvalidSearchException; +import org.openmrs.module.webservices.rest.web.response.ResponseException; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.stereotype.Controller; +import org.springframework.web.bind.annotation.ExceptionHandler; +import org.springframework.web.bind.annotation.RequestMapping; +import org.springframework.web.bind.annotation.RequestMethod; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.ResponseBody; +import org.springframework.web.bind.annotation.ResponseStatus; +import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException; + +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import java.util.Date; + +/** + * Searches observations by the user who created them or the user who voided them, which the core + * REST API cannot do: neither the obs resource, the observation search handler nor core's own + * {@link org.openmrs.parameter.ObsSearchCriteria} has a creator or voidedBy field, so those columns + * can be read off an observation but not searched on. + * + *

Results are paged and ordered most recent action first, and each observation is rendered in the + * standard obs representation, so a client can ask for whatever it needs with `v`: + * + * 

+ * GET /openmrs/ws/rest/v1/pihapps/obsaudit?createdBy=<uuid>&limit=20&totalCount=true
+ * GET /openmrs/ws/rest/v1/pihapps/obsaudit?voidedBy=cd8a4b8e-...&v=custom:(uuid,concept:(display),auditInfo)
+ * GET /openmrs/ws/rest/v1/pihapps/obsaudit?createdBy=<uuid>&startDate=2026-09-01&endDate=2026-09-30
+ *
+ * + *

`createdBy` and `voidedBy` are bound by core's property editors, so each takes a uuid or a + * primary key. + * + *

`startDate` and `endDate` bound when the audit action happened rather than the observation's + * own datetime, and run inclusively. + */ +@Controller +public class ObsAuditRestController { + + protected Log log = LogFactory.getLog(getClass()); + + /** + * The same gate as this package's other administrative endpoints. An observation audit reaches + * across every patient's record, so it is not something a clinical role should be able to run. + */ + private static final String REQUIRED_PRIVILEGE = "App: coreapps.systemAdministration"; + + /** + * Enough to say what was recorded and who touched it. `auditInfo` is what makes this an audit + * result: it carries the creator and the voiding user with their timestamps. + */ + private static final String DEFAULT_REPRESENTATION = + "(uuid,display,obsDatetime,voided,concept:(uuid,display),person:(uuid,display)," + + "encounter:(uuid),value:ref,comment,auditInfo)"; + + @Autowired + private PihAppsService pihAppsService; + + @RequestMapping(value = "/rest/v1/pihapps/obsaudit", method = RequestMethod.GET) + @ResponseBody + public Object searchObs(HttpServletRequest request, HttpServletResponse response, + @RequestParam(value = "createdBy", required = false) User createdBy, + @RequestParam(value = "voidedBy", required = false) User voidedBy, + @RequestParam(value = "startDate", required = false) String startDate, + @RequestParam(value = "endDate", required = false) String endDate) + throws ResponseException { + + if (!Context.hasPrivilege(REQUIRED_PRIVILEGE)) { + return new ResponseEntity<>(HttpStatus.UNAUTHORIZED); + } + + try { + if (createdBy == null && voidedBy == null) { + throw new InvalidSearchException("Please specify createdBy, voidedBy, or both."); + } + + Date fromDate; + Date toDate; + try { + fromDate = AuditRestSupport.parseBound(startDate, false); + toDate = AuditRestSupport.parseBound(endDate, true); + } + catch (Exception e) { + throw new InvalidSearchException(AuditRestSupport.dateFormatMessage(), e); + } + + if (fromDate != null && toDate != null && fromDate.after(toDate)) { + throw new InvalidSearchException("startDate must not be after endDate."); + } + + RequestContext context = RestUtil.getRequestContext(request, response, + new CustomRepresentation(DEFAULT_REPRESENTATION)); + + ObsSearchCriteria searchCriteria = new ObsSearchCriteria(); + searchCriteria.setCreatedBy(createdBy); + searchCriteria.setVoidedBy(voidedBy); + searchCriteria.setAuditOnOrAfter(fromDate); + searchCriteria.setAuditOnOrBefore(toDate); + searchCriteria.setStartIndex(context.getStartIndex()); + searchCriteria.setLimit(context.getLimit()); + + ObsSearchResult result = pihAppsService.getObsByAuditUser(searchCriteria); + Long totalCount = result.getTotalCount(); + boolean hasMore = totalCount > context.getStartIndex() + context.getLimit(); + + // AlreadyPaged renders the envelope every other OpenMRS list endpoint returns: results in + // the requested representation, next and previous links, and totalCount when asked for. + return new AlreadyPaged<>(context, result.getObs(), hasMore, totalCount).toSimpleObject(null); + } + catch (InvalidSearchException e) { + response.setStatus(HttpServletResponse.SC_BAD_REQUEST); + return RestUtil.wrapErrorResponse(e, AuditRestSupport.INVALID_SEARCH_REASON); + } + catch (Exception e) { + log.error("Failed to search observations by audit user", e); + response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + return RestUtil.wrapErrorResponse(e, "Failed to search observations by audit user"); + } + } + + /** + * A parameter core's editors could not resolve is still a bad request, so it is answered in the + * same shape as every other one this endpoint rejects rather than in Spring's own error format. + */ + @ExceptionHandler(MethodArgumentTypeMismatchException.class) + @ResponseStatus(HttpStatus.BAD_REQUEST) + @ResponseBody + public SimpleObject handleUnresolvedParameter(MethodArgumentTypeMismatchException e) { + return AuditRestSupport.unresolvedParameterResponse(e); + } +} From 7a75ddb780d9a21dd44a4d93a414df1bb12e9988 Mon Sep 17 00:00:00 2001 From: Cosmin Date: Tue, 22 Sep 2026 16:05:11 -0400 Subject: [PATCH 2/6] UHM-9514: address PR review feedback --- .../module/pihapps/PihAppsService.java | 45 +++++---- .../module/pihapps/PihAppsServiceImpl.java | 96 ++----------------- .../encounter/EncounterSearchCriteria.java | 34 +++---- .../module/pihapps/obs/ObsSearchCriteria.java | 12 ++- .../pihapps/EncounterAuditSearchTest.java | 73 +++++++++++--- .../module/pihapps/ObsAuditSearchTest.java | 50 ++++++++-- .../rest/EncounterAuditRestController.java | 49 ++++++++-- .../pihapps/rest/ObsAuditRestController.java | 33 ++++++- 8 files changed, 238 insertions(+), 154 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/pihapps/PihAppsService.java b/api/src/main/java/org/openmrs/module/pihapps/PihAppsService.java index 7cd37d4..76d5bfa 100644 --- a/api/src/main/java/org/openmrs/module/pihapps/PihAppsService.java +++ b/api/src/main/java/org/openmrs/module/pihapps/PihAppsService.java @@ -55,36 +55,47 @@ public interface PihAppsService extends OpenmrsService { void revertOrdersToOrdered(List orders); - ObsSearchResult getObs(ObsSearchCriteria searchCriteria); - /** - * Observations whose audit trail names the given user, most recent action first. Voided - * observations are included, since they are the whole point of a voidedBy search and are what - * an auditor most wants to see in a createdBy one. + * Searches observations by whatever {@link ObsSearchCriteria} names: the patient, the concepts, + * the users in their audit trail, and a date range over either the observation's own datetime or + * the audit action. Every filter narrows, and a criteria naming none of them matches every + * observation, so a caller that means to search rather than to list is responsible for + * narrowing it. + * + *

Voided observations are left out unless the criteria ask for them. An audit does ask: they + * are the whole point of a voidedBy search, and what an auditor looking at what a user created + * most wants to see. * - *

The search is described by {@code createdBy}, {@code voidedBy} and the audit date bounds - * on {@link ObsSearchCriteria}; ordering defaults to the audit action the search named. At - * least one of the two users is required. + *

Ordering is the caller's to set, and paging without one is not deterministic. An audit + * wants the most recent audit action first, which means ordering by the column belonging to the + * action it named rather than by the observation's own datetime, with the obs id breaking ties + * so that paging cannot repeat or skip a row. * * @param searchCriteria what to search for, how to page it and how to sort it * @return the matching observations and how many there are in total - * @throws org.openmrs.api.APIException if neither user is given */ @Authorized(PrivilegeConstants.GET_OBS) - ObsSearchResult getObsByAuditUser(ObsSearchCriteria searchCriteria); + ObsSearchResult getObs(ObsSearchCriteria searchCriteria); /** - * Encounters whose audit trail names the given user, or that name the given provider, most - * recent action first. Voided encounters are included, for the same reason voided observations - * are in {@link #getObsByAuditUser(ObsSearchCriteria)}. + * Searches encounters by whatever {@link EncounterSearchCriteria} names: the users in their + * audit trail, the provider recorded on them, their type, and a date range. Every filter + * narrows, and a criteria naming none of them matches every encounter, so a caller that means + * to search rather than to list is responsible for narrowing it. + * + *

Voided encounters are left out unless the criteria ask for them. An audit does ask: they + * are the whole point of a voidedBy search, and what an auditor looking at what a user entered + * most wants to see. * - *

The search is described by {@link EncounterSearchCriteria}; ordering defaults to the audit - * action the search named. At least one user or provider is required. + *

Ordering is the caller's to set, as it is on {@link #getObs(ObsSearchCriteria)}, + * and paging without one is not deterministic. An audit wants the most recent audit action + * first, which means ordering by the column belonging to the action it named — or by the + * encounter's own datetime where only a provider was named — with the encounter id breaking + * ties so that paging cannot repeat or skip a row. * * @param searchCriteria what to search for, how to page it and how to sort it * @return the matching encounters and how many there are in total - * @throws org.openmrs.api.APIException if no user and no provider is given */ @Authorized(PrivilegeConstants.GET_ENCOUNTERS) - EncounterSearchResult getEncountersByAuditUser(EncounterSearchCriteria searchCriteria); + EncounterSearchResult getEncounters(EncounterSearchCriteria searchCriteria); } diff --git a/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java b/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java index 25905d9..78906c4 100644 --- a/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java +++ b/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java @@ -36,7 +36,6 @@ import org.openmrs.Provider; import org.openmrs.User; import org.openmrs.annotation.Authorized; -import org.openmrs.api.APIException; import org.openmrs.api.EncounterService; import org.openmrs.api.LocationService; import org.openmrs.api.ObsService; @@ -585,11 +584,6 @@ public void revertOrdersToOrdered(List orders) { } } - /** - * Unlike the obsDatetime bounds, these are applied exactly as given rather than widened to whole - * days: the caller has already said which moment it means, and an audit range is often asked to - * the second. - */ private void addAuditDateBounds(Criteria c, String property, ObsSearchCriteria searchCriteria) { if (searchCriteria.getAuditOnOrAfter() != null) { c.add(ge(property, searchCriteria.getAuditOnOrAfter())); @@ -601,7 +595,7 @@ private void addAuditDateBounds(Criteria c, String property, ObsSearchCriteria s @Override @Transactional(readOnly = true) - @Authorized(PrivilegeConstants.GET_PATIENTS) + @Authorized(PrivilegeConstants.GET_OBS) @SuppressWarnings({ "unchecked" }) public ObsSearchResult getObs(ObsSearchCriteria searchCriteria) { ObsSearchResult result = new ObsSearchResult(); @@ -628,11 +622,7 @@ public ObsSearchResult getObs(ObsSearchCriteria searchCriteria) { @SuppressWarnings({ "deprecation" }) private Criteria createHibernateObsSearchCriteria(ObsSearchCriteria searchCriteria, boolean applySortCriteria) { Criteria c = sessionFactory.getHibernateSessionFactory().getCurrentSession().createCriteria(Obs.class); - // An audit search leaves off the voided filter every other obs search applies: a voidedBy - // search would otherwise return nothing, and an auditor looking at what a user created wants - // to see the rows that have since been deleted just as much as the surviving ones. Callers - // can tell them apart by each observation's voided flag. - if (searchCriteria.getCreatedBy() == null && searchCriteria.getVoidedBy() == null) { + if (!searchCriteria.isIncludeVoided()) { c.add(eq("voided", false)); } if (searchCriteria.getCreatedBy() != null) { @@ -672,50 +662,11 @@ private Criteria createHibernateObsSearchCriteria(ObsSearchCriteria searchCriter return c; } - @Override - @Transactional(readOnly = true) - @Authorized(PrivilegeConstants.GET_OBS) - public ObsSearchResult getObsByAuditUser(ObsSearchCriteria searchCriteria) { - if (searchCriteria.getCreatedBy() == null && searchCriteria.getVoidedBy() == null) { - throw new APIException("An observation audit search needs at least one of createdBy or voidedBy"); - } - // An audit has an order of its own, so fill it in unless the caller asked for another. - if (searchCriteria.getSortCriteria() == null || searchCriteria.getSortCriteria().isEmpty()) { - searchCriteria.setSortCriteria(auditSortCriteria(searchCriteria)); - } - return getObs(searchCriteria); - } - - /** - * "Most recent first" means the most recent audit action: when the row was created for a - * createdBy search, when it was voided for a voidedBy search. Ordering by the observation's own - * datetime would bury an obs backdated to last year but entered this morning, which is the - * opposite of what an audit needs. The obs id breaks ties so that paging cannot repeat or skip - * a row when several share a timestamp. - */ - private List auditSortCriteria(ObsSearchCriteria searchCriteria) { - String actionDate = searchCriteria.getVoidedBy() != null ? "dateVoided" : "dateCreated"; - List sortCriteria = new ArrayList<>(); - sortCriteria.add(new SortCriteria(actionDate, SortCriteria.Direction.DESC)); - sortCriteria.add(new SortCriteria("obsId", SortCriteria.Direction.DESC)); - return sortCriteria; - } - @Override @Transactional(readOnly = true) @Authorized(PrivilegeConstants.GET_ENCOUNTERS) @SuppressWarnings({ "unchecked" }) - public EncounterSearchResult getEncountersByAuditUser(EncounterSearchCriteria searchCriteria) { - if (searchCriteria.getCreatedBy() == null && searchCriteria.getChangedBy() == null - && searchCriteria.getVoidedBy() == null && searchCriteria.getProvider() == null) { - throw new APIException( - "An encounter audit search needs at least one of createdBy, changedBy, voidedBy or provider"); - } - // An audit has an order of its own, so fill it in unless the caller asked for another. - if (searchCriteria.getSortCriteria() == null || searchCriteria.getSortCriteria().isEmpty()) { - searchCriteria.setSortCriteria(encounterAuditSortCriteria(searchCriteria)); - } - + public EncounterSearchResult getEncounters(EncounterSearchCriteria searchCriteria) { EncounterSearchResult result = new EncounterSearchResult(); // First query to get total count Criteria c = createHibernateEncounterSearchCriteria(searchCriteria, false); @@ -736,38 +687,6 @@ public EncounterSearchResult getEncountersByAuditUser(EncounterSearchCriteria se return result; } - /** - * Orders by the most recent of the audit actions the search named, so that "most recent first" - * means the most recent thing the search is about, and matches whichever column the date bounds - * were applied to. A provider search names no action, so it orders by the encounter's own - * datetime. The encounter id breaks ties so that paging cannot repeat or skip a row when - * several share a timestamp. - */ - private List encounterAuditSortCriteria(EncounterSearchCriteria searchCriteria) { - List sortCriteria = new ArrayList<>(); - sortCriteria.add(new SortCriteria(encounterAuditSortProperty(searchCriteria), SortCriteria.Direction.DESC)); - sortCriteria.add(new SortCriteria("encounterId", SortCriteria.Direction.DESC)); - return sortCriteria; - } - - /** - * The column the results are ordered by: the named audit action, taking the most recent kind of - * action when a search named several, or the encounter's own datetime when only a provider was - * named. - */ - private String encounterAuditSortProperty(EncounterSearchCriteria searchCriteria) { - if (searchCriteria.getVoidedBy() != null) { - return "dateVoided"; - } - if (searchCriteria.getChangedBy() != null) { - return "dateChanged"; - } - if (searchCriteria.getCreatedBy() != null) { - return "dateCreated"; - } - return "encounterDatetime"; - } - /** * Unlike the obsDatetime bounds on an obs search, these are applied exactly as given rather than * widened to whole days: the caller has already said which moment it means. @@ -781,16 +700,13 @@ private void addEncounterAuditDateBounds(Criteria c, String property, EncounterS } } - /** - * Voided encounters are deliberately left in, for the same reason voided observations are: a - * search for who voided something would otherwise return nothing, and an auditor looking at - * what a user entered wants to see what has since been deleted. Callers can tell them apart by - * each encounter's voided flag. - */ @SuppressWarnings({ "deprecation" }) private Criteria createHibernateEncounterSearchCriteria(EncounterSearchCriteria searchCriteria, boolean applySortCriteria) { Criteria c = sessionFactory.getHibernateSessionFactory().getCurrentSession().createCriteria(Encounter.class); + if (!searchCriteria.isIncludeVoided()) { + c.add(eq("voided", false)); + } if (searchCriteria.getEncounterType() != null) { c.add(eq("encounterType", searchCriteria.getEncounterType())); } diff --git a/api/src/main/java/org/openmrs/module/pihapps/encounter/EncounterSearchCriteria.java b/api/src/main/java/org/openmrs/module/pihapps/encounter/EncounterSearchCriteria.java index 9603d8d..abed4b4 100644 --- a/api/src/main/java/org/openmrs/module/pihapps/encounter/EncounterSearchCriteria.java +++ b/api/src/main/java/org/openmrs/module/pihapps/encounter/EncounterSearchCriteria.java @@ -10,37 +10,39 @@ import java.util.List; /** - * Describes an encounter search over the audit trail. Core's own - * {@link org.openmrs.parameter.EncounterSearchCriteria} carries a providers field that no search - * handler exposes, and has no creator, changedBy or voidedBy field at all, which is why this exists - * rather than the module reusing it. + * Describes an encounter search. Core's own {@link org.openmrs.parameter.EncounterSearchCriteria} + * carries a providers field that no search handler exposes, and has no creator, changedBy or + * voidedBy field at all, which is why this exists rather than the module reusing it. * - *

Every filter narrows, so naming several asks for the encounters satisfying all of them. + *

Every filter narrows, so naming several asks for the encounters satisfying all of them, and + * naming none matches every encounter. */ @Data public class EncounterSearchCriteria { /** - * Restrict to encounters this user created. Naming any of {@link #createdBy}, {@link #changedBy}, - * {@link #voidedBy} or {@link #provider} makes the search an audit, which also means voided - * encounters are included: they are the whole point of a voidedBy search, and what an auditor - * most wants to see in the others. + * Whether voided encounters are returned alongside the surviving ones. Off by default, since a + * search is normally asking what a record says now, and callers can tell the two apart by each + * encounter's voided flag. + * + *

An audit search turns this on: voided encounters are the whole point of a + * {@link #voidedBy} search, and what an auditor most wants to see in the others. */ + private boolean includeVoided = false; + + /** Restrict to encounters this user created. */ private User createdBy; - /** Restrict to encounters this user changed. See {@link #createdBy}. */ + /** Restrict to encounters this user changed. */ private User changedBy; - /** Restrict to encounters this user voided. See {@link #createdBy}. */ + /** Restrict to encounters this user voided. */ private User voidedBy; - /** Restrict to encounters this provider is recorded on. See {@link #createdBy}. */ + /** Restrict to encounters this provider is recorded on. */ private Provider provider; - /** - * Restrict to encounters of this type. This narrows an audit but is not an audit of anything on - * its own, so it does not satisfy the requirement that one of the four above is given. - */ + /** Restrict to encounters of this type. */ private EncounterType encounterType; /** diff --git a/api/src/main/java/org/openmrs/module/pihapps/obs/ObsSearchCriteria.java b/api/src/main/java/org/openmrs/module/pihapps/obs/ObsSearchCriteria.java index 28979ff..6c0e3fe 100644 --- a/api/src/main/java/org/openmrs/module/pihapps/obs/ObsSearchCriteria.java +++ b/api/src/main/java/org/openmrs/module/pihapps/obs/ObsSearchCriteria.java @@ -17,10 +17,16 @@ public class ObsSearchCriteria { private Date onOrAfter; /** - * Restrict to observations this user created. Naming either this or {@link #voidedBy} makes the - * search an audit, which also means voided observations are included: they are the whole point - * of a voidedBy search, and what an auditor most wants to see in a createdBy one. + * Whether voided observations are returned alongside the surviving ones. Off by default, since + * a search is normally asking what a record says now, and callers can tell the two apart by + * each observation's voided flag. + * + *

An audit search turns this on: voided observations are the whole point of a + * {@link #voidedBy} search, and what an auditor most wants to see in a {@link #createdBy} one. */ + private boolean includeVoided = false; + + /** Restrict to observations this user created. */ private User createdBy; /** Restrict to observations this user voided. See {@link #createdBy}. */ diff --git a/api/src/test/java/org/openmrs/module/pihapps/EncounterAuditSearchTest.java b/api/src/test/java/org/openmrs/module/pihapps/EncounterAuditSearchTest.java index 190b6fa..030d7b7 100644 --- a/api/src/test/java/org/openmrs/module/pihapps/EncounterAuditSearchTest.java +++ b/api/src/test/java/org/openmrs/module/pihapps/EncounterAuditSearchTest.java @@ -6,12 +6,13 @@ import org.openmrs.EncounterType; import org.openmrs.Provider; import org.openmrs.User; -import org.openmrs.api.APIException; import org.openmrs.api.context.Context; +import org.openmrs.module.pihapps.SortCriteria; import org.openmrs.module.pihapps.encounter.EncounterSearchCriteria; import org.openmrs.module.pihapps.encounter.EncounterSearchResult; import org.openmrs.test.jupiter.BaseModuleContextSensitiveTest; +import java.util.ArrayList; import java.util.Calendar; import java.util.Collections; import java.util.Date; @@ -20,8 +21,11 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.hasItem; +import static org.hamcrest.Matchers.hasItems; +import static org.hamcrest.Matchers.lessThan; +import static org.hamcrest.Matchers.not; import static org.hamcrest.Matchers.is; -import static org.junit.jupiter.api.Assertions.assertThrows; /** * Covers searching encounters by the users in their audit trail, by the provider recorded on them, @@ -65,9 +69,33 @@ private EncounterSearchResult searchResult(User createdBy, User changedBy, User searchCriteria.setEncounterType(encounterType); searchCriteria.setAuditOnOrAfter(fromDate); searchCriteria.setAuditOnOrBefore(toDate); + searchCriteria.setIncludeVoided(true); + searchCriteria.setSortCriteria(auditSortCriteria(createdBy, changedBy, voidedBy)); searchCriteria.setStartIndex(startIndex); searchCriteria.setLimit(limit); - return service.getEncountersByAuditUser(searchCriteria); + return service.getEncounters(searchCriteria); + } + + /** + * The ordering an audit asks for, which the endpoint sets rather than the service: the audit + * action the search named, most recent first, with the encounter id breaking ties. These tests + * assert on order, so they have to ask for the same one the endpoint does. + */ + private List auditSortCriteria(User createdBy, User changedBy, User voidedBy) { + String actionDate = "encounterDatetime"; + if (voidedBy != null) { + actionDate = "dateVoided"; + } + else if (changedBy != null) { + actionDate = "dateChanged"; + } + else if (createdBy != null) { + actionDate = "dateCreated"; + } + List sortCriteria = new ArrayList<>(); + sortCriteria.add(new SortCriteria(actionDate, SortCriteria.Direction.DESC)); + sortCriteria.add(new SortCriteria("encounterId", SortCriteria.Direction.DESC)); + return sortCriteria; } /** The search under test, with the paging arguments left off. */ @@ -163,11 +191,6 @@ public void shouldNarrowByEncounterTypeAndProviderTogether() { assertThat(auditedIds(search(null, null, null, provider, otherType, null, null)), is(Collections.emptyList())); } - @Test - public void shouldRefuseATypeOnlySearch() { - assertThrows(APIException.class, () -> search(null, null, null, null, typeOne, null, null)); - } - @Test public void shouldBoundEachNamedActionByItsOwnDateColumn() { // bruno created 3001 on 1 Sep and 3002 on 25 Aug @@ -233,9 +256,37 @@ public void shouldPageThroughTheResults() { contains(3003)); } + /** + * The audit endpoint asks for voided encounters, so the helper above does too. Every other + * caller gets them left out, which is what this pins down. + */ + @Test + public void shouldLeaveOutVoidedEncountersUnlessAskedFor() { + // 3003 is this fixture's voided encounter, voided by butch on 5 Sep + EncounterSearchCriteria searchCriteria = new EncounterSearchCriteria(); + searchCriteria.setEncounterType(typeOne); + + assertThat(auditedIds(service.getEncounters(searchCriteria).getEncounters()), not(hasItem(3003))); + + searchCriteria.setIncludeVoided(true); + assertThat(auditedIds(service.getEncounters(searchCriteria).getEncounters()), hasItem(3003)); + } + @Test - public void shouldRefuseASearchThatNamesNeitherUserNorProvider() { - assertThrows(APIException.class, () -> search(null, null, null, null, null, september(1), null)); - assertThrows(APIException.class, () -> count(null, null, null, null, null, null, null)); + public void shouldSearchWithoutAnyAuditFilter() { + // The service places no audit-specific requirement on the criteria, so a search naming none + // of them is a plain encounter search. The audit endpoint asks for at least one itself. + List unfiltered = search(null, null, null, null, null, null, null); + + assertThat(auditedIds(unfiltered), hasItems(3001, 3002, 3003, 3004, 3005)); + assertThat(count(null, null, null, null, null, null, null), is((long) unfiltered.size())); + } + + @Test + public void shouldNarrowAnUnfilteredSearchByEncounterTypeAlone() { + List byType = search(null, null, null, null, typeOne, null, null); + + assertThat(byType.stream().allMatch(e -> e.getEncounterType().equals(typeOne)), is(true)); + assertThat(byType.size(), lessThan(search(null, null, null, null, null, null, null).size())); } } diff --git a/api/src/test/java/org/openmrs/module/pihapps/ObsAuditSearchTest.java b/api/src/test/java/org/openmrs/module/pihapps/ObsAuditSearchTest.java index 10ed9e7..48201b7 100644 --- a/api/src/test/java/org/openmrs/module/pihapps/ObsAuditSearchTest.java +++ b/api/src/test/java/org/openmrs/module/pihapps/ObsAuditSearchTest.java @@ -4,12 +4,13 @@ import org.junit.jupiter.api.Test; import org.openmrs.Obs; import org.openmrs.User; -import org.openmrs.api.APIException; import org.openmrs.api.context.Context; import org.openmrs.module.pihapps.obs.ObsSearchCriteria; +import org.openmrs.module.pihapps.SortCriteria; import org.openmrs.module.pihapps.obs.ObsSearchResult; import org.openmrs.test.jupiter.BaseModuleContextSensitiveTest; +import java.util.ArrayList; import java.util.Calendar; import java.util.Date; import java.util.List; @@ -17,8 +18,9 @@ import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.contains; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.hasItems; import static org.hamcrest.Matchers.is; -import static org.junit.jupiter.api.Assertions.assertThrows; /** * Covers searching observations by the user who created or voided them. The fixture's creation and @@ -56,9 +58,24 @@ private ObsSearchResult search(User createdBy, User voidedBy, Date fromDate, Dat searchCriteria.setVoidedBy(voidedBy); searchCriteria.setAuditOnOrAfter(fromDate); searchCriteria.setAuditOnOrBefore(toDate); + searchCriteria.setIncludeVoided(true); + searchCriteria.setSortCriteria(auditSortCriteria(voidedBy)); searchCriteria.setStartIndex(startIndex); searchCriteria.setLimit(limit); - return service.getObsByAuditUser(searchCriteria); + return service.getObs(searchCriteria); + } + + /** + * The ordering an audit asks for, which the endpoint sets rather than the service: the audit + * action the search named, most recent first, with the obs id breaking ties. These tests assert + * on order, so they have to ask for the same one the endpoint does. + */ + private List auditSortCriteria(User voidedBy) { + List sortCriteria = new ArrayList<>(); + String actionDate = voidedBy != null ? "dateVoided" : "dateCreated"; + sortCriteria.add(new SortCriteria(actionDate, SortCriteria.Direction.DESC)); + sortCriteria.add(new SortCriteria("obsId", SortCriteria.Direction.DESC)); + return sortCriteria; } private List obs(User createdBy, User voidedBy, Date fromDate, Date toDate, Integer startIndex, @@ -184,10 +201,31 @@ public void shouldFindNothingWhenTheRangeExcludesEverything() { assertThat(count(bruno, null, september(20, 0), september(21, 0)), is(0L)); } + /** + * The audit endpoint asks for voided observations, so the helper above does too. Every other + * caller gets them left out, which is what this pins down. + */ + @Test + public void shouldLeaveOutVoidedObsUnlessAskedFor() { + // 2004 and 2005 are this fixture's voided observations, both voided by butch + ObsSearchCriteria searchCriteria = new ObsSearchCriteria(); + searchCriteria.setVoidedBy(butch); + + assertThat(service.getObs(searchCriteria).getObs(), is(java.util.Collections.emptyList())); + + searchCriteria.setIncludeVoided(true); + assertThat(obsIds(service.getObs(searchCriteria).getObs()), containsInAnyOrder(2004, 2005)); + } + @Test - public void shouldRefuseAnUnfilteredSearch() { - assertThrows(APIException.class, () -> obs(null, null, null, null, null, null)); - assertThrows(APIException.class, () -> count(null, null, null, null)); + public void shouldSearchWithoutAnyAuditFilter() { + // The service places no audit-specific requirement on the criteria, so a search naming none + // of them is a plain obs search. The audit endpoint asks for at least one itself. + ObsSearchCriteria searchCriteria = new ObsSearchCriteria(); + searchCriteria.setIncludeVoided(true); + + assertThat(obsIds(service.getObs(searchCriteria).getObs()), + hasItems(2001, 2002, 2003, 2004, 2005)); } } diff --git a/omod/src/main/java/org/openmrs/module/pihapps/rest/EncounterAuditRestController.java b/omod/src/main/java/org/openmrs/module/pihapps/rest/EncounterAuditRestController.java index 4d70ce2..99bf107 100644 --- a/omod/src/main/java/org/openmrs/module/pihapps/rest/EncounterAuditRestController.java +++ b/omod/src/main/java/org/openmrs/module/pihapps/rest/EncounterAuditRestController.java @@ -9,6 +9,7 @@ import org.openmrs.api.EncounterService; import org.openmrs.api.context.Context; import org.openmrs.module.pihapps.PihAppsService; +import org.openmrs.module.pihapps.SortCriteria; import org.openmrs.module.pihapps.encounter.EncounterSearchCriteria; import org.openmrs.module.pihapps.encounter.EncounterSearchResult; import org.openmrs.module.webservices.rest.SimpleObject; @@ -32,7 +33,9 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import java.util.ArrayList; import java.util.Date; +import java.util.List; /** * Searches encounters by the user who created, changed or voided them, and by the provider recorded @@ -45,10 +48,10 @@ * whatever it needs with `v`: * * 

- * GET /openmrs/ws/rest/v1/pihapps/encounteraudit?createdBy=<uuid>&limit=20&totalCount=true
- * GET /openmrs/ws/rest/v1/pihapps/encounteraudit?changedBy=<uuid>&startDate=2026-09-01&endDate=2026-09-30
- * GET /openmrs/ws/rest/v1/pihapps/encounteraudit?provider=<uuid>&v=custom:(uuid,encounterDatetime,auditInfo)
- * GET /openmrs/ws/rest/v1/pihapps/encounteraudit?provider=<uuid>&encounterType=<uuid>
+ * GET /openmrs/ws/rest/v1/pihapps/encounter?createdBy=<uuid>&limit=20&totalCount=true
+ * GET /openmrs/ws/rest/v1/pihapps/encounter?changedBy=<uuid>&startDate=2026-09-01&endDate=2026-09-30
+ * GET /openmrs/ws/rest/v1/pihapps/encounter?provider=<uuid>&v=custom:(uuid,encounterDatetime,auditInfo)
+ * GET /openmrs/ws/rest/v1/pihapps/encounter?provider=<uuid>&encounterType=<uuid>
  *
* *

`createdBy`, `changedBy`, `voidedBy` and `provider` are bound by core's property editors, so @@ -86,7 +89,7 @@ public class EncounterAuditRestController { @Autowired private PihAppsService pihAppsService; - @RequestMapping(value = "/rest/v1/pihapps/encounteraudit", method = RequestMethod.GET) + @RequestMapping(value = "/rest/v1/pihapps/encounter", method = RequestMethod.GET) @ResponseBody public Object searchEncounters(HttpServletRequest request, HttpServletResponse response, @RequestParam(value = "createdBy", required = false) User createdBy, @@ -139,12 +142,17 @@ public Object searchEncounters(HttpServletRequest request, HttpServletResponse r searchCriteria.setVoidedBy(voidedBy); searchCriteria.setProvider(provider); searchCriteria.setEncounterType(type); + // A voidedBy search would return nothing with the voided rows filtered out, and an + // auditor looking at what a user entered wants to see what has since been deleted just + // as much as what survives, so an audit always asks for them. + searchCriteria.setIncludeVoided(true); searchCriteria.setAuditOnOrAfter(fromDate); searchCriteria.setAuditOnOrBefore(toDate); + searchCriteria.setSortCriteria(auditSortCriteria(createdBy, changedBy, voidedBy)); searchCriteria.setStartIndex(context.getStartIndex()); searchCriteria.setLimit(context.getLimit()); - EncounterSearchResult result = pihAppsService.getEncountersByAuditUser(searchCriteria); + EncounterSearchResult result = pihAppsService.getEncounters(searchCriteria); Long totalCount = result.getTotalCount(); boolean hasMore = totalCount > context.getStartIndex() + context.getLimit(); @@ -161,6 +169,35 @@ public Object searchEncounters(HttpServletRequest request, HttpServletResponse r } } + /** + * Orders by the most recent of the audit actions the search named, so that "most recent first" + * means the most recent thing the search is about, and matches whichever column the date bounds + * were applied to. A provider search names no action, so it orders by the encounter's own + * datetime. The encounter id breaks ties so that paging cannot repeat or skip a row when + * several share a timestamp. + */ + private List auditSortCriteria(User createdBy, User changedBy, User voidedBy) { + List sortCriteria = new ArrayList<>(); + sortCriteria.add(new SortCriteria(auditSortProperty(createdBy, changedBy, voidedBy), + SortCriteria.Direction.DESC)); + sortCriteria.add(new SortCriteria("encounterId", SortCriteria.Direction.DESC)); + return sortCriteria; + } + + /** The named audit action, taking the most recent kind of action when a search named several. */ + private String auditSortProperty(User createdBy, User changedBy, User voidedBy) { + if (voidedBy != null) { + return "dateVoided"; + } + if (changedBy != null) { + return "dateChanged"; + } + if (createdBy != null) { + return "dateCreated"; + } + return "encounterDatetime"; + } + /** * A parameter core's editors could not resolve is still a bad request, so it is answered in the * same shape as every other one this endpoint rejects rather than in Spring's own error format. diff --git a/omod/src/main/java/org/openmrs/module/pihapps/rest/ObsAuditRestController.java b/omod/src/main/java/org/openmrs/module/pihapps/rest/ObsAuditRestController.java index 44af7be..cd169a2 100644 --- a/omod/src/main/java/org/openmrs/module/pihapps/rest/ObsAuditRestController.java +++ b/omod/src/main/java/org/openmrs/module/pihapps/rest/ObsAuditRestController.java @@ -5,6 +5,7 @@ import org.openmrs.User; import org.openmrs.api.context.Context; import org.openmrs.module.pihapps.PihAppsService; +import org.openmrs.module.pihapps.SortCriteria; import org.openmrs.module.pihapps.obs.ObsSearchCriteria; import org.openmrs.module.pihapps.obs.ObsSearchResult; import org.openmrs.module.webservices.rest.SimpleObject; @@ -28,7 +29,9 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; +import java.util.ArrayList; import java.util.Date; +import java.util.List; /** * Searches observations by the user who created them or the user who voided them, which the core @@ -40,9 +43,9 @@ * standard obs representation, so a client can ask for whatever it needs with `v`: * * 

- * GET /openmrs/ws/rest/v1/pihapps/obsaudit?createdBy=<uuid>&limit=20&totalCount=true
- * GET /openmrs/ws/rest/v1/pihapps/obsaudit?voidedBy=cd8a4b8e-...&v=custom:(uuid,concept:(display),auditInfo)
- * GET /openmrs/ws/rest/v1/pihapps/obsaudit?createdBy=<uuid>&startDate=2026-09-01&endDate=2026-09-30
+ * GET /openmrs/ws/rest/v1/pihapps/obs?createdBy=<uuid>&limit=20&totalCount=true
+ * GET /openmrs/ws/rest/v1/pihapps/obs?voidedBy=cd8a4b8e-...&v=custom:(uuid,concept:(display),auditInfo)
+ * GET /openmrs/ws/rest/v1/pihapps/obs?createdBy=<uuid>&startDate=2026-09-01&endDate=2026-09-30
  *
* *

`createdBy` and `voidedBy` are bound by core's property editors, so each takes a uuid or a @@ -73,7 +76,7 @@ public class ObsAuditRestController { @Autowired private PihAppsService pihAppsService; - @RequestMapping(value = "/rest/v1/pihapps/obsaudit", method = RequestMethod.GET) + @RequestMapping(value = "/rest/v1/pihapps/obs", method = RequestMethod.GET) @ResponseBody public Object searchObs(HttpServletRequest request, HttpServletResponse response, @RequestParam(value = "createdBy", required = false) User createdBy, @@ -113,10 +116,15 @@ public Object searchObs(HttpServletRequest request, HttpServletResponse response searchCriteria.setVoidedBy(voidedBy); searchCriteria.setAuditOnOrAfter(fromDate); searchCriteria.setAuditOnOrBefore(toDate); + // A voidedBy search would return nothing with the voided rows filtered out, and an + // auditor looking at what a user created wants to see what has since been deleted just + // as much as what survives, so an audit always asks for them. + searchCriteria.setIncludeVoided(true); + searchCriteria.setSortCriteria(auditSortCriteria(voidedBy)); searchCriteria.setStartIndex(context.getStartIndex()); searchCriteria.setLimit(context.getLimit()); - ObsSearchResult result = pihAppsService.getObsByAuditUser(searchCriteria); + ObsSearchResult result = pihAppsService.getObs(searchCriteria); Long totalCount = result.getTotalCount(); boolean hasMore = totalCount > context.getStartIndex() + context.getLimit(); @@ -135,6 +143,21 @@ public Object searchObs(HttpServletRequest request, HttpServletResponse response } } + /** + * "Most recent first" means the most recent audit action: when the row was created for a + * createdBy search, when it was voided for a voidedBy search. Ordering by the observation's own + * datetime would bury an obs backdated to last year but entered this morning, which is the + * opposite of what an audit needs. The obs id breaks ties so that paging cannot repeat or skip + * a row when several share a timestamp. + */ + private List auditSortCriteria(User voidedBy) { + List sortCriteria = new ArrayList<>(); + String actionDate = voidedBy != null ? "dateVoided" : "dateCreated"; + sortCriteria.add(new SortCriteria(actionDate, SortCriteria.Direction.DESC)); + sortCriteria.add(new SortCriteria("obsId", SortCriteria.Direction.DESC)); + return sortCriteria; + } + /** * A parameter core's editors could not resolve is still a bad request, so it is answered in the * same shape as every other one this endpoint rejects rather than in Spring's own error format. From 2341ad095cc5a8f783e61800ae399e401a5c1cb4 Mon Sep 17 00:00:00 2001 From: Cosmin Date: Wed, 23 Sep 2026 15:13:19 -0400 Subject: [PATCH 3/6] UHM-9514: generalize encounter and obs rest endpoints --- ...t.java => PihAppsEncounterSearchTest.java} | 4 +- ...rchTest.java => PihAppsObsSearchTest.java} | 2 +- .../module/pihapps/rest/AuditRestSupport.java | 70 ----------- ...va => PihAppsEncounterRestController.java} | 96 ++++++--------- ...ler.java => PihAppsObsRestController.java} | 67 +++++----- .../pihapps/rest/PihAppsRestSupport.java | 115 ++++++++++++++++++ 6 files changed, 186 insertions(+), 168 deletions(-) rename api/src/test/java/org/openmrs/module/pihapps/{EncounterAuditSearchTest.java => PihAppsEncounterSearchTest.java} (99%) rename api/src/test/java/org/openmrs/module/pihapps/{ObsAuditSearchTest.java => PihAppsObsSearchTest.java} (99%) delete mode 100644 omod/src/main/java/org/openmrs/module/pihapps/rest/AuditRestSupport.java rename omod/src/main/java/org/openmrs/module/pihapps/rest/{EncounterAuditRestController.java => PihAppsEncounterRestController.java} (69%) rename omod/src/main/java/org/openmrs/module/pihapps/rest/{ObsAuditRestController.java => PihAppsObsRestController.java} (72%) create mode 100644 omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsRestSupport.java diff --git a/api/src/test/java/org/openmrs/module/pihapps/EncounterAuditSearchTest.java b/api/src/test/java/org/openmrs/module/pihapps/PihAppsEncounterSearchTest.java similarity index 99% rename from api/src/test/java/org/openmrs/module/pihapps/EncounterAuditSearchTest.java rename to api/src/test/java/org/openmrs/module/pihapps/PihAppsEncounterSearchTest.java index 030d7b7..22f3d1b 100644 --- a/api/src/test/java/org/openmrs/module/pihapps/EncounterAuditSearchTest.java +++ b/api/src/test/java/org/openmrs/module/pihapps/PihAppsEncounterSearchTest.java @@ -23,9 +23,9 @@ import static org.hamcrest.Matchers.contains; import static org.hamcrest.Matchers.hasItem; import static org.hamcrest.Matchers.hasItems; +import static org.hamcrest.Matchers.is; import static org.hamcrest.Matchers.lessThan; import static org.hamcrest.Matchers.not; -import static org.hamcrest.Matchers.is; /** * Covers searching encounters by the users in their audit trail, by the provider recorded on them, @@ -33,7 +33,7 @@ * encounter datetimes, so an implementation that bounded or ordered by the wrong column would fail * here rather than look plausible. */ -public class EncounterAuditSearchTest extends BaseModuleContextSensitiveTest { +public class PihAppsEncounterSearchTest extends BaseModuleContextSensitiveTest { private PihAppsService service; diff --git a/api/src/test/java/org/openmrs/module/pihapps/ObsAuditSearchTest.java b/api/src/test/java/org/openmrs/module/pihapps/PihAppsObsSearchTest.java similarity index 99% rename from api/src/test/java/org/openmrs/module/pihapps/ObsAuditSearchTest.java rename to api/src/test/java/org/openmrs/module/pihapps/PihAppsObsSearchTest.java index 48201b7..23ab71d 100644 --- a/api/src/test/java/org/openmrs/module/pihapps/ObsAuditSearchTest.java +++ b/api/src/test/java/org/openmrs/module/pihapps/PihAppsObsSearchTest.java @@ -27,7 +27,7 @@ * voiding dates run in a different order from the observation datetimes, so an implementation that * ordered by the wrong column would fail here rather than look plausible. */ -public class ObsAuditSearchTest extends BaseModuleContextSensitiveTest { +public class PihAppsObsSearchTest extends BaseModuleContextSensitiveTest { private PihAppsService service; diff --git a/omod/src/main/java/org/openmrs/module/pihapps/rest/AuditRestSupport.java b/omod/src/main/java/org/openmrs/module/pihapps/rest/AuditRestSupport.java deleted file mode 100644 index 742a207..0000000 --- a/omod/src/main/java/org/openmrs/module/pihapps/rest/AuditRestSupport.java +++ /dev/null @@ -1,70 +0,0 @@ -package org.openmrs.module.pihapps.rest; - -import org.apache.commons.lang.StringUtils; -import org.openmrs.Provider; -import org.openmrs.module.webservices.rest.SimpleObject; -import org.openmrs.module.webservices.rest.web.ConversionUtil; -import org.openmrs.module.webservices.rest.web.RestUtil; -import org.openmrs.module.webservices.rest.web.response.InvalidSearchException; -import org.openmrs.util.OpenmrsUtil; -import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException; - -import java.util.Date; - -/** - * The parts the audit endpoints in this package have in common: reading a date bound off a request - * parameter, and wording a rejected search the same way. Kept together so the two endpoints cannot - * drift apart on what `endDate=2026-09-30` means. - */ -final class AuditRestSupport { - - /** - * The context a rejected search is reported under. `RestUtil.wrapErrorResponse` prints it ahead - * of the exception's own message, so it says what kind of failure this is and leaves the - * particulars to the message. - */ - static final String INVALID_SEARCH_REASON = "Invalid search parameters"; - - private AuditRestSupport() { - } - - /** - * Reads one end of a date range, in any of the formats the REST API accepts elsewhere. - * - *

A bare date names the whole of that day: `endDate=2026-09-30` means through the end of the - * 30th, not its first instant, since a range given in dates is asking about days. Give a time - * to bound the range to the second instead. - * - * @param value the parameter as it arrived, or null or blank for no bound - * @param isUpperBound whether a date without a time should be stretched to the end of the day - * @return the bound, or null if none was given - */ - static Date parseBound(String value, boolean isUpperBound) { - if (StringUtils.isBlank(value)) { - return null; - } - - String trimmed = value.trim(); - Date date = (Date) ConversionUtil.convert(trimmed, Date.class); - return isUpperBound && isDateOnly(trimmed) ? OpenmrsUtil.getLastMomentOfDay(date) : date; - } - - private static boolean isDateOnly(String value) { - return value.matches("\\d{4}-\\d{2}-\\d{2}"); - } - - static String dateFormatMessage() { - return "startDate and endDate must be ISO 8601, e.g. 2026-09-01 or 2026-09-01T13:45:00.000+0000"; - } - - /** - * Answers a parameter core's property editors could not resolve into the object it names. Kept - * here so that both endpoints word the same failure the same way, and so that a bad `createdBy` - * reads the same to a client whichever one it asked. - */ - static SimpleObject unresolvedParameterResponse(MethodArgumentTypeMismatchException e) { - String noun = Provider.class.equals(e.getRequiredType()) ? "provider" : "user"; - return RestUtil.wrapErrorResponse(new InvalidSearchException("No " + noun + " found with id: " + e.getValue()), - INVALID_SEARCH_REASON); - } -} diff --git a/omod/src/main/java/org/openmrs/module/pihapps/rest/EncounterAuditRestController.java b/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsEncounterRestController.java similarity index 69% rename from omod/src/main/java/org/openmrs/module/pihapps/rest/EncounterAuditRestController.java rename to omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsEncounterRestController.java index 99bf107..894265e 100644 --- a/omod/src/main/java/org/openmrs/module/pihapps/rest/EncounterAuditRestController.java +++ b/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsEncounterRestController.java @@ -9,7 +9,6 @@ import org.openmrs.api.EncounterService; import org.openmrs.api.context.Context; import org.openmrs.module.pihapps.PihAppsService; -import org.openmrs.module.pihapps.SortCriteria; import org.openmrs.module.pihapps.encounter.EncounterSearchCriteria; import org.openmrs.module.pihapps.encounter.EncounterSearchResult; import org.openmrs.module.webservices.rest.SimpleObject; @@ -33,7 +32,6 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; -import java.util.ArrayList; import java.util.Date; import java.util.List; @@ -43,28 +41,42 @@ * providers field but no search handler exposes it, and it has no creator, changedBy or voidedBy * field at all, so those columns can be read off an encounter but not searched on. * - *

Results are paged and ordered by the most recent of the audit actions the search named, and - * each encounter is rendered in the standard encounter representation, so a client can ask for - * whatever it needs with `v`: + *

Results are paged and ordered as `sortBy` asks, and each encounter is rendered in the standard + * encounter representation, so a client can ask for whatever it needs with `v`: * * 

  * GET /openmrs/ws/rest/v1/pihapps/encounter?createdBy=<uuid>&limit=20&totalCount=true
- * GET /openmrs/ws/rest/v1/pihapps/encounter?changedBy=<uuid>&startDate=2026-09-01&endDate=2026-09-30
+ * GET /openmrs/ws/rest/v1/pihapps/encounter?changedBy=<uuid>&auditOnOrAfter=2026-09-01&auditOnOrBefore=2026-09-30
  * GET /openmrs/ws/rest/v1/pihapps/encounter?provider=<uuid>&v=custom:(uuid,encounterDatetime,auditInfo)
  * GET /openmrs/ws/rest/v1/pihapps/encounter?provider=<uuid>&encounterType=<uuid>
+ * GET /openmrs/ws/rest/v1/pihapps/encounter?voidedBy=<uuid>&includeVoided=true
+ * GET /openmrs/ws/rest/v1/pihapps/encounter?provider=<uuid>&sortBy=encounterDatetime-desc&sortBy=encounterId-desc
  *
* *

`createdBy`, `changedBy`, `voidedBy` and `provider` are bound by core's property editors, so * each takes a uuid or a primary key. `encounterType` takes a uuid or its name. * - *

Every filter narrows, so naming several asks for the encounters satisfying all of them. At - * least one user or provider is required; `encounterType` narrows an audit but is not an audit of - * anything on its own. `startDate` and `endDate` bound when the audit action - * happened rather than the encounter's own datetime, which is what core's encounter search already - * covers; they run inclusively, and a bare date names the whole of that day. + *

`includeVoided` decides whether voided encounters come back alongside the surviving ones, and + * is off unless asked for. An audit normally wants them on: a `voidedBy` search returns nothing + * without them, and an auditor looking at what a user entered wants to see what has since been + * deleted just as much as what survives. Callers tell the two apart by each encounter's voided + * flag. + * + *

`sortBy` takes `field-direction`, or just `field` for ascending, and may be given several + * times to order by more than one. Nothing is sorted unless asked, and a page without an ordering + * is not deterministic, so a client that pages should name one ending in something unique such as + * `encounterId`. An audit wants the action it searched on first — `createdBy` with + * `sortBy=dateCreated-desc`, a provider search with `sortBy=encounterDatetime-desc` — since + * ordering by anything else would bury an encounter backdated to last year but entered this + * morning. + * + *

Every filter narrows, so naming several asks for the encounters satisfying all of them, and + * naming none matches every encounter. `auditOnOrAfter` and `auditOnOrBefore` bound whichever + * column the search is about: the named audit action, or the encounter's own datetime where only a + * provider was named. They run inclusively, and a bare date names the whole of that day. */ @Controller -public class EncounterAuditRestController { +public class PihAppsEncounterRestController { protected Log log = LogFactory.getLog(getClass()); @@ -97,8 +109,11 @@ public Object searchEncounters(HttpServletRequest request, HttpServletResponse r @RequestParam(value = "voidedBy", required = false) User voidedBy, @RequestParam(value = "provider", required = false) Provider provider, @RequestParam(value = "encounterType", required = false) String encounterType, - @RequestParam(value = "startDate", required = false) String startDate, - @RequestParam(value = "endDate", required = false) String endDate) + @RequestParam(value = "auditOnOrAfter", required = false) String auditOnOrAfter, + @RequestParam(value = "auditOnOrBefore", required = false) String auditOnOrBefore, + @RequestParam(value = "includeVoided", required = false, + defaultValue = "false") boolean includeVoided, + @RequestParam(value = "sortBy", required = false) List sortBy) throws ResponseException { if (!Context.hasPrivilege(REQUIRED_PRIVILEGE)) { @@ -106,11 +121,6 @@ public Object searchEncounters(HttpServletRequest request, HttpServletResponse r } try { - if (createdBy == null && changedBy == null && voidedBy == null && provider == null) { - throw new InvalidSearchException( - "Please specify at least one of createdBy, changedBy, voidedBy or provider."); - } - EncounterType type = null; if (StringUtils.isNotBlank(encounterType)) { type = getEncounterType(encounterType); @@ -122,15 +132,15 @@ public Object searchEncounters(HttpServletRequest request, HttpServletResponse r Date fromDate; Date toDate; try { - fromDate = AuditRestSupport.parseBound(startDate, false); - toDate = AuditRestSupport.parseBound(endDate, true); + fromDate = PihAppsRestSupport.parseBound(auditOnOrAfter, false); + toDate = PihAppsRestSupport.parseBound(auditOnOrBefore, true); } catch (Exception e) { - throw new InvalidSearchException(AuditRestSupport.dateFormatMessage(), e); + throw new InvalidSearchException(PihAppsRestSupport.dateFormatMessage("auditOnOrAfter", "auditOnOrBefore"), e); } if (fromDate != null && toDate != null && fromDate.after(toDate)) { - throw new InvalidSearchException("startDate must not be after endDate."); + throw new InvalidSearchException("auditOnOrAfter must not be after auditOnOrBefore."); } RequestContext context = RestUtil.getRequestContext(request, response, @@ -142,13 +152,10 @@ public Object searchEncounters(HttpServletRequest request, HttpServletResponse r searchCriteria.setVoidedBy(voidedBy); searchCriteria.setProvider(provider); searchCriteria.setEncounterType(type); - // A voidedBy search would return nothing with the voided rows filtered out, and an - // auditor looking at what a user entered wants to see what has since been deleted just - // as much as what survives, so an audit always asks for them. - searchCriteria.setIncludeVoided(true); + searchCriteria.setIncludeVoided(includeVoided); searchCriteria.setAuditOnOrAfter(fromDate); searchCriteria.setAuditOnOrBefore(toDate); - searchCriteria.setSortCriteria(auditSortCriteria(createdBy, changedBy, voidedBy)); + searchCriteria.setSortCriteria(PihAppsRestSupport.parseSortCriteria(sortBy)); searchCriteria.setStartIndex(context.getStartIndex()); searchCriteria.setLimit(context.getLimit()); @@ -160,7 +167,7 @@ public Object searchEncounters(HttpServletRequest request, HttpServletResponse r } catch (InvalidSearchException e) { response.setStatus(HttpServletResponse.SC_BAD_REQUEST); - return RestUtil.wrapErrorResponse(e, AuditRestSupport.INVALID_SEARCH_REASON); + return RestUtil.wrapErrorResponse(e, PihAppsRestSupport.INVALID_SEARCH_REASON); } catch (Exception e) { log.error("Failed to search encounters by audit user", e); @@ -169,35 +176,6 @@ public Object searchEncounters(HttpServletRequest request, HttpServletResponse r } } - /** - * Orders by the most recent of the audit actions the search named, so that "most recent first" - * means the most recent thing the search is about, and matches whichever column the date bounds - * were applied to. A provider search names no action, so it orders by the encounter's own - * datetime. The encounter id breaks ties so that paging cannot repeat or skip a row when - * several share a timestamp. - */ - private List auditSortCriteria(User createdBy, User changedBy, User voidedBy) { - List sortCriteria = new ArrayList<>(); - sortCriteria.add(new SortCriteria(auditSortProperty(createdBy, changedBy, voidedBy), - SortCriteria.Direction.DESC)); - sortCriteria.add(new SortCriteria("encounterId", SortCriteria.Direction.DESC)); - return sortCriteria; - } - - /** The named audit action, taking the most recent kind of action when a search named several. */ - private String auditSortProperty(User createdBy, User changedBy, User voidedBy) { - if (voidedBy != null) { - return "dateVoided"; - } - if (changedBy != null) { - return "dateChanged"; - } - if (createdBy != null) { - return "dateCreated"; - } - return "encounterDatetime"; - } - /** * A parameter core's editors could not resolve is still a bad request, so it is answered in the * same shape as every other one this endpoint rejects rather than in Spring's own error format. @@ -206,7 +184,7 @@ private String auditSortProperty(User createdBy, User changedBy, User voidedBy) @ResponseStatus(HttpStatus.BAD_REQUEST) @ResponseBody public SimpleObject handleUnresolvedParameter(MethodArgumentTypeMismatchException e) { - return AuditRestSupport.unresolvedParameterResponse(e); + return PihAppsRestSupport.unresolvedParameterResponse(e); } /** diff --git a/omod/src/main/java/org/openmrs/module/pihapps/rest/ObsAuditRestController.java b/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsObsRestController.java similarity index 72% rename from omod/src/main/java/org/openmrs/module/pihapps/rest/ObsAuditRestController.java rename to omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsObsRestController.java index cd169a2..a46eff6 100644 --- a/omod/src/main/java/org/openmrs/module/pihapps/rest/ObsAuditRestController.java +++ b/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsObsRestController.java @@ -5,7 +5,6 @@ import org.openmrs.User; import org.openmrs.api.context.Context; import org.openmrs.module.pihapps.PihAppsService; -import org.openmrs.module.pihapps.SortCriteria; import org.openmrs.module.pihapps.obs.ObsSearchCriteria; import org.openmrs.module.pihapps.obs.ObsSearchResult; import org.openmrs.module.webservices.rest.SimpleObject; @@ -29,7 +28,6 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpServletResponse; -import java.util.ArrayList; import java.util.Date; import java.util.List; @@ -39,23 +37,39 @@ * {@link org.openmrs.parameter.ObsSearchCriteria} has a creator or voidedBy field, so those columns * can be read off an observation but not searched on. * - *

Results are paged and ordered most recent action first, and each observation is rendered in the + *

Results are paged and ordered as `sortBy` asks, and each observation is rendered in the * standard obs representation, so a client can ask for whatever it needs with `v`: * * 

  * GET /openmrs/ws/rest/v1/pihapps/obs?createdBy=<uuid>&limit=20&totalCount=true
  * GET /openmrs/ws/rest/v1/pihapps/obs?voidedBy=cd8a4b8e-...&v=custom:(uuid,concept:(display),auditInfo)
  * GET /openmrs/ws/rest/v1/pihapps/obs?createdBy=<uuid>&startDate=2026-09-01&endDate=2026-09-30
+ * GET /openmrs/ws/rest/v1/pihapps/obs?voidedBy=<uuid>&includeVoided=true
+ * GET /openmrs/ws/rest/v1/pihapps/obs?createdBy=<uuid>&sortBy=dateCreated-desc&sortBy=obsId-desc
  *
* *

`createdBy` and `voidedBy` are bound by core's property editors, so each takes a uuid or a * primary key. * - *

`startDate` and `endDate` bound when the audit action happened rather than the observation's - * own datetime, and run inclusively. + *

`includeVoided` decides whether voided observations come back alongside the surviving ones, + * and is off unless asked for. An audit normally wants them on: a `voidedBy` search returns nothing + * without them, and an auditor looking at what a user created wants to see what has since been + * deleted just as much as what survives. Callers tell the two apart by each observation's voided + * flag. + * + *

`sortBy` takes `field-direction`, or just `field` for ascending, and may be given several + * times to order by more than one. Nothing is sorted unless asked, and a page without an ordering + * is not deterministic, so a client that pages should name one ending in something unique such as + * `obsId`. An audit wants the action it searched on first — `createdBy` with + * `sortBy=dateCreated-desc`, `voidedBy` with `sortBy=dateVoided-desc` — since ordering by the + * observation's own datetime would bury an obs backdated to last year but entered this morning. + * + *

Every filter narrows, and naming none matches every observation. `startDate` and `endDate` + * bound when the audit action happened rather than the observation's own datetime, and run + * inclusively. */ @Controller -public class ObsAuditRestController { +public class PihAppsObsRestController { protected Log log = LogFactory.getLog(getClass()); @@ -82,7 +96,10 @@ public Object searchObs(HttpServletRequest request, HttpServletResponse response @RequestParam(value = "createdBy", required = false) User createdBy, @RequestParam(value = "voidedBy", required = false) User voidedBy, @RequestParam(value = "startDate", required = false) String startDate, - @RequestParam(value = "endDate", required = false) String endDate) + @RequestParam(value = "endDate", required = false) String endDate, + @RequestParam(value = "includeVoided", required = false, + defaultValue = "false") boolean includeVoided, + @RequestParam(value = "sortBy", required = false) List sortBy) throws ResponseException { if (!Context.hasPrivilege(REQUIRED_PRIVILEGE)) { @@ -90,18 +107,14 @@ public Object searchObs(HttpServletRequest request, HttpServletResponse response } try { - if (createdBy == null && voidedBy == null) { - throw new InvalidSearchException("Please specify createdBy, voidedBy, or both."); - } - Date fromDate; Date toDate; try { - fromDate = AuditRestSupport.parseBound(startDate, false); - toDate = AuditRestSupport.parseBound(endDate, true); + fromDate = PihAppsRestSupport.parseBound(startDate, false); + toDate = PihAppsRestSupport.parseBound(endDate, true); } catch (Exception e) { - throw new InvalidSearchException(AuditRestSupport.dateFormatMessage(), e); + throw new InvalidSearchException(PihAppsRestSupport.dateFormatMessage("startDate", "endDate"), e); } if (fromDate != null && toDate != null && fromDate.after(toDate)) { @@ -116,11 +129,8 @@ public Object searchObs(HttpServletRequest request, HttpServletResponse response searchCriteria.setVoidedBy(voidedBy); searchCriteria.setAuditOnOrAfter(fromDate); searchCriteria.setAuditOnOrBefore(toDate); - // A voidedBy search would return nothing with the voided rows filtered out, and an - // auditor looking at what a user created wants to see what has since been deleted just - // as much as what survives, so an audit always asks for them. - searchCriteria.setIncludeVoided(true); - searchCriteria.setSortCriteria(auditSortCriteria(voidedBy)); + searchCriteria.setIncludeVoided(includeVoided); + searchCriteria.setSortCriteria(PihAppsRestSupport.parseSortCriteria(sortBy)); searchCriteria.setStartIndex(context.getStartIndex()); searchCriteria.setLimit(context.getLimit()); @@ -134,7 +144,7 @@ public Object searchObs(HttpServletRequest request, HttpServletResponse response } catch (InvalidSearchException e) { response.setStatus(HttpServletResponse.SC_BAD_REQUEST); - return RestUtil.wrapErrorResponse(e, AuditRestSupport.INVALID_SEARCH_REASON); + return RestUtil.wrapErrorResponse(e, PihAppsRestSupport.INVALID_SEARCH_REASON); } catch (Exception e) { log.error("Failed to search observations by audit user", e); @@ -143,21 +153,6 @@ public Object searchObs(HttpServletRequest request, HttpServletResponse response } } - /** - * "Most recent first" means the most recent audit action: when the row was created for a - * createdBy search, when it was voided for a voidedBy search. Ordering by the observation's own - * datetime would bury an obs backdated to last year but entered this morning, which is the - * opposite of what an audit needs. The obs id breaks ties so that paging cannot repeat or skip - * a row when several share a timestamp. - */ - private List auditSortCriteria(User voidedBy) { - List sortCriteria = new ArrayList<>(); - String actionDate = voidedBy != null ? "dateVoided" : "dateCreated"; - sortCriteria.add(new SortCriteria(actionDate, SortCriteria.Direction.DESC)); - sortCriteria.add(new SortCriteria("obsId", SortCriteria.Direction.DESC)); - return sortCriteria; - } - /** * A parameter core's editors could not resolve is still a bad request, so it is answered in the * same shape as every other one this endpoint rejects rather than in Spring's own error format. @@ -166,6 +161,6 @@ private List auditSortCriteria(User voidedBy) { @ResponseStatus(HttpStatus.BAD_REQUEST) @ResponseBody public SimpleObject handleUnresolvedParameter(MethodArgumentTypeMismatchException e) { - return AuditRestSupport.unresolvedParameterResponse(e); + return PihAppsRestSupport.unresolvedParameterResponse(e); } } diff --git a/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsRestSupport.java b/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsRestSupport.java new file mode 100644 index 0000000..cfbc8db --- /dev/null +++ b/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsRestSupport.java @@ -0,0 +1,115 @@ +package org.openmrs.module.pihapps.rest; + +import org.apache.commons.lang.StringUtils; +import org.openmrs.Provider; +import org.openmrs.module.pihapps.SortCriteria; +import org.openmrs.module.webservices.rest.SimpleObject; +import org.openmrs.module.webservices.rest.web.ConversionUtil; +import org.openmrs.module.webservices.rest.web.RestUtil; +import org.openmrs.module.webservices.rest.web.response.InvalidSearchException; +import org.openmrs.util.OpenmrsUtil; +import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException; + +import java.util.ArrayList; +import java.util.Date; +import java.util.List; + +/** + * The parts the search endpoints in this package have in common: reading a date bound off a request + * parameter, and wording a rejected search the same way. Kept together so that they cannot drift + * apart on what a date-only upper bound means, or on how a bad parameter reads to a client. + */ +final class PihAppsRestSupport { + + /** + * The context a rejected search is reported under. `RestUtil.wrapErrorResponse` prints it ahead + * of the exception's own message, so it says what kind of failure this is and leaves the + * particulars to the message. + */ + static final String INVALID_SEARCH_REASON = "Invalid search parameters"; + + private PihAppsRestSupport() { + } + + /** + * Reads one end of a date range, in any of the formats the REST API accepts elsewhere. + * + *

A bare date names the whole of that day: an upper bound of `2026-09-30` means through the end of the + * 30th, not its first instant, since a range given in dates is asking about days. Give a time + * to bound the range to the second instead. + * + * @param value the parameter as it arrived, or null or blank for no bound + * @param isUpperBound whether a date without a time should be stretched to the end of the day + * @return the bound, or null if none was given + */ + static Date parseBound(String value, boolean isUpperBound) { + if (StringUtils.isBlank(value)) { + return null; + } + + String trimmed = value.trim(); + Date date = (Date) ConversionUtil.convert(trimmed, Date.class); + return isUpperBound && isDateOnly(trimmed) ? OpenmrsUtil.getLastMomentOfDay(date) : date; + } + + private static boolean isDateOnly(String value) { + return value.matches("\\d{4}-\\d{2}-\\d{2}"); + } + + /** + * @param fromParam what the endpoint calls its lower bound + * @param toParam what the endpoint calls its upper bound + */ + static String dateFormatMessage(String fromParam, String toParam) { + return fromParam + " and " + toParam + " must be ISO 8601, e.g. 2026-09-01 or 2026-09-01T13:45:00.000+0000"; + } + + /** + * Reads the `sortBy` parameter, in the `field-direction` form this module's other endpoints + * accept: `encounterDatetime-desc`, or just `encounterDatetime` for ascending. Several may be + * given, and they order the results outermost first. + * + *

Ordering is the caller's to choose, and a page without one is not deterministic — two + * requests for the same page can repeat or skip a row — so a client that pages should always + * name one, ending in something unique such as the primary key. + * + * @param sortBy the parameter as it arrived, or null or empty for no ordering + * @return the criteria in the order given, empty if none were named + * @throws InvalidSearchException if a direction is neither asc nor desc + */ + static List parseSortCriteria(List sortBy) { + List sortCriteria = new ArrayList<>(); + if (sortBy == null) { + return sortCriteria; + } + for (String value : sortBy) { + if (StringUtils.isBlank(value)) { + continue; + } + String[] components = value.trim().split("-", 2); + SortCriteria.Direction direction = SortCriteria.Direction.ASC; + if (components.length > 1) { + try { + direction = SortCriteria.Direction.valueOf(components[1].toUpperCase()); + } + catch (IllegalArgumentException e) { + throw new InvalidSearchException( + "sortBy direction must be asc or desc, e.g. encounterDatetime-desc: " + value, e); + } + } + sortCriteria.add(new SortCriteria(components[0], direction)); + } + return sortCriteria; + } + + /** + * Answers a parameter core's property editors could not resolve into the object it names. Kept + * here so that both endpoints word the same failure the same way, and so that a bad `createdBy` + * reads the same to a client whichever one it asked. + */ + static SimpleObject unresolvedParameterResponse(MethodArgumentTypeMismatchException e) { + String noun = Provider.class.equals(e.getRequiredType()) ? "provider" : "user"; + return RestUtil.wrapErrorResponse(new InvalidSearchException("No " + noun + " found with id: " + e.getValue()), + INVALID_SEARCH_REASON); + } +} From 0ca14da08c29a7a30f8e54d6eefc336cc921ca10 Mon Sep 17 00:00:00 2001 From: Cosmin Date: Thu, 24 Sep 2026 10:59:29 -0400 Subject: [PATCH 4/6] UHM-9514: address PR review comments --- .../module/pihapps/PihAppsServiceImpl.java | 15 ++++---- .../pihapps/PihAppsEncounterSearchTest.java | 35 +++++++++++++++++++ .../rest/PihAppsEncounterRestController.java | 20 ++++------- .../rest/PihAppsObsRestController.java | 19 ++++------ 4 files changed, 55 insertions(+), 34 deletions(-) diff --git a/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java b/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java index 78906c4..eca19ef 100644 --- a/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java +++ b/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java @@ -732,13 +732,14 @@ private Criteria createHibernateEncounterSearchCriteria(EncounterSearchCriteria .add(eq("ep.provider", searchCriteria.getProvider())) .add(eq("ep.voided", false)); c.add(Subqueries.propertyIn("encounterId", encountersNamingProvider)); - // A provider search names no audit action, so on its own it takes the range against the - // encounter's own datetime. Alongside a user filter the range has already been applied - // to that user's action, which is the more specific thing to ask about. - if (searchCriteria.getCreatedBy() == null && searchCriteria.getChangedBy() == null - && searchCriteria.getVoidedBy() == null) { - addEncounterAuditDateBounds(c, "encounterDatetime", searchCriteria); - } + } + // Each user filter above has already bounded the column belonging to its own action, which + // is the more specific thing to ask about. A search naming no action has nothing bounded + // yet, so the range falls back to the encounter's own datetime — otherwise a provider, + // encounterType or unfiltered search would quietly ignore the range it was given. + if (searchCriteria.getCreatedBy() == null && searchCriteria.getChangedBy() == null + && searchCriteria.getVoidedBy() == null) { + addEncounterAuditDateBounds(c, "encounterDatetime", searchCriteria); } if (applySortCriteria && searchCriteria.getSortCriteria() != null) { for (SortCriteria sortCriteria : searchCriteria.getSortCriteria()) { diff --git a/api/src/test/java/org/openmrs/module/pihapps/PihAppsEncounterSearchTest.java b/api/src/test/java/org/openmrs/module/pihapps/PihAppsEncounterSearchTest.java index 22f3d1b..737a75f 100644 --- a/api/src/test/java/org/openmrs/module/pihapps/PihAppsEncounterSearchTest.java +++ b/api/src/test/java/org/openmrs/module/pihapps/PihAppsEncounterSearchTest.java @@ -120,6 +120,13 @@ private List auditedIds(List encounters) { return encounterIds(encounters).stream().filter(id -> id >= 3000).collect(Collectors.toList()); } + private Date day(int month, int dayOfMonth, int hourOfDay, int year) { + Calendar calendar = Calendar.getInstance(); + calendar.clear(); + calendar.set(year, month, dayOfMonth, hourOfDay, 0, 0); + return calendar.getTime(); + } + private Date day(int month, int dayOfMonth, int hourOfDay) { Calendar calendar = Calendar.getInstance(); calendar.clear(); @@ -220,6 +227,34 @@ public void shouldBoundAProviderSearchByTheEncounterDatetime() { assertThat(search(null, null, null, provider, null, september(1), null), is(Collections.emptyList())); } + /** + * A search naming no audit action has no action column to bound, so the range falls back to the + * encounter's own datetime. Only a provider search used to reach that fallback, which meant a + * type-only or unfiltered search silently ignored the range it was given. + */ + @Test + public void shouldBoundASearchThatNamesNoAuditActionByTheEncounterDatetime() { + // every encounter in the fixture happened in 2026, so a 2099 range must exclude them all + Date from = day(Calendar.JANUARY, 1, 0, 2099); + Date to = day(Calendar.JANUARY, 2, 0, 2099); + + assertThat(auditedIds(search(null, null, null, null, typeOne, from, to)), is(Collections.emptyList())); + assertThat(auditedIds(search(null, null, null, null, null, from, to)), is(Collections.emptyList())); + assertThat(auditedIds(search(null, null, null, provider, null, from, to)), is(Collections.emptyList())); + } + + /** + * Each user filter bounds its own action's column, so a range alongside one must not also be + * applied to the encounter datetime — that would demand the encounter itself fall in the window + * as well, which is stricter than what was asked. + */ + @Test + public void shouldNotAlsoBoundTheEncounterDatetimeWhenAnActionIsNamed() { + // 3001 was entered on 1 Sep but happened on 1 Aug, so a September range finds it by creation + assertThat(auditedIds(search(bruno, null, null, null, null, day(Calendar.SEPTEMBER, 1, 0), + day(Calendar.SEPTEMBER, 1, 23))), contains(3001)); + } + @Test public void shouldTreatBothEndsOfTheRangeAsInclusive() { assertThat(auditedIds(search(bruno, null, null, null, null, day(Calendar.SEPTEMBER, 1, 8), diff --git a/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsEncounterRestController.java b/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsEncounterRestController.java index 894265e..768ba1e 100644 --- a/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsEncounterRestController.java +++ b/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsEncounterRestController.java @@ -14,7 +14,6 @@ import org.openmrs.module.webservices.rest.SimpleObject; import org.openmrs.module.webservices.rest.web.RequestContext; import org.openmrs.module.webservices.rest.web.RestUtil; -import org.openmrs.module.webservices.rest.web.representation.CustomRepresentation; import org.openmrs.module.webservices.rest.web.resource.impl.AlreadyPaged; import org.openmrs.module.webservices.rest.web.response.InvalidSearchException; import org.openmrs.module.webservices.rest.web.response.ResponseException; @@ -41,11 +40,14 @@ * providers field but no search handler exposes it, and it has no creator, changedBy or voidedBy * field at all, so those columns can be read off an encounter but not searched on. * - *

Results are paged and ordered as `sortBy` asks, and each encounter is rendered in the standard - * encounter representation, so a client can ask for whatever it needs with `v`: + *

Results are paged and ordered as `sortBy` asks, and each encounter is rendered by the standard + * encounter resource, so `v` behaves as it does anywhere else in the REST API and defaults to the + * same thing. An audit wants `auditInfo` — the creating, changing and voiding users with their + * timestamps — which the default representation leaves out, so it asks for it: * * 

  * GET /openmrs/ws/rest/v1/pihapps/encounter?createdBy=<uuid>&limit=20&totalCount=true
+ * GET /openmrs/ws/rest/v1/pihapps/encounter?createdBy=<uuid>&v=custom:(uuid,display,auditInfo)
  * GET /openmrs/ws/rest/v1/pihapps/encounter?changedBy=<uuid>&auditOnOrAfter=2026-09-01&auditOnOrBefore=2026-09-30
  * GET /openmrs/ws/rest/v1/pihapps/encounter?provider=<uuid>&v=custom:(uuid,encounterDatetime,auditInfo)
  * GET /openmrs/ws/rest/v1/pihapps/encounter?provider=<uuid>&encounterType=<uuid>
@@ -86,15 +88,6 @@ public class PihAppsEncounterRestController {
      */
     private static final String REQUIRED_PRIVILEGE = "App: coreapps.systemAdministration";
 
-    /**
-     * Enough to say what the encounter was and who touched it. `auditInfo` is what makes this an
-     * audit result: it carries the creating, changing and voiding users with their timestamps.
-     */
-    private static final String DEFAULT_REPRESENTATION =
-            "(uuid,display,encounterDatetime,voided,encounterType:(uuid,display),form:(uuid,display)," +
-                    "location:(uuid,display),patient:(uuid,display)," +
-                    "encounterProviders:(uuid,voided,provider:(uuid,display),encounterRole:(uuid,display)),auditInfo)";
-
     @Autowired
     private EncounterService encounterService;
 
@@ -143,8 +136,7 @@ public Object searchEncounters(HttpServletRequest request, HttpServletResponse r
                 throw new InvalidSearchException("auditOnOrAfter must not be after auditOnOrBefore.");
             }
 
-            RequestContext context = RestUtil.getRequestContext(request, response,
-                    new CustomRepresentation(DEFAULT_REPRESENTATION));
+            RequestContext context = RestUtil.getRequestContext(request, response);
 
             EncounterSearchCriteria searchCriteria = new EncounterSearchCriteria();
             searchCriteria.setCreatedBy(createdBy);
diff --git a/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsObsRestController.java b/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsObsRestController.java
index a46eff6..14b7377 100644
--- a/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsObsRestController.java
+++ b/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsObsRestController.java
@@ -10,7 +10,6 @@
 import org.openmrs.module.webservices.rest.SimpleObject;
 import org.openmrs.module.webservices.rest.web.RequestContext;
 import org.openmrs.module.webservices.rest.web.RestUtil;
-import org.openmrs.module.webservices.rest.web.representation.CustomRepresentation;
 import org.openmrs.module.webservices.rest.web.resource.impl.AlreadyPaged;
 import org.openmrs.module.webservices.rest.web.response.InvalidSearchException;
 import org.openmrs.module.webservices.rest.web.response.ResponseException;
@@ -37,11 +36,14 @@
  * {@link org.openmrs.parameter.ObsSearchCriteria} has a creator or voidedBy field, so those columns
  * can be read off an observation but not searched on.
  *
- * 
Results are paged and ordered as `sortBy` asks, and each observation is rendered in the
- * standard obs representation, so a client can ask for whatever it needs with `v`:
+ * 
Results are paged and ordered as `sortBy` asks, and each observation is rendered by the
+ * standard obs resource, so `v` behaves as it does anywhere else in the REST API and defaults to
+ * the same thing. An audit wants `auditInfo` — the creator and the voiding user with their
+ * timestamps — which the default representation leaves out, so it asks for it:
  *
  * 
  * GET /openmrs/ws/rest/v1/pihapps/obs?createdBy=<uuid>&limit=20&totalCount=true
+ * GET /openmrs/ws/rest/v1/pihapps/obs?createdBy=<uuid>&v=custom:(uuid,display,auditInfo)
  * GET /openmrs/ws/rest/v1/pihapps/obs?voidedBy=cd8a4b8e-...&v=custom:(uuid,concept:(display),auditInfo)
  * GET /openmrs/ws/rest/v1/pihapps/obs?createdBy=<uuid>&startDate=2026-09-01&endDate=2026-09-30
  * GET /openmrs/ws/rest/v1/pihapps/obs?voidedBy=<uuid>&includeVoided=true
@@ -79,14 +81,6 @@ public class PihAppsObsRestController {
      */
     private static final String REQUIRED_PRIVILEGE = "App: coreapps.systemAdministration";
 
-    /**
-     * Enough to say what was recorded and who touched it. `auditInfo` is what makes this an audit
-     * result: it carries the creator and the voiding user with their timestamps.
-     */
-    private static final String DEFAULT_REPRESENTATION =
-            "(uuid,display,obsDatetime,voided,concept:(uuid,display),person:(uuid,display)," +
-                    "encounter:(uuid),value:ref,comment,auditInfo)";
-
     @Autowired
     private PihAppsService pihAppsService;
 
@@ -121,8 +115,7 @@ public Object searchObs(HttpServletRequest request, HttpServletResponse response
                 throw new InvalidSearchException("startDate must not be after endDate.");
             }
 
-            RequestContext context = RestUtil.getRequestContext(request, response,
-                    new CustomRepresentation(DEFAULT_REPRESENTATION));
+            RequestContext context = RestUtil.getRequestContext(request, response);
 
             ObsSearchCriteria searchCriteria = new ObsSearchCriteria();
             searchCriteria.setCreatedBy(createdBy);

From 2dff26b886a1b2cb6a20139746bfcabff77dba08 Mon Sep 17 00:00:00 2001
From: Cosmin 
Date: Thu, 24 Sep 2026 15:03:46 -0400
Subject: [PATCH 5/6] UHM-9514: remove sysAdmin privilege requirement

---
 .../module/pihapps/PihAppsServiceImpl.java    | 40 +++++------
 .../openmrs/module/pihapps/PihAppsUtils.java  | 35 ++++++++++
 .../encounter/EncounterSearchCriteria.java    | 42 ++++++++---
 .../pihapps/PihAppsEncounterSearchTest.java   | 43 +++++++++++-
 .../module/pihapps/PihAppsUtilsTest.java      | 35 ++++++++++
 .../rest/PihAppsEncounterRestController.java  | 69 ++++++++++---------
 .../rest/PihAppsObsRestController.java        | 36 ++--------
 .../pihapps/rest/PihAppsRestSupport.java      | 33 +++------
 8 files changed, 209 insertions(+), 124 deletions(-)

diff --git a/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java b/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java
index eca19ef..7df2891 100644
--- a/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java
+++ b/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java
@@ -585,12 +585,7 @@ public void revertOrdersToOrdered(List orders) {
 	}
 
 	private void addAuditDateBounds(Criteria c, String property, ObsSearchCriteria searchCriteria) {
-		if (searchCriteria.getAuditOnOrAfter() != null) {
-			c.add(ge(property, searchCriteria.getAuditOnOrAfter()));
-		}
-		if (searchCriteria.getAuditOnOrBefore() != null) {
-			c.add(le(property, searchCriteria.getAuditOnOrBefore()));
-		}
+		addDateBounds(c, property, searchCriteria.getAuditOnOrAfter(), searchCriteria.getAuditOnOrBefore());
 	}
 
 	@Override
@@ -688,15 +683,17 @@ public EncounterSearchResult getEncounters(EncounterSearchCriteria searchCriteri
 	}
 
 	/**
-	 * Unlike the obsDatetime bounds on an obs search, these are applied exactly as given rather than
-	 * widened to whole days: the caller has already said which moment it means.
+	 * Both ends run inclusively. The upper end is widened to the end of its day when it carries no
+	 * time, so that a range named in days covers the whole of the last one.
 	 */
-	private void addEncounterAuditDateBounds(Criteria c, String property, EncounterSearchCriteria searchCriteria) {
-		if (searchCriteria.getAuditOnOrAfter() != null) {
-			c.add(ge(property, searchCriteria.getAuditOnOrAfter()));
+	private void addDateBounds(Criteria c, String property, Date onOrAfter, Date onOrBefore) {
+		if (onOrAfter != null) {
+			// No adjustment: midnight is already the first moment of its day.
+			c.add(ge(property, onOrAfter));
 		}
-		if (searchCriteria.getAuditOnOrBefore() != null) {
-			c.add(le(property, searchCriteria.getAuditOnOrBefore()));
+		Date upperBound = PihAppsUtils.getEndOfDayIfTimeExcluded(onOrBefore);
+		if (upperBound != null) {
+			c.add(le(property, upperBound));
 		}
 	}
 
@@ -712,16 +709,19 @@ private Criteria createHibernateEncounterSearchCriteria(EncounterSearchCriteria
 		}
 		if (searchCriteria.getCreatedBy() != null) {
 			c.add(eq("creator", searchCriteria.getCreatedBy()));
-			addEncounterAuditDateBounds(c, "dateCreated", searchCriteria);
 		}
 		if (searchCriteria.getChangedBy() != null) {
 			c.add(eq("changedBy", searchCriteria.getChangedBy()));
-			addEncounterAuditDateBounds(c, "dateChanged", searchCriteria);
 		}
 		if (searchCriteria.getVoidedBy() != null) {
 			c.add(eq("voidedBy", searchCriteria.getVoidedBy()));
-			addEncounterAuditDateBounds(c, "dateVoided", searchCriteria);
 		}
+		// Each range names the column it bounds, so none of this depends on which filters are set.
+		addDateBounds(c, "dateCreated", searchCriteria.getCreatedOnOrAfter(), searchCriteria.getCreatedOnOrBefore());
+		addDateBounds(c, "dateChanged", searchCriteria.getChangedOnOrAfter(), searchCriteria.getChangedOnOrBefore());
+		addDateBounds(c, "dateVoided", searchCriteria.getVoidedOnOrAfter(), searchCriteria.getVoidedOnOrBefore());
+		addDateBounds(c, "encounterDatetime", searchCriteria.getEncounterDatetimeOnOrAfter(),
+			searchCriteria.getEncounterDatetimeOnOrBefore());
 		if (searchCriteria.getProvider() != null) {
 			// A subquery rather than a join, so that an encounter naming the provider more than
 			// once is still returned once — a join would need a distinct, and an in-memory distinct
@@ -733,14 +733,6 @@ private Criteria createHibernateEncounterSearchCriteria(EncounterSearchCriteria
 					.add(eq("ep.voided", false));
 			c.add(Subqueries.propertyIn("encounterId", encountersNamingProvider));
 		}
-		// Each user filter above has already bounded the column belonging to its own action, which
-		// is the more specific thing to ask about. A search naming no action has nothing bounded
-		// yet, so the range falls back to the encounter's own datetime — otherwise a provider,
-		// encounterType or unfiltered search would quietly ignore the range it was given.
-		if (searchCriteria.getCreatedBy() == null && searchCriteria.getChangedBy() == null
-				&& searchCriteria.getVoidedBy() == null) {
-			addEncounterAuditDateBounds(c, "encounterDatetime", searchCriteria);
-		}
 		if (applySortCriteria && searchCriteria.getSortCriteria() != null) {
 			for (SortCriteria sortCriteria : searchCriteria.getSortCriteria()) {
 				if (sortCriteria.getDirection() == SortCriteria.Direction.DESC) {
diff --git a/api/src/main/java/org/openmrs/module/pihapps/PihAppsUtils.java b/api/src/main/java/org/openmrs/module/pihapps/PihAppsUtils.java
index a9a9285..0e5967d 100644
--- a/api/src/main/java/org/openmrs/module/pihapps/PihAppsUtils.java
+++ b/api/src/main/java/org/openmrs/module/pihapps/PihAppsUtils.java
@@ -9,6 +9,8 @@
 import org.springframework.stereotype.Component;
 
 import java.util.ArrayDeque;
+import java.util.Calendar;
+import java.util.Date;
 import java.util.Deque;
 import java.util.HashSet;
 import java.util.Locale;
@@ -101,6 +103,39 @@ else if (isShort && isEnglish) {
      * @param root
      * @return a set of Concepts that are recursive set members of root
      */
+    /**
+     * The last moment of a date's day if that date carries no time of day, and the date itself
+     * otherwise. Modelled on the reporting module's {@code DateUtil.getEndOfDayIfTimeExcluded}.
+     *
+     * 
This is for the upper end of an inclusive range. Someone who names a day means the whole
+     * of it, so a bound of `2026-09-30` has to reach 23:59:59.999 or everything recorded after
+     * midnight on the 30th falls outside a range that plainly includes the 30th. A lower bound
+     * needs no such adjustment: midnight is already the first moment of its day.
+     *
+     * 
A time of exactly midnight is read as no time of day, since a Date cannot say whether the
+     * caller wrote `2026-09-30` or `2026-09-30T00:00:00`. A caller that means that first instant
+     * and nothing more should bound the range a moment earlier.
+     *
+     * @param date the upper bound as given, or null for no bound
+     * @return the bound to search on, or null if none was given
+     */
+    public static Date getEndOfDayIfTimeExcluded(Date date) {
+        if (date == null) {
+            return null;
+        }
+        Calendar calendar = Calendar.getInstance();
+        calendar.setTime(date);
+        if (calendar.get(Calendar.HOUR_OF_DAY) != 0 || calendar.get(Calendar.MINUTE) != 0
+                || calendar.get(Calendar.SECOND) != 0 || calendar.get(Calendar.MILLISECOND) != 0) {
+            return date;
+        }
+        calendar.set(Calendar.HOUR_OF_DAY, 23);
+        calendar.set(Calendar.MINUTE, 59);
+        calendar.set(Calendar.SECOND, 59);
+        calendar.set(Calendar.MILLISECOND, 999);
+        return calendar.getTime();
+    }
+
     public static Set getConceptHierarchy(Concept root) {
         Set result = new HashSet<>();
         Set visited = new HashSet<>();
diff --git a/api/src/main/java/org/openmrs/module/pihapps/encounter/EncounterSearchCriteria.java b/api/src/main/java/org/openmrs/module/pihapps/encounter/EncounterSearchCriteria.java
index abed4b4..ef9ed15 100644
--- a/api/src/main/java/org/openmrs/module/pihapps/encounter/EncounterSearchCriteria.java
+++ b/api/src/main/java/org/openmrs/module/pihapps/encounter/EncounterSearchCriteria.java
@@ -46,20 +46,40 @@ public class EncounterSearchCriteria {
     private EncounterType encounterType;
 
     /**
-     * Bound whatever the search is about. Where an audit action is named these bound that action's
-     * column — an encounter backdated to last year but entered this morning was entered this
-     * morning, and core's own encounter search already covers encounterDatetime for the cases it
-     * can reach. A provider search names no action, so there they bound the encounter's own
-     * datetime, which is both what a provider's caseload is asked about and something core cannot
-     * filter by provider.
+     * Bound when the encounter was created. Each of the four ranges below names the column it
+     * applies to, and each is independent of the filters: `createdOnOrAfter` narrows by creation
+     * date whether or not `createdBy` is also given, and naming several asks for all of them.
      *
-     * 
Both ends run inclusively and are applied as given, so a caller that means a whole day
-     * passes that day's last moment.
+     * 
Which range a search wants is the caller's to decide. An audit of what a user entered
+     * wants the range against that user's action — an encounter backdated to last year but entered
+     * this morning was entered this morning — while a provider's caseload is asked about by
+     * {@link #encounterDatetimeOnOrAfter}, when the encounters actually happened.
+     *
+     * 
Both ends of every range run inclusively and are applied as given, so a caller that means
+     * a whole day passes that day's last moment.
      */
-    private Date auditOnOrAfter;
+    private Date createdOnOrAfter;
+
+    /** @see #createdOnOrAfter */
+    private Date createdOnOrBefore;
+
+    /** Bound when the encounter was last changed. @see #createdOnOrAfter */
+    private Date changedOnOrAfter;
+
+    /** @see #createdOnOrAfter */
+    private Date changedOnOrBefore;
+
+    /** Bound when the encounter was voided. @see #createdOnOrAfter */
+    private Date voidedOnOrAfter;
+
+    /** @see #createdOnOrAfter */
+    private Date voidedOnOrBefore;
+
+    /** Bound the encounter's own datetime — when it happened. @see #createdOnOrAfter */
+    private Date encounterDatetimeOnOrAfter;
 
-    /** @see #auditOnOrAfter */
-    private Date auditOnOrBefore;
+    /** @see #createdOnOrAfter */
+    private Date encounterDatetimeOnOrBefore;
 
     private List sortCriteria;
     private Integer startIndex;
diff --git a/api/src/test/java/org/openmrs/module/pihapps/PihAppsEncounterSearchTest.java b/api/src/test/java/org/openmrs/module/pihapps/PihAppsEncounterSearchTest.java
index 737a75f..1a4097a 100644
--- a/api/src/test/java/org/openmrs/module/pihapps/PihAppsEncounterSearchTest.java
+++ b/api/src/test/java/org/openmrs/module/pihapps/PihAppsEncounterSearchTest.java
@@ -67,8 +67,24 @@ private EncounterSearchResult searchResult(User createdBy, User changedBy, User
         searchCriteria.setVoidedBy(voidedBy);
         searchCriteria.setProvider(byProvider);
         searchCriteria.setEncounterType(encounterType);
-        searchCriteria.setAuditOnOrAfter(fromDate);
-        searchCriteria.setAuditOnOrBefore(toDate);
+        // The criteria name the column each range bounds, so the test picks the one the case is
+        // about — the same choice a client makes. Each case names at most one action.
+        if (voidedBy != null) {
+            searchCriteria.setVoidedOnOrAfter(fromDate);
+            searchCriteria.setVoidedOnOrBefore(toDate);
+        }
+        else if (changedBy != null) {
+            searchCriteria.setChangedOnOrAfter(fromDate);
+            searchCriteria.setChangedOnOrBefore(toDate);
+        }
+        else if (createdBy != null) {
+            searchCriteria.setCreatedOnOrAfter(fromDate);
+            searchCriteria.setCreatedOnOrBefore(toDate);
+        }
+        else {
+            searchCriteria.setEncounterDatetimeOnOrAfter(fromDate);
+            searchCriteria.setEncounterDatetimeOnOrBefore(toDate);
+        }
         searchCriteria.setIncludeVoided(true);
         searchCriteria.setSortCriteria(auditSortCriteria(createdBy, changedBy, voidedBy));
         searchCriteria.setStartIndex(startIndex);
@@ -255,6 +271,29 @@ public void shouldNotAlsoBoundTheEncounterDatetimeWhenAnActionIsNamed() {
             day(Calendar.SEPTEMBER, 1, 23))), contains(3001));
     }
 
+    /**
+     * Each range names its own column, so they combine rather than one displacing another and none
+     * of them depends on a matching user filter being given.
+     */
+    @Test
+    public void shouldApplyEachRangeToItsOwnColumnIndependently() {
+        EncounterSearchCriteria byCreationAndHappening = new EncounterSearchCriteria();
+        byCreationAndHappening.setIncludeVoided(true);
+        // 3001 happened on 1 Aug and was entered on 1 Sep; 3002 happened on 1 Jul, entered 25 Aug
+        byCreationAndHappening.setCreatedOnOrAfter(day(Calendar.SEPTEMBER, 1, 0));
+        byCreationAndHappening.setEncounterDatetimeOnOrBefore(day(Calendar.AUGUST, 15, 0));
+
+        assertThat(auditedIds(service.getEncounters(byCreationAndHappening).getEncounters()), contains(3001));
+
+        // a creation range with no createdBy filter still narrows, which the old single range could
+        // not express
+        EncounterSearchCriteria creationOnly = new EncounterSearchCriteria();
+        creationOnly.setIncludeVoided(true);
+        creationOnly.setCreatedOnOrAfter(day(Calendar.JANUARY, 1, 0, 2099));
+
+        assertThat(auditedIds(service.getEncounters(creationOnly).getEncounters()), is(Collections.emptyList()));
+    }
+
     @Test
     public void shouldTreatBothEndsOfTheRangeAsInclusive() {
         assertThat(auditedIds(search(bruno, null, null, null, null, day(Calendar.SEPTEMBER, 1, 8),
diff --git a/api/src/test/java/org/openmrs/module/pihapps/PihAppsUtilsTest.java b/api/src/test/java/org/openmrs/module/pihapps/PihAppsUtilsTest.java
index 948ddf1..1919a58 100644
--- a/api/src/test/java/org/openmrs/module/pihapps/PihAppsUtilsTest.java
+++ b/api/src/test/java/org/openmrs/module/pihapps/PihAppsUtilsTest.java
@@ -4,12 +4,16 @@
 import org.openmrs.Concept;
 
 import java.util.Arrays;
+import java.util.Calendar;
 import java.util.Collections;
+import java.util.Date;
 import java.util.HashSet;
 import java.util.Set;
 import java.util.stream.Collectors;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertNull;
+import static org.junit.jupiter.api.Assertions.assertSame;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -88,6 +92,37 @@ public void getConceptHierarchy_shouldHandleCycles_withoutInfiniteLoop() {
         assertEquals(new HashSet<>(Arrays.asList("A", "B")), result);
     }
 
+    private static Date moment(int hour, int minute, int second, int millisecond) {
+        Calendar calendar = Calendar.getInstance();
+        calendar.clear();
+        calendar.set(2026, Calendar.SEPTEMBER, 30, hour, minute, second);
+        calendar.set(Calendar.MILLISECOND, millisecond);
+        return calendar.getTime();
+    }
+
+    @Test
+    public void getEndOfDayIfTimeExcluded_shouldWidenADateWithNoTimeToTheEndOfItsDay() {
+        assertEquals(moment(23, 59, 59, 999), PihAppsUtils.getEndOfDayIfTimeExcluded(moment(0, 0, 0, 0)));
+    }
+
+    @Test
+    public void getEndOfDayIfTimeExcluded_shouldLeaveADateCarryingATimeAlone() {
+        Date withTime = moment(13, 45, 0, 0);
+        assertSame(withTime, PihAppsUtils.getEndOfDayIfTimeExcluded(withTime));
+    }
+
+    /** A single millisecond past midnight is a time of day, so the bound stands as given. */
+    @Test
+    public void getEndOfDayIfTimeExcluded_shouldLeaveAMomentJustPastMidnightAlone() {
+        Date justPast = moment(0, 0, 0, 1);
+        assertSame(justPast, PihAppsUtils.getEndOfDayIfTimeExcluded(justPast));
+    }
+
+    @Test
+    public void getEndOfDayIfTimeExcluded_shouldReturnNullForNoBound() {
+        assertNull(PihAppsUtils.getEndOfDayIfTimeExcluded(null));
+    }
+
     private static Concept conceptWithId(int id, String uuid) {
         Concept c = mock(Concept.class);
         when(c.getConceptId()).thenReturn(id);
diff --git a/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsEncounterRestController.java b/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsEncounterRestController.java
index 768ba1e..3362aff 100644
--- a/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsEncounterRestController.java
+++ b/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsEncounterRestController.java
@@ -7,7 +7,6 @@
 import org.openmrs.Provider;
 import org.openmrs.User;
 import org.openmrs.api.EncounterService;
-import org.openmrs.api.context.Context;
 import org.openmrs.module.pihapps.PihAppsService;
 import org.openmrs.module.pihapps.encounter.EncounterSearchCriteria;
 import org.openmrs.module.pihapps.encounter.EncounterSearchResult;
@@ -19,7 +18,6 @@
 import org.openmrs.module.webservices.rest.web.response.ResponseException;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.http.HttpStatus;
-import org.springframework.http.ResponseEntity;
 import org.springframework.stereotype.Controller;
 import org.springframework.web.bind.annotation.ExceptionHandler;
 import org.springframework.web.bind.annotation.RequestMapping;
@@ -31,7 +29,6 @@
 
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
-import java.util.Date;
 import java.util.List;
 
 /**
@@ -48,7 +45,8 @@
  * 
  * GET /openmrs/ws/rest/v1/pihapps/encounter?createdBy=<uuid>&limit=20&totalCount=true
  * GET /openmrs/ws/rest/v1/pihapps/encounter?createdBy=<uuid>&v=custom:(uuid,display,auditInfo)
- * GET /openmrs/ws/rest/v1/pihapps/encounter?changedBy=<uuid>&auditOnOrAfter=2026-09-01&auditOnOrBefore=2026-09-30
+ * GET /openmrs/ws/rest/v1/pihapps/encounter?changedBy=<uuid>&changedOnOrAfter=2026-09-01&changedOnOrBefore=2026-09-30
+ * GET /openmrs/ws/rest/v1/pihapps/encounter?provider=<uuid>&encounterDatetimeOnOrAfter=2026-04-01
  * GET /openmrs/ws/rest/v1/pihapps/encounter?provider=<uuid>&v=custom:(uuid,encounterDatetime,auditInfo)
  * GET /openmrs/ws/rest/v1/pihapps/encounter?provider=<uuid>&encounterType=<uuid>
  * GET /openmrs/ws/rest/v1/pihapps/encounter?voidedBy=<uuid>&includeVoided=true
@@ -73,21 +71,22 @@
  * morning.
  *
  * 
Every filter narrows, so naming several asks for the encounters satisfying all of them, and
- * naming none matches every encounter. `auditOnOrAfter` and `auditOnOrBefore` bound whichever
- * column the search is about: the named audit action, or the encounter's own datetime where only a
- * provider was named. They run inclusively, and a bare date names the whole of that day.
+ * naming none matches every encounter.
+ *
+ * 
There are four date ranges, each naming the column it bounds: `createdOnOrAfter`/`Before`,
+ * `changedOnOrAfter`/`Before`, `voidedOnOrAfter`/`Before` and
+ * `encounterDatetimeOnOrAfter`/`Before`. Each is independent of the filters, so `createdOnOrAfter`
+ * narrows by creation date whether or not `createdBy` is given, and naming several asks for all of
+ * them. Which one a search wants is the caller's to decide: an audit of what a user entered wants
+ * the range against that user's action, since an encounter backdated to last year but entered this
+ * morning was entered this morning, while a provider's caseload is asked about by
+ * `encounterDatetime`. All ends run inclusively, and a bare date names the whole of that day.
  */
 @Controller
 public class PihAppsEncounterRestController {
 
     protected Log log = LogFactory.getLog(getClass());
 
-    /**
-     * The same gate as this package's other administrative endpoints. An encounter audit reaches
-     * across every patient's record, so it is not something a clinical role should be able to run.
-     */
-    private static final String REQUIRED_PRIVILEGE = "App: coreapps.systemAdministration";
-
     @Autowired
     private EncounterService encounterService;
 
@@ -102,17 +101,21 @@ public Object searchEncounters(HttpServletRequest request, HttpServletResponse r
                                    @RequestParam(value = "voidedBy", required = false) User voidedBy,
                                    @RequestParam(value = "provider", required = false) Provider provider,
                                    @RequestParam(value = "encounterType", required = false) String encounterType,
-                                   @RequestParam(value = "auditOnOrAfter", required = false) String auditOnOrAfter,
-                                   @RequestParam(value = "auditOnOrBefore", required = false) String auditOnOrBefore,
+                                   @RequestParam(value = "createdOnOrAfter", required = false) String createdOnOrAfter,
+                                   @RequestParam(value = "createdOnOrBefore", required = false) String createdOnOrBefore,
+                                   @RequestParam(value = "changedOnOrAfter", required = false) String changedOnOrAfter,
+                                   @RequestParam(value = "changedOnOrBefore", required = false) String changedOnOrBefore,
+                                   @RequestParam(value = "voidedOnOrAfter", required = false) String voidedOnOrAfter,
+                                   @RequestParam(value = "voidedOnOrBefore", required = false) String voidedOnOrBefore,
+                                   @RequestParam(value = "encounterDatetimeOnOrAfter",
+                                           required = false) String encounterDatetimeOnOrAfter,
+                                   @RequestParam(value = "encounterDatetimeOnOrBefore",
+                                           required = false) String encounterDatetimeOnOrBefore,
                                    @RequestParam(value = "includeVoided", required = false,
                                            defaultValue = "false") boolean includeVoided,
                                    @RequestParam(value = "sortBy", required = false) List sortBy)
             throws ResponseException {
 
-        if (!Context.hasPrivilege(REQUIRED_PRIVILEGE)) {
-            return new ResponseEntity<>(HttpStatus.UNAUTHORIZED);
-        }
-
         try {
             EncounterType type = null;
             if (StringUtils.isNotBlank(encounterType)) {
@@ -122,19 +125,6 @@ public Object searchEncounters(HttpServletRequest request, HttpServletResponse r
                 }
             }
 
-            Date fromDate;
-            Date toDate;
-            try {
-                fromDate = PihAppsRestSupport.parseBound(auditOnOrAfter, false);
-                toDate = PihAppsRestSupport.parseBound(auditOnOrBefore, true);
-            }
-            catch (Exception e) {
-                throw new InvalidSearchException(PihAppsRestSupport.dateFormatMessage("auditOnOrAfter", "auditOnOrBefore"), e);
-            }
-
-            if (fromDate != null && toDate != null && fromDate.after(toDate)) {
-                throw new InvalidSearchException("auditOnOrAfter must not be after auditOnOrBefore.");
-            }
 
             RequestContext context = RestUtil.getRequestContext(request, response);
 
@@ -145,8 +135,19 @@ public Object searchEncounters(HttpServletRequest request, HttpServletResponse r
             searchCriteria.setProvider(provider);
             searchCriteria.setEncounterType(type);
             searchCriteria.setIncludeVoided(includeVoided);
-            searchCriteria.setAuditOnOrAfter(fromDate);
-            searchCriteria.setAuditOnOrBefore(toDate);
+            try {
+                searchCriteria.setCreatedOnOrAfter(PihAppsRestSupport.parseDate(createdOnOrAfter));
+                searchCriteria.setCreatedOnOrBefore(PihAppsRestSupport.parseDate(createdOnOrBefore));
+                searchCriteria.setChangedOnOrAfter(PihAppsRestSupport.parseDate(changedOnOrAfter));
+                searchCriteria.setChangedOnOrBefore(PihAppsRestSupport.parseDate(changedOnOrBefore));
+                searchCriteria.setVoidedOnOrAfter(PihAppsRestSupport.parseDate(voidedOnOrAfter));
+                searchCriteria.setVoidedOnOrBefore(PihAppsRestSupport.parseDate(voidedOnOrBefore));
+                searchCriteria.setEncounterDatetimeOnOrAfter(PihAppsRestSupport.parseDate(encounterDatetimeOnOrAfter));
+                searchCriteria.setEncounterDatetimeOnOrBefore(PihAppsRestSupport.parseDate(encounterDatetimeOnOrBefore));
+            }
+            catch (Exception e) {
+                throw new InvalidSearchException(PihAppsRestSupport.dateFormatMessage(), e);
+            }
             searchCriteria.setSortCriteria(PihAppsRestSupport.parseSortCriteria(sortBy));
             searchCriteria.setStartIndex(context.getStartIndex());
             searchCriteria.setLimit(context.getLimit());
diff --git a/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsObsRestController.java b/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsObsRestController.java
index 14b7377..5fb60c2 100644
--- a/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsObsRestController.java
+++ b/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsObsRestController.java
@@ -3,7 +3,6 @@
 import org.apache.commons.logging.Log;
 import org.apache.commons.logging.LogFactory;
 import org.openmrs.User;
-import org.openmrs.api.context.Context;
 import org.openmrs.module.pihapps.PihAppsService;
 import org.openmrs.module.pihapps.obs.ObsSearchCriteria;
 import org.openmrs.module.pihapps.obs.ObsSearchResult;
@@ -15,7 +14,6 @@
 import org.openmrs.module.webservices.rest.web.response.ResponseException;
 import org.springframework.beans.factory.annotation.Autowired;
 import org.springframework.http.HttpStatus;
-import org.springframework.http.ResponseEntity;
 import org.springframework.stereotype.Controller;
 import org.springframework.web.bind.annotation.ExceptionHandler;
 import org.springframework.web.bind.annotation.RequestMapping;
@@ -27,7 +25,6 @@
 
 import javax.servlet.http.HttpServletRequest;
 import javax.servlet.http.HttpServletResponse;
-import java.util.Date;
 import java.util.List;
 
 /**
@@ -75,12 +72,6 @@ public class PihAppsObsRestController {
 
     protected Log log = LogFactory.getLog(getClass());
 
-    /**
-     * The same gate as this package's other administrative endpoints. An observation audit reaches
-     * across every patient's record, so it is not something a clinical role should be able to run.
-     */
-    private static final String REQUIRED_PRIVILEGE = "App: coreapps.systemAdministration";
-
     @Autowired
     private PihAppsService pihAppsService;
 
@@ -96,32 +87,19 @@ public Object searchObs(HttpServletRequest request, HttpServletResponse response
                             @RequestParam(value = "sortBy", required = false) List sortBy)
             throws ResponseException {
 
-        if (!Context.hasPrivilege(REQUIRED_PRIVILEGE)) {
-            return new ResponseEntity<>(HttpStatus.UNAUTHORIZED);
-        }
-
         try {
-            Date fromDate;
-            Date toDate;
-            try {
-                fromDate = PihAppsRestSupport.parseBound(startDate, false);
-                toDate = PihAppsRestSupport.parseBound(endDate, true);
-            }
-            catch (Exception e) {
-                throw new InvalidSearchException(PihAppsRestSupport.dateFormatMessage("startDate", "endDate"), e);
-            }
-
-            if (fromDate != null && toDate != null && fromDate.after(toDate)) {
-                throw new InvalidSearchException("startDate must not be after endDate.");
-            }
-
             RequestContext context = RestUtil.getRequestContext(request, response);
 
             ObsSearchCriteria searchCriteria = new ObsSearchCriteria();
             searchCriteria.setCreatedBy(createdBy);
             searchCriteria.setVoidedBy(voidedBy);
-            searchCriteria.setAuditOnOrAfter(fromDate);
-            searchCriteria.setAuditOnOrBefore(toDate);
+            try {
+                searchCriteria.setAuditOnOrAfter(PihAppsRestSupport.parseDate(startDate));
+                searchCriteria.setAuditOnOrBefore(PihAppsRestSupport.parseDate(endDate));
+            }
+            catch (Exception e) {
+                throw new InvalidSearchException(PihAppsRestSupport.dateFormatMessage(), e);
+            }
             searchCriteria.setIncludeVoided(includeVoided);
             searchCriteria.setSortCriteria(PihAppsRestSupport.parseSortCriteria(sortBy));
             searchCriteria.setStartIndex(context.getStartIndex());
diff --git a/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsRestSupport.java b/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsRestSupport.java
index cfbc8db..b9fb728 100644
--- a/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsRestSupport.java
+++ b/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsRestSupport.java
@@ -7,7 +7,6 @@
 import org.openmrs.module.webservices.rest.web.ConversionUtil;
 import org.openmrs.module.webservices.rest.web.RestUtil;
 import org.openmrs.module.webservices.rest.web.response.InvalidSearchException;
-import org.openmrs.util.OpenmrsUtil;
 import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException;
 
 import java.util.ArrayList;
@@ -32,36 +31,22 @@ private PihAppsRestSupport() {
     }
 
     /**
-     * Reads one end of a date range, in any of the formats the REST API accepts elsewhere.
+     * Reads a date parameter with no bound semantics, for an endpoint that decides what an end of a
+     * range means further down. Blank means the parameter was not given.
      *
-     * 
A bare date names the whole of that day: an upper bound of `2026-09-30` means through the end of the
-     * 30th, not its first instant, since a range given in dates is asking about days. Give a time
-     * to bound the range to the second instead.
-     *
-     * @param value the parameter as it arrived, or null or blank for no bound
-     * @param isUpperBound whether a date without a time should be stretched to the end of the day
-     * @return the bound, or null if none was given
+     * @param value the parameter as it arrived, or null or blank for no date
+     * @return the date, or null if none was given
      */
-    static Date parseBound(String value, boolean isUpperBound) {
+    static Date parseDate(String value) {
         if (StringUtils.isBlank(value)) {
             return null;
         }
-
-        String trimmed = value.trim();
-        Date date = (Date) ConversionUtil.convert(trimmed, Date.class);
-        return isUpperBound && isDateOnly(trimmed) ? OpenmrsUtil.getLastMomentOfDay(date) : date;
-    }
-
-    private static boolean isDateOnly(String value) {
-        return value.matches("\\d{4}-\\d{2}-\\d{2}");
+        return (Date) ConversionUtil.convert(value.trim(), Date.class);
     }
 
-    /**
-     * @param fromParam what the endpoint calls its lower bound
-     * @param toParam what the endpoint calls its upper bound
-     */
-    static String dateFormatMessage(String fromParam, String toParam) {
-        return fromParam + " and " + toParam + " must be ISO 8601, e.g. 2026-09-01 or 2026-09-01T13:45:00.000+0000";
+    /** The format message for an endpoint with more date parameters than are worth listing. */
+    static String dateFormatMessage() {
+        return "Date parameters must be ISO 8601, e.g. 2026-09-01 or 2026-09-01T13:45:00.000+0000";
     }
 
     /**

From ee7d23f588f314c6ffd008efa96b3869c61fe8a7 Mon Sep 17 00:00:00 2001
From: Cosmin 
Date: Fri, 25 Sep 2026 10:48:44 -0400
Subject: [PATCH 6/6] UHM-9514: obs date ranges are now independent of the
 filters

---
 .../module/pihapps/PihAppsServiceImpl.java    |  8 ++--
 .../module/pihapps/obs/ObsSearchCriteria.java | 29 ++++++++-----
 .../module/pihapps/PihAppsObsSearchTest.java  | 43 ++++++++++++++++++-
 .../rest/PihAppsObsRestController.java        | 26 +++++++----
 4 files changed, 81 insertions(+), 25 deletions(-)

diff --git a/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java b/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java
index 7df2891..c5e0bca 100644
--- a/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java
+++ b/api/src/main/java/org/openmrs/module/pihapps/PihAppsServiceImpl.java
@@ -584,9 +584,6 @@ public void revertOrdersToOrdered(List orders) {
 		}
 	}
 
-	private void addAuditDateBounds(Criteria c, String property, ObsSearchCriteria searchCriteria) {
-		addDateBounds(c, property, searchCriteria.getAuditOnOrAfter(), searchCriteria.getAuditOnOrBefore());
-	}
 
 	@Override
 	@Transactional(readOnly = true)
@@ -622,12 +619,13 @@ private Criteria createHibernateObsSearchCriteria(ObsSearchCriteria searchCriter
 		}
 		if (searchCriteria.getCreatedBy() != null) {
 			c.add(eq("creator", searchCriteria.getCreatedBy()));
-			addAuditDateBounds(c, "dateCreated", searchCriteria);
 		}
 		if (searchCriteria.getVoidedBy() != null) {
 			c.add(eq("voidedBy", searchCriteria.getVoidedBy()));
-			addAuditDateBounds(c, "dateVoided", searchCriteria);
 		}
+		// Each range names the column it bounds, so none of this depends on which filters are set.
+		addDateBounds(c, "dateCreated", searchCriteria.getCreatedOnOrAfter(), searchCriteria.getCreatedOnOrBefore());
+		addDateBounds(c, "dateVoided", searchCriteria.getVoidedOnOrAfter(), searchCriteria.getVoidedOnOrBefore());
 		if (searchCriteria.getPatient() != null) {
 			c.add(eq("person", searchCriteria.getPatient()));
 		}
diff --git a/api/src/main/java/org/openmrs/module/pihapps/obs/ObsSearchCriteria.java b/api/src/main/java/org/openmrs/module/pihapps/obs/ObsSearchCriteria.java
index 6c0e3fe..1826bc2 100644
--- a/api/src/main/java/org/openmrs/module/pihapps/obs/ObsSearchCriteria.java
+++ b/api/src/main/java/org/openmrs/module/pihapps/obs/ObsSearchCriteria.java
@@ -33,19 +33,28 @@ public class ObsSearchCriteria {
     private User voidedBy;
 
     /**
-     * Bound the audit action rather than the observation's own datetime, which {@link #onOrAfter}
-     * and {@link #onOrBefore} cover. An observation backdated to last year but entered this morning
-     * was modified this morning, which is what a search over a timeframe is asking about.
+     * Bound when the observation was created. Each of the ranges below names the column it applies
+     * to, and each is independent of the filters: `createdOnOrAfter` narrows by creation date
+     * whether or not {@link #createdBy} is also given, and naming several asks for all of them.
      *
-     * 
Each user filter is bounded by the column belonging to its action, so naming both users
-     * and a range asks for observations that one user created and the other voided, each within the
-     * window. Both ends run inclusively and are applied as given, so a caller that means a whole
-     * day passes that day's last moment.
+     * 
Which range a search wants is the caller's to decide. An audit bounds the action it is
+     * about rather than the observation's own datetime — an observation backdated to last year but
+     * entered this morning was entered this morning — while {@link #onOrAfter} and
+     * {@link #onOrBefore} bound obsDatetime, when the observation says it was taken.
+     *
+     * 
Both ends of every range run inclusively. An upper end carrying no time of day is read as
+     * the whole of that day.
      */
-    private Date auditOnOrAfter;
+    private Date createdOnOrAfter;
+
+    /** @see #createdOnOrAfter */
+    private Date createdOnOrBefore;
+
+    /** Bound when the observation was voided. @see #createdOnOrAfter */
+    private Date voidedOnOrAfter;
 
-    /** @see #auditOnOrAfter */
-    private Date auditOnOrBefore;
+    /** @see #createdOnOrAfter */
+    private Date voidedOnOrBefore;
 
     private List sortCriteria;
     private Integer startIndex;
diff --git a/api/src/test/java/org/openmrs/module/pihapps/PihAppsObsSearchTest.java b/api/src/test/java/org/openmrs/module/pihapps/PihAppsObsSearchTest.java
index 23ab71d..403115f 100644
--- a/api/src/test/java/org/openmrs/module/pihapps/PihAppsObsSearchTest.java
+++ b/api/src/test/java/org/openmrs/module/pihapps/PihAppsObsSearchTest.java
@@ -56,8 +56,16 @@ private ObsSearchResult search(User createdBy, User voidedBy, Date fromDate, Dat
         ObsSearchCriteria searchCriteria = new ObsSearchCriteria();
         searchCriteria.setCreatedBy(createdBy);
         searchCriteria.setVoidedBy(voidedBy);
-        searchCriteria.setAuditOnOrAfter(fromDate);
-        searchCriteria.setAuditOnOrBefore(toDate);
+        // The criteria name the column each range bounds, so the test picks the one the case is
+        // about — the same choice a client makes. Each case names at most one action.
+        if (voidedBy != null) {
+            searchCriteria.setVoidedOnOrAfter(fromDate);
+            searchCriteria.setVoidedOnOrBefore(toDate);
+        }
+        else {
+            searchCriteria.setCreatedOnOrAfter(fromDate);
+            searchCriteria.setCreatedOnOrBefore(toDate);
+        }
         searchCriteria.setIncludeVoided(true);
         searchCriteria.setSortCriteria(auditSortCriteria(voidedBy));
         searchCriteria.setStartIndex(startIndex);
@@ -88,6 +96,13 @@ private Long count(User createdBy, User voidedBy, Date fromDate, Date toDate) {
     }
 
     /** A moment on a September 2026 day, matching the fixture's audit dates. */
+    private Date august(int dayOfMonth, int hourOfDay) {
+        Calendar calendar = Calendar.getInstance();
+        calendar.clear();
+        calendar.set(2026, Calendar.AUGUST, dayOfMonth, hourOfDay, 0, 0);
+        return calendar.getTime();
+    }
+
     private Date september(int dayOfMonth, int hourOfDay) {
         Calendar calendar = Calendar.getInstance();
         calendar.clear();
@@ -217,6 +232,30 @@ public void shouldLeaveOutVoidedObsUnlessAskedFor() {
         assertThat(obsIds(service.getObs(searchCriteria).getObs()), containsInAnyOrder(2004, 2005));
     }
 
+    /**
+     * Each range names its own column, so they combine rather than one displacing another and none
+     * of them depends on a matching user filter being given.
+     */
+    @Test
+    public void shouldApplyEachRangeToItsOwnColumnIndependently() {
+        // a creation range with no createdBy filter still narrows, which the old single range could
+        // not express — it was silently ignored
+        ObsSearchCriteria creationOnly = new ObsSearchCriteria();
+        creationOnly.setIncludeVoided(true);
+        creationOnly.setCreatedOnOrAfter(september(20, 0));
+
+        assertThat(service.getObs(creationOnly).getObs(), is(java.util.Collections.emptyList()));
+
+        // 2004 was created 28 Aug and voided 4 Sep; 2005 was voided 5 Sep but created back on 20
+        // Aug, so only the creation range tells them apart — the two bound different columns
+        ObsSearchCriteria both = new ObsSearchCriteria();
+        both.setIncludeVoided(true);
+        both.setCreatedOnOrAfter(august(25, 0));
+        both.setVoidedOnOrAfter(september(1, 0));
+
+        assertThat(obsIds(service.getObs(both).getObs()), contains(2004));
+    }
+
     @Test
     public void shouldSearchWithoutAnyAuditFilter() {
         // The service places no audit-specific requirement on the criteria, so a search naming none
diff --git a/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsObsRestController.java b/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsObsRestController.java
index 5fb60c2..13c3a15 100644
--- a/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsObsRestController.java
+++ b/omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsObsRestController.java
@@ -42,7 +42,8 @@
  * GET /openmrs/ws/rest/v1/pihapps/obs?createdBy=<uuid>&limit=20&totalCount=true
  * GET /openmrs/ws/rest/v1/pihapps/obs?createdBy=<uuid>&v=custom:(uuid,display,auditInfo)
  * GET /openmrs/ws/rest/v1/pihapps/obs?voidedBy=cd8a4b8e-...&v=custom:(uuid,concept:(display),auditInfo)
- * GET /openmrs/ws/rest/v1/pihapps/obs?createdBy=<uuid>&startDate=2026-09-01&endDate=2026-09-30
+ * GET /openmrs/ws/rest/v1/pihapps/obs?createdBy=<uuid>&createdOnOrAfter=2026-09-01&createdOnOrBefore=2026-09-30
+ * GET /openmrs/ws/rest/v1/pihapps/obs?voidedBy=<uuid>&voidedOnOrAfter=2026-09-01
  * GET /openmrs/ws/rest/v1/pihapps/obs?voidedBy=<uuid>&includeVoided=true
  * GET /openmrs/ws/rest/v1/pihapps/obs?createdBy=<uuid>&sortBy=dateCreated-desc&sortBy=obsId-desc
  * 

@@ -63,9 +64,14 @@
  * `sortBy=dateCreated-desc`, `voidedBy` with `sortBy=dateVoided-desc` — since ordering by the
  * observation's own datetime would bury an obs backdated to last year but entered this morning.
  *
- * 
Every filter narrows, and naming none matches every observation. `startDate` and `endDate`
- * bound when the audit action happened rather than the observation's own datetime, and run
- * inclusively.
+ * 
Every filter narrows, and naming none matches every observation.
+ *
+ * 
There are two date ranges, each naming the column it bounds: `createdOnOrAfter`/`Before` and
+ * `voidedOnOrAfter`/`Before`. Each is independent of the filters, so `createdOnOrAfter` narrows by
+ * creation date whether or not `createdBy` is given, and naming both asks for both. They bound when
+ * the audit action happened rather than the observation's own datetime, since an observation
+ * backdated to last year but entered this morning was entered this morning. Both ends run
+ * inclusively, and an upper end with no time of day names the whole of that day.
  */
 @Controller
 public class PihAppsObsRestController {
@@ -80,8 +86,10 @@ public class PihAppsObsRestController {
     public Object searchObs(HttpServletRequest request, HttpServletResponse response,
                             @RequestParam(value = "createdBy", required = false) User createdBy,
                             @RequestParam(value = "voidedBy", required = false) User voidedBy,
-                            @RequestParam(value = "startDate", required = false) String startDate,
-                            @RequestParam(value = "endDate", required = false) String endDate,
+                            @RequestParam(value = "createdOnOrAfter", required = false) String createdOnOrAfter,
+                            @RequestParam(value = "createdOnOrBefore", required = false) String createdOnOrBefore,
+                            @RequestParam(value = "voidedOnOrAfter", required = false) String voidedOnOrAfter,
+                            @RequestParam(value = "voidedOnOrBefore", required = false) String voidedOnOrBefore,
                             @RequestParam(value = "includeVoided", required = false,
                                     defaultValue = "false") boolean includeVoided,
                             @RequestParam(value = "sortBy", required = false) List sortBy)
@@ -94,8 +102,10 @@ public Object searchObs(HttpServletRequest request, HttpServletResponse response
             searchCriteria.setCreatedBy(createdBy);
             searchCriteria.setVoidedBy(voidedBy);
             try {
-                searchCriteria.setAuditOnOrAfter(PihAppsRestSupport.parseDate(startDate));
-                searchCriteria.setAuditOnOrBefore(PihAppsRestSupport.parseDate(endDate));
+                searchCriteria.setCreatedOnOrAfter(PihAppsRestSupport.parseDate(createdOnOrAfter));
+                searchCriteria.setCreatedOnOrBefore(PihAppsRestSupport.parseDate(createdOnOrBefore));
+                searchCriteria.setVoidedOnOrAfter(PihAppsRestSupport.parseDate(voidedOnOrAfter));
+                searchCriteria.setVoidedOnOrBefore(PihAppsRestSupport.parseDate(voidedOnOrBefore));
             }
             catch (Exception e) {
                 throw new InvalidSearchException(PihAppsRestSupport.dateFormatMessage(), e);