Skip to content

[WIP] Fix code quality findings#80

Draft
Duvet05 with Copilot wants to merge 1 commit into
mainfrom
copilot/fix-180220923-1238257857-4c0ce64e-6ef3-4122-997b-a5c27285ea1f
Draft

[WIP] Fix code quality findings#80
Duvet05 with Copilot wants to merge 1 commit into
mainfrom
copilot/fix-180220923-1238257857-4c0ce64e-6ef3-4122-997b-a5c27285ea1f

Conversation

Copilot AI commented Jun 2, 2026

Copy link
Copy Markdown
Contributor

Thanks for asking me to work on this. I will get started on it and keep this PR's description up to date as I form a plan and make progress.

Original prompt
Please apply the following diffs and create a pull request.
Once the PR is ready, give it a title based on the messages of the fixes being applied.

[{"message":"The default value for `sticker-width` is `\"297mm\"`, which is the same as the default for `sticker-height`. A 297mm × 297mm sticker is unusually large and likely incorrect. The width default should probably be a different value (e.g., `\"210mm\"` for A4 width, or whatever the intended sticker width is).","fixFiles":[{"filePath":"modules/patientdocuments/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java","diff":"diff --git a/modules/patientdocuments/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/modules/patientdocuments/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java\n--- a/modules/patientdocuments/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java\n+++ b/modules/patientdocuments/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java\n@@ -170,7 +170,7 @@\n     String stickerWidth =\n         getInitializerService().getValueFromKey(\"report.patientIdSticker.size.width\");\n     rootElement.setAttribute(\"sticker-height\", isNotBlank(stickerHeight) ? stickerHeight : \"297mm\");\n-    rootElement.setAttribute(\"sticker-width\", isNotBlank(stickerWidth) ? stickerWidth : \"297mm\");\n+    rootElement.setAttribute(\"sticker-width\", isNotBlank(stickerWidth) ? stickerWidth : \"210mm\");\n   }\n \n   private void configureFontSettings(Element rootElement) {\n"}]},{"message":"String concatenation is used for the log message here, while the rest of the file consistently uses the SLF4J parameterized logging style (e.g., `log.error(\"...: {}\", value, e)`). This should be changed to `log.error(\"Invalid logo path: {}\", logoUrlPath, e)` for consistency and to avoid unnecessary string construction when the log level is disabled.","fixFiles":[{"filePath":"modules/patientdocuments/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java","diff":"diff --git a/modules/patientdocuments/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/modules/patientdocuments/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java\n--- a/modules/patientdocuments/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java\n+++ b/modules/patientdocuments/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java\n@@ -376,7 +376,7 @@\n \n       return resolvedLogoRealPath.toFile();\n     } catch (IllegalArgumentException e) {\n-      log.error(\"Invalid logo path: \" + logoUrlPath, e);\n+      log.error(\"Invalid logo path: {}\", logoUrlPath, e);\n       return null;\n     } catch (IOException e) {\n       log.error(\"Failed to access logo file: {}\", logoUrlPath, e);\n"}]},{"message":"A new `ObjectMapper` instance is created for every row in the data set. `ObjectMapper` is thread-safe and expensive to construct; it should be created once (e.g., as a static final field) and reused.","fixFiles":[{"filePath":"modules/patientdocuments/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java","diff":"diff --git a/modules/patientdocuments/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java b/modules/patientdocuments/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java\n--- a/modules/patientdocuments/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java\n+++ b/modules/patientdocuments/src/main/java/org/openmrs/module/patientdocuments/renderer/PatientIdStickerXmlReportRenderer.java\n@@ -62,6 +62,7 @@\n \n /** ReportRenderer that renders to a default XML format */\n @Component\n+private static final ObjectMapper OBJECT_MAPPER = new ObjectMapper();\n @Handler\n @Localized(\"patientdocuments.patientIdStickerXmlReportRenderer\")\n public class PatientIdStickerXmlReportRenderer extends ReportDesignRenderer {\n@@ -459,8 +460,7 @@\n \n         if (jsonData != null) {\n           try {\n-            ObjectMapper mapper = new ObjectMapper();\n-            Map<String, Object> patientData = mapper.readValue(jsonData, PATIENT_DATA_TYPE);\n+            Map<String, Object> patientData = OBJECT_MAPPER.readValue(jsonData, PATIENT_DATA_TYPE);\n \n             // Process identifiers\n             List<Map<String, Object>> identifiers = getMapList(patientData, \"identifiers\");\n"}]},{"message":"The barcode element is appended to `templatePIDElement` inside the row loop, so if there are multiple data rows, multiple `<barcode>` elements will be appended to the template. The barcode...

View with Codesmith Autofix with Codesmith
Need help on this PR? Tag /codesmith with what you need. Autofix is disabled.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants