Merged
Conversation
Contributor
📝 WalkthroughWalkthroughAdds case record management: new JPA entities ( Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Controller as CaseRecordController
participant Service as CaseRecordService
participant RegistryRepo as RegistryRepository
participant UserRepo as UserRepository
participant SeqRepo as CaseNumberSequenceRepository
participant CaseRepo as CaseRecordRepository
Client->>Controller: POST /api/case-records (request)
Controller->>Service: createCaseRecord(requestDto)
Service->>RegistryRepo: findById(registryId)
RegistryRepo-->>Service: Registry
Service->>UserRepo: findById(ownerUserId)
UserRepo-->>Service: UserEntity (owner)
Service->>UserRepo: findById(assignedUserId)
UserRepo-->>Service: UserEntity (assigned)
Service->>SeqRepo: findWithLockByRegistryAndYear(registry, year)
alt sequence exists
SeqRepo-->>Service: CaseNumberSequence (locked)
else sequence missing
Service->>SeqRepo: saveAndFlush(new CaseNumberSequence(...))
SeqRepo-->>Service: persisted sequence
end
Service->>Service: increment lastValue
Service->>SeqRepo: save(updatedSequence)
SeqRepo-->>Service: persisted
Service->>CaseRepo: save(caseRecord with generated caseNumber)
CaseRepo-->>Service: saved CaseRecord
Service-->>Controller: CaseRecordResponseDto
Controller-->>Client: 201 Created + Location header
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 4❌ Failed checks (1 warning, 3 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/backendlab/team4you/caserecord/CaseRecord.java (1)
38-44:⚠️ Potential issue | 🟠 MajorOwner and assignedUser JPA mappings don't match SQL
NOT NULLconstraints.As noted in the SQL migration review, these fields are
NOT NULLin the database but the JPA mappings allow nulls. Addoptional = falseandnullable = falseto prevent constraint violations at runtime.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java` around lines 38 - 44, The JPA mappings for the CaseRecord class's owner and assignedUser fields currently allow nulls but the DB columns are NOT NULL; update the annotations on both fields (owner and assignedUser) by adding optional = false to the `@ManyToOne` and nullable = false to the `@JoinColumn` to ensure JPA enforces non-nullability and avoids runtime constraint violations.
🧹 Nitpick comments (6)
src/main/java/backendlab/team4you/caserecord/CaseRecord.java (3)
22-23: Redundantunique = truein@Column.The uniqueness is already defined via
@UniqueConstraintin the@Tableannotation (line 13). Having both is not harmful but redundant.✏️ Remove redundant attribute
- `@Column`(name = "case_number", nullable = false, unique = true, length = 50) + `@Column`(name = "case_number", nullable = false, length = 50)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java` around lines 22 - 23, Remove the redundant unique attribute from the `@Column` on the caseNumber field in the CaseRecord class: the uniqueness is already declared via the `@UniqueConstraint` on the `@Table` annotation, so update the field declaration by deleting unique = true from `@Column`(name = "case_number", nullable = false, length = 50) to avoid duplication.
64-82: Constructor doesn't initializecaseNumberwhich is a required field.The constructor accepts all required fields except
caseNumber, which must be set separately viasetCaseNumber(). If forgotten, persisting will fail with a database constraint violation. Consider either:
- Including
caseNumberin the constructor- Documenting that
setCaseNumber()must be called before persistThis design appears intentional (case number generated from
CaseNumberSequence), but the caller must ensure it's set.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java` around lines 64 - 82, The CaseRecord constructor currently leaves the required field caseNumber unset which causes DB constraint failures; update the constructor for CaseRecord to either accept a String caseNumber parameter and assign it to this.caseNumber, or fetch/generate the case number inside the constructor (e.g., via CaseNumberSequence or the existing generator utility) and call this.setCaseNumber(...) before assigning other fields; ensure the constructor version you choose sets this.caseNumber so callers no longer need to remember to call setCaseNumber() separately.
84-97:@PrePersistdefaults won't apply when values are passed via constructor.The constructor sets
statusandconfidentialityLeveldirectly from parameters. If a caller passesnullexplicitly,@PrePersistwill apply defaults. However, if non-null values are passed, they're used as-is.The field initializer
confidentialityLevel = "OPEN"(line 47) combined with the constructor assignment creates a subtle behavior: ifnullis passed to the constructor, the field becomesnull(overwriting the initializer), then@PrePersistresets it to"OPEN". This works but is convoluted.Consider simplifying by using the field initializer consistently and only overwriting in constructor if the parameter is non-null.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java` around lines 84 - 97, The constructor in CaseRecord is overwriting field initializers for status and confidentialityLevel even when callers pass null, creating convoluted behavior with the `@PrePersist` onCreate defaults; update the CaseRecord constructor to only assign this.status and this.confidentialityLevel when the incoming parameters are non-null (leave field initializers intact), so the class-level defaults remain effective and onCreate only handles missing values, and keep the onCreate() `@PrePersist` logic as a safety net.src/main/java/backendlab/team4you/caserecord/CaseNumberSequence.java (2)
16-59: Consider adding@Versionfor optimistic locking.This entity manages sequence numbers and will likely face concurrent access when generating case numbers. Without a
@Versionfield or explicit pessimistic locking in the repository, concurrent updates could result in duplicate case numbers.Options:
- Add
@Version private Long version;for optimistic locking with retry logic- Use
@Lock(LockModeType.PESSIMISTIC_WRITE)on the repository query that fetches the sequence♻️ Option 1: Add optimistic locking
`@Column`(name = "last_value", nullable = false) private Long lastValue; +@Version +private Long version; + protected CaseNumberSequence() {}This requires a corresponding migration to add the
versioncolumn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/caserecord/CaseNumberSequence.java` around lines 16 - 59, Add optimistic locking to the CaseNumberSequence entity by declaring a version field (e.g., add `@Version` private Long version) in the CaseNumberSequence class so JPA will prevent concurrent update races; update persistence migration to add the version column and ensure any service that increments getLastValue()/setLastValue() handles OptimisticLockException with retry logic, or alternatively apply `@Lock`(LockModeType.PESSIMISTIC_WRITE) on the repository method that loads CaseNumberSequence if you prefer pessimistic locking instead.
26-27: Minor: Missing space in annotation.✏️ Formatting fix
- `@Column`(name = "sequence_year",nullable = false) + `@Column`(name = "sequence_year", nullable = false)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/caserecord/CaseNumberSequence.java` around lines 26 - 27, The `@Column` annotation on the year field in class CaseNumberSequence has no space after the comma: update the annotation on the field "year" (the `@Column` on the private Integer year;) to include a space after the comma so it reads with proper formatting (e.g., name = "sequence_year", nullable = false).src/main/resources/db/migration/V3__case_record.sql (1)
22-26: FK constraint naming inconsistency.The user FK constraints use uppercase
FK_CASE_RECORD_ON_*naming while the registry constraint in V4 uses lowercasefk_case_number_sequence_registry. Consider consistent naming conventions across migrations.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/db/migration/V3__case_record.sql` around lines 22 - 26, The FK constraint names in V3 (FK_CASE_RECORD_ON_ASSIGNED_USER and FK_CASE_RECORD_ON_OWNER_USER) are uppercase and inconsistent with the lowercase convention used for fk_case_number_sequence_registry in V4; update the ALTER TABLE statements to use the same lowercase naming style (e.g., fk_case_record_on_assigned_user and fk_case_record_on_owner_user) so all migration FK names follow a consistent convention, and ensure the new names are unique and referenced consistently elsewhere if needed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/resources/db/migration/V3__case_record.sql`:
- Around line 9-10: The DB schema marks owner_user_id and assigned_user_id as
NOT NULL but CaseRecord.java's `@ManyToOne` mappings for owner and assignedUser
are optional; fix the mismatch by either updating the JPA mapping on CaseRecord
(add optional = false to the `@ManyToOne` on owner and assignedUser and add
nullable = false to their `@JoinColumn` annotations) or change the migration
V3__case_record.sql to allow NULL for owner_user_id and assigned_user_id—pick
one approach so the database NOT NULL constraint matches the CaseRecord entity's
nullability.
---
Duplicate comments:
In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java`:
- Around line 38-44: The JPA mappings for the CaseRecord class's owner and
assignedUser fields currently allow nulls but the DB columns are NOT NULL;
update the annotations on both fields (owner and assignedUser) by adding
optional = false to the `@ManyToOne` and nullable = false to the `@JoinColumn` to
ensure JPA enforces non-nullability and avoids runtime constraint violations.
---
Nitpick comments:
In `@src/main/java/backendlab/team4you/caserecord/CaseNumberSequence.java`:
- Around line 16-59: Add optimistic locking to the CaseNumberSequence entity by
declaring a version field (e.g., add `@Version` private Long version) in the
CaseNumberSequence class so JPA will prevent concurrent update races; update
persistence migration to add the version column and ensure any service that
increments getLastValue()/setLastValue() handles OptimisticLockException with
retry logic, or alternatively apply `@Lock`(LockModeType.PESSIMISTIC_WRITE) on the
repository method that loads CaseNumberSequence if you prefer pessimistic
locking instead.
- Around line 26-27: The `@Column` annotation on the year field in class
CaseNumberSequence has no space after the comma: update the annotation on the
field "year" (the `@Column` on the private Integer year;) to include a space after
the comma so it reads with proper formatting (e.g., name = "sequence_year",
nullable = false).
In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java`:
- Around line 22-23: Remove the redundant unique attribute from the `@Column` on
the caseNumber field in the CaseRecord class: the uniqueness is already declared
via the `@UniqueConstraint` on the `@Table` annotation, so update the field
declaration by deleting unique = true from `@Column`(name = "case_number",
nullable = false, length = 50) to avoid duplication.
- Around line 64-82: The CaseRecord constructor currently leaves the required
field caseNumber unset which causes DB constraint failures; update the
constructor for CaseRecord to either accept a String caseNumber parameter and
assign it to this.caseNumber, or fetch/generate the case number inside the
constructor (e.g., via CaseNumberSequence or the existing generator utility) and
call this.setCaseNumber(...) before assigning other fields; ensure the
constructor version you choose sets this.caseNumber so callers no longer need to
remember to call setCaseNumber() separately.
- Around line 84-97: The constructor in CaseRecord is overwriting field
initializers for status and confidentialityLevel even when callers pass null,
creating convoluted behavior with the `@PrePersist` onCreate defaults; update the
CaseRecord constructor to only assign this.status and this.confidentialityLevel
when the incoming parameters are non-null (leave field initializers intact), so
the class-level defaults remain effective and onCreate only handles missing
values, and keep the onCreate() `@PrePersist` logic as a safety net.
In `@src/main/resources/db/migration/V3__case_record.sql`:
- Around line 22-26: The FK constraint names in V3
(FK_CASE_RECORD_ON_ASSIGNED_USER and FK_CASE_RECORD_ON_OWNER_USER) are uppercase
and inconsistent with the lowercase convention used for
fk_case_number_sequence_registry in V4; update the ALTER TABLE statements to use
the same lowercase naming style (e.g., fk_case_record_on_assigned_user and
fk_case_record_on_owner_user) so all migration FK names follow a consistent
convention, and ensure the new names are unique and referenced consistently
elsewhere if needed.
🪄 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: 187d1a63-27f9-49db-9e27-c0bd1c3ed895
📒 Files selected for processing (4)
src/main/java/backendlab/team4you/caserecord/CaseNumberSequence.javasrc/main/java/backendlab/team4you/caserecord/CaseRecord.javasrc/main/resources/db/migration/V3__case_record.sqlsrc/main/resources/db/migration/V4__case_number_sequence.sql
Comment on lines
+9
to
+10
| owner_user_id VARCHAR(255) NOT NULL, | ||
| assigned_user_id VARCHAR(255) NOT NULL, |
Contributor
There was a problem hiding this comment.
Nullability mismatch: SQL requires NOT NULL but JPA mapping is optional.
The owner_user_id and assigned_user_id columns are defined as NOT NULL, but in CaseRecord.java (lines 38-44), the @ManyToOne associations lack optional = false and @JoinColumn lacks nullable = false. This mismatch will cause database constraint violations if the application attempts to persist a CaseRecord with null owner or assignedUser.
🔧 Either fix the JPA mapping or change the SQL
Option 1: Update JPA to match SQL (if users are truly required)
`@ManyToOne`(optional = false, fetch = FetchType.LAZY)
`@JoinColumn`(name = "owner_user_id", nullable = false)
private UserEntity owner;
`@ManyToOne`(optional = false, fetch = FetchType.LAZY)
`@JoinColumn`(name = "assigned_user_id", nullable = false)
private UserEntity assignedUser;Option 2: Update SQL to allow nulls (if users can be optional)
- owner_user_id VARCHAR(255) NOT NULL,
- assigned_user_id VARCHAR(255) NOT NULL,
+ owner_user_id VARCHAR(255),
+ assigned_user_id VARCHAR(255),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| owner_user_id VARCHAR(255) NOT NULL, | |
| assigned_user_id VARCHAR(255) NOT NULL, | |
| owner_user_id VARCHAR(255), | |
| assigned_user_id VARCHAR(255), |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/db/migration/V3__case_record.sql` around lines 9 - 10, The
DB schema marks owner_user_id and assigned_user_id as NOT NULL but
CaseRecord.java's `@ManyToOne` mappings for owner and assignedUser are optional;
fix the mismatch by either updating the JPA mapping on CaseRecord (add optional
= false to the `@ManyToOne` on owner and assignedUser and add nullable = false to
their `@JoinColumn` annotations) or change the migration V3__case_record.sql to
allow NULL for owner_user_id and assigned_user_id—pick one approach so the
database NOT NULL constraint matches the CaseRecord entity's nullability.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/main/java/backendlab/team4you/caserecord/CaseRecord.java (2)
9-23: Redundant unique constraint declaration.The
case_numbercolumn hasunique = truedeclared on both the@Columnannotation (line 22) and the@UniqueConstraintin@Table(line 13). Choose one approach for consistency—typically@UniqueConstraintis preferred when you want to name the constraint explicitly.♻️ Suggested simplification
- `@Column`(name = "case_number", nullable = false, unique = true, length = 50) + `@Column`(name = "case_number", nullable = false, length = 50) private String caseNumber;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java` around lines 9 - 23, The CaseRecord entity currently declares uniqueness for caseNumber twice; remove the redundant unique attribute on the field and keep the named `@UniqueConstraint` on the entity: edit the CaseRecord class to remove unique = true from the `@Column` on caseNumber (but keep nullable = false and length = 50) and retain the `@UniqueConstraint`(name = "uk_case_record_case_number", columnNames = "case_number") on the `@Table` so the constraint is named and managed at the table level.
104-158: Missing setters for mutable business fields.The entity only provides
setCaseNumber()but lacks setters for fields that typically need updates during the case lifecycle:
status– cases transition through states (OPEN → IN_PROGRESS → CLOSED)assignedUser– cases may be reassignedclosedAt– set when closing a casetitle,description– may need correctionsWithout these setters, updating a case record will require reflection or direct field access, which is fragile.
♻️ Suggested setters to add
public void setStatus(String status) { this.status = status; } public void setAssignedUser(UserEntity assignedUser) { this.assignedUser = assignedUser; } public void setClosedAt(LocalDateTime closedAt) { this.closedAt = closedAt; } public void setTitle(String title) { this.title = title; } public void setDescription(String description) { this.description = description; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java` around lines 104 - 158, In CaseRecord, add public setter methods for the mutable business fields so callers can update the entity without reflection: implement setStatus(String status), setAssignedUser(UserEntity assignedUser), setClosedAt(LocalDateTime closedAt), setTitle(String title), and setDescription(String description) in the CaseRecord class (matching the existing field names status, assignedUser, closedAt, title, description); ensure each setter assigns the parameter to the corresponding private field (and optionally update updatedAt if your domain requires).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java`:
- Around line 38-44: Add optional = false to the `@ManyToOne` mappings for the
owner and assignedUser fields in CaseRecord so JPA enforces non-null at the
entity level; update the `@ManyToOne` annotations on the owner and assignedUser
fields (in class CaseRecord) to include optional = false to match the existing
`@JoinColumn`(nullable = false) behavior used for registry.
---
Nitpick comments:
In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java`:
- Around line 9-23: The CaseRecord entity currently declares uniqueness for
caseNumber twice; remove the redundant unique attribute on the field and keep
the named `@UniqueConstraint` on the entity: edit the CaseRecord class to remove
unique = true from the `@Column` on caseNumber (but keep nullable = false and
length = 50) and retain the `@UniqueConstraint`(name =
"uk_case_record_case_number", columnNames = "case_number") on the `@Table` so the
constraint is named and managed at the table level.
- Around line 104-158: In CaseRecord, add public setter methods for the mutable
business fields so callers can update the entity without reflection: implement
setStatus(String status), setAssignedUser(UserEntity assignedUser),
setClosedAt(LocalDateTime closedAt), setTitle(String title), and
setDescription(String description) in the CaseRecord class (matching the
existing field names status, assignedUser, closedAt, title, description); ensure
each setter assigns the parameter to the corresponding private field (and
optionally update updatedAt if your domain requires).
🪄 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: 9e920a9e-8445-4b25-b072-2498c532beb3
📒 Files selected for processing (1)
src/main/java/backendlab/team4you/caserecord/CaseRecord.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/main/java/backendlab/team4you/caserecord/CaseRecord.java (1)
10-14: Avoid duplicate uniqueness declaration forcase_number.
case_numberuniqueness is declared both in@Table(uniqueConstraints=...)and@Column(unique = true). Keep one source of truth to reduce schema drift/confusion.Also applies to: 22-23
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java` around lines 10 - 14, The class CaseRecord currently declares uniqueness for case_number in two places: the `@Table`(uniqueConstraints = {`@UniqueConstraint`(name = "uk_case_record_case_number", columnNames = "case_number")}) and the case_number field's `@Column`(unique = true); remove the redundant declaration to keep a single source of truth — either delete the `@Column`(unique = true) on the case_number field or remove the UniqueConstraint entry in the `@Table` annotation; since the table-level constraint already provides a named constraint (uk_case_record_case_number), prefer removing `@Column`(unique = true) on the case_number field to avoid duplicate constraint definitions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java`:
- Around line 22-23: The caseNumber field in CaseRecord must be validated as
non-null/non-blank before insert and made immutable after initial assignment:
add a `@PrePersist` method (e.g., validateCaseNumberNotBlank) that throws
IllegalStateException if caseNumber is null or blank, remove or restrict the
public setter, or modify setCaseNumber(String caseNumber) to only accept a
non-blank value when this.caseNumber is null (throw IllegalStateException on
attempts to change a previously set value or on blank input); ensure any
constructors that accept caseNumber perform the same non-blank validation so DB
invariants are enforced at the entity level.
- Around line 64-82: The CaseRecord constructor assigns required fields without
validation; update the CaseRecord constructor to validate inputs (use
Objects.requireNonNull for non-nullable objects like registry, owner,
assignedUser if required, and openedAt if it must be present) and validate
string fields (title, description, status, confidentialityLevel) for
non-empty/blank (e.g., trim().isEmpty()) and throw IllegalArgumentException with
clear messages when validations fail; ensure the checks run before assignments
so failures occur fast and reference the constructor and field names (registry,
title, description, status, owner, assignedUser, confidentialityLevel,
openedAt).
---
Nitpick comments:
In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java`:
- Around line 10-14: The class CaseRecord currently declares uniqueness for
case_number in two places: the `@Table`(uniqueConstraints =
{`@UniqueConstraint`(name = "uk_case_record_case_number", columnNames =
"case_number")}) and the case_number field's `@Column`(unique = true); remove the
redundant declaration to keep a single source of truth — either delete the
`@Column`(unique = true) on the case_number field or remove the UniqueConstraint
entry in the `@Table` annotation; since the table-level constraint already
provides a named constraint (uk_case_record_case_number), prefer removing
`@Column`(unique = true) on the case_number field to avoid duplicate constraint
definitions.
🪄 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: 5610ea57-2f97-43ee-9ea5-e009a0589df4
📒 Files selected for processing (1)
src/main/java/backendlab/team4you/caserecord/CaseRecord.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java`:
- Around line 115-127: The getCaseNumber() method currently contains validation
meant for the setter and must be simplified to only return the field; remove the
checks that throw IllegalStateException and the input validations from
getCaseNumber() so it simply returns this.caseNumber. Move the immutability and
input validations into setCaseNumber(String caseNumber): throw
IllegalStateException if this.caseNumber is already set, throw
IllegalArgumentException if the incoming caseNumber is null/blank or longer than
50 characters. Ensure you reference the field caseNumber and the methods
getCaseNumber and setCaseNumber when making the changes.
- Around line 129-131: The validation that prevents changing an already-assigned
caseNumber should be moved out of getCaseNumber() and implemented in
setCaseNumber(String caseNumber) inside the CaseRecord class: in setCaseNumber,
check if this.caseNumber is non-null (already assigned) and the incoming
caseNumber differs, and if so throw an appropriate runtime exception (e.g.,
IllegalStateException) to enforce immutability at the application level; also
include null/blank validation as needed before assignment; remove the validation
from getCaseNumber() so the getter only returns the field.
🪄 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: bdbe5d5b-4aa0-4add-9cb3-61ffb93b6df3
📒 Files selected for processing (1)
src/main/java/backendlab/team4you/caserecord/CaseRecord.java
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (2)
src/main/java/backendlab/team4you/caserecord/CaseRecord.java (2)
115-126:⚠️ Potential issue | 🔴 CriticalRemove setter validation from
getCaseNumber().This is still broken: Line 117 throws whenever
caseNumberis already set, so the getter can never return a persisted value. The null/length checks belong only insetCaseNumber(String).🐛 Proposed fix
public String getCaseNumber() { - - if (this.caseNumber != null) { - throw new IllegalStateException("caseNumber is immutable once set"); - } - if (caseNumber == null || caseNumber.isBlank()) { - throw new IllegalArgumentException("caseNumber is required"); - } - if (caseNumber.length() > 50) { - throw new IllegalArgumentException("caseNumber length must be <= 50"); - } return caseNumber; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java` around lines 115 - 126, The getCaseNumber() method currently contains validation and an immutability check which prevents returning persisted values; move all validation there into the setter: change getCaseNumber() in class CaseRecord to simply return caseNumber, and implement the immutability and input checks inside setCaseNumber(String caseNumber) — i.e. in setCaseNumber(String) throw IllegalStateException if this.caseNumber != null to enforce immutability, and validate the incoming parameter (non-null/non-blank and length <= 50) throwing IllegalArgumentException on violations.
81-85:⚠️ Potential issue | 🟠 MajorBlank
statusandconfidentialityLevelbypass the intended defaults.The constructor accepts
"", but@PrePersistonly normalizesnull. That lets required columnsstatusandconfidentiality_levelpersist as blank strings instead of"OPEN", which creates invalid case states that the DB constraints will not catch.🔧 Proposed fix
- this.status = status; + this.status = (status == null || status.isBlank()) ? null : status; this.owner = Objects.requireNonNull(owner, "owner is required"); this.assignedUser = Objects.requireNonNull(assignedUser, "assignedUser is required"); - this.confidentialityLevel = confidentialityLevel; + this.confidentialityLevel = + (confidentialityLevel == null || confidentialityLevel.isBlank()) ? null : confidentialityLevel; this.openedAt = openedAt; @@ - if (this.status == null) { + if (this.status == null || this.status.isBlank()) { this.status = "OPEN"; } - if (this.confidentialityLevel == null) { + if (this.confidentialityLevel == null || this.confidentialityLevel.isBlank()) { this.confidentialityLevel = "OPEN"; }Also applies to: 98-103
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java` around lines 81 - 85, The constructor assignments for status and confidentialityLevel allow empty strings to bypass defaults; update the CaseRecord constructors to coerce blank strings to null (e.g., replace this.status = status and this.confidentialityLevel = confidentialityLevel with blank-to-null logic) so the existing `@PrePersist` normalization catches them, and also update the method annotated `@PrePersist` to treat blank/empty strings the same as null (use String#isBlank or equivalent) and set the intended default ("OPEN" for status and the class's default confidentiality value) when blank.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java`:
- Around line 115-126: The getCaseNumber() method currently contains validation
and an immutability check which prevents returning persisted values; move all
validation there into the setter: change getCaseNumber() in class CaseRecord to
simply return caseNumber, and implement the immutability and input checks inside
setCaseNumber(String caseNumber) — i.e. in setCaseNumber(String) throw
IllegalStateException if this.caseNumber != null to enforce immutability, and
validate the incoming parameter (non-null/non-blank and length <= 50) throwing
IllegalArgumentException on violations.
- Around line 81-85: The constructor assignments for status and
confidentialityLevel allow empty strings to bypass defaults; update the
CaseRecord constructors to coerce blank strings to null (e.g., replace
this.status = status and this.confidentialityLevel = confidentialityLevel with
blank-to-null logic) so the existing `@PrePersist` normalization catches them, and
also update the method annotated `@PrePersist` to treat blank/empty strings the
same as null (use String#isBlank or equivalent) and set the intended default
("OPEN" for status and the class's default confidentiality value) when blank.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cc7cc9f5-8afa-411d-9d44-e9e5f26f8336
📒 Files selected for processing (1)
src/main/java/backendlab/team4you/caserecord/CaseRecord.java
… fixed flyway migration filenames
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/main/java/backendlab/team4you/caserecord/CaseRecord.java (1)
115-126:⚠️ Potential issue | 🔴 Critical
getCaseNumber()still contains setter validation and never behaves like a getter.Line 117 throws as soon as
caseNumberis populated, and Lines 120-124 validate input that a getter does not receive. The method cannot return a persisted case number.Suggested fix
public String getCaseNumber() { - - if (this.caseNumber != null) { - throw new IllegalStateException("caseNumber is immutable once set"); - } - if (caseNumber == null || caseNumber.isBlank()) { - throw new IllegalArgumentException("caseNumber is required"); - } - if (caseNumber.length() > 50) { - throw new IllegalArgumentException("caseNumber length must be <= 50"); - } return caseNumber; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java` around lines 115 - 126, The getCaseNumber() method currently contains setter-style validation and an immutability check that prevents returning the stored value; replace that logic so getCaseNumber simply returns the field this.caseNumber (or caseNumber) and does not perform null/blank/length checks or throw IllegalStateException; move any required validation and immutability enforcement into the CaseRecord constructor or a dedicated setter/initializer method (e.g., the constructor or a setCaseNumber/initializeCaseNumber method) where you enforce non-null, non-blank and max-length (<=50) and make the field final or guard further mutations.
🧹 Nitpick comments (1)
src/main/resources/db/migration/V5__case_record.sql (1)
22-29: Add indexes for the foreign-key columns.These constraints don't add supporting lookup indexes in the migration.
registry_id,owner_user_id, andassigned_user_idare all likely join/filter columns, so leaving them unindexed will hurtcase_recordreads and FK maintenance as the table grows.Suggested migration follow-up
ALTER TABLE case_record ADD CONSTRAINT FK_CASE_RECORD_ON_REGISTRY FOREIGN KEY (registry_id) REFERENCES registry (id); + +CREATE INDEX idx_case_record_registry_id ON case_record (registry_id); +CREATE INDEX idx_case_record_owner_user_id ON case_record (owner_user_id); +CREATE INDEX idx_case_record_assigned_user_id ON case_record (assigned_user_id);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/db/migration/V5__case_record.sql` around lines 22 - 29, Add supporting indexes for the foreign-key columns on case_record to avoid slow joins and FK maintenance; specifically create indexes on registry_id, owner_user_id, and assigned_user_id (e.g., names like IDX_CASE_RECORD_REGISTRY_ID, IDX_CASE_RECORD_OWNER_USER_ID, IDX_CASE_RECORD_ASSIGNED_USER_ID) immediately after the ALTER TABLE ... ADD CONSTRAINT statements so the FK constraints FK_CASE_RECORD_ON_REGISTRY, FK_CASE_RECORD_ON_OWNER_USER and FK_CASE_RECORD_ON_ASSIGNED_USER have supporting lookup indexes for efficient reads and deletes/updates.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java`:
- Around line 65-85: The CaseRecord constructor currently leaves caseNumber
unset which conflicts with the `@PrePersist` requirement and the lack of
allocation logic in CaseNumberSequence; either (A) add caseNumber as a required
parameter to the CaseRecord(...) constructor and assign it there (so callers
supply the allocated number), or (B) centralize allocation by creating/using a
CaseNumber allocation service/factory that reads/increments
CaseNumberSequence.lastValue and calls CaseRecord.setCaseNumber(...) before
saving (ensure all creation paths use that service), referencing the CaseRecord
constructor, setCaseNumber(), and CaseNumberSequence to locate the changes.
---
Duplicate comments:
In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java`:
- Around line 115-126: The getCaseNumber() method currently contains
setter-style validation and an immutability check that prevents returning the
stored value; replace that logic so getCaseNumber simply returns the field
this.caseNumber (or caseNumber) and does not perform null/blank/length checks or
throw IllegalStateException; move any required validation and immutability
enforcement into the CaseRecord constructor or a dedicated setter/initializer
method (e.g., the constructor or a setCaseNumber/initializeCaseNumber method)
where you enforce non-null, non-blank and max-length (<=50) and make the field
final or guard further mutations.
---
Nitpick comments:
In `@src/main/resources/db/migration/V5__case_record.sql`:
- Around line 22-29: Add supporting indexes for the foreign-key columns on
case_record to avoid slow joins and FK maintenance; specifically create indexes
on registry_id, owner_user_id, and assigned_user_id (e.g., names like
IDX_CASE_RECORD_REGISTRY_ID, IDX_CASE_RECORD_OWNER_USER_ID,
IDX_CASE_RECORD_ASSIGNED_USER_ID) immediately after the ALTER TABLE ... ADD
CONSTRAINT statements so the FK constraints FK_CASE_RECORD_ON_REGISTRY,
FK_CASE_RECORD_ON_OWNER_USER and FK_CASE_RECORD_ON_ASSIGNED_USER have supporting
lookup indexes for efficient reads and deletes/updates.
🪄 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: 94712f90-27c0-4f69-ad1a-b68af58eb33b
📒 Files selected for processing (4)
pom.xmlsrc/main/java/backendlab/team4you/caserecord/CaseRecord.javasrc/main/resources/db/migration/V5__case_record.sqlsrc/main/resources/db/migration/V6__case_number_sequence.sql
✅ Files skipped from review due to trivial changes (1)
- pom.xml
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/backendlab/team4you/caserecord/CaseRecord.java (1)
115-127:⚠️ Potential issue | 🔴 CriticalCritical:
getCaseNumber()still contains broken validation logic.Despite being marked as addressed, this getter still has misplaced validation that makes it unusable:
- Line 117-118: Throws if
caseNumberis not null—meaning you can never read a set value- Lines 120-125: Validation logic belongs in the setter, not getter
- Result: This method always throws an exception regardless of field state
🐛 Proposed fix: simplify getter to only return the field
public String getCaseNumber() { - - if (this.caseNumber != null) { - throw new IllegalStateException("caseNumber is immutable once set"); - } - if (caseNumber == null || caseNumber.isBlank()) { - throw new IllegalArgumentException("caseNumber is required"); - } - if (caseNumber.length() > 50) { - throw new IllegalArgumentException("caseNumber length must be <= 50"); - } return caseNumber; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java` around lines 115 - 127, The getCaseNumber() method currently contains misplaced validation and an inverted immutability check; replace its body to simply return the caseNumber field (i.e., remove all throws/validation from getCaseNumber()). Move the immutability check (if this.caseNumber != null -> throw) and the null/blank/length validations (caseNumber null/blank and length > 50) into the setter or constructor responsible for assigning the field (e.g., setCaseNumber(String) or the class constructor), ensuring that setCaseNumber enforces immutability and input constraints while getCaseNumber only returns the stored value.
🧹 Nitpick comments (1)
src/main/java/backendlab/team4you/caserecord/CaseRecord.java (1)
65-86: Consider validatingstatusandconfidentialityLevelin constructor.The constructor validates
registry,title,owner, andassignedUserbut acceptsstatusandconfidentialityLevelwithout validation. While@PrePersistdefaultsnullvalues to"OPEN", a blank string like""would bypass the null check and persist an invalid value (since the DB column is NOT NULL but doesn't enforce non-blank).♻️ Optional: add blank check for status/confidentialityLevel
this.description = description; - this.status = status; + this.status = (status == null || status.isBlank()) ? null : status; this.owner = Objects.requireNonNull(owner, "owner is required"); this.assignedUser = Objects.requireNonNull(assignedUser, "assignedUser is required"); - this.confidentialityLevel = confidentialityLevel; + this.confidentialityLevel = (confidentialityLevel == null || confidentialityLevel.isBlank()) ? null : confidentialityLevel; this.openedAt = openedAt;This normalizes blank strings to
nullso@PrePersistcan apply defaults.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java` around lines 65 - 86, The CaseRecord constructor currently accepts blank strings for status and confidentialityLevel which can bypass `@PrePersist` defaults; update the CaseRecord constructor to validate status and confidentialityLevel (e.g., treat null or blank "" as null so `@PrePersist` can apply defaults, or enforce non-blank values) by normalizing blank values before assigning to this.status and this.confidentialityLevel; reference the CaseRecord constructor and the status/confidentialityLevel fields when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java`:
- Around line 115-127: The getCaseNumber() method currently contains misplaced
validation and an inverted immutability check; replace its body to simply return
the caseNumber field (i.e., remove all throws/validation from getCaseNumber()).
Move the immutability check (if this.caseNumber != null -> throw) and the
null/blank/length validations (caseNumber null/blank and length > 50) into the
setter or constructor responsible for assigning the field (e.g.,
setCaseNumber(String) or the class constructor), ensuring that setCaseNumber
enforces immutability and input constraints while getCaseNumber only returns the
stored value.
---
Nitpick comments:
In `@src/main/java/backendlab/team4you/caserecord/CaseRecord.java`:
- Around line 65-86: The CaseRecord constructor currently accepts blank strings
for status and confidentialityLevel which can bypass `@PrePersist` defaults;
update the CaseRecord constructor to validate status and confidentialityLevel
(e.g., treat null or blank "" as null so `@PrePersist` can apply defaults, or
enforce non-blank values) by normalizing blank values before assigning to
this.status and this.confidentialityLevel; reference the CaseRecord constructor
and the status/confidentialityLevel fields when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9df7076a-092c-4b16-8af4-9ec780bdd429
📒 Files selected for processing (4)
pom.xmlsrc/main/java/backendlab/team4you/caserecord/CaseRecord.javasrc/main/resources/db/migration/V5__case_record.sqlsrc/main/resources/db/migration/V6__case_number_sequence.sql
✅ Files skipped from review due to trivial changes (1)
- pom.xml
# Conflicts: # src/main/java/backendlab/team4you/user/UserEntity.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
src/main/java/backendlab/team4you/registry/Registry.java (1)
20-31: Centralize regex/message to avoid drift between annotation and constructor.The same validation rule is duplicated in two places. A shared constant reduces maintenance risk.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/registry/Registry.java` around lines 20 - 31, The code duplicates the same regex/message in the `@Pattern` on field code and in the Registry(String name, String code) constructor; introduce a single constant (e.g., CODE_REGEX and optionally CODE_MESSAGE) in the Registry class and reference that constant in both the `@Pattern` annotation and the constructor validation so the rule/message are centralized and cannot drift (update the `@Pattern` to use the constant and replace the hard-coded regex and message in the constructor throw).src/test/java/backendlab/team4you/caserecord/CaseRecordServiceTest.java (1)
89-132: Good exception path coverage.Both
RegistryNotFoundExceptionandUserNotFoundExceptionscenarios are tested with appropriate message assertions.Consider adding a test for when
assignedUseris not found to ensure complete coverage of allUserNotFoundExceptionpaths, and a test for new sequence creation whenfindWithLockByRegistryAndYearreturns empty.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/backendlab/team4you/caserecord/CaseRecordServiceTest.java` around lines 89 - 132, Add two tests: one that constructs a CaseRecordRequestDto where the assigned user id is missing (e.g., "missing-assigned"), stub registryRepository.findById(...) to return the Registry and stub userRepository.findById("missing-assigned") to return Optional.empty(), then assert caseRecordService.createCaseRecord(requestDto) throws UserNotFoundException with message "user not found: missing-assigned"; and another that stubs sequenceRepository.findWithLockByRegistryAndYear(registry, year) to return Optional.empty(), calls caseRecordService.createCaseRecord(requestDto) and verifies the code path that creates a new sequence (e.g., verify sequenceRepository.save(...) was called or the returned CaseRecord has the expected newly initialized sequence/number), using existing fixtures like Registry, CaseRecordRequestDto, registryRepository, sequenceRepository and caseRecordService.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@src/main/java/backendlab/team4you/caserecord/CaseNumberSequenceRepository.java`:
- Around line 14-15: allocateNextCaseNumber must handle the race where no
(registry, year) row exists so two threads try to insert the same row and the
second gets a DB constraint error; update allocateNextCaseNumber to attempt the
existing logic (call findWithLockByRegistryAndYear) and when Optional.empty()
create-and-save the new CaseNumberSequence inside a try/catch that catches
DataIntegrityViolationException (or ConstraintViolationException), and on catch
re-query findWithLockByRegistryAndYear (inside the same transaction or a new
retry transaction), obtain the locked row and proceed to increment/return the
next number—alternatively implement a small retry loop that on conflict retries
the read-with-lock → increment → save path so the unique constraint
uk_case_number_sequence_registry_year no longer results in an unhandled
exception.
In `@src/main/java/backendlab/team4you/caserecord/CaseRecordRequestDto.java`:
- Around line 23-27: Replace the `@NotNull` annotations on the string ID fields in
CaseRecordRequestDto with `@NotBlank` to reject empty/whitespace IDs at validation
time; update the annotations on ownerUserId and assignedUserId (in class
CaseRecordRequestDto) to use javax.validation.constraints.NotBlank (and add the
import if missing) so requests with blank IDs return 400 instead of allowing
empty values that lead to downstream 404s.
In `@src/main/java/backendlab/team4you/caserecord/CaseRecordService.java`:
- Around line 63-75: allocateNextCaseNumber can race when orElseGet creates a
new CaseNumberSequence without a DB lock; modify allocateNextCaseNumber to
handle this by attempting the current flow (use
caseNumberSequenceRepository.findWithLockByRegistryAndYear and save) but wrap
the save in a try/catch for the unique constraint/DataIntegrityViolation (or
ConstraintViolationException), and on catch re-query
caseNumberSequenceRepository.findWithLockByRegistryAndYear (now expecting it to
exist) and use that locked entity to compute nextValue and call buildCaseNumber;
reference allocateNextCaseNumber, findWithLockByRegistryAndYear,
CaseNumberSequence, caseNumberSequenceRepository, and buildCaseNumber in the
change.
In `@src/main/java/backendlab/team4you/exceptions/ApiExceptionHandler.java`:
- Around line 55-62: The catch-all method handleGenericException in
ApiExceptionHandler currently discards the Exception; update it to log the
exception before returning the sanitized response by calling the class logger
(e.g., logger.error(..., exception)) or the existing logging mechanism used in
this class to record the error message and stack trace, then return
buildErrorResponse as before; ensure you reference the same logger instance (or
add one if missing) so the original exception is recorded for debugging while
the response remains unchanged.
In `@src/main/java/backendlab/team4you/registry/Registry.java`:
- Around line 28-33: The Registry constructor currently calls code.matches(...)
which throws NullPointerException for null input; update the Registry(String
name, String code) constructor to check for null first (e.g., if (code == null)
throw new IllegalArgumentException("code must not be null and must be exactly 2
uppercase letters")) before performing the regex validation, then keep the
existing regex check for "[A-Z]{2}" and assign to this.code only after both
validations pass.
---
Nitpick comments:
In `@src/main/java/backendlab/team4you/registry/Registry.java`:
- Around line 20-31: The code duplicates the same regex/message in the `@Pattern`
on field code and in the Registry(String name, String code) constructor;
introduce a single constant (e.g., CODE_REGEX and optionally CODE_MESSAGE) in
the Registry class and reference that constant in both the `@Pattern` annotation
and the constructor validation so the rule/message are centralized and cannot
drift (update the `@Pattern` to use the constant and replace the hard-coded regex
and message in the constructor throw).
In `@src/test/java/backendlab/team4you/caserecord/CaseRecordServiceTest.java`:
- Around line 89-132: Add two tests: one that constructs a CaseRecordRequestDto
where the assigned user id is missing (e.g., "missing-assigned"), stub
registryRepository.findById(...) to return the Registry and stub
userRepository.findById("missing-assigned") to return Optional.empty(), then
assert caseRecordService.createCaseRecord(requestDto) throws
UserNotFoundException with message "user not found: missing-assigned"; and
another that stubs sequenceRepository.findWithLockByRegistryAndYear(registry,
year) to return Optional.empty(), calls
caseRecordService.createCaseRecord(requestDto) and verifies the code path that
creates a new sequence (e.g., verify sequenceRepository.save(...) was called or
the returned CaseRecord has the expected newly initialized sequence/number),
using existing fixtures like Registry, CaseRecordRequestDto, registryRepository,
sequenceRepository and caseRecordService.
🪄 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: 7eca0171-e00c-4508-bc33-4ba5ad3d5a76
📒 Files selected for processing (23)
pom.xmlsrc/main/java/backendlab/team4you/caserecord/CaseNumberSequence.javasrc/main/java/backendlab/team4you/caserecord/CaseNumberSequenceRepository.javasrc/main/java/backendlab/team4you/caserecord/CaseRecord.javasrc/main/java/backendlab/team4you/caserecord/CaseRecordController.javasrc/main/java/backendlab/team4you/caserecord/CaseRecordRepository.javasrc/main/java/backendlab/team4you/caserecord/CaseRecordRequestDto.javasrc/main/java/backendlab/team4you/caserecord/CaseRecordResponseDto.javasrc/main/java/backendlab/team4you/caserecord/CaseRecordService.javasrc/main/java/backendlab/team4you/controller/SignupController.javasrc/main/java/backendlab/team4you/exceptions/ApiExceptionHandler.javasrc/main/java/backendlab/team4you/exceptions/GlobalExceptionHandler.javasrc/main/java/backendlab/team4you/exceptions/RegistryNotFoundException.javasrc/main/java/backendlab/team4you/registry/Registry.javasrc/main/java/backendlab/team4you/registry/RegistryRepository.javasrc/main/java/backendlab/team4you/user/UserEntity.javasrc/main/java/backendlab/team4you/user/UserRepository.javasrc/main/java/backendlab/team4you/user/UserService.javasrc/main/resources/db/migration/V7__case_record.sqlsrc/main/resources/db/migration/V8__case_number_sequence.sqlsrc/test/java/backendlab/team4you/caserecord/CaseRecordControllerTest.javasrc/test/java/backendlab/team4you/caserecord/CaseRecordServiceTest.javasrc/test/java/backendlab/team4you/caserecord/CaseRecordTest.java
💤 Files with no reviewable changes (2)
- src/main/java/backendlab/team4you/user/UserService.java
- src/main/java/backendlab/team4you/exceptions/GlobalExceptionHandler.java
✅ Files skipped from review due to trivial changes (6)
- src/main/java/backendlab/team4you/controller/SignupController.java
- src/main/java/backendlab/team4you/user/UserEntity.java
- src/main/resources/db/migration/V8__case_number_sequence.sql
- src/main/java/backendlab/team4you/caserecord/CaseRecordResponseDto.java
- src/main/java/backendlab/team4you/registry/RegistryRepository.java
- src/main/resources/db/migration/V7__case_record.sql
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/backendlab/team4you/caserecord/CaseNumberSequence.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
src/test/java/backendlab/team4you/caserecord/CaseRecordServiceTest.java (2)
111-133: Consider adding a test for assigned user not found.This test covers the owner lookup failure, but the service also validates the
assignedUserand throwsUserNotFoundExceptionif not found (see context snippet 1, line 44). Adding a similar test would ensure full coverage of all lookup failure paths.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/backendlab/team4you/caserecord/CaseRecordServiceTest.java` around lines 111 - 133, Add a new unit test in CaseRecordServiceTest that mirrors shouldThrowWhenOwnerDoesNotExist but targets the assigned user lookup: construct a CaseRecordRequestDto with a valid owner id but set assignedUser to a missing id (e.g., "missing-assigned"), mock registryRepository.findById(1L) to return a Registry and mock userRepository.findById("missing-assigned") to return Optional.empty(), then assert that calling caseRecordService.createCaseRecord(requestDto) throws UserNotFoundException with message "user not found: missing-assigned"; reference the createCaseRecord method, CaseRecordRequestDto, userRepository.findById and UserNotFoundException to locate the relevant code.
221-225: Remove redundant fully qualified class name.
assertThatThrownByis already statically imported at line 22 (import static org.assertj.core.api.Assertions.*;). This is inconsistent with lines 106 and 130 where it's used without qualification.🧹 Suggested fix
- org.assertj.core.api.Assertions.assertThatThrownBy(() -> + assertThatThrownBy(() -> caseRecordService.createCaseRecord(requestDto) )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/backendlab/team4you/caserecord/CaseRecordServiceTest.java` around lines 221 - 225, Replace the redundant fully qualified call org.assertj.core.api.Assertions.assertThatThrownBy(...) with the existing static import usage assertThatThrownBy(...) to match other assertions in the test; keep the same lambda (caseRecordService.createCaseRecord(requestDto)) and the chained assertions (.isInstanceOf(IllegalStateException.class).hasMessageContaining("failed to allocate case number after 3 attempts")) so only the qualifier is removed for consistency with the static import.
🤖 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/test/java/backendlab/team4you/caserecord/CaseRecordServiceTest.java`:
- Around line 63-73: The test is time-dependent because CaseRecordService uses
LocalDateTime.now(), so inject a java.time.Clock into CaseRecordService (add a
constructor parameter or field) and replace LocalDateTime.now() calls with
LocalDateTime.now(clock) (e.g., in createCaseRecord and any case-number
generation logic); update production wiring to pass Clock.systemDefaultZone()
and update CaseRecordServiceTest to provide a fixed Clock (e.g.,
Clock.fixed(Instant.parse("2026-01-01T00:00:00Z"), ZoneId.systemDefault())) so
the expected case number "KS26-1" remains deterministic.
---
Nitpick comments:
In `@src/test/java/backendlab/team4you/caserecord/CaseRecordServiceTest.java`:
- Around line 111-133: Add a new unit test in CaseRecordServiceTest that mirrors
shouldThrowWhenOwnerDoesNotExist but targets the assigned user lookup: construct
a CaseRecordRequestDto with a valid owner id but set assignedUser to a missing
id (e.g., "missing-assigned"), mock registryRepository.findById(1L) to return a
Registry and mock userRepository.findById("missing-assigned") to return
Optional.empty(), then assert that calling
caseRecordService.createCaseRecord(requestDto) throws UserNotFoundException with
message "user not found: missing-assigned"; reference the createCaseRecord
method, CaseRecordRequestDto, userRepository.findById and UserNotFoundException
to locate the relevant code.
- Around line 221-225: Replace the redundant fully qualified call
org.assertj.core.api.Assertions.assertThatThrownBy(...) with the existing static
import usage assertThatThrownBy(...) to match other assertions in the test; keep
the same lambda (caseRecordService.createCaseRecord(requestDto)) and the chained
assertions
(.isInstanceOf(IllegalStateException.class).hasMessageContaining("failed to
allocate case number after 3 attempts")) so only the qualifier is removed for
consistency with the static import.
🪄 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: 6d0a585e-7b25-459e-b0c4-5d0c44411b93
📒 Files selected for processing (5)
src/main/java/backendlab/team4you/caserecord/CaseRecordRequestDto.javasrc/main/java/backendlab/team4you/caserecord/CaseRecordService.javasrc/main/java/backendlab/team4you/exceptions/ApiExceptionHandler.javasrc/main/java/backendlab/team4you/registry/Registry.javasrc/test/java/backendlab/team4you/caserecord/CaseRecordServiceTest.java
✅ Files skipped from review due to trivial changes (1)
- src/main/java/backendlab/team4you/caserecord/CaseRecordRequestDto.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/main/java/backendlab/team4you/registry/Registry.java
- src/main/java/backendlab/team4you/caserecord/CaseRecordService.java
- src/main/java/backendlab/team4you/exceptions/ApiExceptionHandler.java
Comment on lines
+63
to
+73
| CaseNumberSequence sequence = new CaseNumberSequence(registry, 2026, 0L); | ||
|
|
||
| when(registryRepository.findById(1L)).thenReturn(Optional.of(registry)); | ||
| when(userRepository.findById(owner.getIdAsString())).thenReturn(Optional.of(owner)); | ||
| when(userRepository.findById(assignedUser.getIdAsString())).thenReturn(Optional.of(assignedUser)); | ||
| when(caseNumberSequenceRepository.findWithLockByRegistryAndYear(any(), any())).thenReturn(Optional.of(sequence)); | ||
| when(caseRecordRepository.save(any(CaseRecord.class))).thenAnswer(invocation -> invocation.getArgument(0)); | ||
|
|
||
| CaseRecordResponseDto response = caseRecordService.createCaseRecord(requestDto); | ||
|
|
||
| assertThat(response.caseNumber()).isEqualTo("KS26-1"); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if CaseRecordService has Clock injection capability
rg -n "Clock" --type=java -C3 -g '*CaseRecordService*'Repository: ithsjava25/project-backend-team4you
Length of output: 61
🏁 Script executed:
# Find and examine CaseRecordService
fd -t f 'CaseRecordService\.java' --type f | head -5Repository: ithsjava25/project-backend-team4you
Length of output: 146
🏁 Script executed:
# Search for LocalDateTime usage in the service
rg -n "LocalDateTime\.now()" --type=java | grep -i caseRepository: ithsjava25/project-backend-team4you
Length of output: 423
🏁 Script executed:
# Look at the buildCaseNumber method
rg -n "buildCaseNumber" --type=java -A 10Repository: ithsjava25/project-backend-team4you
Length of output: 1701
🏁 Script executed:
# View the test file at lines 44-88 to see the exact assertion
sed -n '44,88p' src/test/java/backendlab/team4you/caserecord/CaseRecordServiceTest.javaRepository: ithsjava25/project-backend-team4you
Length of output: 2404
🏁 Script executed:
# Check what the registry code is set to in the test setup
rg -n "registry\.setCode\|registry = new\|registry\.getCode" src/test/java/backendlab/team4you/caserecord/CaseRecordServiceTest.java -B2 -A2Repository: ithsjava25/project-backend-team4you
Length of output: 61
Time-dependent test will fail after 2026.
The service uses LocalDateTime.now().getYear() to build the case number, but this test hardcodes the expected year as "KS26-1". When run in 2027 or later, the assertion will fail because the service will generate "KS27-1" (or later).
Consider injecting a java.time.Clock into CaseRecordService that can be mocked in tests. This allows deterministic testing regardless of when the test runs.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/test/java/backendlab/team4you/caserecord/CaseRecordServiceTest.java`
around lines 63 - 73, The test is time-dependent because CaseRecordService uses
LocalDateTime.now(), so inject a java.time.Clock into CaseRecordService (add a
constructor parameter or field) and replace LocalDateTime.now() calls with
LocalDateTime.now(clock) (e.g., in createCaseRecord and any case-number
generation logic); update production wiring to pass Clock.systemDefaultZone()
and update CaseRecordServiceTest to provide a fixed Clock (e.g.,
Clock.fixed(Instant.parse("2026-01-01T00:00:00Z"), ZoneId.systemDefault())) so
the expected case number "KS26-1" remains deterministic.
gvaguirres
approved these changes
Apr 10, 2026
This was referenced Apr 10, 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
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.
resolves #10
Summary by CodeRabbit
New Features
Tests
Chores
Bug Fixes