Sync experimental with maintenance#2394
Closed
sebastian-j-ibanez wants to merge 1116 commits intoexperimentalfrom
Closed
Sync experimental with maintenance#2394sebastian-j-ibanez wants to merge 1116 commits intoexperimentalfrom
sebastian-j-ibanez wants to merge 1116 commits intoexperimentalfrom
Conversation
…es-are-not-displaying
…re instance-based The static `clientKeystore` variable in EdtClientBuilder was shared between MCEDT and HCV services, causing a race condition where HCV validation would overwrite the MCEDT keystore path. This resulted in MCEDT upload failures after HCV validation operations.
Enhance JavaDoc documentation for setClientKeystoreFilename and setExternalClientKeystoreFilename methods to comply with CLAUDE.md standards. Added detailed descriptions, @param tags with specific data types, and @SInCE tags based on git history. - EdtClientBuilder.setClientKeystoreFilename: Added context about instance-based keystore paths preventing race conditions - DelegateFactory/OnlineHCValidator/EDTBaseTest: Converted C-style comments to proper JavaDoc with comprehensive parameter documentation Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
Per CLAUDE.md documentation standards, @SInCE tags must use accurate dates from git history. Updated all keystore configuration method JavaDoc to reflect the actual commit date of these changes. Co-authored-by: Deval Italiya <D3V41@users.noreply.github.com>
…ration Added validation to check for empty or whitespace-only strings in addition to null checks. This prevents Files.exists() from resolving to the current directory when an empty string is provided, which would cause authentication failures with confusing error messages. Addresses Sourcery code review comments on DelegateFactory.java and OnlineHCValidator.java. Co-authored-by: Deval Italiya <D3V41@users.noreply.github.com>
Added test to verify that EdtClientBuilder instances maintain independent keystore configurations. This prevents regression to the previous race condition where a static clientKeystore variable was shared across all instances, causing MCEDT uploads to fail after HCV validation. The test uses reflection to verify that two builder instances can be configured with different keystore paths without interfering with each other. If clientKeystore were ever made static again, this test would fail. Addresses Sourcery code review suggestion for regression test coverage. Co-authored-by: Deval Italiya <D3V41@users.noreply.github.com>
Added tests to verify that setExternalClientKeystoreFilename correctly handles edge cases where the keystore path is null or points to a non-existent file. Both cases should result in the default keystore being used, per the method's documented contract. These tests ensure that: 1. Null paths do not modify the builder's keystore configuration 2. Non-existent file paths do not modify the builder's keystore configuration 3. The behavior matches the JavaDoc promise of falling back to the default keystore at src/main/resources/clientKeystore.properties Addresses Sourcery code review suggestion for test coverage of documented fallback behavior. Co-authored-by: Deval Italiya <D3V41@users.noreply.github.com>
- Replace jtidy 1.0.5 (unmaintained since ~2010) with Jsoup 1.17.2 - Migrate parseJSP2PDF(), parseString2PDF(), and parseString2Bin() methods - Configure Jsoup with XML syntax and XHTML entities for iText compatibility - Set prettyPrint(false) to prevent whitespace issues in iText XML parser - Fix character encoding by using UTF-8 explicitly (was platform-dependent) - Remove jtidy dependency from pom.xml - Add comprehensive integration tests with medical terminology and special characters Benefits: - Removes 15-year-old unmaintained library - Fixes character encoding bug (critical for French Canadian patient names) - Better HTML5 support with actively maintained library - Improved security with modern, maintained dependency Fixes #2154 Co-authored-by: Michael Yingbull <yingbull@users.noreply.github.com>
- Exclude cglib:cglib:2.2.2 from spring-aop dependency - Add explicit cglib:cglib-nodep:3.3.0 dependency - cglib-nodep bundles ASM internally, eliminating classpath conflicts - Resolves collision between asm:asm:3.3.1 and org.ow2.asm:asm:9.9 - Compatible with Spring 5.3.39 and Hibernate 5.6.15.Final Fixes #2221 Co-authored-by: Michael Yingbull <yingbull@users.noreply.github.com>
- Replace dom4j imports with JDOM2 equivalents in ManageDashboard2Action - Migrate XML parsing from SAXReader to SAXBuilder - Update validation logic to use standard javax.xml.validation API - Remove dom4j dependency from pom.xml - Maintain all XXE attack prevention security features Closes #2138 Co-authored-by: Michael Yingbull <yingbull@users.noreply.github.com>
…Phase 1) - Migrated 9 files from org.apache.commons.beanutils.BeanUtils to org.springframework.beans.BeanUtils - Reversed parameter order in copyProperties() calls (Spring uses source-first convention) - Migrated BeanUtilHlp to use Spring BeanWrapper for property access - Removed explicit commons-beanutils 1.11.0 dependency from pom.xml - Simplified exception handling (generic Exception instead of specific reflection exceptions) This migration eliminates the unmaintained commons-beanutils library and its associated CVE exposure (CVE-2014-0114, CVE-2019-10086, CVE-2025-48734). Spring BeanUtils is actively maintained and already included in our Spring Framework 5.3.39 dependency. Files modified: - src/main/java/ca/openosp/openo/util/BeanUtilHlp.java - src/main/java/ca/openosp/openo/prescript/web/CopyFavorites2Action.java - src/main/java/ca/openosp/openo/integration/mchcv/HCValidationResult.java - src/main/java/ca/openosp/openo/form/pharmaForms/formBPMH/business/BpmhForm2Handler.java - src/main/java/ca/openosp/openo/dashboard/handler/TicklerHandler.java - src/main/java/ca/openosp/openo/dashboard/factory/DashboardBeanFactory.java - src/main/java/ca/openosp/openo/dashboard/factory/DrilldownBeanFactory.java - src/main/java/ca/openosp/openo/commn/model/Demographic.java - src/main/java/ca/openosp/openo/PMmodule/web/ClientManager2Action.java - pom.xml Fixes #2202 Co-authored-by: Michael Yingbull <yingbull@users.noreply.github.com>
- Update Maven dependency to com.github.hazendaz:displaytag:2.9.0 - Modernize 17-year-old dead dependency with actively maintained fork - Simplify dependency exclusions (hazendaz uses modern versions) - Remove exclusions for commons-beanutils, commons-lang, itext - Keep SLF4J exclusions to avoid conflicts with OpenO's logging This is a temporary bridge solution. Full migration to DataTables is planned for Q2 2026 (see issue #2152). Fixes #2152 (Phase 1 - Upgrade to maintained fork) IMPORTANT: After merging, run 'make lock' to update dependency lock files. Co-authored-by: Michael Yingbull <yingbull@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
Co-authored-by: cubic-dev-ai[bot] <191113872+cubic-dev-ai[bot]@users.noreply.github.com>
- Add PathValidationUtils.validateUpload() to prevent path traversal attacks - Add FEATURE_SECURE_PROCESSING to SchemaFactory for complete XXE protection - Enforce external DTD/schema access restrictions on SchemaFactory These changes address security issues identified in code reviews by cubic-dev-ai and coderabbitai. Co-authored-by: Michael Yingbull <yingbull@users.noreply.github.com>
- Fix PHI exposure in debug logging (now logs only content length) - Add explicit UTF-8 encoding to all .getBytes() calls (lines 253, 270, 319) - Extract duplicated Jsoup configuration to configureJsoupForXhtml() helper - Add comprehensive JavaDoc to parseJSP2PDF, parseString2PDF, parseString2Bin - Fix BDD test naming: all tests now follow should<Action>_when<Condition> pattern - Extract common test setup to @beforeeach to reduce duplication Co-authored-by: Michael Yingbull <yingbull@users.noreply.github.com>
chore: upgrade cglib from 2.2.2 to 3.3.0 to resolve ASM conflicts
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
chore: upgrade displaytag 1.2 → hazendaz 2.9.0
Change BeanUtilHlp.getPropertyValue() to return "null" string for null properties instead of empty string, matching Apache Commons BeanUtils behavior. This maintains compatibility with existing code like MSPReconcile.java that expects the string "null" for null values. Co-authored-by: Michael Yingbull <yingbull@users.noreply.github.com>
Complete migration from Apache Commons BeanComparator to modern Java 8+ Comparator API. This completes Phase 1 of commons-beanutils removal by eliminating all remaining Apache Commons BeanUtils dependencies. Changes: - Replaced BeanComparator with Comparator.comparing() in 9 files - Replaced ReverseComparator with .reversed() method - Removed all org.apache.commons.beanutils imports - Zero new dependencies (uses built-in Java Comparator) - Type-safe property references instead of string-based reflection Benefits: - Eliminates CVE exposure (CVE-2014-0114, CVE-2019-10086, CVE-2025-48734) - Better performance (no reflection overhead) - Compile-time safety (method references vs string property names) - More maintainable modern Java code Files migrated: 1. ProviderData.java (2 usages - formattedName sorting) 2. ProviderProperty2Action.java (2 usages - lastName sorting) 3. RptMeasurementTypesBeanHandler.java (2 usages - typeDisplayName, typeDescription) 4. RptLabReportData.java (1 usage - lastName sorting) 5. ThirdApptTimeReporter.java (1 usage - startTime reversed) 6. EctStyleSheetBeanHandler.java (1 usage - id sorting) 7. AppointmentStatusMgrImpl.java (1 usage - id sorting) 8. ProviderNameBeanHandler.java (1 usage - firstName sorting) 9. MsgMessengerGroupData.java (1 usage - lastName sorting) Related-to: #2202 Co-authored-by: Michael Yingbull <yingbull@users.noreply.github.com>
chore: update lock file for recent dependency updates.
…URL params, and added request with header field
…ter-document-use' into hotfix-package/20260305
Introduced 'No-Show' and 'Cancelled' status filters to billing report JSPs and backend DAO methods, allowing users to include or exclude these appointment types in unbilled reports. Updated UI to present filter checkboxes and refactored DAO method signatures and queries to support the new filtering options.
…_filters Enhancement-main: Add status filters to unbilled appointment reports
Bug fix + Enhancement Package 20260305
… taglib URI + logoutSSO fixes)
…dundant call with dependency changes
…g/02172026 Resolved Conflicts: src/main/java/ca/openosp/openo/commn/dao/EmailConfigDao.java src/main/java/ca/openosp/openo/commn/dao/EmailConfigDaoImpl.java src/main/webapp/oscarRx/SearchDrug3.jsp
… develop-staging/02172026
Resolved Conflicts: src/main/java/ca/openosp/openo/prescript/pageUtil/RxChooseDrug2Action.java src/main/java/ca/openosp/openo/prescript/pageUtil/RxChoosePatient2Action.java src/main/java/ca/openosp/openo/prescript/pageUtil/RxSearchDrug2Action.java src/main/java/ca/openosp/openo/prescript/pageUtil/RxShowAllergy2Action.java src/main/java/ca/openosp/openo/prescript/pageUtil/RxUpdateDrugref2Action.java src/main/java/ca/openosp/openo/prescript/pageUtil/RxWriteScript2Action.java src/main/java/ca/openosp/openo/scratch/Scratch2Action.java
OpenO EMR Staging 02/17/2026: Security, Dependencies & Documentation + Additional recent commits with dependency upgrades and compilation error fixes
…s-upgrade-6.8-port Resolved Conflicts: src/main/java/ca/openosp/openo/integration/clinicalconnect/ClinicalConnectViewer2Action.java
Port of Struts 2.5.33 → 6.8.0 upgrade (from PR #1065)
Develop merge into maintenance
…xclusion behaviour on client image action
…setup method per action
…ve out the old fileUpload interceptor, to avoid ALL usage completely with any potential additions later on, or any CVEs past the current one, this block should be completely removed when upgrading to struts 7.0.0 or later, as the fileUpload interceptor is completely removed, no reason to have these changes at that point
… re-adding some missing filename and contentype setters, refactoring code, dealing with null values or closed inputstreams
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces comprehensive UI testing documentation and infrastructure, including Playwright MCP configuration, execution guides for nine test scenarios, and a centralized test writing guide. The review comments highlight a naming convention inconsistency between docs/test/claude-test-context.md and docs/test/test-writing-best-practices.md regarding the use of underscores in BDD-style test method names. I recommend standardizing the documentation to explicitly favor the camelCase pattern with a single underscore as the preferred convention.
| - `should_throw_exception_when_tickler_not_found()` | ||
| - `should_load_spring_context()` | ||
|
|
||
| **AVOID**: Mixed camelCase with underscores (e.g., `shouldReturnTickler_whenValid`) |
There was a problem hiding this comment.
This guideline states to 'AVOID: Mixed camelCase with underscores'. However, docs/test/test-writing-best-practices.md (line 10) explicitly states that should<ExpectedBehavior>_when<Condition> (camelCase with ONE underscore) is the 'PREFERRED' pattern. This creates an inconsistency in the BDD naming convention. Please clarify which pattern should be followed.
| ```java | ||
| // Pattern 1: should_<expectedBehavior>_when_<condition> | ||
| // Pattern 1: should<ExpectedBehavior>_when<Condition> (PREFERRED - camelCase with ONE underscore) | ||
| @Test |
There was a problem hiding this comment.
This guideline states that should<ExpectedBehavior>_when<Condition> (camelCase with ONE underscore) is the 'PREFERRED' pattern. However, docs/test/claude-test-context.md (line 29) explicitly states to 'AVOID: Mixed camelCase with underscores'. This creates an inconsistency in the BDD naming convention. Please clarify which pattern should be followed.
…, OWASP output encoding for eForm filenames, try-with-resources for stream handling, and removing untrusted data from exception messages
…t would give NullPointerExceptions
…-upload-interceptor-migration Security: migrate file upload actions from FileUploadInterceptor to ActionFileUploadInterceptor
Collaborator
|
Honestly unsure if we are going to use experimental anymore. When I was looking into the branch, really the only changes that were needed to be ported over to maintenance was the struts 6.8 upgrade, other than that the diffs seemed to be overlapping or older versions of changes in files, so I don't think we need to worry about experimental in my opinion. But I could be wrong |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
8 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by cubic
Syncs experimental into maintenance, delivering a full Playwright-based UI test framework, hardened CI/CD/devcontainer setup, and a broad set of security and performance fixes across core modules.
.claude/commands/ui-tests, gold screenshots, and complete guides indocs/ui-tests/./openo-dev:resetsql-developerfor DB resets; security hooks (validate-owasp-encoding.py,validate-sql-safety.py); modern test context injector.container-images.yml), parallelized Maven workflow, SonarCloud analysis, dependency submission, tighter@claudetriggers, issue triage; removed deprecated workflows..mcp.jsonfor@playwright/mcp, Chromium path verifier script, fonts, and Playwright deps; Ubuntu 24.04 t64 libs;.gitignoreupdated for UI test outputs.jtidy→jsoupfor Doc2PDF,dom4j→jdom2,commons-beanutils→SpringBeanUtils,displaytag→com.github.hazendaz:displaytag:2.9.0,cglib→3.3.0; HAPI parent POM restored; lock files updated.update-2026-01-26-tickler-indexes.sql)./health/heartbeatendpoint via base filter; extensive docs (JSP refactoring, modern test framework, UI test process, jasypt→Spring Crypto migration).Written for commit 93de516. Summary will update on new commits.