Fix logo and address not showing in Sticker by jnsereko · Pull Request #3 · openmrs/openmrs-module-patientdocuments

jnsereko · 2025-10-14T12:36:40Z

Description

This PR addresses two issues:

Logo not displaying
- The logo could not be pulled from other modules using relative paths with Docker (e.g., moduleResources/legacyui/images/openmrs_logo_white_large.png or moduleResources/legacyui/openmrs_logo_white_large.png
- Solution: Added ability to supply a custom logo resolved under OPENMRS_APPLICATION_DATA_DIRECTORY (e.g., /openmrs/data/my_custom_logo.png) and as a fall back, an OpenMRS logo pulled from the servlet context ie /images/openmrs_logo_white_large.png
Address not showing
- The Address object from the patient object was being accessed with the wrong property name
- Solution: Corrected the property name to properly retrieve patient address information

Screenshot

Related issue

https://openmrs.atlassian.net/browse/O3-5123

jnsereko · 2025-10-15T11:13:24Z

Hey @dkayiwa and @ibacher could you kindly look at this as we add this module to the distro here

dkayiwa · 2025-10-15T12:27:51Z

Don't we already have the openmrs logo in core?

api/src/main/resources/msfStickerFopStylesheet.xsl

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

jnsereko · 2025-10-28T11:36:36Z

hello @ibacher and @dkayiwa I have improved logic to;

get the OpenMRS logo from the servlet context and cache it in the OPENMRS_APPLICATION_DATA_DIRECTORY as a fallback in case custom logo has not been found.
get the custom log from OPENMRS_APPLICATION_DATA_DIRECTORY and its relative path eg /openmrs/data/my_custom_logo.png

jnsereko · 2025-10-29T06:43:05Z

Hey @dkayiwa and @ibacher
Could you kindly re-review this?

dkayiwa · 2025-10-29T12:10:57Z

...mrs/module/patientdocuments/web/rest/controller/PatientIdStickerDataPdfExportController.java

 		}
 	}
+
+	private void loadAndCacheDefaultLogo(ServletContext servletContext) {


Why are you caching it?

@dkayiwa since we can only get the default logo using the servlet context, yet we need to use it from the API, I thought it’s better saving it for better performance.

What was the performance difference?

@dkayiwa , the logo needs to be accessed for every patient document generated. In a busy hospital setting with potentially hundreds of patients per day, repeatedly reading from the servlet context for each document would mean hundreds of I/O operations daily for the same 8KB file. Caching it once to the application data directory eliminates this repeated overhead. The servlet context resource stream needs to be opened and closed on each access, whereas with caching, we read once and reuse. I've also added a check to ensure we only cache if the file doesn't already exist.

Are you caching it in memory such that you do not do any further I/O on disk?

@dkayiwa, good point. I've removed the caching mechanism entirely as it was adding unnecessary disk I/O and complexity without clear performance benefits.
The logo is now loaded directly from the servlet context and embedded as a base64 data URI in the XML output.

dkayiwa · 2025-10-30T12:42:07Z

readme/PatientIdStickerXSL.md


 The optional header section can contain:
- An organizational logo on the left (loaded from HTTP URL)
+- An organizational logo on the left (from a file path under `OPENMRS_APPLICATION_DATA_DIRECTORY`)


Is this correct?

Yes it is. We get the custom logo like new File(OpenmrsUtil.getDirectoryInApplicationDataDirectory(""), logoUrlPath)

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

dkayiwa · 2025-11-01T20:25:39Z

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

+			// 2. Fall back to default logo
+			if (isBlank(logoPath) && defaultLogoBytes != null && defaultLogoBytes.length > 0) {
+				String base64Image = Base64.getEncoder().encodeToString(defaultLogoBytes);
+				logoPath = "data:image/png;base64," + base64Image;


What happens when the logo is not a png?

Thank you @dkayiwa. This is the default fallback logo, which we hardcoded to a PNG file at DEFAULT_LOGO_CLASSPATH = "/images/openmrs_logo_white_large.png"
Are we expecting the default logo format to change from PNG to something else? If so, I can make this more dynamic, but otherwise the hardcoded MIME type should match the hardcoded PNG file.

What happens when someone's config has a JPG?

The user-supplied logo will be retrieved using its absolute path (step 1 in configureLogo()) before falling back to the default logo. So if someone configures a JPG in their report.patientIdSticker.logourl setting, the absolute file path to that JPG will be used directly. The MIME type image/png only applies to our hardcoded default fallback logo (openmrs_logo_white_large.png), not to user-supplied images.

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

dkayiwa · 2025-11-01T20:27:56Z

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

-							Map<String, String> addressData = (Map<String, String>) patientData.get("preferredAddress");
-							if (addressData != null) {
+							List<Map<String, String>> addressData = (List<Map<String, String>>) patientData.get("addresses");
+							if (addressData != null && !addressData.isEmpty()) {


Is this also related to the logo?

No! This ensures that the address data is pulled from the right variable in the Patient Data object.
Since its a single line of change and related to intended field field not showing, i thought its better merging the work with the logo not showing.

...mrs/module/patientdocuments/web/rest/controller/PatientIdStickerDataPdfExportController.java

…64 data URI

jnsereko · 2025-11-04T07:50:53Z

Hey @dkayiwa and @ibacher,
Could you kindly take a look at my latest changes and share your feedback? Thanks!

dkayiwa · 2025-11-04T11:57:41Z

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java

+		// Create and append logo elements
 		Element branding = doc.createElement("branding");
 		Element image = doc.createElement("logo");
 		image.setTextContent(logoPath);


What is displayed for the image when logoPath is an empty string?

I have conditionally added logo creation.

dkayiwa · 2025-11-04T12:51:45Z

Is the pull request description still valid?

jnsereko · 2025-11-04T15:57:21Z

Is the pull request description still valid?

I have revised the pull request description and believe it now provides a more comprehensive and accurate representation of the changes.

dkayiwa · 2025-11-04T18:52:38Z

@jnsereko though i have merged this, i would like us to limit this to only locations in the application data directory. Can you deal with the security safeguards against path traversal or unintended file reads?
After doing that in a followup pull request, we then should be able to include this in O3

jnsereko · 2025-11-05T14:36:47Z

@jnsereko though i have merged this, i would like us to limit this to only locations in the application data directory. Can you deal with the security safeguards against path traversal or unintended file reads? After doing that in a followup pull request, we then should be able to include this in O3

Thank you @dkayiwa!
I have created a followup Up PR solving this here.

Fix logo and address not showing in Sticker

0d006e9

jnsereko marked this pull request as draft October 14, 2025 12:45

jnsereko requested review from dkayiwa and ibacher October 15, 2025 10:58

jnsereko marked this pull request as ready for review October 15, 2025 10:58

This was referenced Oct 15, 2025

(feat) O3-5032: Add Patient documents module to RefApp openmrs/openmrs-distro-referenceapplication#940

Merged

(feat) O3-5124: Add Patient Identifier Sticker configuration to Distro openmrs/openmrs-content-referenceapplication#10

Closed

ibacher reviewed Oct 15, 2025

View reviewed changes

api/src/main/resources/msfStickerFopStylesheet.xsl Show resolved Hide resolved

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java Outdated Show resolved Hide resolved

jnsereko mentioned this pull request Oct 16, 2025

(feat) O3-5124: Add Patient Identifier Sticker configuration to Distro openmrs/openmrs-content-referenceapplication-demo#48

Merged

ibacher reviewed Oct 23, 2025

View reviewed changes

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java Outdated Show resolved Hide resolved

jnsereko added 3 commits October 28, 2025 00:59

working non-clean logo

1aed602

pull logo from servlet context as default

6d6f03a

remove added openmrs logo

1fdce7f

jnsereko requested a review from ibacher October 28, 2025 10:22

update documentation

c405768

load default logo if no logo config suplied

f82dddc

dkayiwa reviewed Oct 29, 2025

View reviewed changes

only cache if the file doesn't already exist.

bb58b1f

jnsereko requested a review from dkayiwa October 30, 2025 07:29

jnsereko added 2 commits October 30, 2025 15:30

remove ability to cache logo

4a533f9

remove ability to cache logo

16233d6

dkayiwa reviewed Oct 30, 2025

View reviewed changes

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java Outdated Show resolved Hide resolved

update documentation to remove caching

317aef0

jnsereko requested a review from dkayiwa October 30, 2025 13:49

dkayiwa reviewed Nov 1, 2025

View reviewed changes

...ain/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java Outdated Show resolved Hide resolved

dkayiwa reviewed Nov 1, 2025

View reviewed changes

...mrs/module/patientdocuments/web/rest/controller/PatientIdStickerDataPdfExportController.java Outdated Show resolved Hide resolved

dkayiwa reviewed Nov 1, 2025

View reviewed changes

...mrs/module/patientdocuments/web/rest/controller/PatientIdStickerDataPdfExportController.java Outdated Show resolved Hide resolved

jnsereko and others added 3 commits November 3, 2025 16:16

Fix formatting of loadDefaultLogo call

6427674

Simplify logo loading by removing unnecessary variable

aa3aac8

update documentation to show support for both absolute paths and base…

85f8aa8

…64 data URI

jnsereko requested a review from dkayiwa November 3, 2025 14:34

dkayiwa reviewed Nov 4, 2025

View reviewed changes

Add logo element creation conditionally

71ad294

jnsereko requested a review from dkayiwa November 4, 2025 15:56

dkayiwa merged commit ede0660 into openmrs:main Nov 4, 2025
1 check passed

jnsereko mentioned this pull request Nov 11, 2025

(feat) O3-5186: Add security safeguards for logo path configuration #5

Merged

Conversation

jnsereko commented Oct 14, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Screenshot

Related issue

Uh oh!

jnsereko commented Oct 15, 2025

Uh oh!

dkayiwa commented Oct 15, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jnsereko commented Oct 28, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnsereko commented Oct 29, 2025

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!

dkayiwa Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

jnsereko Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

jnsereko commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dkayiwa commented Nov 4, 2025

Uh oh!

jnsereko commented Nov 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

dkayiwa commented Nov 4, 2025

Uh oh!

jnsereko commented Nov 5, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

jnsereko commented Oct 14, 2025 •

edited

Loading

jnsereko commented Oct 28, 2025 •

edited

Loading

dkayiwa Oct 30, 2025 •

edited

Loading

jnsereko Oct 30, 2025 •

edited

Loading

jnsereko commented Nov 4, 2025 •

edited

Loading

jnsereko commented Nov 4, 2025 •

edited

Loading