feature/add-caseofficer - #54
Merged
Merged
Conversation
…tication handler, add a new case officer in application runner and creation of fronted for case officer
Contributor
📝 WalkthroughWalkthroughAdds a CASE_OFFICER role, seeds two dev case-officer users, extends security and post-login routing, introduces admin assignment endpoints and a CaseOfficerController with UI fragments, paginated case queries, and access-control changes allowing assigned officers to act on cases. Changes
Sequence DiagramssequenceDiagram
rect rgba(200,200,255,0.5)
participant Admin as Admin User
participant Controller as AdminController
participant CaseRepo as CaseRecordRepository
participant UserRepo as UserRepository
participant DB as Database
end
Admin->>Controller: POST /admin/cases/assign (caseId, officerId)
Controller->>CaseRepo: findById(caseId)
CaseRepo->>DB: Query CaseRecord
DB-->>CaseRepo: CaseRecord
CaseRepo-->>Controller: CaseRecord
Controller->>UserRepo: findById(officerId)
UserRepo->>DB: Query UserEntity
DB-->>UserRepo: UserEntity (CASE_OFFICER)
UserRepo-->>Controller: UserEntity
Controller->>CaseRepo: save(updated CaseRecord with assignedUser)
CaseRepo->>DB: Persist updated CaseRecord
DB-->>CaseRepo: Confirmation
CaseRepo-->>Controller: Saved CaseRecord
Controller-->>Admin: Success response
sequenceDiagram
rect rgba(200,255,200,0.5)
participant Officer as Case Officer
participant Controller as CaseOfficerController
participant UserRepo as UserRepository
participant CaseRepo as CaseRecordRepository
participant DB as Database
end
Officer->>Controller: GET /case-officer/cases?page=0 (authenticated)
Controller->>UserRepo: findById(principal.name)
UserRepo->>DB: Query UserEntity
DB-->>UserRepo: UserEntity (CASE_OFFICER)
UserRepo-->>Controller: UserEntity with id
Controller->>CaseRepo: findByAssignedUserId(officerId, PageRequest)
CaseRepo->>DB: Query paginated CaseRecords
DB-->>CaseRepo: Page<CaseRecord>
CaseRepo-->>Controller: Page with cases
Controller-->>Officer: Render cases fragment (page, totalPages, cases)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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 |
Closed
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (9)
src/main/java/backendlab/team4you/Team4youApplication.java (1)
52-77: Dev seeding for case officers looks good; optional DRY cleanup available.Profile guard and empty-repo check keep this safe from running in prod, and seed data is consistent with existing admin/user blocks. If you want to reduce duplication, a small helper like
createUser(byte[] id, String name, String display, String rawPassword, UserRole role, String email)would collapse all five blocks nicely.♻️ Example helper extraction
+ java.util.function.BiConsumer<UserEntity, String> persist = (u, label) -> { + repository.save(u); + System.out.println("✅ " + label + " created"); + };Or a dedicated private method that takes the fields and encapsulates the
new UserEntity(...) → set* → save → logsequence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/Team4youApplication.java` around lines 52 - 77, The case-officer seeding duplicates the new/set/save/log pattern; extract a small helper (e.g., createUser or createAndSaveUser) that accepts the unique fields (Bytes.fromBase64 id, String username, String displayName, String rawPassword, UserRole role, String email), constructs the UserEntity, calls setPasswordHash(encoder.encode(...)), setRole(...), setEmail(...), calls repository.save(...) and prints the log; then replace the repeated blocks for officer1/officer2 (and other similar blocks) with calls to that helper to keep UserEntity creation, setPasswordHash, setRole, setEmail, repository.save and System.out.println centralized.src/main/resources/templates/case-officer-layout.html (2)
5-5: Unusedsecnamespace declaration.
xmlns:secis declared but nosec:*attributes are used in this template. Either drop the declaration or add the role/authority guards you need — leaving it in just adds noise.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/case-officer-layout.html` at line 5, The template currently declares the Thymeleaf Spring Security namespace via xmlns:sec but never uses any sec:* attributes, so either remove the unused xmlns:sec declaration from the root HTML element or add the appropriate Spring Security attribute(s) (e.g., use sec:authorize on the relevant elements like navigation links, buttons or sections) to enforce role/authority guards; locate the xmlns:sec declaration in the template and either delete that attribute or add sec:authorize="hasRole('ROLE_X')" / similar sec:* attributes where access control is required.
10-11: Reusingadmin.cssfrom the case-officer layout — intentional?Pulling in
/css/admin.cssfor a non-admin role works but couples the case-officer UI to styles named for a different role. If the intent is a shared layout look, consider renaming the stylesheet to something neutral (e.g.dashboard.css) or introducing a smallcase-officer.cssthat imports/extends the shared rules. Purely cosmetic, no functional impact.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/case-officer-layout.html` around lines 10 - 11, The case-officer layout currently imports /css/admin.css which couples admin-specific naming to a non-admin view; update the template (case-officer-layout.html) to use a neutral shared stylesheet name (e.g., replace the admin.css reference with dashboard.css) or create a new case-officer.css that imports or extends the shared rules from admin.css and then reference that instead (keep the existing components/form.css link unchanged); ensure the link element that currently points to @{/css/admin.css} is updated to the chosen neutral stylesheet name and verify the new stylesheet imports any shared rules from admin.css if you choose the extending approach.src/main/java/backendlab/team4you/config/CustomAuthenticationSuccessHandler.java (1)
45-62: Redirect branching works; consider flattening the role dispatch for readability.The new
ROLE_CASE_OFFICERbranch is correct and mirrors the existing admin check. The nestedif/elseinside the outerelseis a bit harder to scan — a flat role-based dispatch reads cleaner and scales better if more roles are added:♻️ Optional refactor
- var authorities = authentication.getAuthorities(); - - boolean isAdmin = authorities.stream() - .anyMatch(a -> a.getAuthority().equals("ROLE_ADMIN")); - - if (isAdmin) - getRedirectStrategy().sendRedirect(request, response, "/admin"); - else { - boolean isCaseOfficer = authorities.stream() - .anyMatch(a -> a.getAuthority().equals("ROLE_CASE_OFFICER")); - - if(isCaseOfficer) - getRedirectStrategy().sendRedirect(request, response, "/case-officer"); - else { - getRedirectStrategy().sendRedirect(request, response, "/home"); - } - } + String target = authentication.getAuthorities().stream() + .map(GrantedAuthority::getAuthority) + .map(a -> switch (a) { + case "ROLE_ADMIN" -> "/admin"; + case "ROLE_CASE_OFFICER" -> "/case-officer"; + default -> null; + }) + .filter(Objects::nonNull) + .findFirst() + .orElse("/home"); + + getRedirectStrategy().sendRedirect(request, response, target);Note: if a user ever holds both
ROLE_ADMINandROLE_CASE_OFFICER, the current code prefers admin; the snippet above depends on authority iteration order, so keep an explicit priority check if that matters.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/config/CustomAuthenticationSuccessHandler.java` around lines 45 - 62, The redirect logic in CustomAuthenticationSuccessHandler is nested and should be flattened for readability and easier extension: replace the nested if/else that computes isAdmin and isCaseOfficer with a single flat role-dispatch sequence that checks authorities (authentication.getAuthorities()) in priority order (e.g., check ROLE_ADMIN first, then ROLE_CASE_OFFICER, then default) and calls getRedirectStrategy().sendRedirect(request, response, ...) for each branch; keep the explicit priority so users with multiple roles still go to the intended route.src/main/java/backendlab/team4you/user/UserRepository.java (1)
30-30: Unbounded result set — consider capping or paginating if the officer list grows.
findByRole(UserRole)returns the entire set without aPageable. ForCASE_OFFICERlookups it's likely fine today, but if this is reused forUSERit could load every row into memory. Consider reusing the existing paginated overload, or at least documenting the intended use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/user/UserRepository.java` at line 30, The findByRole(UserRole role) method in UserRepository returns an unbounded List which can load all rows into memory; update the repository to use pagination by replacing or overloading it with a pageable signature such as Page<UserEntity> findByRole(UserRole role, Pageable pageable) (or delegate the current call to the existing paginated overload), and update any callers to pass a Pageable; alternatively, if you must keep the no-pageable method, add a Javadoc on findByRole(UserRole) clarifying it is intended only for small fixed-size roles (e.g., CASE_OFFICER) and must not be used for general USER lookups.src/main/java/backendlab/team4you/controller/AdminController.java (2)
33-33: Unused static import.
UserRole.CASE_OFFICERis referenced fully qualified at line 180 (UserRole.CASE_OFFICER), so the static import is never exercised. Either drop the import or use the unqualifiedCASE_OFFICERat the call site.♻️ Cleanup
-import static backendlab.team4you.user.UserRole.CASE_OFFICER; -or, alternatively, keep the import and use it:
- List<UserEntity> officers = userRepository.findByRole(UserRole.CASE_OFFICER); + List<UserEntity> officers = userRepository.findByRole(CASE_OFFICER);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/controller/AdminController.java` at line 33, Remove the unused static import of CASE_OFFICER in AdminController or use it at the call site; specifically either delete the line "import static backendlab.team4you.user.UserRole.CASE_OFFICER;" or change the reference to UserRole.CASE_OFFICER inside AdminController (e.g., in the method that checks roles) to the unqualified CASE_OFFICER to make the static import effective (ensure all references compile after the change).
174-188: Inconsistent page size and no deterministic ordering.
/admin/casesusesPageRequest.of(page, 10), whileCaseOfficerController.listCasesuses size5. Also, neither call specifies aSort, so page content is in whatever order the DB returns — which JPA/Hibernate does not guarantee to be stable across pages. Consider a shared constant for page size and an explicit sort (e.g.,Sort.by(Sort.Direction.DESC, "createdAt")) so pagination is deterministic and UX is consistent.♻️ Proposed fix
- Page<CaseRecord> cases = caseRecordRepository.findAll(PageRequest.of(page, 10)); + Page<CaseRecord> cases = caseRecordRepository.findAll( + PageRequest.of(page, 10, Sort.by(Sort.Direction.DESC, "createdAt")));🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/controller/AdminController.java` around lines 174 - 188, The pagination here is inconsistent and unordered: update AdminController.listCases to use a shared page-size constant (match CaseOfficerController.listCases) instead of hard-coded 10, and add an explicit Sort (e.g., Sort.by(Sort.Direction.DESC, "createdAt")) to the PageRequest (replace PageRequest.of(page, 10) with PageRequest.of(page, PAGE_SIZE, sort)); create the PAGE_SIZE constant in a common place (e.g., a controller base or config class) and change CaseOfficerController.listCases to use that same constant so both controllers use the same size and deterministic ordering for CaseRecord pages.src/main/java/backendlab/team4you/config/SecurityConfig.java (1)
42-50: Redundant matcher —/webauthn/register/**is shadowed by/webauthn/**.Line 46 already authorizes
/webauthn/**forUSER,ADMIN,CASE_OFFICER. Because Spring Security evaluates matchers in declaration order and picks the first match, the/webauthn/register/**segment in line 50 never gets evaluated (it is always matched by line 46 first). Since the role set is identical, there is no behavioral impact today, but the rule is dead code and will silently ignore any future tightening of/webauthn/register/**. Consider removing the duplicated path, or moving the more specific rule above the broader one if you ever need to diverge the two.♻️ Suggested simplification
.requestMatchers("/webauthn/**").hasAnyRole("USER", ADMIN, CASE_OFFICER) .requestMatchers("/admin/**").hasRole(ADMIN) .requestMatchers("/case-officer/**").hasRole(CASE_OFFICER) .requestMatchers("/home", "/profile/**").hasAnyRole("USER", ADMIN, CASE_OFFICER) - .requestMatchers("/add-passkey", "/webauthn/register/**").hasAnyRole("USER", ADMIN, CASE_OFFICER) + .requestMatchers("/add-passkey").hasAnyRole("USER", ADMIN, CASE_OFFICER)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/config/SecurityConfig.java` around lines 42 - 50, In SecurityConfig, the requestMatcher for "/webauthn/register/**" is redundant because the broader "/webauthn/**" rule (in the same SecurityConfig configuration) will always match first; either remove the duplicate requestMatchers("/webauthn/register/**") entry or, if you intend different rules later, move the more specific "/webauthn/register/**" requestMatcher above the "/webauthn/**" one so it is evaluated first — update the requestMatchers ordering in the SecurityConfig class accordingly to reflect the chosen approach.src/main/java/backendlab/team4you/controller/CaseOfficerController.java (1)
56-66: Remove stale commented-out delete handler or implement it.The commented-out
deleteApplication(1) referencesapplicationService, which isn't a field on this controller, (2) calls a case-delete endpoint that thecase-officer-cases.htmltemplate already wires up viahx-post, and (3) leaves dead code + a danglingPostMappingimport. The template's "Ta bort" button depends on this handler existing.I can implement this properly with a
CaseRecordRepository.deletecall plus an ownership check (officer may only delete cases assigned to them) and remove the stale imports. Want me to open an issue to track it?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/controller/CaseOfficerController.java` around lines 56 - 66, The commented-out deleteApplication handler in CaseOfficerController must be implemented (or removed) because the template expects a POST endpoint at "/case-officer/cases/delete"; implement a public method deleteApplication that accepts `@RequestParam` Long id and Model model, injects and uses CaseRecordRepository to locate the CaseRecord by id, verifies ownership by comparing the record's assigned officer (e.g., CaseRecord.getAssignedOfficer()/getOfficerId()) against the currently authenticated officer (from the controller's auth principal/session), only calls CaseRecordRepository.delete(record) if the officer owns it, sets an appropriate model attribute like "message" on success or an error message on failure, and returns the same fragment view "fragments/alert :: success"; also remove the stale commented code and any unused imports (including the dangling PostMapping import) if you choose to delete rather than implement.
🤖 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/backendlab/team4you/caserecord/CaseRecordRepository.java`:
- Line 21: The repository method findByAssignedUserId is invalid because
CaseRecord has a ManyToOne field named assignedUser (type UserEntity), not a
String assignedUserId; replace or adapt it to a correct signature: either change
to Page<CaseRecord> findByAssignedUser(UserEntity assignedUser, Pageable
pageable) to match the entity, or keep the id-based lookup by adding an explicit
`@Query` that joins on assignedUser.id (and accept Pageable), and also switch the
parameter type from PageRequest to the interface Pageable to match other repos
like UserRepository.findByRole; update any callers accordingly.
In `@src/main/java/backendlab/team4you/config/SecurityConfig.java`:
- Around line 43-44: SecurityConfig and S3Controller are inconsistent and
S3Service lacks file-level authorization: either remove the generic
"/api/files/download/**" and "/api/files/delete/**" requestMatchers in
SecurityConfig if those endpoints are unused, or implement ownership checks in
S3Service and align controller annotations so authorization is consistent.
Specifically, remove or restrict the SecurityConfig requestMatchers for
"/api/files/download/**" and "/api/files/delete/**" to match S3Controller's
`@PreAuthorize`(ADMIN) or eliminate those endpoints; alternatively add a
validation method (e.g., in CaseFileService) that verifies a file key belongs to
one of the current user's assigned cases and call that from S3Controller before
invoking S3Service so S3Service only operates on authorized keys. Ensure symbols
referenced: SecurityConfig, S3Controller, S3Service, CaseFileService and the
paths /api/files/download/**, /api/files/delete/** are updated accordingly.
In `@src/main/java/backendlab/team4you/controller/AdminController.java`:
- Line 15: The import groovy.util.logging.Slf4j is incorrect for Java; replace
it with lombok.extern.slf4j.Slf4j and keep the `@Slf4j` annotation on
AdminController so the logger is generated correctly, and add Lombok as a
project dependency in pom.xml (org.projectlombok:lombok) so the annotation
processor is available during compile; ensure the dependency is added (optional
true if desired) and that your build plugins/processors allow Lombok to run.
- Around line 190-207: Replace the generic RuntimeException in assignCase by
throwing the existing CaseRecordNotFoundException (or
ResponseStatusException(HttpStatus.NOT_FOUND,...)) when
caseRecordRepository.findById(caseId) is empty, and add a role check after
loading the officer: verify officer.getRoles() (or officer.getRole() / Role
enum) contains the CASE_OFFICER role constant before assigning; if not, throw a
UserNotFoundException or a new Forbidden/BadRequest exception with a clear
message and do not persist the assignment. Ensure you reference the assignCase
method, caseRecordRepository.findById, userRepository.findById,
CaseRecordNotFoundException (or ResponseStatusException), and the CASE_OFFICER
role in your changes.
In `@src/main/java/backendlab/team4you/controller/CaseOfficerController.java`:
- Around line 31-35: Remove the System.out.println(auth.getAuthorities()) call
from CaseOfficerController.admin and replace it with structured, leveled logging
(e.g., use the controller's logger at debug/trace level) if you need to record
authority info, or omit logging entirely to avoid leaking roles; also rename the
method admin to caseOfficer (or another descriptive name) to match the
"/case-officer" endpoint and update any callers/tests accordingly.
- Around line 37-54: The listCases method in CaseOfficerController returns only
a fragment ("fragments/case-officer-cases :: content") which breaks when the
endpoint is loaded directly in a browser; update listCases to detect the
HX-Request header (e.g., via HttpServletRequest or Spring's Header parameter for
"HX-Request") and branch: if HX-Request is present return the fragment as
currently done, otherwise return the full page view (the layout template that
includes the fragment) so direct navigation renders complete HTML; apply the
same HX-Request branching pattern to the analogous admin endpoints that
currently return fragments.
In `@src/main/resources/templates/fragments/case-officer-cases.html`:
- Around line 40-50: The button posts to an unimplemented endpoint so implement
a POST handler in CaseOfficerController (e.g.,
`@PostMapping`("/case-officer/cases/delete") public String deleteCase(...)) that
validates the authenticated officer owns/is assigned to the case (use your
ownership check method or CaseService.isAssignedToOfficer(caseId, officerId)),
performs the deletion (or safer: marks status closed/archived via
CaseService.closeCase(caseId)) and returns the same fragment HTML (so HTMX
hx-swap="outerHTML" replaces the card) or return a 403/404 on invalid access;
alternatively remove/disable the button in the fragment until this handler
exists. Ensure the handler accepts the id param matching the fragment's hx-post
and enforces authorization before mutating data.
---
Nitpick comments:
In
`@src/main/java/backendlab/team4you/config/CustomAuthenticationSuccessHandler.java`:
- Around line 45-62: The redirect logic in CustomAuthenticationSuccessHandler is
nested and should be flattened for readability and easier extension: replace the
nested if/else that computes isAdmin and isCaseOfficer with a single flat
role-dispatch sequence that checks authorities (authentication.getAuthorities())
in priority order (e.g., check ROLE_ADMIN first, then ROLE_CASE_OFFICER, then
default) and calls getRedirectStrategy().sendRedirect(request, response, ...)
for each branch; keep the explicit priority so users with multiple roles still
go to the intended route.
In `@src/main/java/backendlab/team4you/config/SecurityConfig.java`:
- Around line 42-50: In SecurityConfig, the requestMatcher for
"/webauthn/register/**" is redundant because the broader "/webauthn/**" rule (in
the same SecurityConfig configuration) will always match first; either remove
the duplicate requestMatchers("/webauthn/register/**") entry or, if you intend
different rules later, move the more specific "/webauthn/register/**"
requestMatcher above the "/webauthn/**" one so it is evaluated first — update
the requestMatchers ordering in the SecurityConfig class accordingly to reflect
the chosen approach.
In `@src/main/java/backendlab/team4you/controller/AdminController.java`:
- Line 33: Remove the unused static import of CASE_OFFICER in AdminController or
use it at the call site; specifically either delete the line "import static
backendlab.team4you.user.UserRole.CASE_OFFICER;" or change the reference to
UserRole.CASE_OFFICER inside AdminController (e.g., in the method that checks
roles) to the unqualified CASE_OFFICER to make the static import effective
(ensure all references compile after the change).
- Around line 174-188: The pagination here is inconsistent and unordered: update
AdminController.listCases to use a shared page-size constant (match
CaseOfficerController.listCases) instead of hard-coded 10, and add an explicit
Sort (e.g., Sort.by(Sort.Direction.DESC, "createdAt")) to the PageRequest
(replace PageRequest.of(page, 10) with PageRequest.of(page, PAGE_SIZE, sort));
create the PAGE_SIZE constant in a common place (e.g., a controller base or
config class) and change CaseOfficerController.listCases to use that same
constant so both controllers use the same size and deterministic ordering for
CaseRecord pages.
In `@src/main/java/backendlab/team4you/controller/CaseOfficerController.java`:
- Around line 56-66: The commented-out deleteApplication handler in
CaseOfficerController must be implemented (or removed) because the template
expects a POST endpoint at "/case-officer/cases/delete"; implement a public
method deleteApplication that accepts `@RequestParam` Long id and Model model,
injects and uses CaseRecordRepository to locate the CaseRecord by id, verifies
ownership by comparing the record's assigned officer (e.g.,
CaseRecord.getAssignedOfficer()/getOfficerId()) against the currently
authenticated officer (from the controller's auth principal/session), only calls
CaseRecordRepository.delete(record) if the officer owns it, sets an appropriate
model attribute like "message" on success or an error message on failure, and
returns the same fragment view "fragments/alert :: success"; also remove the
stale commented code and any unused imports (including the dangling PostMapping
import) if you choose to delete rather than implement.
In `@src/main/java/backendlab/team4you/Team4youApplication.java`:
- Around line 52-77: The case-officer seeding duplicates the new/set/save/log
pattern; extract a small helper (e.g., createUser or createAndSaveUser) that
accepts the unique fields (Bytes.fromBase64 id, String username, String
displayName, String rawPassword, UserRole role, String email), constructs the
UserEntity, calls setPasswordHash(encoder.encode(...)), setRole(...),
setEmail(...), calls repository.save(...) and prints the log; then replace the
repeated blocks for officer1/officer2 (and other similar blocks) with calls to
that helper to keep UserEntity creation, setPasswordHash, setRole, setEmail,
repository.save and System.out.println centralized.
In `@src/main/java/backendlab/team4you/user/UserRepository.java`:
- Line 30: The findByRole(UserRole role) method in UserRepository returns an
unbounded List which can load all rows into memory; update the repository to use
pagination by replacing or overloading it with a pageable signature such as
Page<UserEntity> findByRole(UserRole role, Pageable pageable) (or delegate the
current call to the existing paginated overload), and update any callers to pass
a Pageable; alternatively, if you must keep the no-pageable method, add a
Javadoc on findByRole(UserRole) clarifying it is intended only for small
fixed-size roles (e.g., CASE_OFFICER) and must not be used for general USER
lookups.
In `@src/main/resources/templates/case-officer-layout.html`:
- Line 5: The template currently declares the Thymeleaf Spring Security
namespace via xmlns:sec but never uses any sec:* attributes, so either remove
the unused xmlns:sec declaration from the root HTML element or add the
appropriate Spring Security attribute(s) (e.g., use sec:authorize on the
relevant elements like navigation links, buttons or sections) to enforce
role/authority guards; locate the xmlns:sec declaration in the template and
either delete that attribute or add sec:authorize="hasRole('ROLE_X')" / similar
sec:* attributes where access control is required.
- Around line 10-11: The case-officer layout currently imports /css/admin.css
which couples admin-specific naming to a non-admin view; update the template
(case-officer-layout.html) to use a neutral shared stylesheet name (e.g.,
replace the admin.css reference with dashboard.css) or create a new
case-officer.css that imports or extends the shared rules from admin.css and
then reference that instead (keep the existing components/form.css link
unchanged); ensure the link element that currently points to @{/css/admin.css}
is updated to the chosen neutral stylesheet name and verify the new stylesheet
imports any shared rules from admin.css if you choose the extending approach.
🪄 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: a6341a71-0f24-465a-a572-5988abd7a524
📒 Files selected for processing (13)
src/main/java/backendlab/team4you/Team4youApplication.javasrc/main/java/backendlab/team4you/caserecord/CaseRecordRepository.javasrc/main/java/backendlab/team4you/config/CustomAuthenticationSuccessHandler.javasrc/main/java/backendlab/team4you/config/SecurityConfig.javasrc/main/java/backendlab/team4you/controller/AdminController.javasrc/main/java/backendlab/team4you/controller/CaseOfficerController.javasrc/main/java/backendlab/team4you/user/UserRepository.javasrc/main/java/backendlab/team4you/user/UserRole.javasrc/main/resources/templates/case-officer-layout.htmlsrc/main/resources/templates/case-officer.htmlsrc/main/resources/templates/fragments/case-officer-cases.htmlsrc/main/resources/templates/fragments/case-officer-sidenav.htmlsrc/main/resources/templates/fragments/navbar.html
| import backendlab.team4you.user.UserRepository; | ||
| import backendlab.team4you.user.UserRole; | ||
| import backendlab.team4you.user.UserService; | ||
| import groovy.util.logging.Slf4j; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm Lombok is available and check for any other file importing the wrong Slf4j annotation.
rg -nP --type=java '^import\s+groovy\.util\.logging\.'
rg -nP 'lombok' build.gradle build.gradle.kts pom.xml 2>/dev/nullRepository: ithsjava25/project-backend-team4you
Length of output: 182
🏁 Script executed:
#!/bin/bash
# Find and examine AdminController.java to check for `@Slf4j` annotation and log.* calls
cat -n src/main/java/backendlab/team4you/controller/AdminController.java | head -40
echo "---"
echo "Searching for log. calls in AdminController:"
rg 'log\.' src/main/java/backendlab/team4you/controller/AdminController.java
echo "---"
echo "Searching for `@Slf4j` annotation in AdminController:"
rg '@Slf4j' src/main/java/backendlab/team4you/controller/AdminController.javaRepository: ithsjava25/project-backend-team4you
Length of output: 2034
🏁 Script executed:
#!/bin/bash
# Find build dependency files and check for Lombok
echo "=== Locating build files ==="
find . -maxdepth 3 -type f \( -name "pom.xml" -o -name "build.gradle*" -o -name "gradle.properties" \) ! -path './.*' | head -20
echo ""
echo "=== Checking for Lombok in dependencies ==="
find . -maxdepth 3 -type f \( -name "pom.xml" -o -name "build.gradle*" \) ! -path './.*' -exec grep -l -i "lombok" {} \;Repository: ithsjava25/project-backend-team4you
Length of output: 163
🏁 Script executed:
#!/bin/bash
# Check pom.xml for Lombok and examine dependency section
echo "=== Full pom.xml (or first 150 lines) ==="
head -150 pom.xml
echo ""
echo "=== Search for Lombok anywhere in pom.xml ==="
grep -i "lombok" pom.xml || echo "No Lombok found in pom.xml"
echo ""
echo "=== Check for all Groovy imports in codebase ==="
rg 'import groovy\.util\.logging' --type=javaRepository: ithsjava25/project-backend-team4you
Length of output: 5628
Wrong @Slf4j import — groovy.util.logging.Slf4j does nothing in Java, and Lombok is missing from dependencies.
The code uses @Slf4j (line 36) but imports Groovy's version (line 15). Groovy's @Slf4j is an AST transformation that only works in Groovy-compiled code, not Java. Additionally, Lombok is not declared as a dependency in pom.xml. This code will not compile.
Two fixes required:
- Correct the import to Lombok
- Add Lombok to
pom.xmldependencies
Required changes
-import groovy.util.logging.Slf4j;
+import lombok.extern.slf4j.Slf4j;In pom.xml, add:
<dependency>
<groupId>org.projectlombok</groupId>
<artifactId>lombok</artifactId>
<optional>true</optional>
</dependency>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/backendlab/team4you/controller/AdminController.java` at line
15, The import groovy.util.logging.Slf4j is incorrect for Java; replace it with
lombok.extern.slf4j.Slf4j and keep the `@Slf4j` annotation on AdminController so
the logger is generated correctly, and add Lombok as a project dependency in
pom.xml (org.projectlombok:lombok) so the annotation processor is available
during compile; ensure the dependency is added (optional true if desired) and
that your build plugins/processors allow Lombok to run.
…case instead of delete it
…pdated security configurations * **Access Control**: Updated `CaseFileAccessService` to validate permissions using `CaseRecord` objects instead of just IDs, and introduced `isAssignedOfficer` to permit access for officers assigned to specific records. * **Security**: Tightened `SecurityConfig` by restricting file download and deletion endpoints to the `ADMIN` role. * **Service Logic**: Modified `CaseFileService` to retrieve the `CaseRecord` before performing permission checks during file uploads. * **Error Handling**: Added a new constructor to `CaseRecordNotFoundException` to support `String` identifiers. * **Tests**: Updated `CaseFileServiceTest` to reflect the changes in method signatures and the order of operations in the upload workflow.
…ature/add-caseofficer
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/main/java/backendlab/team4you/config/SecurityConfig.java (1)
46-50: Redundant matcher:/webauthn/register/**is already covered by/webauthn/**.Since Spring Security evaluates
authorizeHttpRequestsmatchers in declaration order and uses the first match, the/webauthn/**rule on Line 46 (with the samehasAnyRole("USER", ADMIN, CASE_OFFICER)) already matches/webauthn/register/**. The second mention on Line 50 is dead configuration and can be simplified to avoid future confusion if the role set on one of the two lines diverges.♻️ Proposed simplification
- .requestMatchers("/add-passkey", "/webauthn/register/**").hasAnyRole("USER", ADMIN, CASE_OFFICER) + .requestMatchers("/add-passkey").hasAnyRole("USER", ADMIN, CASE_OFFICER)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/config/SecurityConfig.java` around lines 46 - 50, The request matcher "/webauthn/register/**" is redundant because "/webauthn/**" in SecurityConfig's authorizeHttpRequests already covers it; remove the duplicate matcher (the "/webauthn/register/**" entry in the same chain that grants hasAnyRole("USER", ADMIN, CASE_OFFICER")) so only "/webauthn/**" remains, leaving the other matchers (/webauthn/**, /admin/**, /case-officer/**, /home, /profile/**, /add-passkey) intact to avoid dead configuration and potential future divergence.src/main/java/backendlab/team4you/casefile/access/CaseFileAccessService.java (1)
33-34: Optional: use braces consistently for single-statementifblocks.The rest of this file uses braces for all
ifbodies (e.g., L21-23, L25-27, L29-31, L40-42, L44-46). The new branches drop them, which is a minor readability/maintenance risk (common source of bugs when adding a second statement later). Consider aligning with the surrounding style.♻️ Suggested tweak
- if (isAssignedOfficer(user, caseFile.getCaseRecord())) - return true; - + if (isAssignedOfficer(user, caseFile.getCaseRecord())) { + return true; + }(Apply analogously at L48-49, L63-64, and L91-92.)
Also applies to: 48-49, 63-64, 91-92
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/casefile/access/CaseFileAccessService.java` around lines 33 - 34, In CaseFileAccessService.java, make the single-statement if blocks consistent with the surrounding style by adding braces to each bare if that returns true; specifically wrap the bodies of the ifs that call isAssignedOfficer(user, caseFile.getCaseRecord()) (and the analogous bare ifs at the other occurrences) with { ... } so they match the existing braced blocks used elsewhere in the class (apply the same change to the similar bare ifs noted in the comment).src/test/java/backendlab/team4you/casefile/CaseFileServiceTest.java (1)
140-148: Optional: assert the permission check is short-circuited when the case record is missing.Since
uploadFilenow resolves theCaseRecordbefore callingcanUploadFile, addingverify(caseFileAccessService, never()).canUploadFile(any(), any(CaseRecord.class), any());(orverifyNoInteractions(caseFileAccessService)) would pin down the intended short-circuit and prevent regressions where permission checks accidentally run before existence checks.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/backendlab/team4you/casefile/CaseFileServiceTest.java` around lines 140 - 148, Add a verification to ensure permission checks are short‑circuited when the CaseRecord is missing: after the existing assertions in the test that call caseFileService.uploadFile(99L, ...), add a verify that caseFileAccessService.canUploadFile was never invoked (e.g., verify(caseFileAccessService, never()).canUploadFile(any(), any(CaseRecord.class), any()) or verifyNoInteractions(caseFileAccessService)). This targets the behavior around caseRecordRepository.findByIdWithLock returning Optional.empty() and ensures uploadFile resolves the CaseRecord before any canUploadFile check.src/main/java/backendlab/team4you/controller/AdminController.java (1)
33-35: Unused static import.
CASE_OFFICERis never referenced unqualified in this file — lines 180 and 202 both useUserRole.CASE_OFFICER. The static import and the two blank lines beneath it can be removed.Proposed cleanup
-import static backendlab.team4you.user.UserRole.CASE_OFFICER; - - `@Controller`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/controller/AdminController.java` around lines 33 - 35, Remove the unused static import of CASE_OFFICER at the top of AdminController and delete the two extra blank lines following it; leave existing qualified references (UserRole.CASE_OFFICER) unchanged so no other code changes are required.src/main/resources/templates/fragments/case-officer-cases.html (1)
16-22: Minor: status badge text isn't localized.Line 18 renders the raw enum (
OPEN,CLOSED, …) while the rest of the UI is in Swedish ("Mina ärenden", "Stäng ärende"). Also, line 21's placeholder text literally saysOPENdespite binding tocase.confidentialityLevel— slightly confusing for future maintainers reading the template. Consider mapping the enum to a localized label (e.g., via#messagesor a smallth:switch).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/fragments/case-officer-cases.html` around lines 16 - 22, The status badge currently renders the raw enum (case.status) and the confidentiality span shows a confusing placeholder; change both to use localized message keys instead of raw enum names—use the case.status.name() and case.confidentialityLevel (or a small th:switch on case.status / case.confidentialityLevel) to resolve message keys (e.g. via `#messages` like #{case.status.OPEN} / #{case.confidentialityLevel.HIGH}) so the displayed labels are localized and replace the literal placeholder text; update the th:text bindings on the status badge and confidentiality span to use these message lookups.src/main/resources/templates/fragments/case-officer-sidenav.html (1)
19-23: Minor: mixed-language labels in sidebar."Add Passkeys" is in English while sibling entries ("Hantera ärenden", "Logga ut") are in Swedish. Consider "Lägg till passnyckel" (or whatever terminology matches the rest of the app) for consistency.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/fragments/case-officer-sidenav.html` around lines 19 - 23, Replace the English label "Add Passkeys" with the Swedish equivalent used by the app to keep sidebar language consistent: update the span text inside the anchor element with th:href="@{/webauthn/register}" (the <span> currently containing "Add Passkeys") to "Lägg till passnyckel" or the app's preferred Swedish term.
🤖 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/resources/templates/fragments/case-officer-sidenav.html`:
- Around line 10-15: The hx-get attribute on the anchor is a hardcoded absolute
path and should use Thymeleaf processing so the request respects the
application's context path; replace the static hx-get="/case-officer/cases" on
the anchor element with a Thymeleaf-processed attribute (e.g., using th:attr to
set hx-get or Thymeleaf's hx:get support) so it uses the same
@{/case-officer/cases} URL resolution as th:href.
---
Nitpick comments:
In
`@src/main/java/backendlab/team4you/casefile/access/CaseFileAccessService.java`:
- Around line 33-34: In CaseFileAccessService.java, make the single-statement if
blocks consistent with the surrounding style by adding braces to each bare if
that returns true; specifically wrap the bodies of the ifs that call
isAssignedOfficer(user, caseFile.getCaseRecord()) (and the analogous bare ifs at
the other occurrences) with { ... } so they match the existing braced blocks
used elsewhere in the class (apply the same change to the similar bare ifs noted
in the comment).
In `@src/main/java/backendlab/team4you/config/SecurityConfig.java`:
- Around line 46-50: The request matcher "/webauthn/register/**" is redundant
because "/webauthn/**" in SecurityConfig's authorizeHttpRequests already covers
it; remove the duplicate matcher (the "/webauthn/register/**" entry in the same
chain that grants hasAnyRole("USER", ADMIN, CASE_OFFICER")) so only
"/webauthn/**" remains, leaving the other matchers (/webauthn/**, /admin/**,
/case-officer/**, /home, /profile/**, /add-passkey) intact to avoid dead
configuration and potential future divergence.
In `@src/main/java/backendlab/team4you/controller/AdminController.java`:
- Around line 33-35: Remove the unused static import of CASE_OFFICER at the top
of AdminController and delete the two extra blank lines following it; leave
existing qualified references (UserRole.CASE_OFFICER) unchanged so no other code
changes are required.
In `@src/main/resources/templates/fragments/case-officer-cases.html`:
- Around line 16-22: The status badge currently renders the raw enum
(case.status) and the confidentiality span shows a confusing placeholder; change
both to use localized message keys instead of raw enum names—use the
case.status.name() and case.confidentialityLevel (or a small th:switch on
case.status / case.confidentialityLevel) to resolve message keys (e.g. via
`#messages` like #{case.status.OPEN} / #{case.confidentialityLevel.HIGH}) so the
displayed labels are localized and replace the literal placeholder text; update
the th:text bindings on the status badge and confidentiality span to use these
message lookups.
In `@src/main/resources/templates/fragments/case-officer-sidenav.html`:
- Around line 19-23: Replace the English label "Add Passkeys" with the Swedish
equivalent used by the app to keep sidebar language consistent: update the span
text inside the anchor element with th:href="@{/webauthn/register}" (the <span>
currently containing "Add Passkeys") to "Lägg till passnyckel" or the app's
preferred Swedish term.
In `@src/test/java/backendlab/team4you/casefile/CaseFileServiceTest.java`:
- Around line 140-148: Add a verification to ensure permission checks are
short‑circuited when the CaseRecord is missing: after the existing assertions in
the test that call caseFileService.uploadFile(99L, ...), add a verify that
caseFileAccessService.canUploadFile was never invoked (e.g.,
verify(caseFileAccessService, never()).canUploadFile(any(),
any(CaseRecord.class), any()) or verifyNoInteractions(caseFileAccessService)).
This targets the behavior around caseRecordRepository.findByIdWithLock returning
Optional.empty() and ensures uploadFile resolves the CaseRecord before any
canUploadFile check.
🪄 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: 27344ada-0d02-4494-bfef-61a23db9850e
📒 Files selected for processing (10)
src/main/java/backendlab/team4you/casefile/CaseFileService.javasrc/main/java/backendlab/team4you/casefile/access/CaseFileAccessService.javasrc/main/java/backendlab/team4you/caserecord/CaseRecordRepository.javasrc/main/java/backendlab/team4you/config/SecurityConfig.javasrc/main/java/backendlab/team4you/controller/AdminController.javasrc/main/java/backendlab/team4you/controller/CaseOfficerController.javasrc/main/java/backendlab/team4you/exceptions/CaseRecordNotFoundException.javasrc/main/resources/templates/fragments/case-officer-cases.htmlsrc/main/resources/templates/fragments/case-officer-sidenav.htmlsrc/test/java/backendlab/team4you/casefile/CaseFileServiceTest.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/backendlab/team4you/exceptions/CaseRecordNotFoundException.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/backendlab/team4you/caserecord/CaseRecordRepository.java
- src/main/java/backendlab/team4you/controller/CaseOfficerController.java
Comment on lines
+10
to
+15
| <a th:href="@{/case-officer/cases}" | ||
| hx-get="/case-officer/cases" | ||
| hx-target="#content-area"> | ||
| <i class="fa-solid fa-file-invoice"></i> | ||
| <span>Hantera ärenden</span> | ||
| </a> |
Contributor
There was a problem hiding this comment.
Use Thymeleaf URL processing for hx-get to respect the context path.
hx-get="/case-officer/cases" is a hardcoded absolute path. If the app is ever deployed under a non-root context path (or behind a reverse-proxy prefix), this request will 404 while th:href="@{...}" on the same anchor still works. Rewrite via th:attr (or Thymeleaf's hx:get namespace if enabled) for consistency.
Suggested change
-<a th:href="@{/case-officer/cases}"
- hx-get="/case-officer/cases"
- hx-target="#content-area">
+<a th:href="@{/case-officer/cases}"
+ th:attr="hx-get=@{/case-officer/cases}"
+ hx-target="#content-area">📝 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
| <a th:href="@{/case-officer/cases}" | |
| hx-get="/case-officer/cases" | |
| hx-target="#content-area"> | |
| <i class="fa-solid fa-file-invoice"></i> | |
| <span>Hantera ärenden</span> | |
| </a> | |
| <a th:href="@{/case-officer/cases}" | |
| th:attr="hx-get=@{/case-officer/cases}" | |
| hx-target="#content-area"> | |
| <i class="fa-solid fa-file-invoice"></i> | |
| <span>Hantera ärenden</span> | |
| </a> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/templates/fragments/case-officer-sidenav.html` around
lines 10 - 15, The hx-get attribute on the anchor is a hardcoded absolute path
and should use Thymeleaf processing so the request respects the application's
context path; replace the static hx-get="/case-officer/cases" on the anchor
element with a Thymeleaf-processed attribute (e.g., using th:attr to set hx-get
or Thymeleaf's hx:get support) so it uses the same @{/case-officer/cases} URL
resolution as th:href.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/main/java/backendlab/team4you/casefile/CaseFileService.java (1)
65-74: Optional: consider validating file size before acquiring the pessimistic write lock.
findByIdWithLocktakes aPESSIMISTIC_WRITErow lock (perCaseRecordRepository.findByIdWithLock), so a too-large or unauthorized upload still acquires (and holds for the rest of the transaction) a write lock on theCaseRecord. Cheap pre-checks likefile.getSize() > MAX_FILE_SIZE_BYTEScould be moved before the locked lookup to avoid unnecessary lock acquisition for clearly invalid requests. Authorization must stay after the fetch sincecanUploadFilenow needs the entity.♻️ Proposed reordering
ConfidentialityLevel effectiveConfidentialityLevel = confidentialityLevel != null ? confidentialityLevel : ConfidentialityLevel.OPEN; + if (file.getSize() > MAX_FILE_SIZE_BYTES) { + throw new FileTooLargeException(MAX_FILE_SIZE_BYTES); + } + CaseRecord caseRecord = caseRecordRepository.findByIdWithLock(caseRecordId) .orElseThrow(() -> new CaseRecordNotFoundException(caseRecordId)); if (!caseFileAccessService.canUploadFile(actor, caseRecord, effectiveConfidentialityLevel)) { throw new AccessDeniedException("Du har inte behörighet att ladda upp denna fil."); } - - if (file.getSize() > MAX_FILE_SIZE_BYTES) { - throw new FileTooLargeException(MAX_FILE_SIZE_BYTES); - }Note: this would also require updating the file-too-large tests in
CaseFileServiceTestto drop thefindByIdWithLock/canUploadFilestubs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/casefile/CaseFileService.java` around lines 65 - 74, Move the cheap file-size pre-check before acquiring the pessimistic write lock to avoid holding CaseRecordRepository.findByIdWithLock (PESSIMISTIC_WRITE) for invalid requests: in CaseFileService, check if file.getSize() > MAX_FILE_SIZE_BYTES and throw FileTooLargeException(MAX_FILE_SIZE_BYTES) before calling findByIdWithLock, keeping the authorization check via caseFileAccessService.canUploadFile(caseRecord, ...) after the entity is fetched; update affected tests in CaseFileServiceTest to no longer stub findByIdWithLock/canUploadFile for the file-too-large scenario.src/test/java/backendlab/team4you/casefile/CaseFileServiceTest.java (1)
496-535: Optional: consolidate the twowhenFileTooLargetests.
uploadFile_shouldThrowFileTooLargeException_whenFileTooLarge(lines 498–513) anduploadFile_shouldNotAccessRepositoriesOrS3_whenFileTooLarge(lines 517–535) share the same arrange/act and only differ in the verification phase. They can be merged into a single test that asserts both the exception and the interaction expectations, reducing duplication and stub maintenance.♻️ Suggested consolidation
- `@Test` - `@DisplayName`("uploadFile should throw FileTooLargeException when file is too large") - void uploadFile_shouldThrowFileTooLargeException_whenFileTooLarge() { - MockMultipartFile file = new MockMultipartFile( - "file", - "big.pdf", - "application/pdfile", - new byte[6 * 1024 * 1024] - ); - - when(caseRecordRepository.findByIdWithLock(1L)).thenReturn(Optional.of(caseRecord)); - - when(caseFileAccessService.canUploadFile(actor, caseRecord, ConfidentialityLevel.OPEN)).thenReturn(true); - - assertThatThrownBy(() -> caseFileService.uploadFile(1L, file, ConfidentialityLevel.OPEN, actor)) - .isInstanceOf(FileTooLargeException.class) - .hasMessageContaining("Filen är för stor."); - } - `@Test` `@DisplayName`("uploadFile should not access repositories or s3 when file is too large") void uploadFile_shouldNotAccessRepositoriesOrS3_whenFileTooLarge() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/backendlab/team4you/casefile/CaseFileServiceTest.java` around lines 496 - 535, Combine the two tests uploadFile_shouldThrowFileTooLargeException_whenFileTooLarge and uploadFile_shouldNotAccessRepositoriesOrS3_whenFileTooLarge into one test that sets up the same MockMultipartFile, stubs caseRecordRepository.findByIdWithLock(1L) and caseFileAccessService.canUploadFile(...), calls caseFileService.uploadFile(1L, file, ConfidentialityLevel.OPEN, actor) inside an assertion that it throws FileTooLargeException with the expected message, and then additionally verifies interactions: verify(caseRecordRepository).findByIdWithLock(1L) and verifyNoInteractions(caseFileRepository, s3Service); keep the original method name (or rename to a consolidated name) and reuse the same actor and caseRecord fixtures to remove duplication.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/main/java/backendlab/team4you/casefile/CaseFileService.java`:
- Around line 65-74: Move the cheap file-size pre-check before acquiring the
pessimistic write lock to avoid holding CaseRecordRepository.findByIdWithLock
(PESSIMISTIC_WRITE) for invalid requests: in CaseFileService, check if
file.getSize() > MAX_FILE_SIZE_BYTES and throw
FileTooLargeException(MAX_FILE_SIZE_BYTES) before calling findByIdWithLock,
keeping the authorization check via
caseFileAccessService.canUploadFile(caseRecord, ...) after the entity is
fetched; update affected tests in CaseFileServiceTest to no longer stub
findByIdWithLock/canUploadFile for the file-too-large scenario.
In `@src/test/java/backendlab/team4you/casefile/CaseFileServiceTest.java`:
- Around line 496-535: Combine the two tests
uploadFile_shouldThrowFileTooLargeException_whenFileTooLarge and
uploadFile_shouldNotAccessRepositoriesOrS3_whenFileTooLarge into one test that
sets up the same MockMultipartFile, stubs
caseRecordRepository.findByIdWithLock(1L) and
caseFileAccessService.canUploadFile(...), calls caseFileService.uploadFile(1L,
file, ConfidentialityLevel.OPEN, actor) inside an assertion that it throws
FileTooLargeException with the expected message, and then additionally verifies
interactions: verify(caseRecordRepository).findByIdWithLock(1L) and
verifyNoInteractions(caseFileRepository, s3Service); keep the original method
name (or rename to a consolidated name) and reuse the same actor and caseRecord
fixtures to remove duplication.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 647ff5b8-9972-4673-8cdd-2f7df9724805
📒 Files selected for processing (2)
src/main/java/backendlab/team4you/casefile/CaseFileService.javasrc/test/java/backendlab/team4you/casefile/CaseFileServiceTest.java
MartinStenhagen
approved these changes
Apr 27, 2026
This was referenced Apr 27, 2026
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.
Summary by CodeRabbit
New Features
UI Updates
Bug Fixes / Access