feat: secure visa editing flow and refine applicant notifications#58
Merged
Conversation
-Logic: Restricted visa editing to only 'SUBMITTED' and 'INCOMPLETE' statuses in VisaViewController. - Security: Added ownership checks to prevent unauthorized editing of visa cases. - Bug Fix: Fixed a bug where 'Action Required' was incorrectly displayed for submitted cases. - Data: Updated DataInitializer with realistic test scenarios for incomplete and submitted cases. - Test: Updated test to match new production code
📝 WalkthroughWalkthroughReplaces the startup user-only seeding with a combined data initializer that seeds visas, comments, and audit logs; simplifies MinIO bucket initialization by removing CORS setup; tightens edit/update access in the visa controller with status and ownership checks; updates a properties formatting line; and adjusts tests accordingly. (48 words) Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application<br/>Startup
participant DI as DataInitializer
participant UR as UserRepository
participant PE as PasswordEncoder
participant VR as VisaRepository
participant CR as CommentRepository
participant CLS as CommentLogService
participant FLS as FileLogService
App->>DI: instantiate initData CommandLineRunner
DI->>UR: getOrCreateUser(username, pw)
UR->>PE: encode(password)
PE-->>UR: encodedPassword
UR-->>DI: user saved/retrieved
DI->>VR: visaRepository.count()
alt count == 0
DI->>VR: save Visa(s) (various statuses/types)
VR-->>DI: visas persisted
DI->>CR: save comment(s) for visa(s)
CR-->>DI: comment(s) persisted
DI->>CLS: create comment audit log(s)
CLS-->>DI: audit log(s) persisted
DI->>FLS: create file upload audit log(s) for applicable visa
FLS-->>DI: file log(s) persisted
end
DI-->>App: CommandLineRunner completed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java (1)
209-214:⚠️ Potential issue | 🟡 Minor
statusInformationmissing from theIOExceptionre-render path.In the
IOExceptioncatch the model getsvisa, the apply-model basics, andisEdit, but unlike theIllegalArgumentExceptionpath (line 226) it omitsmodel.addAttribute("statusInformation", visa.statusInformation());. Ifedit-form.html's "Feedback from Handler" banner is gated onstatusInformation, an upload failure will hide it. Add the same attribute here for consistency.🛠️ Proposed fix
} catch (java.io.IOException e) { bindingResult.reject("upload.error", "Failed to upload new document: " + e.getMessage()); model.addAttribute("visa", visa); prepareApplyModel(principal.getUserId(), model); model.addAttribute("isEdit", true); + model.addAttribute("statusInformation", visa.statusInformation()); return "visa/edit-form";Based on learnings: prior PR review on this controller flagged the same omission for the
IllegalArgumentExceptionbranch (now fixed at line 226); the same fix should apply uniformly to all error-path re-renders.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java` around lines 209 - 214, In VisaViewController inside the catch (java.io.IOException e) path of the document-upload handling method, add model.addAttribute("statusInformation", visa.statusInformation()); before preparing the apply model/return so the same statusInformation attribute set in the IllegalArgumentException branch is present on re-render; this ensures the "Feedback from Handler" banner in edit-form.html is shown consistently after upload failures.src/main/resources/templates/visa/details.html (1)
181-195:⚠️ Potential issue | 🟠 MajorExpose edit entry point for SUBMITTED visa applicants.
The controller allows edits when
visaStatus ∈ {INCOMPLETE, SUBMITTED}(lines 145–146), but applicants have no UI path to the edit form for SUBMITTED cases.my-applications.htmlhas only a "Details" link, anddetails.htmlgates the edit button to INCOMPLETE only (line 181). Add an edit link tomy-applications.html(e.g., conditional on status being INCOMPLETE or SUBMITTED) or adjust the banner condition indetails.htmlto include SUBMITTED.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/visa/details.html` around lines 181 - 195, Update the UI so applicants with SUBMITTED visas can reach the edit form: modify the conditional in details.html that currently checks "visa.visaStatus.name() == 'INCOMPLETE' and visa.applicantId == currentUser.id" to allow SUBMITTED as well (e.g., check for INCOMPLETE or SUBMITTED), or alternatively add an Edit link in my-applications.html conditioned on visa.visaStatus.name() being INCOMPLETE or SUBMITTED and visa.applicantId == currentUser.id; ensure the th:href remains "@{/visa/{id}/edit(id=${visa.id})}" so the controller's existing edit handling for statuses INCOMPLETE and SUBMITTED is used.
🧹 Nitpick comments (5)
src/main/java/org/example/visacasemanagementsystem/config/DataInitializer.java (2)
27-27: Replace fully-qualified class names with imports.
CommentRepository,Comment, andLocalDateTimeare referenced via FQN. Importing them keeps signatures and method bodies readable.♻️ Proposed cleanup
import java.time.LocalDate; +import java.time.LocalDateTime; + +import org.example.visacasemanagementsystem.comment.entity.Comment; +import org.example.visacasemanagementsystem.comment.repository.CommentRepository; @@ - org.example.visacasemanagementsystem.comment.repository.CommentRepository commentRepository, + CommentRepository commentRepository, @@ - private void saveCommentAndLog(org.example.visacasemanagementsystem.comment.repository.CommentRepository commentRepo, + private void saveCommentAndLog(CommentRepository commentRepo, CommentLogService logService, User author, Visa visa, String text) { - org.example.visacasemanagementsystem.comment.entity.Comment c = new org.example.visacasemanagementsystem.comment.entity.Comment(); + Comment c = new Comment(); c.setVisa(visa); c.setAuthor(author); c.setText(text); - c.setCreatedAt(java.time.LocalDateTime.now()); + c.setCreatedAt(LocalDateTime.now());Also applies to: 69-78
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/config/DataInitializer.java` at line 27, Replace the fully-qualified type usages in DataInitializer with normal imports: add import statements for org.example.visacasemanagementsystem.comment.repository.CommentRepository, org.example.visacasemanagementsystem.comment.model.Comment (or the correct Comment class), and java.time.LocalDateTime at the top of the file, then update all occurrences in the DataInitializer class (constructor parameters and any method bodies, e.g., where CommentRepository, Comment, and LocalDateTime are referenced around the constructor and the initialization block at lines ~69-78) to use the simple class names instead of fully-qualified names to improve readability.
40-40: Use the SLF4J logger instead ofSystem.out.println.
FileServiceand other classes already use@Slf4j; aligning here keeps log levels, formatting, and routing consistent (and avoids stdout noise in tests).♻️ Proposed cleanup
+import lombok.extern.slf4j.Slf4j; + `@Configuration` +@Slf4j public class DataInitializer { @@ - System.out.println("--- Starting generation of test data ---"); + log.info("--- Starting generation of test data ---"); @@ - System.out.println("--- Test data generated successfully! ---"); + log.info("--- Test data generated successfully! ---");Also applies to: 64-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/config/DataInitializer.java` at line 40, Replace the System.out.println calls in DataInitializer with SLF4J logging: add or use the class-level logger (e.g., annotate DataInitializer with `@Slf4j` or obtain a Logger) and change the prints to log.info(...) (or appropriate level) for the messages such as the "--- Starting generation of test data ---" and the second occurrence around line 64 so logging is consistent with other classes like FileService.src/main/java/org/example/visacasemanagementsystem/file/FileService.java (2)
67-69: Stale "TA BORT ELLER KOMMENTERA BORT" instruction comment.These lines are an internal note telling future-you to remove a CORS try-block that's already been removed. The comment is no longer actionable and clutters the method. Suggest replacing with a single concise English log line (or just remove).
♻️ Proposed cleanup
- // --- VIKTIGT: TA BORT ELLER KOMMENTERA BORT HELA TRY-BLOCKET FÖR CORS --- - // Det är detta block som orsakar 501 Not Implemented i MinIO - log.info("Bucket initialization complete. Skipping programmatic CORS config for local MinIO compatibility."); + log.info("Bucket initialization complete. Skipping programmatic CORS config for local MinIO compatibility.");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/file/FileService.java` around lines 67 - 69, Remove the stale Swedish instruction comment in FileService around the bucket/CORS section and replace it with a single concise English log entry (or delete the log line entirely) to avoid clutter; locate the log.info call that currently reads "Bucket initialization complete. Skipping programmatic CORS config for local MinIO compatibility." inside the FileService class (bucket initialization method) and either keep a short English message like "Bucket initialization complete; programmatic CORS disabled for local MinIO." or remove the redundant log statement.
37-38: Remove the unusedcorsAllowedOriginsfield and property.After removing the CORS configuration block from
initializeBucket(), this field is no longer referenced anywhere in the codebase. Remove the field declaration and its corresponding property fromapplication.propertiesto eliminate dead code and confusion.♻️ Proposed cleanup
- `@Value`("${minio.corsAllowedOrigins:http://localhost:8080}") - private String corsAllowedOrigins; - public FileService(S3Client s3Client, S3Presigner s3Presigner) {Also remove from
src/main/resources/application.properties:-minio.corsAllowedOrigins=${MINIO_CORS:http://localhost:8080}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/file/FileService.java` around lines 37 - 38, Remove the dead field corsAllowedOrigins from the FileService class and delete its corresponding property from application.properties; specifically, delete the field declaration "@Value(\"${minio.corsAllowedOrigins:http://localhost:8080}\") private String corsAllowedOrigins;" from FileService and remove the minio.corsAllowedOrigins entry in src/main/resources/application.properties so there are no unused CORS-related symbols remaining.src/main/resources/application.properties (1)
19-19:minio.corsAllowedOriginsis now dead config.
FileService.initializeBucket()no longer reads/uses CORS origins after the programmatic CORS block was removed. ThecorsAllowedOriginsproperty here and the matching@Valuefield inFileService(lines 37–38) can be removed unless reintroduced elsewhere.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/application.properties` at line 19, Remove the dead CORS config and its unused field: delete the minio.corsAllowedOrigins entry from application.properties and remove the corresponding `@Value-annotated` field (the corsAllowedOrigins/string field) from the FileService class and any now-unused imports; also verify FileService.initializeBucket() and other methods no longer reference that field and adjust or remove any commented/leftover CORS logic if present.
🤖 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/visacasemanagementsystem/config/DataInitializer.java`:
- Around line 22-67: The DataInitializer's initData CommandLineRunner currently
runs in all environments; wrap it with a Spring profile guard so it only runs in
non-production environments — e.g., annotate the bean (or the DataInitializer
class) with `@Profile`({"dev","test"}) (or `@Profile`("!prod") if you prefer) to
prevent upserting users and seeding visas in production; update imports to
org.springframework.context.annotation.Profile and keep getOrCreateUser,
initData, and the existing PasswordEncoder usage unchanged.
- Line 102: Update the seeded nationality value in DataInitializer by changing
the call to v.setNationality(...) from "Sweden" to the demonym "Swedish" so it
matches service-layer expectations and VisaServiceIntegrationTest; locate the
v.setNationality("Sweden") invocation in the DataInitializer class (the Visa
entity initialization) and replace the literal with "Swedish".
In
`@src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java`:
- Around line 145-150: Replace the string-based status checks in
VisaViewController (in showEditForm and processUpdate) that call
visa.visaStatus().name().equals(...) with a type-safe EnumSet< VisaStatus >
constant (e.g., EnumSet.of(VisaStatus.INCOMPLETE, VisaStatus.SUBMITTED)) and
test membership via contains(visa.visaStatus()) to avoid duplicated string
logic; also update the IOException catch block inside processUpdate to add the
missing model attribute by calling model.addAttribute("statusInformation",
visa.statusInformation()) (same attribute used in the IllegalArgumentException
handler) so the edit form's feedback banner renders correctly.
In `@src/main/resources/application.properties`:
- Around line 8-9: The property spring.jpa.hibernate.ddl-auto=create must be
removed from application.properties and placed into a profile-scoped file (e.g.,
application-dev.properties or application-test.properties) so production doesn’t
run with destructive DDL; update application.properties to set
spring.jpa.hibernate.ddl-auto=validate or spring.jpa.hibernate.ddl-auto=none (or
remove the key entirely) and create/update application-dev.properties to contain
spring.jpa.hibernate.ddl-auto=create so only the dev profile uses schema
creation.
---
Outside diff comments:
In
`@src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java`:
- Around line 209-214: In VisaViewController inside the catch
(java.io.IOException e) path of the document-upload handling method, add
model.addAttribute("statusInformation", visa.statusInformation()); before
preparing the apply model/return so the same statusInformation attribute set in
the IllegalArgumentException branch is present on re-render; this ensures the
"Feedback from Handler" banner in edit-form.html is shown consistently after
upload failures.
In `@src/main/resources/templates/visa/details.html`:
- Around line 181-195: Update the UI so applicants with SUBMITTED visas can
reach the edit form: modify the conditional in details.html that currently
checks "visa.visaStatus.name() == 'INCOMPLETE' and visa.applicantId ==
currentUser.id" to allow SUBMITTED as well (e.g., check for INCOMPLETE or
SUBMITTED), or alternatively add an Edit link in my-applications.html
conditioned on visa.visaStatus.name() being INCOMPLETE or SUBMITTED and
visa.applicantId == currentUser.id; ensure the th:href remains
"@{/visa/{id}/edit(id=${visa.id})}" so the controller's existing edit handling
for statuses INCOMPLETE and SUBMITTED is used.
---
Nitpick comments:
In
`@src/main/java/org/example/visacasemanagementsystem/config/DataInitializer.java`:
- Line 27: Replace the fully-qualified type usages in DataInitializer with
normal imports: add import statements for
org.example.visacasemanagementsystem.comment.repository.CommentRepository,
org.example.visacasemanagementsystem.comment.model.Comment (or the correct
Comment class), and java.time.LocalDateTime at the top of the file, then update
all occurrences in the DataInitializer class (constructor parameters and any
method bodies, e.g., where CommentRepository, Comment, and LocalDateTime are
referenced around the constructor and the initialization block at lines ~69-78)
to use the simple class names instead of fully-qualified names to improve
readability.
- Line 40: Replace the System.out.println calls in DataInitializer with SLF4J
logging: add or use the class-level logger (e.g., annotate DataInitializer with
`@Slf4j` or obtain a Logger) and change the prints to log.info(...) (or
appropriate level) for the messages such as the "--- Starting generation of test
data ---" and the second occurrence around line 64 so logging is consistent with
other classes like FileService.
In `@src/main/java/org/example/visacasemanagementsystem/file/FileService.java`:
- Around line 67-69: Remove the stale Swedish instruction comment in FileService
around the bucket/CORS section and replace it with a single concise English log
entry (or delete the log line entirely) to avoid clutter; locate the log.info
call that currently reads "Bucket initialization complete. Skipping programmatic
CORS config for local MinIO compatibility." inside the FileService class (bucket
initialization method) and either keep a short English message like "Bucket
initialization complete; programmatic CORS disabled for local MinIO." or remove
the redundant log statement.
- Around line 37-38: Remove the dead field corsAllowedOrigins from the
FileService class and delete its corresponding property from
application.properties; specifically, delete the field declaration
"@Value(\"${minio.corsAllowedOrigins:http://localhost:8080}\") private String
corsAllowedOrigins;" from FileService and remove the minio.corsAllowedOrigins
entry in src/main/resources/application.properties so there are no unused
CORS-related symbols remaining.
In `@src/main/resources/application.properties`:
- Line 19: Remove the dead CORS config and its unused field: delete the
minio.corsAllowedOrigins entry from application.properties and remove the
corresponding `@Value-annotated` field (the corsAllowedOrigins/string field) from
the FileService class and any now-unused imports; also verify
FileService.initializeBucket() and other methods no longer reference that field
and adjust or remove any commented/leftover CORS logic if present.
🪄 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: 8858e8f8-2ed8-4202-adfb-8fe6ed39439c
📒 Files selected for processing (7)
src/main/java/org/example/visacasemanagementsystem/config/DataInitializer.javasrc/main/java/org/example/visacasemanagementsystem/file/FileService.javasrc/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.javasrc/main/resources/application.propertiessrc/main/resources/templates/visa/details.htmlsrc/test/java/org/example/visacasemanagementsystem/visa/VisaServiceIntegrationTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java
Comment on lines
+22
to
+67
| public class DataInitializer { | ||
|
|
||
| @Bean | ||
| public CommandLineRunner initData(UserRepository userRepository, | ||
| VisaRepository visaRepository, | ||
| org.example.visacasemanagementsystem.comment.repository.CommentRepository commentRepository, | ||
| CommentLogService commentLogService, | ||
| FileLogService fileLogService, | ||
| PasswordEncoder passwordEncoder) { | ||
| return args -> { | ||
|
|
||
| // 1. Initialize Users (Help method further down) | ||
| User applicant = getOrCreateUser(userRepository, passwordEncoder, "user@test.com", "John Doe", UserAuthorization.USER); | ||
| User admin = getOrCreateUser(userRepository, passwordEncoder, "admin@test.com", "Admin Officer", UserAuthorization.ADMIN); | ||
| User sysadmin = getOrCreateUser(userRepository, passwordEncoder, "sysadmin@test.com", "System Admin", UserAuthorization.SYSADMIN); | ||
|
|
||
| // 2. Initialize Visa Cases (Only if database is empty) | ||
| if (visaRepository.count() == 0) { | ||
| System.out.println("--- Starting generation of test data ---"); | ||
|
|
||
| // CASE A: (INCOMPLETE) | ||
| Visa incompleteVisa = createVisa(visaRepository, applicant, admin, VisaStatus.INCOMPLETE, VisaType.STUDY, "SWE555666"); | ||
| Long v1Id = incompleteVisa.getId(); | ||
|
|
||
| fileLogService.createFileLog(applicant.getId(), v1Id, "passport_scan.pdf", FileEventType.UPLOADED, "Initial passport scan submitted."); | ||
|
|
||
| saveCommentAndLog(commentRepository, commentLogService, admin, incompleteVisa, "Page 2 of the passport is missing. Please complete the upload."); | ||
| saveCommentAndLog(commentRepository, commentLogService, applicant, incompleteVisa, "I am sorry, uploading it now!"); | ||
|
|
||
| fileLogService.createFileLog(applicant.getId(), v1Id, "passport_page2.pdf", FileEventType.UPLOADED, "Supplementary passport page submitted."); | ||
|
|
||
| // CASE B: (REJECTED) | ||
| Visa rejectedVisa = createVisa(visaRepository, applicant, admin, VisaStatus.REJECTED, VisaType.EMPLOYMENT, "SWE-REJ-99"); | ||
| saveCommentAndLog(commentRepository, commentLogService, admin, rejectedVisa, "Rejection: Applicant does not meet the income requirements for this category."); | ||
|
|
||
| // CASE C: (GRANTED) | ||
| Visa grantedVisa = createVisa(visaRepository, applicant, admin, VisaStatus.GRANTED, VisaType.TOURIST, "SWE-OK-777"); | ||
| saveCommentAndLog(commentRepository, commentLogService, admin, grantedVisa, "All documentation verified. Visa has been granted."); | ||
|
|
||
| // CASE D: (SUBMITTED) | ||
| createVisa(visaRepository, applicant, null, VisaStatus.SUBMITTED, VisaType.TOURIST, "NEW-12345"); | ||
|
|
||
| System.out.println("--- Test data generated successfully! ---"); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
Restrict seed runner to dev/test profiles.
This CommandLineRunner always upserts users (including ADMIN/SYSADMIN with hardcoded password "password", line 89 in getOrCreateUser) and, when visaRepository.count() == 0, seeds visas. With spring.jpa.hibernate.ddl-auto=create (now the default per application.properties), the count check is true on every startup and seeding runs unconditionally — including in production, where deterministic admin credentials would be re-installed. Gate this bean on a non-prod profile.
🛡️ Proposed guard
`@Configuration`
+@org.springframework.context.annotation.Profile({"dev", "test"})
public class DataInitializer {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/visacasemanagementsystem/config/DataInitializer.java`
around lines 22 - 67, The DataInitializer's initData CommandLineRunner currently
runs in all environments; wrap it with a Spring profile guard so it only runs in
non-production environments — e.g., annotate the bean (or the DataInitializer
class) with `@Profile`({"dev","test"}) (or `@Profile`("!prod") if you prefer) to
prevent upserting users and seeding visas in production; update imports to
org.springframework.context.annotation.Profile and keep getOrCreateUser,
initData, and the existing PasswordEncoder usage unchanged.
Comment on lines
+145
to
+150
| boolean statusAllowsEdit = visa.visaStatus().name().equals("INCOMPLETE") | ||
| || visa.visaStatus().name().equals("SUBMITTED"); | ||
|
|
||
| if (!statusAllowsEdit) { | ||
| return "redirect:/visa/" + id; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm VisaStatus is an enum with INCOMPLETE / SUBMITTED constants.
ast-grep --pattern $'public enum VisaStatus { $$$ }'
rg -nP '\bINCOMPLETE\b|\bSUBMITTED\b' --type=java -C1 -g 'VisaStatus*.java'Repository: ithsjava25/visa-case-management-system
Length of output: 1151
🏁 Script executed:
#!/bin/bash
# Examine the exact code at the specified lines to verify the string comparison and duplication claims
sed -n '140,155p' src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java
echo "---"
sed -n '182,195p' src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java
echo "---"
sed -n '209,220p' src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.javaRepository: ithsjava25/visa-case-management-system
Length of output: 1862
🏁 Script executed:
#!/bin/bash
# Find and read the prepareApplyModel method
rg -n "prepareApplyModel" src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java -A 15Repository: ithsjava25/visa-case-management-system
Length of output: 3900
Replace string comparisons with enum equality and fix missing model attribute in IOException handler.
Both showEditForm (lines 145–150) and processUpdate (lines 182–191) compare visa status by calling .name().equals(...). Using the VisaStatus enum directly is type-safe and survives enum renames. The check is duplicated across both methods—a single EnumSet constant avoids drift.
Additionally, the IOException catch block (lines 209–214) in processUpdate is missing model.addAttribute("statusInformation", visa.statusInformation()), while the IllegalArgumentException handler at line 226 includes it. This causes the edit form's "Feedback from Handler" banner to render incorrectly when document uploads fail.
♻️ Proposed cleanup
+ private static final java.util.Set<VisaStatus> EDITABLE_STATUSES =
+ java.util.EnumSet.of(VisaStatus.INCOMPLETE, VisaStatus.SUBMITTED);
+
+ private boolean isEditable(VisaDTO visa) {
+ return EDITABLE_STATUSES.contains(visa.visaStatus());
+ }Then in both handlers:
- boolean statusAllowsEdit = visa.visaStatus().name().equals("INCOMPLETE")
- || visa.visaStatus().name().equals("SUBMITTED");
-
- if (!statusAllowsEdit) {
+ if (!isEditable(visa)) {
return "redirect:/visa/" + id;
}And in the IOException handler:
} catch (java.io.IOException e) {
bindingResult.reject("upload.error", "Failed to upload new document: " + e.getMessage());
model.addAttribute("visa", visa);
prepareApplyModel(principal.getUserId(), model);
model.addAttribute("isEdit", true);
+ model.addAttribute("statusInformation", visa.statusInformation());
return "visa/edit-form";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java`
around lines 145 - 150, Replace the string-based status checks in
VisaViewController (in showEditForm and processUpdate) that call
visa.visaStatus().name().equals(...) with a type-safe EnumSet< VisaStatus >
constant (e.g., EnumSet.of(VisaStatus.INCOMPLETE, VisaStatus.SUBMITTED)) and
test membership via contains(visa.visaStatus()) to avoid duplicated string
logic; also update the IOException catch block inside processUpdate to add the
missing model attribute by calling model.addAttribute("statusInformation",
visa.statusInformation()) (same attribute used in the IllegalArgumentException
handler) so the edit form's feedback banner renders correctly.
Comment on lines
+8
to
+9
| # application.properties (default/test/prod) | ||
| spring.jpa.hibernate.ddl-auto=update | ||
| spring.jpa.hibernate.ddl-auto=create |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Look for profile-scoped property files that may override ddl-auto, and any data-loss safeguards.
fd -t f 'application.*\.properties|application.*\.ya?ml' src/main/resources src/test/resources
rg -nP 'ddl-auto|hibernate\.hbm2ddl' -C2Repository: ithsjava25/visa-case-management-system
Length of output: 961
🏁 Script executed:
# Verify: are there any other profile files or overrides we may have missed?
fd -t f 'application.*\.' src/main/resources src/test/resources | sortRepository: ithsjava25/visa-case-management-system
Length of output: 226
Move spring.jpa.hibernate.ddl-auto=create to a profile-scoped properties file; production will otherwise drop and recreate the entire database on every startup.
The default application.properties applies in production unless explicitly overridden. With ddl-auto=create, Hibernate will destroy all data on every application restart—a catastrophic misconfiguration in any production environment.
The test profile correctly overrides this with create-drop in application-test.properties, but no equivalent prod/dev profile exists to protect production. Move ddl-auto=create to application-dev.properties or application-test.properties and set the default to validate or none.
Suggested fix
In src/main/resources/application.properties:
-spring.jpa.hibernate.ddl-auto=create
+spring.jpa.hibernate.ddl-auto=validateIn src/main/resources/application-dev.properties (create if missing):
spring.jpa.hibernate.ddl-auto=create📝 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
| # application.properties (default/test/prod) | |
| spring.jpa.hibernate.ddl-auto=update | |
| spring.jpa.hibernate.ddl-auto=create | |
| # application.properties (default/test/prod) | |
| spring.jpa.hibernate.ddl-auto=validate |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/application.properties` around lines 8 - 9, The property
spring.jpa.hibernate.ddl-auto=create must be removed from application.properties
and placed into a profile-scoped file (e.g., application-dev.properties or
application-test.properties) so production doesn’t run with destructive DDL;
update application.properties to set spring.jpa.hibernate.ddl-auto=validate or
spring.jpa.hibernate.ddl-auto=none (or remove the key entirely) and
create/update application-dev.properties to contain
spring.jpa.hibernate.ddl-auto=create so only the dev profile uses schema
creation.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/main/java/org/example/visacasemanagementsystem/config/DataInitializer.java (1)
22-67:⚠️ Potential issue | 🟠 MajorRestrict seed runner to dev/test profiles.
This
CommandLineRunnerstill seeds users (with hardcoded"password"ingetOrCreateUserat line 89) on every startup, and seeds visas whenevervisaRepository.count() == 0— which, combined withspring.jpa.hibernate.ddl-auto=create, means it also re-seeds in production. Gate this bean on a non-prod profile.🛡️ Proposed guard
`@Configuration` +@org.springframework.context.annotation.Profile({"dev", "test"}) public class DataInitializer {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/config/DataInitializer.java` around lines 22 - 67, The CommandLineRunner in DataInitializer (method initData) should be restricted to non-production profiles so seeding (and hardcoded passwords via getOrCreateUser) doesn't run in prod; annotate the bean/class with a profile guard (e.g., `@Profile`("!prod") or `@Profile`({"dev","test"})) or use a conditional so the initData CommandLineRunner only loads in dev/test, and update getOrCreateUser to avoid a hardcoded "password" (pull a seeded password from config or skip creating default credentials when not explicitly enabled).
🧹 Nitpick comments (4)
src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java (1)
182-191: Authorization + status guard look correct.Fetching the
VisaDTOonce, asserting applicant ownership, and rejecting non-editable statuses before validation/upload work prevents both unauthorized writes and accidental edits onGRANTED/REJECTEDcases.UnauthorizedExceptionis mapped to 403 byGlobalExceptionHandler, so the failure path is well-defined.One minor UX consideration: when the status guard at line 189–190 fires after the user submitted form changes, those changes are silently dropped on the redirect. Consider attaching a flash attribute (e.g.,
RedirectAttributes#addFlashAttribute("errorMessage", …)) so the details page can explain why the edit was rejected.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java` around lines 182 - 191, When the status guard in VisaViewController (checking visa.visaStatus() and redirecting when not INCOMPLETE or SUBMITTED) drops user changes silently, attach a flash error message so the details page can show why the edit was rejected; update the redirect path to use RedirectAttributes#addFlashAttribute("errorMessage", "Cannot edit application in current status: " + currentStatus) before returning "redirect:/visa/" + id so the UI can surface the reason to the user.src/main/java/org/example/visacasemanagementsystem/config/DataInitializer.java (3)
39-65: Seeding is not transactional — partial failures will leave inconsistent state.
initDataperforms many independent writes (visa save, file logs, comment saves, comment logs) without a surrounding transaction. If any later call throws, earlier rows persist butvisaRepository.count()will already be non-zero on the next startup, so the seeder won't retry — leaving the database in a half-seeded state that's awkward to recover from.Consider extracting the seeding body into a
@Transactionalmethod on a helper bean (or annotating the runner's invocation path) so the whole seed succeeds or rolls back atomically.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/config/DataInitializer.java` around lines 39 - 65, The seeding code inside initData (the block guarded by visaRepository.count() == 0 that calls createVisa, fileLogService.createFileLog, saveCommentAndLog, commentLogService, etc.) must run inside a single transaction so partial failures roll back; extract the entire seeding body into a `@Transactional` method on a Spring-managed bean (either make DataInitializer a `@Component` with a `@Transactional` seed() method or create a new Seeder bean with a transactional seedVisas() method) and have the runner invoke that transactional method (don't keep the writes directly in the runner thread), ensuring the method includes the createVisa, fileLogService.createFileLog and saveCommentAndLog calls so the whole operation commits or rolls back atomically.
27-27: Replace fully-qualified class names with imports.
org.example.visacasemanagementsystem.comment.repository.CommentRepositoryandorg.example.visacasemanagementsystem.comment.entity.Comment(andjava.time.LocalDateTimeat line 77) are referenced via FQN multiple times. Importing them keeps the file consistent with the other imports already in use.♻️ Proposed cleanup
+import org.example.visacasemanagementsystem.comment.entity.Comment; +import org.example.visacasemanagementsystem.comment.repository.CommentRepository; +import java.time.LocalDateTime; @@ - org.example.visacasemanagementsystem.comment.repository.CommentRepository commentRepository, + CommentRepository commentRepository, @@ - private void saveCommentAndLog(org.example.visacasemanagementsystem.comment.repository.CommentRepository commentRepo, + private void saveCommentAndLog(CommentRepository commentRepo, CommentLogService logService, User author, Visa visa, String text) { - - org.example.visacasemanagementsystem.comment.entity.Comment c = new org.example.visacasemanagementsystem.comment.entity.Comment(); + Comment c = new Comment(); @@ - c.setCreatedAt(java.time.LocalDateTime.now()); + c.setCreatedAt(LocalDateTime.now());Also applies to: 69-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/config/DataInitializer.java` at line 27, Replace occurrences of the fully-qualified types org.example.visacasemanagementsystem.comment.repository.CommentRepository, org.example.visacasemanagementsystem.comment.entity.Comment, and java.time.LocalDateTime with simple imports and short type names; update the top of the file to add imports for CommentRepository, Comment, and LocalDateTime and then refactor usages in the DataInitializer constructor and in methods (references around the comment creation code and the constructor parameter list) to use CommentRepository, Comment, and LocalDateTime directly instead of FQNs.
40-64: Use a logger instead ofSystem.out.println.
System.out.printlnwrites outside the structured logging pipeline (no level, no timestamp, no MDC). Switch to SLF4J so these markers are filterable like the rest of the application.♻️ Proposed change
+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; + `@Configuration` public class DataInitializer { + private static final Logger log = LoggerFactory.getLogger(DataInitializer.class); @@ - System.out.println("--- Starting generation of test data ---"); + log.info("--- Starting generation of test data ---"); @@ - System.out.println("--- Test data generated successfully! ---"); + log.info("--- Test data generated successfully! ---");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/config/DataInitializer.java` around lines 40 - 64, The DataInitializer class currently uses System.out.println (the two calls around test data generation) which bypasses structured logging; replace them with an SLF4J logger by adding a private static final org.slf4j.Logger (LoggerFactory.getLogger(DataInitializer.class)) and change the println calls to logger.info(...) (or appropriate level) so messages flow through the application's logging pipeline; also add the necessary SLF4J import and remove the System.out usage to ensure timestamps, levels and MDC are preserved.
🤖 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/org/example/visacasemanagementsystem/config/DataInitializer.java`:
- Around line 22-67: The CommandLineRunner in DataInitializer (method initData)
should be restricted to non-production profiles so seeding (and hardcoded
passwords via getOrCreateUser) doesn't run in prod; annotate the bean/class with
a profile guard (e.g., `@Profile`("!prod") or `@Profile`({"dev","test"})) or use a
conditional so the initData CommandLineRunner only loads in dev/test, and update
getOrCreateUser to avoid a hardcoded "password" (pull a seeded password from
config or skip creating default credentials when not explicitly enabled).
---
Nitpick comments:
In
`@src/main/java/org/example/visacasemanagementsystem/config/DataInitializer.java`:
- Around line 39-65: The seeding code inside initData (the block guarded by
visaRepository.count() == 0 that calls createVisa, fileLogService.createFileLog,
saveCommentAndLog, commentLogService, etc.) must run inside a single transaction
so partial failures roll back; extract the entire seeding body into a
`@Transactional` method on a Spring-managed bean (either make DataInitializer a
`@Component` with a `@Transactional` seed() method or create a new Seeder bean with
a transactional seedVisas() method) and have the runner invoke that
transactional method (don't keep the writes directly in the runner thread),
ensuring the method includes the createVisa, fileLogService.createFileLog and
saveCommentAndLog calls so the whole operation commits or rolls back atomically.
- Line 27: Replace occurrences of the fully-qualified types
org.example.visacasemanagementsystem.comment.repository.CommentRepository,
org.example.visacasemanagementsystem.comment.entity.Comment, and
java.time.LocalDateTime with simple imports and short type names; update the top
of the file to add imports for CommentRepository, Comment, and LocalDateTime and
then refactor usages in the DataInitializer constructor and in methods
(references around the comment creation code and the constructor parameter list)
to use CommentRepository, Comment, and LocalDateTime directly instead of FQNs.
- Around line 40-64: The DataInitializer class currently uses System.out.println
(the two calls around test data generation) which bypasses structured logging;
replace them with an SLF4J logger by adding a private static final
org.slf4j.Logger (LoggerFactory.getLogger(DataInitializer.class)) and change the
println calls to logger.info(...) (or appropriate level) so messages flow
through the application's logging pipeline; also add the necessary SLF4J import
and remove the System.out usage to ensure timestamps, levels and MDC are
preserved.
In
`@src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java`:
- Around line 182-191: When the status guard in VisaViewController (checking
visa.visaStatus() and redirecting when not INCOMPLETE or SUBMITTED) drops user
changes silently, attach a flash error message so the details page can show why
the edit was rejected; update the redirect path to use
RedirectAttributes#addFlashAttribute("errorMessage", "Cannot edit application in
current status: " + currentStatus) before returning "redirect:/visa/" + id so
the UI can surface the reason to the user.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 3cb30157-697a-4ee0-9e55-5015a591d450
📒 Files selected for processing (5)
src/main/java/org/example/visacasemanagementsystem/config/DataInitializer.javasrc/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.javasrc/main/resources/application.propertiessrc/main/resources/templates/visa/details.htmlsrc/test/java/org/example/visacasemanagementsystem/visa/VisaServiceIntegrationTest.java
✅ Files skipped from review due to trivial changes (1)
- src/main/resources/application.properties
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/resources/templates/visa/details.html
- src/test/java/org/example/visacasemanagementsystem/visa/VisaServiceIntegrationTest.java
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.
-Logic: Restricted visa editing to only 'SUBMITTED' and 'INCOMPLETE' statuses in VisaViewController.
Summary by CodeRabbit
New Features
Improvements
Chores / Tests