Feature/synchronize UI design#52
Merged
Merged
Conversation
- Removed dashboards and replaced them with function specific views which is handled by the new /home endpoint - Separated visas/dashboard into /visa/cases for admins and visa/my-application for users - Added new log/visa and log/user templates which shows respective log information with filtering and pagination. - Made all the new views accessible based on authorization in the header Fix: - GlobalExceptionHandler now correctly redirects to home instead of dashboard through a button on the template. - Changed /visas endpoint to /visa and fixed reference in SecurityConfig - Made /log only accessible to sysadmins in the SecurityConfig
Fix: Removed tests related to /dashboard
…serLogService Fix: Changed Reference to Visa ID on templates
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRoot now redirects to /home with role-based targets; new paginated/filterable audit log controller/views added; visa routes renamed from /visas to /visa with case-list endpoints and service/repo support; security and logout behavior adjusted; many templates refactored to shared CSS; tests and build config updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client
participant LogCtrl as LogViewController
participant Service as VisaLogService
participant Repo as VisaLogRepository
participant DB as Database
Client->>LogCtrl: GET /log/visa?eventType=&from=&to=&page=&size=
LogCtrl->>LogCtrl: normalize dates, clamp page/size, build PageRequest
LogCtrl->>Service: findFiltered(eventType, fromDateTime, toDateTime, pageable)
Service->>Service: build JPA Specification (predicates)
Service->>Repo: findAll(spec, pageable)
Repo->>DB: execute query with predicates + pagination
DB-->>Repo: Page<VisaLog>
Repo-->>Service: Page<VisaLog>
Service->>Service: map entities -> DTOs
Service-->>LogCtrl: Page<VisaLogDTO>
LogCtrl->>Client: render view `log/visa` (model: logs, eventTypes, filters)
sequenceDiagram
participant Client as Client
participant AppCtrl as ApplicationViewController
participant Sec as SecurityContext
participant VisaCtrl as VisaViewController
Client->>AppCtrl: GET /
AppCtrl->>AppCtrl: redirect / -> /home
Client->>AppCtrl: GET /home
AppCtrl->>Sec: iterate authorities -> set flags
alt SYSADMIN
Sec-->>AppCtrl: ROLE_SYSADMIN
AppCtrl-->>Client: redirect /log/visa
else ADMIN
Sec-->>AppCtrl: ROLE_ADMIN
AppCtrl-->>Client: redirect /visa/cases
else
AppCtrl-->>Client: redirect /visa/my-applications
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 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: 6
🧹 Nitpick comments (17)
src/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.java (2)
41-41: Restrict/user/logoutmatcher to POST to match the controller.
UserViewController.logoutis a@PostMapping("/user/logout"), but the matcher here authorizes the path for any HTTP method. A GET to/user/logoutby an authenticated user will pass authorization and fall through to a 404 (or to Spring's default error handling), which is slightly surprising and, more importantly, invites accidental logout links. Constraining this to POST aligns the security rule with the actual handler and makes the intent explicit.🔒 Proposed fix
- .requestMatchers("/user/logout").authenticated() + .requestMatchers(org.springframework.http.HttpMethod.POST, "/user/logout").authenticated()🤖 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/SecurityConfig.java` at line 41, The security rule currently allows any HTTP method for "/user/logout" in SecurityConfig by using requestMatchers("/user/logout").authenticated(); restrict this to POST to match the controller by changing the matcher to target POST (e.g., use requestMatchers(HttpMethod.POST, "/user/logout").authenticated() or the framework's post-specific matcher) and add the necessary HttpMethod import; reference SecurityConfig and UserViewController.logout to ensure the rule aligns with the `@PostMapping` handler.
60-60: RemovehttpBasic(withDefaults())from the security configuration.HTTP Basic authentication is not used anywhere in the codebase—no tests reference it, no clients depend on it, and the application relies exclusively on form login for authentication. Keeping an unused authentication mechanism expands the attack surface without providing any benefit. If HTTP Basic support is needed in the future, it can be re-added with proper documentation and test coverage at that time.
🤖 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/SecurityConfig.java` at line 60, Remove the HTTP Basic configuration call from SecurityConfig by deleting the .httpBasic(withDefaults()) invocation inside the HttpSecurity configuration (the method configuring HttpSecurity in class SecurityConfig); ensure formLogin() remains configured and that any static import of withDefaults() is removed, and run/adjust security tests to reflect that HTTP Basic is no longer enabled.src/main/resources/static/css/tokens.css (2)
128-129: Minor:--radius-lgand--radius-pillaliasing is fine, but consider a true pill value.The comment is honest that both resolve to
12pxtoday. For actual pill buttons/badges most design systems use9999px(or999em) so the shape is radius-independent of element height. If a pill treatment is ever applied to a tall element, the current12pxwill silently look like a rounded rectangle instead of a pill.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/css/tokens.css` around lines 128 - 129, The current CSS token alias --radius-pill equals --radius-lg (12px), which will fail to produce true pill shapes for taller elements; change the --radius-pill token to a height-independent large value (e.g., 9999px or 999em) instead of 12px so pill buttons/badges always render fully rounded, and update any related comment to note it's intentionally a max-radius for pill shapes (referencing the --radius-lg and --radius-pill tokens).
120-121: Stylelintvalue-keyword-caseerrors here are false positives — don't lowercase these font names.Font family identifiers like
BlinkMacSystemFont,Roboto,SFMono-Regular,Menlo, andConsolasare proper-case by convention and match what Apple/Google/Microsoft actually ship. Lowercasing them is technically valid CSS (idents are case-insensitive infont-family) but is unusual and hurts readability.Prefer to silence the rule for font stacks rather than accommodate it:
🔧 Suggested stylelint config tweak
"value-keyword-case": [ "lower", { + "ignoreProperties": ["/font/", "font-family"], + "ignoreKeywords": ["BlinkMacSystemFont", "Roboto", "SFMono-Regular", "Menlo", "Consolas"] } ]Or add an inline ignore next to the declarations:
+ /* stylelint-disable value-keyword-case -- proper-case font family names */ --font-system: -apple-system, BlinkMacSystemFont, "Segoe UI", Roboto, sans-serif; --font-mono: ui-monospace, SFMono-Regular, Menlo, Consolas, monospace; + /* stylelint-enable value-keyword-case */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/css/tokens.css` around lines 120 - 121, The stylelint rule value-keyword-case is flagging proper-case font names as false positives in the CSS variables --font-system and --font-mono; fix by silencing the rule for these declarations — either add an inline suppression comment immediately before/after the --font-system and --font-mono lines (e.g. a stylelint-disable-next-line/value-keyword-case around those declarations) or update the stylelint config to ignore value-keyword-case for font-family values (e.g. allow-list or an overrides pattern for tokens.css) so the font names remain properly cased.src/main/java/org/example/visacasemanagementsystem/audit/service/VisaLogService.java (2)
38-61: Recommended refactor: deduplicatefindFilteredacrossVisaLogServiceandUserLogService.
UserLogService#findFiltered(Lines 44-61 of that file) is a near-identical copy of this method — only the entity type and the event-type attribute name differ. If a filter gets added (e.g.actorUserId,visaCaseId) it will have to be applied in two places and kept in sync.A couple of directions worth considering:
- Extract a small generic helper
buildTimeRangeSpec(String eventAttr, Enum<?> eventValue, LocalDateTime from, LocalDateTime to)returningSpecification<T>, and have each service call it plus map.- Or introduce a tiny abstract base (
AbstractAuditLogService<E, DTO>) that ownsfindFilteredand delegates the event-attribute name + mapper to subclasses.Not blocking — flagging because the duplication is likely to bit-rot as the audit pages evolve.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/audit/service/VisaLogService.java` around lines 38 - 61, The two near-identical methods VisaLogService.findFiltered and UserLogService.findFiltered should be deduplicated: extract a reusable Specification builder (e.g. buildTimeRangeSpec(String eventAttr, Enum<?> eventValue, LocalDateTime from, LocalDateTime to) returning Specification<T>) or introduce an AbstractAuditLogService<E, DTO> that implements findFiltered and delegates the entity-specific pieces (the event attribute name and the mapper) to subclasses; update VisaLogService and UserLogService to call the shared helper/base method (keep visaLogMapper::toDTO and the repository calls in subclasses if using the abstract base) so future filters (actorUserId, visaCaseId, etc.) are added once.
44-61: Attribute names in the Specification are correct; consider adding compile-time safety for long-term maintenance.The hardcoded strings
root.get("visaEventType")androot.get("timeStamp")correctly match theVisaLogentity fields. However, string-based attribute access remains runtime-verified—a future field rename could silently fail until the endpoint is exercised.For long-term robustness, consider one of these approaches:
- Use the JPA static metamodel (e.g.,
VisaLog_.timeStamp,VisaLog_.visaEventType) generated by Hibernate JPA Modelgen, providing compile-time verification.- Extract a typed
Specification<VisaLog>factory method to centralize the attribute names and reduce duplication withUserLogService.findFiltered.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/audit/service/VisaLogService.java` around lines 44 - 61, The Specification in VisaLogService.findFiltered uses string attribute names ("visaEventType", "timeStamp") which are runtime-checked; switch to a compile-time safe approach by either using the JPA static metamodel (e.g., replace root.get("timeStamp") and root.get("visaEventType") with VisaLog_.timeStamp and VisaLog_.visaEventType) or extract a typed Specification factory method (e.g., createVisaLogSpecification(...) used by findFiltered, similar to UserLogService.findFiltered) that centralizes attribute access so renames are caught at compile time.src/test/java/org/example/visacasemanagementsystem/visa/VisaServiceTest.java (1)
311-446: Optional: extract a small test helper forVisaDTOconstruction.The six new tests each instantiate
VisaDTOwith 15 positionalnullarguments (e.g., Lines 324–327, 367–368, 411–414). This is fine today but will force every test to be touched if the DTO gains or reorders fields. AvisaDtoWithId(Long id)helper would make intent clearer and localize future breakage.♻️ Proposed helper
+ private static VisaDTO visaDtoWithId(Long id) { + return new VisaDTO(id, null, null, null, null, null, + null, null, null, null, null, null, null, null, null); + }Then in each test:
- VisaDTO d1 = new VisaDTO(1L, null, null, null, null, null, - null, null, null, null, null, null, null, null, null); + VisaDTO d1 = visaDtoWithId(1L);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/visa/VisaServiceTest.java` around lines 311 - 446, Extract a small test helper in VisaServiceTest named something like visaDtoWithId(Long id) that returns a VisaDTO instance with the given id and default/null values for other fields, then replace all inline new VisaDTO(...15 nulls...) uses (in tests findOpenCasesByHandler_shouldMapEntitiesToDTOs_andQueryAssignedAndIncomplete, findUnassignedCases_shouldMapEntitiesToDTOs_andQuerySubmittedWithNullHandler, findHandledCasesByHandler_shouldMapEntitiesToDTOs_andQueryGrantedAndRejected, etc.) with calls to visaDtoWithId(id) to centralize construction and make tests resilient to DTO field changes.src/main/resources/templates/visa/details.html (1)
181-195: Brittle string comparison for handler assignment state.The
Action Requiredbanner is only rendered whenvisa.handlerName == 'Unassigned'. This ties a UI-critical condition to a display label rather than a structural field (e.g.visa.handlerId == nullor a dedicated boolean on the DTO). If the label ever gets localised, trimmed, or changed to-for symmetry with other placeholders elsewhere in this template, this branch silently stops rendering and incomplete-status applicants lose the Edit call-to-action. Preferable to gate on an id/null check exposed byVisaDTO.🤖 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, The template's th:if currently checks the display label visa.handlerName == 'Unassigned' which is brittle; change the condition to check the structural handler identifier (e.g., visa.handlerId == null) or a dedicated boolean on the DTO instead of handlerName so the banner renders regardless of label/localization; update the th:if expression that references visa.handlerName to use visa.handlerId (or the boolean flag) and ensure the rest of the block (the Action Required banner and Edit link) remains unchanged.src/main/resources/templates/visa/cases.html (2)
29-32: Inconsistent user-display model attribute.Other visa templates in this PR render the logged-in user as
${currentUser.fullName}(e.g.,apply-form.htmlline 29,edit-form.htmlline 79). Here it is${name}, which means the controller has to seed a different attribute just for this page. If the controller already addscurrentUser, prefer${currentUser.fullName}for consistency; otherwise, rename the controller-side model attribute so the admin/sysadmin landing page follows the same convention as the rest of the visa views.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/visa/cases.html` around lines 29 - 32, The template uses the model attribute ${name} instead of the project's standard ${currentUser.fullName}; update the view in cases.html to render ${currentUser.fullName} (replace ${name}) so it matches other visa templates, or if the controller currently only supplies "name" instead of "currentUser", change the controller to add a "currentUser" model attribute (with a fullName property) and keep cases.html using ${currentUser.fullName} to maintain consistency with apply-form.html and edit-form.html.
37-188: Three near-identical case tables — candidate for a Thymeleaf fragment.
Open Cases,Unassigned Cases, andHandled Casesrender the exact same 8-column table (Reference/Visa ID aside). Any future column change or styling tweak has to be repeated three times and will inevitably drift. Extracting afragments/cases :: section(title, list, emptyMsg, countSuffix)fragment would collapse ~120 lines into three fragment invocations and keep the page honest.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/visa/cases.html` around lines 37 - 188, The three near-identical tables for "Open Cases", "Unassigned Cases" and "Handled Cases" should be extracted into a reusable Thymeleaf fragment (e.g. fragments/cases :: section(title, list, emptyMsg, countSuffix)) to avoid duplication; create the fragment that accepts a title, the list variable (iterated as visa), an empty-state message and count suffix, move the table markup (including the span with class case-count and th:text logic, the th:each="visa : ${...}", the status span with th:classappend and the date formatting using `#temporals`) into that fragment, then replace each table block with a fragment invocation passing the appropriate title, list (openCases / unassignedCases / handledCases), empty message and suffix.src/main/resources/templates/log/user.html (1)
8-41: Duplicated page-scoped CSS withlog/visa.html.The entire
<style>block here is byte-for-byte identical to the one inlog/visa.html, and the comment acknowledges this. Any future tweak has to be made in two places and will drift otherwise. Consider extracting these rules intocomponents.css(e.g., a shared.log-page/.filter-bar/.pagerruleset) or into a Thymeleaf fragment so both log pages pull from a single source of truth.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/log/user.html` around lines 8 - 41, Extract the duplicated page-scoped CSS block (the rules for .container, .log-count, .filter-bar, .pager and related selectors) out of user.html and visa.html into a single shared stylesheet or Thymeleaf fragment (e.g., components.css or a fragment like fragments/log-styles) and replace the inline <style> in both templates with a single include: either a <link rel="stylesheet"> to the shared CSS or a Thymeleaf th:insert/th:replace of the fragment; ensure the shared resource contains the .container, .log-count, .filter-bar, .filter-bar .field, .filter-bar label, .filter-bar input/select, .filter-bar .actions, .pager and .pager .page-info rules so both pages use the same source of truth.src/test/java/org/example/visacasemanagementsystem/audit/controller/LogViewControllerTest.java (1)
99-101: Optional:@WithMockUseris effectively redundant alongsideloginAsSysadmin().Every test has both
@WithMockUser(roles = "SYSADMIN")and a call tologinAsSysadmin(). The manualSecurityContextHolder.getContext().setAuthentication(auth)inloginAsSysadmin()overwrites whatever@WithMockUserinstalled, so the annotation is dead weight. Since you've already documented (lines 53–60) that the default principal can't be used, consider dropping@WithMockUserto avoid confusion about which principal is actually active during the test.Also applies to: 124-125, 159-160, 178-179, 195-196, 224-225, 247-248
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/audit/controller/LogViewControllerTest.java` around lines 99 - 101, Remove the redundant `@WithMockUser` annotations from the tests that call loginAsSysadmin(), since loginAsSysadmin() sets the SecurityContext directly and overrides the annotation; locate test methods such as visaLog_AsSysadmin_NoFilters_ShouldReturnPageWithDefaults() (and the other tests at the commented ranges) and delete the `@WithMockUser`(roles = "SYSADMIN") declarations so the tests rely solely on the loginAsSysadmin() helper for authentication.src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java (1)
299-301: Remove the deadOPEN_STATUSES/HANDLED_STATUSESfields and their stale comment.These constants exist solely to keep the
VisaStatusimport alive — the controller does not useVisaStatusanywhere else (the back-link logic on lines 300–301 keys offUserAuthorization, notVisaStatus, so the line 299 comment is also inaccurate). Hiding an unused import behind dead static fields is a code smell; if the symbol isn't needed, drop the import.♻️ Proposed cleanup
-import org.example.visacasemanagementsystem.visa.VisaStatus; import org.example.visacasemanagementsystem.visa.VisaType;- // Silence unused-field warnings and make the static VisaStatus enum available to the back-link logic model.addAttribute("backUrl", user.userAuthorization() == UserAuthorization.USER ? "/visa/my-applications" : "/visa/cases");- - // Kept to prevent "unused import" removal of VisaStatus - `@SuppressWarnings`("unused") - private static final List<VisaStatus> OPEN_STATUSES = List.of(VisaStatus.ASSIGNED, VisaStatus.INCOMPLETE); - `@SuppressWarnings`("unused") - private static final List<VisaStatus> HANDLED_STATUSES = List.of(VisaStatus.GRANTED, VisaStatus.REJECTED); }Also applies to: 315-319
🤖 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 299 - 301, Remove the dead OPEN_STATUSES and HANDLED_STATUSES static fields and the stale comment that claims they silence unused-field warnings in VisaViewController; delete those fields and the comment, remove the unused import of VisaStatus, and then verify no other code in VisaViewController (e.g., back-link logic using user.userAuthorization()/UserAuthorization) references VisaStatus so the class still compiles.src/main/resources/static/css/components.css (2)
139-153: Heads-up: broad element selectors (input,select,textarea,table,tr:hover td).These bare-tag rules apply everywhere
components.cssis loaded, which is site-wide. Any future<input>in a different context (e.g. inline search box, toggle, future component library) will inherit the dark-form styling (background, 100 % width,resize: none) and any table will gain hover borders. That's fine for the current design language, but expect to fight specificity with class-scoped overrides as the app grows. Consider scoping these under a class like.form-group input/.data-tableif/when that becomes painful.Also applies to: 317-339
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/css/components.css` around lines 139 - 153, The global element selectors in components.css (input, select, textarea, table, tr:hover td) apply site-wide and will unintentionally style any future inputs/tables; to fix, narrow the selectors by scoping them under a namespace/class (for example replace bare selectors with .form-group input, .form-group select, .form-group textarea for form controls and .data-table table, .data-table tr:hover td for tables) and move the existing rules (width, background, resize, hover borders, etc.) into those scoped selectors so global tags are no longer overridden.
187-187: Use the standard::file-selector-buttonpseudo-element instead of WebKit-specific::-webkit-file-upload-button.The
::-webkit-file-upload-buttonpseudo-element is WebKit/Blink-specific and not supported in Firefox or other non-Chromium browsers. Firefox has supported the standard::file-selector-buttonpseudo-element since version 82, which works across all modern browsers. Replace the WebKit prefix with the standard pseudo-element for cross-browser consistency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/css/components.css` at line 187, The CSS rule uses the WebKit-specific pseudo-element input[type="file"]::-webkit-file-upload-button which breaks cross-browser support; replace that selector with the standard input[type="file"]::file-selector-button and keep the same declarations so the file input button styling works in Firefox and other non‑Chromium browsers (update the selector occurrence for input[type="file"]::-webkit-file-upload-button to input[type="file"]::file-selector-button).src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java (1)
517-522: Smoke test is narrower than the comment suggests.
@WebMvcTest(VisaViewController.class)only wires the single controller under test, so this 404 simply confirmsVisaViewControlleritself doesn't map/visas/dashboard— it doesn't prove the old path is absent from the whole app (another@Controllercould still pick it up and this test wouldn't catch it). For the "silently coming back during future refactors" guarantee called out in the comment (lines 513–515), a full-context integration test (or at least@SpringBootTest) would be needed. Fine to keep this as-is for cheap coverage, but consider tightening the comment so future readers don't over-trust it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java` around lines 517 - 522, The test oldPluralPath_ShouldNoLongerResolve in VisaViewControllerTest only verifies VisaViewController (because of `@WebMvcTest`(VisaViewController.class)), so it cannot guarantee the old /visas/dashboard path is absent application-wide; either change the test to a full-context integration test by using `@SpringBootTest` with `@AutoConfigureMockMvc` (and reuse MockMvc) to assert a 404 across the entire application, or simply update the test comment to state it only checks VisaViewController scope (not the whole app) to avoid overclaiming. Ensure updates reference VisaViewControllerTest, the oldPluralPath_ShouldNoLongerResolve test method, and the `@WebMvcTest`(VisaViewController.class) annotation so reviewers can locate and apply the change.src/main/java/org/example/visacasemanagementsystem/audit/controller/LogViewController.java (1)
44-94: Optional: DRY the two log handlers.
visaLoganduserLogare structurally identical aside from the enum type, service, and view name. Not a blocker, but a small generic helper would make future columns/filters easier to add in one place.♻️ Sketch of a shared helper
private <E extends Enum<E>, D> String renderLogPage( E eventType, LocalDate from, LocalDate to, int page, int size, BiFunction<E, Pageable, Page<D>> fetchFn, E[] allEventTypes, String pageTitle, String view, Model model) { Page<D> logs = fetchFn.apply(eventType, buildPageable(page, size)); model.addAttribute("logs", logs); model.addAttribute("eventTypes", allEventTypes); model.addAttribute("selectedEventType", eventType); model.addAttribute("from", from); model.addAttribute("to", to); model.addAttribute("pageTitle", pageTitle); return view; }(Signature above simplified — you'd need a functional interface that also takes
from/to.)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/audit/controller/LogViewController.java` around lines 44 - 94, visaLog and userLog are duplicate handlers differing only by event enum, DTO/service and view; extract a generic helper (e.g., private <E extends Enum<E>, D> String renderLogPage(...)) and have visaLog and userLog delegate to it. The helper should accept parameters for eventType, from, to, page, size, a fetch function that takes (eventType, from, to, pageable) and returns Page<D> (or a small functional interface if three args are needed), the array of all event types, pageTitle and view name, and the Model; replace the duplicated attribute-setting logic in visaLog and userLog with calls to renderLogPage while keeping existing method signatures and using visaLogService.findFiltered and userLogService.findFiltered as the fetch functions and VisaEventType.values()/UserEventType.values() and "log/visa"/"log/user" for views.
🤖 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/static/css/components.css`:
- Around line 180-198: The file input currently hides the filename by setting
input[type="file"] { color: transparent; width: 105px !important; overflow:
hidden; } and using input[type="file"]::before as a fake button; fix by either
(A) restoring visibility: remove or toggle color: transparent, increase width
(remove the 105px !important) and allow overflow so the native filename can
show, keeping the ::before styled button as an adornment, or (B) switch to the
accessible pattern: keep a visually-hidden input[type="file"] and use a styled
<label for="..."> (replace the present input[type="file"]::before behavior) plus
an adjacent filename <span> that you update in a small change handler on the
input's change event to set the selected filename; update selectors referenced
above (input[type="file"], input[type="file"]::before) accordingly.
In `@src/main/resources/templates/error/error.html`:
- Line 80: The error page copy in templates/error/error.html currently reads
"Please verify your request to home page." — replace that sentence with clearer
wording such as "Please verify your request or return to the home page." Update
the <p> element text in the error template so the message reads the suggested
copy (or an equivalent clearer variant) to avoid the awkward phrasing.
In `@src/main/resources/templates/log/user.html`:
- Around line 107-111: The status badge can NPE when log.userEventType is null;
update the span rendering (the <span class="status" ...> that uses
`#strings.toLowerCase`(log.userEventType.name()) and
th:text="${log.userEventType}") to guard against null by checking
log.userEventType before calling name()/toLowerCase and providing a safe
fallback class/text (e.g., "unknown" or empty) so both th:classappend and
th:text never call name() on a null userEventType.
- Around line 121-140: The pager links currently inject
eventType=${selectedEventType} without null-checking, which emits eventType=
when no filter is set; update both pager anchor hrefs that reference
selectedEventType (the "Previous" and "Next" links within the pager block
referencing logs.number, logs.size, selectedEventType, from, to) to use the same
ternary pattern used for from/to (e.g. eventType=${selectedEventType != null ?
selectedEventType : null}) so the parameter is omitted when unset; apply the
same change to the corresponding pager links in log/visa.html as well.
In `@src/main/resources/templates/log/visa.html`:
- Around line 110-114: Null checks are missing for log.visaEventType and pager
eventType param: update the span to avoid calling name() on null (e.g., use a
ternary like log.visaEventType != null ? 'status-' +
`#strings.toLowerCase`(log.visaEventType.name()) : '' for th:classappend and
log.visaEventType != null ? log.visaEventType : '' for th:text) and change the
pager link parameters to mirror the from/to null-ternary pattern used elsewhere
so eventType is emitted as selectedEventType != null ? selectedEventType : ''
(instead of emitting an empty "eventType=" which breaks enum binding).
In `@src/main/resources/templates/visa/my-applications.html`:
- Line 25: The template uses property-style access on Java record DTOs; update
occurrences of ${currentUser.fullName} and the visa property usages to
method-style calls to match existing templates: replace ${currentUser.fullName}
with ${currentUser.fullName()}, and for each visa entry replace ${visa.id},
${visa.visaType}, ${visa.visaStatus}, ${visa.travelDate}, ${visa.updatedAt} with
${visa.id()}, ${visa.visaType()}, ${visa.visaStatus()}, ${visa.travelDate()},
${visa.updatedAt()} respectively so the template uses the record accessor
methods (currentUser.fullName(), visa.id(), visa.visaType(), visa.visaStatus(),
visa.travelDate(), visa.updatedAt()).
---
Nitpick comments:
In
`@src/main/java/org/example/visacasemanagementsystem/audit/controller/LogViewController.java`:
- Around line 44-94: visaLog and userLog are duplicate handlers differing only
by event enum, DTO/service and view; extract a generic helper (e.g., private <E
extends Enum<E>, D> String renderLogPage(...)) and have visaLog and userLog
delegate to it. The helper should accept parameters for eventType, from, to,
page, size, a fetch function that takes (eventType, from, to, pageable) and
returns Page<D> (or a small functional interface if three args are needed), the
array of all event types, pageTitle and view name, and the Model; replace the
duplicated attribute-setting logic in visaLog and userLog with calls to
renderLogPage while keeping existing method signatures and using
visaLogService.findFiltered and userLogService.findFiltered as the fetch
functions and VisaEventType.values()/UserEventType.values() and
"log/visa"/"log/user" for views.
In
`@src/main/java/org/example/visacasemanagementsystem/audit/service/VisaLogService.java`:
- Around line 38-61: The two near-identical methods VisaLogService.findFiltered
and UserLogService.findFiltered should be deduplicated: extract a reusable
Specification builder (e.g. buildTimeRangeSpec(String eventAttr, Enum<?>
eventValue, LocalDateTime from, LocalDateTime to) returning Specification<T>) or
introduce an AbstractAuditLogService<E, DTO> that implements findFiltered and
delegates the entity-specific pieces (the event attribute name and the mapper)
to subclasses; update VisaLogService and UserLogService to call the shared
helper/base method (keep visaLogMapper::toDTO and the repository calls in
subclasses if using the abstract base) so future filters (actorUserId,
visaCaseId, etc.) are added once.
- Around line 44-61: The Specification in VisaLogService.findFiltered uses
string attribute names ("visaEventType", "timeStamp") which are runtime-checked;
switch to a compile-time safe approach by either using the JPA static metamodel
(e.g., replace root.get("timeStamp") and root.get("visaEventType") with
VisaLog_.timeStamp and VisaLog_.visaEventType) or extract a typed Specification
factory method (e.g., createVisaLogSpecification(...) used by findFiltered,
similar to UserLogService.findFiltered) that centralizes attribute access so
renames are caught at compile time.
In
`@src/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.java`:
- Line 41: The security rule currently allows any HTTP method for "/user/logout"
in SecurityConfig by using requestMatchers("/user/logout").authenticated();
restrict this to POST to match the controller by changing the matcher to target
POST (e.g., use requestMatchers(HttpMethod.POST, "/user/logout").authenticated()
or the framework's post-specific matcher) and add the necessary HttpMethod
import; reference SecurityConfig and UserViewController.logout to ensure the
rule aligns with the `@PostMapping` handler.
- Line 60: Remove the HTTP Basic configuration call from SecurityConfig by
deleting the .httpBasic(withDefaults()) invocation inside the HttpSecurity
configuration (the method configuring HttpSecurity in class SecurityConfig);
ensure formLogin() remains configured and that any static import of
withDefaults() is removed, and run/adjust security tests to reflect that HTTP
Basic is no longer enabled.
In
`@src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java`:
- Around line 299-301: Remove the dead OPEN_STATUSES and HANDLED_STATUSES static
fields and the stale comment that claims they silence unused-field warnings in
VisaViewController; delete those fields and the comment, remove the unused
import of VisaStatus, and then verify no other code in VisaViewController (e.g.,
back-link logic using user.userAuthorization()/UserAuthorization) references
VisaStatus so the class still compiles.
In `@src/main/resources/static/css/components.css`:
- Around line 139-153: The global element selectors in components.css (input,
select, textarea, table, tr:hover td) apply site-wide and will unintentionally
style any future inputs/tables; to fix, narrow the selectors by scoping them
under a namespace/class (for example replace bare selectors with .form-group
input, .form-group select, .form-group textarea for form controls and
.data-table table, .data-table tr:hover td for tables) and move the existing
rules (width, background, resize, hover borders, etc.) into those scoped
selectors so global tags are no longer overridden.
- Line 187: The CSS rule uses the WebKit-specific pseudo-element
input[type="file"]::-webkit-file-upload-button which breaks cross-browser
support; replace that selector with the standard
input[type="file"]::file-selector-button and keep the same declarations so the
file input button styling works in Firefox and other non‑Chromium browsers
(update the selector occurrence for
input[type="file"]::-webkit-file-upload-button to
input[type="file"]::file-selector-button).
In `@src/main/resources/static/css/tokens.css`:
- Around line 128-129: The current CSS token alias --radius-pill equals
--radius-lg (12px), which will fail to produce true pill shapes for taller
elements; change the --radius-pill token to a height-independent large value
(e.g., 9999px or 999em) instead of 12px so pill buttons/badges always render
fully rounded, and update any related comment to note it's intentionally a
max-radius for pill shapes (referencing the --radius-lg and --radius-pill
tokens).
- Around line 120-121: The stylelint rule value-keyword-case is flagging
proper-case font names as false positives in the CSS variables --font-system and
--font-mono; fix by silencing the rule for these declarations — either add an
inline suppression comment immediately before/after the --font-system and
--font-mono lines (e.g. a stylelint-disable-next-line/value-keyword-case around
those declarations) or update the stylelint config to ignore value-keyword-case
for font-family values (e.g. allow-list or an overrides pattern for tokens.css)
so the font names remain properly cased.
In `@src/main/resources/templates/log/user.html`:
- Around line 8-41: Extract the duplicated page-scoped CSS block (the rules for
.container, .log-count, .filter-bar, .pager and related selectors) out of
user.html and visa.html into a single shared stylesheet or Thymeleaf fragment
(e.g., components.css or a fragment like fragments/log-styles) and replace the
inline <style> in both templates with a single include: either a <link
rel="stylesheet"> to the shared CSS or a Thymeleaf th:insert/th:replace of the
fragment; ensure the shared resource contains the .container, .log-count,
.filter-bar, .filter-bar .field, .filter-bar label, .filter-bar input/select,
.filter-bar .actions, .pager and .pager .page-info rules so both pages use the
same source of truth.
In `@src/main/resources/templates/visa/cases.html`:
- Around line 29-32: The template uses the model attribute ${name} instead of
the project's standard ${currentUser.fullName}; update the view in cases.html to
render ${currentUser.fullName} (replace ${name}) so it matches other visa
templates, or if the controller currently only supplies "name" instead of
"currentUser", change the controller to add a "currentUser" model attribute
(with a fullName property) and keep cases.html using ${currentUser.fullName} to
maintain consistency with apply-form.html and edit-form.html.
- Around line 37-188: The three near-identical tables for "Open Cases",
"Unassigned Cases" and "Handled Cases" should be extracted into a reusable
Thymeleaf fragment (e.g. fragments/cases :: section(title, list, emptyMsg,
countSuffix)) to avoid duplication; create the fragment that accepts a title,
the list variable (iterated as visa), an empty-state message and count suffix,
move the table markup (including the span with class case-count and th:text
logic, the th:each="visa : ${...}", the status span with th:classappend and the
date formatting using `#temporals`) into that fragment, then replace each table
block with a fragment invocation passing the appropriate title, list (openCases
/ unassignedCases / handledCases), empty message and suffix.
In `@src/main/resources/templates/visa/details.html`:
- Around line 181-195: The template's th:if currently checks the display label
visa.handlerName == 'Unassigned' which is brittle; change the condition to check
the structural handler identifier (e.g., visa.handlerId == null) or a dedicated
boolean on the DTO instead of handlerName so the banner renders regardless of
label/localization; update the th:if expression that references visa.handlerName
to use visa.handlerId (or the boolean flag) and ensure the rest of the block
(the Action Required banner and Edit link) remains unchanged.
In
`@src/test/java/org/example/visacasemanagementsystem/audit/controller/LogViewControllerTest.java`:
- Around line 99-101: Remove the redundant `@WithMockUser` annotations from the
tests that call loginAsSysadmin(), since loginAsSysadmin() sets the
SecurityContext directly and overrides the annotation; locate test methods such
as visaLog_AsSysadmin_NoFilters_ShouldReturnPageWithDefaults() (and the other
tests at the commented ranges) and delete the `@WithMockUser`(roles = "SYSADMIN")
declarations so the tests rely solely on the loginAsSysadmin() helper for
authentication.
In
`@src/test/java/org/example/visacasemanagementsystem/visa/VisaServiceTest.java`:
- Around line 311-446: Extract a small test helper in VisaServiceTest named
something like visaDtoWithId(Long id) that returns a VisaDTO instance with the
given id and default/null values for other fields, then replace all inline new
VisaDTO(...15 nulls...) uses (in tests
findOpenCasesByHandler_shouldMapEntitiesToDTOs_andQueryAssignedAndIncomplete,
findUnassignedCases_shouldMapEntitiesToDTOs_andQuerySubmittedWithNullHandler,
findHandledCasesByHandler_shouldMapEntitiesToDTOs_andQueryGrantedAndRejected,
etc.) with calls to visaDtoWithId(id) to centralize construction and make tests
resilient to DTO field changes.
In
`@src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java`:
- Around line 517-522: The test oldPluralPath_ShouldNoLongerResolve in
VisaViewControllerTest only verifies VisaViewController (because of
`@WebMvcTest`(VisaViewController.class)), so it cannot guarantee the old
/visas/dashboard path is absent application-wide; either change the test to a
full-context integration test by using `@SpringBootTest` with
`@AutoConfigureMockMvc` (and reuse MockMvc) to assert a 404 across the entire
application, or simply update the test comment to state it only checks
VisaViewController scope (not the whole app) to avoid overclaiming. Ensure
updates reference VisaViewControllerTest, the
oldPluralPath_ShouldNoLongerResolve test method, and the
`@WebMvcTest`(VisaViewController.class) annotation so reviewers can locate and
apply the change.
🪄 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: e8d8cf9c-d50b-4c1d-bc26-b95e36146562
📒 Files selected for processing (38)
src/main/java/org/example/visacasemanagementsystem/ApplicationViewController.javasrc/main/java/org/example/visacasemanagementsystem/audit/controller/LogViewController.javasrc/main/java/org/example/visacasemanagementsystem/audit/repository/UserLogRepository.javasrc/main/java/org/example/visacasemanagementsystem/audit/repository/VisaLogRepository.javasrc/main/java/org/example/visacasemanagementsystem/audit/service/UserLogService.javasrc/main/java/org/example/visacasemanagementsystem/audit/service/VisaLogService.javasrc/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.javasrc/main/java/org/example/visacasemanagementsystem/exception/GlobalExceptionHandler.javasrc/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.javasrc/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.javasrc/main/java/org/example/visacasemanagementsystem/visa/repository/VisaRepository.javasrc/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.javasrc/main/resources/static/css/components.csssrc/main/resources/static/css/tokens.csssrc/main/resources/templates/dashboard/admin.htmlsrc/main/resources/templates/dashboard/applicant.htmlsrc/main/resources/templates/dashboard/sysadmin.htmlsrc/main/resources/templates/error/error.htmlsrc/main/resources/templates/fragments/header.htmlsrc/main/resources/templates/log/user.htmlsrc/main/resources/templates/log/visa.htmlsrc/main/resources/templates/profile/edit.htmlsrc/main/resources/templates/profile/view.htmlsrc/main/resources/templates/user/list.htmlsrc/main/resources/templates/user/login.htmlsrc/main/resources/templates/user/signup.htmlsrc/main/resources/templates/visa/apply-form.htmlsrc/main/resources/templates/visa/cases.htmlsrc/main/resources/templates/visa/dashboard.htmlsrc/main/resources/templates/visa/details.htmlsrc/main/resources/templates/visa/edit-form.htmlsrc/main/resources/templates/visa/my-applications.htmlsrc/test/java/org/example/visacasemanagementsystem/audit/controller/LogViewControllerTest.javasrc/test/java/org/example/visacasemanagementsystem/audit/service/UserLogServiceTest.javasrc/test/java/org/example/visacasemanagementsystem/audit/service/VisaLogServiceTest.javasrc/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaServiceTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java
💤 Files with no reviewable changes (4)
- src/main/resources/templates/dashboard/admin.html
- src/main/resources/templates/visa/dashboard.html
- src/main/resources/templates/dashboard/sysadmin.html
- src/main/resources/templates/dashboard/applicant.html
This was
linked to
issues
Apr 24, 2026
…g service methods as well as CSS and template refactoring.
…ed failing tests from earlier fixes.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/resources/templates/visa/details.html (1)
107-112:⚠️ Potential issue | 🟡 MinorIncomplete handler-name → handler-id migration — "Waiting for assignment" branch is unreachable.
Line 110 still uses
visa.handlerName != 'Unassigned', but elsewhere in this PR the sentinel for "no handler" ishandlerId == null(Line 181) andhandlerName == null(fragmentcases.htmlLine 46). SincehandlerNameisnull(not the literal string"Unassigned") for unassigned cases, the ternary is always true and always renders the (null/blank)visa.handlerName; the "Waiting for assignment" fallback never fires.🐛 Proposed fix
- <p class="user-info" th:if="${currentUser.userAuthorization.name() != 'USER'}"> - Handled by: <span th:text="${visa.handlerName != 'Unassigned' ? visa.handlerName : 'Waiting for assignment'}"></span> - </p> + <p class="user-info" th:if="${currentUser.userAuthorization.name() != 'USER'}"> + Handled by: <span th:text="${visa.handlerId != null ? visa.handlerName : 'Waiting for assignment'}"></span> + </p>🤖 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 107 - 112, The template's ternary uses the literal string check visa.handlerName != 'Unassigned' which no longer matches the migration where unassigned cases use null; update the condition in the header block so the fallback "Waiting for assignment" is used when visa.handlerId is null or visa.handlerName is null (e.g., change the th:if / ternary to check visa.handlerId == null or visa.handlerName == null and render handlerName otherwise), ensuring the same sentinel as used elsewhere (handlerId == null / handlerName == null).
🧹 Nitpick comments (1)
src/main/resources/templates/fragments/cases.html (1)
25-29: Prefer#lists.isEmpty(cases)for null-safety and consistency.
cases.isEmpty()will NPE if a caller ever passesnullfor thecasesparameter, and it's inconsistent with other templates in this PR (e.g.,details.htmluses#lists.isEmpty(visa.downloadUrls)). Today's callers always pass a non-null list from the controller, but the fragment is a reusable abstraction and should defend against future callers.♻️ Proposed fix
- <div th:if="${cases.isEmpty()}" class="empty-state" th:text="${emptyMsg}"> + <div th:if="${`#lists.isEmpty`(cases)}" class="empty-state" th:text="${emptyMsg}"> No cases. </div> - <table th:if="${!cases.isEmpty()}"> + <table th:if="${!#lists.isEmpty(cases)}">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/fragments/cases.html` around lines 25 - 29, Replace the direct calls to cases.isEmpty() with Thymeleaf's null-safe list utility: change th:if="${cases.isEmpty()}" to th:if="${`#lists.isEmpty`(cases)}" and the table condition th:if="${!cases.isEmpty()}" to th:if="${!#lists.isEmpty(cases)}"; this keeps the fragment null-safe and consistent with other templates (e.g., usage of `#lists.isEmpty`(visa.downloadUrls)).
🤖 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/audit/controller/LogViewController.java`:
- Around line 5-6: Remove the unused DTO imports in LogViewController: delete
the import statements for
org.example.visacasemanagementsystem.audit.dto.UserLogDTO and
org.example.visacasemanagementsystem.audit.dto.VisaLogDTO from the
LogViewController class so spotless:check passes; verify no references to
UserLogDTO or VisaLogDTO remain in LogViewController before committing.
In `@src/main/resources/templates/error/error.html`:
- Line 74: The header fragment include currently uses th:replace with
sec:authorize on the same <nav> element (fragments/header :: nav), but fragment
processing runs before the security check so anonymous users can see it; fix by
moving the security check to a separate wrapper (e.g., add a surrounding element
with sec:authorize="isAuthenticated()" that contains the th:replace call for
fragments/header :: nav) so Thymeleaf only includes the fragment for
authenticated users.
---
Outside diff comments:
In `@src/main/resources/templates/visa/details.html`:
- Around line 107-112: The template's ternary uses the literal string check
visa.handlerName != 'Unassigned' which no longer matches the migration where
unassigned cases use null; update the condition in the header block so the
fallback "Waiting for assignment" is used when visa.handlerId is null or
visa.handlerName is null (e.g., change the th:if / ternary to check
visa.handlerId == null or visa.handlerName == null and render handlerName
otherwise), ensuring the same sentinel as used elsewhere (handlerId == null /
handlerName == null).
---
Nitpick comments:
In `@src/main/resources/templates/fragments/cases.html`:
- Around line 25-29: Replace the direct calls to cases.isEmpty() with
Thymeleaf's null-safe list utility: change th:if="${cases.isEmpty()}" to
th:if="${`#lists.isEmpty`(cases)}" and the table condition
th:if="${!cases.isEmpty()}" to th:if="${!#lists.isEmpty(cases)}"; this keeps the
fragment null-safe and consistent with other templates (e.g., usage of
`#lists.isEmpty`(visa.downloadUrls)).
🪄 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: 81f459cc-dc30-4a87-9de9-28f4251be530
📒 Files selected for processing (20)
pom.xmlsrc/main/java/org/example/visacasemanagementsystem/audit/controller/LogViewController.javasrc/main/java/org/example/visacasemanagementsystem/audit/service/UserLogService.javasrc/main/java/org/example/visacasemanagementsystem/audit/service/VisaLogService.javasrc/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.javasrc/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.javasrc/main/resources/static/css/components.csssrc/main/resources/static/css/tokens.csssrc/main/resources/templates/error/error.htmlsrc/main/resources/templates/fragments/cases.htmlsrc/main/resources/templates/log/user.htmlsrc/main/resources/templates/log/visa.htmlsrc/main/resources/templates/visa/cases.htmlsrc/main/resources/templates/visa/details.htmlsrc/main/resources/templates/visa/my-applications.htmlsrc/test/java/org/example/visacasemanagementsystem/audit/controller/LogViewControllerTest.javasrc/test/java/org/example/visacasemanagementsystem/audit/service/UserLogServiceTest.javasrc/test/java/org/example/visacasemanagementsystem/audit/service/VisaLogServiceTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaServiceTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java
✅ Files skipped from review due to trivial changes (6)
- src/main/resources/templates/visa/cases.html
- src/main/resources/static/css/components.css
- src/main/resources/templates/visa/my-applications.html
- src/main/resources/templates/log/visa.html
- src/test/java/org/example/visacasemanagementsystem/audit/service/VisaLogServiceTest.java
- src/test/java/org/example/visacasemanagementsystem/visa/VisaServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (8)
- src/main/resources/static/css/tokens.css
- src/main/java/org/example/visacasemanagementsystem/audit/service/UserLogService.java
- src/main/resources/templates/log/user.html
- src/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.java
- src/test/java/org/example/visacasemanagementsystem/audit/controller/LogViewControllerTest.java
- src/main/java/org/example/visacasemanagementsystem/audit/service/VisaLogService.java
- src/test/java/org/example/visacasemanagementsystem/audit/service/UserLogServiceTest.java
- src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java
Contributor
Author
|
@coderabbitai Can you do check the pull request once more since the rate limit was exceeded last? |
|
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (18)
src/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.java (1)
467-502: Optional: extract status groupings to constants.
openStatusesandhandledStatusesare recomputed on every call. Hoisting them toprivate static final List<VisaStatus>constants (e.g.OPEN_STATUSES,HANDLED_STATUSES) would clarify intent and avoid per-invocation list allocation. Non-blocking.♻️ Proposed refactor
+ private static final List<VisaStatus> OPEN_STATUSES = + List.of(VisaStatus.ASSIGNED, VisaStatus.INCOMPLETE); + private static final List<VisaStatus> HANDLED_STATUSES = + List.of(VisaStatus.GRANTED, VisaStatus.REJECTED); + public List<VisaDTO> findOpenCasesByHandler(Long handlerId) { - List<VisaStatus> openStatuses = List.of(VisaStatus.ASSIGNED, VisaStatus.INCOMPLETE); return visaRepository.findByHandler_IdAndVisaStatusIn( handlerId, - openStatuses, + OPEN_STATUSES, Sort.by("updatedAt").descending()) .stream() .map(visaMapper::toDTO) .toList(); } @@ public List<VisaDTO> findHandledCasesByHandler(Long handlerId) { - List<VisaStatus> handledStatuses = List.of(VisaStatus.GRANTED, VisaStatus.REJECTED); return visaRepository.findByHandler_IdAndVisaStatusIn( handlerId, - handledStatuses, + HANDLED_STATUSES, Sort.by("updatedAt").descending())🤖 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/service/VisaService.java` around lines 467 - 502, Extract the per-call lists into class-level constants to avoid reallocations: define private static final List<VisaStatus> OPEN_STATUSES = List.of(VisaStatus.ASSIGNED, VisaStatus.INCOMPLETE) and private static final List<VisaStatus> HANDLED_STATUSES = List.of(VisaStatus.GRANTED, VisaStatus.REJECTED) in VisaService, then replace the local openStatuses and handledStatuses variables in findOpenCasesByHandler(Long) and findHandledCasesByHandler(Long) to use OPEN_STATUSES and HANDLED_STATUSES respectively (keep the Sort.by("updatedAt").descending() and mapping via visaMapper::toDTO unchanged).src/test/java/org/example/visacasemanagementsystem/audit/service/VisaLogServiceTest.java (1)
207-222: Optional: extract the Criteria-mock setup into a helper.Tests for eventType-only / from-only / to-only repeat ~6 identical lines mocking
Root/CriteriaQuery/CriteriaBuilder/Path/Predicateand aroot.get(nullable(SingularAttribute.class))stub. A small helper (e.g.CriteriaMocks fixture()) would cut duplication and let each test focus on the assertion that differs.Not blocking — the current shape is readable.
Also applies to: 242-256, 277-291
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/audit/service/VisaLogServiceTest.java` around lines 207 - 222, Tests in VisaLogServiceTest repeat the same Criteria mocks (Root, CriteriaQuery, CriteriaBuilder, Path, Predicate) and the root.get(nullable(SingularAttribute.class)) stub across multiple cases; extract that repeated setup into a small helper (e.g., a private method like setupCriteriaMocks or a CriteriaMocks fixture) that creates and stubs Root, CriteriaQuery, CriteriaBuilder, Path and returns the mocks (or assigns fields) so each test can call the helper and then only assert the specific behavior of specCaptor.getValue().toPredicate(root, query, cb) and verifications of cb.equal / greaterThanOrEqualTo / lessThanOrEqualTo; update tests at the other locations (around lines noted) to use the helper to remove the duplicated when(...) and mock(...) lines while keeping existing assertions and specCaptor usage.src/main/resources/templates/fragments/header.html (1)
41-43: Add an explicit CSRF hidden input to the logout form for defensive consistency, or clarify why login/signup/apply forms rely on auto-injection.The logout form (lines 41–43) relies on Thymeleaf's Spring Security dialect to auto-inject the CSRF token on POST, while visa operation forms (
details.htmllines 208, 222, 230) and profile edit forms (profile/edit.htmllines 40–41, 80–81) include explicit<input type="hidden" th:name="${_csrf.parameterName}" th:value="${_csrf.token}" />tokens. However, authentication forms (login.html,signup.html) and the visa apply form (apply-form.html) also omit explicit CSRF, suggesting auto-injection is working across the codebase. If you prefer defensive explicitness, add the CSRF input to the logout form; if auto-injection is reliable, the current form is fine. Either way, the inconsistency should be documented or standardized across all POST forms.♻️ Proposed explicit CSRF input (optional)
<form th:action="@{/user/logout}" method="post" class="logout-form"> + <input type="hidden" + th:name="${_csrf.parameterName}" + th:value="${_csrf.token}" /> <button type="submit" class="btn-signout">Sign Out</button> </form>Resolves the deferred
/visas/logoutlearning: Sign Out now delegates to/user/logoutvia a POST form, matching Spring Security conventions.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/fragments/header.html` around lines 41 - 43, The logout POST form (form with th:action="@{/user/logout}" and class "logout-form" containing button "btn-signout") should include an explicit CSRF hidden input for consistency with other forms; add an input using th:name="${_csrf.parameterName}" and th:value="${_csrf.token}" inside that form (or, if you intentionally rely on Thymeleaf/Spring Security auto-injection, add a short comment in the template explaining that auto-injection is relied upon) so all POST forms are standardized.src/main/resources/templates/visa/edit-form.html (2)
79-79: Inconsistent record-accessor style vs other updated templates in this PR.This file uses property-style access (
${currentUser.fullName},${updateVisaDto.visaStatus},${updateVisaDto.id},${visa.id}, etc.), while sibling templates updated in this PR (e.g.,visa/my-applications.html) consistently use method-style access (${currentUser.fullName()},${visa.id()}). Both work with Spring 6's SpEL on records, but the mixed convention makes the codebase harder to grep and maintain.For consistency, either standardize this template on method-style accessors (
fullName(),id(),visaStatus(),downloadUrls(),s3Keys()) or revisitmy-applications.htmlto use property-style — but pick one and stick with it across the visa templates touched here.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/visa/edit-form.html` at line 79, This template mixes property-style SpEL with the rest of the PR; update all record field accesses in edit-form.html to use method-style accessors to match the other templates (e.g., change occurrences of currentUser.fullName, updateVisaDto.visaStatus, updateVisaDto.id, visa.id and any downloadUrls/s3Keys references to currentUser.fullName(), updateVisaDto.visaStatus(), updateVisaDto.id(), visa.id() and downloadUrls()/s3Keys() respectively) so the visa templates consistently use the method-style record accessors.
137-141: Inlineth:onclickwith confirm()+JS DOM lookup is fragile and CSP-unfriendly.The current pattern submits a hidden form via inline
onclickscript generated by Thymeleaf. This works but:
- Inline event handlers fail under a strict CSP (
script-srcwithout'unsafe-inline').- The fallback if the user clicks Cancel is implicit (the boolean short-circuit).
- The form is rendered in a separate sibling block (line 164–170), so the markup relies on element-id wiring across far-apart sections.
A cleaner alternative is to inline the delete
<form>directly in the document row and use a single inline submit button with aconfirmhandler, eliminating the cross-element id coupling entirely. This is optional — current behavior is correct.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/visa/edit-form.html` around lines 137 - 141, The inline th:onclick that uses confirm(...) and document.getElementById('delete-doc-' + ${iter.index}).submit() is CSP-unfriendly and couples the button to a distant hidden form; instead move the delete <form> (the one currently rendered as the hidden sibling with id 'delete-doc-<index>') into the same document row as the remove <button> and replace the inline th:onclick wiring with a plain submit button inside that form (use <button type="submit">) and, if you still want a JS confirmation, attach a non-inline event listener (or use a data-confirm attribute that a central script handles) rather than using th:onclick; update references to delete-doc-${iter.index} and the button markup in the template accordingly.src/main/resources/templates/visa/my-applications.html (1)
33-68: Empty state rendered below the table — table header still shows when there are no applications.When
${visas}is empty, the<table>(with its headers) still renders followed by the "You have no applications yet." message, producing an empty table with column headers. Consider rendering either the table or the empty-state, not both.♻️ Suggested structure
<main> - <table> + <table th:if="${not `#lists.isEmpty`(visas)}"> <thead> ... </thead> <tbody> <tr th:each="visa : ${visas}"> ... </tr> </tbody> </table> <div th:if="${`#lists.isEmpty`(visas)}" class="empty-state"> You have no applications yet. </div> </main>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/visa/my-applications.html` around lines 33 - 68, The template currently renders the <table> (with headers) unconditionally and separately renders the empty-state when ${visas} is empty, causing an empty table to appear; change the structure so the table (the <table> element that iterates th:each="visa : ${visas}" and the header row) is rendered only when the list is not empty and the "You have no applications yet." message is rendered when `#lists.isEmpty`(visas) is true — e.g., wrap the entire table with th:if="${!#lists.isEmpty(visas)}" (or move the th:if from the empty-state to the table inversion) and keep the existing empty-state div with th:if="${`#lists.isEmpty`(visas)}"; ensure the th:each on visa and the view link href (@{/visa/{id}(id=${visa.id()})}) remain unchanged.src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java (1)
513-525:createMockUseris a misleadingly named helper — it has a hidden side effect onSecurityContextHolder.The method's name suggests it just creates a
UserDTO, but it also mutates globalSecurityContextHolderstate viasetAuthentication(...). Several tests call it purely for the auth side-effect and discard the return value (e.g., lines 146, 300, 364, 385, 403, 423). This makes the tests harder to reason about and easy to misuse.Combined with
@WithMockUseron every test (which Spring Security's test infrastructure already uses to populate the security context), there are two competing mechanisms setting authentication — the explicitsetAuthenticationhere ends up overriding@WithMockUser. Consider either:
- Splitting the helper into
createMockUserDTO(...)(pure) andmockAuthenticatedAs(...)(side-effecting), or- Dropping
@WithMockUserand using only the explicit principal setup, or- Switching fully to Spring Security test support (e.g.,
with(user(...))/@WithUserDetails) and removing the manualSecurityContextHolderwrites.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java` around lines 513 - 525, createMockUser currently both returns a UserDTO and mutates global SecurityContextHolder via SecurityContextHolder.getContext().setAuthentication(...), which is misleading and conflicts with `@WithMockUser`; split it into a pure factory (e.g., createMockUserDTO(Long id, UserAuthorization role) that only constructs and returns UserDTO/User objects) and a side-effecting helper (e.g., mockAuthenticatedAs(User user) or mockAuthenticatedAs(UserDTO dto) that creates a UserPrincipal and TestingAuthenticationToken and calls SecurityContextHolder.getContext().setAuthentication(...)); update tests that only need the DTO to call the pure factory and tests that need to set authentication to call the new mockAuthenticatedAs helper (or remove the manual SecurityContextHolder writes and rely on `@WithMockUser` / with(user(...)) consistently).src/main/java/org/example/visacasemanagementsystem/audit/service/UserLogService.java (1)
45-62: LGTM — clean dynamic Specification with proper null-filter handling.The use of
Specification.unrestricted()as the starting point and the conditionaland(...)chaining cleanly avoids binding null enum parameters in JPQL, and the mapping viaPage#mappreserves pagination metadata.Note:
findFilteredhere mirrorsVisaLogService#findFilteredvery closely (same shape, same predicate construction). If a third audit-log type ever lands, consider extracting the predicate-builder pattern (e.g., a small helper that takes a metamodel attribute + value and returns anandlambda) into a shared utility, but this is purely an optional cleanup.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/audit/service/UserLogService.java` around lines 45 - 62, No functional change required—findFiltered in UserLogService is correct; optionally extract the repeated predicate-construction used by UserLogService#findFiltered and VisaLogService#findFiltered into a small shared helper (e.g., SpecificationUtils.buildEqualsOrRange) that accepts a metamodel attribute (UserLog_.userEventType/UserLog_.timeStamp) and a value or range bounds and returns a Specification to be and()-chained; implement the helper to produce the same lambdas (equal, greaterThanOrEqualTo, lessThanOrEqualTo) so both services can call the helper instead of duplicating the spec construction.src/main/java/org/example/visacasemanagementsystem/ApplicationViewController.java (1)
27-40: LGTM — role-based routing aligns withSecurityConfigand downstream@PreAuthorizeguards.The three redirect targets (
/log/visa,/visa/cases,/visa/my-applications) all have matching role protections (perSecurityConfig/log/**→ SYSADMIN and@PreAuthorizeon the visa controller methods), so the routing is consistent.Optional micro-tweak: the authorities stream is iterated twice. A single pass would be slightly more efficient and read more uniformly:
♻️ Optional simplification
- boolean isSysAdmin = principal.getAuthorities().stream() - .anyMatch(a -> Objects.equals(a.getAuthority(), "ROLE_SYSADMIN")); - boolean isAdmin = principal.getAuthorities().stream() - .anyMatch(a -> Objects.equals(a.getAuthority(), "ROLE_ADMIN")); - - if (isSysAdmin) return "redirect:/log/visa"; - if (isAdmin) return "redirect:/visa/cases"; - return "redirect:/visa/my-applications"; + for (var authority : principal.getAuthorities()) { + String role = authority.getAuthority(); + if ("ROLE_SYSADMIN".equals(role)) return "redirect:/log/visa"; + if ("ROLE_ADMIN".equals(role)) return "redirect:/visa/cases"; + } + return "redirect:/visa/my-applications";Note: the original code's behavior of preferring SYSADMIN over ADMIN when both are present is preserved only if the iteration finds SYSADMIN first. If a principal can hold both roles and ordering matters, keep the explicit two-pass version.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/ApplicationViewController.java` around lines 27 - 40, The home method currently iterates principal.getAuthorities() twice to compute isSysAdmin and isAdmin; collapse this into a single pass over principal.getAuthorities() (inside home) that sets two booleans (isSysAdmin, isAdmin) as it sees each authority so you avoid double iteration while preserving the existing preference logic (ensure SYSADMIN redirect still takes precedence if both flags end up true). Locate the home method and update the authority-check logic that produces isSysAdmin and isAdmin accordingly.src/main/resources/templates/error/error.html (1)
70-94: LGTM — auth-aware header and CTAs are correctly wired.The header is now wrapped in a
th:block sec:authorize="isAuthenticated()"(so the previously-flaggedth:replaceprecedence bypass is no longer reachable), the CTAs are gated bysec:authorizefor authenticated vs anonymous users, and the.no-navclass trick keeps the layout vertically centered when no nav is rendered.One small consideration: if
errorTitle/errorMessagearen't populated by the upstream handler (e.g., a Spring Security filter-chain failure that bypassesGlobalExceptionHandler), lines 80–81 will render empty strings. Adding a default via Elvis would harden the page:♻️ Optional null-safe defaults
- <div class="error-code" th:text="${errorTitle}">⚠️Error.</div> - <h1 th:text="${errorMessage}">Something went wrong</h1> + <div class="error-code" th:text="${errorTitle ?: 'Error'}">⚠️Error.</div> + <h1 th:text="${errorMessage ?: 'Something went wrong'}">Something went wrong</h1>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/error/error.html` around lines 70 - 94, Add null-safe defaults for the errorTitle and errorMessage bindings so the page shows sensible text when the upstream handler doesn't populate them: update the th:text on the element rendering errorTitle (the element with class "error-code") to use the Elvis/default operator with a fallback like "⚠️ Error" and update the th:text on the h1 rendering errorMessage to use an Elvis/default fallback like "Something went wrong"; keep the same attributes (th:text) and security gating unchanged.src/test/java/org/example/visacasemanagementsystem/audit/service/UserLogServiceTest.java (1)
340-361: LGTM — solid coverage of the newSpecificationpath.The eventType / from / to / all-filters cases correctly capture the built
Specificationand verify the predicate composition by replaying it against mocked Criteria API components. One thing worth flagging as an awareness item rather than a defect: the all-filters test at Line 348 stubsroot.get(...)to return paths in a fixed sequence (eventTypePath, timeStampPath, timeStampPath), so it implicitly couples the test to the order in whichfindFilteredcallsroot.get(...)and to the order ofcb.and(...)chaining. If the service is later reordered to combine predicates in a different sequence (e.g., from/to before eventType), this test will fail in a way that looks like a regression even though behavior is unchanged. Worth a comment noting the assumption, or a refactor towardargThat-style matchers keyed on the metamodel attribute.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/audit/service/UserLogServiceTest.java` around lines 340 - 361, The test is brittle because it stubs root.get(...) and cb.and(...) with a fixed sequence (eventTypePath, timeStampPath, timeStampPath) which couples the specCaptor verification to call order; update the test to either add a clear comment describing that assumption near the specCaptor verification or refactor the stubbing to match by attribute rather than position (use argument matchers when stubbing root.get(...) and cb.equal/greaterThanOrEqualTo/lessThanOrEqualTo so you return eventTypePath/timeStampPath based on the passed SingularAttribute, or use argThat on the metamodel attribute), ensuring specCaptor.getValue().toPredicate(root, query, cb) is driven by attribute-aware stubs instead of sequence-dependent returns for root.get, and keep references to root.get, cb.equal, cb.greaterThanOrEqualTo, cb.lessThanOrEqualTo, and specCaptor in your changes.src/main/resources/static/css/components.css (1)
129-153: Heavy global restyling oflabelandinput/select/textareamay unintentionally bleed into pages outside this PR's scope.
label { display: block; text-transform: uppercase; … }and theinput, select, textareablock (which forceswidth: 100%,resize: none, and a dark-theme color scheme) apply to every form element on any page that linkscomponents.css. A few practical consequences worth being aware of:
- Any inline form (e.g., a label adjacent to a checkbox/radio) loses inline alignment because of the forced
display: block.resize: noneontextareais then overridden ad-hoc indetails.html(resize: vertical) — the inverse of the usual default — making it easy to forget on new templates.- Future widget styles (e.g., the recommended
.file-input-labelfrom the prior file-input discussion) will inherit the uppercase 11px reset and need explicit overrides (note: you've already preempted this withtext-transform: nonein the suggested.file-input-labelrule).Not a blocker — this is a deliberate "shared = canonical" trade-off — but consider scoping the broad selectors to a wrapper class (e.g.,
.form-group label,.form-group input) so opt-in is explicit and the global cascade stays small.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/css/components.css` around lines 129 - 153, The current global selectors (label and input, select, textarea) are too broad; restrict them by scoping to a wrapper such as .form-group (e.g., change label to .form-group label and input, select, textarea to .form-group input, .form-group select, .form-group textarea), move properties that break defaults (display:block, width:100%, resize:none, color) into that scoped rule, and ensure existing exceptions like .file-input-label and details.html’s textarea override remain or are updated to .form-group-specific rules so other pages won’t inherit these styles unintentionally.src/main/resources/templates/fragments/cases.html (1)
50-50: Use.name()instead of relying on enumtoString()for the CSS class.
#strings.toLowerCase(visa.visaStatus)works because Thymeleaf coerces the enum viatoString(), which by default returnsname(). If anyone overridesVisaStatus.toString()later (a common practice for human-readable labels), the CSS class names silently break (e.g.,status-grantedbecomesstatus-visa granted). Use.name()to lock the contract, mirroring the explicit pattern used inlog/visa.htmlandlog/user.html:♻️ Suggested change
- <span class="status" - th:classappend="${'status-' + `#strings.toLowerCase`(visa.visaStatus)}" - th:text="${visa.visaStatus}">ASSIGNED</span> + <span class="status" + th:classappend="${'status-' + `#strings.toLowerCase`(visa.visaStatus.name())}" + th:text="${visa.visaStatus}">ASSIGNED</span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/fragments/cases.html` at line 50, The Thymeleaf expression currently coerces the enum via toString() in the th:classappend attribute (th:classappend="${'status-' + `#strings.toLowerCase`(visa.visaStatus)}"), which can break if VisaStatus.toString() is overridden; change the expression to call the enum's name() explicitly and pass that into the lowercase helper (use visa.visaStatus.name() with `#strings.toLowerCase`) so the CSS class generation uses the stable enum name (referencing the th:classappend attribute and visa.visaStatus).src/main/resources/templates/log/visa.html (1)
30-37: Use.name()for<option>values to prevent future binding failures iftoString()is overridden.Currently,
th:value="${type}"serializes the enum viatoString(), which works today because bothVisaEventTypeandUserEventTypeuse the defaultEnum.toString()(returning the enum name). However, if either enum later overridestoString()to return a human-readable format (e.g.,"Created"), the rendered<option value="Created">will no longer round-trip through Spring's enum converter on form submit, andeventTypewill silently bind tonullinstead of the selected value.The display text can remain human-readable, but the option value should explicitly use the enum name to lock the wire format:
♻️ Suggested change (apply same to `log/user.html`)
<select id="eventType" name="eventType"> <option value="" th:selected="${selectedEventType == null}">All</option> <option th:each="type : ${eventTypes}" - th:value="${type}" + th:value="${type.name()}" th:text="${type}" th:selected="${selectedEventType != null and selectedEventType.name() == type.name()}"> </option> </select>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/log/visa.html` around lines 30 - 37, The select option values currently use th:value="${type}" which relies on Enum.toString() and can break binding if enums override toString(); change the value binding to use the enum name (th:value="${type.name()}") while keeping the display text (th:text) as-is for human-readable labels, and apply the same update to the analogous select in log/user.html; target the select with id/name "eventType" and enums like VisaEventType/UserEventType when making this change.src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java (1)
62-81: Three-listshowCaseswiring is correct, butmeIdis fetched twice.
principal.getUserId()is invoked at Line 66 (insidefindById) and again at Line 69 to populatemeId. They will always return the same value, so this is harmless — just slightly redundant. Optional cleanup: pullmeIdfirst and reuse it for thefindByIdcall:♻️ Optional cleanup
- public String showCases(`@AuthenticationPrincipal` UserPrincipal principal, Model model) { - UserDTO user = userService.findById(principal.getUserId()) - .orElseThrow(() -> new EntityNotFoundException("User not found.")); - - Long meId = principal.getUserId(); - - List<VisaDTO> openCases = visaService.findOpenCasesByHandler(meId); + public String showCases(`@AuthenticationPrincipal` UserPrincipal principal, Model model) { + Long meId = principal.getUserId(); + UserDTO user = userService.findById(meId) + .orElseThrow(() -> new EntityNotFoundException("User not found.")); + + List<VisaDTO> openCases = visaService.findOpenCasesByHandler(meId);Also worth confirming — given
templates/visa/cases.htmlaccesses${currentUser.fullName}(no parens) whilemy-applications.htmluses${currentUser.fullName()}— that both expression styles resolve correctly againstUserDTO(depends on whetherfullNameis exposed as a record accessor or a JavaBean property). IfUserDTOis a Javarecord, Thymeleaf's OGNL/SpringEL can usually resolve both forms, but inconsistency invites template-rendering bugs later.🤖 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 62 - 81, In showCases, avoid calling principal.getUserId() twice: read meId = principal.getUserId() first and reuse meId in the call to userService.findById(...) and the subsequent visaService methods (update references to UserDTO user = userService.findById(meId)... and keep model population unchanged); also verify UserDTO exposes a consistent fullName accessor (getter or record accessor) so templates using ${currentUser.fullName} and ${currentUser.fullName()} both resolve or update templates to one consistent form.src/test/java/org/example/visacasemanagementsystem/audit/controller/LogViewControllerTest.java (1)
130-135: Stub may not match the actualPageableproduced by the controller.You stub
findFiltered(eq(GRANTED), eq(expectedFrom), eq(expectedTo), any(Pageable.class))to return a non-empty page, but in the "no filters" test (Lines 103–104) you stub the same method withisNull()matchers. Because Mockito picks the most recent matching stub, this is OK in isolation, but be aware that if any of the eq-matched arguments don't match (e.g., the controller someday switches end-of-day toLocalTime.MAX.minusNanos(...)or an instant truncation), this stub silently returnsnulland the controller will NPE rendering the page — making the failure look unrelated to the actual mismatch. Consider usingwhen(...).thenReturn(page)withany()for date args plus a separateArgumentCaptor<LocalDateTime>assertion, so a regression on the conversion produces a clear, targeted assertion failure rather than a downstream NPE.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/audit/controller/LogViewControllerTest.java` around lines 130 - 135, The test stubs visaLogService.findFiltered(...) with strict eq(expectedFrom)/eq(expectedTo) which can silently stop matching if the controller alters date conversion; change the stub in LogViewControllerTest to use more flexible matchers (e.g., any(LocalDateTime.class) or any()) for the from/to arguments when calling when(visaLogService.findFiltered(...)). Return the prepared Page (PageImpl) from that stub, and add an ArgumentCaptor<LocalDateTime> (capturing the from/to arguments passed into visaLogService.findFiltered) to assert the controller's actual conversion/rounding behavior explicitly so regressions fail with a clear assertion instead of an NPE; keep the other matchers (e.g., eq(VisaEventType.GRANTED) or any(Pageable.class)) as appropriate.src/main/java/org/example/visacasemanagementsystem/audit/controller/LogViewController.java (2)
51-51:defaultValue = "" + DEFAULT_PAGE_SIZErelies on compile-time constant folding.Because
DEFAULT_PAGE_SIZEisstatic final intinitialized with a literal, JLS §15.28 makes"" + DEFAULT_PAGE_SIZEa constant expression, so the annotation is legal here. Worth being aware though that ifDEFAULT_PAGE_SIZEever stops being a compile-time constant (e.g., computed from a config value) this annotation will stop compiling. A small alternative is to define aString DEFAULT_PAGE_SIZE_STR = "20"constant alongside the int and reference that directly to make the dependency explicit.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/audit/controller/LogViewController.java` at line 51, The annotation uses defaultValue = "" + DEFAULT_PAGE_SIZE which relies on compile-time constant folding; replace this with an explicit String constant to avoid future compile errors: add a String constant (e.g., DEFAULT_PAGE_SIZE_STR = "20") alongside the existing static final int DEFAULT_PAGE_SIZE and update the `@RequestParam` on the size parameter in LogViewController (the method using the size param) to use DEFAULT_PAGE_SIZE_STR; ensure both constants are kept in sync or derive the int from the String if needed.
26-28: Belt-and-braces authorization — fine, but note the redundancy.
SecurityConfig.requestMatchers("/log/**").hasRole("SYSADMIN")already gates these URLs, so the class-level@PreAuthorize("hasRole('SYSADMIN')")is defensive duplication. Not a problem (extra layer of safety in case SecurityConfig is ever loosened), just worth documenting the intent so a future maintainer doesn't drop one of the two guards thinking it's dead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/audit/controller/LogViewController.java` around lines 26 - 28, The class-level `@PreAuthorize`("hasRole('SYSADMIN')") on LogViewController is intentionally duplicated with SecurityConfig.requestMatchers("/log/**").hasRole("SYSADMIN") as a defensive guard; add a short clarifying comment above the LogViewController class (or adjacent to the `@PreAuthorize` annotation) stating that this redundancy is deliberate to provide an extra layer of authorization protection in case SecurityConfig changes, so future maintainers don't remove one of the checks.
🤖 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/user/controller/UserViewController.java`:
- Around line 77-84: The logout POST is failing because the logout form in the
header fragment doesn't include the CSRF token; update the form in
src/main/resources/templates/fragments/header.html that posts to /user/logout
(used by the logout() method in UserViewController) to include a hidden input
with the CSRF parameter name and token (use _csrf.parameterName and _csrf.token)
so the Spring Security CSRF filter accepts the POST; ensure the form action
remains th:action="@{/user/logout}" and method="post".
In
`@src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java`:
- Around line 240-274: The PR broadened case-management endpoints approveVisa,
requestMoreInformation, rejectVisa, and assignCaseToHandler to
hasAnyRole('ADMIN','SYSADMIN'), which may violate separation-of-duties; if
SYSADMIN should remain audit-only, change those `@PreAuthorize` annotations back
to hasRole('ADMIN') on the four methods (approveVisa, requestMoreInformation,
rejectVisa, assignCaseToHandler) and ensure SYSADMIN retains read-only access
via the ApplicationViewController.home/view endpoints instead of mutation
rights; if the broader access was intentional, add a short code comment near
these methods documenting the product decision and update authorization tests to
reflect SYSADMIN can perform these actions.
In `@src/main/resources/static/css/components.css`:
- Around line 164-167: The current input:disabled rule reduces opacity to 0.4
which makes text (derived from --color-text-secondary over --color-input-bg)
fail WCAG contrast; update the input:disabled CSS rule to use higher opacity
(about 0.6) and/or explicitly set a higher-contrast color or distinct background
for disabled inputs (reference the variables --color-text-secondary and
--color-input-bg) so disabled-but-readable fields remain >=4.5:1 contrast —
modify the input:disabled selector to increase opacity and optionally add a
background-color or color override to preserve legibility.
- Around line 187-198: The CSS currently targets input[type="file"]::before
which is unsupported in Firefox/Safari; replace the nonstandard pseudo-element
usage by applying styling to the standard ::file-selector-button and keep the
existing ::-webkit-file-upload-button fallback, updating rules that reference
input[type="file"]::before to instead target
input[type="file"]::file-selector-button (and retain
::-webkit-file-upload-button for WebKit) so the fake button styling (background,
border, padding, margin-right, color, font-weight, font-size) applies
cross-browser.
In `@src/main/resources/static/css/tokens.css`:
- Around line 41-43: The variables --color-text-dimmer (`#555`) and
--color-placeholder (`#444`) do not meet WCAG AA contrast for normal text; update
--color-text-dimmer to at least `#767676` and raise --color-placeholder to a value
that yields ≥4.5:1 against dark backgrounds (or restrict its use to
non-essential decorative placeholders), then test the updated tokens against the
affected selectors (.empty-state, .site-nav .nav-user, ::placeholder and the "No
comments yet." message) to ensure they meet the contrast threshold or are only
used where WCAG AAA is acceptable for small text.
In `@src/main/resources/templates/fragments/cases.html`:
- Around line 53-54: The fragment unconditionally calls `#temporals.format` on
visa.travelDate and visa.updatedAt which throws when those fields are null;
update the two <td> cells to guard the calls (mirror the existing pattern used
elsewhere) by checking visa.travelDate != null and visa.updatedAt != null before
calling `#temporals.format` (e.g., use a conditional/th:if or a
ternary/Elvis-style expression in the th:text) so formatting runs only when the
corresponding visa.travelDate or visa.updatedAt is present.
In `@src/main/resources/templates/log/user.html`:
- Around line 73-77: The span element rendering user log event chips currently
uses the base "status" class and a th:classappend that produces
"status-<userEventType>" names (see the span with class="status" and the
th:classappend="${log.userEventType != null ? 'status-' +
`#strings.toLowerCase`(log.userEventType.name()) : ''}" expression), but
components.css lacks rules for UserEventType values (CREATED, UPDATED, DELETED,
AUTHORIZATION_CHANGED); either add corresponding CSS rules (.status-created,
.status-updated, .status-deleted, .status-authorization_changed) to
components.css to style these pills, or change the span to use the existing
styled badge class (replace the base "status" with "event-badge" and keep the
th:classappend or remove it) so user-log events render with the intended styled,
monospace audit chip.
In `@src/main/resources/templates/user/list.html`:
- Line 26: The span rendering the total users ("user-count" with
th:text="${`#lists.size`(users) + ' users total'}") always uses "users" and so
shows "1 users total"; change the expression to perform pluralization—either
replace the current th:text with a conditional/ternary expression that uses
"user" when `#lists.size`(users) == 1 and "users" otherwise, or route through a
message bundle key (e.g. users.total) with the count as a parameter and
pluralization handled in messages; update the span's th:text to use that
conditional or message key accordingly.
In `@src/main/resources/templates/visa/apply-form.html`:
- Line 33: Hidden applicantId input creates a tamper vector because VisaService
currently reads dto.applicantId instead of the trusted principal userId; remove
the hidden input from the template and make the service use the
controller-supplied userId argument (update VisaService.applyForVisa to read the
applicant from the userId parameter, not CreateVisaDTO.applicantId), or
alternatively remove applicantId from CreateVisaDTO and have the controller
(VisaViewController) populate it from principal.getUserId() before calling
VisaService.applyForVisa; ensure only the trusted userId source is used and
remove reliance on form-submitted applicantId.
In
`@src/test/java/org/example/visacasemanagementsystem/audit/service/VisaLogServiceTest.java`:
- Around line 326-331: The mocks for root.get(...) and the chained cb.and(...)
calls rely on a specific evaluation order (root.get called for eventTypePath
then timeStampPath twice, and cb.and(eqPred, gtePred) then cb.and(and1,
ltePred)), so either add a one-line comment above these when(...) stubs
explaining that the specification builder evaluates checks in that sequence
(event-type → from → to) or replace the multi-value thenReturn usage with
Mockito InOrder verification on root.get and cb.and to assert the exact call
order (referencing root.get, cb.equal, cb.greaterThanOrEqualTo,
cb.lessThanOrEqualTo, and cb.and along with eqPred/gtePred/ltePred/and1/and2) so
the test explicitly documents/verifies the order dependency.
---
Nitpick comments:
In
`@src/main/java/org/example/visacasemanagementsystem/ApplicationViewController.java`:
- Around line 27-40: The home method currently iterates
principal.getAuthorities() twice to compute isSysAdmin and isAdmin; collapse
this into a single pass over principal.getAuthorities() (inside home) that sets
two booleans (isSysAdmin, isAdmin) as it sees each authority so you avoid double
iteration while preserving the existing preference logic (ensure SYSADMIN
redirect still takes precedence if both flags end up true). Locate the home
method and update the authority-check logic that produces isSysAdmin and isAdmin
accordingly.
In
`@src/main/java/org/example/visacasemanagementsystem/audit/controller/LogViewController.java`:
- Line 51: The annotation uses defaultValue = "" + DEFAULT_PAGE_SIZE which
relies on compile-time constant folding; replace this with an explicit String
constant to avoid future compile errors: add a String constant (e.g.,
DEFAULT_PAGE_SIZE_STR = "20") alongside the existing static final int
DEFAULT_PAGE_SIZE and update the `@RequestParam` on the size parameter in
LogViewController (the method using the size param) to use
DEFAULT_PAGE_SIZE_STR; ensure both constants are kept in sync or derive the int
from the String if needed.
- Around line 26-28: The class-level `@PreAuthorize`("hasRole('SYSADMIN')") on
LogViewController is intentionally duplicated with
SecurityConfig.requestMatchers("/log/**").hasRole("SYSADMIN") as a defensive
guard; add a short clarifying comment above the LogViewController class (or
adjacent to the `@PreAuthorize` annotation) stating that this redundancy is
deliberate to provide an extra layer of authorization protection in case
SecurityConfig changes, so future maintainers don't remove one of the checks.
In
`@src/main/java/org/example/visacasemanagementsystem/audit/service/UserLogService.java`:
- Around line 45-62: No functional change required—findFiltered in
UserLogService is correct; optionally extract the repeated
predicate-construction used by UserLogService#findFiltered and
VisaLogService#findFiltered into a small shared helper (e.g.,
SpecificationUtils.buildEqualsOrRange) that accepts a metamodel attribute
(UserLog_.userEventType/UserLog_.timeStamp) and a value or range bounds and
returns a Specification to be and()-chained; implement the helper to produce the
same lambdas (equal, greaterThanOrEqualTo, lessThanOrEqualTo) so both services
can call the helper instead of duplicating the spec construction.
In
`@src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java`:
- Around line 62-81: In showCases, avoid calling principal.getUserId() twice:
read meId = principal.getUserId() first and reuse meId in the call to
userService.findById(...) and the subsequent visaService methods (update
references to UserDTO user = userService.findById(meId)... and keep model
population unchanged); also verify UserDTO exposes a consistent fullName
accessor (getter or record accessor) so templates using ${currentUser.fullName}
and ${currentUser.fullName()} both resolve or update templates to one consistent
form.
In
`@src/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.java`:
- Around line 467-502: Extract the per-call lists into class-level constants to
avoid reallocations: define private static final List<VisaStatus> OPEN_STATUSES
= List.of(VisaStatus.ASSIGNED, VisaStatus.INCOMPLETE) and private static final
List<VisaStatus> HANDLED_STATUSES = List.of(VisaStatus.GRANTED,
VisaStatus.REJECTED) in VisaService, then replace the local openStatuses and
handledStatuses variables in findOpenCasesByHandler(Long) and
findHandledCasesByHandler(Long) to use OPEN_STATUSES and HANDLED_STATUSES
respectively (keep the Sort.by("updatedAt").descending() and mapping via
visaMapper::toDTO unchanged).
In `@src/main/resources/static/css/components.css`:
- Around line 129-153: The current global selectors (label and input, select,
textarea) are too broad; restrict them by scoping to a wrapper such as
.form-group (e.g., change label to .form-group label and input, select, textarea
to .form-group input, .form-group select, .form-group textarea), move properties
that break defaults (display:block, width:100%, resize:none, color) into that
scoped rule, and ensure existing exceptions like .file-input-label and
details.html’s textarea override remain or are updated to .form-group-specific
rules so other pages won’t inherit these styles unintentionally.
In `@src/main/resources/templates/error/error.html`:
- Around line 70-94: Add null-safe defaults for the errorTitle and errorMessage
bindings so the page shows sensible text when the upstream handler doesn't
populate them: update the th:text on the element rendering errorTitle (the
element with class "error-code") to use the Elvis/default operator with a
fallback like "⚠️ Error" and update the th:text on the h1 rendering errorMessage
to use an Elvis/default fallback like "Something went wrong"; keep the same
attributes (th:text) and security gating unchanged.
In `@src/main/resources/templates/fragments/cases.html`:
- Line 50: The Thymeleaf expression currently coerces the enum via toString() in
the th:classappend attribute (th:classappend="${'status-' +
`#strings.toLowerCase`(visa.visaStatus)}"), which can break if
VisaStatus.toString() is overridden; change the expression to call the enum's
name() explicitly and pass that into the lowercase helper (use
visa.visaStatus.name() with `#strings.toLowerCase`) so the CSS class generation
uses the stable enum name (referencing the th:classappend attribute and
visa.visaStatus).
In `@src/main/resources/templates/fragments/header.html`:
- Around line 41-43: The logout POST form (form with th:action="@{/user/logout}"
and class "logout-form" containing button "btn-signout") should include an
explicit CSRF hidden input for consistency with other forms; add an input using
th:name="${_csrf.parameterName}" and th:value="${_csrf.token}" inside that form
(or, if you intentionally rely on Thymeleaf/Spring Security auto-injection, add
a short comment in the template explaining that auto-injection is relied upon)
so all POST forms are standardized.
In `@src/main/resources/templates/log/visa.html`:
- Around line 30-37: The select option values currently use th:value="${type}"
which relies on Enum.toString() and can break binding if enums override
toString(); change the value binding to use the enum name
(th:value="${type.name()}") while keeping the display text (th:text) as-is for
human-readable labels, and apply the same update to the analogous select in
log/user.html; target the select with id/name "eventType" and enums like
VisaEventType/UserEventType when making this change.
In `@src/main/resources/templates/visa/edit-form.html`:
- Line 79: This template mixes property-style SpEL with the rest of the PR;
update all record field accesses in edit-form.html to use method-style accessors
to match the other templates (e.g., change occurrences of currentUser.fullName,
updateVisaDto.visaStatus, updateVisaDto.id, visa.id and any downloadUrls/s3Keys
references to currentUser.fullName(), updateVisaDto.visaStatus(),
updateVisaDto.id(), visa.id() and downloadUrls()/s3Keys() respectively) so the
visa templates consistently use the method-style record accessors.
- Around line 137-141: The inline th:onclick that uses confirm(...) and
document.getElementById('delete-doc-' + ${iter.index}).submit() is
CSP-unfriendly and couples the button to a distant hidden form; instead move the
delete <form> (the one currently rendered as the hidden sibling with id
'delete-doc-<index>') into the same document row as the remove <button> and
replace the inline th:onclick wiring with a plain submit button inside that form
(use <button type="submit">) and, if you still want a JS confirmation, attach a
non-inline event listener (or use a data-confirm attribute that a central script
handles) rather than using th:onclick; update references to
delete-doc-${iter.index} and the button markup in the template accordingly.
In `@src/main/resources/templates/visa/my-applications.html`:
- Around line 33-68: The template currently renders the <table> (with headers)
unconditionally and separately renders the empty-state when ${visas} is empty,
causing an empty table to appear; change the structure so the table (the <table>
element that iterates th:each="visa : ${visas}" and the header row) is rendered
only when the list is not empty and the "You have no applications yet." message
is rendered when `#lists.isEmpty`(visas) is true — e.g., wrap the entire table
with th:if="${!#lists.isEmpty(visas)}" (or move the th:if from the empty-state
to the table inversion) and keep the existing empty-state div with
th:if="${`#lists.isEmpty`(visas)}"; ensure the th:each on visa and the view link
href (@{/visa/{id}(id=${visa.id()})}) remain unchanged.
In
`@src/test/java/org/example/visacasemanagementsystem/audit/controller/LogViewControllerTest.java`:
- Around line 130-135: The test stubs visaLogService.findFiltered(...) with
strict eq(expectedFrom)/eq(expectedTo) which can silently stop matching if the
controller alters date conversion; change the stub in LogViewControllerTest to
use more flexible matchers (e.g., any(LocalDateTime.class) or any()) for the
from/to arguments when calling when(visaLogService.findFiltered(...)). Return
the prepared Page (PageImpl) from that stub, and add an
ArgumentCaptor<LocalDateTime> (capturing the from/to arguments passed into
visaLogService.findFiltered) to assert the controller's actual
conversion/rounding behavior explicitly so regressions fail with a clear
assertion instead of an NPE; keep the other matchers (e.g.,
eq(VisaEventType.GRANTED) or any(Pageable.class)) as appropriate.
In
`@src/test/java/org/example/visacasemanagementsystem/audit/service/UserLogServiceTest.java`:
- Around line 340-361: The test is brittle because it stubs root.get(...) and
cb.and(...) with a fixed sequence (eventTypePath, timeStampPath, timeStampPath)
which couples the specCaptor verification to call order; update the test to
either add a clear comment describing that assumption near the specCaptor
verification or refactor the stubbing to match by attribute rather than position
(use argument matchers when stubbing root.get(...) and
cb.equal/greaterThanOrEqualTo/lessThanOrEqualTo so you return
eventTypePath/timeStampPath based on the passed SingularAttribute, or use
argThat on the metamodel attribute), ensuring
specCaptor.getValue().toPredicate(root, query, cb) is driven by attribute-aware
stubs instead of sequence-dependent returns for root.get, and keep references to
root.get, cb.equal, cb.greaterThanOrEqualTo, cb.lessThanOrEqualTo, and
specCaptor in your changes.
In
`@src/test/java/org/example/visacasemanagementsystem/audit/service/VisaLogServiceTest.java`:
- Around line 207-222: Tests in VisaLogServiceTest repeat the same Criteria
mocks (Root, CriteriaQuery, CriteriaBuilder, Path, Predicate) and the
root.get(nullable(SingularAttribute.class)) stub across multiple cases; extract
that repeated setup into a small helper (e.g., a private method like
setupCriteriaMocks or a CriteriaMocks fixture) that creates and stubs Root,
CriteriaQuery, CriteriaBuilder, Path and returns the mocks (or assigns fields)
so each test can call the helper and then only assert the specific behavior of
specCaptor.getValue().toPredicate(root, query, cb) and verifications of cb.equal
/ greaterThanOrEqualTo / lessThanOrEqualTo; update tests at the other locations
(around lines noted) to use the helper to remove the duplicated when(...) and
mock(...) lines while keeping existing assertions and specCaptor usage.
In
`@src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java`:
- Around line 513-525: createMockUser currently both returns a UserDTO and
mutates global SecurityContextHolder via
SecurityContextHolder.getContext().setAuthentication(...), which is misleading
and conflicts with `@WithMockUser`; split it into a pure factory (e.g.,
createMockUserDTO(Long id, UserAuthorization role) that only constructs and
returns UserDTO/User objects) and a side-effecting helper (e.g.,
mockAuthenticatedAs(User user) or mockAuthenticatedAs(UserDTO dto) that creates
a UserPrincipal and TestingAuthenticationToken and calls
SecurityContextHolder.getContext().setAuthentication(...)); update tests that
only need the DTO to call the pure factory and tests that need to set
authentication to call the new mockAuthenticatedAs helper (or remove the manual
SecurityContextHolder writes and rely on `@WithMockUser` / with(user(...))
consistently).
🪄 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: 449a6dbd-34d0-451f-997a-8959120739ea
📒 Files selected for processing (40)
pom.xmlsrc/main/java/org/example/visacasemanagementsystem/ApplicationViewController.javasrc/main/java/org/example/visacasemanagementsystem/audit/controller/LogViewController.javasrc/main/java/org/example/visacasemanagementsystem/audit/repository/UserLogRepository.javasrc/main/java/org/example/visacasemanagementsystem/audit/repository/VisaLogRepository.javasrc/main/java/org/example/visacasemanagementsystem/audit/service/UserLogService.javasrc/main/java/org/example/visacasemanagementsystem/audit/service/VisaLogService.javasrc/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.javasrc/main/java/org/example/visacasemanagementsystem/exception/GlobalExceptionHandler.javasrc/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.javasrc/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.javasrc/main/java/org/example/visacasemanagementsystem/visa/repository/VisaRepository.javasrc/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.javasrc/main/resources/static/css/components.csssrc/main/resources/static/css/tokens.csssrc/main/resources/templates/dashboard/admin.htmlsrc/main/resources/templates/dashboard/applicant.htmlsrc/main/resources/templates/dashboard/sysadmin.htmlsrc/main/resources/templates/error/error.htmlsrc/main/resources/templates/fragments/cases.htmlsrc/main/resources/templates/fragments/header.htmlsrc/main/resources/templates/log/user.htmlsrc/main/resources/templates/log/visa.htmlsrc/main/resources/templates/profile/edit.htmlsrc/main/resources/templates/profile/view.htmlsrc/main/resources/templates/user/list.htmlsrc/main/resources/templates/user/login.htmlsrc/main/resources/templates/user/signup.htmlsrc/main/resources/templates/visa/apply-form.htmlsrc/main/resources/templates/visa/cases.htmlsrc/main/resources/templates/visa/dashboard.htmlsrc/main/resources/templates/visa/details.htmlsrc/main/resources/templates/visa/edit-form.htmlsrc/main/resources/templates/visa/my-applications.htmlsrc/test/java/org/example/visacasemanagementsystem/audit/controller/LogViewControllerTest.javasrc/test/java/org/example/visacasemanagementsystem/audit/service/UserLogServiceTest.javasrc/test/java/org/example/visacasemanagementsystem/audit/service/VisaLogServiceTest.javasrc/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaServiceTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java
💤 Files with no reviewable changes (4)
- src/main/resources/templates/dashboard/sysadmin.html
- src/main/resources/templates/dashboard/applicant.html
- src/main/resources/templates/visa/dashboard.html
- src/main/resources/templates/dashboard/admin.html
Comment on lines
+73
to
+77
| <td> | ||
| <span class="status" | ||
| th:classappend="${log.userEventType != null ? 'status-' + #strings.toLowerCase(log.userEventType.name()) : ''}" | ||
| th:text="${log.userEventType}">CREATED</span> | ||
| </td> |
There was a problem hiding this comment.
Heads-up: status-* classes for UserEventType aren't defined in components.css.
components.css only defines .status-submitted/unassigned/assigned/incomplete/granted/rejected. For UserEventType (CREATED, UPDATED, DELETED, AUTHORIZATION_CHANGED), th:classappend will produce class names like status-created and status-authorization_changed, none of which are styled — so user-log event chips render as the unstyled base .status pill. If that's intentional (treating user-log events as plain badges), no change needed. Otherwise, either add .status-created/updated/deleted/authorization_changed rules to components.css or use the existing .event-badge class instead of .status here for a styled, monospace audit chip.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/templates/log/user.html` around lines 73 - 77, The span
element rendering user log event chips currently uses the base "status" class
and a th:classappend that produces "status-<userEventType>" names (see the span
with class="status" and the th:classappend="${log.userEventType != null ?
'status-' + `#strings.toLowerCase`(log.userEventType.name()) : ''}" expression),
but components.css lacks rules for UserEventType values (CREATED, UPDATED,
DELETED, AUTHORIZATION_CHANGED); either add corresponding CSS rules
(.status-created, .status-updated, .status-deleted,
.status-authorization_changed) to components.css to style these pills, or change
the span to use the existing styled badge class (replace the base "status" with
"event-badge" and keep the th:classappend or remove it) so user-log events
render with the intended styled, monospace audit chip.
|
|
||
| <form th:action="@{/visas/apply}" th:object="${createVisaDTO}" method="post" enctype="multipart/form-data"> | ||
| <form th:action="@{/visa/apply}" th:object="${createVisaDTO}" method="post" enctype="multipart/form-data"> | ||
| <input type="hidden" name="applicantId" th:value="${createVisaDTO.applicantId}" /> |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check whether VisaService.applyForVisa ever reads dto.applicantId() vs. the userId argument.
fd -t f 'VisaService.java' --exec rg -nP -C3 'applyForVisa|applicantId\(\)|\.applicantId\b' {}Repository: ithsjava25/visa-case-management-system
Length of output: 566
Remove the hidden applicantId field — the service reads it instead of the trusted userId parameter.
The submit handler in VisaViewController extracts the applicant from principal.getUserId() and passes it as a separate userId argument to VisaService.applyForVisa(CreateVisaDTO dto, Long userId, String s3Key). However, VisaService ignores that argument and reads dto.applicantId() directly from the DTO (line 87 of VisaService.java), which comes from the hidden input you're posting. This creates a tamper vector where a user can submit an application with any applicantId, effectively applying for visas on behalf of other users. Either:
- Remove the
applicantIdfield fromCreateVisaDTOand have the service use theuserIdparameter, or - If the DTO field must remain, remove the hidden input and ensure the service uses only the
userIdargument.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/templates/visa/apply-form.html` at line 33, Hidden
applicantId input creates a tamper vector because VisaService currently reads
dto.applicantId instead of the trusted principal userId; remove the hidden input
from the template and make the service use the controller-supplied userId
argument (update VisaService.applyForVisa to read the applicant from the userId
parameter, not CreateVisaDTO.applicantId), or alternatively remove applicantId
from CreateVisaDTO and have the controller (VisaViewController) populate it from
principal.getUserId() before calling VisaService.applyForVisa; ensure only the
trusted userId source is used and remove reliance on form-submitted applicantId.
Critical - Remove tamperable applicantId from CreateVisaDTO; visa ownership is derived solely from the authenticated principal. Update the apply form and all CreateVisaDTO call sites accordingly. - Add explicit CSRF token input to the /user/logout form and the apply-visa form to match the codebase's established pattern. - Document the deliberate SYSADMIN-broad-access design on the four case-mutation endpoints in VisaViewController. Accessibility / UX - Bump --color-text-dimmer and --color-placeholder to #767676 for WCAG AA contrast against the dark surfaces. - Raise disabled-input opacity from 0.4 to 0.6 (was below AA for normal text). - Replace nonstandard input[type=file]::before with the standard ::file-selector-button rule (plus a ::-webkit-file-upload-button fallback) so file inputs render correctly in Firefox and Safari. - Add status pill rules for user-event log entries (CREATED, UPDATED, DELETED, AUTHORIZATION_CHANGED). - Pluralize the user-list count. - Null-guard date formatting in fragments/cases.html and switch the status classappend to enum.name() instead of toString. - error.html falls back to "Error" / "Something went wrong" when the model attributes are missing. - log/visa.html and log/user.html: bind <option> values to enum.name() so the SYSADMIN filter stays deterministic. - visa/my-applications.html: only render the <table> when the list is non-empty (was showing an empty header above the empty-state). - visa/edit-form.html: replace inline th:onclick with a data-form-id attribute + delegated .js-delete-doc listener for CSP compatibility. Refactors - VisaService: extract OPEN_STATUSES / HANDLED_STATUSES as class-level constants; eliminate per-request List.of(...) allocations. - LogViewController: extract DEFAULT_PAGE_SIZE_STR so the @RequestParam defaultValue cannot drift from DEFAULT_PAGE_SIZE; add a class-level note explaining the deliberate redundancy between @PreAuthorize and the SecurityConfig URL rule. - ApplicationViewController.home: collapse two stream walks over the authorities collection into a single-pass for-loop. Tests - Update CreateVisaDTO call sites in VisaServiceTest, VisaServiceIntegrationTest, and VisaMapperTest to the 4-arg form. - VisaLogServiceTest and UserLogServiceTest: document the ordering dependency of the multi-value thenReturn stubs on the Specification builder's evaluation sequence (eventType -> from -> to).
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/resources/templates/visa/edit-form.html (1)
128-143:⚠️ Potential issue | 🔴 CriticalFix
downloadUrlspopulation in VisaMapper—currently hardcoded to empty list.The mapper's
toDTO()method (line 32 in VisaMapper.java) hardcodesdownloadUrlstoList.of(), making the "Current Passport Documents" section unreachable in the template. The template's condition at line 128 checksth:if="${not#lists.isEmpty(visa.downloadUrls)}", but this list will always be empty.Additionally, the original concern about parallelism remains valid:
downloadUrlsands3Keysare iterated separately at lines 131 and 164 with matching indices. If these collections ever differ in length or ordering, document deletion will target the wrong S3 key.Recommended fix: Populate
downloadUrlsin the mapper (likely fromvisa.getS3Keys()or a separate method that generates pre-signed URLs), and ensure both lists remain strictly parallel or refactor to iterate a single zipped structure (e.g., a list of{downloadUrl, s3Key}pairs).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/visa/edit-form.html` around lines 128 - 143, VisaMapper.toDTO() currently sets downloadUrls to an empty list which prevents the template check th:if="${not `#lists.isEmpty`(visa.downloadUrls)}" from ever rendering; change VisaMapper.toDTO() to populate downloadUrls from the source S3 keys (e.g., visa.getS3Keys()) or by generating presigned URLs via your existing S3 helper, and return those URLs in the DTO instead of List.of(). Also eliminate fragile parallel iteration by either (a) adding a single documents collection on the DTO (e.g., List<DocumentDTO> with fields downloadUrl and s3Key) and mapping visa.getS3Keys() -> List<DocumentDTO>, or (b) ensure downloadUrls and s3Keys are created in the same order and same length inside VisaMapper.toDTO() so the template's separate iterations remain safe; update the template to iterate the new paired structure if you choose option (a).
🧹 Nitpick comments (7)
src/main/resources/templates/fragments/cases.html (2)
18-21: Pluralization is English-only and inline.The count string is built via inline concatenation (
' case' + (size == 1 ? '' : 's') + countSuffix). It works, but it's not localizable and forces every caller to pass acountSuffixthat already encodes English grammar (e.g." waiting"," handled"). If you anticipate any i18n or want to dedupe this across other count chips in the app, consider moving the phrasing intomessages.propertiesand using#messages.msg('cases.count', size)(with ICU/{0,choice,...}for plural forms). Non-blocking for this PR.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/fragments/cases.html` around lines 18 - 21, The inline English-only pluralization in the span with class "case-count" (which currently builds the string via ${`#lists.size`(cases) + ' case' + (`#lists.size`(cases) == 1 ? '' : 's') + countSuffix}) should be moved into message resources; add a messages.properties key like cases.count that uses ICU/plural formatting (or choice) and accept the count as a parameter, then change the template to call `#messages.msg`('cases.count', `#lists.size`(cases)) and remove the ad-hoc countSuffix usage so localization and pluralization are centralized.
49-51: Consider usingvisaStatus.name()for the displayed text too.Line 50 uses
visa.visaStatus.name()for the CSS class while line 51 uses${visa.visaStatus}for the text, which relies on the enum'stoString(). These will diverge ifVisaStatusever overridestoString()(e.g. to return a localized/display label). Aligning both to.name()keeps the rendered text and thestatus-*class in sync, and matches the PR's stated move toward explicitenum.name()usage in templates.♻️ Suggested alignment
- <span class="status" - th:classappend="${visa.visaStatus != null ? 'status-' + `#strings.toLowerCase`(visa.visaStatus.name()) : ''}" - th:text="${visa.visaStatus}">ASSIGNED</span> + <span class="status" + th:classappend="${visa.visaStatus != null ? 'status-' + `#strings.toLowerCase`(visa.visaStatus.name()) : ''}" + th:text="${visa.visaStatus != null ? visa.visaStatus.name() : '-'}">ASSIGNED</span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/fragments/cases.html` around lines 49 - 51, The displayed status text uses ${visa.visaStatus} while the CSS class uses visa.visaStatus.name(), which can diverge if VisaStatus overrides toString(); update the template fragment so the span's text also uses visa.visaStatus.name() (i.e., change the th:text to use ${visa.visaStatus.name()}) to keep the rendered text and the status-* class in sync with the enum name.src/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.java (2)
487-494: Document SYSADMIN visibility expectation forfindUnassignedCases.This method has no
handlerIdparameter, so any caller that reaches it can see every unassignedSUBMITTEDcase in the system. Authorization is enforced at the controller (VisaViewController#showCasesis the only caller per the provided snippet), but the service exposes a broad listing API. Worth a short Javadoc note that the method is intended for ADMIN/SYSADMIN view contexts only, mirroring the PR's "deliberate SYSADMIN access" documentation effort. No behavior change required.🤖 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/service/VisaService.java` around lines 487 - 494, The public service method VisaService.findUnassignedCases() returns every unassigned SUBMITTED case and therefore exposes a broad listing that is intended only for SYSADMIN/ADMIN view contexts; add a concise Javadoc above the findUnassignedCases method documenting that this method is deliberately for ADMIN/SYSADMIN visibility only (mirror the PR's "deliberate SYSADMIN access" wording), note that authorization is enforced at the controller (VisaViewController#showCases) and callers should not rely on this service for per-handler-restricted listings—no behavior change required.
469-507: Optional: hoist the repeatedSortto a constant.
Sort.by("updatedAt").descending()is allocated on every call acrossfindOpenCasesByHandler,findUnassignedCases,findHandledCasesByHandler(and several pre-existing methods likefindAll,findVisasByApplicant,findVisasByApplicantId,findVisasByHandlerId). SinceSortis immutable, you can hoist it next toOPEN_STATUSES/HANDLED_STATUSESand reuse it. Purely a nit; behavior is unchanged.♻️ Suggested constant
private static final List<VisaStatus> HANDLED_STATUSES = List.of(VisaStatus.GRANTED, VisaStatus.REJECTED); + private static final Sort SORT_BY_UPDATED_AT_DESC = + Sort.by("updatedAt").descending();Then reuse
SORT_BY_UPDATED_AT_DESCin the three new case-query methods (and optionally in the existingfindAll/findVisasByApplicant/findVisasByHandlerId).🤖 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/service/VisaService.java` around lines 469 - 507, Multiple methods (findOpenCasesByHandler, findUnassignedCases, findHandledCasesByHandler) repeatedly allocate the same Sort instance via Sort.by("updatedAt").descending(); hoist this to a private static final constant (e.g., SORT_BY_UPDATED_AT_DESC) near OPEN_STATUSES/HANDLED_STATUSES and replace the inline Sort.by(...) calls in those methods (and optionally other methods like findAll, findVisasByApplicant, findVisasByApplicantId, findVisasByHandlerId) to reuse the constant.src/main/resources/templates/visa/my-applications.html (1)
49-56: Consider explicit.name()and null guards on the#temporals.formatcalls.Two minor consistency points:
- The PR description calls out a deliberate move toward
enum.name()usage in templates for null-safety/consistency. Lines 50–51 still passvisa.visaStatus()directly to#strings.toLowerCaseandth:text, relying onEnum#toString. Functionally equivalent for unmodified enums, but using.name()would match the rest of the PR.#temporals.format(visa.travelDate(), …)and#temporals.format(visa.updatedAt(), …)will throw if either field is null.travelDateis@NotNullon create so it should always be set, butupdatedAtdepends on whether the row has been persisted/audited. If there's any path where these can be null in the model rendered to this template, ath:ifguard or?: '-'Elvis fallback would be safer.♻️ Suggested tweaks
- <td th:text="${visa.visaType()}">STUDENT</td> + <td th:text="${visa.visaType().name()}">STUDENT</td> <td> <span class="status" - th:classappend="${'status-' + `#strings.toLowerCase`(visa.visaStatus())}" - th:text="${visa.visaStatus()}"> + th:classappend="${'status-' + `#strings.toLowerCase`(visa.visaStatus().name())}" + th:text="${visa.visaStatus().name()}"> SUBMITTED </span> </td> - <td th:text="${`#temporals.format`(visa.travelDate(), 'yyyy-MM-dd')}">2026-05-20</td> - <td th:text="${`#temporals.format`(visa.updatedAt(), 'yyyy-MM-dd HH:mm')}">-</td> + <td th:text="${visa.travelDate() != null ? `#temporals.format`(visa.travelDate(), 'yyyy-MM-dd') : '-'}">2026-05-20</td> + <td th:text="${visa.updatedAt() != null ? `#temporals.format`(visa.updatedAt(), 'yyyy-MM-dd HH:mm') : '-'}">-</td>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/visa/my-applications.html` around lines 49 - 56, Use enum.name() instead of relying on toString for the status span by calling visa.visaStatus().name() when passing into `#strings.toLowerCase` and th:text (replace uses of visa.visaStatus()); and guard temporal formatting calls by handling nulls—either wrap `#temporals.format`(visa.travelDate(), ...) / `#temporals.format`(visa.updatedAt(), ...) with a th:if to only format when travelDate()/updatedAt() are non-null or use an Elvis fallback (e.g., show '-' when null) so `#temporals.format` is never invoked with a null argument.src/main/resources/templates/log/visa.html (1)
58-87: Hide the table when there are no rows.When
logs.totalElements == 0, the<table>still renders its header row and an empty<tbody>immediately above the empty-state message, which produces a visually empty table next to "No log entries match the current filters." Wrapping the table in ath:if(or moving it insideth:if="${logs.totalElements > 0}") avoids the redundant header bar.♻️ Proposed tweak
- <table> + <table th:if="${logs.totalElements > 0}"> <thead>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/log/visa.html` around lines 58 - 87, The table header is still rendered when there are zero logs; wrap the entire <table> block (the table, thead and tbody currently iterating with th:each on "log : ${logs.content}") with a Thymeleaf conditional so it only renders when logs.totalElements > 0 (e.g. add th:if="${logs.totalElements > 0}" on the table element or move the table inside th:if="${logs.totalElements > 0}"), leaving the existing <div th:if="${logs.totalElements == 0}" class="empty-state"> message as-is.src/main/resources/templates/visa/edit-form.html (1)
174-193: Optional: scope the file-input listener and avoid double-binding.Two minor robustness items in the script:
document.getElementById('passportFile').addEventListener(...)will throw aTypeErrorif the element is ever conditionally hidden (e.g., a future template variant). Guarding with?.or a null check makes this resilient.- If this template is ever included in a larger page or re-rendered, the delegated click handlers attached at module scope can double-bind. Wrapping in
DOMContentLoadedor{ once: false }semantics is fine today, but consider event delegation ondocumentfor better resilience.Non-blocking; current behavior is correct for the single-page use case.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/visa/edit-form.html` around lines 174 - 193, Guard the passportFile listener and avoid double-binding the delete handlers: check for the file input element (document.getElementById('passportFile')) before calling addEventListener (or register the handler inside a DOMContentLoaded callback) so it won't throw when the element is absent, and replace the per-button querySelectorAll('.js-delete-doc') loop with a single delegated click listener on document that uses event.target.closest('.js-delete-doc') to find the button, reads btn.dataset.formId and submits the target form after confirm; reference the IDs/classes 'passportFile', 'file-name' and the class 'js-delete-doc' when making these changes.
🤖 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/templates/fragments/cases.html`:
- Line 46: The template fragment duplicates the "Unassigned" fallback that
VisaMapper.toDTO already provides; update the template to remove the defensive
ternary and simply render visa.handlerName (e.g., change the td to use
th:text="${visa.handlerName}") or, if you prefer explicitness, reference the
same centralized constant/translation key used by VisaMapper.toDTO so both the
mapper (VisaMapper.toDTO) and the template (fragments/cases.html rendering
visa.handlerName) stay in sync.
- Line 39: The empty action column header (<th></th>) is missing an accessible
name; update the <th> in the cases.html template to include an explicit column
label (e.g., "Actions" or "Details") and mark it visually hidden using your
project's screen-reader utility (or add scope="col" plus the visible/hidden
label). Locate the empty <th> and replace it with a labelled header that uses
your SR-only class or scope attribute so screen readers announce the column for
the row action links.
In `@src/main/resources/templates/visa/edit-form.html`:
- Around line 164-170: The hidden delete forms (th:id="'delete-doc-' +
${iter.index}" with th:action="@{/visa/{id}/documents/delete(id=${visa.id})}")
are missing the CSRF token and will be rejected by Spring Security; add a hidden
input inside each form using Thymeleaf CSRF variables (use _csrf.parameterName
and _csrf.token) so each generated form includes the CSRF parameter and token
before submission.
---
Outside diff comments:
In `@src/main/resources/templates/visa/edit-form.html`:
- Around line 128-143: VisaMapper.toDTO() currently sets downloadUrls to an
empty list which prevents the template check th:if="${not
`#lists.isEmpty`(visa.downloadUrls)}" from ever rendering; change
VisaMapper.toDTO() to populate downloadUrls from the source S3 keys (e.g.,
visa.getS3Keys()) or by generating presigned URLs via your existing S3 helper,
and return those URLs in the DTO instead of List.of(). Also eliminate fragile
parallel iteration by either (a) adding a single documents collection on the DTO
(e.g., List<DocumentDTO> with fields downloadUrl and s3Key) and mapping
visa.getS3Keys() -> List<DocumentDTO>, or (b) ensure downloadUrls and s3Keys are
created in the same order and same length inside VisaMapper.toDTO() so the
template's separate iterations remain safe; update the template to iterate the
new paired structure if you choose option (a).
---
Nitpick comments:
In
`@src/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.java`:
- Around line 487-494: The public service method
VisaService.findUnassignedCases() returns every unassigned SUBMITTED case and
therefore exposes a broad listing that is intended only for SYSADMIN/ADMIN view
contexts; add a concise Javadoc above the findUnassignedCases method documenting
that this method is deliberately for ADMIN/SYSADMIN visibility only (mirror the
PR's "deliberate SYSADMIN access" wording), note that authorization is enforced
at the controller (VisaViewController#showCases) and callers should not rely on
this service for per-handler-restricted listings—no behavior change required.
- Around line 469-507: Multiple methods (findOpenCasesByHandler,
findUnassignedCases, findHandledCasesByHandler) repeatedly allocate the same
Sort instance via Sort.by("updatedAt").descending(); hoist this to a private
static final constant (e.g., SORT_BY_UPDATED_AT_DESC) near
OPEN_STATUSES/HANDLED_STATUSES and replace the inline Sort.by(...) calls in
those methods (and optionally other methods like findAll, findVisasByApplicant,
findVisasByApplicantId, findVisasByHandlerId) to reuse the constant.
In `@src/main/resources/templates/fragments/cases.html`:
- Around line 18-21: The inline English-only pluralization in the span with
class "case-count" (which currently builds the string via ${`#lists.size`(cases) +
' case' + (`#lists.size`(cases) == 1 ? '' : 's') + countSuffix}) should be moved
into message resources; add a messages.properties key like cases.count that uses
ICU/plural formatting (or choice) and accept the count as a parameter, then
change the template to call `#messages.msg`('cases.count', `#lists.size`(cases)) and
remove the ad-hoc countSuffix usage so localization and pluralization are
centralized.
- Around line 49-51: The displayed status text uses ${visa.visaStatus} while the
CSS class uses visa.visaStatus.name(), which can diverge if VisaStatus overrides
toString(); update the template fragment so the span's text also uses
visa.visaStatus.name() (i.e., change the th:text to use
${visa.visaStatus.name()}) to keep the rendered text and the status-* class in
sync with the enum name.
In `@src/main/resources/templates/log/visa.html`:
- Around line 58-87: The table header is still rendered when there are zero
logs; wrap the entire <table> block (the table, thead and tbody currently
iterating with th:each on "log : ${logs.content}") with a Thymeleaf conditional
so it only renders when logs.totalElements > 0 (e.g. add
th:if="${logs.totalElements > 0}" on the table element or move the table inside
th:if="${logs.totalElements > 0}"), leaving the existing <div
th:if="${logs.totalElements == 0}" class="empty-state"> message as-is.
In `@src/main/resources/templates/visa/edit-form.html`:
- Around line 174-193: Guard the passportFile listener and avoid double-binding
the delete handlers: check for the file input element
(document.getElementById('passportFile')) before calling addEventListener (or
register the handler inside a DOMContentLoaded callback) so it won't throw when
the element is absent, and replace the per-button
querySelectorAll('.js-delete-doc') loop with a single delegated click listener
on document that uses event.target.closest('.js-delete-doc') to find the button,
reads btn.dataset.formId and submits the target form after confirm; reference
the IDs/classes 'passportFile', 'file-name' and the class 'js-delete-doc' when
making these changes.
In `@src/main/resources/templates/visa/my-applications.html`:
- Around line 49-56: Use enum.name() instead of relying on toString for the
status span by calling visa.visaStatus().name() when passing into
`#strings.toLowerCase` and th:text (replace uses of visa.visaStatus()); and guard
temporal formatting calls by handling nulls—either wrap
`#temporals.format`(visa.travelDate(), ...) / `#temporals.format`(visa.updatedAt(),
...) with a th:if to only format when travelDate()/updatedAt() are non-null or
use an Elvis fallback (e.g., show '-' when null) so `#temporals.format` is never
invoked with a null argument.
🪄 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: 06a8e76c-2290-470a-b8d1-b270a479f882
📒 Files selected for processing (20)
src/main/java/org/example/visacasemanagementsystem/ApplicationViewController.javasrc/main/java/org/example/visacasemanagementsystem/audit/controller/LogViewController.javasrc/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.javasrc/main/java/org/example/visacasemanagementsystem/visa/dto/CreateVisaDTO.javasrc/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.javasrc/main/resources/static/css/components.csssrc/main/resources/static/css/tokens.csssrc/main/resources/templates/error/error.htmlsrc/main/resources/templates/fragments/cases.htmlsrc/main/resources/templates/fragments/header.htmlsrc/main/resources/templates/log/user.htmlsrc/main/resources/templates/log/visa.htmlsrc/main/resources/templates/user/list.htmlsrc/main/resources/templates/visa/apply-form.htmlsrc/main/resources/templates/visa/edit-form.htmlsrc/main/resources/templates/visa/my-applications.htmlsrc/test/java/org/example/visacasemanagementsystem/audit/service/VisaLogServiceTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaMapperTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaServiceIntegrationTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaServiceTest.java
✅ Files skipped from review due to trivial changes (2)
- src/main/resources/static/css/components.css
- src/main/resources/static/css/tokens.css
🚧 Files skipped from review as they are similar to previous changes (2)
- src/main/resources/templates/log/user.html
- src/main/resources/templates/visa/apply-form.html
…i-design # Conflicts: # src/main/java/org/example/visacasemanagementsystem/audit/service/UserLogService.java # src/main/java/org/example/visacasemanagementsystem/audit/service/VisaLogService.java # src/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.java # src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java # src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java # src/main/resources/templates/user/login.html # src/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.java # src/test/java/org/example/visacasemanagementsystem/visa/VisaServiceTest.java # src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.java (1)
19-19:⚠️ Potential issue | 🟡 MinorUnused
withDefaultsstatic import — Spotless failure.After replacing
logout(withDefaults())withlogout(AbstractHttpConfigurer::disable), the static import on line 19 has no remaining call sites and is whatspotless:checkis reporting on line 16. Drop it.🧹 Proposed fix
-import static org.springframework.security.config.Customizer.withDefaults; - `@Configuration`🤖 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/SecurityConfig.java` at line 19, Remove the now-unused static import withDefaults from SecurityConfig.java — locate the import statement "import static org.springframework.security.config.Customizer.withDefaults;" and delete it since you already replaced logout(withDefaults()) with logout(AbstractHttpConfigurer::disable) and there are no remaining references to withDefaults.
🧹 Nitpick comments (2)
src/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.java (1)
350-359: Redundant@WithMockUsernext to.with(authentication(...)).
SecurityMockMvcRequestPostProcessors.authentication(...)already overrides theSecurityContextfor this request, so the class-level@WithMockUsersetup is shadowed. Either drop@WithMockUseror remove the explicitauthentication(...)to avoid future confusion about which principal is actually under test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.java` around lines 350 - 359, The test userListView_AsNonSysAdmin_ShouldReturnForbidden has a redundant `@WithMockUser` which is shadowed by the explicit .with(authentication(authFor(...))) request post-processor; remove one of them to avoid ambiguity — either delete the `@WithMockUser` annotation or stop using .with(authentication(...)) in the mockMvc.perform call so the test's principal is set from a single source (refer to the authFor(...) helper and the mockMvc.perform(get("/user/list")...) invocation when making the change).src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java (1)
155-167: StalecurrentUserIdrequest parameter — controller no longer reads it.
submitApplication,processUpdate, etc. now resolve the user from@AuthenticationPrincipal UserPrincipal principal; theparam("currentUserId", ...)calls on lines 159, 182, 252, 279, 310, and 343 are leftovers from the pre-refactor signature. They're silently ignored, but they obscure intent and will mislead the next person reading the test. Consider removing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java` around lines 155 - 167, Tests are passing a stale request parameter "currentUserId" which the controller no longer reads (submitApplication, processUpdate, etc. now use `@AuthenticationPrincipal` UserPrincipal); remove all .param("currentUserId", ...) calls from VisaViewControllerTest and instead ensure the MockMvc request supplies an authenticated principal (e.g., use MockMvc security helpers like with(user(...)) or with(authentication(...)) or the test helper that builds a UserPrincipal) so the controller receives the expected principal; update any related test helper setup that previously relied on the param to populate authentication.
🤖 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/audit/service/UserLogService.java`:
- Around line 47-50: Method-level authorization is missing on
UserLogService.findFiltered(...) — add a `@PreAuthorize` annotation on the
findFiltered method (in the UserLogService class) to mirror the existing
protection on findAll() (e.g., `@PreAuthorize`("isAuthenticated()") or stricter
`@PreAuthorize`("hasRole('SYSADMIN')") to match the URL rule) so service-level
callers cannot bypass access checks; place the annotation directly above the
findFiltered(...) declaration and ensure the class imports the correct Spring
Security annotation.
In
`@src/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.java`:
- Around line 53-54: The catch-all rule in SecurityConfig that uses
.anyRequest().hasRole("SYSADMIN") is too broad and blocks special paths; update
the HttpSecurity config (the chain where .requestMatchers(...) and
.anyRequest().hasRole(...) are defined) to explicitly permit /favicon.ico,
/login/oauth2/** and /error by inserting
.requestMatchers("/favicon.ico").permitAll(),
.requestMatchers("/login/oauth2/**").permitAll(), and
.requestMatchers("/error").permitAll() before the .anyRequest() rule (or
alternatively change .anyRequest().hasRole("SYSADMIN") to
.anyRequest().denyAll() for explicit deny semantics). Ensure these
requestMatchers are added in the same configuration method in SecurityConfig so
they execute prior to the catch‑all.
- Line 67: The logout path currently only clears the SecurityContext (via
SecurityContextLogoutHandler) but does not invalidate the HTTP session, leaving
session cookies valid; update the custom logout handler (the method that calls
new SecurityContextLogoutHandler().logout(...), e.g. your controller logout
method and any logout configuration referenced in SecurityConfig where
.logout(...) is configured) to call request.getSession(false) and if non-null
call invalidate() after invoking SecurityContextLogoutHandler.logout; ensure the
invalidation happens before returning the redirect so the server-side session is
destroyed and the session cookie cannot be reused.
In
`@src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java`:
- Line 17: The import org.springframework.security.access.prepost.PreAuthorize
in VisaViewController is unused and causing Spotless failure; fix by either
removing that unused import line from the VisaViewController class or re-enable
the commented `@PreAuthorize` annotations on the relevant controller methods so
the import is used (ensure any re-enabled `@PreAuthorize` uses the correct SpEL
and required roles/permissions).
- Around line 47-80: The controller methods showMyApplications, showApplyForm,
submitApplication and showCases currently have their `@PreAuthorize` annotations
commented out, allowing any authenticated user to access endpoints that should
be role-restricted; restore method-level security by uncommenting and applying
the intended checks: use `@PreAuthorize`("hasRole('USER')") on showApplyForm and
submitApplication (and showMyApplications if meant for USERs) and
`@PreAuthorize`("hasAnyRole('ADMIN','SYSADMIN')") on showCases so only
admins/sysadmins can see unassigned cases, then rebuild and run tests to verify
access control; locate these annotations adjacent to the method declarations in
VisaViewController to make the change.
In
`@src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java`:
- Around line 73-97: The test method
showCases_AsAdmin_ShouldReturnThreeListsAndCasesView currently uses
`@WithMockUser` which defaults to ROLE_USER and therefore doesn't match the
intended admin principal; update the test annotation to `@WithMockUser`(roles =
"ADMIN") (and similarly adjust other tests that expect admin/SYSADMIN) so
MockMvc's security context matches the createMockUser(UserAuthorization.ADMIN)
setup and the controller method showCases (protected by `@PreAuthorize`) will be
authorized during the test.
---
Outside diff comments:
In
`@src/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.java`:
- Line 19: Remove the now-unused static import withDefaults from
SecurityConfig.java — locate the import statement "import static
org.springframework.security.config.Customizer.withDefaults;" and delete it
since you already replaced logout(withDefaults()) with
logout(AbstractHttpConfigurer::disable) and there are no remaining references to
withDefaults.
---
Nitpick comments:
In
`@src/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.java`:
- Around line 350-359: The test userListView_AsNonSysAdmin_ShouldReturnForbidden
has a redundant `@WithMockUser` which is shadowed by the explicit
.with(authentication(authFor(...))) request post-processor; remove one of them
to avoid ambiguity — either delete the `@WithMockUser` annotation or stop using
.with(authentication(...)) in the mockMvc.perform call so the test's principal
is set from a single source (refer to the authFor(...) helper and the
mockMvc.perform(get("/user/list")...) invocation when making the change).
In
`@src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java`:
- Around line 155-167: Tests are passing a stale request parameter
"currentUserId" which the controller no longer reads (submitApplication,
processUpdate, etc. now use `@AuthenticationPrincipal` UserPrincipal); remove all
.param("currentUserId", ...) calls from VisaViewControllerTest and instead
ensure the MockMvc request supplies an authenticated principal (e.g., use
MockMvc security helpers like with(user(...)) or with(authentication(...)) or
the test helper that builds a UserPrincipal) so the controller receives the
expected principal; update any related test helper setup that previously relied
on the param to populate authentication.
🪄 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: 5d59ee1a-d1ff-4d0d-afba-229479827011
📒 Files selected for processing (13)
src/main/java/org/example/visacasemanagementsystem/audit/service/UserLogService.javasrc/main/java/org/example/visacasemanagementsystem/audit/service/VisaLogService.javasrc/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.javasrc/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.javasrc/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.javasrc/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.javasrc/main/resources/templates/fragments/cases.htmlsrc/main/resources/templates/user/login.htmlsrc/main/resources/templates/visa/edit-form.htmlsrc/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaServiceIntegrationTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaServiceTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java
✅ Files skipped from review due to trivial changes (3)
- src/main/java/org/example/visacasemanagementsystem/audit/service/VisaLogService.java
- src/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.java
- src/main/resources/templates/visa/edit-form.html
🚧 Files skipped from review as they are similar to previous changes (4)
- src/main/resources/templates/user/login.html
- src/test/java/org/example/visacasemanagementsystem/visa/VisaServiceIntegrationTest.java
- src/main/resources/templates/fragments/cases.html
- src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java
Comment on lines
+47
to
+80
| // @PreAuthorize("hasRole('USER')") | ||
| @GetMapping("/my-applications") | ||
| public String showMyApplications(@AuthenticationPrincipal UserPrincipal principal, Model model) { | ||
| UserDTO user = userService.findById(principal.getUserId()) | ||
| .orElseThrow(() -> new EntityNotFoundException("User not found.")); | ||
| List<VisaDTO> visas; | ||
| // If the current user = ADMIN/SYSADMIN show everything | ||
| if ( user.userAuthorization() == UserAuthorization.ADMIN || | ||
| user.userAuthorization() == UserAuthorization.SYSADMIN) { | ||
| visas = visaService.findAll(); | ||
| } else { | ||
| // Normal user/applicant can only se their own visa applications | ||
| visas = visaService.findVisasByApplicant(principal.getUserId()); | ||
| } | ||
|
|
||
| // Send data to Thymeleaf | ||
| List<VisaDTO> visas = visaService.findVisasByApplicant(principal.getUserId()); | ||
|
|
||
| model.addAttribute("visas", visas); | ||
| model.addAttribute("currentUser", user); | ||
|
|
||
| return "visa/dashboard"; | ||
| return "visa/my-applications"; | ||
| } | ||
|
|
||
| // ─── ADMIN + SYSADMIN: "Visa Cases" three-list page ──────────────────── | ||
| // @PreAuthorize("hasAnyRole('ADMIN', 'SYSADMIN')") | ||
| @GetMapping("/cases") | ||
| public String showCases(@AuthenticationPrincipal UserPrincipal principal, Model model) { | ||
| UserDTO user = userService.findById(principal.getUserId()) | ||
| .orElseThrow(() -> new EntityNotFoundException("User not found.")); | ||
|
|
||
| Long meId = principal.getUserId(); | ||
|
|
||
| List<VisaDTO> openCases = visaService.findOpenCasesByHandler(meId); | ||
| List<VisaDTO> unassignedCases = visaService.findUnassignedCases(); | ||
| List<VisaDTO> handledCases = visaService.findHandledCasesByHandler(meId); | ||
|
|
||
| model.addAttribute("currentUser", user); | ||
| model.addAttribute("openCases", openCases); | ||
| model.addAttribute("unassignedCases", unassignedCases); | ||
| model.addAttribute("handledCases", handledCases); | ||
|
|
||
| return "visa/cases"; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -nP --type=java -C2 '/visa/cases|hasAnyRole\(.*ADMIN.*SYSADMIN'
rg -nP --type=java -C2 '@PreAuthorize' src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.javaRepository: ithsjava25/visa-case-management-system
Length of output: 8853
🏁 Script executed:
#!/bin/bash
# Check SecurityConfig for URL-level role protection
fd -t f SecurityConfig.java | head -1 | xargs cat -n
# Check PreAuthorize import in VisaViewController
rg -n 'import.*PreAuthorize|import.*security.access' src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.javaRepository: ithsjava25/visa-case-management-system
Length of output: 4994
🏁 Script executed:
#!/bin/bash
# Get the import section of VisaViewController
head -n 30 src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.javaRepository: ithsjava25/visa-case-management-system
Length of output: 1466
🏁 Script executed:
#!/bin/bash
# Check if PreAuthorize import exists anywhere in the file
rg 'PreAuthorize' src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java | head -20Repository: ithsjava25/visa-case-management-system
Length of output: 215
🏁 Script executed:
#!/bin/bash
# Check if there's any mention of PreAuthorize import earlier in file or in comments
rg -n 'import.*PreAuthorize|PreAuthorize' src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java | head -5
# Double-check the entire import block for any security imports
head -n 25 src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java | rg 'import'Repository: ithsjava25/visa-case-management-system
Length of output: 1470
Authorization bypass: /visa/cases, /visa/apply, and /visa/my-applications lack role enforcement.
SecurityConfig protects /visa/** with only .authenticated() (line 47), with no URL-level role gating. The method-level @PreAuthorize annotations on showCases, showApplyForm, submitApplication, and showMyApplications are commented out (lines 46, 61, 82) and provide no protection.
Result: any authenticated USER can:
GET /visa/casesand accessfindUnassignedCases(), leaking the global unassigned-case list to regular applicants.GET /visa/applyand submit applications without role restriction (contradicts the "USER only" intent).GET /visa/my-applicationsis accessible toADMIN/SYSADMIN(less severe but violates documented role separation).
Uncomment the @PreAuthorize annotations to restore role-based method-level protection:
Re-enable method-level authorization
- // ─── USER: "My Applications" landing page ─────────────────────────────
-// `@PreAuthorize`("hasRole('USER')")
+ // ─── USER: "My Applications" landing page ─────────────────────────────
+ `@PreAuthorize`("hasRole('USER')")
`@GetMapping`("/my-applications")
@@
- // ─── ADMIN + SYSADMIN: "Visa Cases" three-list page ────────────────────
-// `@PreAuthorize`("hasAnyRole('ADMIN', 'SYSADMIN')")
+ // ─── ADMIN + SYSADMIN: "Visa Cases" three-list page ────────────────────
+ `@PreAuthorize`("hasAnyRole('ADMIN', 'SYSADMIN')")
`@GetMapping`("/cases")
@@
- // ─── Apply / edit / details — unchanged semantics, renamed URLs ────────
-// `@PreAuthorize`("hasRole('USER')")
+ // ─── Apply / edit / details — unchanged semantics, renamed URLs ────────
+ `@PreAuthorize`("hasRole('USER')")
`@GetMapping`("/apply")🤖 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 47 - 80, The controller methods showMyApplications, showApplyForm,
submitApplication and showCases currently have their `@PreAuthorize` annotations
commented out, allowing any authenticated user to access endpoints that should
be role-restricted; restore method-level security by uncommenting and applying
the intended checks: use `@PreAuthorize`("hasRole('USER')") on showApplyForm and
submitApplication (and showMyApplications if meant for USERs) and
`@PreAuthorize`("hasAnyRole('ADMIN','SYSADMIN')") on showCases so only
admins/sysadmins can see unassigned cases, then rebuild and run tests to verify
access control; locate these annotations adjacent to the method declarations in
VisaViewController to make the change.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java (1)
58-73:⚠️ Potential issue | 🟡 Minor
POST /user/signupis reachable by already-authenticated users.The GET handler now redirects authenticated users to
/home(line 49-51), but the POST counterpart will happily create anotherUSERaccount for any logged-in caller (and even one withADMIN/SYSADMINrole, sinceSecurityConfigkeeps/user/signuppermitAll). Consider mirroring the GET-side guard so authenticated principals are bounced beforeuserService.createUserruns:🛡️ Suggested guard
`@PostMapping`("/user/signup") -public String createUser(`@RequestParam` String fullName, +public String createUser(`@AuthenticationPrincipal` UserPrincipal principal, + `@RequestParam` String fullName, `@RequestParam` String email, `@RequestParam` String password, Model model) { + if (principal != null) { + return "redirect:/home"; + } try {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java` around lines 58 - 73, The POST createUser handler allows authenticated principals to create new accounts; add the same guard as the GET signup handler at the start of the createUser method so authenticated users are redirected to "/home" before calling userService.createUser. Implement this by checking the current Authentication (e.g., SecurityContextHolder.getContext().getAuthentication() or an injected Principal/@AuthenticationPrincipal), verify authentication.isAuthenticated() and that the principal is not anonymous, and return "redirect:/home" if true; otherwise proceed to construct CreateUserDTO and call userService.createUser as before.src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java (1)
358-437:⚠️ Potential issue | 🟡 MinorAdmin-only handler tests still default to
ROLE_USERvia plain@WithMockUser.
showCases_AsAdminwas correctly updated to@WithMockUser(roles = "ADMIN"), butapproveVisa_AsAdmin(Line 359),assignCaseToHandler_Success(Line 380),requestMoreInformation_Success(Line 397),rejectVisa_AsAdmin(Line 418), andviewDetails_AsAdmin_ShouldAllowViewingOtherVisas(Line 491) are still using plain@WithMockUser, which produces aROLE_USERprincipal. They currently pass only because@WebMvcTestdoesn't loadSecurityConfig's URL-level rules. Once method-level@PreAuthorizeis added (see the critical SecurityConfig comment), these tests will start returning 403 and silently fail to validate the admin path.🧪 Suggested annotation alignment
- `@WithMockUser` + `@WithMockUser`(roles = "ADMIN") void approveVisa_AsAdmin_ShouldRedirectToDetailsView() throws Exception { … - `@WithMockUser` + `@WithMockUser`(roles = "ADMIN") void assignCaseToHandler_Success_ShouldRedirectToDetails() throws Exception { … - `@WithMockUser` + `@WithMockUser`(roles = "ADMIN") void requestMoreInformation_Success_ShouldRedirectToDetails() throws Exception { … - `@WithMockUser` + `@WithMockUser`(roles = "ADMIN") void rejectVisa_AsAdmin_ShouldRedirectToDetailsView() throws Exception { … - `@WithMockUser` + `@WithMockUser`(roles = "ADMIN") void viewDetails_AsAdmin_ShouldAllowViewingOtherVisas() throws Exception {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java` around lines 358 - 437, The admin-only tests (approveVisa_AsAdmin_ShouldRedirectToDetailsView, assignCaseToHandler_Success_ShouldRedirectToDetails, requestMoreInformation_Success_ShouldRedirectToDetails, rejectVisa_AsAdmin_ShouldRedirectToDetailsView and the later viewDetails_AsAdmin_ShouldAllowViewingOtherVisas) are using plain `@WithMockUser` which creates ROLE_USER; update each of these test methods to use `@WithMockUser`(roles = "ADMIN") so the mock principal has ADMIN authority and the controller’s admin-only checks (e.g., `@PreAuthorize`) will pass; locate the annotations above the methods named approveVisa_AsAdmin_ShouldRedirectToDetailsView, assignCaseToHandler_Success_ShouldRedirectToDetails, requestMoreInformation_Success_ShouldRedirectToDetails, rejectVisa_AsAdmin_ShouldRedirectToDetailsView and viewDetails_AsAdmin_ShouldAllowViewingOtherVisas and change the annotation accordingly.src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java (1)
134-258:⚠️ Potential issue | 🔴 CriticalAll
/visa/{id}*endpoints are SYSADMIN-only — USERs and ADMINs cannot access them.SecurityConfig contains no explicit matchers for
/visa/{id},/visa/{id}/edit,/visa/{id}/approve,/visa/{id}/reject,/visa/{id}/request-info,/visa/{id}/assign,/visa/{id}/documents/delete, or the parameterized routes. These paths fall through to line 58:.anyRequest().hasRole("SYSADMIN"). Meanwhile, VisaViewController has no method-level@PreAuthorizeannotations to override this catch-all.Consequence at runtime:
- A USER cannot
GET /visa/{id}(view own application) — SecurityFilterChain returns 403 before the controller is invoked.- An ADMIN cannot
POST /visa/{id}/approve,/reject,/request-info, or/assign— same 403.- Even though
VisaServicehas@PreAuthorize("hasRole('ADMIN')")onapproveVisa()etc., those service-level guards are never reached because URL-level authorization is enforced first.Fix: Either add a
/visa/**matcher allowing all authenticated users (lines 42–49), or annotate each controller method with@PreAuthorizereflecting the intended role (@PreAuthorize("hasAnyRole('USER', 'ADMIN', 'SYSADMIN')")for view/edit,@PreAuthorize("hasAnyRole('ADMIN', 'SYSADMIN')")for case actions). The comment in lines 42–43 ("role-specific protection lives on individual@PreAuthorize") does not apply here since those annotations are absent.🤖 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 134 - 258, The controller endpoints under VisaViewController (e.g., showEditForm, processUpdate, deleteVisaDocument, approveVisa, requestMoreInformation, rejectVisa, assignCaseToHandler) are blocked by the global .anyRequest().hasRole("SYSADMIN") rule; add explicit method-level security annotations to restore intended access: annotate view/edit endpoints (showEditForm, processUpdate, and the GET /visa/{id} handler) with `@PreAuthorize`("hasAnyRole('USER','ADMIN','SYSADMIN')") and annotate case-action endpoints (approveVisa, rejectVisa, requestMoreInformation, assignCaseToHandler, deleteVisaDocument if appropriate) with `@PreAuthorize`("hasAnyRole('ADMIN','SYSADMIN')"); alternatively, adjust SecurityConfig to include a /visa/** matcher that permits authenticated users and rely on service-layer `@PreAuthorize` where present—pick one approach and apply consistently to the methods named above.
🧹 Nitpick comments (5)
src/main/resources/templates/fragments/cases.html (2)
18-22: Count expression concatenatesnullifcountSuffixis omitted.If a caller invokes
caseSection(...)without an explicitcountSuffix(or passesnull), the rendered label becomes e.g."3 casesnull". Current callers invisa/cases.htmlreportedly pass strings, but the fragment is more robust if it defaults explicitly:♻️ Suggested null-safe rendering
- <span class="case-count" - th:text="${`#lists.size`(cases) + ' case' + (`#lists.size`(cases) == 1 ? '' : 's') + countSuffix}"> + <span class="case-count" + th:text="${`#lists.size`(cases) + ' case' + (`#lists.size`(cases) == 1 ? '' : 's') + (countSuffix != null ? countSuffix : '')}"> 0 cases </span>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/fragments/cases.html` around lines 18 - 22, The count expression currently concatenates a null countSuffix and results like "3 casesnull"; update the span's Thymeleaf expression to default countSuffix to an empty string when null (e.g., use the Elvis/default operator or `#strings.defaultString`) so the computed text for the element (the expression in the span with class "case-count") appends '' instead of null; keep the rest of the pluralization logic intact.
46-50: Enum rendering uses defaulttoString()— confirm consistency with the rest of the PR.Line 46 (
${visa.visaType}) and line 50 (${visa.visaStatus}) rely on enum defaulttoString(), while line 49 explicitly callsvisa.visaStatus.name(). The PR objective mentions "useenum.name()in templates" for stability against futuretoString()overrides. IfVisaTypeorVisaStatusever gain a customtoString()(e.g., for a localized label), the text on lines 46/50 will silently diverge from the CSS class on line 49.♻️ Optional consistency tweak
- <td th:text="${visa.visaType}">STUDENT</td> + <td th:text="${visa.visaType != null ? visa.visaType.name() : ''}">STUDENT</td> <td> <span class="status" th:classappend="${visa.visaStatus != null ? 'status-' + `#strings.toLowerCase`(visa.visaStatus.name()) : ''}" - th:text="${visa.visaStatus}">ASSIGNED</span> + th:text="${visa.visaStatus != null ? visa.visaStatus.name() : ''}">ASSIGNED</span> </td>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/fragments/cases.html` around lines 46 - 50, The template currently renders enums via their default toString() at ${visa.visaType} and ${visa.visaStatus} while using visa.visaStatus.name() for the CSS class; update the text renderings to use enum.name() as well (e.g., replace references to visa.visaType and visa.visaStatus with visa.visaType.name() and visa.visaStatus.name()) so the displayed text and the class generation (which uses `#strings.toLowerCase`(visa.visaStatus.name())) remain consistent even if toString() is later overridden.src/test/java/org/example/visacasemanagementsystem/visa/VisaServiceTest.java (1)
322-459: LGTM — solid coverage for the new case-list queries.The new tests correctly assert both the repository call signatures (status sets +
Sort.by("updatedAt").descending()) and the DTO mapping/order. Empty-result paths verifyingverifyNoInteractions(visaMapper)is a nice touch.Optional nit: in the empty-list tests (Lines 362-375, 401-413, 446-459) the stubs use
anyLong()/anyList()/any(Sort.class), so an accidentally wrong status set or sort wouldn't be caught. Tightening these to the sameeq(...)arguments as their non-empty counterparts would close the gap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/visa/VisaServiceTest.java` around lines 322 - 459, The empty-list tests use loose matchers which can hide incorrect query args; update the stubs in findOpenCasesByHandler_shouldReturnEmptyList_whenHandlerHasNoOpenCases to use eq(handlerId), eq(List.of(VisaStatus.ASSIGNED, VisaStatus.INCOMPLETE)), eq(Sort.by("updatedAt").descending()) for the visaRepository.findByHandler_IdAndVisaStatusIn call, update findUnassignedCases_shouldReturnEmptyList_whenNoSubmittedCasesExist to use eq(VisaStatus.SUBMITTED) and eq(Sort.by("updatedAt").descending()) for visaRepository.findByVisaStatusAndHandlerIsNull, and update findHandledCasesByHandler_shouldReturnEmptyList_whenHandlerHasClosedNothing to use eq(handlerId), eq(List.of(VisaStatus.GRANTED, VisaStatus.REJECTED)), eq(Sort.by("updatedAt").descending()) for visaRepository.findByHandler_IdAndVisaStatusIn so the stubs exactly match the expected query arguments and prevent false positives.src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java (1)
157-166: Stale.param("currentUserId", …)form fields.
currentUserIdis no longer a@RequestParamon the controller (it's resolved from@AuthenticationPrincipal), so these params are silently ignored. They aren't harmful, just dead code that suggests the old API. Cleaning them up makes the tests easier to read and avoids the impression that the controller still binds the parameter.Also applies to: 181-187, 249-258, 277-285
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java` around lines 157 - 166, Tests in VisaViewControllerTest are passing a stale form param "currentUserId" to multipart requests even though the controller now resolves the user via `@AuthenticationPrincipal`, so remove the dead .param("currentUserId", userId.toString()) entries from the multipart requests in the test methods (e.g., the mockMvc.perform(multipart("/visa/apply")...) and the other occurrences called out) to clean up the tests and avoid implying the controller binds that request parameter; keep the remaining params and csrf() as-is.src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java (1)
94-106: Remove redundant session invalidation — SecurityContextLogoutHandler already handles it.
SecurityContextLogoutHandler.logout()invalidates the HTTP session by default (invalidateHttpSession=true). The explicitgetSession(false)/invalidate()block on lines 101–104 is redundant and can be removed. While harmless on Tomcat (wheregetSession(false)returnsnullafter invalidation), other containers may retain the session reference and throwIllegalStateExceptionon a secondinvalidate()call.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java` around lines 94 - 106, The logout method in UserViewController currently calls SecurityContextLogoutHandler.logout(request, response, auth) and then redundantly fetches and invalidates the HttpSession again; remove the explicit session invalidation block (the getSession(false)/invalidate() lines) to avoid double-invalidation and potential IllegalStateException in some containers. Keep the SecurityContextLogoutHandler.logout(...) call as the sole logout/session invalidation mechanism (ensure any custom logout handler configuration still uses invalidateHttpSession=true 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/java/org/example/visacasemanagementsystem/config/SecurityConfig.java`:
- Around line 42-58: SecurityConfig currently only matches /visa/cases,
/visa/my-applications and /visa/apply so per-case endpoints like GET /visa/{id},
/visa/{id}/edit and all POST /visa/{id}/... fall through to
.anyRequest().hasRole("SYSADMIN") and become inaccessible; update the
SecurityConfig requestMatchers to include a more general /visa/** rule (e.g.,
add a .requestMatchers("/visa/**").authenticated() or finer-grained roles as
needed) and then add explicit method-level `@PreAuthorize` annotations on
VisaViewController methods (and keep/adjust VisaService `@PreAuthorize`
annotations) so owner-facing actions use hasRole('USER') while admin handlers
use hasAnyRole('ADMIN','SYSADMIN').
---
Outside diff comments:
In
`@src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java`:
- Around line 58-73: The POST createUser handler allows authenticated principals
to create new accounts; add the same guard as the GET signup handler at the
start of the createUser method so authenticated users are redirected to "/home"
before calling userService.createUser. Implement this by checking the current
Authentication (e.g., SecurityContextHolder.getContext().getAuthentication() or
an injected Principal/@AuthenticationPrincipal), verify
authentication.isAuthenticated() and that the principal is not anonymous, and
return "redirect:/home" if true; otherwise proceed to construct CreateUserDTO
and call userService.createUser as before.
In
`@src/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.java`:
- Around line 134-258: The controller endpoints under VisaViewController (e.g.,
showEditForm, processUpdate, deleteVisaDocument, approveVisa,
requestMoreInformation, rejectVisa, assignCaseToHandler) are blocked by the
global .anyRequest().hasRole("SYSADMIN") rule; add explicit method-level
security annotations to restore intended access: annotate view/edit endpoints
(showEditForm, processUpdate, and the GET /visa/{id} handler) with
`@PreAuthorize`("hasAnyRole('USER','ADMIN','SYSADMIN')") and annotate case-action
endpoints (approveVisa, rejectVisa, requestMoreInformation, assignCaseToHandler,
deleteVisaDocument if appropriate) with
`@PreAuthorize`("hasAnyRole('ADMIN','SYSADMIN')"); alternatively, adjust
SecurityConfig to include a /visa/** matcher that permits authenticated users
and rely on service-layer `@PreAuthorize` where present—pick one approach and
apply consistently to the methods named above.
In
`@src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java`:
- Around line 358-437: The admin-only tests
(approveVisa_AsAdmin_ShouldRedirectToDetailsView,
assignCaseToHandler_Success_ShouldRedirectToDetails,
requestMoreInformation_Success_ShouldRedirectToDetails,
rejectVisa_AsAdmin_ShouldRedirectToDetailsView and the later
viewDetails_AsAdmin_ShouldAllowViewingOtherVisas) are using plain `@WithMockUser`
which creates ROLE_USER; update each of these test methods to use
`@WithMockUser`(roles = "ADMIN") so the mock principal has ADMIN authority and the
controller’s admin-only checks (e.g., `@PreAuthorize`) will pass; locate the
annotations above the methods named
approveVisa_AsAdmin_ShouldRedirectToDetailsView,
assignCaseToHandler_Success_ShouldRedirectToDetails,
requestMoreInformation_Success_ShouldRedirectToDetails,
rejectVisa_AsAdmin_ShouldRedirectToDetailsView and
viewDetails_AsAdmin_ShouldAllowViewingOtherVisas and change the annotation
accordingly.
---
Nitpick comments:
In
`@src/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.java`:
- Around line 94-106: The logout method in UserViewController currently calls
SecurityContextLogoutHandler.logout(request, response, auth) and then
redundantly fetches and invalidates the HttpSession again; remove the explicit
session invalidation block (the getSession(false)/invalidate() lines) to avoid
double-invalidation and potential IllegalStateException in some containers. Keep
the SecurityContextLogoutHandler.logout(...) call as the sole logout/session
invalidation mechanism (ensure any custom logout handler configuration still
uses invalidateHttpSession=true if needed).
In `@src/main/resources/templates/fragments/cases.html`:
- Around line 18-22: The count expression currently concatenates a null
countSuffix and results like "3 casesnull"; update the span's Thymeleaf
expression to default countSuffix to an empty string when null (e.g., use the
Elvis/default operator or `#strings.defaultString`) so the computed text for the
element (the expression in the span with class "case-count") appends '' instead
of null; keep the rest of the pluralization logic intact.
- Around line 46-50: The template currently renders enums via their default
toString() at ${visa.visaType} and ${visa.visaStatus} while using
visa.visaStatus.name() for the CSS class; update the text renderings to use
enum.name() as well (e.g., replace references to visa.visaType and
visa.visaStatus with visa.visaType.name() and visa.visaStatus.name()) so the
displayed text and the class generation (which uses
`#strings.toLowerCase`(visa.visaStatus.name())) remain consistent even if
toString() is later overridden.
In
`@src/test/java/org/example/visacasemanagementsystem/visa/VisaServiceTest.java`:
- Around line 322-459: The empty-list tests use loose matchers which can hide
incorrect query args; update the stubs in
findOpenCasesByHandler_shouldReturnEmptyList_whenHandlerHasNoOpenCases to use
eq(handlerId), eq(List.of(VisaStatus.ASSIGNED, VisaStatus.INCOMPLETE)),
eq(Sort.by("updatedAt").descending()) for the
visaRepository.findByHandler_IdAndVisaStatusIn call, update
findUnassignedCases_shouldReturnEmptyList_whenNoSubmittedCasesExist to use
eq(VisaStatus.SUBMITTED) and eq(Sort.by("updatedAt").descending()) for
visaRepository.findByVisaStatusAndHandlerIsNull, and update
findHandledCasesByHandler_shouldReturnEmptyList_whenHandlerHasClosedNothing to
use eq(handlerId), eq(List.of(VisaStatus.GRANTED, VisaStatus.REJECTED)),
eq(Sort.by("updatedAt").descending()) for
visaRepository.findByHandler_IdAndVisaStatusIn so the stubs exactly match the
expected query arguments and prevent false positives.
In
`@src/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java`:
- Around line 157-166: Tests in VisaViewControllerTest are passing a stale form
param "currentUserId" to multipart requests even though the controller now
resolves the user via `@AuthenticationPrincipal`, so remove the dead
.param("currentUserId", userId.toString()) entries from the multipart requests
in the test methods (e.g., the mockMvc.perform(multipart("/visa/apply")...) and
the other occurrences called out) to clean up the tests and avoid implying the
controller binds that request parameter; keep the remaining params and csrf()
as-is.
🪄 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: 0587c53c-3ed0-4d9b-9d14-da22aced6a02
📒 Files selected for processing (13)
src/main/java/org/example/visacasemanagementsystem/audit/service/UserLogService.javasrc/main/java/org/example/visacasemanagementsystem/audit/service/VisaLogService.javasrc/main/java/org/example/visacasemanagementsystem/config/SecurityConfig.javasrc/main/java/org/example/visacasemanagementsystem/user/controller/UserViewController.javasrc/main/java/org/example/visacasemanagementsystem/visa/controller/VisaViewController.javasrc/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.javasrc/main/resources/templates/fragments/cases.htmlsrc/main/resources/templates/user/login.htmlsrc/main/resources/templates/visa/edit-form.htmlsrc/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaServiceIntegrationTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaServiceTest.javasrc/test/java/org/example/visacasemanagementsystem/visa/VisaViewControllerTest.java
✅ Files skipped from review due to trivial changes (1)
- src/test/java/org/example/visacasemanagementsystem/user/controller/UserViewControllerTest.java
🚧 Files skipped from review as they are similar to previous changes (4)
- src/test/java/org/example/visacasemanagementsystem/visa/VisaServiceIntegrationTest.java
- src/main/java/org/example/visacasemanagementsystem/audit/service/VisaLogService.java
- src/main/java/org/example/visacasemanagementsystem/audit/service/UserLogService.java
- src/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.java
…exception of role specific ones Feature: 403 AccessDenied now properly redirects to the edit page
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.
Summary by CodeRabbit
New Features
UI Improvements
Navigation Updates
Tests