Feature/extend lifecycle for staff - #62
Merged
Merged
Conversation
…th update and list conversion methods, introduce `getStaffById` in `StaffService`, and add `@Repository` annotations for repositories.
…, and expand `StaffService` with staff update and retrieval logic
… enhance `StaffService` with delete and filter methods, and update `StaffRepository` with role and department-based queries.
…te, delete, and filtered retrieval; enhance `StaffService` with additional validations and logic; introduce `StaffServiceTest` for comprehensive unit testing.
|
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 48 minutes and 13 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 (1)
📝 WalkthroughWalkthroughConverts staff endpoints to a REST API, implements StaffService with role-based method security and repository queries, introduces UpdateStaffDTO and mapper updates, annotates repositories with Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as "StaffController\n(/api/staff)"
participant Security as "SecurityFilter\n(SecurityConfig)"
participant Service as "StaffService"
participant Repo as "StaffRepository"
participant DB as "Database"
Client->>Security: HTTP request /api/staff...
Note right of Security: Authorization rules checked\n(e.g., /api/staff/** roles)
Security->>Controller: forward if authorized
Controller->>Service: call service method (get/update/delete/list)
Service->>Repo: query or save entity (findById/findByRole/save/delete)
Repo->>DB: execute SQL
DB-->>Repo: result
Repo-->>Service: entity/list
Service-->>Controller: DTO / status
Controller-->>Client: HTTP response (ResponseEntity)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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: 4
🧹 Nitpick comments (1)
src/main/java/org/example/cyberwatch/features/form/repository/EmploymentFormRepository.java (1)
6-10:@Repositoryis redundant here; consider removing for consistency.For
JpaRepositoryinterfaces, Spring Data already registers the bean, so this annotation/import can be omitted. It would also align with existing repository style insrc/main/java/org/example/cyberwatch/features/activitylog/repository/ActivityLogRepository.java(Line 1-10) andsrc/main/java/org/example/cyberwatch/features/comment/repository/CommentRepository.java(Line 1-10).Optional cleanup diff
import org.springframework.data.jpa.repository.JpaRepository; -import org.springframework.stereotype.Repository; @@ -@Repository public interface EmploymentFormRepository extends JpaRepository<EmploymentForm, Long> {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/features/form/repository/EmploymentFormRepository.java` around lines 6 - 10, Remove the redundant Spring `@Repository` annotation and its import from the EmploymentFormRepository interface: delete the import line for org.springframework.stereotype.Repository and remove the `@Repository` annotation above the interface declaration so the JpaRepository-based repository relies on Spring Data auto-registration (addressing EmploymentFormRepository).
🤖 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/cyberwatch/features/staff/controller/StaffController.java`:
- Around line 50-51: The TypeMismatch handler is hardcoded to enumerate
Status.values(), causing invalid-value errors from endpoints like
StaffController.getStaffByRoleOrDepartment to show ticket statuses; update
GlobalExceptionHandler.handleTypeMismatch to inspect the exception's required
type (e.g., ex.getRequiredType()) and, if it is an enum, enumerate its constants
(using getEnumConstants() and their names) to produce the correct "valid values"
list for Role or Department; ensure you null-check requiredType and fall back to
a generic message if it's not an enum.
- Line 27: The method-level `@PreAuthorize` annotations in StaffController (e.g.,
the `@PreAuthorize`("hasAnyRole('HR', 'CEO', 'CTO')") and the other role checks at
the other endpoints) conflict with the global /api/staff/** ADMIN requirement in
SecurityConfig; update the security configuration so the endpoint-level roles
are honored by either (A) relaxing the SecurityConfig rule that currently
requires hasRole("ADMIN") for /api/staff/** to include the roles used in
StaffController, or (B) removing the global /api/staff/** ADMIN gate and relying
on the controller's `@PreAuthorize` annotations; modify SecurityConfig and verify
the changes affect StaffController methods (the `@PreAuthorize` annotations) so
HR/CEO/CTO users can access the intended endpoints.
In
`@src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java`:
- Around line 51-56: When updating in StaffService (around
staffRepository.findById(...) and staffMapper.updateEntity(...)), guard against
duplicate emails by checking staffRepository.findByEmail(dto.getEmail()) before
calling staffRepository.save(existingStaff); if dto.getEmail() is non-null and
differs from existingStaff.getEmail() and findByEmail(...) returns a different
Staff (id != staffId) then throw the same IllegalStateException path you use
elsewhere (e.g., "Email already in use") so updates produce a controlled 409
instead of a DB constraint error; ensure null-safe comparisons of emails and
compare entity IDs when deciding conflict.
In
`@src/test/java/org/example/cyberwatch/features/staff/service/StaffServiceTest.java`:
- Around line 85-86: The test UpdateStaffWhenValid currently constructs
UpdateStaffDTO with phoneNumber set to null which violates the `@NotBlank`
constraint and doesn't mirror valid controller input; update the test to supply
a non-blank phone number (e.g., "070-1234567") when creating UpdateStaffDTO in
StaffServiceTest (the instance used by the updateStaffWhenValid test) so the DTO
passes validation and the "valid input" path exercised by
StaffController.updateStaff(...) is realistic.
---
Nitpick comments:
In
`@src/main/java/org/example/cyberwatch/features/form/repository/EmploymentFormRepository.java`:
- Around line 6-10: Remove the redundant Spring `@Repository` annotation and its
import from the EmploymentFormRepository interface: delete the import line for
org.springframework.stereotype.Repository and remove the `@Repository` annotation
above the interface declaration so the JpaRepository-based repository relies on
Spring Data auto-registration (addressing EmploymentFormRepository).
🪄 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: 6395eadc-e2b6-4d8d-ac0c-8183d63686dc
📒 Files selected for processing (9)
src/main/java/org/example/cyberwatch/config/SecurityConfig.javasrc/main/java/org/example/cyberwatch/features/form/repository/EmploymentFormRepository.javasrc/main/java/org/example/cyberwatch/features/staff/controller/StaffController.javasrc/main/java/org/example/cyberwatch/features/staff/model/StaffDTO.javasrc/main/java/org/example/cyberwatch/features/staff/model/StaffMapper.javasrc/main/java/org/example/cyberwatch/features/staff/model/UpdateStaffDTO.javasrc/main/java/org/example/cyberwatch/features/staff/repository/StaffRepository.javasrc/main/java/org/example/cyberwatch/features/staff/service/StaffService.javasrc/test/java/org/example/cyberwatch/features/staff/service/StaffServiceTest.java
💤 Files with no reviewable changes (1)
- src/main/java/org/example/cyberwatch/features/staff/model/StaffDTO.java
… update related tests in `StaffServiceTest`.
…rvice`, update role-based access in `SecurityConfig`, and refine exception handling for enum validation in `GlobalExceptionHandler`.
# Conflicts: # src/main/java/org/example/cyberwatch/config/SecurityConfig.java
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java (2)
44-66: Consider adding@Transactionalfor atomicity.
updateStaffperforms read → email check → update → save operations. Without@Transactional, these are separate transactions, creating a small window for race conditions (e.g., another request claims the email between check and save).Similarly,
deleteStaffdoes find → delete which should be atomic.Suggested addition
Add import:
import org.springframework.transaction.annotation.Transactional;Then annotate the methods:
`@PreAuthorize`("hasAnyRole('HR', 'ADMIN')") + `@Transactional` public StaffDTO updateStaff(Long staffId, UpdateStaffDTO dto) {`@PreAuthorize`("hasAnyRole('HR', 'ADMIN')") + `@Transactional` public void deleteStaff(Long staffId) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java` around lines 44 - 66, The updateStaff and deleteStaff flows in StaffService must be made atomic to prevent race conditions; add the import for org.springframework.transaction.annotation.Transactional and annotate the methods updateStaff(...) and deleteStaff(...) in StaffService with `@Transactional` (use default read-write for updateStaff and deleteStaff; optionally use `@Transactional`(readOnly = true) only for pure reads elsewhere), ensuring the methods execute within a single transaction so the read→check→save and find→delete sequences are atomic.
82-88: Consider usingtoDTOListfor consistency.Lines 84 and 87 use
.stream().map(staffMapper::toDto).toList(), whilegetAllStaff()on line 41 usesstaffMapper.toDTOList(). Using the mapper method consistently improves readability.Optional refactor
if (role != null) { - return staffRepository.findByRole(role) - .stream().map(staffMapper::toDto).toList(); + return staffMapper.toDTOList(staffRepository.findByRole(role)); } else if (department != null) { - return staffRepository.findByDepartment(department) - .stream().map(staffMapper::toDto).toList(); + return staffMapper.toDTOList(staffRepository.findByDepartment(department)); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java` around lines 82 - 88, Replace the inline stream mapping in the role and department branches with the mapper's bulk method for consistency: instead of using staffRepository.findByRole(role).stream().map(staffMapper::toDto).toList() and staffRepository.findByDepartment(department).stream().map(staffMapper::toDto).toList(), call staffMapper.toDTOList(...) on the returned lists (from staffRepository.findByRole and staffRepository.findByDepartment) to match the existing use of staffMapper.toDTOList() in getAllStaff().src/main/java/org/example/cyberwatch/exception/GlobalExceptionHandler.java (1)
30-47: Clean refactor for dynamic enum value enumeration.The approach correctly extracts enum constants at runtime using reflection, making the handler work for any enum type (not just
Status). This is a good improvement for maintainability.One minor consideration:
ex.getValue()could potentially benullif the binding failed before parsing (though rare). Consider null-safe handling:Optional defensive improvement
return ResponseEntity.status(HttpStatus.BAD_REQUEST).body( - Map.of("error", "Ogiltigt värde '" + ex.getValue() + Map.of("error", "Ogiltigt värde '" + (ex.getValue() != null ? ex.getValue() : "null") + "'. Giltiga värden: " + validValues) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/cyberwatch/exception/GlobalExceptionHandler.java` around lines 30 - 47, The handler handleTypeMismatch for MethodArgumentTypeMismatchException should guard against a null ex.getValue(); update both places that build the error message (inside the enum branch and the fallback) to use a null-safe string conversion (e.g., String.valueOf(ex.getValue()) or a conditional that substitutes "null" or an empty string) so the message never throws NPE or prints "null" unexpectedly; ensure you also fix the fallback Map.of construction to include the closing quotes/concatenation around the value when replacing ex.getValue() with the null-safe expression.
🤖 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/cyberwatch/features/staff/service/StaffService.java`:
- Around line 56-60: The null-safe equality check should be used to avoid a
NullPointerException when existingStaff.getEmail() is null: replace the
conditional that uses existingStaff.getEmail().equals(dto.getEmail()) with a
null-safe comparison (e.g., Objects.equals(existingStaff.getEmail(),
dto.getEmail())) inside the StaffService update flow so the subsequent
staffRepository.findByEmail(dto.getEmail()).ifPresent(...) still runs only when
emails differ; ensure you import java.util.Objects and keep the existing
staffRepository.findByEmail(...) block unchanged.
---
Nitpick comments:
In `@src/main/java/org/example/cyberwatch/exception/GlobalExceptionHandler.java`:
- Around line 30-47: The handler handleTypeMismatch for
MethodArgumentTypeMismatchException should guard against a null ex.getValue();
update both places that build the error message (inside the enum branch and the
fallback) to use a null-safe string conversion (e.g.,
String.valueOf(ex.getValue()) or a conditional that substitutes "null" or an
empty string) so the message never throws NPE or prints "null" unexpectedly;
ensure you also fix the fallback Map.of construction to include the closing
quotes/concatenation around the value when replacing ex.getValue() with the
null-safe expression.
In
`@src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java`:
- Around line 44-66: The updateStaff and deleteStaff flows in StaffService must
be made atomic to prevent race conditions; add the import for
org.springframework.transaction.annotation.Transactional and annotate the
methods updateStaff(...) and deleteStaff(...) in StaffService with
`@Transactional` (use default read-write for updateStaff and deleteStaff;
optionally use `@Transactional`(readOnly = true) only for pure reads elsewhere),
ensuring the methods execute within a single transaction so the read→check→save
and find→delete sequences are atomic.
- Around line 82-88: Replace the inline stream mapping in the role and
department branches with the mapper's bulk method for consistency: instead of
using staffRepository.findByRole(role).stream().map(staffMapper::toDto).toList()
and
staffRepository.findByDepartment(department).stream().map(staffMapper::toDto).toList(),
call staffMapper.toDTOList(...) on the returned lists (from
staffRepository.findByRole and staffRepository.findByDepartment) to match the
existing use of staffMapper.toDTOList() in getAllStaff().
🪄 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: 11295338-2382-4f34-9f0e-ec182f1a5911
📒 Files selected for processing (5)
src/main/java/org/example/cyberwatch/config/SecurityConfig.javasrc/main/java/org/example/cyberwatch/exception/GlobalExceptionHandler.javasrc/main/java/org/example/cyberwatch/features/staff/controller/StaffController.javasrc/main/java/org/example/cyberwatch/features/staff/service/StaffService.javasrc/test/java/org/example/cyberwatch/features/staff/service/StaffServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/org/example/cyberwatch/features/staff/service/StaffServiceTest.java
- src/main/java/org/example/cyberwatch/features/staff/controller/StaffController.java
… update validation.
This was referenced Apr 14, 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
1 participant
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.
This pull request introduces a complete implementation of the staff management feature, including controller, service, repository, DTOs, mapping logic, and comprehensive unit tests. It enables secure CRUD operations for staff entities, supports filtering by role or department, and enforces validation and authorization. The changes also improve code structure and maintainability by separating update and view models and adding repository annotations.
Staff Management Feature Implementation
Controller and Service Layer:
StaffControllerwith REST endpoints for retrieving, updating, deleting, and filtering staff members, secured with appropriate role-based access controls. (StaffController.java)StaffServiceto handle business logic for staff operations, including validation, exception handling, and logging. (StaffService.java)DTOs and Mapping:
UpdateStaffDTOfor validated staff updates and updatedStaffDTOto remove validation annotations, separating concerns between input validation and data transfer. (UpdateStaffDTO.java,StaffDTO.java) [1] [2]StaffMapperwith update and list mapping methods to support new service logic. (StaffMapper.java)Repository Layer:
@Repository. (StaffRepository.java)Testing:
StaffServiceTest.java)General Improvements
@Repositoryfor proper Spring management. (EmploymentFormRepository.java,StaffRepository.java) [1] [2]SecurityConfig.java,StaffDTO.java,StaffMapper.java) [1] [2] [3]Summary by CodeRabbit
New Features
Security
Tests
Bug Fixes