Add MedicalRecordController for managing medical record operations#71
Merged
annikaholmqvist94 merged 4 commits intomainfrom Mar 31, 2026
Merged
Add MedicalRecordController for managing medical record operations#71annikaholmqvist94 merged 4 commits intomainfrom
MedicalRecordController for managing medical record operations#71annikaholmqvist94 merged 4 commits intomainfrom
Conversation
- Introduced endpoints for creating, updating, viewing, assigning, and closing medical records. - Enforced role- and policy-based validations via `MedicalRecordPolicy`. - Updated `MedicalRecordService` to support controller operations, including ownership and clinic-based constraints. Closes #8
annikaholmqvist94
requested review from
TatjanaTrajkovic,
johanbriger and
lindaeskilsson
📝 WalkthroughWalkthroughAdds a new Spring REST controller Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant Controller as MedicalRecordController
participant Service as MedicalRecordService
participant PetRepo as PetRepository
participant ClinicRepo as ClinicRepository
participant Policy as MedicalRecordPolicy
participant Repo as MedicalRecordRepository
Client->>Controller: POST /api/medical-records (CreateMedicalRecordRequest)
Controller->>Service: create(title, desc, petId, clinicId, currentUser)
Service->>PetRepo: findById(petId)
PetRepo-->>Service: Pet / not found
Service->>ClinicRepo: findById(clinicId)
ClinicRepo-->>Service: Clinic / not found
Service->>Policy: canCreate(currentUser, pet, clinic)
Policy-->>Service: allowed/forbidden
Service->>Repo: save(new MedicalRecord owner=pet.owner createdBy=currentUser status=OPEN)
Repo-->>Service: saved MedicalRecord
Service-->>Controller: MedicalRecord
Controller-->>Client: 201 Created (MedicalRecordResponse)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/main/java/org/example/vet1177/services/MedicalRecordService.java (1)
144-146: Avoid exposingsave(...)as a public escape hatch.This just mirrors the repository and lets other callers bypass the rule-specific entry points you added here (
create,update,updateStatus,close). Unless there is a concrete caller that cannot use those APIs, I'd remove it or narrow the visibility.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/MedicalRecordService.java` around lines 144 - 146, Remove or restrict the public save(...) escape hatch: either delete the MedicalRecordService.save method or reduce its visibility from public to private/protected so callers cannot bypass the rule-enforcing entry points; ensure all existing call sites use the service methods create, update, updateStatus, or close instead of calling medicalRecordRepository.save directly, and if any legitimate external caller cannot use those APIs, add a narrowly-scoped service method that enforces the same validation/authorization before delegating to medicalRecordRepository.save.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/vet1177/controllers/MedicalRecordController.java`:
- Around line 69-85: The controller currently only exposes getMyRecords for the
authenticated owner; add a new handler in MedicalRecordController (e.g.,
getRecordsByOwner or getByOwnerEndpoint) mapped to GET "/owner/{ownerId}" that
calls medicalRecordService.getByOwner(ownerId) and returns the mapped
List<MedicalRecordSummaryResponse>; enforce access rules so that ADMIN and VET
roles can access any ownerId while OWNER can only access when ownerId equals
currentUser.getId() (throw ForbiddenException otherwise), and annotate it with
`@Transactional`(readOnly = true) and `@GetMapping`("/owner/{ownerId}") to satisfy
the required API.
- Around line 169-176: The controller currently fetches any User via
userService.getById and relies on medicalRecordPolicy.canAssignVet only for
access checks, while medicalRecordService.assignVet unconditionally sets status
to IN_PROGRESS allowing non-VET assignees and reopening CLOSED records; update
the domain/service layer (MedicalRecordService.assignVet) to validate that the
assignee has role VET (reject otherwise) and to prevent changing a record whose
status is FINAL/CLOSED, throwing a domain exception; ensure
MedicalRecordController still calls medicalRecordPolicy.canAssignVet for
authorization but remove responsibility for role/status validation from the
controller and centralize checks in MedicalRecordService.assignVet (and related
domain model) before persisting.
- Around line 94-102: The current code authorizes using records.get(0) which is
unsafe; change to authorize per-record or query only records the caller may see:
call medicalRecordPolicy.canView (or an equivalent boolean check) for each
MedicalRecord returned by medicalRecordService.getByPet(petId), filter the
records list to only those the policy allows, then map the filtered list to
MedicalRecordSummaryResponse.from and return that; alternatively, add a service
method (e.g., medicalRecordService.getByPetAllowedForUser(petId, currentUser))
that returns only permitted records and use that instead, and ensure you still
perform an explicit pet-level access check if an empty result should not bypass
authorization.
---
Nitpick comments:
In `@src/main/java/org/example/vet1177/services/MedicalRecordService.java`:
- Around line 144-146: Remove or restrict the public save(...) escape hatch:
either delete the MedicalRecordService.save method or reduce its visibility from
public to private/protected so callers cannot bypass the rule-enforcing entry
points; ensure all existing call sites use the service methods create, update,
updateStatus, or close instead of calling medicalRecordRepository.save directly,
and if any legitimate external caller cannot use those APIs, add a
narrowly-scoped service method that enforces the same validation/authorization
before delegating to medicalRecordRepository.save.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 494304b2-8286-4820-bc71-67df2dbd29db
📒 Files selected for processing (2)
src/main/java/org/example/vet1177/controllers/MedicalRecordController.javasrc/main/java/org/example/vet1177/services/MedicalRecordService.java
src/main/java/org/example/vet1177/controllers/MedicalRecordController.java
Show resolved
Hide resolved
src/main/java/org/example/vet1177/controllers/MedicalRecordController.java
Outdated
Show resolved
Hide resolved
Comment on lines
+169
to
+176
| MedicalRecord record = medicalRecordService.getById(id); | ||
| User vetToAssign = userService.getById(request.vetId()); | ||
| medicalRecordPolicy.canAssignVet(currentUser, record, vetToAssign); | ||
|
|
||
| return ResponseEntity.ok( | ||
| MedicalRecordResponse.from( | ||
| medicalRecordService.assignVet(id, vetToAssign, currentUser) | ||
| ) |
There was a problem hiding this comment.
assign-vet can persist an invalid assignee and reopen closed records.
userService.getById(request.vetId()) accepts any User, while the provided MedicalRecordPolicy.canAssignVet(...) only checks access rules. Combined with MedicalRecordService.assignVet(...) always forcing IN_PROGRESS, this route can assign a non-vet and mutate a final record back out of CLOSED. Reject non-VET assignees and final records in the domain layer before saving.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/vet1177/controllers/MedicalRecordController.java`
around lines 169 - 176, The controller currently fetches any User via
userService.getById and relies on medicalRecordPolicy.canAssignVet only for
access checks, while medicalRecordService.assignVet unconditionally sets status
to IN_PROGRESS allowing non-VET assignees and reopening CLOSED records; update
the domain/service layer (MedicalRecordService.assignVet) to validate that the
assignee has role VET (reject otherwise) and to prevent changing a record whose
status is FINAL/CLOSED, throwing a domain exception; ensure
MedicalRecordController still calls medicalRecordPolicy.canAssignVet for
authorization but remove responsibility for role/status validation from the
controller and centralize checks in MedicalRecordService.assignVet (and related
domain model) before persisting.
…nment - Implement `isAllowed` method in `MedicalRecordPolicy` for user-role validation. - Introduce `getByOwner` endpoint in `MedicalRecordController` with ownership-specific constraints. - Update `getByPet` to filter records based on user permissions. - Enhance validation for vet assignment to finalized records. Closes #8
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/main/java/org/example/vet1177/controllers/MedicalRecordController.java (3)
37-55: Consider returning201 Createdfor POST.For REST semantics, resource creation typically returns
201 Createdrather than200 OK.♻️ Proposed fix
- return ResponseEntity.ok( + return ResponseEntity.status(HttpStatus.CREATED).body( MedicalRecordResponse.from( medicalRecordService.create(Add import:
import org.springframework.http.HttpStatus;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/controllers/MedicalRecordController.java` around lines 37 - 55, The create method in MedicalRecordController currently returns 200 OK; change it to return 201 Created by using ResponseEntity.status(HttpStatus.CREATED).body(...) instead of ResponseEntity.ok(...), and add the import for org.springframework.http.HttpStatus; update the create(...) return to wrap MedicalRecordResponse.from(...) with that status builder to follow REST semantics.
237-237: Add trailing newline at end of file.The file is missing a trailing newline, which is a common convention for text files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/controllers/MedicalRecordController.java` at line 237, The file MedicalRecordController (class MedicalRecordController) is missing a trailing newline at EOF; open the file and ensure there is a single newline character after the final closing brace (}) so the file ends with a newline, save the file and run your formatter or git pre-commit hook to verify the trailing newline is present.
4-11: Remove duplicate import.
org.example.vet1177.entities.*is imported twice (lines 4 and 11).🧹 Proposed fix
import jakarta.validation.Valid; import org.example.vet1177.entities.*; import org.example.vet1177.policy.MedicalRecordPolicy; import org.example.vet1177.services.MedicalRecordService; import org.example.vet1177.services.UserService; import org.springframework.http.ResponseEntity; import org.example.vet1177.dto.request.medicalrecord.*; import org.example.vet1177.dto.response.medicalrecord.*; -import org.example.vet1177.entities.*; import org.example.vet1177.exception.ForbiddenException;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/controllers/MedicalRecordController.java` around lines 4 - 11, The file imports the same package twice causing a duplicate import: remove the redundant import statement for org.example.vet1177.entities.* so only a single import remains; locate the duplicate entries in MedicalRecordController imports (the two occurrences of org.example.vet1177.entities.*) and delete one of them to clean up the imports.src/main/java/org/example/vet1177/services/MedicalRecordService.java (1)
163-165: Remove the unusedsave()method.This method is not called anywhere in the codebase. All legitimate modifications go through purpose-specific methods (
create,update,assignVet,updateStatus,close) that enforce policy checks. Exposing rawsavebypasses these checks and weakens encapsulation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/MedicalRecordService.java` around lines 163 - 165, Remove the public MedicalRecord save(MedicalRecord record) method from MedicalRecordService because it is unused and bypasses policy checks; delete the entire method (the one that simply calls medicalRecordRepository.save(record)), search for any remaining references to MedicalRecordService.save and replace them with the existing intent-specific methods (create, update, assignVet, updateStatus, close) if needed, then run tests/compile to ensure no callers remain and adjust any callers to use the appropriate policy-enforcing method.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/vet1177/controllers/MedicalRecordController.java`:
- Around line 87-106: The getByOwner method in MedicalRecordController currently
only restricts OWNER role and allows VETs to see all an owner's records; update
getByOwner to enforce clinic-level access for VETs by filtering the
medicalRecordService.getByOwner(ownerId) results to only include records from
the currentUser's clinic when currentUser.getRole() == Role.VET (reuse the same
policy/logic used in getByPetAllowedForUser or call
canViewClinic/currentUser.getClinicId to determine allowed clinic and drop
records from other clinics), ensuring ADMIN remains unrestricted or handled as
before.
---
Nitpick comments:
In `@src/main/java/org/example/vet1177/controllers/MedicalRecordController.java`:
- Around line 37-55: The create method in MedicalRecordController currently
returns 200 OK; change it to return 201 Created by using
ResponseEntity.status(HttpStatus.CREATED).body(...) instead of
ResponseEntity.ok(...), and add the import for
org.springframework.http.HttpStatus; update the create(...) return to wrap
MedicalRecordResponse.from(...) with that status builder to follow REST
semantics.
- Line 237: The file MedicalRecordController (class MedicalRecordController) is
missing a trailing newline at EOF; open the file and ensure there is a single
newline character after the final closing brace (}) so the file ends with a
newline, save the file and run your formatter or git pre-commit hook to verify
the trailing newline is present.
- Around line 4-11: The file imports the same package twice causing a duplicate
import: remove the redundant import statement for org.example.vet1177.entities.*
so only a single import remains; locate the duplicate entries in
MedicalRecordController imports (the two occurrences of
org.example.vet1177.entities.*) and delete one of them to clean up the imports.
In `@src/main/java/org/example/vet1177/services/MedicalRecordService.java`:
- Around line 163-165: Remove the public MedicalRecord save(MedicalRecord
record) method from MedicalRecordService because it is unused and bypasses
policy checks; delete the entire method (the one that simply calls
medicalRecordRepository.save(record)), search for any remaining references to
MedicalRecordService.save and replace them with the existing intent-specific
methods (create, update, assignVet, updateStatus, close) if needed, then run
tests/compile to ensure no callers remain and adjust any callers to use the
appropriate policy-enforcing method.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: c6ddbcfd-9050-45b7-bc5d-b1d3acb4bac6
📒 Files selected for processing (3)
src/main/java/org/example/vet1177/controllers/MedicalRecordController.javasrc/main/java/org/example/vet1177/policy/MedicalRecordPolicy.javasrc/main/java/org/example/vet1177/services/MedicalRecordService.java
src/main/java/org/example/vet1177/controllers/MedicalRecordController.java
Outdated
Show resolved
Hide resolved
…er endpoint - Introduced `getByOwnerAllowedForUser` in `MedicalRecordService` for policy-based filtering. - Updated `MedicalRecordController` to use the new service method.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/example/vet1177/services/MedicalRecordService.java (1)
144-154:⚠️ Potential issue | 🟠 Major
updateStatusallows reopening closed records, bypassing finality.Unlike
update,assignVet, andclose, theupdateStatusmethod does not checkrecord.getStatus().isFinal(). This allows transitioning aCLOSEDrecord back toOPENorIN_PROGRESS, which may undermine the domain invariant thatCLOSEDis terminal.If reopening is intentional (e.g., admin override), consider documenting this. Otherwise, add a guard:
🛡️ Proposed fix
public MedicalRecord updateStatus(UUID recordId, RecordStatus newStatus, User updatedBy) { MedicalRecord record = getById(recordId); + if (record.getStatus().isFinal()) { + throw new BusinessRuleException("Stängda ärenden kan inte ändras"); + } record.setStatus(newStatus); record.setUpdatedBy(updatedBy);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/MedicalRecordService.java` around lines 144 - 154, The updateStatus method currently allows changing a record's status even when the existing status is final; add the same final-status guard used by update/assignVet/close to prevent reopening closed records by fetching the record via getById(UUID) and checking record.getStatus().isFinal() before applying newStatus — if final, throw an IllegalStateException (or domain-specific exception) unless an explicit override path is provided; also keep the existing closed-at handling (RecordStatus.CLOSED and setClosedAt) and then call medicalRecordRepository.save(record).
🧹 Nitpick comments (4)
src/main/java/org/example/vet1177/controllers/MedicalRecordController.java (2)
4-11: Duplicate wildcard import.
org.example.vet1177.entities.*is imported twice (lines 4 and 11). Remove the duplicate.♻️ Proposed fix
import jakarta.validation.Valid; import org.example.vet1177.entities.*; import org.example.vet1177.policy.MedicalRecordPolicy; import org.example.vet1177.services.MedicalRecordService; import org.example.vet1177.services.UserService; import org.springframework.http.ResponseEntity; import org.example.vet1177.dto.request.medicalrecord.*; import org.example.vet1177.dto.response.medicalrecord.*; -import org.example.vet1177.entities.*; import org.example.vet1177.exception.ForbiddenException;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/controllers/MedicalRecordController.java` around lines 4 - 11, Remove the duplicate wildcard import of org.example.vet1177.entities.* in MedicalRecordController.java's import block — keep a single import declaration and delete the repeated one so imports only include one org.example.vet1177.entities.* entry and no redundant duplicates.
236-236: Missing newline at end of file.Most code style conventions expect a trailing newline at EOF.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/controllers/MedicalRecordController.java` at line 236, The file ends without a trailing newline; open the MedicalRecordController class file and add a single newline character after the final closing brace of the MedicalRecordController (i.e., after the last "}" at the end of src/main/java/org/example/vet1177/controllers/MedicalRecordController.java) so the file ends with a newline to satisfy EOF style conventions.src/main/java/org/example/vet1177/services/MedicalRecordService.java (2)
93-109: In-memory filtering may not scale for large record sets.Both
getByPetAllowedForUserandgetByOwnerAllowedForUserfetch all records and filter in-memory. For pets or owners with many records, this could cause performance issues. Consider adding pagination or pushing authorization predicates to the query layer if data volume grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/MedicalRecordService.java` around lines 93 - 109, Both methods (getByPetAllowedForUser and getByOwnerAllowedForUser) currently load all records and filter in-memory; change them to avoid full-result streaming by either adding pagination (accept a Pageable parameter and call repository methods that return Page<MedicalRecord>) or by moving the authorization predicate into the query layer (create repository queries such as findByPetIdAndAllowedForUser(...) / findByOwnerIdAndAllowedForUser(...) that accept the currentUser or userId/roles and apply joins/WHERE clauses to enforce medicalRecordPolicy rules). Update the service method signatures (getByPetAllowedForUser and getByOwnerAllowedForUser) to accept Pageable or the new repository parameters and call the new repository methods instead of loading all results and filtering.
172-174: Publicsave()bypasses domain validations.This method allows direct persistence without the status checks, role validations, or policy enforcement present in other service methods. If this is for internal/test use only, consider reducing visibility or documenting the intended use case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/MedicalRecordService.java` around lines 172 - 174, The public save(MedicalRecord) method in MedicalRecordService bypasses domain validations by calling medicalRecordRepository.save(record) directly; either reduce its visibility (make it package-private or private) or make it delegate to the existing validated flow used by other service methods (perform the same status checks, role validations and policy enforcement) before calling medicalRecordRepository.save; if the method is only for tests, annotate or document it (e.g. `@VisibleForTesting`) and restrict access to test packages. Ensure you reference and reuse the same validation helpers or methods used elsewhere in MedicalRecordService so behavior remains consistent.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/main/java/org/example/vet1177/services/MedicalRecordService.java`:
- Around line 144-154: The updateStatus method currently allows changing a
record's status even when the existing status is final; add the same
final-status guard used by update/assignVet/close to prevent reopening closed
records by fetching the record via getById(UUID) and checking
record.getStatus().isFinal() before applying newStatus — if final, throw an
IllegalStateException (or domain-specific exception) unless an explicit override
path is provided; also keep the existing closed-at handling (RecordStatus.CLOSED
and setClosedAt) and then call medicalRecordRepository.save(record).
---
Nitpick comments:
In `@src/main/java/org/example/vet1177/controllers/MedicalRecordController.java`:
- Around line 4-11: Remove the duplicate wildcard import of
org.example.vet1177.entities.* in MedicalRecordController.java's import block —
keep a single import declaration and delete the repeated one so imports only
include one org.example.vet1177.entities.* entry and no redundant duplicates.
- Line 236: The file ends without a trailing newline; open the
MedicalRecordController class file and add a single newline character after the
final closing brace of the MedicalRecordController (i.e., after the last "}" at
the end of
src/main/java/org/example/vet1177/controllers/MedicalRecordController.java) so
the file ends with a newline to satisfy EOF style conventions.
In `@src/main/java/org/example/vet1177/services/MedicalRecordService.java`:
- Around line 93-109: Both methods (getByPetAllowedForUser and
getByOwnerAllowedForUser) currently load all records and filter in-memory;
change them to avoid full-result streaming by either adding pagination (accept a
Pageable parameter and call repository methods that return Page<MedicalRecord>)
or by moving the authorization predicate into the query layer (create repository
queries such as findByPetIdAndAllowedForUser(...) /
findByOwnerIdAndAllowedForUser(...) that accept the currentUser or userId/roles
and apply joins/WHERE clauses to enforce medicalRecordPolicy rules). Update the
service method signatures (getByPetAllowedForUser and getByOwnerAllowedForUser)
to accept Pageable or the new repository parameters and call the new repository
methods instead of loading all results and filtering.
- Around line 172-174: The public save(MedicalRecord) method in
MedicalRecordService bypasses domain validations by calling
medicalRecordRepository.save(record) directly; either reduce its visibility
(make it package-private or private) or make it delegate to the existing
validated flow used by other service methods (perform the same status checks,
role validations and policy enforcement) before calling
medicalRecordRepository.save; if the method is only for tests, annotate or
document it (e.g. `@VisibleForTesting`) and restrict access to test packages.
Ensure you reference and reuse the same validation helpers or methods used
elsewhere in MedicalRecordService so behavior remains consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 94e94b95-c031-4f25-92ab-6634f3d219e2
📒 Files selected for processing (2)
src/main/java/org/example/vet1177/controllers/MedicalRecordController.javasrc/main/java/org/example/vet1177/services/MedicalRecordService.java
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/org/example/vet1177/services/MedicalRecordService.java (3)
147-148: Minor formatting inconsistency.The closing brace is on the same line as the throw statement, inconsistent with similar blocks elsewhere in this file (e.g., lines 129-131).
✨ Suggested fix
- if (record.getStatus().isFinal()) { - throw new BusinessRuleException("Stängda ärenden kan inte ändras");} + if (record.getStatus().isFinal()) { + throw new BusinessRuleException("Stängda ärenden kan inte ändras"); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/MedicalRecordService.java` around lines 147 - 148, The throw statement formatting in MedicalRecordService (the if block checking record.getStatus().isFinal()) has the closing brace on the same line as the throw; update the block so the closing brace is on its own line to match the file's style (e.g., change "throw new BusinessRuleException(...");}" to a separate closing brace line) so the if block in the MedicalRecordService method aligns with other similar blocks.
174-177: Consider restricting access to rawsave()method.This method bypasses all business rule validations (
isFinal()checks, policy enforcement). While it may be intentional for internal use cases, exposing it aspublicallows callers to circumvent validations enforced byupdate(),assignVet(),updateStatus(), andclose().Consider either:
- Restricting visibility to package-private if only used internally
- Adding Javadoc documenting the intended use case and warning against misuse
📝 Option 1: Restrict visibility
- public MedicalRecord save(MedicalRecord record) { + MedicalRecord save(MedicalRecord record) { return medicalRecordRepository.save(record); }📝 Option 2: Add documentation
+ /** + * Raw save bypassing business rules. Use with caution. + * Intended for batch operations or migrations only. + */ public MedicalRecord save(MedicalRecord record) { return medicalRecordRepository.save(record); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/MedicalRecordService.java` around lines 174 - 177, The public save(MedicalRecord record) method in MedicalRecordService lets callers bypass validations (isFinal(), policy checks enforced by update(), assignVet(), updateStatus(), close()) — change its visibility to package-private if it is only for internal use, or if it must remain callable externally add a clear Javadoc on MedicalRecordService.save(MedicalRecord) documenting intended internal-only use and warning that callers should use update(), assignVet(), updateStatus(), or close() to enforce business rules; ensure references to medicalRecordRepository.save(...) remain reachable from within the class after changing visibility.
93-109: LGTM, with a note on scaling.The in-memory filtering with
isAllowed()is correct and readable. For typical use cases where records per pet/owner are bounded, this performs well.If record volumes grow significantly, consider pushing role-based filtering to the repository layer with custom queries (e.g.,
findByPetIdAndClinicIdfor VET users) to reduce memory overhead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/vet1177/services/MedicalRecordService.java` around lines 93 - 109, Current in-memory filtering in getByPetAllowedForUser and getByOwnerAllowedForUser uses medicalRecordRepository.findByPetId and findByOwnerId then filters with medicalRecordPolicy.isAllowed, which is fine for small volumes but will not scale; update these methods to push role/clinic-based constraints into the repository layer by adding new query methods (e.g., findByPetIdAndClinicId, findByOwnerIdAndClinicId or a custom criteria/specification) and call the appropriate repository method based on currentUser’s role/clinic before applying medicalRecordPolicy.isAllowed so you only load allowed candidate records; ensure you reference getByPetAllowedForUser, getByOwnerAllowedForUser, medicalRecordRepository.findByPetId/findByOwnerId and medicalRecordPolicy.isAllowed when implementing the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/vet1177/services/MedicalRecordService.java`:
- Around line 126-142: The assignVet method currently checks role but not
clinic; before setting assignedVet in assignVet(UUID recordId, User vetToAssign,
User updatedBy) fetch record via getById(recordId) and validate that vetToAssign
belongs to the same clinic as the record (compare record.getClinic() or clinicId
fields on record and vetToAssign), throwing a BusinessRuleException with a clear
message if they differ or if clinic info is missing; perform this clinic check
immediately after the Role.VET check and before modifying/saving the record to
preserve clinic isolation enforced elsewhere (see
MedicalRecordPolicy.canCreate).
---
Nitpick comments:
In `@src/main/java/org/example/vet1177/services/MedicalRecordService.java`:
- Around line 147-148: The throw statement formatting in MedicalRecordService
(the if block checking record.getStatus().isFinal()) has the closing brace on
the same line as the throw; update the block so the closing brace is on its own
line to match the file's style (e.g., change "throw new
BusinessRuleException(...");}" to a separate closing brace line) so the if block
in the MedicalRecordService method aligns with other similar blocks.
- Around line 174-177: The public save(MedicalRecord record) method in
MedicalRecordService lets callers bypass validations (isFinal(), policy checks
enforced by update(), assignVet(), updateStatus(), close()) — change its
visibility to package-private if it is only for internal use, or if it must
remain callable externally add a clear Javadoc on
MedicalRecordService.save(MedicalRecord) documenting intended internal-only use
and warning that callers should use update(), assignVet(), updateStatus(), or
close() to enforce business rules; ensure references to
medicalRecordRepository.save(...) remain reachable from within the class after
changing visibility.
- Around line 93-109: Current in-memory filtering in getByPetAllowedForUser and
getByOwnerAllowedForUser uses medicalRecordRepository.findByPetId and
findByOwnerId then filters with medicalRecordPolicy.isAllowed, which is fine for
small volumes but will not scale; update these methods to push role/clinic-based
constraints into the repository layer by adding new query methods (e.g.,
findByPetIdAndClinicId, findByOwnerIdAndClinicId or a custom
criteria/specification) and call the appropriate repository method based on
currentUser’s role/clinic before applying medicalRecordPolicy.isAllowed so you
only load allowed candidate records; ensure you reference
getByPetAllowedForUser, getByOwnerAllowedForUser,
medicalRecordRepository.findByPetId/findByOwnerId and
medicalRecordPolicy.isAllowed when implementing the change.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 38c89ad5-e62d-4504-b918-73809146f34f
📒 Files selected for processing (1)
src/main/java/org/example/vet1177/services/MedicalRecordService.java
Comment on lines
+126
to
142
| public MedicalRecord assignVet(UUID recordId, User vetToAssign, User updatedBy) { | ||
| MedicalRecord record = getById(recordId); | ||
| record.setAssignedVet(vet); | ||
| if (record.getStatus().isFinal()) { | ||
| throw new BusinessRuleException( | ||
| "Kan inte tilldela handläggare till ett stängt ärende"); | ||
| } | ||
|
|
||
| if (vetToAssign.getRole() != Role.VET) { | ||
| throw new BusinessRuleException( | ||
| "Endast veterinärer kan tilldelas som handläggare"); | ||
| } | ||
|
|
||
| record.setAssignedVet(vetToAssign); | ||
| record.setStatus(RecordStatus.IN_PROGRESS); | ||
| record.setUpdatedBy(updatedBy); | ||
| return medicalRecordRepository.save(record); | ||
| } |
There was a problem hiding this comment.
Missing clinic validation for vet assignment.
The assignVet() method validates that the user has the VET role but does not verify that the vet belongs to the same clinic as the record. This could allow assigning a vet from clinic A to a record belonging to clinic B, breaking the multi-tenant clinic isolation enforced elsewhere (see canCreate() in MedicalRecordPolicy.java lines 33-35).
🛡️ Proposed fix to add clinic validation
public MedicalRecord assignVet(UUID recordId, User vetToAssign, User updatedBy) {
MedicalRecord record = getById(recordId);
if (record.getStatus().isFinal()) {
throw new BusinessRuleException(
"Kan inte tilldela handläggare till ett stängt ärende");
}
if (vetToAssign.getRole() != Role.VET) {
throw new BusinessRuleException(
"Endast veterinärer kan tilldelas som handläggare");
}
+ if (vetToAssign.getClinic() == null ||
+ !vetToAssign.getClinic().getId().equals(record.getClinic().getId())) {
+ throw new BusinessRuleException(
+ "Veterinären måste tillhöra samma klinik som ärendet");
+ }
+
record.setAssignedVet(vetToAssign);
record.setStatus(RecordStatus.IN_PROGRESS);
record.setUpdatedBy(updatedBy);
return medicalRecordRepository.save(record);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public MedicalRecord assignVet(UUID recordId, User vetToAssign, User updatedBy) { | |
| MedicalRecord record = getById(recordId); | |
| record.setAssignedVet(vet); | |
| if (record.getStatus().isFinal()) { | |
| throw new BusinessRuleException( | |
| "Kan inte tilldela handläggare till ett stängt ärende"); | |
| } | |
| if (vetToAssign.getRole() != Role.VET) { | |
| throw new BusinessRuleException( | |
| "Endast veterinärer kan tilldelas som handläggare"); | |
| } | |
| record.setAssignedVet(vetToAssign); | |
| record.setStatus(RecordStatus.IN_PROGRESS); | |
| record.setUpdatedBy(updatedBy); | |
| return medicalRecordRepository.save(record); | |
| } | |
| public MedicalRecord assignVet(UUID recordId, User vetToAssign, User updatedBy) { | |
| MedicalRecord record = getById(recordId); | |
| if (record.getStatus().isFinal()) { | |
| throw new BusinessRuleException( | |
| "Kan inte tilldela handläggare till ett stängt ärende"); | |
| } | |
| if (vetToAssign.getRole() != Role.VET) { | |
| throw new BusinessRuleException( | |
| "Endast veterinärer kan tilldelas som handläggare"); | |
| } | |
| if (vetToAssign.getClinic() == null || | |
| !vetToAssign.getClinic().getId().equals(record.getClinic().getId())) { | |
| throw new BusinessRuleException( | |
| "Veterinären måste tillhöra samma klinik som ärendet"); | |
| } | |
| record.setAssignedVet(vetToAssign); | |
| record.setStatus(RecordStatus.IN_PROGRESS); | |
| record.setUpdatedBy(updatedBy); | |
| return medicalRecordRepository.save(record); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/org/example/vet1177/services/MedicalRecordService.java` around
lines 126 - 142, The assignVet method currently checks role but not clinic;
before setting assignedVet in assignVet(UUID recordId, User vetToAssign, User
updatedBy) fetch record via getById(recordId) and validate that vetToAssign
belongs to the same clinic as the record (compare record.getClinic() or clinicId
fields on record and vetToAssign), throwing a BusinessRuleException with a clear
message if they differ or if clinic info is missing; perform this clinic check
immediately after the Role.VET check and before modifying/saving the record to
preserve clinic isolation enforced elsewhere (see
MedicalRecordPolicy.canCreate).
johanbriger
approved these changes
Mar 31, 2026
This was referenced Mar 31, 2026
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
2 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.
MedicalRecordPolicy.MedicalRecordServiceto support controller operations, including ownership and clinic-based constraints.Closes #8
Summary by CodeRabbit
New Features
Security / Behavior