Validation in folder feature/staff - #21
Merged
Merged
Conversation
… and setters; streamline code by removing redundant methods
…red fields are validated for data integrity
…ensure staff and department fields are validated for data integrity
…setters; remove redundant methods to streamline code
📝 WalkthroughWalkthroughMultiple JPA entity classes were refactored to use Lombok-generated accessors ( Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate 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: 5
🧹 Nitpick comments (1)
src/main/java/org/example/cyberwatch/features/staff/model/Staff.java (1)
57-59: Add helpers for the new bidirectionalreportFormsrelation.
reportFormsis the inverse side of the association; the FK is owned byReportForm.staffinsrc/main/java/org/example/cyberwatch/features/form/model/ReportForm.java, Lines 45-47. ExposingaddReportForm/removeReportFormhelpers here makes it much harder for callers to leave the two sides out of sync.Suggested change
`@OneToMany`(mappedBy = "staff", cascade = CascadeType.ALL, orphanRemoval = true) private Set<ReportForm> reportForms = new HashSet<>(); + + public void addReportForm(ReportForm reportForm) { + reportForms.add(reportForm); + reportForm.setStaff(this); + } + + public void removeReportForm(ReportForm reportForm) { + reportForms.remove(reportForm); + reportForm.setStaff(null); + }🤖 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 57 - 59, Staff.java's bidirectional relation reportForms is the inverse side (owned by ReportForm.staff) and needs helper methods to keep both sides in sync; add instance methods addReportForm(ReportForm reportForm) and removeReportForm(ReportForm reportForm) in the Staff class that respectively add/remove the ReportForm from the reportForms Set and set/unset reportForm.setStaff(this) (and null on remove) so the ReportForm.staff and Staff.reportForms are always consistent. Ensure the helpers handle nulls and avoid duplicate adds or removes.
🤖 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/ReportForm.java`:
- Around line 45-47: The ReportForm.staff association is marked non-null at the
DB level (`@JoinColumn`(nullable = false)) but lacks Bean Validation; update the
ReportForm class by annotating the private Staff staff field with `@NotNull`
(import javax.validation.constraints.NotNull) alongside the existing `@ManyToOne`
and `@JoinColumn` annotations so validation fails fast in the application layer,
consistent with other entities like Management, HR, and Consultant.
- Around line 49-51: Add ordering and bidirectional consistency for the
attachments collection: annotate the attachments field with `@OrderColumn` (or
`@OrderBy`) so JPA preserves list order, and add helper methods on ReportForm
(e.g., addAttachment(Attachment a) and removeAttachment(Attachment a)) that
set/unset Attachment.reportForm to this and update the attachments list
accordingly; ensure these helpers are used anywhere attachments are modified so
the inverse side (Attachment.reportForm) and the owning FK remain consistent
with the ReportForm.attachments list.
In `@src/main/java/org/example/cyberwatch/features/staff/model/HR.java`:
- Around line 24-26: The `@NotNull` on the HR.department field uses the wrong
message ("Staff cannot be null"); update the validation message to accurately
reference the department (e.g., "Department cannot be null" or similar) in the
HR class by changing the `@NotNull`(message = "...") annotation on the Department
department field so validation errors report the correct missing-field name.
In `@src/main/java/org/example/cyberwatch/features/staff/model/Management.java`:
- Around line 25-27: The `@NotNull` message on the Management.department field is
incorrect; update the validation message on the Department department field (the
`@NotNull` annotation) to reference "Department" instead of "Staff" so the API
returns a correct error (e.g., change message = "Staff cannot be null" to
message = "Department cannot be null" on the department field in class
Management).
In `@src/main/java/org/example/cyberwatch/features/staff/model/StaffDTO.java`:
- Around line 11-13: The class-level Lombok `@Data` on StaffDTO exposes sensitive
fields via generated toString/equals/hashCode; replace `@Data` with explicit
annotations: use `@Getter` and `@Setter` plus `@NoArgsConstructor` and
`@AllArgsConstructor`, add `@EqualsAndHashCode`(onlyExplicitlyIncluded = true) and
mark the non-PII identifier (e.g., id) with `@EqualsAndHashCode.Include`, and
avoid generating a full toString by using `@ToString`(onlyExplicitlyIncluded =
true) and annotate only safe fields (e.g., name, role) with `@ToString.Include` so
socialSecurityNumber, email, and phoneNumber are never included in generated
methods for StaffDTO.
---
Nitpick comments:
In `@src/main/java/org/example/cyberwatch/features/staff/model/Staff.java`:
- Around line 57-59: Staff.java's bidirectional relation reportForms is the
inverse side (owned by ReportForm.staff) and needs helper methods to keep both
sides in sync; add instance methods addReportForm(ReportForm reportForm) and
removeReportForm(ReportForm reportForm) in the Staff class that respectively
add/remove the ReportForm from the reportForms Set and set/unset
reportForm.setStaff(this) (and null on remove) so the ReportForm.staff and
Staff.reportForms are always consistent. Ensure the helpers handle nulls and
avoid duplicate adds or removes.
🪄 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: 04420864-57a2-4015-91c7-09033d037729
📒 Files selected for processing (7)
src/main/java/org/example/cyberwatch/features/form/model/Attachment.javasrc/main/java/org/example/cyberwatch/features/form/model/ReportForm.javasrc/main/java/org/example/cyberwatch/features/staff/model/Consultant.javasrc/main/java/org/example/cyberwatch/features/staff/model/HR.javasrc/main/java/org/example/cyberwatch/features/staff/model/Management.javasrc/main/java/org/example/cyberwatch/features/staff/model/Staff.javasrc/main/java/org/example/cyberwatch/features/staff/model/StaffDTO.java
…d is validated in ReportForm and StaffDTO
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/java/org/example/cyberwatch/features/staff/model/Staff.java (1)
65-67: Consider using@NoArgsConstructorfor consistency.The manual no-args constructor works, but
StaffDTOuses@NoArgsConstructor. Using the Lombok annotation here would be more consistent and reduce boilerplate.♻️ Suggested change
`@Getter` `@Setter` +@NoArgsConstructor `@Entity` public class Staff { // ... fields and relationships ... - - public 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/Staff.java` around lines 65 - 67, Replace the manual empty no-args constructor in the Staff class with Lombok's `@NoArgsConstructor` for consistency with StaffDTO; remove the explicit public Staff() { } and add the `@NoArgsConstructor` annotation on the Staff class declaration (ensure Lombok is imported: lombok.NoArgsConstructor) so the boilerplate constructor is generated automatically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/org/example/cyberwatch/features/staff/model/Staff.java`:
- Around line 62-63: The Staff.assignedTickets `@OneToMany` mappedBy references a
non-existent Ticket.assignee; add a matching field in Ticket named "assignee"
annotated with `@ManyToOne` (targetEntity = Staff.class) and the appropriate join
column so the bidirectional mapping works, then adjust cascade/orphan behavior
on Staff.assignedTickets: replace CascadeType.ALL and orphanRemoval=true with
cascade = {CascadeType.PERSIST, CascadeType.MERGE} and remove orphanRemoval (or
alternatively keep desired cascade but implement a `@PreRemove` method on Staff to
reassign or handle tickets before deletion) to avoid automatically deleting
tickets when a staff is removed.
---
Nitpick comments:
In `@src/main/java/org/example/cyberwatch/features/staff/model/Staff.java`:
- Around line 65-67: Replace the manual empty no-args constructor in the Staff
class with Lombok's `@NoArgsConstructor` for consistency with StaffDTO; remove the
explicit public Staff() { } and add the `@NoArgsConstructor` annotation on the
Staff class declaration (ensure Lombok is imported: lombok.NoArgsConstructor) so
the boilerplate constructor is generated automatically.
🪄 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: e73b9a89-b048-4656-bdc5-9142a84a0ddf
📒 Files selected for processing (9)
src/main/java/org/example/cyberwatch/features/form/model/ReportForm.javasrc/main/java/org/example/cyberwatch/features/staff/model/HR.javasrc/main/java/org/example/cyberwatch/features/staff/model/Management.javasrc/main/java/org/example/cyberwatch/features/staff/model/Staff.javasrc/main/java/org/example/cyberwatch/features/staff/model/StaffDTO.javasrc/main/java/org/example/cyberwatch/features/ticket/model/entitys/staff/Consultant.javasrc/main/java/org/example/cyberwatch/features/ticket/model/entitys/staff/HR.javasrc/main/java/org/example/cyberwatch/features/ticket/model/entitys/staff/Management.javasrc/main/java/org/example/cyberwatch/features/ticket/model/entitys/staff/Staff.java
💤 Files with no reviewable changes (4)
- src/main/java/org/example/cyberwatch/features/ticket/model/entitys/staff/HR.java
- src/main/java/org/example/cyberwatch/features/ticket/model/entitys/staff/Management.java
- src/main/java/org/example/cyberwatch/features/ticket/model/entitys/staff/Consultant.java
- src/main/java/org/example/cyberwatch/features/ticket/model/entitys/staff/Staff.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/org/example/cyberwatch/features/staff/model/HR.java
- src/main/java/org/example/cyberwatch/features/staff/model/Management.java
- src/main/java/org/example/cyberwatch/features/form/model/ReportForm.java
Comment on lines
+62
to
+63
| @OneToMany(mappedBy = "assignee", cascade = CascadeType.ALL, orphanRemoval = true) | ||
| private Set<Ticket> assignedTickets = new HashSet<>(); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Ticket entity for assignee field and its relationship configuration
ast-grep --pattern $'class Ticket {
$$$
assignee
$$$
}'
# Search for Staff deletion handling in services
rg -n -C3 'delete.*[Ss]taff|[Ss]taff.*delete' --type javaRepository: ithsjava25/project-backend-super-cool-team
Length of output: 68
🏁 Script executed:
# Find Ticket entity file
fd -t f -i ticket.java
# Search for assignee field more broadly
rg -n "assignee" --type java
# Look for Staff service/repository deletion methods
fd -t f -i "staffservice\|staffrepository" --type java
# Search for `@PreRemove` or deletion handlers
rg -n "@PreRemove\|@PreDelete\|beforeDelete" --type javaRepository: ithsjava25/project-backend-super-cool-team
Length of output: 504
🏁 Script executed:
# Read the Ticket entity
cat -n src/main/java/org/example/cyberwatch/features/ticket/model/Ticket.java
# Search for deletion handling in services (broader pattern)
rg -n "delete|remove|@PreRemove|@PreDelete" src/main/java/org/example/cyberwatch/features/staff/ -A 2 -B 2
# Find Staff service or controller
fd -t f -name "*Staff*" | grep -E "service|controller|repository"Repository: ithsjava25/project-backend-super-cool-team
Length of output: 440
🏁 Script executed:
# Read the complete Staff.java file
cat -n src/main/java/org/example/cyberwatch/features/staff/model/Staff.java
# Search for Staff service files
find src -name "*Staff*Service*" -o -name "*Staff*Repository*"
# Search for any deletion methods
rg "delete|remove" src/main/java/org/example/cyberwatch/features/staff/Repository: ithsjava25/project-backend-super-cool-team
Length of output: 3176
🏁 Script executed:
# Read the StaffService
cat -n src/main/java/org/example/cyberwatch/features/staff/service/StaffService.java
# Read the StaffRepository
cat -n src/main/java/org/example/cyberwatch/features/staff/repository/StaffRepository.javaRepository: ithsjava25/project-backend-super-cool-team
Length of output: 508
Fix broken relationship mapping — Ticket entity is missing the assignee field.
The @OneToMany(mappedBy = "assignee", ...) on line 62 references a field that doesn't exist in the Ticket class. The Ticket entity is currently empty and needs to define the matching @ManyToOne relationship to Staff named assignee for this mapping to work.
Additionally, once the assignee field is added to Ticket, reconsider the cascade configuration: CascadeType.ALL with orphanRemoval = true will delete all assigned tickets when a staff member is deleted. For audit/history purposes, consider using cascade = {CascadeType.PERSIST, CascadeType.MERGE} without orphanRemoval, or implement reassignment logic in a @PreRemove method.
🤖 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 62 - 63, The Staff.assignedTickets `@OneToMany` mappedBy references a
non-existent Ticket.assignee; add a matching field in Ticket named "assignee"
annotated with `@ManyToOne` (targetEntity = Staff.class) and the appropriate join
column so the bidirectional mapping works, then adjust cascade/orphan behavior
on Staff.assignedTickets: replace CascadeType.ALL and orphanRemoval=true with
cascade = {CascadeType.PERSIST, CascadeType.MERGE} and remove orphanRemoval (or
alternatively keep desired cascade but implement a `@PreRemove` method on Staff to
reassign or handle tickets before deletion) to avoid automatically deleting
tickets when a staff is removed.
Contributor
Author
There was a problem hiding this comment.
This is being handled in another PR
There was a problem hiding this comment.
@codebyNorthsteep, understood! I'll defer to the other PR for the Ticket.assignee mapping and cascade configuration fix. Would you like me to open a tracking GitHub issue as a reminder to ensure the fix is completed there?
codebyNorthsteep
requested review from
Ericthilen,
alicewersen-rgb and
gitnes94
March 31, 2026 20:06
gitnes94
approved these changes
Apr 1, 2026
This was referenced Apr 1, 2026
This was referenced Apr 21, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request refactors several model classes in the
features/form/modelandfeatures/staff/modelpackages to modernize and simplify the codebase. The main changes include the adoption of Lombok annotations to reduce boilerplate, improved validation using Jakarta validation annotations, and the introduction of new relationships between entities. Additionally, a newStaffDTOclass is added for API data transfer.Key changes:
Lombok adoption and code simplification
@Getter,@Setter,@Data,@NoArgsConstructor, and@AllArgsConstructorannotations in model classes (Staff,Consultant,HR,Management,Attachment,ReportForm, and newStaffDTO), and removed manual getter/setter methods. [1] [2] [3] [4] [5] [6] [7]Validation improvements
@NotBlank,@Pattern,@Size,@NotNull,@Email) to model fields inStaff,Consultant,HR,Management, and the newStaffDTOto enforce data integrity and provide clearer error messages. [1] [2] [3] [4] [5]Entity relationships
ReportFormnow has aManyToOnerelationship toStaffand aOneToManyrelationship withAttachment, maintaining attachment order.Staffnow has aOneToManyrelationship withReportForm(asreportForms).DTO introduction
StaffDTOclass for API requests/responses, with validation annotations to ensure data correctness when transferring staff data.Field-level enhancements
These changes collectively improve code maintainability, reduce boilerplate, and enforce stricter data validation across the application.
Summary by CodeRabbit
New Features
Refactor
Chores