feature/expand-activity-log#14
Conversation
…ransaction management
📝 WalkthroughWalkthroughThe pull request modifies transaction propagation for Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
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 docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/group1/projectbackend/service/ActivityLogService.java (1)
38-53:⚠️ Potential issue | 🔴 Critical
REQUIRES_NEWwill silently break activity logging for newly created tickets.
createActivityLogruns in a brand-new transaction (separate JDBC connection) that immediately callssupportTicketRepository.findById(dto.getSupportTicketId())(line 45). In theSupportTicketServiceImpl.createTicketflow, the ticket has just been persisted in the outer transaction and is not yet committed whenlogActivitySafelyis invoked. Under the defaultREAD_COMMITTEDisolation level, the new transaction cannot see that row, sofindByIdreturns empty and throwsResourceNotFoundException— which is then silently caught and logged by thecatch (RuntimeException)block inlogActivitySafely.Net effect: the
TICKET_CREATEDactivity log entry is lost for every newly created ticket.The
updateStatus,CommentService.createComment, andDocumentServiceImpl.upload/deleteDocumentcallers are not affected because they reference user/ticket rows that were committed before the request.Options:
- Drop
REQUIRES_NEWand keep activity-log creation in the caller's transaction (simplest; lets the new ticket be visible).- Keep
REQUIRES_NEWbut defer the call until after the parent commit (e.g.,TransactionSynchronizationManager.registerSynchronization(...).afterCommit()orApplicationEventPublisher+@TransactionalEventListener(AFTER_COMMIT)).- Pass the already-loaded
User/SupportTicketentities to avoid refetching via FK, eliminating the visibility dependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/group1/projectbackend/service/ActivityLogService.java` around lines 38 - 53, The method ActivityLogService.createActivityLog is annotated with `@Transactional`(propagation = Propagation.REQUIRES_NEW), which prevents the newly created SupportTicket in SupportTicketServiceImpl.createTicket from being visible and causes findById(dto.getSupportTicketId()) to throw; change createActivityLog to run in the caller's transaction by removing Propagation.REQUIRES_NEW (use the default `@Transactional` or no annotation) so the uncommitted ticket is visible to supportTicketRepository.findById, or alternatively if you must keep REQUIRES_NEW, defer the logging until after commit via TransactionSynchronizationManager.registerSynchronization(...).afterCommit() or switch callers to pass the already-loaded User/SupportTicket entities into createActivityLog to avoid refetching.
🧹 Nitpick comments (1)
src/test/java/org/group1/projectbackend/service/SupportTicketServiceTest.java (1)
95-113: Good failure-tolerance coverage; consider an integration test for the REQUIRES_NEW interaction.The two new tests correctly assert the "swallow-and-continue" contract of
logActivitySafely. However, sinceactivityLogServiceis mocked here, they will not catch the real interaction issue betweenREQUIRES_NEWand the outer (uncommitted)saveTickettransaction (see the comment onActivityLogService.createActivityLog). An integration test with a realActivityLogServiceand database would surface that — and would also act as a regression guard once that issue is addressed.Also applies to: 155-169
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/org/group1/projectbackend/service/SupportTicketServiceTest.java` around lines 95 - 113, Add an integration test that uses the real ActivityLogService and the DB (not the mocked activityLogService) to exercise the REQUIRES_NEW interaction and catch the transaction ordering issue: in the SupportTicketServiceTest class create a new test (or convert the existing shouldCreateTicketEvenWhenActivityLogFails variant) that boots the Spring context (e.g., `@SpringBootTest` or `@DataJpaTest` with service beans), injects the real ActivityLogService and repositories, calls supportTicketService.createTicket("alice", request) and asserts the SupportTicket is persisted even if ActivityLogService.createActivityLog throws or rolls back; this ensures the logActivitySafely/ActivityLogService.createActivityLog REQUIRES_NEW behavior is validated against the actual DB transaction boundaries and will act as a regression guard.
🤖 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/group1/projectbackend/service/ActivityLogService.java`:
- Around line 38-53: The method ActivityLogService.createActivityLog is
annotated with `@Transactional`(propagation = Propagation.REQUIRES_NEW), which
prevents the newly created SupportTicket in
SupportTicketServiceImpl.createTicket from being visible and causes
findById(dto.getSupportTicketId()) to throw; change createActivityLog to run in
the caller's transaction by removing Propagation.REQUIRES_NEW (use the default
`@Transactional` or no annotation) so the uncommitted ticket is visible to
supportTicketRepository.findById, or alternatively if you must keep
REQUIRES_NEW, defer the logging until after commit via
TransactionSynchronizationManager.registerSynchronization(...).afterCommit() or
switch callers to pass the already-loaded User/SupportTicket entities into
createActivityLog to avoid refetching.
---
Nitpick comments:
In
`@src/test/java/org/group1/projectbackend/service/SupportTicketServiceTest.java`:
- Around line 95-113: Add an integration test that uses the real
ActivityLogService and the DB (not the mocked activityLogService) to exercise
the REQUIRES_NEW interaction and catch the transaction ordering issue: in the
SupportTicketServiceTest class create a new test (or convert the existing
shouldCreateTicketEvenWhenActivityLogFails variant) that boots the Spring
context (e.g., `@SpringBootTest` or `@DataJpaTest` with service beans), injects the
real ActivityLogService and repositories, calls
supportTicketService.createTicket("alice", request) and asserts the
SupportTicket is persisted even if ActivityLogService.createActivityLog throws
or rolls back; this ensures the
logActivitySafely/ActivityLogService.createActivityLog REQUIRES_NEW behavior is
validated against the actual DB transaction boundaries and will act as a
regression guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0358dfb0-0e1d-4cdc-af11-710d51f740a0
📒 Files selected for processing (6)
src/main/java/org/group1/projectbackend/service/ActivityLogService.javasrc/main/java/org/group1/projectbackend/service/CommentService.javasrc/main/java/org/group1/projectbackend/service/impl/DocumentServiceImpl.javasrc/main/java/org/group1/projectbackend/service/impl/SupportTicketServiceImpl.javasrc/test/java/org/group1/projectbackend/service/DocumentServiceTest.javasrc/test/java/org/group1/projectbackend/service/SupportTicketServiceTest.java
1 participant
Summary by CodeRabbit
Bug Fixes
Tests