Skip to content

feat: implement and improve audit logging with user tracking and UI enhancements - #38

Merged
addee1 merged 13 commits into
mainfrom
feature/audit-logging-improvements
Apr 21, 2026
Merged

feat: implement and improve audit logging with user tracking and UI enhancements#38
addee1 merged 13 commits into
mainfrom
feature/audit-logging-improvements

Conversation

@addee1

@addee1 addee1 commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

This PR completes and improves the audit logging functionality across the application.

What was added/changed:

  • Added user tracking to audit logs (who performed the action)
  • Improved audit logging in services (status changes, assignments, attachments)
  • Replaced technical values (e.g. objectKey) with user-friendly data (e.g. file names)
  • Implemented formatted timestamps for better readability
  • Updated UI to clearly display:
    • Who performed the action
    • What action was performed
    • Relevant details (e.g. status changes, attachments)
    • When the action occurred

Result:

Audit logs are now significantly more user-friendly and provide a clear overview of system activity (Who, What, When), instead of raw technical data.

Summary by CodeRabbit

  • New Features
    • Audit logs now show acting user (or "System"), include field-level old/new values, and cover ticket creation, status changes, assignments/unassignments, and attachment additions.
  • UX Improvements
    • Audit entries rendered as structured cards with readable action labels and formatted timestamps.
  • Tests
    • Unit tests updated to verify new audit logging behavior.

@coderabbitai

coderabbitai Bot commented Apr 21, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@addee1 has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 26 minutes and 29 seconds before requesting another review.

Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 26 minutes and 29 seconds.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

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 configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 35402de7-6985-4fd2-be6f-1546c97532f8

📥 Commits

Reviewing files that changed from the base of the PR and between b51a8e2 and eb56868.

📒 Files selected for processing (3)
  • src/main/java/org/example/alfs/config/SecurityConfig.java
  • src/main/java/org/example/alfs/services/TicketService.java
  • src/test/java/org/example/alfs/services/TicketServiceTest.java
📝 Walkthrough

Walkthrough

Adds user-aware audit logging and rendering: new AuditService.log overload that accepts a User, TicketService and AttachmentService emit user-backed audit entries, AuditLog and TicketViewDTO expose formatted createdAt, and the UI template renders structured audit cards with user, action, values, and timestamp.

Changes

Cohort / File(s) Summary
Audit Entity
src/main/java/org/example/alfs/entities/AuditLog.java
Added public String getFormattedCreatedAt() returning empty string if createdAt is null, otherwise formatted with dd MMM yyyy HH:mm.
Audit Service
src/main/java/org/example/alfs/services/AuditService.java
Added overloaded log(...) that accepts a User and persists an AuditLog with the user set; existing log(...) without User remains.
Ticket service integration
src/main/java/org/example/alfs/services/TicketService.java
Injected AuditService via constructor; emits audit logs for ticket creation, status changes, investigator assign/unassign (captures previous state where needed); createNewTicket annotated @Transactional.
Attachment service change
src/main/java/org/example/alfs/services/AttachmentService.java
Audit call in uploadToTicket changed to log the persisted attachment filename and include User when calling auditService.log(...).
DTO formatting
src/main/java/org/example/alfs/dto/ticket/TicketViewDTO.java
Introduced shared DateTimeFormatter (DISPLAY_FORMATTER) with Locale.ENGLISH and reused it in getFormattedCreatedAt().
UI / Template
src/main/jte/view.jte
Refactored audit log rendering to show acting user (or "System"), mapped action labels, optional old/new value rows, and log.getFormattedCreatedAt().
Tests
src/test/java/org/example/alfs/services/TicketServiceTest.java
Added @Mock AuditService and verification of auditService.log(...) invocation in unassignInvestigator_shouldSucceed test.

Sequence Diagram(s)

sequenceDiagram
    autonumber
    actor User
    participant UI as "UI (view.jte)"
    participant TicketService as "TicketService"
    participant TicketRepo as "TicketRepository"
    participant AttachmentService as "AttachmentService"
    participant AuditService as "AuditService"
    participant DB as "AuditLog (DB)"

    User->>UI: create ticket / update status / upload attachment
    UI->>TicketService: createTicket / updateStatus / assign/unassign
    TicketService->>TicketRepo: save(ticket)
    TicketRepo-->>TicketService: savedTicket
    TicketService->>AuditService: log(action, field, old, new, ticket, user)
    AuditService->>DB: create & persist AuditLog (action, user, ticket, values)
    DB-->>AuditService: persisted
    AuditService-->>TicketService: return

    User->>UI: upload file
    UI->>AttachmentService: uploadToTicket(file, ticket, user)
    AttachmentService->>AttachmentService: persist attachment (att)
    AttachmentService->>AuditService: log(ATTACHMENT_ADDED, null, null, att.getFileName(), ticket, user)
    AuditService->>DB: create & persist AuditLog
    DB-->>AuditService: persisted
    AuditService-->>AttachmentService: return
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

Possibly related issues

Possibly related PRs

Suggested reviewers

  • simonforsberg
  • FionaSprinkles

Poem

🐰 I hop through code with a bright little grin,
Tucking timestamps where audits begin.
Users and actions now tidy and neat,
Old values noted — no detail we’ll beat.
Carrot crumbs of logs, a celebratory treat! 🥕

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 30.77% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely summarizes the main objective: implementing audit logging with user tracking and UI enhancements, which aligns with all core changes across the codebase.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feature/audit-logging-improvements

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/alfs/services/TicketService.java (1)

361-373: ⚠️ Potential issue | 🔴 Critical

Critical: NullPointerException on every unassign.

ticket.setInvestigator(null) runs at line 361, so by the time ticket.getInvestigator().getUsername() is evaluated at line 369, getInvestigator() returns null and this dereference throws NPE — failing every unassign operation (and, because the method is @Transactional, rolling back the unassign itself). Capture the username before clearing the reference.

🐛 Proposed fix
+        String previousInvestigator = ticket.getInvestigator().getUsername();
+
         ticket.setInvestigator(null);
         ticket.setStatus(TicketStatus.OPEN);

         Ticket savedTicket = ticketRepository.save(ticket);

         auditService.log(
                 AuditAction.UNASSIGNED,
                 "investigator",
-                ticket.getInvestigator().getUsername(),
+                previousInvestigator,
                 null,
                 savedTicket,
                 user
         );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/alfs/services/TicketService.java` around lines 361
- 373, The code nulls the investigator before logging, causing a
NullPointerException when auditService.log tries to call
ticket.getInvestigator().getUsername(); fix by capturing the investigator
username into a local variable (e.g., String oldInvestigator =
ticket.getInvestigator() != null ? ticket.getInvestigator().getUsername() :
null) before calling ticket.setInvestigator(null), then pass that local variable
to auditService.log (retain use of AuditAction.UNASSIGNED,
ticketRepository.save(ticket), and savedTicket for context).
🧹 Nitpick comments (4)
src/main/java/org/example/alfs/services/TicketService.java (1)

45-81: Recommended: make createNewTicket transactional and drop the ticketId: technical prefix.

Two points on this method:

  1. Unlike updateTicketStatus / assignInvestigator / unassignInvestigator, createNewTicket is not annotated @Transactional. ticketRepository.save(...) and auditService.log(...) therefore run in separate transactions, so an audit failure leaves a ticket persisted without a matching CREATED audit row (and vice-versa for orphan audit rows would not happen here). Wrapping the method in @Transactional keeps the two writes atomic and consistent with the rest of the service.
  2. The PR objective explicitly calls out "replacing technical values (e.g., objectKey) with user-friendly data", yet newValue here is "ticketId:" + saved.getId(). Consider using the ticket title (or leaving newValue null since CREATED doesn't really have a before/after value) to stay consistent with the user-friendly rendering goal.
♻️ Proposed refactor
-    //createNewTicket
+    //createNewTicket
+    `@Transactional`
     public TicketViewDTO createNewTicket(TicketCreateDTO dto) {
@@
         auditService.log(
                 AuditAction.CREATED,
-                "ticket",
+                "title",
                 null,
-                "ticketId:" + saved.getId(),
+                saved.getTitle(),
                 saved,
                 user
         );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/alfs/services/TicketService.java` around lines 45 -
81, Mark createNewTicket as `@Transactional` so ticketRepository.save(...) and
auditService.log(...) run in the same transaction; then change the
auditService.log(...) call to avoid the technical "ticketId:<id>" newValue—use a
user-friendly value such as saved.getTitle() (or null) instead of the
"ticketId:" prefix so the CREATED audit row contains readable information
consistent with other service methods.
src/main/java/org/example/alfs/services/AuditService.java (1)

20-42: Recommended: collapse the two overloads to avoid duplication.

The old 5-arg log(...) is duplicated boilerplate. Per the context snippet note for this PR, all current callers (TicketService, AttachmentService) already use the 6-arg overload, so the old one is effectively unused. Either delegate to the new method (passing null for user) or remove it outright so future callers don't silently drop the acting user.

♻️ Proposed refactor
-    // keeping old for safety
     public void log(AuditAction action, String fieldName, String oldValue, String newValue, Ticket ticket) {
-        AuditLog log = new AuditLog();
-        log.setAction(action);
-        log.setFieldName(fieldName);
-        log.setOldValue(oldValue);
-        log.setNewValue(newValue);
-        log.setTicket(ticket);
-        // createdAt sätts automatiskt via `@PrePersist` i AuditLog
-        auditLogRepository.save(log);
+        log(action, fieldName, oldValue, newValue, ticket, null);
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/alfs/services/AuditService.java` around lines 20 -
42, Collapse the duplicate log overloads in AuditService by removing the 5-arg
log(AuditAction, String, String, String, Ticket) or delegating it to the 6-arg
log(...) that accepts User; locate the two methods named log in AuditService and
either delete the older 5-arg version or replace its body with a single call to
log(action, fieldName, oldValue, newValue, ticket, null) so all audit writes go
through the AuditLog creation path that sets user and persists via
auditLogRepository.save(log); ensure callers (TicketService, AttachmentService)
keep using the 6-arg signature or are updated accordingly.
src/main/jte/view.jte (1)

309-319: Optional: compare enum values directly instead of via toString().equals(...).

log.getAction().toString().equals("...") branching is fragile (silent drift if an enum constant is renamed — the compiler won't catch it). JTE supports direct enum references, so prefer identity comparison against AuditAction constants, which is both cheaper and refactor-safe.

♻️ Proposed refactor
-                            `@if`(log.getAction().toString().equals("ATTACHMENT_ADDED"))
+                            `@if`(log.getAction() == org.example.alfs.enums.AuditAction.ATTACHMENT_ADDED)
                                 File uploaded
-                            `@elseif`(log.getAction().toString().equals("STATUS_CHANGED"))
+                            `@elseif`(log.getAction() == org.example.alfs.enums.AuditAction.STATUS_CHANGED)
                                 Status updated
-                            `@elseif`(log.getAction().toString().equals("ASSIGNED"))
+                            `@elseif`(log.getAction() == org.example.alfs.enums.AuditAction.ASSIGNED)
                                 Investigator assigned
-                            `@elseif`(log.getAction().toString().equals("CREATED"))
+                            `@elseif`(log.getAction() == org.example.alfs.enums.AuditAction.CREATED)
                                 Ticket created
                             `@else`
-                                ${log.getAction().toString().replace("_", " ")}
+                                ${log.getAction().name().replace("_", " ")}
                             `@endif`

Note: UNASSIGNED and COMMENT_ADDED fall through to the @else, rendering as e.g. "UNASSIGNED" — you may want to add explicit branches for them too.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/jte/view.jte` around lines 309 - 319, Replace fragile string
comparisons using log.getAction().toString().equals("...") with direct enum
identity checks against the AuditAction constants (e.g., log.getAction() ==
AuditAction.ATTACHMENT_ADDED) inside the template; update the conditional chain
in the view to use these enum references for ATTACHMENT_ADDED, STATUS_CHANGED,
ASSIGNED, CREATED and consider adding explicit branches for UNASSIGNED and
COMMENT_ADDED so they don't fall through to the generic `@else` rendering. Ensure
you import or reference the AuditAction enum in the template context if needed
so the template can resolve AuditAction constants.
src/main/java/org/example/alfs/entities/AuditLog.java (1)

55-60: Optional: extract formatter as a static constant with an explicit locale.

Each call instantiates a new DateTimeFormatter; hoisting it to a private static final field avoids the allocation and is idiomatic. Also, MMM is locale-sensitive and currently falls back to the JVM default locale, which can produce inconsistent month abbreviations across environments. Consider pinning a locale (e.g., Locale.ENGLISH) to keep the rendered timestamp stable.

♻️ Proposed refactor
+import java.time.format.DateTimeFormatter;
+import java.util.Locale;
@@
 public class AuditLog {
+
+    private static final DateTimeFormatter DISPLAY_FORMATTER =
+            DateTimeFormatter.ofPattern("dd MMM yyyy HH:mm", Locale.ENGLISH);
@@
     public String getFormattedCreatedAt() {
         if (createdAt == null) return "";
-        return createdAt.format(
-                java.time.format.DateTimeFormatter.ofPattern("dd MMM yyyy HH:mm")
-        );
+        return createdAt.format(DISPLAY_FORMATTER);
     }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/alfs/entities/AuditLog.java` around lines 55 - 60,
The getFormattedCreatedAt method currently creates a new DateTimeFormatter on
each call and relies on the JVM default locale; extract the formatter into a
private static final field (e.g., CREATED_AT_FORMATTER) using
DateTimeFormatter.ofPattern("dd MMM yyyy HH:mm", Locale.ENGLISH) and update
AuditLog.getFormattedCreatedAt to use that constant when formatting createdAt to
avoid repeated allocations and ensure stable month abbreviations across
environments.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/main/java/org/example/alfs/services/TicketService.java`:
- Around line 361-373: The code nulls the investigator before logging, causing a
NullPointerException when auditService.log tries to call
ticket.getInvestigator().getUsername(); fix by capturing the investigator
username into a local variable (e.g., String oldInvestigator =
ticket.getInvestigator() != null ? ticket.getInvestigator().getUsername() :
null) before calling ticket.setInvestigator(null), then pass that local variable
to auditService.log (retain use of AuditAction.UNASSIGNED,
ticketRepository.save(ticket), and savedTicket for context).

---

Nitpick comments:
In `@src/main/java/org/example/alfs/entities/AuditLog.java`:
- Around line 55-60: The getFormattedCreatedAt method currently creates a new
DateTimeFormatter on each call and relies on the JVM default locale; extract the
formatter into a private static final field (e.g., CREATED_AT_FORMATTER) using
DateTimeFormatter.ofPattern("dd MMM yyyy HH:mm", Locale.ENGLISH) and update
AuditLog.getFormattedCreatedAt to use that constant when formatting createdAt to
avoid repeated allocations and ensure stable month abbreviations across
environments.

In `@src/main/java/org/example/alfs/services/AuditService.java`:
- Around line 20-42: Collapse the duplicate log overloads in AuditService by
removing the 5-arg log(AuditAction, String, String, String, Ticket) or
delegating it to the 6-arg log(...) that accepts User; locate the two methods
named log in AuditService and either delete the older 5-arg version or replace
its body with a single call to log(action, fieldName, oldValue, newValue,
ticket, null) so all audit writes go through the AuditLog creation path that
sets user and persists via auditLogRepository.save(log); ensure callers
(TicketService, AttachmentService) keep using the 6-arg signature or are updated
accordingly.

In `@src/main/java/org/example/alfs/services/TicketService.java`:
- Around line 45-81: Mark createNewTicket as `@Transactional` so
ticketRepository.save(...) and auditService.log(...) run in the same
transaction; then change the auditService.log(...) call to avoid the technical
"ticketId:<id>" newValue—use a user-friendly value such as saved.getTitle() (or
null) instead of the "ticketId:" prefix so the CREATED audit row contains
readable information consistent with other service methods.

In `@src/main/jte/view.jte`:
- Around line 309-319: Replace fragile string comparisons using
log.getAction().toString().equals("...") with direct enum identity checks
against the AuditAction constants (e.g., log.getAction() ==
AuditAction.ATTACHMENT_ADDED) inside the template; update the conditional chain
in the view to use these enum references for ATTACHMENT_ADDED, STATUS_CHANGED,
ASSIGNED, CREATED and consider adding explicit branches for UNASSIGNED and
COMMENT_ADDED so they don't fall through to the generic `@else` rendering. Ensure
you import or reference the AuditAction enum in the template context if needed
so the template can resolve AuditAction constants.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 9dfec583-2fce-4abd-8e45-8622d60c8857

📥 Commits

Reviewing files that changed from the base of the PR and between 9437115 and bb8e8aa.

📒 Files selected for processing (5)
  • src/main/java/org/example/alfs/entities/AuditLog.java
  • src/main/java/org/example/alfs/services/AttachmentService.java
  • src/main/java/org/example/alfs/services/AuditService.java
  • src/main/java/org/example/alfs/services/TicketService.java
  • src/main/jte/view.jte

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
src/main/java/org/example/alfs/services/TicketService.java (2)

324-336: ⚠️ Potential issue | 🟠 Major

Audit the implicit status transitions during assignment changes.

Assigning an investigator also changes status OPEN -> IN_PROGRESS, and unassigning changes IN_PROGRESS -> OPEN, but the audit trail only records the investigator field. This leaves user-visible status changes missing from the log.

📝 Proposed fix
+        TicketStatus oldStatus = ticket.getStatus();
         ticket.setInvestigator(investigator);
         ticket.setStatus(TicketStatus.IN_PROGRESS);

         Ticket savedTicket = ticketRepository.save(ticket);

         auditService.log(
                 AuditAction.ASSIGNED,
                 "investigator",
                 null,
                 investigator.getUsername(),
                 savedTicket,
                 user
         );
+        auditService.log(
+                AuditAction.STATUS_CHANGED,
+                "status",
+                oldStatus.name(),
+                TicketStatus.IN_PROGRESS.name(),
+                savedTicket,
+                user
+        );
+        TicketStatus oldStatus = ticket.getStatus();
         String oldInvestigator = ticket.getInvestigator().getUsername();

         ticket.setInvestigator(null);
         ticket.setStatus(TicketStatus.OPEN);

         Ticket savedTicket = ticketRepository.save(ticket);

         auditService.log(
                 AuditAction.UNASSIGNED,
                 "investigator",
                 oldInvestigator,
                 null,
                 savedTicket,
                 user
         );
+        auditService.log(
+                AuditAction.STATUS_CHANGED,
+                "status",
+                oldStatus.name(),
+                TicketStatus.OPEN.name(),
+                savedTicket,
+                user
+        );

Also applies to: 361-375

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/alfs/services/TicketService.java` around lines 324
- 336, The audit currently only logs the investigator change in TicketService
when assigning/unassigning (see ticket.setInvestigator and ticket.setStatus
followed by auditService.log); update the audit entries to also record the
implicit status transition by capturing the previous status (ticket.getStatus()
before mutation) and the new status (TicketStatus.IN_PROGRESS or
TicketStatus.OPEN) and include them in the auditService.log payload (e.g., as an
additional field or a separate audit entry) so both investigator and status
changes are auditable; apply the same change to the corresponding unassign
branch (the other block around the unassign logic referenced in the review).

45-72: ⚠️ Potential issue | 🟠 Major

Add @Transactional to make ticket creation and audit logging atomic.

Line 63 persists the ticket, then line 65 logs the audit entry via auditService.log(), which immediately calls auditLogRepository.save(). Without a transaction boundary on this method, if the audit save fails, the ticket remains persisted—leaving it without its required audit trail. The other audit-related lifecycle methods (updateTicketStatus, assignInvestigator, unassignInvestigator) are all annotated with @Transactional.

Suggested fix
+    `@Transactional`
     public TicketViewDTO createNewTicket(TicketCreateDTO dto) {
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/main/java/org/example/alfs/services/TicketService.java` around lines 45 -
72, Annotate the createNewTicket method in TicketService with `@Transactional` so
ticketRepository.save(...) and the subsequent auditService.log(...) (which calls
auditLogRepository.save()) run in the same transaction; update the method
signature to include the annotation on createNewTicket(TicketCreateDTO dto) and
ensure the class imports
org.springframework.transaction.annotation.Transactional (or the project’s
transaction annotation) so a failure in auditService.log will roll back the
ticket persist.
🧹 Nitpick comments (1)
src/test/java/org/example/alfs/services/TicketServiceTest.java (1)

44-45: Assert the new audit side effects, not just the injected mock.

The mock lets the service instantiate, but none of the lifecycle tests verify AuditAction, field values, ticket, or acting user. A regression that removes or mislabels audit logging would still pass.

🧪 Example assertions to add in lifecycle tests
+            verify(auditService).log(
+                    eq(AuditAction.CREATED),
+                    eq("ticket"),
+                    isNull(),
+                    startsWith("ticketId:"),
+                    any(Ticket.class),
+                    eq(reporter)
+            );
+            verify(auditService).log(
+                    eq(AuditAction.STATUS_CHANGED),
+                    eq("status"),
+                    eq(TicketStatus.OPEN.name()),
+                    eq(TicketStatus.IN_PROGRESS.name()),
+                    eq(ticket),
+                    eq(investigator)
+            );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/example/alfs/services/TicketServiceTest.java` around lines
44 - 45, The tests currently only inject a mock AuditService (AuditService) but
don't assert its side effects; update the lifecycle tests in TicketServiceTest
to capture and assert the AuditAction details: use an
ArgumentCaptor<AuditAction> (or Mockito.verify with argThat) for
auditService.record/save (whichever method is called), verify it was invoked the
expected number of times, and assert captured AuditAction fields (action type,
field values, the associated Ticket instance, and acting user/id) match the
expected lifecycle step; ensure each lifecycle test (create/update/close)
includes these assertions so regressions to audit logging are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/main/java/org/example/alfs/services/TicketService.java`:
- Around line 324-336: The audit currently only logs the investigator change in
TicketService when assigning/unassigning (see ticket.setInvestigator and
ticket.setStatus followed by auditService.log); update the audit entries to also
record the implicit status transition by capturing the previous status
(ticket.getStatus() before mutation) and the new status
(TicketStatus.IN_PROGRESS or TicketStatus.OPEN) and include them in the
auditService.log payload (e.g., as an additional field or a separate audit
entry) so both investigator and status changes are auditable; apply the same
change to the corresponding unassign branch (the other block around the unassign
logic referenced in the review).
- Around line 45-72: Annotate the createNewTicket method in TicketService with
`@Transactional` so ticketRepository.save(...) and the subsequent
auditService.log(...) (which calls auditLogRepository.save()) run in the same
transaction; update the method signature to include the annotation on
createNewTicket(TicketCreateDTO dto) and ensure the class imports
org.springframework.transaction.annotation.Transactional (or the project’s
transaction annotation) so a failure in auditService.log will roll back the
ticket persist.

---

Nitpick comments:
In `@src/test/java/org/example/alfs/services/TicketServiceTest.java`:
- Around line 44-45: The tests currently only inject a mock AuditService
(AuditService) but don't assert its side effects; update the lifecycle tests in
TicketServiceTest to capture and assert the AuditAction details: use an
ArgumentCaptor<AuditAction> (or Mockito.verify with argThat) for
auditService.record/save (whichever method is called), verify it was invoked the
expected number of times, and assert captured AuditAction fields (action type,
field values, the associated Ticket instance, and acting user/id) match the
expected lifecycle step; ensure each lifecycle test (create/update/close)
includes these assertions so regressions to audit logging are caught.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 8df01335-77ec-4dba-ac37-30ecad55d66c

📥 Commits

Reviewing files that changed from the base of the PR and between bb8e8aa and 394e4c7.

📒 Files selected for processing (2)
  • src/main/java/org/example/alfs/services/TicketService.java
  • src/test/java/org/example/alfs/services/TicketServiceTest.java

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
src/test/java/org/example/alfs/services/TicketServiceTest.java (1)

776-807: ⚠️ Potential issue | 🟡 Minor

Strengthen the audit verification: the previous-investigator username is never actually checked.

investigatorUser() doesn't set a username, so ticket.getInvestigator().getUsername() — the whole point of the oldValue captured by unassignInvestigator — is null in this test. The any() matcher at position 3 happily accepts that null, so the test currently passes even if the service had a bug that forgot to capture the previous username (e.g., reordered the statements and read the username after setInvestigator(null), which would NPE, or if it passed an empty string). Set a real username on the investigator and assert it explicitly.

🧪 Suggested stronger assertion
             User investigator = investigatorUser();
+            investigator.setUsername("inv-user");
             ticket.setInvestigator(investigator);
             ticket.setStatus(TicketStatus.IN_PROGRESS);
@@
             verify(auditService).log(
                     eq(AuditAction.UNASSIGNED),
                     eq("investigator"),
-                    any(),
+                    eq("inv-user"),
                     isNull(),
                     any(),
                     eq(admin)
             );
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@src/test/java/org/example/alfs/services/TicketServiceTest.java` around lines
776 - 807, Test currently doesn't verify the previous-investigator username
captured in the audit log; set a concrete username on the investigator and
assert it explicitly. In the test method unassignInvestigator_shouldSucceed,
after creating User investigator = investigatorUser() set
investigator.setUsername("investigator1") (or update investigatorUser() to
return a user with a non-null username), then change the auditService.log
verifier's third argument from any() to eq("investigator1") (keeping the other
matchers the same) so the test asserts TicketService.unassignInvestigator
actually passes the prior username as oldValue when calling auditService.log
with AuditAction.UNASSIGNED.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Outside diff comments:
In `@src/test/java/org/example/alfs/services/TicketServiceTest.java`:
- Around line 776-807: Test currently doesn't verify the previous-investigator
username captured in the audit log; set a concrete username on the investigator
and assert it explicitly. In the test method unassignInvestigator_shouldSucceed,
after creating User investigator = investigatorUser() set
investigator.setUsername("investigator1") (or update investigatorUser() to
return a user with a non-null username), then change the auditService.log
verifier's third argument from any() to eq("investigator1") (keeping the other
matchers the same) so the test asserts TicketService.unassignInvestigator
actually passes the prior username as oldValue when calling auditService.log
with AuditAction.UNASSIGNED.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 620e4585-9e8b-4d1c-9b7d-1ac7fcf92a38

📥 Commits

Reviewing files that changed from the base of the PR and between 394e4c7 and b51a8e2.

📒 Files selected for processing (4)
  • src/main/java/org/example/alfs/dto/ticket/TicketViewDTO.java
  • src/main/java/org/example/alfs/services/TicketService.java
  • src/main/jte/view.jte
  • src/test/java/org/example/alfs/services/TicketServiceTest.java
✅ Files skipped from review due to trivial changes (1)
  • src/main/java/org/example/alfs/dto/ticket/TicketViewDTO.java
🚧 Files skipped from review as they are similar to previous changes (1)
  • src/main/jte/view.jte

@addee1

addee1 commented Apr 21, 2026

Copy link
Copy Markdown
Contributor Author

The nitpicks is fixed.

@addee1 addee1 self-assigned this Apr 21, 2026

@simonforsberg simonforsberg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Highly approved! Mighty fine work.

@addee1
addee1 merged commit 41c00e6 into main Apr 21, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants