fix: redact PHI from ProviderPeriodAppsTo.toString()#3081
Conversation
Reviewer's GuideRedacts PHI from ProviderPeriodAppsTo.toString() by limiting output to safe identifiers and adds regression tests to ensure the safe format, PHI exclusion, and null identifier handling. File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (2)
📜 Recent review details
|
| Layer / File(s) | Summary |
|---|---|
toString() update and regression tests src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsTo.java, src/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java |
toString() now returns only appointmentNo and providerNo; the new tests assert that notes, name, demographic, location, resources, status, user, and update datetime do not appear, and that null identifiers format correctly. |
Estimated code review effort
🎯 2 (Simple) | ⏱️ ~5 minutes
Poem
🐇 I hopped through logs with a careful grin,
Two safe identifiers now printed within.
No notes, no names, just the tidy pair—
A quieter toString with less to share.
🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage | Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
✅ Passed checks (4 passed)
| Check name | Status | Explanation |
|---|---|---|
| Title check | ✅ Passed | The title clearly summarizes the main change: redacting PHI from ProviderPeriodAppsTo.toString(). |
| Description check | ✅ Passed | The description is directly related to the code changes and regression tests in this pull request. |
| Linked Issues check | ✅ Passed | The changes remove PHI-bearing fields from toString() and keep only appointmentNo and providerNo, matching #2808. |
| Out of Scope Changes check | ✅ Passed | The added regression tests and the narrowed string output are in scope for the PHI redaction fix. |
✨ Finishing Touches
🧪 Generate unit tests (beta)
- Create PR with unit tests
✨ Simplify code
- Create PR with simplified code
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 @coderabbitai help to get the list of available commands.
![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 simplifies the toString() method of ProviderPeriodAppsTo to output only safe identifiers (appointmentNo and providerNo) and introduces a new unit test class ProviderPeriodAppsToUnitTest to verify this behavior. The reviewer's feedback correctly identifies that the test method names do not conform to the repository's BDD test naming convention (should<Action>_<preposition><Condition>) and provides appropriate suggestions to align them with the style guide.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 1 issue
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location path="src/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java" line_range="30-41" />
<code_context>
+ String result = appointment.toString();
+
+ assertThat(result).isEqualTo("ProviderPeriodAppsTo[appointmentNo=12345, providerNo=provider-abc]");
+ assertThat(result)
+ .doesNotContain("clinical note")
+ .doesNotContain("chest pain")
+ .doesNotContain("Jane Patient")
+ .doesNotContain("67890")
+ .doesNotContain("exam room 7")
+ .doesNotContain("ultrasound room")
+ .doesNotContain("scheduler-user")
+ .doesNotContain("1718000000");
+ }
+
</code_context>
<issue_to_address>
**suggestion (testing):** Consider asserting on field presence rather than specific numeric substrings for PHI exclusion
Using `.doesNotContain("1718000000")` to detect PHI leakage couples the test to a specific numeric value that could legitimately appear in `appointmentNo` or `providerNo` later, making the test brittle. Instead, consider asserting on the absence of the field label (e.g. `"updateDatetime="`) or a more clearly PHI-like string format, so the test verifies that PHI fields are excluded rather than a particular numeric representation.
```suggestion
String result = appointment.toString();
assertThat(result).isEqualTo("ProviderPeriodAppsTo[appointmentNo=12345, providerNo=provider-abc]");
assertThat(result)
.doesNotContain("clinical note")
.doesNotContain("chest pain")
.doesNotContain("Jane Patient")
.doesNotContain("67890")
.doesNotContain("exam room 7")
.doesNotContain("ultrasound room")
.doesNotContain("scheduler-user")
// Assert on the absence of the PHI field label instead of a specific numeric value
.doesNotContain("updateDatetime=");
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
DragonFSKY
force-pushed
the
fix/provider-period-apps-to-string
branch
from
30bfe22 to
e4e9655
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@src/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java`:
- Around line 1-13: The new test class ProviderPeriodAppsToUnitTest is missing
the mandatory CARLOS project header, so add the standard project header at the
top of the file before the package declaration, following the format defined in
docs/copyright-header-carlos.md.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c7d8dfcb-7b99-4446-9448-dd0a81480b0f
📒 Files selected for processing (2)
src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsTo.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
📜 Review details
⚠️ CI failures not shown inline (2)
GitHub Actions: DCO / 0_DCO Sign-Off.txt: fix: redact PHI from ProviderPeriodAppsTo.toString()
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1m�[0m
�[36;1mpr_json=$(curl -fsSL \�[0m
�[36;1m -H "Authorization: ***" \�[0m
�[36;1m -H "Accept: application/vnd.github+json" \�[0m
�[36;1m "https://api.github.com/repos/$REPO/pulls/$PR_NUMBER")�[0m
�[36;1mpr_commit_count=$(jq -r '.commits' <<< "$pr_json")�[0m
�[36;1mbase_ref=$(jq -r '.base.ref' <<< "$pr_json")�[0m
�[36;1m�[0m
�[36;1mif [ "$pr_commit_count" -gt 250 ]; then�[0m
�[36;1m echo "::error::CARLOS EMR DCO check cannot automatically verify $pr_commit_count commits because GitHub's pull request commits API is capped at 250 commits."�[0m
GitHub Actions: DCO / DCO Sign-Off: fix: redact PHI from ProviderPeriodAppsTo.toString()
Conclusion: failure
##[group]Run set -euo pipefail
�[36;1mset -euo pipefail�[0m
�[36;1m�[0m
�[36;1mpr_json=$(curl -fsSL \�[0m
�[36;1m -H "Authorization: ***" \�[0m
�[36;1m -H "Accept: application/vnd.github+json" \�[0m
�[36;1m "https://api.github.com/repos/$REPO/pulls/$PR_NUMBER")�[0m
�[36;1mpr_commit_count=$(jq -r '.commits' <<< "$pr_json")�[0m
�[36;1mbase_ref=$(jq -r '.base.ref' <<< "$pr_json")�[0m
�[36;1m�[0m
�[36;1mif [ "$pr_commit_count" -gt 250 ]; then�[0m
�[36;1m echo "::error::CARLOS EMR DCO check cannot automatically verify $pr_commit_count commits because GitHub's pull request commits API is capped at 250 commits."�[0m
🧰 Additional context used
📓 Path-based instructions (9)
**/*.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.java: Useio.github.carlos_emr.carlos.*namespace for ALL new code; specific packages: DAOs inio.github.carlos_emr.carlos.commn.dao.*, Models inio.github.carlos_emr.carlos.commn.model.*, exception: ProviderDao inio.github.carlos_emr.carlos.dao.ProviderDao
UsePathValidationUtilsfor ALL file path operations that include user input
NEVER log or expose PHI (Protected Health Information) in logs or error messages
UseSpringUtils.getBean(ClassName.class)for Spring bean injection in Struts2 actions and non-Spring-managed classes
JavaDoc is required on all public classes and methods; do NOT include@authortags, use@sincetags with git log timestamps, use CARLOS copyright header for new files
**/*.java: Keep Java package names asio.github.carlos_emr.carlos.*for backward compatibility - DO NOT change internal package names
UsePathValidationUtilsfor ALL file path operations to prevent path traversal attacks
PHI (Patient Health Information) must NEVER be logged or exposed in error messages
Use NEW namespaceio.github.carlos_emr.carlos.*for ALL new code - avoid oldorg.oscarehr.*andoscar.*packages
Use sanctioned suffixes for layer naming:*Action,*ViewModelAssembler,*ViewModel,*Loader,*Resolver,*Composer,*Validator,*Persister,*Calculator,*Parser,*ImportService,*Service,*Dao,*Dto,*Command. Never combine two role-suffixes or use forbidden suffixes like*Manager,*Helper,*Utils,*Prep
Public JavaDoc is required for public entrypoint classes and non-obvious public/protected methods; carrier DTOs, getters/setters, and private helpers do not need method-by-method JavaDoc
DO NOT add@authortags in JavaDoc (misleading after Bitbucket→GitHub migration); use git history for accurate dates with@sincetags
Inline comments should explain the why: legacy compatibility, security/compliance constraints, file-format offsets, transaction boundaries, and non-obvious control flow. Do not narrate o...
Files:
src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsTo.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
**/*.{jsp,java}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use OWASP Encoder for ALL user input output: in JSP use
${e:forHtml(value)}with taglib<%@ taglib uri="owasp.encoder.jakarta" prefix="e" %>, in Java useEncode.forHtml(value), with context-specific variants (forHtmlAttribute,forJavaScript,forCssString,forUri,forUriComponent)Use OWASP Encoder null-safe CARLOS wrappers for all user input encoding:
<carlos:encode>tag,${carlos:forXxx()}EL functions, orSafeEncode.forXxx()Java methods. DO NOT use<e:forXxx>,${e:forXxx()}, orEncode.forXxx()directly in new code.
Files:
src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsTo.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
**/*.{java,sql}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Use parameterized queries ONLY in SQL/HQL operations; never use string concatenation for SQL queries
Files:
src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsTo.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
src/**/*.java
📄 CodeRabbit inference engine (GEMINI.md)
src/**/*.java: Keep Java packages asio.github.carlos_emr.carlos.*for backward compatibility - do NOT change these internal namespace paths
Preserve existing copyright headers in modified files and preserve GPL version exactly as-is; never remove existing copyright notices
Files:
src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsTo.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
{src/main/java/io/github/carlos_emr/carlos/**/*.java,src/main/webapp/**/*.jsp}
📄 CodeRabbit inference engine (GEMINI.md)
PHI (Patient Health Information) must NEVER be logged or exposed in error messages
Files:
src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsTo.java
src/main/java/io/github/carlos_emr/carlos/**/*.java
📄 CodeRabbit inference engine (GEMINI.md)
src/main/java/io/github/carlos_emr/carlos/**/*.java: All public classes and methods MUST have comprehensive JavaDoc; do NOT add@authortags; use@sincewith accurate git history dates; document@param,@return, and@throwstags with specific types
Add inline comments on separate lines (not same-line) for complex logic; include comprehensive comments for non-obvious algorithms or healthcare-specific calculations
Files:
src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsTo.java
**/*
📄 CodeRabbit inference engine (CLAUDE.md)
Preserve all existing copyright headers in modified files; do not remove or change GPL version statements
Files:
src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsTo.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
**/*.{java,jsp,xml,js,css}
📄 CodeRabbit inference engine (CLAUDE.md)
New files must use CARLOS project header as defined in
docs/copyright-header-carlos.md
Files:
src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsTo.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
**/*Test.java
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*Test.java: Unit tests must useCarlosUnitTestBasebase class for mocked tests (no database); integration tests must useCarlosTestBasefor H2 database tests
Follow BDD naming convention for test methods:should<Action>_<preposition><Condition>()with ONE underscore, camelCase, andshouldprefix
**/*Test.java: Use hierarchical BDD test naming pattern:should<Action>_<prepositionOrContext><Condition>()with exactly ONE underscore separator, all camelCase, suffix lowercase (e.g.,shouldReturnTickler_whenValidIdProvided)
All integration tests should extendCarlosTestBase; unit tests should extendCarlosUnitTestBaseor domain-specific bases
For Log4j2 assertion testing, useio.github.carlos_emr.carlos.test.logging.LogCapturefromsrc/test/java/io/github/carlos_emr/carlos/test/logging/LogCapture.java- do NOT define local AbstractAppender or per-test logger-config copies
SpringUtils identity across multiple Spring contexts: assert type (isInstanceOf) not instance identity (isSameAs/isEqualTo)
Files:
src/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
🧠 Learnings (18)
📚 Learning: 2026-03-06T23:54:01.319Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 563
File: src/main/java/io/github/carlos_emr/carlos/lab/ca/all/parsers/AlphaHandler.java:59-63
Timestamp: 2026-03-06T23:54:01.319Z
Learning: In the carlos-emr/carlos repository, for Java sources under src/main/java, empty no-arg constructors should have only a minimal one-line JavaDoc. Do not add since or other metadata tags to empty constructors, as they provide no meaningful value. Place documentation efforts in the class-level JavaDoc instead.
Applied to files:
src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsTo.java
📚 Learning: 2026-03-13T21:29:23.775Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 590
File: src/main/java/io/github/carlos_emr/carlos/eform/actions/DisplayImage2Action.java:69-69
Timestamp: 2026-03-13T21:29:23.775Z
Learning: When reviewing Java source files, allow since Javadoc tags to reflect substantial rewrites or major API changes (e.g., security overhaul, new API surface). Do not flag the tag as incorrect solely because the file predates the tagged date. If flagging is considered, verify against commit history or release notes to confirm the tag corresponds to a meaningful change, not the original introduction. Apply this guidance across Java sources in the repository (src/main/java/**/*.java).
Applied to files:
src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsTo.java
📚 Learning: 2026-04-11T22:46:51.202Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1556
File: src/main/java/io/github/carlos_emr/carlos/demographic/dto/DemographicListItemDTO.java:74-179
Timestamp: 2026-04-11T22:46:51.202Z
Learning: In this repository, do not require method-level JavaDoc for plain public JavaBean-style getters and setters in Java files under src/main/java/**/*.java. Their meaning should be covered by field-level documentation and/or the class-level JavaDoc. Only require JavaDoc for non-trivial public methods (e.g., constructors with logic, factory methods, and computed/derived accessors where behavior is not just returning/setting a field). In code reviews, missing JavaDoc on simple getters/setters should not be flagged as a documentation violation.
Applied to files:
src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsTo.java
📚 Learning: 2026-04-11T22:47:04.719Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1556
File: src/main/java/io/github/carlos_emr/carlos/provider/dto/ProviderSummaryDTO.java:57-104
Timestamp: 2026-04-11T22:47:04.719Z
Learning: In this repository, do not require JavaDoc for trivial getter and setter methods. Only JavaDoc for types/classes and for more meaningful public API points is expected—e.g., class-level JavaDoc, constructors, static factory methods, and non-trivial public methods (such as formatting helpers). In code reviews, ignore missing JavaDoc on getters/setters.
Applied to files:
src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsTo.java
📚 Learning: 2026-05-24T01:01:46.756Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1832
File: src/main/java/io/github/carlos_emr/carlos/utility/CachedDateFormats.java:165-216
Timestamp: 2026-05-24T01:01:46.756Z
Learning: For Java sources under src/main/java/**/*.java, follow the repository’s Documentation Standards (CLAUDE.md) regarding Javadoc requirements: do not require mechanical param/return/throws tags for trivial wrapper/delegator public methods that are essentially one-line pass-throughs to a cached or underlying instance (e.g., methods like parseDefault/formatDefault/parse/format in CachedDateFormats.java). Instead, rely on class-level Javadoc to document the overall contracts, including thread-safety, null handling, return value semantics, and any mutation/restoration behavior (such as timezone restoration). Require full per-method Javadoc tag coverage only for non-trivial public methods whose behavior is not obvious from the context/signature.
Applied to files:
src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsTo.java
📚 Learning: 2026-06-25T16:10:08.376Z
Learnt from: Ben-Heerema
Repo: carlos-emr/carlos PR: 3023
File: src/main/java/io/github/carlos_emr/carlos/encounter/oscarConsultationRequest/pageUtil/EConsult2Action.java:287-328
Timestamp: 2026-06-25T16:10:08.376Z
Learning: When reviewing Java code that reconstructs an origin/authority from a `java.net.URI` (e.g., `scheme://host:port`), assume `URI#getHost()` may already return bracketed IPv6 literals such as `[::1]` (not just `::1`). Do not “re-bracket” IPv6 manually when rebuilding the origin from the host returned by `getHost()`. If `getHost()` is `null`, only then apply the malformed-colon/authority fallback rejection logic on that authority-fallback path (the path that runs when `getHost()` is `null`), not on the normal host-based reconstruction path.
Applied to files:
src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsTo.java
📚 Learning: 2026-03-24T17:34:18.508Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 716
File: src/main/java/io/github/carlos_emr/carlos/eform/actions/RtlPreventions2Action.java:155-156
Timestamp: 2026-03-24T17:34:18.508Z
Learning: In this repository (carlos-emr/carlos), treat `demographic_no` as an internal database surrogate integer key (the PK of the demographic table), not as PHI. When reviewing code, do not flag logging or output of `demographic_no` as PHI exposure. PHI to flag includes clinical/demographic data elements like patient name, date of birth, HIN, and address.
Applied to files:
src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsTo.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
📚 Learning: 2026-04-07T02:57:10.703Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1111
File: src/main/java/io/github/carlos_emr/carlos/util/JDBCUtil.java:140-142
Timestamp: 2026-04-07T02:57:10.703Z
Learning: When reviewing this repository’s Java code, flag any unprotected use of `SchemaFactory.newInstance(...)` (e.g., instances that do not restrict `XMLConstants.ACCESS_EXTERNAL_DTD` and `XMLConstants.ACCESS_EXTERNAL_SCHEMA` to empty strings). Recommend migrating those call sites to `io.github.carlos_emr.carlos.utility.XmlUtils.createSecureSchemaFactory()` so XML Schema validation is hardened against external entity/schema access. Known migrated call sites include `IndicatorTemplateHandler.java` and `DemographicExportAction42Action.java` (post-PR `#1111`).
Applied to files:
src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsTo.javasrc/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
📚 Learning: 2026-03-21T14:58:44.822Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 704
File: src/test/java/io/github/carlos_emr/carlos/commn/dao/Hl7TextInfoDaoIntegrationTest.java:109-110
Timestamp: 2026-03-21T14:58:44.822Z
Learning: For the test-only HibernateTemplate implementation in src/test/java/io/github/carlos_emr/carlos/test/base/HibernateTemplate.java, positional parameter binding is 1-based (it calls query.setParameter(i + 1, ...)). Therefore, in any test code that calls hibernateTemplate.find(queryString, ...), all HQL positional parameters in queryString must use ?1, ?2, etc. Never use ?0 (or other 0-based indices), otherwise Hibernate will raise a runtime parameter binding error.
Applied to files:
src/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
📚 Learning: 2026-05-24T01:02:42.707Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1832
File: src/test/java/io/github/carlos_emr/carlos/webserv/rest/ReportingServiceUnitTest.java:163-270
Timestamp: 2026-05-24T01:02:42.707Z
Learning: In this repository’s Java test code (under src/test/java), CRUD/operation tags on test methods such as Tag("create"), Tag("read"), or Tag("query") are optional “filtering vocabulary” and are NOT required. Since CI does not enforce these CRUD/operation tags, do not flag missing CRUD/operation Tag values on test methods. Only enforce the mandatory test-type tags defined in CLAUDE.md (e.g., Tag("unit"), Tag("fast")).
Applied to files:
src/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
📚 Learning: 2026-05-24T01:02:47.540Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1832
File: src/test/java/io/github/carlos_emr/carlos/util/UtilDateUtilitiesUnitTest.java:42-44
Timestamp: 2026-05-24T01:02:47.540Z
Learning: In this repository, the `CarlosUnitTestBase` test base class is intended only for tests that need to mock `SpringUtils` and `LogAction` statics. For pure static-method unit tests (e.g., tests like `UtilDateUtilitiesUnitTest`, `SafeEncodeUnitTest`, `QueryAppenderUnitTest`, `TextualizerUnitTest`, `RequestNegotiationUnitTest`), do not require/flag extending `CarlosUnitTestBase`—absence of `CarlosUnitTestBase` is expected when static mocking of `SpringUtils`/`LogAction` isn’t needed.
Applied to files:
src/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
📚 Learning: 2026-05-24T01:02:46.757Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1832
File: src/test/java/io/github/carlos_emr/carlos/util/DateUtilsConvertDate8CharUnitTest.java:39-42
Timestamp: 2026-05-24T01:02:46.757Z
Learning: When reviewing this repo’s Java code that uses CRUD/operation method `Tag(...)` annotations, treat most operation tags (e.g., `Tag("read")`) as optional filtering vocabulary. Only `Tag("integration")` and `Tag("dao")` are required for type/layer classification. Because CI (e.g., `bdd-test-naming.yml`/Surefire) enforces only method naming conventions and does not verify operation tags, do not raise review issues solely for missing optional CRUD operation `Tag` annotations.
Applied to files:
src/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
📚 Learning: 2026-05-24T01:02:47.540Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1832
File: src/test/java/io/github/carlos_emr/carlos/util/UtilDateUtilitiesUnitTest.java:42-44
Timestamp: 2026-05-24T01:02:47.540Z
Learning: In this repository’s JUnit 5 tests, treat CRUD/operation tags (Tag("read"), Tag("create"), Tag("update"), Tag("delete")) as optional filtering vocabulary. Only Tag("integration") and Tag("dao") are required for type/layer classification. If a test is missing one or more CRUD/operation tags, do not flag it as a required change; only enforce what CI actually checks (method/test naming conventions) and rely on Tag("integration")/Tag("dao") for the relevant classification.
Applied to files:
src/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
📚 Learning: 2026-05-24T01:02:48.955Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1832
File: src/test/java/io/github/carlos_emr/carlos/utility/CachedDateFormatsUnitTest.java:48-50
Timestamp: 2026-05-24T01:02:48.955Z
Learning: For this repo’s Java test code (src/test/java/**), do not require CRUD/operation tag annotations like Tag("read") or Tag("write") on test classes or test methods. Treat those CRUD/operation tags as optional filtering vocabulary only. For layer/type distinction, only Tag("integration") and Tag("dao") are required. Code review should avoid flagging missing Tag("read"/"write") (or similar CRUD operation tags) when tests otherwise follow the CI-enforced naming conventions.
Applied to files:
src/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
📚 Learning: 2026-04-25T21:36:29.428Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1967
File: src/test/java/io/github/carlos_emr/carlos/billings/ca/on/pageUtil/ViewBillingShortcutPg12ActionUnitTest.java:86-86
Timestamp: 2026-04-25T21:36:29.428Z
Learning: In tests that extend `CarlosUnitTestBase`, do not flag unused Mockito stubs in `BeforeEach` setup methods as actionable issues. `CarlosUnitTestBase` does not enable Mockito strict-stubs mode, and there is no current plan to turn it on, so unused stubs will not trigger `UnnecessaryStubbingException`.
Applied to files:
src/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
📚 Learning: 2026-04-25T21:36:34.936Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1967
File: src/test/java/io/github/carlos_emr/carlos/utility/ErrorPageLoggerUnitTest.java:15-15
Timestamp: 2026-04-25T21:36:34.936Z
Learning: In this repo’s unit test files, there is an intentional copy-paste-friendly convention for inner test helpers (e.g., a `private static final class CapturingAppender` extending `AbstractAppender`): it may use fully-qualified types like `java.util.List<LogEvent>` and `new java.util.ArrayList<>()` even when the outer test class already imports `List`. During code review, do not flag these as redundant-FQN or inconsistent-import nitpicks when they follow this convention for such inner appender/test helper classes.
Applied to files:
src/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
📚 Learning: 2026-05-24T01:02:48.124Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1832
File: src/test/java/io/github/carlos_emr/carlos/utility/DateUtilsFormatUnitTest.java:32-34
Timestamp: 2026-05-24T01:02:48.124Z
Learning: When reviewing this repository’s unit tests for pure static utility methods (e.g., classes like DateUtils.format, SafeEncodeUnitTest, QueryAppenderUnitTest, TextualizerUnitTest, RequestNegotiationUnitTest), do not require them to extend CarlosUnitTestBase or to include CRUD/operation-level tags. These tests are intended to use only Tag("unit") with no base class and no operation-level tags because CarlosUnitTestBase exists specifically to mock the SpringUtils/LogAction statics. Per CLAUDE.md and current CI enforcement (bdd-test-naming.yml/surefire), only method naming conventions are enforced for this category; missing operation tags and missing CarlosUnitTestBase should not be flagged. (Still flag issues if the test is not actually for pure static utility methods, or if it violates the required Tag("integration")/Tag("dao") expectations for the categories that require them.)
Applied to files:
src/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
📚 Learning: 2026-05-24T01:02:46.757Z
Learnt from: yingbull
Repo: carlos-emr/carlos PR: 1832
File: src/test/java/io/github/carlos_emr/carlos/util/DateUtilsConvertDate8CharUnitTest.java:39-42
Timestamp: 2026-05-24T01:02:46.757Z
Learning: In this repo, `CarlosUnitTestBase` is intended only for tests that need to mock the `SpringUtils` / `LogAction` statics. For unit tests that only exercise pure static methods (e.g., `DateUtils`, `SafeEncode`, `QueryAppender`, `Textualizer`, `RequestNegotiation`) and do not interact with those statics, do not extend `CarlosUnitTestBase`; keep the test self-contained to avoid unnecessary setup with no benefit.
Applied to files:
src/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java
🔇 Additional comments (2)
src/main/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsTo.java (1)
152-154: LGTM!src/test/java/io/github/carlos_emr/carlos/webserv/rest/to/model/ProviderPeriodAppsToUnitTest.java (1)
15-49: LGTM!
DragonFSKY
force-pushed
the
fix/provider-period-apps-to-string
branch
2 times, most recently
from
6b67521 to
20452ea
Compare
Signed-off-by: dragonfsky <dragonfsky@gmail.com>
DragonFSKY
force-pushed
the
fix/provider-period-apps-to-string
branch
from
20452ea to
54417c8
Compare
1 participant
Fixes #2808
Summary
ProviderPeriodAppsTo.toString()appointmentNoandproviderNo, matching the issue's proposed safe formatTests
mvn -Dtest=ProviderPeriodAppsToUnitTest testSummary by Sourcery
Redact sensitive appointment and patient details from ProviderPeriodAppsTo string representation and add regression coverage for the safe format.
Bug Fixes:
Tests:
Summary by cubic
Redacts PHI from
ProviderPeriodAppsTo.toString(), limiting output toappointmentNoandproviderNoin the formatProviderPeriodAppsTo[appointmentNo=..., providerNo=...]. Aligns with #2808 to prevent leaking patient details in logs.Written for commit 54417c8. Summary will update on new commits.