Feature/extend employment form - #55
Merged
Merged
Conversation
… comments for future S3 file upload logic
…ploymentForm for S3 support; and apply Flyway migrations
…dling for new staff, and enhance EmploymentFormController with approved form retrieval functionality.
…lity, update/reject/delete workflows, and secure password generation; update dependencies to include Apache Commons Lang.
…ploymentMapper` for updating forms; add `UpdateEmploymentDTO` model with validation.
… duplicate SSN validation logic, streamline form retrieval methods, and enhance code readability.
…m` endpoint in `EmploymentFormController`, rename `cv_s3_key` to `employed_s3_key` in `Staff`, and enhance `DataInitializer` with password encoding.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds commons‑lang3; registers BCrypt PasswordEncoder and encodes seeded staff passwords; extends EmploymentForm and Staff with S3 key and password fields; implements S3 JSON archival and expands employment-form lifecycle (create/update/approve/reject/delete) with DB migrations and unit tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Controller as EmploymentFormController
participant Service as EmploymentFormService
participant DB as Database
participant JSON as ObjectMapper
participant S3 as S3Service
participant Encoder as PasswordEncoder
Client->>Controller: POST /employment (create)
Controller->>Service: createForm(dto, hrEmail)
Service->>DB: save(employmentForm)
DB-->>Service: saved form
Service-->>Controller: EmploymentFormDTO
Controller-->>Client: 201 Created
Client->>Controller: POST /{id}/approve
Controller->>Service: approveAndFinalizeEmployment(id, approverEmail)
Service->>DB: findById(id)
DB-->>Service: EmploymentForm (PENDING)
Service->>JSON: serialize(form)
JSON-->>Service: jsonData
Service->>S3: uploadJsonData(key, jsonData)
S3-->>Service: s3Key
Service->>Encoder: generate & encode(password)
Encoder-->>Service: hashedPassword
Service->>DB: staffRepository.save(new Staff with hashedPassword)
DB-->>Service: saved staff
Service->>DB: save(updated EmploymentForm with s3Key/status)
DB-->>Service: saved form
Service-->>Controller: approval message (contains temp password)
Controller-->>Client: 200 OK
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 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 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: 8
🧹 Nitpick comments (9)
pom.xml (1)
166-166: Optional: Remove redundant scope declaration.The
<scope>compile</scope>declaration is redundant sincecompileis the default scope in Maven. You can safely remove this line for cleaner configuration.♻️ Simplify dependency declaration
<dependency> <groupId>org.apache.commons</groupId> <artifactId>commons-lang3</artifactId> <version>3.20.0</version> - <scope>compile</scope> </dependency>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pom.xml` at line 166, The dependency declaration contains an explicit <scope>compile</scope> which is redundant because Maven's default scope is compile; remove the <scope>compile</scope> element from the dependency block(s) (the XML element containing the dependency where <scope>compile</scope> appears) so the pom.xml remains functionally identical but cleaner and less verbose.src/main/java/org/example/cyberwatch/features/form/model/EmploymentMapper.java (1)
45-46: Consider throwing on null inputs for void methods.Unlike
toDTO/toEntitywhich returnnullsignaling no result,updateEntitysilently does nothing when either parameter is null. This could mask bugs in callers that accidentally pass null.♻️ Proposed defensive approach
public void updateEntity(UpdateEmploymentDTO dto, EmploymentForm entity) { - if (dto == null || entity == null) return; + if (dto == null || entity == null) { + throw new IllegalArgumentException("DTO and entity must not be null"); + } entity.setSocialSecurityNumber(dto.getSocialSecurityNumber());🤖 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 45 - 46, The updateEntity method currently returns silently when dto or entity is null; change it to validate inputs and throw a descriptive unchecked exception instead (e.g., throw new IllegalArgumentException or use Objects.requireNonNull) so callers immediately fail fast; update the updateEntity(UpdateEmploymentDTO dto, EmploymentForm entity) implementation to check dto and entity and throw with clear messages like "dto must not be null" / "entity must not be null".src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (2)
144-145: Semantic confusion: UsingapprovedByto store rejector.Setting
approvedByfor a rejected form is semantically misleading. Consider adding a separaterejectedByfield or renaming the field to something more generic likeprocessedBy.This is a data model consideration that may require a migration, so flagging for awareness rather than immediate action.
🤖 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 144 - 145, The code in EmploymentFormService sets ApprovalStatus.REJECTED and then calls form.setApprovedBy(rejector), which is semantically confusing; update the data model and service to avoid misusing approvedBy by either adding a new rejectedBy field (and use form.setRejectedBy(rejector) when ApprovalStatus.REJECTED) or renaming approvedBy to a neutral processedBy and use form.setProcessedBy(rejector) for both approvals and rejections; update the EmploymentFormService usage (the setStatus(...) call sites and any getters) and prepare a data migration plan to populate the new field from existing approvedBy values if you change the schema name.
84-93: Performance: Loading all forms into memory for filtering.
searchAndFilterFormscallsfindAll()and filters in-memory. This won't scale well as the number of forms grows. Consider implementing repository query methods with proper filtering.♻️ Recommended approach
Add a custom query method to
EmploymentFormRepository:`@Query`("SELECT f FROM EmploymentForm f WHERE " + "(:ssn IS NULL OR f.socialSecurityNumber LIKE %:ssn%) AND " + "(:department IS NULL OR f.department = :department) AND " + "(:status IS NULL OR f.status = :status)") List<EmploymentForm> searchForms(`@Param`("ssn") String ssn, `@Param`("department") Department department, `@Param`("status") ApprovalStatus status);🤖 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 84 - 93, searchAndFilterForms currently loads all entities via employmentFormRepository.findAll() and filters in-memory, which doesn't scale; add a repository query method (e.g. in EmploymentFormRepository define searchForms(String ssn, Department department, ApprovalStatus status) using a JPQL/@Query with optional parameters for ssn (LIKE), department and status), then replace the findAll() call in searchAndFilterForms with a call to employmentFormRepository.searchForms(...) and keep mapping with employmentMapper::toDTO so filtering happens in the database rather than in-memory.src/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java (3)
95-113: Test doesn't verify the rejector assignment.The test verifies that status changes to
REJECTEDandsaveis called, but doesn't assert thatform.getApprovedBy()was set to the manager. This is an important part of the rejection flow for audit purposes.💚 Proposed enhancement
// Assert assertEquals(ApprovalStatus.REJECTED, form.getStatus()); + assertEquals(manager, form.getApprovedBy()); verify(formRepository).save(form);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java` around lines 95 - 113, The test rejectForm_Success in EmploymentFormServiceTest currently verifies status and save but doesn't assert the rejector assignment; update the test to assert that the EmploymentForm's approver field was set to the manager returned by staffRepository.findByEmail (verify form.getApprovedBy() equals the local manager instance used in the Arrange), and keep the existing verify(formRepository).save(form) and status assertion; reference the rejectForm call on service, the manager variable, and form.getApprovedBy()/EmploymentForm to locate where to add the assertion.
85-85: Remove non-English comment.The comment
// Rättat här: existingForm istället för existingDtoappears to be a development note in Swedish. Consider removing it or translating to English for consistency.✏️ Proposed fix
EmploymentForm existingForm = new EmploymentForm(); existingForm.setCreatedBy(creator); - existingForm.setStatus(ApprovalStatus.PENDING); // Rättat här: existingForm istället för existingDto + existingForm.setStatus(ApprovalStatus.PENDING);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java` at line 85, Remove or translate the Swedish inline comment after the call to existingForm.setStatus(ApprovalStatus.PENDING) in the EmploymentFormServiceTest class; locate the occurrence of existingForm.setStatus(...) and either delete the comment `// Rättat här: existingForm istället för existingDto` or replace it with a brief English comment such as `// fixed: use existingForm instead of existingDto` to keep test code comments consistent.
45-143: Consider adding test coverage for edge cases and remaining methods.The current tests cover the happy paths well. Consider adding tests for:
deleteFormauthorization and status checkssearchAndFilterFormsfiltering logic- SSN validation when SSN changes during update
- Duplicate SSN rejection in
createFormWould you like me to generate additional test cases for these scenarios?
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java` around lines 45 - 143, Add unit tests covering the missing edge cases: create tests for deleteForm to assert authorization and status checks (e.g., call deleteForm with non-owner and with non-deletable statuses to expect exceptions), add tests for searchAndFilterForms to verify filtering/sorting/pagination behavior with various criteria, add tests for updateFormBeforeApproval to assert SSN validation when SSN is changed (reject when duplicate via staffRepository.existsBySocialSecurityNumber or formRepository.existsBySocialSecurityNumber), and add a test for createForm to assert duplicate SSN rejection (mock formRepository.existsBySocialSecurityNumber and staffRepository.existsBySocialSecurityNumber to return true and expect exception). Reference test targets: deleteForm, searchAndFilterForms, updateFormBeforeApproval, and createForm; mock repositories (formRepository, staffRepository), mapper methods (mapper.toEntity/formToStaff), and assert expected exceptions and repository interactions.src/main/java/org/example/cyberwatch/features/ticket/service/S3Service.java (1)
41-58: Consider adding input validation for key and jsonData parameters.The method doesn't validate inputs before attempting the S3 upload. Null or empty values would result in less informative errors from the S3 SDK.
🛡️ Proposed defensive validation
public String uploadJsonData(String key, String jsonData) { + if (key == null || key.isBlank()) { + throw new IllegalArgumentException("S3 key cannot be null or blank"); + } + if (jsonData == null) { + throw new IllegalArgumentException("JSON data cannot be null"); + } try { PutObjectRequest putObjectRequest = PutObjectRequest.builder()Additionally, be aware that this method will silently overwrite existing objects at the same key if S3 versioning is not enabled on the bucket.
🤖 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/service/S3Service.java` around lines 41 - 58, The uploadJsonData method lacks input validation and can pass null/empty values into s3Client.putObject; add defensive checks at the start of uploadJsonData for key and jsonData (e.g., null or blank key, null or empty jsonData) and throw a clear IllegalArgumentException with context (include the parameter name and expected format). Optionally validate jsonData is not just whitespace or use a lightweight JSON sanity check before creating RequestBody.fromString(jsonData). Keep existing logging (log.info) and preserve exception wrapping for S3 errors; reference uploadJsonData, bucketName, s3Client and RequestBody when adding the checks so they sit before calling s3Client.putObject.src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.java (1)
26-27: Remove or translate the development comment.The comment
//get form att fylla i, används även när man updaterar?is in Swedish and appears to be a development note. Consider removing it or providing a clear English comment.✏️ Proposed fix
- //get form att fylla i, används även när man updaterar? - `@PostMapping`(value = "/employment")🤖 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 26 - 27, The inline Swedish developer comment in EmploymentFormController ("//get form att fylla i, används även när man updaterar?") should be removed or replaced with a clear English comment; locate the comment in the EmploymentFormController class and either delete it or replace it with a concise English description of the method/behavior it was annotating (e.g., explaining that the endpoint returns the employment form for create/update scenarios) so the codebase contains no non-English development notes.
🤖 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 45-68: Add two endpoints in EmploymentFormController: a POST
mapping for "/{id}/reject" annotated with `@PreAuthorize`("hasRole('CEO') or
hasRole('CTO')") that accepts `@PathVariable` Long id, `@RequestBody` String
rejectionReason (or a small DTO) and Authentication auth, then calls
employmentFormService.rejectForm(id, rejectionReason, auth.getName()) and
returns the appropriate ResponseEntity (e.g., OK or no-content with payload as
needed); and a DELETE mapping for "/{id}" with authorization that allows the
form creator or MANAGEMENT role (e.g., `@PreAuthorize`("hasRole('MANAGEMENT') or
`@authenticationPrincipal`?.username == ???" or simply accept Authentication and
rely on employmentFormService.deleteForm(id, auth.getName()) to enforce creator
check), implement method signature deleteForm(`@PathVariable` Long id,
Authentication auth) that calls employmentFormService.deleteForm(id,
auth.getName()) and returns ResponseEntity.noContent() or appropriate response.
Ensure method names and service calls reference
employmentFormService.rejectForm(...) and employmentFormService.deleteForm(...).
In
`@src/main/java/org/example/cyberwatch/features/form/model/EmploymentForm.java`:
- Around line 67-68: The entity field in EmploymentForm named employedS3Key is
annotated with `@Column`(name = "cv_s3_key") which mismatches the database
migration V3__update_employment_form_for_s3.sql that creates employed_s3_key;
update the `@Column` on the employedS3Key field (in class EmploymentForm) to use
name = "employed_s3_key" so the JPA mapping matches the migration and S3 keys
persist, or alternatively adjust the migration to create cv_s3_key if you prefer
the entity name.
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 200-201: The method in EmploymentFormService currently returns the
rawPassword and logs it; remove the raw password from the returned string and
from any logs (replace with masked value or omit), and instead deliver the
password via a secure channel: call an existing EmailService (e.g.,
EmailService.sendTemporaryPassword or sendPasswordToEmployee) with the
form/employee identifier and rawPassword, or create and invoke a
securePasswordDelivery method to handle emailing and one-time retrieval; update
the return value to a safe message or a minimal secure response object (no
password field) and ensure logger.info("Form {} approved by {}", formId,
loggedInManagement) remains but does not include rawPassword.
- Around line 113-114: In EmploymentFormService, the check that compares
existingForm.getCreatedBy().getEmail() to loggedInHrEmail can NPE if
getCreatedBy() is null; change the logic to first verify
existingForm.getCreatedBy() != null before calling getEmail(), and if createdBy
is null throw a clear IllegalStateException (or a custom exception) with a
message like "Form has no creator; only the HR staff who created this form can
update it" so the intent remains the same but avoids a NullPointerException.
- Line 232: Replace the use of the Social Security Number in the S3 object key
to avoid embedding PII: in EmploymentFormService locate the s3Key construction
(String s3Key = "archive/employments/" + form.getSocialSecurityNumber() +
".json") and change it to use a non-PII identifier such as form.getId(), a
generated UUID, or a hashed token derived from the form ID (e.g.,
UUID.randomUUID() or form.getId().toString()) so the path becomes
archive/employments/{nonPiiId}.json; ensure any downstream code that reads or
deletes the object uses the same non-PII key.
- Around line 163-165: The delete authorization check in EmploymentFormService
currently compares requester.getRole().name() to the non-existent "MANAGEMENT"
role, so update the condition to compare against actual Role enum values (for
example Role.ADMIN and/or other managerial roles like Role.CEO or
Role.PROJECT_MANAGER) instead of the string "MANAGEMENT"; modify the if to
something like checking requester.getRole() == Role.ADMIN (or use a set:
EnumSet.of(Role.ADMIN, Role.CEO,
Role.PROJECT_MANAGER).contains(requester.getRole())) to allow intended
management roles to delete, and add an import for the Role enum if not already
imported.
- Around line 205-208: The generateSecurePassword method currently uses
RandomStringUtils.random(...), which is not cryptographically secure; replace
that call in EmploymentFormService.generateSecurePassword with the secure API by
calling RandomStringUtils.secure().nextAlphanumeric(12) so the method uses
SecureRandom-backed generation for passwords instead of the static random(...)
variant.
In `@src/main/resources/db/migration/V3__update_employment_form_for_s3.sql`:
- Around line 1-3: Migration added column employed_s3_key but the JPA entity
EmploymentForm (field annotated with `@Column`(name = "cv_s3_key")) expects
cv_s3_key, causing Hibernate to read/write a non-existent column; update the SQL
in V3__update_employment_form_for_s3.sql to add column cv_s3_key VARCHAR(500)
(or alternatively change the `@Column` name in EmploymentForm.java to
"employed_s3_key") so the DB column name and the entity mapping (the `@Column` on
the EmploymentForm field at line ~67) match.
---
Nitpick comments:
In `@pom.xml`:
- Line 166: The dependency declaration contains an explicit
<scope>compile</scope> which is redundant because Maven's default scope is
compile; remove the <scope>compile</scope> element from the dependency block(s)
(the XML element containing the dependency where <scope>compile</scope> appears)
so the pom.xml remains functionally identical but cleaner and less verbose.
In
`@src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.java`:
- Around line 26-27: The inline Swedish developer comment in
EmploymentFormController ("//get form att fylla i, används även när man
updaterar?") should be removed or replaced with a clear English comment; locate
the comment in the EmploymentFormController class and either delete it or
replace it with a concise English description of the method/behavior it was
annotating (e.g., explaining that the endpoint returns the employment form for
create/update scenarios) so the codebase contains no non-English development
notes.
In
`@src/main/java/org/example/cyberwatch/features/form/model/EmploymentMapper.java`:
- Around line 45-46: The updateEntity method currently returns silently when dto
or entity is null; change it to validate inputs and throw a descriptive
unchecked exception instead (e.g., throw new IllegalArgumentException or use
Objects.requireNonNull) so callers immediately fail fast; update the
updateEntity(UpdateEmploymentDTO dto, EmploymentForm entity) implementation to
check dto and entity and throw with clear messages like "dto must not be null" /
"entity must not be null".
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 144-145: The code in EmploymentFormService sets
ApprovalStatus.REJECTED and then calls form.setApprovedBy(rejector), which is
semantically confusing; update the data model and service to avoid misusing
approvedBy by either adding a new rejectedBy field (and use
form.setRejectedBy(rejector) when ApprovalStatus.REJECTED) or renaming
approvedBy to a neutral processedBy and use form.setProcessedBy(rejector) for
both approvals and rejections; update the EmploymentFormService usage (the
setStatus(...) call sites and any getters) and prepare a data migration plan to
populate the new field from existing approvedBy values if you change the schema
name.
- Around line 84-93: searchAndFilterForms currently loads all entities via
employmentFormRepository.findAll() and filters in-memory, which doesn't scale;
add a repository query method (e.g. in EmploymentFormRepository define
searchForms(String ssn, Department department, ApprovalStatus status) using a
JPQL/@Query with optional parameters for ssn (LIKE), department and status),
then replace the findAll() call in searchAndFilterForms with a call to
employmentFormRepository.searchForms(...) and keep mapping with
employmentMapper::toDTO so filtering happens in the database rather than
in-memory.
In `@src/main/java/org/example/cyberwatch/features/ticket/service/S3Service.java`:
- Around line 41-58: The uploadJsonData method lacks input validation and can
pass null/empty values into s3Client.putObject; add defensive checks at the
start of uploadJsonData for key and jsonData (e.g., null or blank key, null or
empty jsonData) and throw a clear IllegalArgumentException with context (include
the parameter name and expected format). Optionally validate jsonData is not
just whitespace or use a lightweight JSON sanity check before creating
RequestBody.fromString(jsonData). Keep existing logging (log.info) and preserve
exception wrapping for S3 errors; reference uploadJsonData, bucketName, s3Client
and RequestBody when adding the checks so they sit before calling
s3Client.putObject.
In
`@src/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java`:
- Around line 95-113: The test rejectForm_Success in EmploymentFormServiceTest
currently verifies status and save but doesn't assert the rejector assignment;
update the test to assert that the EmploymentForm's approver field was set to
the manager returned by staffRepository.findByEmail (verify form.getApprovedBy()
equals the local manager instance used in the Arrange), and keep the existing
verify(formRepository).save(form) and status assertion; reference the rejectForm
call on service, the manager variable, and form.getApprovedBy()/EmploymentForm
to locate where to add the assertion.
- Line 85: Remove or translate the Swedish inline comment after the call to
existingForm.setStatus(ApprovalStatus.PENDING) in the EmploymentFormServiceTest
class; locate the occurrence of existingForm.setStatus(...) and either delete
the comment `// Rättat här: existingForm istället för existingDto` or replace it
with a brief English comment such as `// fixed: use existingForm instead of
existingDto` to keep test code comments consistent.
- Around line 45-143: Add unit tests covering the missing edge cases: create
tests for deleteForm to assert authorization and status checks (e.g., call
deleteForm with non-owner and with non-deletable statuses to expect exceptions),
add tests for searchAndFilterForms to verify filtering/sorting/pagination
behavior with various criteria, add tests for updateFormBeforeApproval to assert
SSN validation when SSN is changed (reject when duplicate via
staffRepository.existsBySocialSecurityNumber or
formRepository.existsBySocialSecurityNumber), and add a test for createForm to
assert duplicate SSN rejection (mock formRepository.existsBySocialSecurityNumber
and staffRepository.existsBySocialSecurityNumber to return true and expect
exception). Reference test targets: deleteForm, searchAndFilterForms,
updateFormBeforeApproval, and createForm; mock repositories (formRepository,
staffRepository), mapper methods (mapper.toEntity/formToStaff), and assert
expected exceptions and repository interactions.
🪄 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: 7426f4be-b0ed-42e8-b46a-44ff0733178d
📒 Files selected for processing (14)
pom.xmlsrc/main/java/org/example/cyberwatch/config/DataInitializer.javasrc/main/java/org/example/cyberwatch/config/SecurityConfig.javasrc/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.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/model/UpdateEmploymentDTO.javasrc/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.javasrc/main/java/org/example/cyberwatch/features/staff/model/Staff.javasrc/main/java/org/example/cyberwatch/features/ticket/service/S3Service.javasrc/main/resources/db/migration/V3__update_employment_form_for_s3.sqlsrc/main/resources/db/migration/V4__update_staff_with_s3_and_password.sqlsrc/test/java/org/example/cyberwatch/features/form/EmploymentFormControllerTests.javasrc/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java
… secure password generation, S3 archiving update, and `employed_s3_key` renaming; add `ObjectMapper` to `SecurityConfig`.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (2)
203-205:⚠️ Potential issue | 🟠 MajorDo not return the generated password from this service.
This still leaks a credential into HTTP logs, traces, proxies, and client storage. Return a neutral success message and deliver the temporary password through a one-time secure channel instead.
🤖 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 203 - 205, The service method in EmploymentFormService currently logs approval and returns the generated rawPassword (see logger.info("Form {} approved by {}", formId, loggedInManagement) and the return that appends rawPassword); remove the rawPassword from the HTTP response and instead return a neutral success message (e.g., "Employment approved, temporary password delivered") and invoke a secure one-time delivery mechanism (call an existing EmailService/NotificationService or create a OneTimeSecretService method such as sendTemporaryPassword(formId, userId, rawPassword) or storeOneTimeSecret(...)) to deliver the temporary password; ensure no password value is included in logs, responses, or exceptions and replace the current return that exposes rawPassword with the neutral message.
164-166:⚠️ Potential issue | 🟡 MinorGuard
createdBybefore comparing emails.A null creator will throw before the CEO/CTO fallback can run, so malformed or legacy rows turn this authorization check into a 500.
Suggested fix
- if (!form.getCreatedBy().getEmail().equals(loggedInEmail) + if ((form.getCreatedBy() == null || !form.getCreatedBy().getEmail().equals(loggedInEmail)) && requester.getRole() != Role.CEO && requester.getRole() != Role.CTO) { throw new IllegalStateException("Only the HR staff who created this form or management can delete it"); }🤖 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 164 - 166, The authorization check in EmploymentFormService currently calls form.getCreatedBy().getEmail() without guarding for a null createdBy, causing an NPE for legacy/malformed rows; change the condition so you first test whether form.getCreatedBy() is null (treating null as “not the creator”) or the creator's email does not equal loggedInEmail, and only then apply the requester role fallback checks (requester.getRole() != Role.CEO and requester.getRole() != Role.CTO); update the existing if-condition that references form.getCreatedBy().getEmail(), loggedInEmail, requester.getRole(), Role.CEO and Role.CTO to perform the null check before any getEmail() call.
🧹 Nitpick comments (1)
src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (1)
85-93: Push search/filtering down to the repository.
findAll()materializes the full table and then filters SSN/department/status in memory, so this path will not benefit from DB-side filtering or pagination as the dataset grows.🤖 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 85 - 93, searchAndFilterForms currently calls employmentFormRepository.findAll() and does in-memory filtering; change it to push filtering into the data layer by adding repository-level query(s) or a JPA Specification so the DB performs the SSN/department/status predicates (and supports pagination). Update EmploymentFormRepository to provide either dynamic query methods (e.g. findBySocialSecurityNumberContainingAndDepartmentAndStatus with nullable params handled) or implement a Specification/Criteria-based method (e.g. findAll(Specification<EmploymentForm>, Pageable)), then call that new repo method from searchAndFilterForms (instead of findAll()) and map results with employmentMapper::toDTO.
🤖 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/service/EmploymentFormService.java`:
- Around line 189-201: The S3 archive call (archiveToS3(form)) happens before
the database persists and can roll back, causing orphaned S3 writes; move the
archiveToS3 invocation to run only after the DB transaction successfully
commits—either by calling it after employmentFormRepository.save(form) and
staffRepository.save(newStaff) within a non-transactional context or by
registering a transaction synchronization / publishing a post-commit event
(e.g., using TransactionSynchronizationManager or an
`@TransactionalEventListener`) that invokes archiveToS3(form); also ensure
archiveToS3 is idempotent or handle exceptions/logging separately so failures do
not affect DB state.
- Around line 198-201: EmploymentFormService creates a Staff via
employmentMapper.formToStaff(form) but never sets the employedS3Key, so the
newStaff saved by staffRepository.save(newStaff) has no archive reference; fix
by assigning the archive key from the form to the Staff before saving (e.g.,
ensure EmploymentMapper.formToStaff copies employedS3Key or explicitly call
newStaff.setEmployedS3Key(form.getEmployedS3Key()) after mapping and before
staffRepository.save(newStaff)) so the persisted Staff row contains the
employedS3Key.
---
Duplicate comments:
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 203-205: The service method in EmploymentFormService currently
logs approval and returns the generated rawPassword (see logger.info("Form {}
approved by {}", formId, loggedInManagement) and the return that appends
rawPassword); remove the rawPassword from the HTTP response and instead return a
neutral success message (e.g., "Employment approved, temporary password
delivered") and invoke a secure one-time delivery mechanism (call an existing
EmailService/NotificationService or create a OneTimeSecretService method such as
sendTemporaryPassword(formId, userId, rawPassword) or storeOneTimeSecret(...))
to deliver the temporary password; ensure no password value is included in logs,
responses, or exceptions and replace the current return that exposes rawPassword
with the neutral message.
- Around line 164-166: The authorization check in EmploymentFormService
currently calls form.getCreatedBy().getEmail() without guarding for a null
createdBy, causing an NPE for legacy/malformed rows; change the condition so you
first test whether form.getCreatedBy() is null (treating null as “not the
creator”) or the creator's email does not equal loggedInEmail, and only then
apply the requester role fallback checks (requester.getRole() != Role.CEO and
requester.getRole() != Role.CTO); update the existing if-condition that
references form.getCreatedBy().getEmail(), loggedInEmail, requester.getRole(),
Role.CEO and Role.CTO to perform the null check before any getEmail() call.
---
Nitpick comments:
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 85-93: searchAndFilterForms currently calls
employmentFormRepository.findAll() and does in-memory filtering; change it to
push filtering into the data layer by adding repository-level query(s) or a JPA
Specification so the DB performs the SSN/department/status predicates (and
supports pagination). Update EmploymentFormRepository to provide either dynamic
query methods (e.g. findBySocialSecurityNumberContainingAndDepartmentAndStatus
with nullable params handled) or implement a Specification/Criteria-based method
(e.g. findAll(Specification<EmploymentForm>, Pageable)), then call that new repo
method from searchAndFilterForms (instead of findAll()) and map results with
employmentMapper::toDTO.
🪄 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: 3ebef616-3b3b-46fd-9d95-c8e8dc76a023
📒 Files selected for processing (3)
src/main/java/org/example/cyberwatch/config/SecurityConfig.javasrc/main/java/org/example/cyberwatch/features/form/model/EmploymentForm.javasrc/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/java/org/example/cyberwatch/features/form/model/EmploymentForm.java
- src/main/java/org/example/cyberwatch/config/SecurityConfig.java
… reordering database operations before S3 archiving; improve password generation and staff creation logic.
…er configuration separation.
…, retrieval by ID, rejection with reason, and deletion; improve pending and approved forms handling.
…ate `approveForm` to return success response with service result.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (1)
84-94: In-memory filtering will not scale.
searchAndFilterFormsloads all forms viafindAll()and filters in Java. This is inefficient for large datasets and bypasses database indexing.Consider using a repository query with
@Queryor Spring Data JPA Specifications to push filtering to the database.♻️ Example: Push filtering to repository
// In EmploymentFormRepository `@Query`("SELECT f FROM EmploymentForm f WHERE " + "(:ssn IS NULL OR f.socialSecurityNumber LIKE %:ssn%) AND " + "(:department IS NULL OR f.department = :department) AND " + "(:status IS NULL OR f.status = :status)") List<EmploymentForm> searchForms( `@Param`("ssn") String socialSecurityNumber, `@Param`("department") Department department, `@Param`("status") ApprovalStatus status);Then in the service:
public List<EmploymentFormDTO> searchAndFilterForms(String socialSecurityNumber, Department department, ApprovalStatus status) { - List<EmploymentForm> forms = employmentFormRepository.findAll(); - - return forms.stream() - .filter(form -> socialSecurityNumber == null || form.getSocialSecurityNumber().contains(socialSecurityNumber)) - .filter(form -> department == null || form.getDepartment().equals(department)) - .filter(form -> status == null || form.getStatus().equals(status)) - .map(employmentMapper::toDTO) - .toList(); + return employmentMapper.toDTOList( + employmentFormRepository.searchForms(socialSecurityNumber, department, status)); }🤖 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 84 - 94, The method searchAndFilterForms currently loads all records via employmentFormRepository.findAll() and filters in-memory, which won't scale; change this to delegate filtering to the database by adding a repository query or Specification in EmploymentFormRepository (e.g., a method like searchForms or a Specification-based finder) that accepts socialSecurityNumber, Department and ApprovalStatus and performs the conditional WHERE predicates, then update searchAndFilterForms to call that new repository method and map results with employmentMapper::toDTO.src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.java (1)
81-90: Consider PII exposure in query parameters.Passing
socialSecurityNumberas a query parameter means SSN values may appear in server access logs, browser history, and proxy logs. For sensitive PII searches, consider using a POST request with a request body instead.🤖 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 81 - 90, The current GET endpoint searchForms in EmploymentFormController accepts socialSecurityNumber as a query parameter (risking PII in logs); change the API to accept a POST (e.g., `@PostMapping`("/search")) that consumes a request body DTO (create e.g., EmploymentFormSearchRequest with socialSecurityNumber, department, status) and have searchForms accept that DTO instead of `@RequestParam`; update the call to employmentFormService.searchAndFilterForms to take the DTO (or its fields) and keep the same `@PreAuthorize` check and response type, ensuring any client/consumer updates and controller tests are adjusted accordingly.
🤖 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/config/S3Config.java`:
- Around line 36-39: S3Config currently defines a plain ObjectMapper in the
objectMapper() bean which causes LocalDateTime to be serialized as arrays;
either delete the objectMapper() bean to let Spring Boot inject its
auto-configured ObjectMapper, or if you must keep the custom bean, update the
objectMapper() method to register Jackson's JavaTimeModule and disable
SerializationFeature.WRITE_DATES_AS_TIMESTAMPS so LocalDateTime is serialized as
ISO‑8601 strings.
In
`@src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.java`:
- Around line 38-45: The approveForm method in EmploymentFormController is
calling employmentFormService.approveAndFinalizeEmployment(id,
authentication.getName()) twice which causes a failure on the second call; fix
by calling employmentFormService.approveAndFinalizeEmployment(...) only once,
capture its return value in a local variable, and return that variable in the
ResponseEntity instead of invoking the service a second time (adjust the method
signature/return handling in approveForm accordingly).
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 192-211: The S3 key is being set after both EmploymentForm and
Staff are persisted, so persist the S3 key first: call archiveToS3(form) before
converting/saving Staff and before employmentFormRepository.save, then after
archiveToS3 returns set form.getEmployedS3Key and copy that value into newStaff
via newStaff.setEmployedS3Key(...); finally encode password and persist newStaff
and employmentFormRepository.save(form). Update the flow in
EmploymentFormService so archiveToS3(form) runs prior to
employmentMapper.formToStaff(form), staffRepository.save(...), and
employmentFormRepository.save(...), while keeping the existing error logging
around archiveToS3.
- Around line 170-174: The current check in EmploymentFormService uses
form.getCreatedBy().getEmail() which can NPE if createdBy is null; update the
conditional to first handle a null createdBy (e.g., treat null as "not created
by the requester") before calling getEmail(): check form.getCreatedBy() != null
&& form.getCreatedBy().getEmail().equals(loggedInEmail) (or invert logic to test
createdBy == null || !createdBy.getEmail().equals(loggedInEmail)) combined with
the existing role checks on requester.getRole() against Role.CEO and Role.CTO,
and only throw the IllegalStateException when the requester is neither the
creator (safely evaluated) nor a manager.
- Around line 145-153: EmploymentFormService currently saves the form
(employmentFormRepository.save(form)) before calling archiveToS3(form), but
archiveToS3 sets form.setEmployedS3Key(s3Key) so the S3 key is never persisted;
move the save to after archiving (or call employmentFormRepository.save(form)
again after archiveToS3 returns) so the updated employedS3Key is persisted,
ensuring any exceptions from archiveToS3 are still logged via the existing
try/catch around archiveToS3.
---
Nitpick comments:
In
`@src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.java`:
- Around line 81-90: The current GET endpoint searchForms in
EmploymentFormController accepts socialSecurityNumber as a query parameter
(risking PII in logs); change the API to accept a POST (e.g.,
`@PostMapping`("/search")) that consumes a request body DTO (create e.g.,
EmploymentFormSearchRequest with socialSecurityNumber, department, status) and
have searchForms accept that DTO instead of `@RequestParam`; update the call to
employmentFormService.searchAndFilterForms to take the DTO (or its fields) and
keep the same `@PreAuthorize` check and response type, ensuring any
client/consumer updates and controller tests are adjusted accordingly.
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 84-94: The method searchAndFilterForms currently loads all records
via employmentFormRepository.findAll() and filters in-memory, which won't scale;
change this to delegate filtering to the database by adding a repository query
or Specification in EmploymentFormRepository (e.g., a method like searchForms or
a Specification-based finder) that accepts socialSecurityNumber, Department and
ApprovalStatus and performs the conditional WHERE predicates, then update
searchAndFilterForms to call that new repository method and map results with
employmentMapper::toDTO.
🪄 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: 298703c8-9996-49b7-9375-129d39a76892
📒 Files selected for processing (4)
src/main/java/org/example/cyberwatch/config/S3Config.javasrc/main/java/org/example/cyberwatch/config/SecurityConfig.javasrc/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.javasrc/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/org/example/cyberwatch/config/SecurityConfig.java
…mproved transactional integrity, fix role-based validation logic, and adjust `EmploymentFormController` for consistent service method usage.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (3)
211-213:⚠️ Potential issue | 🔴 CriticalThe approval response still returns the raw password.
Line 213 includes the generated password in the service response, and the controller returns it verbatim. That exposes credentials to HTTP logs, proxies, and client storage. Return a neutral success message here and deliver the password only through the planned secure channel.
🤖 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 211 - 213, The service currently returns the generated rawPassword string in the response (see EmploymentFormService where logger.info("Form {} approved by {}", formId, loggedInManagement) and the return that concatenates rawPassword); remove the raw password from the return value and instead return a neutral success message (e.g., "Employment approved; credentials delivered via secure channel"), and ensure no logging of rawPassword anywhere in EmploymentFormService or related controller methods (update the controller method that consumes the service return to stop expecting/propagating the password). Leave the password delivery responsibility to the planned secure/email flow so credentials are never exposed in service responses, logs, or HTTP payloads.
147-153:⚠️ Potential issue | 🔴 CriticalS3 archival still happens before the transaction is durable.
Lines 148 and 202 upload to S3 while the DB transaction is still open. Any later failure in
staffRepository.save(...),employmentFormRepository.save(...), or the commit leaves orphaned archives and mismatched state. Move the upload to an after-commit/outbox path.Also applies to: 201-209
🤖 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 147 - 153, The S3 upload (archiveToS3) is currently executed while the DB transaction is still open, which can leave orphaned S3 objects if subsequent saves or the commit fail; move the upload to run only after a successful commit by deferring archiveToS3 until transaction completion (for example use Spring's TransactionSynchronizationManager.registerSynchronization/afterCommit or publish an event and handle it with `@TransactionalEventListener`(phase = AFTER_COMMIT)), or persist an outbox record alongside employmentFormRepository.save/staffRepository.save and have a separate worker send to S3; update EmploymentFormService to save entities inside the transaction first (employmentFormRepository.save, staffRepository.save) and trigger archiveToS3 only in the after-commit/outbox path.
171-174:⚠️ Potential issue | 🟠 MajorNull
createdBynow opens a delete-authorization bypass.Because Line 171 guards the whole check with
form.getCreatedBy() != null, a form without a creator skips authorization entirely, so any HR caller that reaches this method can delete it. TreatcreatedBy == nullas “not creator” and requireisCreator || isManagement.🤖 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 171 - 174, The current check lets forms with null createdBy bypass authorization; change it to treat null createdBy as "not creator" by computing a boolean isCreator = form.getCreatedBy() != null && form.getCreatedBy().getEmail().equals(loggedInEmail) (or equivalent) and then require isCreator OR management; i.e. replace the existing compound guarded-if with a check like if (!isCreator && requester.getRole() != Role.CEO && requester.getRole() != Role.CTO) throw new IllegalStateException(...), referencing form.getCreatedBy(), getEmail(), loggedInEmail, requester.getRole(), Role.CEO and Role.CTO.
🧹 Nitpick comments (1)
src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (1)
85-93:findAll()makes every search a full table scan in the app.Line 86 loads the entire table and Lines 88-93 filter in memory, so even selective searches pay O(N) DB + heap cost. Push these predicates down to the database before this endpoint starts seeing real volume.
🤖 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 85 - 93, The current searchAndFilterForms in EmploymentFormService calls employmentFormRepository.findAll and filters in memory (using employmentMapper::toDTO after filtering), causing full table scans; change this to push predicates into the DB by adding repository query support (either concrete query methods or a JPA Specification/Criteria query) that accepts optional parameters for socialSecurityNumber (use "contains"/LIKE), department and status, then call that new repository method from searchAndFilterForms and map results with employmentMapper.toDTO; update EmploymentFormRepository (e.g., add findBySocialSecurityNumberContainingAndDepartmentAndStatus or a method that applies nullable parameters) or implement a Specification builder used by EmploymentFormService to build DB-side predicates.
🤖 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 81-88: Change the public API to accept a JSON body instead of
query params: update EmploymentFormController.searchForms to be a POST mapping
(replace `@GetMapping` with `@PostMapping`) and accept a request body DTO (e.g.,
EmploymentFormSearchDTO containing socialSecurityNumber, department, and status)
annotated with `@RequestBody` instead of `@RequestParam`; forward that DTO to
employmentFormService.searchAndFilterForms (or add an overload that accepts the
DTO) and update any method signatures and tests accordingly so SSNs are no
longer exposed in URL query strings.
---
Duplicate comments:
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 211-213: The service currently returns the generated rawPassword
string in the response (see EmploymentFormService where logger.info("Form {}
approved by {}", formId, loggedInManagement) and the return that concatenates
rawPassword); remove the raw password from the return value and instead return a
neutral success message (e.g., "Employment approved; credentials delivered via
secure channel"), and ensure no logging of rawPassword anywhere in
EmploymentFormService or related controller methods (update the controller
method that consumes the service return to stop expecting/propagating the
password). Leave the password delivery responsibility to the planned
secure/email flow so credentials are never exposed in service responses, logs,
or HTTP payloads.
- Around line 147-153: The S3 upload (archiveToS3) is currently executed while
the DB transaction is still open, which can leave orphaned S3 objects if
subsequent saves or the commit fail; move the upload to run only after a
successful commit by deferring archiveToS3 until transaction completion (for
example use Spring's
TransactionSynchronizationManager.registerSynchronization/afterCommit or publish
an event and handle it with `@TransactionalEventListener`(phase = AFTER_COMMIT)),
or persist an outbox record alongside
employmentFormRepository.save/staffRepository.save and have a separate worker
send to S3; update EmploymentFormService to save entities inside the transaction
first (employmentFormRepository.save, staffRepository.save) and trigger
archiveToS3 only in the after-commit/outbox path.
- Around line 171-174: The current check lets forms with null createdBy bypass
authorization; change it to treat null createdBy as "not creator" by computing a
boolean isCreator = form.getCreatedBy() != null &&
form.getCreatedBy().getEmail().equals(loggedInEmail) (or equivalent) and then
require isCreator OR management; i.e. replace the existing compound guarded-if
with a check like if (!isCreator && requester.getRole() != Role.CEO &&
requester.getRole() != Role.CTO) throw new IllegalStateException(...),
referencing form.getCreatedBy(), getEmail(), loggedInEmail, requester.getRole(),
Role.CEO and Role.CTO.
---
Nitpick comments:
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 85-93: The current searchAndFilterForms in EmploymentFormService
calls employmentFormRepository.findAll and filters in memory (using
employmentMapper::toDTO after filtering), causing full table scans; change this
to push predicates into the DB by adding repository query support (either
concrete query methods or a JPA Specification/Criteria query) that accepts
optional parameters for socialSecurityNumber (use "contains"/LIKE), department
and status, then call that new repository method from searchAndFilterForms and
map results with employmentMapper.toDTO; update EmploymentFormRepository (e.g.,
add findBySocialSecurityNumberContainingAndDepartmentAndStatus or a method that
applies nullable parameters) or implement a Specification builder used by
EmploymentFormService to build DB-side predicates.
🪄 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: 6f0aca08-06ae-4dad-9735-39fc6bd45b6e
📒 Files selected for processing (2)
src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.javasrc/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java
…tabind.ObjectMapper` with `tools.jackson.databind.ObjectMapper` and remove redundant `ObjectMapper` bean from `S3Config`.
…endpoint from `EmploymentFormService` and `EmploymentFormController`.
Contributor
Author
|
@coderabbitai resume |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (2)
198-200:⚠️ Potential issue | 🔴 CriticalDo not return the generated password in the response body.
This still exposes a credential through the API path, where it can end up in logs, proxies, APM tooling, and client storage. Return a neutral success message and deliver the password only through the planned secure channel.
Minimal safe return value
- return "Employment has been approved, generated password for new employee: " + rawPassword; + return "Employment approved. Credentials will be delivered through a secure channel.";🤖 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 198 - 200, In EmploymentFormService (the approval flow around the logger.info("Form {} approved by {}", formId, loggedInManagement) and return statement), stop returning rawPassword in the HTTP response: replace the returned string with a neutral success message (e.g., "Employment approved") and ensure no password is logged or included in the response; instead hand off rawPassword to the planned secure delivery mechanism (email/service) or a separate secure method for delivering credentials, and update any callers/tests that assert the previous response.
134-140:⚠️ Potential issue | 🔴 CriticalArchive after commit, not inside the transaction.
archiveToS3is an irreversible external write here. If a later DB save or the transaction commit fails, S3 still contains an archive for a rejection/approval that never became durable in the database. Move the archive step to anAFTER_COMMITevent/synchronization and persist the key separately.Also applies to: 188-196
🤖 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 134 - 140, Remove the direct call to archiveToS3(form) from the transactional flow in EmploymentFormService and instead perform the S3 upload after the DB transaction successfully commits: in the method that currently calls archiveToS3(form) and employmentFormRepository.save(form), persist any archive metadata you need (e.g., a placeholder key or "pending" flag) via employmentFormRepository.save(form), then register a TransactionSynchronization (TransactionSynchronizationManager.registerSynchronization or use `@TransactionalEventListener`(phase = AFTER_COMMIT)) that invokes archiveToS3(form) after commit and updates the persisted entity with the final S3 key if needed. Apply the same change for the other occurrence referenced (around lines 188-196) so all irreversible external writes happen in AFTER_COMMIT synchronization.
🤖 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 81-84: The rejectForm method currently accepts the free-text
reason as a `@RequestParam` (in rejectForm), which exposes HR data in URLs/logs;
change the API to accept a small DTO in the request body (e.g., create a record
RejectEmploymentFormRequest(`@NotBlank` String reason)) and update rejectForm to
take `@RequestBody` `@Valid` RejectEmploymentFormRequest request instead of
`@RequestParam` String reason, then use request.reason() (or request.getReason())
inside the method; add the jakarta.validation.@NotBlank import and `@Valid` import
and ensure controller has validation enabled so incoming reason is validated and
no longer appears in URLs/access logs.
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 216-223: The duplicate-checking in validateSsnNotExists currently
compares raw strings; normalize the incoming SSN first (e.g., convert
YYYYMMDD-NNNN to YYMMDD-NNNN or otherwise canonicalize/remove century and
non-digit characters to a single canonical format) and then call
employmentFormRepository.existsBySocialSecurityNumber(normalizedSsn) and
staffRepository.existsBySocialSecurityNumber(normalizedSsn); update any related
persistence/DTO handling to use the same normalize function so saved values and
future checks use the same canonical representation (add a helper
normalizeSsn(String) and call it from validateSsnNotExists).
- Around line 52-56: The current creation code in EmploymentFormService uses
employmentMapper.toEntity(form) but only sets ApprovalStatus.PENDING when the
mapped entity's status is null, allowing clients to supply APPROVED/REJECTED;
change this so that after mapping you always set
formEntity.setStatus(ApprovalStatus.PENDING) (remove the null-check) to ignore
any client-supplied status and ensure all new forms persist with PENDING status;
update any related comments and keep use of the ApprovalStatus enum and
EmploymentFormService method where the mapping occurs.
- Around line 158-161: In EmploymentFormService (the delete/check block using
form.getCreatedBy()), change the guard so a null createdBy is treated as "not
the creator": replace the current if that only runs when getCreatedBy() != null
with a condition like "if ((form.getCreatedBy() == null ||
!form.getCreatedBy().getEmail().equals(loggedInEmail)) && requester.getRole() !=
Role.CEO && requester.getRole() != Role.CTO) throw ...", so only the actual
creator or CEO/CTO can bypass the delete restriction.
---
Duplicate comments:
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 198-200: In EmploymentFormService (the approval flow around the
logger.info("Form {} approved by {}", formId, loggedInManagement) and return
statement), stop returning rawPassword in the HTTP response: replace the
returned string with a neutral success message (e.g., "Employment approved") and
ensure no password is logged or included in the response; instead hand off
rawPassword to the planned secure delivery mechanism (email/service) or a
separate secure method for delivering credentials, and update any callers/tests
that assert the previous response.
- Around line 134-140: Remove the direct call to archiveToS3(form) from the
transactional flow in EmploymentFormService and instead perform the S3 upload
after the DB transaction successfully commits: in the method that currently
calls archiveToS3(form) and employmentFormRepository.save(form), persist any
archive metadata you need (e.g., a placeholder key or "pending" flag) via
employmentFormRepository.save(form), then register a TransactionSynchronization
(TransactionSynchronizationManager.registerSynchronization or use
`@TransactionalEventListener`(phase = AFTER_COMMIT)) that invokes
archiveToS3(form) after commit and updates the persisted entity with the final
S3 key if needed. Apply the same change for the other occurrence referenced
(around lines 188-196) so all irreversible external writes happen in
AFTER_COMMIT synchronization.
🪄 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: 5ac43642-8952-4601-9a7c-a410b2736593
📒 Files selected for processing (3)
src/main/java/org/example/cyberwatch/config/S3Config.javasrc/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.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/config/S3Config.java
✅ Actions performedReviews resumed. |
… remove rejection reason requirement, and adjust role-based validation logic; update tests and controller accordingly.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java (2)
193-195:⚠️ Potential issue | 🟠 MajorDo not return the generated password in the response.
This still exposes a credential to API consumers, logs, proxies, and monitoring. Return a neutral success message and hand the password off to the planned secure delivery path instead.
🤖 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 193 - 195, The method in EmploymentFormService currently returns the generated rawPassword and logs approval details (see variables formId, loggedInManagement, rawPassword); remove the rawPassword from the response and logs, replace the return value with a neutral success message like "Employment approved" (or similar), and instead invoke the secure delivery path (e.g., call the planned email/PasswordDeliveryService or enqueue a secure job) to hand off rawPassword for out-of-band delivery; ensure no other logging or returned objects include rawPassword.
129-135:⚠️ Potential issue | 🔴 CriticalArchive to S3 only after the transaction commits.
Both workflows upload to S3 before the final database save/commit. If
staffRepository.save(...),employmentFormRepository.save(...), or the transaction commit fails afterward, the DB rolls back but the archive is already published. Move the upload to anAFTER_COMMIThook/event and keep it idempotent.Also applies to: 183-191
🤖 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 129 - 135, The S3 upload (archiveToS3) is happening before the database commit which can lead to published archives when the transaction rolls back; change the flow to perform the archive only after the transaction successfully commits by moving the upload into an AFTER_COMMIT callback/event and make the upload idempotent. Specifically, remove direct calls to archiveToS3 around employmentFormRepository.save and staffRepository.save, register a transactional synchronization or application event listener (e.g., using TransactionSynchronizationManager.registerSynchronization or a `@TransactionalEventListener`(phase = AFTER_COMMIT)) that receives the formId/form and calls archiveToS3, and ensure archiveToS3 (or its caller) is safe to retry (idempotent) so duplicate events or retries won’t create inconsistent state.
🤖 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/service/EmploymentFormService.java`:
- Around line 124-125: The service currently only verifies the email exists
(Staff rejector = staffRepository.findByEmail(loggedInManagementEmail)...); add
an explicit role/authorization check in EmploymentFormService after resolving
the Staff (same for the other occurrence around lines 168-169) to ensure only
management roles (CEO or CTO) can approve/reject: after retrieving the Staff
object, verify staff.getRole() (or the equivalent role field) is one of the
allowed enums/strings ("CEO","CTO") and throw an AuthorizationException (or
reuse the existing access-denied exception) if not; mirror the same guard logic
used in deleteForm so both approve/reject code paths enforce CEO/CTO
authorization.
- Line 18: Replace the incorrect import of ObjectMapper in EmploymentFormService
(currently referenced as tools.jackson.databind.ObjectMapper) with the standard
Jackson package; update the import to use
com.fasterxml.jackson.databind.ObjectMapper so the EmploymentFormService class
compiles and uses the proper Jackson ObjectMapper implementation.
---
Duplicate comments:
In
`@src/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.java`:
- Around line 193-195: The method in EmploymentFormService currently returns the
generated rawPassword and logs approval details (see variables formId,
loggedInManagement, rawPassword); remove the rawPassword from the response and
logs, replace the return value with a neutral success message like "Employment
approved" (or similar), and instead invoke the secure delivery path (e.g., call
the planned email/PasswordDeliveryService or enqueue a secure job) to hand off
rawPassword for out-of-band delivery; ensure no other logging or returned
objects include rawPassword.
- Around line 129-135: The S3 upload (archiveToS3) is happening before the
database commit which can lead to published archives when the transaction rolls
back; change the flow to perform the archive only after the transaction
successfully commits by moving the upload into an AFTER_COMMIT callback/event
and make the upload idempotent. Specifically, remove direct calls to archiveToS3
around employmentFormRepository.save and staffRepository.save, register a
transactional synchronization or application event listener (e.g., using
TransactionSynchronizationManager.registerSynchronization or a
`@TransactionalEventListener`(phase = AFTER_COMMIT)) that receives the formId/form
and calls archiveToS3, and ensure archiveToS3 (or its caller) is safe to retry
(idempotent) so duplicate events or retries won’t create inconsistent state.
🪄 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: f022b997-c60c-465b-a59c-4b388b9d6b7d
📒 Files selected for processing (3)
src/main/java/org/example/cyberwatch/features/form/controller/EmploymentFormController.javasrc/main/java/org/example/cyberwatch/features/form/service/EmploymentFormService.javasrc/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/test/java/org/example/cyberwatch/features/form/service/EmploymentFormServiceTest.java
# Conflicts: # src/main/java/org/example/cyberwatch/config/SecurityConfig.java
… form approval and rejection.
This was referenced Apr 14, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 several significant enhancements to the employment form workflow, including secure password handling, improved form update and approval processes, and integration with AWS S3 for archiving. It also adds new endpoints and DTOs to support updating and filtering employment forms, and improves security practices by using password hashing.
Employment Form Workflow Enhancements:
UpdateEmploymentDTO, anupdateFormBeforeApprovalservice method, and a corresponding endpoint inEmploymentFormControllerto allow HR to edit pending forms. [1] [2] [3] [4]Security Improvements:
PasswordEncoder(BCrypt) for staff accounts, both in the data initializer and when creating new staff from approved forms. [1] [2] [3] [4] [5] [6]pom.xmlto include the Apache Commons Lang3 library, used for secure random password generation. [1] [2]Archiving and Data Management:
S3Service, and the S3 key is stored in the form entity. [1] [2]Codebase and Dependency Updates:
EmploymentFormServiceto use Lombok's@RequiredArgsConstructorfor cleaner dependency injection and updated imports accordingly.pom.xmlfor utility functions.These changes collectively improve the security, maintainability, and usability of the employment form management features.
Summary by CodeRabbit
New Features
Improvements
Chores
Tests