Feature/visualize comment log and file log#56
Conversation
- Implemented findFiltered service methods CommentLogService and FileLogService - Implemented sysadmin accessible end points /log/comment and /log/file - Updated header with links to new endpoints - Created matching html pages for File Log and Comment Log - Created tests for service methods and controller
Feature: Comment Log now links to the Visa Case where the comment was made
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
📝 WalkthroughWalkthroughThis PR extends the audit logging system by introducing dedicated comment and file log pages with filtering and pagination capabilities, updating service-layer authorization checks to include SYSADMIN roles, and implementing comprehensive test coverage for the new endpoints and filtering logic. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User (Authenticated)
participant Controller as LogViewController
participant Service as CommentLogService/<br/>FileLogService
participant Repo as Repository<br/>(JpaSpecificationExecutor)
participant DB as Database
User->>Controller: GET /log/comment?eventType=X&from=Y&to=Z&page=0
activate Controller
Controller->>Service: findFiltered(eventType, from, to, pageable)
deactivate Controller
activate Service
Service->>Service: Build dynamic Specification<br/>(conditionally append predicates)
Service->>Repo: findAll(specification, pageable)
deactivate Service
activate Repo
Repo->>DB: Execute JPA Criteria Query<br/>(with optional filters)
DB-->>Repo: Page<CommentLog>
deactivate Repo
activate Service
Service->>Service: Map entities to DTOs
Service-->>Controller: Page<CommentLogDTO>
deactivate Service
Controller-->>User: Render log/comment.html<br/>with filtered results & pagination
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate 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: 2
🧹 Nitpick comments (7)
src/main/resources/templates/log/file.html (1)
50-50: Move inline button styles into the existing component CSS.Inlined
style="width: auto; padding: 8px 16px;"on the submit button mixes presentation into the template and is duplicated inlog/comment.html(line 50). Prefer a small modifier class (e.g.,btn-submit btn-inline) defined incomponents.css.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/log/file.html` at line 50, Remove the inline style on the submit button element and replace it with a small modifier class (e.g., add "btn-inline" alongside "btn-submit" on the <button> in the template), then add the corresponding CSS rule in the shared component stylesheet (components.css) like .btn-inline { width: auto; padding: 8px 16px; } so presentation is centralized; apply the same change to the duplicate button in log/comment.html to avoid duplication.src/main/resources/templates/log/comment.html (1)
1-123: Consider extracting a shared log-page Thymeleaf fragment.
log/comment.htmlandlog/file.htmlare structurally identical: same header/count, same filter form, same pager, same empty state, with only the action URL, table columns, and event-type field differing. As more log views are added (or the existing ones evolve — pager styling, accessibility, sort controls, etc.), they will drift. Afragments/log-page.htmlfragment parameterized by the action URL, column definitions, and event-type field would remove the duplication.Not blocking — the current template renders correctly and pagination/filter state is preserved.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/log/comment.html` around lines 1 - 123, Extract the repeated page shell (header, log-count, filter form, pager and empty-state) from log/comment.html and log/file.html into a parameterized Thymeleaf fragment e.g. fragments/log-page.html; expose parameters for the form action URL, the eventTypes/selectedEventType bindings, the date field names/values (from/to), the logs Page object, and the table body/header insertion point. Replace the duplicated markup in log/comment.html and log/file.html with a single th:replace call to fragments/log-page.html, passing the specific action URL and a fragment (or markup) for the table columns/rows and event-type select so each page supplies only its differing pieces while the common pagination/filtering logic lives in the fragment.src/test/java/org/example/visacasemanagementsystem/audit/service/CommentLogServiceTest.java (2)
199-199: Nit:nullable(SingularAttribute.class)is broader than needed.
root.get(...)is invoked here with aSingularAttribute(via the JPA static metamodelCommentLog_), not with a null argument.any(SingularAttribute.class)(which excludes nulls) would more precisely express the expectation;nullable(...)is typically reserved for matchers that must also acceptnull. Same nit applies at lines 234, 269, and 317. Cosmetic only.🤖 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/CommentLogServiceTest.java` at line 199, Replace the broader nullable matcher with a non-null matcher where the test expects a real SingularAttribute: change usages of nullable(SingularAttribute.class) in the CommentLogServiceTest Mockito stubbings (the when(root.get(...)) calls) to any(SingularAttribute.class) so the matcher more precisely reflects that CommentLog_.<...> attributes (not null) are passed; apply the same update to the other occurrences mentioned (the other when(root.get(...)) stubbings in the test).
280-330: Composition assertion is tightly coupled to evaluation order — consider loosening or guarding.The chained
cb.and(eqPred, gtePred) → and1andcb.and(and1, ltePred) → and2stubs (Lines 321–322) replicate the production code's exact predicate-combining order. The inline comment already calls this out, which is good. However,Specification.and(...)is a default method implementation ofSpecificationin Spring Data JPA; if that internal composition strategy ever changes (e.g. to flatten into a singlecb.and(p1, p2, p3)varargs call, whichcb.andsupports), every "all filters" test in this file will silently break even though the production logic remains semantically equivalent.Two lighter-touch options if you want to keep this future-proof without losing intent:
- Assert only the three leaf predicate calls (
cb.equal,cb.greaterThanOrEqualTo,cb.lessThanOrEqualTo) plus thatactualis non-null, and drop thecb.and(...)chaining stubs.- Or use
verify(cb, atLeastOnce()).and(any(Predicate.class), any(Predicate.class))to assert that AND-composition occurred without pinning the exact pairing tree.Not blocking — the comment makes the coupling explicit, and the same pattern is mirrored in
FileLogServiceTest.🤖 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/CommentLogServiceTest.java` around lines 280 - 330, The test tightly couples to the exact predicate-composition order by stubbing cb.and(eqPred, gtePred) -> and1 and cb.and(and1, ltePred) -> and2 when capturing the Specification in CommentLogServiceTest.findFiltered_shouldComposeAllPredicates_WhenAllFiltersAreSet; loosen this by removing the cb.and(...) chaining stubs and either (a) assert the three leaf predicate calls (verify cb.equal(eventTypePath,...), cb.greaterThanOrEqualTo(...), cb.lessThanOrEqualTo(...)) and that specCaptor.getValue().toPredicate(...) returns non-null, or (b) replace the strict and stubs with a more permissive verify like verify(cb, atLeastOnce()).and(any(Predicate.class), any(Predicate.class)) so the test checks that AND-composition occurred without depending on the internal tree shape of Specification.and.src/main/java/org/example/visacasemanagementsystem/audit/controller/LogViewController.java (1)
138-145: Minor:pageTitlemodel attribute is set but not asserted in the new tests.
renderLogPageaddspageTitle(e.g."Comment Log","File Log") to the model, butLogViewControllerTestdoesn't assert it for any endpoint (existing or new). If the templates rely on${pageTitle}for the page heading or<title>, a regression that drops it from the controller would slip through web-layer tests. Consider addingmodel().attribute("pageTitle", "Comment Log")(and similar) to the existing default-render tests, or accept the gap as covered by template/integration tests.🤖 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 138 - 145, The controller method renderLogPage adds a "pageTitle" model attribute but tests in LogViewControllerTest do not assert it; update the web-layer tests that exercise renderLogPage (e.g., the tests that hit endpoints which call renderLogPage) to include assertions like model().attribute("pageTitle", "<expected title>") for each endpoint (for example "Comment Log" and "File Log") so the model contract is verified; locate the controller method renderLogPage and the corresponding test methods in LogViewControllerTest and add the model attribute assertions with the exact expected strings.src/test/java/org/example/visacasemanagementsystem/audit/controller/LogViewControllerTest.java (1)
338-429: Optional: asymmetric paging-edge coverage across the four endpoints.Pagination clamping (
MAX_PAGE_SIZE, negative page/size minima) lives in a single shared helper (buildPageableinLogViewController), so per-endpoint duplication isn't required for correctness. That said, the matrix is currently uneven:
Endpoint NoFilters Filters ExplicitPaging OversizedSize NegativePaging /log/visa✅ ✅ ✅ ✅ ✅ /log/user✅ ✅ — — — /log/comment✅ ✅ — ✅ — /log/file✅ ✅ ✅ — — If you want endpoint-symmetric guarantees (e.g. catching a future regression where a new handler stops calling
buildPageable), add the missing oversized/negative cases. Otherwise relying on the/log/visacases as the canonical clamping contract is fine — this is a code-organization choice, not a defect.🤖 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 338 - 429, The test coverage is asymmetric for paging edge cases; add tests that assert buildPageable clamping/negative handling for the remaining endpoints by exercising /log/user, /log/comment, and /log/file with oversized size and negative page/size params and verifying the corresponding service.findFiltered calls receive the clamped Pageable; specifically add tests similar to commentLog_AsSysadmin_WithOversizedPageSize_ShouldClampToMax and the visa negative-page/size tests but targeting UserLogController methods (userLog handler -> userLogService.findFiltered), CommentLogController handler (commentLogService.findFiltered) and FileLogController handler (fileLogService.findFiltered), using mockMvc.perform(get(...).param(...)) and eq(PageRequest.of(..., clampedOrMinSize, Sort.by(...))) in verify to assert the shared buildPageable behavior.src/test/java/org/example/visacasemanagementsystem/audit/service/FileLogServiceTest.java (1)
1-335: Heavy duplication withCommentLogServiceTest— consider an abstract base or parameterized helper later.This file is structurally identical to
CommentLogServiceTest.java(same filter scenarios, same mocked Criteria API plumbing, sameArgumentCaptorpatterns), differing only in entity/event-type names and DTO record shape. As more*LogServiceTestclasses accrete, an abstractAbstractLogServiceFilteredTest<E extends Enum<E>, ENTITY, DTO>could host the fourfindFiltered_*cases and thecreateX_shouldPassSameEntityInstancecase. Not blocking for this PR — flag it for a follow-up cleanup afterVisa/Userlog services adopt the samefindFilteredpattern.🤖 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/FileLogServiceTest.java` around lines 1 - 335, Tests in FileLogServiceTest duplicate CommentLogServiceTest; extract common filtered-spec and instance-passing tests into a generic base test class and reuse it. Create an abstract AbstractLogServiceFilteredTest<E, ENTITY, DTO> that contains the findFiltered_* tests (the ones invoking FileLogService#findFiltered behavior and capturing Specification) and the createFileLog_shouldPassSameEntityInstance check, parameterize it by enum/event type, entity and DTO, and have FileLogServiceTest and CommentLogServiceTest extend it, overriding/implementing factory methods or fields for the service under test, mapper, repository, example entity/DTO and the enum constant; update the concrete tests to only include service-specific cases not covered by the base class.
🤖 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/VisaLogService.java`:
- Line 47: The service-level authorization on VisaLogService (the `@PreAuthorize`
annotation on the findFiltered method) is too permissive and inconsistent with
the controller and PR summary; change the annotation on findFiltered (and
similarly on findAll in VisaLogService and on the findFiltered methods in
CommentLogService and FileLogService) from `@PreAuthorize`("isAuthenticated()") to
require the SYSADMIN role (e.g. `@PreAuthorize`("hasRole('SYSADMIN')" or the
equivalent expression used elsewhere) so the service layer enforces the same
SYSADMIN-only restriction as LogViewController.
In
`@src/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.java`:
- Line 332: Review whether SYSADMIN should have business-action privileges; if
SYSADMIN should be limited to audit/system tasks, change the security
annotations on the business endpoints (methods approveVisa, rejectVisa,
requestMoreInformation, assignHandler in VisaService) to restrict to ADMIN-only
(e.g., replace hasAnyRole('ADMIN','SYSADMIN') with an ADMIN-only expression such
as hasRole('ADMIN') or hasAnyRole('ADMIN')); if SYSADMINs are intentionally
allowed, leave as-is and add a comment documenting that SYSADMINs may be
assigned as handlers so reviewers know the choice.
---
Nitpick comments:
In
`@src/main/java/org/example/visacasemanagementsystem/audit/controller/LogViewController.java`:
- Around line 138-145: The controller method renderLogPage adds a "pageTitle"
model attribute but tests in LogViewControllerTest do not assert it; update the
web-layer tests that exercise renderLogPage (e.g., the tests that hit endpoints
which call renderLogPage) to include assertions like
model().attribute("pageTitle", "<expected title>") for each endpoint (for
example "Comment Log" and "File Log") so the model contract is verified; locate
the controller method renderLogPage and the corresponding test methods in
LogViewControllerTest and add the model attribute assertions with the exact
expected strings.
In `@src/main/resources/templates/log/comment.html`:
- Around line 1-123: Extract the repeated page shell (header, log-count, filter
form, pager and empty-state) from log/comment.html and log/file.html into a
parameterized Thymeleaf fragment e.g. fragments/log-page.html; expose parameters
for the form action URL, the eventTypes/selectedEventType bindings, the date
field names/values (from/to), the logs Page object, and the table body/header
insertion point. Replace the duplicated markup in log/comment.html and
log/file.html with a single th:replace call to fragments/log-page.html, passing
the specific action URL and a fragment (or markup) for the table columns/rows
and event-type select so each page supplies only its differing pieces while the
common pagination/filtering logic lives in the fragment.
In `@src/main/resources/templates/log/file.html`:
- Line 50: Remove the inline style on the submit button element and replace it
with a small modifier class (e.g., add "btn-inline" alongside "btn-submit" on
the <button> in the template), then add the corresponding CSS rule in the shared
component stylesheet (components.css) like .btn-inline { width: auto; padding:
8px 16px; } so presentation is centralized; apply the same change to the
duplicate button in log/comment.html to avoid duplication.
In
`@src/test/java/org/example/visacasemanagementsystem/audit/controller/LogViewControllerTest.java`:
- Around line 338-429: The test coverage is asymmetric for paging edge cases;
add tests that assert buildPageable clamping/negative handling for the remaining
endpoints by exercising /log/user, /log/comment, and /log/file with oversized
size and negative page/size params and verifying the corresponding
service.findFiltered calls receive the clamped Pageable; specifically add tests
similar to commentLog_AsSysadmin_WithOversizedPageSize_ShouldClampToMax and the
visa negative-page/size tests but targeting UserLogController methods (userLog
handler -> userLogService.findFiltered), CommentLogController handler
(commentLogService.findFiltered) and FileLogController handler
(fileLogService.findFiltered), using mockMvc.perform(get(...).param(...)) and
eq(PageRequest.of(..., clampedOrMinSize, Sort.by(...))) in verify to assert the
shared buildPageable behavior.
In
`@src/test/java/org/example/visacasemanagementsystem/audit/service/CommentLogServiceTest.java`:
- Line 199: Replace the broader nullable matcher with a non-null matcher where
the test expects a real SingularAttribute: change usages of
nullable(SingularAttribute.class) in the CommentLogServiceTest Mockito stubbings
(the when(root.get(...)) calls) to any(SingularAttribute.class) so the matcher
more precisely reflects that CommentLog_.<...> attributes (not null) are passed;
apply the same update to the other occurrences mentioned (the other
when(root.get(...)) stubbings in the test).
- Around line 280-330: The test tightly couples to the exact
predicate-composition order by stubbing cb.and(eqPred, gtePred) -> and1 and
cb.and(and1, ltePred) -> and2 when capturing the Specification in
CommentLogServiceTest.findFiltered_shouldComposeAllPredicates_WhenAllFiltersAreSet;
loosen this by removing the cb.and(...) chaining stubs and either (a) assert the
three leaf predicate calls (verify cb.equal(eventTypePath,...),
cb.greaterThanOrEqualTo(...), cb.lessThanOrEqualTo(...)) and that
specCaptor.getValue().toPredicate(...) returns non-null, or (b) replace the
strict and stubs with a more permissive verify like verify(cb,
atLeastOnce()).and(any(Predicate.class), any(Predicate.class)) so the test
checks that AND-composition occurred without depending on the internal tree
shape of Specification.and.
In
`@src/test/java/org/example/visacasemanagementsystem/audit/service/FileLogServiceTest.java`:
- Around line 1-335: Tests in FileLogServiceTest duplicate
CommentLogServiceTest; extract common filtered-spec and instance-passing tests
into a generic base test class and reuse it. Create an abstract
AbstractLogServiceFilteredTest<E, ENTITY, DTO> that contains the findFiltered_*
tests (the ones invoking FileLogService#findFiltered behavior and capturing
Specification) and the createFileLog_shouldPassSameEntityInstance check,
parameterize it by enum/event type, entity and DTO, and have FileLogServiceTest
and CommentLogServiceTest extend it, overriding/implementing factory methods or
fields for the service under test, mapper, repository, example entity/DTO and
the enum constant; update the concrete tests to only include service-specific
cases not covered by the base class.
🪄 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: f2eb8cdf-eb91-4618-993f-3688b8b3ccf0
📒 Files selected for processing (13)
src/main/java/org/example/visacasemanagementsystem/audit/controller/LogViewController.javasrc/main/java/org/example/visacasemanagementsystem/audit/repository/CommentLogRepository.javasrc/main/java/org/example/visacasemanagementsystem/audit/repository/FileLogRepository.javasrc/main/java/org/example/visacasemanagementsystem/audit/service/CommentLogService.javasrc/main/java/org/example/visacasemanagementsystem/audit/service/FileLogService.javasrc/main/java/org/example/visacasemanagementsystem/audit/service/VisaLogService.javasrc/main/java/org/example/visacasemanagementsystem/visa/service/VisaService.javasrc/main/resources/templates/fragments/header.htmlsrc/main/resources/templates/log/comment.htmlsrc/main/resources/templates/log/file.htmlsrc/test/java/org/example/visacasemanagementsystem/audit/controller/LogViewControllerTest.javasrc/test/java/org/example/visacasemanagementsystem/audit/service/CommentLogServiceTest.javasrc/test/java/org/example/visacasemanagementsystem/audit/service/FileLogServiceTest.java
| * only non-null filters are added to the query, so Hibernate never has to | ||
| * bind a null enum parameter (which fails in Hibernate 6/7). | ||
| */ | ||
| @PreAuthorize("isAuthenticated()") |
There was a problem hiding this comment.
Service-level guard is weaker than the controller and inconsistent with the PR summary.
The new @PreAuthorize("isAuthenticated()") here (and on the sibling findFiltered methods in CommentLogService/FileLogService) lets any authenticated principal — including USER and ADMIN — invoke this method directly, even though LogViewController only exposes the page to SYSADMIN. The AI-generated summary also describes this PR as "updating service-layer authorization checks to include SYSADMIN roles", which doesn't match what the annotation actually expresses.
For defense in depth (and to align with the summary), consider tightening to the role that actually owns the audit-log surface, e.g.:
🔒 Suggested change
- `@PreAuthorize`("isAuthenticated()")
+ `@PreAuthorize`("hasRole('SYSADMIN')")
public Page<VisaLogDTO> findFiltered(VisaEventType eventType,The existing findAll() annotation could reasonably be tightened in the same pass, since it's reached by the same SYSADMIN-only page.
🤖 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`
at line 47, The service-level authorization on VisaLogService (the `@PreAuthorize`
annotation on the findFiltered method) is too permissive and inconsistent with
the controller and PR summary; change the annotation on findFiltered (and
similarly on findAll in VisaLogService and on the findFiltered methods in
CommentLogService and FileLogService) from `@PreAuthorize`("isAuthenticated()") to
require the SYSADMIN role (e.g. `@PreAuthorize`("hasRole('SYSADMIN')" or the
equivalent expression used elsewhere) so the service layer enforces the same
SYSADMIN-only restriction as LogViewController.
|
@coderabbitai review the last two commits |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 951 🏁 Script executed: Length of output: 58022
Commit 1 — Restrict
|
1 participant
Summary by CodeRabbit
Release Notes