UHM-9514: migrate audit REST end points from pihcore by cioan · Pull Request #54 · PIH/openmrs-module-pihapps

cioan · 2026-09-21T19:54:06Z

Thanks @mseaton for reviewing this PR initially when it was in pihcore. As you suggested, I have moved this code into pihapps, so that we could use it in Malawi and Rwanda. I also addressed the suggestions you made on that previous PR (PIH/openmrs-module-pihcore#694).

AuditRestSupport still has the date-only detection and the end-of-day stretch. No OpenMRS REST code does this, the stock handlers pass the parsed instant straight through. But date-range.ts sends bare YYYY-MM-DD on purpose and its doc comment says so explicitly: "those endpoints read a date-only endDate as the whole of that day, so there is no need to spell out a time." Going fully stock would clip endDate to midnight and silently drop everything audited during the final day of a range.

Please let me know if I missed anything else.

mseaton · 2026-09-21T20:11:31Z

+	 * @throws org.openmrs.api.APIException if neither user is given
+	 */
+	@Authorized(PrivilegeConstants.GET_OBS)
+	ObsSearchResult getObsByAuditUser(ObsSearchCriteria searchCriteria);


I see no need for this method. The existing getObs method is perfectly sufficient and this adds no real value.

@mseaton , getObsByAuditUser methods is just a thin wrapper — it adds exactly three things over getObs:

The audit ordering. [dateVoided|dateCreated DESC, obsId DESC], filled in when the caller hasn't asked for its own sort. This is the substantive part.

The "at least one of createdBy or voidedBy" guard. Already duplicated in ObsAuditRestController, which throws InvalidSearchException before it ever builds the criteria — so at the REST layer
this is dead weight. It only protects non-REST callers.

A different privilege. @Authorized(GET_OBS) vs getObs's @Authorized(GET_PATIENTS). Note today's effective check is GET_OBS only: the wrapper calls this.getObs(...), and Spring AOP doesn't
re-apply the interceptor on self-invocation, so getObs's annotation never fires for the audit path. Routing the controller straight at getObs would switch the check to GET_PATIENTS.

For the record, the current arrangement is sound: @Authorized(GET_OBS) on getObsByAuditUser is the check that actually runs for the audit path, and GET_OBS is the right privilege for an obs
search. The guard being in both the controller and the service is defence in depth rather than redundancy — the controller's version produces the 400 with a useful message, the service's protects
any future non-REST caller from running an unbounded scan of the obs table.

@mseaton , please let me know what you think about the arguments above, and if you still want this getObsByAuditUser method be removed and just call directly the pihApps.getObs?

Yes, responding to the comments above, I was thinking that we should just update the existing getObs to have the correct privilege, and move the audit ordering and keep the createdby/voidedby guard in the rest controller (or even in the consuming page itself as arguments to the endpoint).

mseaton · 2026-09-21T20:11:51Z

+	 * @throws org.openmrs.api.APIException if no user and no provider is given
+	 */
+	@Authorized(PrivilegeConstants.GET_ENCOUNTERS)
+	EncounterSearchResult getEncountersByAuditUser(EncounterSearchCriteria searchCriteria);


Same, this method should be more generic, e.g getEncounters

@mseaton , just to clarify, here you just want this method to be renamed to getEncounters?

Correct @cioan

mseaton · 2026-09-21T20:12:40Z

+	/**
+	 * 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.


Not sure why this comment is desired.

mseaton · 2026-09-21T20:21:51Z

+	 * action when a search named several, or the encounter's own datetime when only a provider was
+	 * named.
+	 */
+	private String encounterAuditSortProperty(EncounterSearchCriteria searchCriteria) {


We should move this to the REST endpoint. I don't think this is general purpose.

mseaton · 2026-09-21T20:24:16Z

+			// 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);


This doesn't seem right to me. I think we should have explicit properties in EncounterSearchCriteria for limiting encounters by datetime and have it generally applied.

@cioan I still don't understand what this is trying to do here. Why is this in the provider-filtering block? What does encounterDatetime have to do with the provider filter? And why would we be filtering on encounterDatetime related to the audit dates provided? Seems unrelated. If we want to filter by encounterDatetime, then the search criteria should allow that and a user should set it.

mseaton · 2026-09-21T20:25:24Z

+
+        String trimmed = value.trim();
+        Date date = (Date) ConversionUtil.convert(trimmed, Date.class);
+        return isUpperBound && isDateOnly(trimmed) ? OpenmrsUtil.getLastMomentOfDay(date) : date;


I feel like the REST module must already support this, no? And if not, it should?

mseaton · 2026-09-21T20:27:12Z

+    @Autowired
+    private PihAppsService pihAppsService;
+
+    @RequestMapping(value = "/rest/v1/pihapps/encounteraudit", method = RequestMethod.GET)


If possible, it would be nice if this could be a generic encounter endpoint, and just take in more explicit arguments to indicate what kind of filtering and sorting is expected. This isn't really adding that much that is audit-specific.

mseaton · 2026-09-21T20:28:59Z

Looks much better @cioan - thanks! A few follow-up comments, let me know what you think of the ideas.

cioan · 2026-09-22T20:06:46Z

Thanks @mseaton ! I think I have addressed all your comments above. Please let me know I missed anything. Thanks!

mseaton

Looking much better @cioan . A few more comments - again, happy to discuss and debate.

mseaton · 2026-09-22T21:11:13Z

+     * 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";


Not sure if this is the right privilege?

ZL requirement was that this type of audit functions should be available to users with System Administration privileges. Which privilege should I use then?

Well, the service is gated around View Encounters, right? We might want to gate the page around system administration, but the REST endpoint is independent of the page, especially if we make this a more generic EncounterRestController, not an audit-specific one

@mseaton , I have removed the privilege. But, here are the implications of this decision:

What now protects these endpoints. They aren't open — the service layer still gates them, and because the controllers call the Spring proxy (pihAppsService.getObs(...), not a self-invocation),
the AOP authorization advice does fire:

getObs → @Authorized(PrivilegeConstants.GET_OBS)

getEncounters → @Authorized(PrivilegeConstants.GET_ENCOUNTERS)

plus OpenMRS's usual requirement that the REST caller be authenticated. This is the conventional OpenMRS arrangement — core REST resources rely on service-layer @Authorized rather than checking privileges in the web layer — so it's consistent with how the rest of the API works.

But it is a real widening, and worth a deliberate look before it ships. Access moves from "holds the App: coreapps.systemAdministration app privilege" to "holds Get Observations / GetEncounters", which in a typical PIH role setup is essentially every clinical user. Combined with the other changes in this session, these endpoints now accept a request with no filters at all and page through every encounter or observation in the database, with auditInfo available on request. So a nurse-level account could enumerate the full record set rather than just the patients in front of them.

If that's the intent for a general-purpose endpoint, it's fine as it stands. If you'd rather keep a gate without the coreapps app coupling, the middle option is a dedicated privilege on the service methods — something like PihApps: Search All Encounters — which keeps the check in the conventional place while not granting it to everyone who can read a chart. Happy to wire that up if you want it.

I would just gate the endpoints with the same privileges that the service methods we added require @cioan , right?

@mseaton , that is already in place, please see above. Do you mean to explicitly add again @Authorized(PrivilegeConstants.GET_ENCOUNTERS) to the PihAppsEncounterRestController? That would be redundant?

Sorry, I'm fine with out it @cioan - I had read / misread what you put above as Claude recommending some kind of check here, so I was just saying to use what we know is needed. But I don't think we necessarily need it if the service is already gated.

cioan · 2026-09-23T19:17:29Z

Thanks @mseaton ! I have addressed your latest comments. I just had two questions regarding the privilege to use, and the time bound feature. Thanks!

cioan · 2026-09-24T19:10:27Z

@mseaton , please review the latest updates to this PR and let me know if I missed anything? Thanks!

mseaton · 2026-09-25T12:13:09Z

+		}
+		if (searchCriteria.getCreatedBy() != null) {
+			c.add(eq("creator", searchCriteria.getCreatedBy()));
+			addAuditDateBounds(c, "dateCreated", searchCriteria);


Just noticed this here @cioan - why do we only apply dateCreated filter if createdBy is not null? Shouldn't these be independent?

mseaton · 2026-09-25T12:17:17Z

+		}
+		if (searchCriteria.getVoidedBy() != null) {
+			c.add(eq("voidedBy", searchCriteria.getVoidedBy()));
+			addAuditDateBounds(c, "dateVoided", searchCriteria);


Same here @cioan

mseaton

3 quick comments I made today @cioan and then I think you should feel free to merge once these are resolved.

cioan · 2026-09-25T14:50:45Z

Thanks @mseaton ! I have addressed those 3 comments as well. I am testing now the frontend audit app to make sure that it still works as before. I will merge this in, unless you see something else that I missed? Thanks!

UHM-9514: migrate audit REST end points from pihcore

0771f01

cioan requested a review from mseaton September 21, 2026 19:54

mseaton requested changes Sep 21, 2026

View reviewed changes

UHM-9514: address PR review feedback

7a75ddb

cioan requested a review from mseaton September 22, 2026 20:53

mseaton reviewed Sep 22, 2026

View reviewed changes

UHM-9514: generalize encounter and obs rest endpoints

2341ad0

cioan requested a review from mseaton September 23, 2026 19:17

mseaton reviewed Sep 23, 2026

View reviewed changes

Comment thread omod/src/main/java/org/openmrs/module/pihapps/rest/PihAppsEncounterRestController.java Outdated

mseaton reviewed Sep 23, 2026

View reviewed changes

UHM-9514: address PR review comments

0ca14da

cioan requested a review from mseaton September 24, 2026 15:04

UHM-9514: remove sysAdmin privilege requirement

2dff26b

mseaton reviewed Sep 25, 2026

View reviewed changes

UHM-9514: obs date ranges are now independent of the filters

ee7d23f

cioan merged commit b183124 into main Sep 25, 2026

cioan deleted the UHM-9514 branch September 25, 2026 15:00

Conversation

cioan commented Sep 21, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mseaton commented Sep 21, 2026

Uh oh!

cioan commented Sep 22, 2026

Uh oh!

mseaton left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cioan commented Sep 23, 2026

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

cioan commented Sep 24, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mseaton left a comment

Choose a reason for hiding this comment

Uh oh!

cioan commented Sep 25, 2026

Uh oh!

Reviewers