Feature/caselifecycle#58
Merged
Merged
Conversation
…pdate related services, repositories, and tests
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 25 minutes and 38 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughReplaces string statuses with a new CaseStatus enum, enforces closed-case access rules, converts delete to soft-close, records status-change audit events (entity/DTO/mapper updated), extends repositories for status filtering, updates services/controllers/templates/tests to handle closed-case listing and status transitions. Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Client/UI
participant CS as CaseService
participant CR as CaseRepository
participant AES as AuditService
participant AE as AuditEventEntity
participant DB as Database
UI->>CS: request (create/update/delete/addNote/assign/upload)
CS->>CR: fetch CaseEntity (exclude CLOSED where required)
CR-->>CS: CaseEntity
CS->>CS: determine new CaseStatus (CREATED/UPDATED/COMMUNICATION/HANDLER_ASSIGNED/CLOSED)
CS->>AE: build AuditEventEntity (caseId, actor, statusChange)
CS->>AES: record(AuditEventEntity)
AES->>DB: persist AuditEventEntity
DB-->>AES: ack
CS->>CR: save updated CaseEntity (with new status)
CR->>DB: persist CaseEntity
DB-->>CR: ack
CS-->>UI: response (DTO)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/test/java/org/example/projektarendehantering/application/service/CaseServiceTest.java (1)
262-262:⚠️ Potential issue | 🟡 MinorInconsistent verification: still checks
delete()but service now uses soft-delete.Since
deleteCasenow sets status toCLOSEDand callssave()instead ofdelete(), verifyingdelete()was never called will always pass regardless of authorization logic. Consider verifyingsave()was never called or that status remains unchanged.🔧 Suggested fix
- verify(caseRepository, never()).delete(any(CaseEntity.class)); + verify(caseRepository, never()).save(any(CaseEntity.class)); + assertThat(caseEntity.getStatus()).isNotEqualTo(CaseStatus.CLOSED);The same issue applies to lines 273 and 285.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/projektarendehantering/application/service/CaseServiceTest.java` at line 262, The test's verification is outdated: CaseService.deleteCase now performs a soft-delete by setting CaseEntity status to CLOSED and calling caseRepository.save(...) instead of caseRepository.delete(...); update the assertions in CaseServiceTest (those around deleteCase and the checks at the locations mentioned) to verify that caseRepository.save(...) was/was not called as appropriate and that the CaseEntity's status field was changed to CLOSED (or left unchanged for unauthorized cases) rather than asserting delete(...) was never invoked; reference the deleteCase method, CaseService, caseRepository, save(), delete(), and the CaseEntity.status/CLOSED constant when making these changes.src/main/java/org/example/projektarendehantering/application/service/CaseService.java (1)
126-134:⚠️ Potential issue | 🟡 Minor
deleteCaseallows closing an already-closed case, generating a redundant audit.Unlike other methods that filter out
CaseStatus.CLOSED,deleteCasedoesn't check if the case is already closed. This allows recording a "CLOSED -> CLOSED" audit event, which may be undesirable.🔧 Suggested fix
`@Transactional` public void deleteCase(Actor actor, UUID caseId) { CaseEntity entity = caseRepository.findById(caseId) + .filter(ce -> ce.getStatus() != CaseStatus.CLOSED) .orElseThrow(() -> new ResponseStatusException(HttpStatus.NOT_FOUND, "Case not found")); requireCanDelete(actor, entity);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/CaseService.java` around lines 126 - 134, The deleteCase method currently closes a case unconditionally causing redundant CLOSED->CLOSED audits; update deleteCase to check entity.getStatus() and if it's already CaseStatus.CLOSED either return immediately (no-op) or throw a 409/appropriate ResponseStatusException, skipping caseRepository.save(entity) and recordStatusChange; keep the existing requireCanDelete(actor, entity) call but perform the CLOSED check before setting status and recording the change to prevent recording a CLOSED->CLOSED audit event.
🧹 Nitpick comments (2)
src/main/java/org/example/projektarendehantering/application/service/DocumentService.java (1)
54-56: Consider unifying closed-case behavior across document endpoints.
uploadDocument/listDocumentstreat closed cases as “not found”, while other paths throw “Case is closed”. A single policy would make client handling more predictable.Also applies to: 117-119, 184-186
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java` around lines 54 - 56, The uploadDocument and listDocuments flows use caseRepository.findById(...).filter(ce -> ce.getStatus() != CaseStatus.CLOSED).orElseThrow(() -> new BadRequestException("Case not found")) while other code paths throw a "Case is closed" error; unify them: pick a single policy (e.g. throw new BadRequestException("Case is closed") or a dedicated CaseClosedException) and apply it consistently in uploadDocument, listDocuments and the other locations that check CaseStatus (references: CaseEntity, caseRepository.findById(...).filter(...), and the methods uploadDocument and listDocuments); replace the differing orElseThrow/assert messages so every closed-case check throws the same exception type and message so clients see a predictable response.src/main/java/org/example/projektarendehantering/application/service/CaseService.java (1)
168-173: Usingpeek()with authorization check may cause confusing behavior.If a user requests cases for a patient but lacks access to some of them,
requireCanReadwill throw on the first unauthorized case, failing the entire request. Consider whether the intent is:
- All-or-nothing (current behavior): User must have access to all cases for the patient
- Filter to accessible: Return only cases the user can read
If option 2 is intended:
♻️ Alternative: filter instead of fail
`@Transactional`(readOnly = true) public List<CaseDTO> getCasesForPatient(Actor actor, UUID patientId) { return caseRepository.findAllByPatient_IdAndStatusNot(patientId, CaseStatus.CLOSED).stream() - .peek(entity -> requireCanRead(actor, entity)) + .filter(entity -> canRead(actor, entity)) .map(caseMapper::toDTO) .collect(Collectors.toList()); } + + private boolean canRead(Actor actor, CaseEntity entity) { + if (isManager(actor)) return true; + if (isDoctor(actor) && actor.userId().equals(entity.getOwnerId())) return true; + if (isNurse(actor) && actor.userId().equals(entity.getHandlerId())) return true; + return false; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/application/service/CaseService.java` around lines 168 - 173, The current getCasesForPatient uses peek(requireCanRead) which throws on the first unauthorized case (all-or-nothing); if we want to return only cases the actor can read, replace the peek with a filter that retains entities the actor may read (e.g., call a non-throwing canRead(actor, entity) predicate or wrap requireCanRead in a try/catch and return true/false) before mapping with caseMapper::toDTO; update or add a canRead helper if needed and keep caseRepository.findAllByPatient_IdAndStatusNot and caseMapper::toDTO intact.
🤖 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/projektarendehantering/application/service/DocumentService.java`:
- Around line 101-102: The status change in DocumentService currently calls
caseEntity.setStatus(CaseStatus.COMMUNICATION) and
caseRepository.save(caseEntity) without creating an audit entry; update
DocumentService to record the same audit/event used by CaseService when changing
case status (e.g., invoke the CaseService.auditStatusTransition or the central
auditEvent/createCaseHistory method) immediately after setting the status and
before/after saving so the COMMUNICATION transition is persisted to the case
history; ensure you reference and reuse the existing audit API used by
CaseService rather than duplicating logic.
---
Outside diff comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/CaseService.java`:
- Around line 126-134: The deleteCase method currently closes a case
unconditionally causing redundant CLOSED->CLOSED audits; update deleteCase to
check entity.getStatus() and if it's already CaseStatus.CLOSED either return
immediately (no-op) or throw a 409/appropriate ResponseStatusException, skipping
caseRepository.save(entity) and recordStatusChange; keep the existing
requireCanDelete(actor, entity) call but perform the CLOSED check before setting
status and recording the change to prevent recording a CLOSED->CLOSED audit
event.
In
`@src/test/java/org/example/projektarendehantering/application/service/CaseServiceTest.java`:
- Line 262: The test's verification is outdated: CaseService.deleteCase now
performs a soft-delete by setting CaseEntity status to CLOSED and calling
caseRepository.save(...) instead of caseRepository.delete(...); update the
assertions in CaseServiceTest (those around deleteCase and the checks at the
locations mentioned) to verify that caseRepository.save(...) was/was not called
as appropriate and that the CaseEntity's status field was changed to CLOSED (or
left unchanged for unauthorized cases) rather than asserting delete(...) was
never invoked; reference the deleteCase method, CaseService, caseRepository,
save(), delete(), and the CaseEntity.status/CLOSED constant when making these
changes.
---
Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/application/service/CaseService.java`:
- Around line 168-173: The current getCasesForPatient uses peek(requireCanRead)
which throws on the first unauthorized case (all-or-nothing); if we want to
return only cases the actor can read, replace the peek with a filter that
retains entities the actor may read (e.g., call a non-throwing canRead(actor,
entity) predicate or wrap requireCanRead in a try/catch and return true/false)
before mapping with caseMapper::toDTO; update or add a canRead helper if needed
and keep caseRepository.findAllByPatient_IdAndStatusNot and caseMapper::toDTO
intact.
In
`@src/main/java/org/example/projektarendehantering/application/service/DocumentService.java`:
- Around line 54-56: The uploadDocument and listDocuments flows use
caseRepository.findById(...).filter(ce -> ce.getStatus() !=
CaseStatus.CLOSED).orElseThrow(() -> new BadRequestException("Case not found"))
while other code paths throw a "Case is closed" error; unify them: pick a single
policy (e.g. throw new BadRequestException("Case is closed") or a dedicated
CaseClosedException) and apply it consistently in uploadDocument, listDocuments
and the other locations that check CaseStatus (references: CaseEntity,
caseRepository.findById(...).filter(...), and the methods uploadDocument and
listDocuments); replace the differing orElseThrow/assert messages so every
closed-case check throws the same exception type and message so clients see a
predictable response.
🪄 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: e1f52d39-18d6-4fda-9e25-dec3f545312e
📒 Files selected for processing (12)
src/main/java/org/example/projektarendehantering/application/service/AuditEventMapper.javasrc/main/java/org/example/projektarendehantering/application/service/CaseService.javasrc/main/java/org/example/projektarendehantering/application/service/DocumentService.javasrc/main/java/org/example/projektarendehantering/common/CaseStatus.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/AuditEventEntity.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/CaseEntity.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/CaseRepository.javasrc/main/java/org/example/projektarendehantering/presentation/dto/AuditEventDTO.javasrc/main/java/org/example/projektarendehantering/presentation/dto/CaseDTO.javasrc/main/resources/data.sqlsrc/test/java/org/example/projektarendehantering/application/service/CaseServiceTest.javasrc/test/resources/data-test.sql
…fety, and add audit logging for document-related case status changes
Collaborator
|
Ser riktigt bra ut, enda jag hittar är att kanske kolla detta :
|
…d-case checks, and enhance templates, controllers, and tests accordingly
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.java (1)
45-46: Add assertions for new audit behavior, not just mock wiring.Since
AuditServiceis now injected, add tests verifyingauditService.record(...)is called when status transitions toCOMMUNICATION, and not called when status is alreadyCOMMUNICATION.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.java` around lines 45 - 46, DocumentServiceTest currently only wires the injected AuditService mock but doesn't assert behavior; add two tests in DocumentServiceTest that exercise the method that changes status (the same call already used in tests) and verify auditService.record(...) is invoked exactly once when transitioning from a non-COMMUNICATION status to COMMUNICATION, and verify auditService.record(...) is not invoked when the document is already in COMMUNICATION. Use Mockito.verify(auditService, times(1)).record(...) with the expected args for the first case and Mockito.verify(auditService, never()).record(any()) for the second case (or verifyZeroInteractions/reset the mock as needed) to make the assertions precise.src/test/java/org/example/projektarendehantering/presentation/rest/CaseControllerTest.java (1)
188-210: Add a non-manager access test for/api/cases/closed.The new tests cover manager success and anonymous access, but not authenticated non-manager access (e.g., DOCTOR). Add that case to lock authorization behavior.
🧪 Suggested test addition
+ `@Test` + `@WithMockUser`(roles = "DOCTOR") + void getClosedCases_shouldBeDenied_forNonManager() throws Exception { + mockMvc.perform(get("/api/cases/closed")) + .andExpect(status().isForbidden()); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/projektarendehantering/presentation/rest/CaseControllerTest.java` around lines 188 - 210, Add a test that authenticates a non-manager user (e.g., Actor with Role.DOCTOR) and asserts that calling the controller endpoint /api/cases/closed returns a forbidden response; specifically, add a test similar to getClosedCases_shouldReturnList_forManager that stubs securityActorAdapter.currentUser() to return a DOCTOR Actor and then perform get("/api/cases/closed") expecting status().isForbidden(), ensuring CaseService.getClosedCases is not invoked (or not stubbed) in this scenario to verify authorization enforcement.src/main/java/org/example/projektarendehantering/presentation/web/UiController.java (1)
55-59: Guard the closed-cases UI route at the controller boundary.Line 55 should be explicitly restricted to managers so direct URL access is blocked before service invocation.
🔧 Proposed change
import org.springframework.http.HttpStatus; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.stereotype.Controller; @@ `@GetMapping`("/ui/cases/closed") + `@PreAuthorize`("hasRole('MANAGER')") public String listClosedCases(Model model) { model.addAttribute("cases", caseService.getClosedCases(securityActorAdapter.currentUser())); return "cases/closed"; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/presentation/web/UiController.java` around lines 55 - 59, The GET handler listClosedCases in UiController must be restricted to managers at the controller boundary: add a method-level security annotation (e.g. `@PreAuthorize`("hasRole('MANAGER')") or `@Secured`("ROLE_MANAGER")) to the listClosedCases method to block direct URL access before calling caseService.getClosedCases(securityActorAdapter.currentUser()); ensure method security is enabled in your security config (e.g. prePostEnabled=true or equivalent) so the annotation is enforced.src/main/java/org/example/projektarendehantering/presentation/rest/CaseController.java (1)
43-46: Add explicit controller-level authorization for closed-cases endpoint.The service layer already checks for
MANAGERrole viaisManager(actor), but adding@PreAuthorize("hasRole('MANAGER')")at the controller level provides fail-fast denial before the service is invoked and makes the authorization requirement explicit in the API contract.🔧 Proposed change
import org.springframework.http.ResponseEntity; +import org.springframework.security.access.prepost.PreAuthorize; import org.springframework.web.bind.annotation.*; @@ - `@GetMapping`("/closed") + `@GetMapping`("/closed") + `@PreAuthorize`("hasRole('MANAGER')") public ResponseEntity<List<CaseDTO>> getClosedCases() { return ResponseEntity.ok(caseService.getClosedCases(securityActorAdapter.currentUser())); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/projektarendehantering/presentation/rest/CaseController.java` around lines 43 - 46, The closed-cases endpoint lacks an explicit controller-level authorization annotation; add `@PreAuthorize`("hasRole('MANAGER')") to the getClosedCases handler (or apply the same annotation at the controller class level) so requests are fail-fast denied before calling caseService.getClosedCases and securityActorAdapter.currentUser(); target the getClosedCases method in CaseController to implement this 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/projektarendehantering/application/service/DocumentService.java`:
- Around line 53-54: The code in DocumentService currently hides closed cases by
using .filter(ce -> ce.getStatus() != CaseStatus.CLOSED).orElseThrow(() -> new
BadRequestException("Case not found")), which conflicts with other places that
return "Case is closed"; update all such locations in DocumentService so that
when a Case is found but its status is CLOSED you throw a consistent, explicit
exception/message (e.g., BadRequestException("Case is closed") or a dedicated
ClosedCaseException) instead of "Case not found". Locate the stream/filter usage
and any conditional checks that map CLOSED to a not-found error (the occurrences
around the current filter/orElseThrow and the checks used in authorization
paths) and replace the orElseThrow or conditional branch to first check presence
and then if present and status == CLOSED throw the unified "Case is closed"
exception; leave genuine not-found situations throwing "Case not found".
---
Nitpick comments:
In
`@src/main/java/org/example/projektarendehantering/presentation/rest/CaseController.java`:
- Around line 43-46: The closed-cases endpoint lacks an explicit
controller-level authorization annotation; add
`@PreAuthorize`("hasRole('MANAGER')") to the getClosedCases handler (or apply the
same annotation at the controller class level) so requests are fail-fast denied
before calling caseService.getClosedCases and
securityActorAdapter.currentUser(); target the getClosedCases method in
CaseController to implement this change.
In
`@src/main/java/org/example/projektarendehantering/presentation/web/UiController.java`:
- Around line 55-59: The GET handler listClosedCases in UiController must be
restricted to managers at the controller boundary: add a method-level security
annotation (e.g. `@PreAuthorize`("hasRole('MANAGER')") or
`@Secured`("ROLE_MANAGER")) to the listClosedCases method to block direct URL
access before calling
caseService.getClosedCases(securityActorAdapter.currentUser()); ensure method
security is enabled in your security config (e.g. prePostEnabled=true or
equivalent) so the annotation is enforced.
In
`@src/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.java`:
- Around line 45-46: DocumentServiceTest currently only wires the injected
AuditService mock but doesn't assert behavior; add two tests in
DocumentServiceTest that exercise the method that changes status (the same call
already used in tests) and verify auditService.record(...) is invoked exactly
once when transitioning from a non-COMMUNICATION status to COMMUNICATION, and
verify auditService.record(...) is not invoked when the document is already in
COMMUNICATION. Use Mockito.verify(auditService, times(1)).record(...) with the
expected args for the first case and Mockito.verify(auditService,
never()).record(any()) for the second case (or verifyZeroInteractions/reset the
mock as needed) to make the assertions precise.
In
`@src/test/java/org/example/projektarendehantering/presentation/rest/CaseControllerTest.java`:
- Around line 188-210: Add a test that authenticates a non-manager user (e.g.,
Actor with Role.DOCTOR) and asserts that calling the controller endpoint
/api/cases/closed returns a forbidden response; specifically, add a test similar
to getClosedCases_shouldReturnList_forManager that stubs
securityActorAdapter.currentUser() to return a DOCTOR Actor and then perform
get("/api/cases/closed") expecting status().isForbidden(), ensuring
CaseService.getClosedCases is not invoked (or not stubbed) in this scenario to
verify authorization enforcement.
🪄 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: be8befef-589a-44a0-a868-bf7627bb7e56
📒 Files selected for processing (10)
src/main/java/org/example/projektarendehantering/application/service/CaseService.javasrc/main/java/org/example/projektarendehantering/application/service/DocumentService.javasrc/main/java/org/example/projektarendehantering/infrastructure/persistence/CaseRepository.javasrc/main/java/org/example/projektarendehantering/presentation/rest/CaseController.javasrc/main/java/org/example/projektarendehantering/presentation/web/UiController.javasrc/main/resources/templates/cases/closed.htmlsrc/main/resources/templates/cases/list.htmlsrc/test/java/org/example/projektarendehantering/application/service/CaseServiceTest.javasrc/test/java/org/example/projektarendehantering/application/service/DocumentServiceTest.javasrc/test/java/org/example/projektarendehantering/presentation/rest/CaseControllerTest.java
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/templates/cases/closed.html
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/org/example/projektarendehantering/application/service/CaseServiceTest.java
- src/main/java/org/example/projektarendehantering/application/service/CaseService.java
…ed-case handling across services and tests
This was referenced Apr 16, 2026
Closed
Merged
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.
closes # 16
Summary by CodeRabbit
New Features
Bug Fixes