implement workflow for EmploymentForm - #48
Merged
codebyNorthsteep merged 18 commits intoApr 7, 2026
Conversation
…ations; introduce EmploymentMapper
…EmploymentFormDTO
…mploymentMapper; add StaffMapper for staff entity conversion
…or staff entity conversion and implement error handling in createEmployee method
…ploymentForm and getPendingForms endpoints in EmploymentFormController; update EmploymentFormService to return EmploymentFormDTO and handle staff creation
…ethod and add validation for social security number
…proval logic in EmploymentFormService
…ms with the logged-in HR staff; update service method to accept HR identifier
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds an employment form workflow: new REST controller endpoints to create, approve, and list pending forms; DTOs, mappers, and service logic to create/approve forms and finalize staff; repository and entity changes (ApprovalStatus enum, unique email, Staff relations); removes HR/Management/Consultant entities; adds controller tests. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as EmploymentFormController
participant Service as EmploymentFormService
participant Mapper as EmploymentMapper
participant EmpRepo as EmploymentFormRepository
participant StaffRepo as StaffRepository
participant DB as Database
Client->>Controller: POST /api/forms/employment (CreateEmploymentDTO, Auth)
Controller->>Service: createForm(dto, loggedInHr)
Service->>EmpRepo: existsBySocialSecurityNumber(ssn)
EmpRepo->>DB: Query by SSN
DB-->>EmpRepo: result
Service->>StaffRepo: existsBySocialSecurityNumber(ssn)
StaffRepo->>DB: Query by SSN
DB-->>StaffRepo: result
Service->>StaffRepo: findByEmail(loggedInHr)
StaffRepo->>DB: Query by email
DB-->>StaffRepo: HR Staff
Service->>Mapper: toEntity(dto)
Mapper-->>Service: EmploymentForm
Service->>EmpRepo: save(form)
EmpRepo->>DB: Persist
DB-->>EmpRepo: Saved form
Service->>Mapper: toDTO(savedForm)
Mapper-->>Service: EmploymentFormDTO
Service-->>Controller: EmploymentFormDTO
Controller-->>Client: 201 Created
sequenceDiagram
participant Client
participant Controller as EmploymentFormController
participant Service as EmploymentFormService
participant Mapper as EmploymentMapper
participant EmpRepo as EmploymentFormRepository
participant StaffRepo as StaffRepository
participant DB as Database
Client->>Controller: POST /api/forms/{id}/approve
Controller->>Service: approveAndFinalizeEmployment(id)
Service->>EmpRepo: findById(id)
EmpRepo->>DB: Query by ID
DB-->>EmpRepo: EmploymentForm
Service->>Service: validate status == PENDING
Service->>Mapper: formToStaff(form)
Mapper-->>Service: Staff
Service->>StaffRepo: save(staff)
StaffRepo->>DB: Persist Staff
DB-->>StaffRepo: Saved Staff
Service->>EmpRepo: deleteById(id)
EmpRepo->>DB: Delete form
DB-->>EmpRepo: Deleted
Service-->>Controller: void
Controller-->>Client: 204 No Content
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (7)
src/main/java/org/example/cyberwatch/features/staff/model/Staff.java (1)
63-64: InitializecreatedFormseagerly.This is the only collection on
Staffthat starts asnull. Keeping it uninitialized makes transientStaffinstances behave differently fromreportFormsandassignedTickets, and simple calls likegetCreatedForms().add(...)will blow up.♻️ Proposed fix
- private Set<EmploymentForm> createdForms; + private Set<EmploymentForm> createdForms = new HashSet<>();🤖 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/model/Staff.java` around lines 63 - 64, The createdForms collection in Staff is left null causing NPEs when used on transient instances; initialize it like the other collections (reportForms, assignedTickets) by instantiating createdForms (e.g., new HashSet<>()) at declaration or in the Staff constructor and ensure its getter getCreatedForms() returns the non-null Set so calls like getCreatedForms().add(...) are safe; update the field initialization for createdForms in the Staff class accordingly.src/main/java/org/example/cyberwatch/features/staff/model/StaffMapper.java (1)
21-30:toEntityis not safe as an update mapper yet.It drops both
idandsocialSecurityNumber, so a future profile-update flow built on this will create a newStaffthat violates the entity’s required fields instead of updating the existing row. Prefer mutating an existing entity here, or map the immutable fields explicitly.♻️ Safer shape for updates
- public Staff toEntity(StaffDTO dto) { - Staff staff = new Staff(); + public void updateEntity(StaffDTO dto, Staff staff) { staff.setFirstName(dto.getFirstName()); staff.setLastName(dto.getLastName()); staff.setEmail(dto.getEmail()); staff.setPhoneNumber(dto.getPhoneNumber()); staff.setRole(dto.getRole()); staff.setDepartment(dto.getDepartment()); - return staff; }🤖 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/model/StaffMapper.java` around lines 21 - 30, The toEntity(StaffDTO dto) mapper drops immutable/required fields (id and socialSecurityNumber) which will cause updates to create invalid new Staff rows; change toEntity to either accept an existing Staff to mutate (e.g., toEntity(StaffDTO dto, Staff existing)) or, if kept as factory, explicitly copy dto.getId() and dto.getSocialSecurityNumber() into the new Staff (or validate presence) so id and socialSecurityNumber are preserved for update flows; update usages of toEntity to pass an existing Staff when performing updates and keep toEntity only for creates if you choose that split.src/test/java/org/example/cyberwatch/form/EmploymentFormControllerTests.java (1)
28-94: The security-sensitive endpoints are still untested.This suite only covers the create route, but the PR also adds manager approval, pending-form listing, and role checks. Please add happy-path plus
403coverage for those routes so access-control regressions are caught in the slice tests.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/cyberwatch/form/EmploymentFormControllerTests.java` around lines 28 - 94, Add slice tests for the newly introduced manager-approval and pending-form listing endpoints and include 403 checks for role enforcement: in EmploymentFormControllerTests, add a happy-path test that mocks employmentFormService.getPendingForms() and asserts GET "/api/forms/employment/pending" returns 200 with expected JSON (use mockMvc and objectMapper), add a happy-path test that mocks employmentFormService.approveForm(id, ...) and asserts PUT/POST to the controller's manager-approval endpoint returns 200/204 (include csrf() for non-GET), and for each endpoint add a corresponding test using `@WithMockUser` with a non-authorized role to assert status().isForbidden(); when creating these tests reference the mocked service methods employmentFormService.getPendingForms and employmentFormService.approveForm and reuse mockMvc, objectMapper, and the existing `@MockitoBean` employmentFormService to stub returns.src/main/java/org/example/cyberwatch/features/form/model/CreateEmploymentDTO.java (1)
49-52:createdDateandhrIdshould not be in a creation request DTO.These fields are server-side concerns:
createdDateis auto-generated via@CreationTimestampon the entity, andhrIdis derived from the authenticated user. Including them in the creation DTO implies clients can set them, which is misleading and could be a security concern if the mapper were to copy them.Consider removing these fields from
CreateEmploymentDTOor marking them as ignored during mapping.🤖 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/model/CreateEmploymentDTO.java` around lines 49 - 52, Remove server-controlled fields from the creation DTO: delete createdDate and hrId from CreateEmploymentDTO (or at minimum stop exposing them) and update any mapping logic that maps CreateEmploymentDTO to the Employment entity (e.g., the mapper that handles CreateEmploymentDTO -> Employment) to not copy these fields; instead let the entity populate createdDate via `@CreationTimestamp` and derive hrId from the authenticated principal when constructing the Employment entity on the server side.src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.java (1)
27-31: Consider handling nullAuthenticationparameter.If this endpoint is called without proper authentication (e.g., during testing or misconfiguration),
authentication.getName()will throw an NPE. While Spring Security should prevent unauthenticated access, defensive coding would add resilience.🤖 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/controller/EmploymentFormController.java` around lines 27 - 31, The createEmploymentForm method currently calls authentication.getName() without null-checking; update createEmploymentForm to defensively handle null Authentication (and null getName()) by checking if authentication == null || authentication.getName() == null and if so either return an appropriate 401/403 ResponseEntity (e.g., ResponseEntity.status(HttpStatus.UNAUTHORIZED).build()) or throw an AccessDeniedException, otherwise continue to call employmentFormService.createForm with the authenticated principal; ensure you reference the Authentication parameter in createEmploymentForm and keep the loggedInHr variable assignment guarded by this check.src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (1)
41-43: Mixed language comment and assumption about authentication identifier.The comment contains Swedish ("Antar att du har en sådan metod"). Also,
authentication.getName()returning an email depends on how authentication is configured—it could return a username, ID, or email. This assumption should be documented or verified.🤖 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/service/EmploymentFormService.java` around lines 41 - 43, The inline comment contains Swedish and assumes authentication.getName() is an email; replace the Swedish comment and remove the assumption, and instead explicitly verify or document what authentication.getName() returns (email vs username/ID) and use the matching repository lookup: either staffRepository.findByEmail(loggedInHr) or staffRepository.findByUsername(loggedInHr), or extract the email from authentication.getPrincipal() if needed; update the code paths around setHrId(), Staff, staffRepository.findByEmail and loggedInHr to use the correct identifier and add a brief JavaDoc/comment stating which identifier is expected by authentication configuration.src/main/java/org/example/cyberwatch/features/form/model/EmploymentFormDTO.java (1)
39-40: Remove or clarify the leftover comment.The comment
//Long when fetching the Id?appears to be a development note that should be resolved or removed before merging.🤖 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/model/EmploymentFormDTO.java` around lines 39 - 40, Remove the leftover developer comment above the hrId field in EmploymentFormDTO; either delete the line "//Long when fetching the Id?" or replace it with a clear Javadoc or inline comment that explains the purpose of the hrId field (e.g., what it represents and when it is used) so the field declaration private Long hrId; is self-explanatory.
🤖 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/form/controller/EmploymentFormController.java`:
- Around line 34-39: Update the incorrect role names in the PreAuthorize
annotations so they match the Role enum: in EmploymentFormController change
hasRole('MANAGER') to hasRole('PROJECT_MANAGER') in the
approveForm(`@PathVariable` Long id) method and likewise replace
hasRole('MANAGER') with hasRole('PROJECT_MANAGER') in the getPendingForms(...)
method (leave hasRole('HR') as-is); ensure the annotations on these methods
reference the enum value PROJECT_MANAGER exactly.
In
`@src/main/java/org/example/cyberwatch/features/form/model/CreateEmploymentDTO.java`:
- Around line 46-47: The `@NotNull` on the status field of CreateEmploymentDTO
conflicts with EmploymentFormService.createForm()'s defaulting logic; remove the
`@NotNull` annotation from the private ApprovalStatus status field in
CreateEmploymentDTO so null values can pass validation and let
EmploymentFormService.createForm() apply the PENDING default (or alternatively,
if you prefer strictness, remove the defaulting branch in
EmploymentFormService.createForm() instead—pick one approach and make the
corresponding change).
In
`@src/main/java/org/example/cyberwatch/features/form/model/EmploymentForm.java`:
- Around line 67-73: EmploymentForm now maps createdBy and approvedBy to Staff
(fields EmploymentForm.createdBy and EmploymentForm.approvedBy) which conflicts
with existing inverse mappings in HR and Management; either remove the
`@OneToMany`(mappedBy = "createdBy") from HR and `@OneToMany`(mappedBy =
"approvedBy") from Management so Staff is the sole inverse owner, or revert
EmploymentForm to reference HR and Management instead—pick one approach and make
the entity annotations consistent (EmploymentForm, Staff, HR, Management,
createdBy, approvedBy). Also add a DB migration that backfills hr_id and
approver_management_id into the new Staff FK (or vice versa depending on chosen
mapping) so existing rows map to the correct staff records (ensure the migration
copies/joins old entity IDs to the correct staff_id values and updates the
foreign key columns).
In
`@src/main/java/org/example/cyberwatch/features/form/model/EmploymentMapper.java`:
- Around line 56-62: The updateEntity(UpdateEmploymentDTO dto, EmploymentForm
entity) currently only copies status and ignores other updatable fields; modify
it to check each field on UpdateEmploymentDTO (e.g., getSsn(), getFirstName(),
getLastName(), getEmail(), getPhone(), getRole(), getDepartment(), getStatus())
and when a DTO getter is non-null (or non-empty if required) call the
corresponding EmploymentForm setter (e.g., setSsn(), setFirstName(),
setLastName(), setEmail(), setPhone(), setRole(), setDepartment(), setStatus())
so all intended updates are applied; keep the existing null guards for
dto/entity and ensure you only overwrite entity fields when the DTO provides a
value.
- Around line 11-26: The toDTO(EmploymentForm entity) method lacks a null check
and will NPE if entity is null; update toDTO in EmploymentMapper to return null
(or an empty/default EmploymentFormDTO per project conventions) when entity is
null, e.g., check if entity == null at the start of toDTO and short-circuit
before accessing any getters; reference the toDTO method, EmploymentForm
parameter, and EmploymentFormDTO return type when making the change.
In
`@src/main/java/org/example/cyberwatch/features/form/model/UpdateEmploymentDTO.java`:
- Around line 18-51: UpdateEmploymentDTO declares many validated fields
(socialSecurityNumber, firstName, lastName, email, phoneNumber, role,
department, status) but is unused by any controller and
EmploymentMapper.updateEntity() only copies status, creating a misleading API
contract; fix by either removing the DTO entirely if no update endpoint is
intended, or updating the API and mapping logic so the DTO is actually consumed:
add a controller endpoint that accepts UpdateEmploymentDTO and modify
EmploymentMapper.updateEntity(UpdateEmploymentDTO, Employment) to copy the
validated properties (or alternatively strip validation and fields down to only
status if only status updates are supported), ensuring validation annotations
match the supported update behavior.
In
`@src/main/java/org/example/cyberwatch/features/form/repository/EmploymentFormRepository.java`:
- Line 14: The create flow currently only checks
EmploymentFormRepository.existsBySocialSecurityNumber(...), which misses SSNs
already persisted to Staff (because approveAndFinalizeEmployment(...) moves data
into Staff and deletes the form); update the form creation path to also consult
StaffRepository.existsBySocialSecurityNumber(socialSecurityNumber) and reject
the submission if either repository reports existence. Locate the
service/controller method that saves new EmploymentForm (the code that calls
EmploymentFormRepository.save(...) or performs the current
existsBySocialSecurityNumber check) and add a second guard calling
StaffRepository.existsBySocialSecurityNumber(...), returning the same validation
error path when true.
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 44-50: The defaulting to ApprovalStatus.PENDING is unreachable and
done on the DTO after mapping; remove the `@NotNull` on CreateEmploymentDTO.status
(if you want service-side defaulting) and ensure the default is applied before
converting into the entity — i.e., in EmploymentFormService set
form.setStatus(ApprovalStatus.PENDING) when form.getStatus()==null prior to
calling employmentMapper.toEntity(form), or alternatively apply the default
directly on the mapped entity (formEntity.setStatus(ApprovalStatus.PENDING))
before saving with employmentFormRepository.save and returning
employmentMapper.toDTO(...).
- Around line 36-39: The duplicate-SSN safety net currently checks only
employmentFormRepository.existsBySocialSecurityNumber(...) in
EmploymentFormService; add an additional check against the Staff table (via
staffRepository, e.g. staffRepository.existsBySocialSecurityNumber(...) or
equivalent) before accepting a new EmploymentForm and before
approveAndFinalizeEmployment() persists a Staff record, and throw the same
IllegalStateException (or a consistent domain exception) with a clear message
when a matching SSN is found to prevent a unique-constraint failure on save.
In
`@src/main/java/org/example/cyberwatch/features/staff/repository/StaffRepository.java`:
- Line 11: The email field is not guaranteed unique yet code relies on
StaffRepository.findByEmail(...) returning a single result (used by
EmploymentFormService.createForm), so add a uniqueness constraint: update the
Staff entity's email property to include `@Column`(name = "email", unique = true,
nullable = false) (or create a DB migration to add a UNIQUE constraint on the
email column) and run/verify migrations so findByEmail(...) cannot return
duplicates at runtime.
---
Nitpick comments:
In
`@src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.java`:
- Around line 27-31: The createEmploymentForm method currently calls
authentication.getName() without null-checking; update createEmploymentForm to
defensively handle null Authentication (and null getName()) by checking if
authentication == null || authentication.getName() == null and if so either
return an appropriate 401/403 ResponseEntity (e.g.,
ResponseEntity.status(HttpStatus.UNAUTHORIZED).build()) or throw an
AccessDeniedException, otherwise continue to call
employmentFormService.createForm with the authenticated principal; ensure you
reference the Authentication parameter in createEmploymentForm and keep the
loggedInHr variable assignment guarded by this check.
In
`@src/main/java/org/example/cyberwatch/features/form/model/CreateEmploymentDTO.java`:
- Around line 49-52: Remove server-controlled fields from the creation DTO:
delete createdDate and hrId from CreateEmploymentDTO (or at minimum stop
exposing them) and update any mapping logic that maps CreateEmploymentDTO to the
Employment entity (e.g., the mapper that handles CreateEmploymentDTO ->
Employment) to not copy these fields; instead let the entity populate
createdDate via `@CreationTimestamp` and derive hrId from the authenticated
principal when constructing the Employment entity on the server side.
In
`@src/main/java/org/example/cyberwatch/features/form/model/EmploymentFormDTO.java`:
- Around line 39-40: Remove the leftover developer comment above the hrId field
in EmploymentFormDTO; either delete the line "//Long when fetching the Id?" or
replace it with a clear Javadoc or inline comment that explains the purpose of
the hrId field (e.g., what it represents and when it is used) so the field
declaration private Long hrId; is self-explanatory.
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 41-43: The inline comment contains Swedish and assumes
authentication.getName() is an email; replace the Swedish comment and remove the
assumption, and instead explicitly verify or document what
authentication.getName() returns (email vs username/ID) and use the matching
repository lookup: either staffRepository.findByEmail(loggedInHr) or
staffRepository.findByUsername(loggedInHr), or extract the email from
authentication.getPrincipal() if needed; update the code paths around setHrId(),
Staff, staffRepository.findByEmail and loggedInHr to use the correct identifier
and add a brief JavaDoc/comment stating which identifier is expected by
authentication configuration.
In `@src/main/java/org/example/cyberwatch/features/staff/model/Staff.java`:
- Around line 63-64: The createdForms collection in Staff is left null causing
NPEs when used on transient instances; initialize it like the other collections
(reportForms, assignedTickets) by instantiating createdForms (e.g., new
HashSet<>()) at declaration or in the Staff constructor and ensure its getter
getCreatedForms() returns the non-null Set so calls like
getCreatedForms().add(...) are safe; update the field initialization for
createdForms in the Staff class accordingly.
In `@src/main/java/org/example/cyberwatch/features/staff/model/StaffMapper.java`:
- Around line 21-30: The toEntity(StaffDTO dto) mapper drops immutable/required
fields (id and socialSecurityNumber) which will cause updates to create invalid
new Staff rows; change toEntity to either accept an existing Staff to mutate
(e.g., toEntity(StaffDTO dto, Staff existing)) or, if kept as factory,
explicitly copy dto.getId() and dto.getSocialSecurityNumber() into the new Staff
(or validate presence) so id and socialSecurityNumber are preserved for update
flows; update usages of toEntity to pass an existing Staff when performing
updates and keep toEntity only for creates if you choose that split.
In
`@src/test/java/org/example/cyberwatch/form/EmploymentFormControllerTests.java`:
- Around line 28-94: Add slice tests for the newly introduced manager-approval
and pending-form listing endpoints and include 403 checks for role enforcement:
in EmploymentFormControllerTests, add a happy-path test that mocks
employmentFormService.getPendingForms() and asserts GET
"/api/forms/employment/pending" returns 200 with expected JSON (use mockMvc and
objectMapper), add a happy-path test that mocks
employmentFormService.approveForm(id, ...) and asserts PUT/POST to the
controller's manager-approval endpoint returns 200/204 (include csrf() for
non-GET), and for each endpoint add a corresponding test using `@WithMockUser`
with a non-authorized role to assert status().isForbidden(); when creating these
tests reference the mocked service methods employmentFormService.getPendingForms
and employmentFormService.approveForm and reuse mockMvc, objectMapper, and the
existing `@MockitoBean` employmentFormService to stub returns.
🪄 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: ac0d7bf5-3ba3-4a70-b14e-00d5093a84e6
📒 Files selected for processing (16)
src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.javasrc/main/java/org/example/cyberwatch/features/form/controller/FormController.javasrc/main/java/org/example/cyberwatch/features/form/model/CreateEmploymentDTO.javasrc/main/java/org/example/cyberwatch/features/form/model/EmploymentForm.javasrc/main/java/org/example/cyberwatch/features/form/model/EmploymentFormDTO.javasrc/main/java/org/example/cyberwatch/features/form/model/EmploymentMapper.javasrc/main/java/org/example/cyberwatch/features/form/model/ReportForm.javasrc/main/java/org/example/cyberwatch/features/form/model/UpdateEmploymentDTO.javasrc/main/java/org/example/cyberwatch/features/form/repository/EmploymentFormRepository.javasrc/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.javasrc/main/java/org/example/cyberwatch/features/form/service/ReportFormService.javasrc/main/java/org/example/cyberwatch/features/staff/model/Staff.javasrc/main/java/org/example/cyberwatch/features/staff/model/StaffMapper.javasrc/main/java/org/example/cyberwatch/features/staff/repository/StaffRepository.javasrc/main/java/org/example/cyberwatch/shared/model/enums/ApprovalStatus.javasrc/test/java/org/example/cyberwatch/form/EmploymentFormControllerTests.java
💤 Files with no reviewable changes (2)
- src/main/java/org/example/cyberwatch/features/form/model/ReportForm.java
- src/main/java/org/example/cyberwatch/features/form/controller/FormController.java
| public interface StaffRepository extends JpaRepository<Staff, Long> { | ||
| boolean existsBySocialSecurityNumber(String socialSecurityNumber); | ||
|
|
||
| Optional<Staff> findByEmail(String email); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd 'Staff.java$' src --exec sed -n '40,46p' {}
echo
rg -n -C2 '\bfindByEmail\s*\(' src
echo
rg -ni --iglob '*.sql' --iglob '*.xml' --iglob '*.yml' --iglob '*.yaml' 'staff.*email|email.*staff|unique.*email|email.*unique'Repository: ithsjava25/project-backend-super-cool-team
Length of output: 1640
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check Staff entity for `@Table` and `@UniqueConstraint` annotations
echo "=== Checking Staff.java for JPA annotations ==="
fd 'Staff.java$' src --exec cat {} | sed -n '1,50p'
echo
echo "=== Searching for migration files ==="
fd -e 'sql' -e 'xml' -e 'yml' -e 'yaml' . | grep -iE '(migration|changelog|flyway|liquibase|db|schema)' | head -20
echo
echo "=== Searching for `@Table` or `@UniqueConstraint` in Staff.java ==="
rg -n '@Table|@UniqueConstraint' src/main/java/org/example/cyberwatch/features/staff/model/Staff.javaRepository: ithsjava25/project-backend-super-cool-team
Length of output: 2286
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get the full Staff.java file
echo "=== Full Staff.java content ==="
wc -l src/main/java/org/example/cyberwatch/features/staff/model/Staff.java
cat src/main/java/org/example/cyberwatch/features/staff/model/Staff.java
echo
echo "=== All migration files ==="
find src/main/resources/db -type f -name '*.sql' -o -name '*.xml'
echo
echo "=== Check migration V1 for Staff/email ==="
cat src/main/resources/db/migration/V1__add_ticket_assignee.sql | head -30Repository: ithsjava25/project-backend-super-cool-team
Length of output: 3165
Add unique constraint to email column before relying on single-result lookups.
EmploymentFormService.createForm(...) resolves the logged-in HR through findByEmail(...) with .orElseThrow(), which expects a single result. However, the Staff entity does not enforce uniqueness on the email field—unlike socialSecurityNumber which explicitly has @Column(unique = true). The database migration files do not define a unique constraint either. Without this constraint, the lookup can fail at runtime if duplicates exist.
Add @Column(name = "email", unique = true, nullable = false) to the email field or create a migration to enforce this at the database level.
🤖 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/repository/StaffRepository.java`
at line 11, The email field is not guaranteed unique yet code relies on
StaffRepository.findByEmail(...) returning a single result (used by
EmploymentFormService.createForm), so add a uniqueness constraint: update the
Staff entity's email property to include `@Column`(name = "email", unique = true,
nullable = false) (or create a DB migration to add a UNIQUE constraint on the
email column) and run/verify migrations so findByEmail(...) cannot return
duplicates at runtime.
Contributor
Author
There was a problem hiding this comment.
@Column(name = "email", unique = true, nullable = false)
@Email(message = "Email should be valid")
@NotBlank(message = "Email cannot be blank")
private String email;
is implemented
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
…ss, and enhance dependence checks for social security numbers in EmploymentForm services and repository
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/org/example/cyberwatch/features/form/model/EmploymentForm.java (1)
68-73:⚠️ Potential issue | 🟠 MajorEnsure FK migration is committed for the
Staffassociation switch.Line 68 and Line 72 still use legacy FK columns (
hr_id,approver_management_id) while the entity target is nowStaff. Without an explicit backfill/constraint migration for existing data, those values can map incorrectly (or fail) againststaff.employee_id.#!/bin/bash set -euo pipefail echo "=== Entity mappings involved in createdBy/approvedBy ===" fd -i 'EmploymentForm.java|Staff.java|HR.java|Management.java' src/main/java --exec sed -n '1,260p' {} echo echo "=== Migrations touching employment_form and related FK columns ===" fd -e sql src/main/resources/db/migration --exec rg -n -C2 'employment_form|hr_id|approver_management_id|employee_id|staff_id' {}Expected verification outcome:
- Only the intended inverse mappings remain active for
createdBy/approvedBy.- A migration exists that safely backfills/realigns old FK values before enforcing new references.
🤖 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/model/EmploymentForm.java` around lines 68 - 73, EmploymentForm still maps createdBy and approvedBy to legacy FK columns hr_id and approver_management_id while target entity is Staff; add a DB migration that backfills and realigns existing hr_id/approver_management_id values to the new staff primary key (employee_id or staff_id as used by Staff) and then add/rename FK columns and constraints accordingly, then update the entity mapping if you plan to rename columns (EmploymentForm.createdBy / EmploymentForm.approvedBy) so the `@JoinColumn` matches the final column name and the FK constraint references Staff's PK; ensure the migration runs before applying a schema-level FK enforcement and include a verification step querying employment_form rows to confirm all legacy ids were converted and constraints can be applied without failures.
🧹 Nitpick comments (1)
src/main/java/org/example/cyberwatch/features/form/model/EmploymentMapper.java (1)
58-61: MaketoDTOListnull-safe for mapper consistency.
toDTOListwill throw ifentitiesis null, while other mapper entrypoints are null-guarded.Small defensive refactor
public List<EmploymentFormDTO> toDTOList(List<EmploymentForm> entities) { + if (entities == null || entities.isEmpty()) return List.of(); return entities.stream() .map(this::toDTO) .toList(); }🤖 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/model/EmploymentMapper.java` around lines 58 - 61, The toDTOList method in EmploymentMapper should be null-safe like other mappers: check if the incoming List<EmploymentForm> entities is null and return an empty list (e.g., Collections.emptyList() or List.of()) instead of streaming a null reference; update EmploymentMapper.toDTOList to perform the null check and then map each element with this::toDTO when non-null.
🤖 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/form/model/EmploymentMapper.java`:
- Around line 39-40: The mapper currently trusts client-provided status in
EmploymentMapper (entity.setStatus(dto.getStatus())); change the mapping so new
entities always get the server-side default status PENDING (e.g.,
entity.setStatus(Status.PENDING) or the appropriate enum constant) instead of
using dto.getStatus(), leaving EmploymentFormService.createForm() unchanged;
update EmploymentMapper where entity.setStatus(dto.getStatus()) is called to
unconditionally assign the PENDING value.
---
Duplicate comments:
In
`@src/main/java/org/example/cyberwatch/features/form/model/EmploymentForm.java`:
- Around line 68-73: EmploymentForm still maps createdBy and approvedBy to
legacy FK columns hr_id and approver_management_id while target entity is Staff;
add a DB migration that backfills and realigns existing
hr_id/approver_management_id values to the new staff primary key (employee_id or
staff_id as used by Staff) and then add/rename FK columns and constraints
accordingly, then update the entity mapping if you plan to rename columns
(EmploymentForm.createdBy / EmploymentForm.approvedBy) so the `@JoinColumn`
matches the final column name and the FK constraint references Staff's PK;
ensure the migration runs before applying a schema-level FK enforcement and
include a verification step querying employment_form rows to confirm all legacy
ids were converted and constraints can be applied without failures.
---
Nitpick comments:
In
`@src/main/java/org/example/cyberwatch/features/form/model/EmploymentMapper.java`:
- Around line 58-61: The toDTOList method in EmploymentMapper should be
null-safe like other mappers: check if the incoming List<EmploymentForm>
entities is null and return an empty list (e.g., Collections.emptyList() or
List.of()) instead of streaming a null reference; update
EmploymentMapper.toDTOList to perform the null check and then map each element
with this::toDTO when non-null.
🪄 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: b3cb244e-231e-4740-b648-cab9027284f7
📒 Files selected for processing (6)
src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.javasrc/main/java/org/example/cyberwatch/features/form/model/CreateEmploymentDTO.javasrc/main/java/org/example/cyberwatch/features/form/model/EmploymentForm.javasrc/main/java/org/example/cyberwatch/features/form/model/EmploymentMapper.javasrc/main/java/org/example/cyberwatch/features/form/repository/EmploymentFormRepository.javasrc/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/org/example/cyberwatch/features/form/model/CreateEmploymentDTO.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/example/cyberwatch/features/form/repository/EmploymentFormRepository.java
- src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.java
- src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java
… role management and reduce redundancy
…ller method, and update exception handling
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/org/example/cyberwatch/features/staff/exception/StaffNotFoundException.java (1)
4-4: Remove this unhelpful comment.This comment is technically inaccurate (this is a constructor, not a method) and doesn't add value—it describes what was done rather than why. Such comments become noise for future maintainers. Consider removing it.
Suggested fix
public class StaffNotFoundException extends RuntimeException { - //Added extra method public StaffNotFoundException(Long id) { super("Staff with id " + id + " not found"); }🤖 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/exception/StaffNotFoundException.java` at line 4, Remove the unhelpful inline comment "//Added extra method" from the StaffNotFoundException class; specifically delete that comment near the StaffNotFoundException constructor and, if desired, replace it with a concise Javadoc on the constructor explaining its purpose (e.g., that it creates the exception with a message) rather than a non-informative note.
🤖 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/ticket/controller/TicketController.java`:
- Around line 64-67: Change the controller method signature from
ResponseEntity<?> to ResponseEntity<TicketResponseDTO> in assignTicketToStaff
and map the domain Ticket returned by
ticketService.assignTicketToStaff(ticketId, dto.getStaffId()) to a
TicketResponseDTO before returning; locate the controller method
assignTicketToStaff and replace the raw return with ResponseEntity.ok(<mapped
TicketResponseDTO>) using the existing mapping utility (e.g.,
TicketMapper.toResponse(...) or TicketResponseDTO.from(...)), or construct the
DTO fields explicitly if no mapper exists, ensuring you do not expose internal
entity fields.
---
Nitpick comments:
In
`@src/main/java/org/example/cyberwatch/features/staff/exception/StaffNotFoundException.java`:
- Line 4: Remove the unhelpful inline comment "//Added extra method" from the
StaffNotFoundException class; specifically delete that comment near the
StaffNotFoundException constructor and, if desired, replace it with a concise
Javadoc on the constructor explaining its purpose (e.g., that it creates the
exception with a message) rather than a non-informative note.
🪄 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: ae2323a6-be24-4884-84ab-6b6e14a1f60c
📒 Files selected for processing (3)
src/main/java/org/example/cyberwatch/features/staff/exception/StaffNotFoundException.javasrc/main/java/org/example/cyberwatch/features/ticket/controller/TicketController.javasrc/main/resources/db/migration/V1__add_ticket_assignee.sql
💤 Files with no reviewable changes (1)
- src/main/resources/db/migration/V1__add_ticket_assignee.sql
Comment on lines
+64
to
67
| public ResponseEntity<?> assignTicketToStaff( | ||
| @PathVariable Long ticketId, | ||
| @Valid @RequestBody AssignTicketDTO dto) { | ||
| return ResponseEntity.ok(ticketService.assignTicketToStaff(ticketId, dto.getStaffId())); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the controller/service type mismatch for assignTicketToStaff.
set -euo pipefail
echo "1) Controller method signature:"
rg -nP '\bResponseEntity<\?>\s+assignTicketToStaff\s*\(' -C2
echo
echo "2) Service method return type:"
rg -nP '\bpublic\s+Ticket\s+assignTicketToStaff\s*\(' -C2
echo
echo "3) All assignTicketToStaff declarations/usages for manual contract check:"
rg -nP '\bassignTicketToStaff\s*\(' -C2Repository: ithsjava25/project-backend-super-cool-team
Length of output: 3741
🏁 Script executed:
#!/bin/bash
echo "=== Other endpoints in TicketController to verify return type pattern ==="
rg -nP 'public ResponseEntity' src/main/java/org/example/cyberwatch/features/ticket/controller/TicketController.java -A2
echo
echo "=== TicketResponseDTO definition ==="
fd -e java -x grep -l 'class TicketResponseDTO' {} \;
echo
echo "=== Check if assignTicketToStaff has performedById or similar actor parameter ==="
rg -nP 'assignTicketToStaff' src/main/java/org/example/cyberwatch/features/ticket/controller/TicketController.java -B5 -A5
echo
echo "=== Full assignTicketToStaff endpoint code ==="
sed -n '62,68p' src/main/java/org/example/cyberwatch/features/ticket/controller/TicketController.javaRepository: ithsjava25/project-backend-super-cool-team
Length of output: 2818
Keep a typed response contract; don't widen to ResponseEntity<?>.
Line 64 weakens the endpoint contract and masks that assignTicketToStaff currently returns a domain Ticket instead of TicketResponseDTO. This makes the API inconsistent with the rest of TicketController (all other endpoints return ResponseEntity<TicketResponseDTO>) and can leak internal entity fields.
Change the return type to ResponseEntity<TicketResponseDTO> and map the service output to the DTO before returning.
🤖 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/ticket/controller/TicketController.java`
around lines 64 - 67, Change the controller method signature from
ResponseEntity<?> to ResponseEntity<TicketResponseDTO> in assignTicketToStaff
and map the domain Ticket returned by
ticketService.assignTicketToStaff(ticketId, dto.getStaffId()) to a
TicketResponseDTO before returning; locate the controller method
assignTicketToStaff and replace the raw return with ResponseEntity.ok(<mapped
TicketResponseDTO>) using the existing mapping utility (e.g.,
TicketMapper.toResponse(...) or TicketResponseDTO.from(...)), or construct the
DTO fields explicitly if no mapper exists, ensuring you do not expose internal
entity fields.
Contributor
Author
There was a problem hiding this comment.
Create an issue for this
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo ithsjava25/project-backend-super-cool-team \
--title "TicketController: Restore typed ResponseEntity<TicketResponseDTO> in assignTicketToStaff" \
--assignee codebyNorthsteep \
--body "## Summary
The \`assignTicketToStaff\` endpoint in \`TicketController\` was changed from \`ResponseEntity<TicketResponseDTO>\` to \`ResponseEntity<?>\`, weakening the API contract and making it inconsistent with all other endpoints in the same controller.
Additionally, the underlying service method \`ticketService.assignTicketToStaff\` currently returns a domain \`Ticket\` entity rather than a \`TicketResponseDTO\`, which can inadvertently leak internal entity fields to API consumers.
## Steps to resolve
1. Change the controller method return type back to \`ResponseEntity<TicketResponseDTO>\`.
2. Map the domain \`Ticket\` returned by \`ticketService.assignTicketToStaff(ticketId, dto.getStaffId())\` to a \`TicketResponseDTO\` before returning (using an existing mapper/utility or explicit construction).
3. Ensure no internal entity fields are exposed in the response.
## References
- PR: https://github.com/ithsjava25/project-backend-super-cool-team/pull/48
- Review comment: https://github.com/ithsjava25/project-backend-super-cool-team/pull/48#discussion_r3044021864
Requested by `@codebyNorthsteep`"
Length of output: 71
@codebyNorthsteep, the issue has been created and assigned to you: #49 — TicketController: Restore typed ResponseEntity<TicketResponseDTO> in assignTicketToStaff.
It captures:
- Restoring the typed
ResponseEntity<TicketResponseDTO>return type. - Mapping the domain
Ticketfrom the service to aTicketResponseDTObefore returning, to avoid leaking internal entity fields.
🧠 Learnings used
Learnt from: gitnes94
Repo: ithsjava25/project-backend-super-cool-team PR: 31
File: src/main/java/org/example/cyberwatch/features/ticket/controller/TicketController.java:47-49
Timestamp: 2026-04-07T06:09:03.792Z
Learning: In `src/main/java/org/example/cyberwatch/features/ticket/controller/TicketController.java`, the `performedById` and `uploadedById` parameters in endpoints (`advanceStatus`, `setStatus`, `reopen`, `assignTicketToStaff`, `uploadFile`) are currently accepted as caller-controlled `RequestParam` values, making the audit trail forgeable. This is a known security issue that will be fixed in a future PR once authentication is implemented. The plan is to resolve the actor server-side via `AuthenticationPrincipal StaffUserDetails principal` and remove `performedById`/`uploadedById` from the public request signatures entirely.
This was referenced Apr 7, 2026
This was referenced Apr 20, 2026
Merged
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 comprehensive implementation for employment form management, including new endpoints, DTOs, services, and entity/model adjustments. The changes enable HR users to create employment forms, managers to approve them, and the system to convert approved forms into staff records. Additionally, the codebase is refactored to use a consistent
ApprovalStatusenum and to generalize relationships to theStaffentity.Key changes are grouped below by theme:
Employment Form Feature Implementation
EmploymentFormControllerwith endpoints for creating employment forms (HR), approving forms (Manager), and listing pending forms (HR/Manager), enforcing role-based access control.EmploymentFormServicewith logic to create employment forms, prevent duplicate SSNs, retrieve pending forms, and finalize employment by converting forms to staff records.EmploymentFormRepositoryfor finding forms by status and checking for duplicate SSNs.CreateEmploymentDTO,EmploymentFormDTO,UpdateEmploymentDTO) for form data transfer and validation, and createdEmploymentMapperfor conversions between entities and DTOs. [1] [2] [3] [4]Model and Entity Refactoring
EmploymentFormand related DTOs to use the newApprovalStatusenum instead of the previousStatustype, and generalized relationships fromHR/ManagementtoStafffor both creator and approver. [1] [2] [3]Staffentity to include a new relationship for created employment forms and imported the relevant form model. [1] [2]ApprovalStatusenum to standardize approval state management.Repository and Mapper Enhancements
StaffRepositorywith methods to check for existing staff by SSN and to find staff by email for authentication/authorization logic.StaffMapperfor converting betweenStaffentities and DTOs.Cleanup and Other Adjustments
FormControllerclass, as its responsibilities are now handled by the new employment form controller.ReportFormattachments mapping for consistency.ReportFormServicefor future report form handling.These changes collectively establish a robust foundation for employment form workflows, improve code maintainability, and ensure proper validation and authorization throughout the process.
Summary by CodeRabbit
New Features
Refactor
Chores / Breaking Changes
Tests