Feature/protocol pdf export#65
Merged
Merged
Conversation
… ProtocolArchiveController and Service
… ProtocolArchiveController and Service
…is already created.
Contributor
|
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 (3)
📝 WalkthroughWalkthroughAdds protocol PDF generation and archival (including viewer-aware paragraph filtering and archival CaseFile linking), a new upload API for generated files, repository locking and guards to prevent edits on archived protocols/meetings with protocols, new exceptions and handlers, UI fragments for case-officer/protocol dashboards, PDFBox dependency, migrations, and tests. Changes
Sequence DiagramsequenceDiagram
actor Admin
participant ProtocolArchiveController
participant ProtocolArchiveService
participant ProtocolPdfService
participant CaseRecordService
participant CaseFileService
participant S3Service
participant Database
Admin->>ProtocolArchiveController: POST /admin/protocols/{id}/archive-pdf
ProtocolArchiveController->>ProtocolArchiveService: archiveProtocolPdf(protocolId, user)
ProtocolArchiveService->>Database: findWithLockById(protocolId)
Database-->>ProtocolArchiveService: Protocol (locked)
alt Protocol already archived
ProtocolArchiveService-->>ProtocolArchiveController: return existing CaseFile
else Protocol not ready
ProtocolArchiveService-->>ProtocolArchiveController: throw ProtocolNotReadyForPdfException
else Ready for PDF
ProtocolArchiveService->>CaseRecordService: findOrCreateAnnualProtocolCase(registry, year, user)
CaseRecordService-->>ProtocolArchiveService: CaseRecord
ProtocolArchiveService->>ProtocolPdfService: generatePdf(protocolId, adminUser)
ProtocolPdfService->>Database: getProtocol with paragraphs
Database-->>ProtocolPdfService: Protocol + paragraphs
ProtocolPdfService-->>ProtocolArchiveService: byte[] (PDF bytes)
ProtocolArchiveService->>CaseFileService: uploadGeneratedFile(caseRecord, filename, bytes, CONFIDENTIAL/OPEN)
CaseFileService->>S3Service: uploadFileIfAbsent(s3Key, bytes)
S3Service-->>CaseFileService: success
CaseFileService->>Database: save(CaseFile)
Database-->>CaseFileService: CaseFile (persisted)
CaseFileService-->>ProtocolArchiveService: CaseFile
ProtocolArchiveService->>Database: protocol.setArchivedPdfFile(caseFile)
ProtocolArchiveService->>Database: save(protocol)
Database-->>ProtocolArchiveService: Protocol (updated)
ProtocolArchiveService-->>ProtocolArchiveController: CaseFile
end
ProtocolArchiveController-->>Admin: return fragment with success message
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (1)
src/main/resources/templates/fragments/admin-protocols.html (1)
140-145: Drive the empty state fromparagraphViewstoo.The loop now renders
paragraphViews, but the empty-state message still checksselectedProtocol.paragraphs. That split source of truth can produce stale UI if the view model ever changes.♻️ Suggested tweak
- <div th:if="${`#lists.isEmpty`(selectedProtocol.paragraphs)}"> + <div th:if="${`#lists.isEmpty`(paragraphViews)}">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/fragments/admin-protocols.html` around lines 140 - 145, The empty-state check currently uses selectedProtocol.paragraphs while the list rendering uses paragraphViews; change the empty-state condition to use paragraphViews (i.e., use `#lists.isEmpty`(paragraphViews)) so the message is driven from the same view model as the th:each that iterates paragraphViews; update the th:if on the div that shows "Protokollet saknar paragrafer." to reference paragraphViews instead of selectedProtocol.paragraphs to avoid a split source of truth.
🤖 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/backendlab/team4you/casefile/CaseFileService.java`:
- Around line 209-247: The pessimistic DB lock from findByIdWithLock is held
across the S3 call (s3Service.uploadFileIfAbsent), causing unnecessary
contention; change the flow so you acquire the lock only to call
allocateNextDocumentNumber (and persist the increment), compute
documentReference and s3Key (using buildUniqueS3Key), then commit/close that
transaction to release the lock before performing s3Service.uploadFileIfAbsent;
after the upload, start a new transaction, reload the CaseRecord without a lock
(or attach by id), construct the CaseFile with the reserved document number and
save via caseFileRepository.saveAndFlush—this ensures the lock is not held
during the potentially slow S3 upload while still reserving document numbers
safely.
- Around line 216-218: Add a null-guard for the byte[] before checking its
length: in CaseFileService (the method that currently does "if (bytes.length >
MAX_FILE_SIZE_BYTES) { throw new FileTooLargeException(MAX_FILE_SIZE_BYTES); }")
first check "if (bytes == null)" and throw a clear validation exception (e.g.,
IllegalArgumentException or your project's validation exception type) with a
message like "file bytes must not be null", then proceed to the existing length
check using MAX_FILE_SIZE_BYTES and throw FileTooLargeException as before.
In `@src/main/java/backendlab/team4you/caserecord/CaseRecordRepository.java`:
- Around line 24-25: The query in CaseRecordRepository.findByAssignedUserId is
non-deterministic with unsorted PageRequests; modify the `@Query` to include a
stable ORDER BY (e.g., ORDER BY c.id or ORDER BY c.createdAt, depending on the
intended canonical ordering) so paging is deterministic, or alternatively
validate/require the incoming Pageable to include a stable Sort and throw/handle
if absent; update the `@Query` annotation for findByAssignedUserId (or add
repository-level validation) to enforce the chosen deterministic ordering.
In `@src/main/java/backendlab/team4you/caserecord/CaseRecordService.java`:
- Around line 147-158: The current findOrCreateAnnualProtocolCase uses
registry.getName() for the lookup and is not atomic; change the lookup to use an
immutable identifier (registry.getId()) by adding/using a repository method like
findByRegistryIdAndYear (or findByRegistryAndYear) instead of
findByRegistryAndTitle, and make creation atomic by adding a DB uniqueness
constraint/unique index on (registry_id, year) and handling concurrent inserts:
keep `@Transactional` on findOrCreateAnnualProtocolCase/createAnnualProtocolCase,
attempt to save the new CaseRecord, and if a DataIntegrityViolationException (or
unique constraint violation) occurs, catch it and re-query using the stable key
to return the existing row; alternatively use a pessimistic lock on the registry
record before checking/creating to serialize concurrent creators.
In `@src/main/java/backendlab/team4you/config/SecurityConfig.java`:
- Around line 46-50: Add explicit role protection for the bare "/case-officer"
path by adding a requestMatchers entry for "/case-officer" that calls
hasRole(CASE_OFFICER) before the existing requestMatchers("/case-officer/**").
Locate the authorization chain in SecurityConfig where
requestMatchers("/case-officer/**").hasRole(CASE_OFFICER) is declared and insert
.requestMatchers("/case-officer").hasRole(CASE_OFFICER) immediately before it so
the standalone endpoint is restricted to the CASE_OFFICER role.
In `@src/main/java/backendlab/team4you/controller/CaseOfficerController.java`:
- Around line 62-71: In closeCase, validate and parse the incoming id safely and
guard against a null assigned user: wrap Long.parseLong(id) in a try/catch and
throw a ResponseStatusException(HttpStatus.BAD_REQUEST, "Invalid case id") on
NumberFormatException instead of letting it propagate; after fetching caseRecord
from caseRecordRepository in closeCase, check if caseRecord.getAssignedUser() is
null and throw a ResponseStatusException(HttpStatus.FORBIDDEN, "Case has no
assigned officer") (or another appropriate 4xx) before calling getId(); keep
existing userRepository lookup and the assigned-user-id equality check but
perform these new checks first to avoid NPEs and 500s.
In
`@src/main/java/backendlab/team4you/exceptions/ProtocolNotReadyForPdfException.java`:
- Around line 6-8: The exception message in ProtocolNotReadyForPdfException(Long
protocolId) is misleading because Protocol.isReadyForPdf() requires both a
decision and non-blank decision text; update the super(...) message to reflect
both requirements (e.g., "Alla paragrafer måste ha beslut och beslutstext innan
PDF kan skapas.") so users know missing decision text is also a cause; keep the
constructor signature and this.protocolId assignment unchanged.
In `@src/main/java/backendlab/team4you/meeting/MeetingService.java`:
- Around line 396-399: ensureMeetingHasNoProtocol currently calls
protocolRepository.existsByMeetingId(meetingId) without validating meetingId;
add a null check at the top of ensureMeetingHasNoProtocol and throw the same
domain validation error used elsewhere for missing meeting id (e.g. the existing
MeetingValidationException or IllegalArgumentException) when meetingId is null
so repository layer is not invoked; keep the rest of the logic
(protocolRepository.existsByMeetingId and MeetingHasProtocolException)
unchanged.
In `@src/main/java/backendlab/team4you/protocol/ProtocolArchiveService.java`:
- Around line 39-44: The null-check on Protocol.getArchivedPdfFile() is
non-atomic and can race under concurrency; change the read to an atomic locked
fetch: add a repository method (e.g., ProtocolRepository.findByIdForUpdate(Long
id)) annotated with a pessimistic write lock (or use EntityManager.find with
LockModeType.PESSIMISTIC_WRITE), then in ProtocolArchiveService replace the call
to protocolRepository.findById(...) with the locked fetch (still throwing
ProtocolNotFoundException if missing), perform the getArchivedPdfFile()
null-check only after acquiring the lock, and only then create/upload and save
the archived file to ensure idempotent behavior and avoid duplicate/orphan file
records.
- Around line 60-76: The code is archiving an admin-rendered PDF as
ConfidentialityLevel.OPEN; change upload to use the protocol's actual
confidentiality level instead of OPEN. In ProtocolArchiveService, when calling
protocolPdfService.generatePdf(protocolId, createSystemAdminUser()) and then
caseFileService.uploadGeneratedFile(..., ConfidentialityLevel.OPEN,
currentUser), replace ConfidentialityLevel.OPEN with the correct confidentiality
derived from the protocol (e.g., protocol.getConfidentialityLevel() or a helper
that computes the effective confidentiality for protocol), ensuring the stored
CaseFile uses that confidentiality level rather than OPEN.
In `@src/main/java/backendlab/team4you/protocol/ProtocolController.java`:
- Around line 69-80: Both POST handlers createProtocol and
updateParagraphDecision render the same fragment but omit the paragraphViews
model attribute, so the fragment's ${paragraphViews} is empty after POST; fix by
calling model.addAttribute("paragraphViews",
protocolViewService.getParagraphsForViewer(protocolId, currentUser)) in both
createProtocol(...) and updateParagraphDecision(...) before returning the
fragment (use the same currentUser retrieval as in viewProtocol and the
protocolId used in the response).
In `@src/main/java/backendlab/team4you/protocol/ProtocolPdfController.java`:
- Around line 31-43: The downloadPdf endpoint currently regenerates the PDF with
the caller’s permissions by calling protocolPdfService.generatePdf(protocolId,
currentUser); update it to serve the same bytes as the archive flow instead:
either return the archived file from ProtocolArchiveService (e.g., use a method
like ProtocolArchiveService.getArchivedPdf(protocolId) if available) or
regenerate using the same fixed system-admin user that ProtocolArchiveService
uses (obtain the system/admin user via the existing user service or archive
service helper and call protocolPdfService.generatePdf(protocolId,
systemAdminUser)), and then return those bytes from downloadPdf so the
downloaded PDF matches the archived copy.
In `@src/main/java/backendlab/team4you/protocol/ProtocolPdfService.java`:
- Around line 116-120: In splitText, avoid adding an empty string when the first
word exceeds maxCharsPerLine by only pushing currentLine to lines when it
contains content; modify the branch in ProtocolPdfService.splitText that
currently does "if (currentLine.length() + word.length() + 1 > maxCharsPerLine)
{ lines.add(currentLine.toString()); currentLine = new StringBuilder(word); }"
so that you check currentLine.length() > 0 before calling lines.add(...), and
otherwise just set currentLine = new StringBuilder(word); this ensures long
first words don't produce an empty line while preserving existing wrapping logic
for subsequent words.
In `@src/main/java/backendlab/team4you/protocol/ProtocolService.java`:
- Around line 104-112: The check for archived status on Protocol is racy; before
calling paragraph.updateDecision(decisionType, decisionText) you must re-read
the Protocol under a database lock or use optimistic locking so another
transaction cannot archive it concurrently. Fix options: (A) perform a locked
read of the Protocol (e.g., using EntityManager.lock(...) or a repository method
that loads the Protocol with PESSIMISTIC_WRITE/PESSIMISTIC_READ) inside the same
transaction, re-check protocol.getArchivedPdfFile() and throw
ProtocolAlreadyArchivedException if set; or (B) add an `@Version` field to the
Protocol entity to enable optimistic locking and handle OptimisticLockException
on commit (retry or surface an error). Apply the chosen change in the
ProtocolService method that contains protocol.getArchivedPdfFile() /
paragraph.updateDecision(...) and ensure the Archived check is performed on the
locked/locked-versioned entity before mutating the paragraph.
In `@src/main/java/backendlab/team4you/Team4youApplication.java`:
- Around line 53-77: The current seed logic only runs when repository.count() ==
0 so new CASE_OFFICER accounts (UserEntity officer1/officer2) aren’t added to
existing DBs; instead, change the seeding to check for each username and only
create if missing (e.g., use repository.existsByUsername(username) or
repository.findByUsername(...) to detect absence) and then create officer1 and
officer2 with the same setPasswordHash, setRole, setEmail and repository.save
calls; update the initialization code around repository.count() to perform
per-username checks for "officer1" and "officer2" and seed only those that don’t
exist.
In `@src/test/java/backendlab/team4you/protocol/ProtocolControllerTest.java`:
- Around line 41-45: The test is missing stubs for UserService and
ProtocolViewService used by the controller; in the ProtocolControllerTest method
stub userService.getCurrentUser(principal) to return your testUser and stub
protocolViewService.getParagraphsForViewer(100L, testUser) to return an empty
list (or desired paragraph list), then assert the model contains the
"paragraphViews" attribute (and its contents) to verify integration; locate the
MockitoBean mocks protocolViewService and userService and add when(...)
returns(...) stubs for getCurrentUser(...) and getParagraphsForViewer(...) and
an assertion that model.get("paragraphViews") matches the stubbed value.
---
Nitpick comments:
In `@src/main/resources/templates/fragments/admin-protocols.html`:
- Around line 140-145: The empty-state check currently uses
selectedProtocol.paragraphs while the list rendering uses paragraphViews; change
the empty-state condition to use paragraphViews (i.e., use
`#lists.isEmpty`(paragraphViews)) so the message is driven from the same view
model as the th:each that iterates paragraphViews; update the th:if on the div
that shows "Protokollet saknar paragrafer." to reference paragraphViews instead
of selectedProtocol.paragraphs to avoid a split source of truth.
🪄 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: 9039d608-1994-4eee-8496-9bfc90acb9e4
📒 Files selected for processing (51)
.github/workflows/ci.ymlpom.xmlsrc/main/java/backendlab/team4you/Team4youApplication.javasrc/main/java/backendlab/team4you/casefile/CaseFileService.javasrc/main/java/backendlab/team4you/casefile/access/CaseFileAccessService.javasrc/main/java/backendlab/team4you/caserecord/CaseRecordRepository.javasrc/main/java/backendlab/team4you/caserecord/CaseRecordService.javasrc/main/java/backendlab/team4you/config/CustomAuthenticationSuccessHandler.javasrc/main/java/backendlab/team4you/config/SecurityConfig.javasrc/main/java/backendlab/team4you/controller/AdminController.javasrc/main/java/backendlab/team4you/controller/CaseOfficerController.javasrc/main/java/backendlab/team4you/exceptions/CaseRecordNotFoundException.javasrc/main/java/backendlab/team4you/exceptions/GlobalRestExceptionHandler.javasrc/main/java/backendlab/team4you/exceptions/GlobalViewExceptionHandler.javasrc/main/java/backendlab/team4you/exceptions/MeetingHasProtocolException.javasrc/main/java/backendlab/team4you/exceptions/ProtocolAlreadyArchivedException.javasrc/main/java/backendlab/team4you/exceptions/ProtocolNotReadyForPdfException.javasrc/main/java/backendlab/team4you/meeting/MeetingService.javasrc/main/java/backendlab/team4you/protocol/DashboardProtocolController.javasrc/main/java/backendlab/team4you/protocol/Protocol.javasrc/main/java/backendlab/team4you/protocol/ProtocolArchiveController.javasrc/main/java/backendlab/team4you/protocol/ProtocolArchiveService.javasrc/main/java/backendlab/team4you/protocol/ProtocolController.javasrc/main/java/backendlab/team4you/protocol/ProtocolParagraph.javasrc/main/java/backendlab/team4you/protocol/ProtocolParagraphViewDto.javasrc/main/java/backendlab/team4you/protocol/ProtocolPdfController.javasrc/main/java/backendlab/team4you/protocol/ProtocolPdfService.javasrc/main/java/backendlab/team4you/protocol/ProtocolService.javasrc/main/java/backendlab/team4you/protocol/ProtocolViewController.javasrc/main/java/backendlab/team4you/protocol/ProtocolViewService.javasrc/main/java/backendlab/team4you/user/UserRepository.javasrc/main/java/backendlab/team4you/user/UserRole.javasrc/main/resources/db/migration/V24__archived_pdf_file_id.sqlsrc/main/resources/templates/case-officer-layout.htmlsrc/main/resources/templates/case-officer.htmlsrc/main/resources/templates/dashboard-layout.htmlsrc/main/resources/templates/fragments/admin-protocols.htmlsrc/main/resources/templates/fragments/case-officer-cases.htmlsrc/main/resources/templates/fragments/case-officer-sidenav.htmlsrc/main/resources/templates/fragments/dashboard-protocols.htmlsrc/main/resources/templates/fragments/navbar.htmlsrc/main/resources/templates/fragments/side-navbar.htmlsrc/main/resources/templates/protocol-view.htmlsrc/test/java/backendlab/team4you/casefile/CaseFileServiceTest.javasrc/test/java/backendlab/team4you/meeting/MeetingControllerTest.javasrc/test/java/backendlab/team4you/meeting/MeetingServiceTest.javasrc/test/java/backendlab/team4you/protocol/ProtocolArchiveServiceTest.javasrc/test/java/backendlab/team4you/protocol/ProtocolControllerTest.javasrc/test/java/backendlab/team4you/protocol/ProtocolPdfServiceTest.javasrc/test/java/backendlab/team4you/protocol/ProtocolServiceTest.javasrc/test/java/backendlab/team4you/protocol/ProtocolViewServiceTest.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
src/main/java/backendlab/team4you/Team4youApplication.java (1)
80-80:⚠️ Potential issue | 🟡 MinorMatch the existence check to the field being queried.
seedUser(...)persistsusernameanddisplayNameseparately, but this guard callsexistsByName(username). That happens to work fordevandadmin, but not forofficer1/officer2, where the saved name iscaseofficer1/caseofficer2. In practice, those users can be re-seeded on every startup or trip a uniqueness constraint.Suggested fix
- if (repository.existsByName(username)) { + if (repository.existsByName(displayName)) { return; }If
usernameis the real unique identifier, addingexistsByUsername(...)would be even clearer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/Team4youApplication.java` at line 80, The startup guard is checking repository.existsByName(username) while seedUser(...) persists username and displayName separately, causing a mismatch for users like "caseofficer1"; change the existence check to match the actual unique identifier (e.g., call repository.existsByUsername(username) instead of existsByName(username)) and, if not present, add an existsByUsername(String username) method to the repository interface; ensure the seedUser call in Team4youApplication uses the same field (username) for both saving and checking to avoid reseeding duplicates.src/main/java/backendlab/team4you/casefile/CaseFileService.java (1)
209-250:⚠️ Potential issue | 🟠 MajorDon't hold the case lock through the S3 upload.
findByIdWithLock(...)keeps a pessimistic lock on the case record whileuploadFileIfAbsent(...)runs. A slow upload will block every other document operation on that case much longer than needed. Reserve the document number under lock, release that transaction, then perform the S3 upload and persist theCaseFilein a follow-up transaction.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/casefile/CaseFileService.java` around lines 209 - 250, The code currently holds the pessimistic lock (findByIdWithLock) while performing the S3 upload (s3Service.uploadFileIfAbsent), which can block other operations; refactor so you reserve the document number under the lock and immediately release the transaction (use allocateNextDocumentNumber while inside the locked transaction), then outside that transaction perform s3Service.uploadFileIfAbsent(s3Key, bytes, normalizedContentType) and only after a successful upload start a new transaction to create and persist the CaseFile via caseFileRepository.saveAndFlush; keep identifiers findByIdWithLock, allocateNextDocumentNumber, buildUniqueS3Key, s3Service.uploadFileIfAbsent and caseFileRepository.saveAndFlush to locate where to split transactional boundaries.src/main/java/backendlab/team4you/caserecord/CaseRecordService.java (1)
153-157:⚠️ Potential issue | 🟠 MajorMake the annual protocol case lookup/create atomic on a stable key.
This still depends on a formatted title and a non-atomic
find...orElseGet(...). Two concurrent archive requests can both miss the row and create duplicates, and a registry code change would also break the lookup for an already-created annual case. Please switch this flow to a stable(registryId, year)lookup backed by a DB uniqueness constraint, then handle the insert race by re-reading on conflict.Also applies to: 175-195
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/caserecord/CaseRecordService.java` around lines 153 - 157, Change the annual protocol lookup/creation in CaseRecordService to use a stable key (registry id + year) instead of the formatted title: add/find by (registry.getId(), year) via caseRecordRepository (introduce a repository method like findByRegistryIdAndYear), add a DB uniqueness constraint on (registry_id, year), and modify createAnnualProtocolCase flow to attempt insert and, on unique-key conflict, re-query by (registryId, year) and return the existing row; ensure all references that previously used title-based lookup (including the code around createAnnualProtocolCase) are updated to use the stable lookup and re-read-on-conflict semantics to avoid duplicate creations/races.
🧹 Nitpick comments (3)
src/test/java/backendlab/team4you/protocol/ProtocolArchiveServiceTest.java (2)
160-167:setFieldhelper is brittle for inherited fields and hard to debug on failure.Line 162 uses
getDeclaredField, which fails ifidmoves to a superclass. Consider a small hierarchy-aware lookup and a more specific exception message.Diff suggestion
@@ private void setField(Object target, String fieldName, Object value) { try { - Field field = target.getClass().getDeclaredField(fieldName); + Field field = findField(target.getClass(), fieldName); + if (field == null) { + throw new NoSuchFieldException("Field '" + fieldName + "' not found on " + target.getClass().getName()); + } field.setAccessible(true); field.set(target, value); - } catch (Exception exception) { - throw new RuntimeException(exception); + } catch (ReflectiveOperationException exception) { + throw new IllegalStateException("Failed to set field '" + fieldName + "'", exception); } } + + private Field findField(Class<?> type, String fieldName) { + Class<?> current = type; + while (current != null) { + try { + return current.getDeclaredField(fieldName); + } catch (NoSuchFieldException ignored) { + current = current.getSuperclass(); + } + } + return null; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/backendlab/team4you/protocol/ProtocolArchiveServiceTest.java` around lines 160 - 167, The setField helper uses target.getClass().getDeclaredField(fieldName) which fails for inherited fields; change setField to walk the class hierarchy (loop on Class<?> c = target.getClass(); c != null; c = c.getSuperclass()) and call getDeclaredField on each until found, then setAccessible(true) and set the value; also catch NoSuchFieldException separately and throw a clearer exception (e.g., new IllegalStateException("Field '"+fieldName+"' not found on "+target.getClass(), e)) so failures include the field name and target class for easier debugging while keeping other exceptions wrapped as before.
77-84: Add fail-fast interaction assertions in exception-path tests.Line 77 and Line 103 assert exception type, but they do not assert that downstream collaborators stayed untouched. Adding that check makes these tests stricter against side effects.
Diff suggestion
@@ void archiveProtocolPdf_shouldThrow_whenProtocolNotFound() { when(protocolRepository.findWithLockById(1L)).thenReturn(Optional.empty()); assertThatThrownBy(() -> protocolArchiveService.archiveProtocolPdf(1L, currentUser)) .isInstanceOf(ProtocolNotFoundException.class); + verifyNoInteractions(protocolPdfService, caseRecordService, caseFileService); + verify(protocolRepository, never()).save(any()); } @@ void archiveProtocolPdf_shouldThrow_whenProtocolNotReady() { when(protocolRepository.findWithLockById(1L)).thenReturn(Optional.of(protocol)); assertThatThrownBy(() -> protocolArchiveService.archiveProtocolPdf(1L, currentUser)) .isInstanceOf(ProtocolNotReadyForPdfException.class); + verifyNoInteractions(protocolPdfService, caseRecordService, caseFileService); + verify(protocolRepository, never()).save(any()); }Also applies to: 103-110
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/backendlab/team4you/protocol/ProtocolArchiveServiceTest.java` around lines 77 - 84, The test archiveProtocolPdf_shouldThrow_whenProtocolNotFound currently only asserts the exception; add fail-fast interaction assertions to ensure no downstream collaborators are touched: after calling protocolArchiveService.archiveProtocolPdf(1L, currentUser) and asserting ProtocolNotFoundException, add verifyNoInteractions/verifyNoMoreInteractions checks against the mocked collaborators used by this test class (e.g., pdfArchiveService, fileStorageService, auditService) and verifyZeroInteractions/verifyNoMoreInteractions(protocolRepository) as appropriate; apply the same pattern to the other exception-path test around lines 103-110 to ensure no side effects.src/main/java/backendlab/team4you/protocol/ProtocolController.java (1)
66-70: Extract repeated model population to a helper to reduce drift.The same list attributes are re-added in three handlers. A shared helper keeps responses consistent when attributes evolve.
Diff suggestion
@@ model.addAttribute( "completedMeetingsWithoutProtocol", meetingRepository.findCompletedMeetingsWithoutProtocol() ); - model.addAttribute("protocols", protocolRepository.findAll()); + model.addAttribute("protocols", protocolRepository.findAll()); return "fragments/admin-protocols :: content"; @@ - model.addAttribute( - "completedMeetingsWithoutProtocol", - meetingRepository.findCompletedMeetingsWithoutProtocol() - ); - model.addAttribute("protocols", protocolRepository.findAll()); + populateProtocolListModel(model); return "fragments/admin-protocols :: content"; @@ - model.addAttribute( - "completedMeetingsWithoutProtocol", - meetingRepository.findCompletedMeetingsWithoutProtocol() - ); - model.addAttribute("protocols", protocolRepository.findAll()); + populateProtocolListModel(model); return "fragments/admin-protocols :: content"; @@ - model.addAttribute( - "completedMeetingsWithoutProtocol", - meetingRepository.findCompletedMeetingsWithoutProtocol() - ); - model.addAttribute("protocols", protocolRepository.findAll()); + populateProtocolListModel(model); return "fragments/admin-protocols :: content"; } + + private void populateProtocolListModel(Model model) { + model.addAttribute( + "completedMeetingsWithoutProtocol", + meetingRepository.findCompletedMeetingsWithoutProtocol() + ); + model.addAttribute("protocols", protocolRepository.findAll()); + }Also applies to: 86-90, 117-121
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/protocol/ProtocolController.java` around lines 66 - 70, The controller repeatedly adds the same model attributes (meetingRepository.findCompletedMeetingsWithoutProtocol() and protocolRepository.findAll()) in multiple handlers; extract that logic into a private helper in ProtocolController (e.g., private void populateModelWithProtocolsAndMeetings(Model model)) that performs model.addAttribute("completedMeetingsWithoutProtocol", meetingRepository.findCompletedMeetingsWithoutProtocol()) and model.addAttribute("protocols", protocolRepository.findAll()), and replace the repeated blocks in each handler method with a single call to populateModelWithProtocolsAndMeetings(model) to keep responses consistent as attributes evolve.
🤖 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/backendlab/team4you/meeting/MeetingService.java`:
- Around line 396-402: The advisory check in ensureMeetingHasNoProtocol(...) is
racy; make the "no protocol" invariant enforced by serializing on the Meeting
row: in createProtocolForCompletedMeeting(...) (or inside
ensureMeetingHasNoProtocol) load the Meeting entity using a DB-level lock (e.g.,
JPA OPTIMISTIC lock via `@Version` on Meeting and validate version, or use a
PESSIMISTIC_WRITE lock via EntityManager.find(...,
LockModeType.PESSIMISTIC_WRITE) or repository method) and then re-check
protocolRepository.existsByMeetingId(meetingId) (or check a joined/derived
relation) before inserting the protocol; reference ensureMeetingHasNoProtocol,
createProtocolForCompletedMeeting, Meeting, and
protocolRepository.existsByMeetingId when making the change.
- Around line 267-270: The protocol guard is invoked after loading the Meeting,
which allows a null meetingId to reach the repository; move the call to
ensureMeetingHasNoProtocol(meetingId) to occur before getMeetingById(meetingId)
in removeAgendaItem so the guard runs first; apply the same reorder to the other
methods showing the same pattern (the other occurrences around lines 298-300 and
325-327) so ensureMeetingHasNoProtocol(...) always runs before
getMeetingById(...).
In `@src/main/java/backendlab/team4you/protocol/ProtocolPdfService.java`:
- Around line 178-185: writeLine currently sets a PDType1Font
(PDType1Font(FontName)) which only supports WinAnsi and will throw on Unicode
characters; change writeLine to use a Unicode-capable font (e.g., load a
TrueType/OTF and set PDType0Font via PDType0Font.load(document, fontStream))
before calling content.showText(text) or implement normalization/fallback to
strip/replace unsupported codepoints; update the content.setFont(...) call and
ensure the loaded PDType0Font instance is reused (cache it) rather than
recreating for every call.
---
Duplicate comments:
In `@src/main/java/backendlab/team4you/casefile/CaseFileService.java`:
- Around line 209-250: The code currently holds the pessimistic lock
(findByIdWithLock) while performing the S3 upload
(s3Service.uploadFileIfAbsent), which can block other operations; refactor so
you reserve the document number under the lock and immediately release the
transaction (use allocateNextDocumentNumber while inside the locked
transaction), then outside that transaction perform
s3Service.uploadFileIfAbsent(s3Key, bytes, normalizedContentType) and only after
a successful upload start a new transaction to create and persist the CaseFile
via caseFileRepository.saveAndFlush; keep identifiers findByIdWithLock,
allocateNextDocumentNumber, buildUniqueS3Key, s3Service.uploadFileIfAbsent and
caseFileRepository.saveAndFlush to locate where to split transactional
boundaries.
In `@src/main/java/backendlab/team4you/caserecord/CaseRecordService.java`:
- Around line 153-157: Change the annual protocol lookup/creation in
CaseRecordService to use a stable key (registry id + year) instead of the
formatted title: add/find by (registry.getId(), year) via caseRecordRepository
(introduce a repository method like findByRegistryIdAndYear), add a DB
uniqueness constraint on (registry_id, year), and modify
createAnnualProtocolCase flow to attempt insert and, on unique-key conflict,
re-query by (registryId, year) and return the existing row; ensure all
references that previously used title-based lookup (including the code around
createAnnualProtocolCase) are updated to use the stable lookup and
re-read-on-conflict semantics to avoid duplicate creations/races.
In `@src/main/java/backendlab/team4you/Team4youApplication.java`:
- Line 80: The startup guard is checking repository.existsByName(username) while
seedUser(...) persists username and displayName separately, causing a mismatch
for users like "caseofficer1"; change the existence check to match the actual
unique identifier (e.g., call repository.existsByUsername(username) instead of
existsByName(username)) and, if not present, add an existsByUsername(String
username) method to the repository interface; ensure the seedUser call in
Team4youApplication uses the same field (username) for both saving and checking
to avoid reseeding duplicates.
---
Nitpick comments:
In `@src/main/java/backendlab/team4you/protocol/ProtocolController.java`:
- Around line 66-70: The controller repeatedly adds the same model attributes
(meetingRepository.findCompletedMeetingsWithoutProtocol() and
protocolRepository.findAll()) in multiple handlers; extract that logic into a
private helper in ProtocolController (e.g., private void
populateModelWithProtocolsAndMeetings(Model model)) that performs
model.addAttribute("completedMeetingsWithoutProtocol",
meetingRepository.findCompletedMeetingsWithoutProtocol()) and
model.addAttribute("protocols", protocolRepository.findAll()), and replace the
repeated blocks in each handler method with a single call to
populateModelWithProtocolsAndMeetings(model) to keep responses consistent as
attributes evolve.
In `@src/test/java/backendlab/team4you/protocol/ProtocolArchiveServiceTest.java`:
- Around line 160-167: The setField helper uses
target.getClass().getDeclaredField(fieldName) which fails for inherited fields;
change setField to walk the class hierarchy (loop on Class<?> c =
target.getClass(); c != null; c = c.getSuperclass()) and call getDeclaredField
on each until found, then setAccessible(true) and set the value; also catch
NoSuchFieldException separately and throw a clearer exception (e.g., new
IllegalStateException("Field '"+fieldName+"' not found on "+target.getClass(),
e)) so failures include the field name and target class for easier debugging
while keeping other exceptions wrapped as before.
- Around line 77-84: The test
archiveProtocolPdf_shouldThrow_whenProtocolNotFound currently only asserts the
exception; add fail-fast interaction assertions to ensure no downstream
collaborators are touched: after calling
protocolArchiveService.archiveProtocolPdf(1L, currentUser) and asserting
ProtocolNotFoundException, add verifyNoInteractions/verifyNoMoreInteractions
checks against the mocked collaborators used by this test class (e.g.,
pdfArchiveService, fileStorageService, auditService) and
verifyZeroInteractions/verifyNoMoreInteractions(protocolRepository) as
appropriate; apply the same pattern to the other exception-path test around
lines 103-110 to ensure no side effects.
🪄 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: 209dfbe5-5db4-471a-b806-d7b51b927e72
📒 Files selected for processing (19)
src/main/java/backendlab/team4you/Team4youApplication.javasrc/main/java/backendlab/team4you/casefile/CaseFileService.javasrc/main/java/backendlab/team4you/caserecord/CaseRecordRepository.javasrc/main/java/backendlab/team4you/caserecord/CaseRecordService.javasrc/main/java/backendlab/team4you/config/SecurityConfig.javasrc/main/java/backendlab/team4you/controller/CaseOfficerController.javasrc/main/java/backendlab/team4you/exceptions/ProtocolNotReadyForPdfException.javasrc/main/java/backendlab/team4you/meeting/MeetingService.javasrc/main/java/backendlab/team4you/protocol/ProtocolArchiveService.javasrc/main/java/backendlab/team4you/protocol/ProtocolController.javasrc/main/java/backendlab/team4you/protocol/ProtocolPdfController.javasrc/main/java/backendlab/team4you/protocol/ProtocolPdfService.javasrc/main/java/backendlab/team4you/protocol/ProtocolRepository.javasrc/main/java/backendlab/team4you/protocol/ProtocolService.javasrc/main/java/backendlab/team4you/user/UserRepository.javasrc/test/java/backendlab/team4you/protocol/ProtocolArchiveServiceTest.javasrc/test/java/backendlab/team4you/protocol/ProtocolControllerTest.javasrc/test/java/backendlab/team4you/protocol/ProtocolPdfServiceTest.javasrc/test/java/backendlab/team4you/protocol/ProtocolServiceTest.java
✅ Files skipped from review due to trivial changes (2)
- src/main/java/backendlab/team4you/config/SecurityConfig.java
- src/main/java/backendlab/team4you/user/UserRepository.java
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/java/backendlab/team4you/exceptions/ProtocolNotReadyForPdfException.java
- src/main/java/backendlab/team4you/protocol/ProtocolPdfController.java
- src/main/java/backendlab/team4you/caserecord/CaseRecordRepository.java
- src/test/java/backendlab/team4you/protocol/ProtocolControllerTest.java
- src/test/java/backendlab/team4you/protocol/ProtocolServiceTest.java
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
src/main/java/backendlab/team4you/protocol/ProtocolPdfService.java (1)
43-51: Avoid redundant protocol fetch in PDF generation path.Line 43 already loads
Protocol, but Line 50 callsProtocolViewService.getParagraphsForViewer(protocolId, viewer), which (persrc/main/java/backendlab/team4you/protocol/ProtocolViewService.java:21-27) fetches the same protocol again. This adds an avoidable DB roundtrip for every PDF export.♻️ Suggested direction
- List<ProtocolParagraphViewDto> paragraphViews = - protocolViewService.getParagraphsForViewer(protocolId, viewer); + List<ProtocolParagraphViewDto> paragraphViews = + protocolViewService.getParagraphsForViewer(protocol, viewer);Then add/adjust an overload in
ProtocolViewServicethat maps from an already loadedProtocol.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/protocol/ProtocolPdfService.java` around lines 43 - 51, Protocol is loaded in ProtocolPdfService via protocolRepository.findById(...) but you still call ProtocolViewService.getParagraphsForViewer(protocolId, viewer) which causes a duplicate DB fetch; add an overload in ProtocolViewService (e.g., getParagraphsForViewer(Protocol protocol, Viewer viewer) or similar) that maps paragraphs from an already-loaded Protocol and update ProtocolPdfService to call that overload instead of passing protocolId; also implement the new overload in ProtocolViewService to reuse the existing mapping logic and remove any protocolRepository lookups inside that path.src/test/java/backendlab/team4you/meeting/MeetingServiceTest.java (1)
167-355: Consider extracting a test helper for repeated lock/protocol stubbing.The same two stubs are repeated across many tests. A small helper (e.g.,
stubUnlockedMeeting(10L)) would reduce duplication and improve readability.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/backendlab/team4you/meeting/MeetingServiceTest.java` around lines 167 - 355, Introduce a small test helper to centralize the repeated stubbing of meeting lock and protocol checks (e.g., a private method stubUnlockedMeeting(Long meetingId) in MeetingServiceTest) that calls meetingRepository.findByIdWithLock(meetingId) -> Optional.of(meeting) and protocolRepository.existsByMeetingId(meetingId) -> false; replace all duplicated when(...) lines in tests (references: meetingRepository.findByIdWithLock and protocolRepository.existsByMeetingId) with a single call to that helper to reduce duplication and improve readability.
🤖 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/backendlab/team4you/meeting/MeetingRepository.java`:
- Around line 32-33: The query method in MeetingRepository uses a named
parameter :id but doesn't explicitly bind it; update the method signature for
findByIdWithLock in the MeetingRepository interface to add an explicit
`@Param`("id") on the Long id parameter so the JPQL :id is bound reliably (i.e.,
annotate the id parameter of findByIdWithLock with `@Param`("id")).
In `@src/main/java/backendlab/team4you/protocol/ProtocolPdfService.java`:
- Around line 123-126: The current wrapping logic in ProtocolPdfService only
breaks on whitespace (it uses currentLine, word and maxCharsPerLine) so very
long single tokens (URLs/identifiers) can overflow; modify the wrapping routine
to detect when a token's length > maxCharsPerLine and split that token into
substrings/chunks of at most maxCharsPerLine (appending the currentLine if
non-empty before splitting), adding each chunk to lines (or starting a new
currentLine with the remainder) so no produced line exceeds maxCharsPerLine;
ensure you update handling around lines.add(currentLine.toString()) and
currentLine = new StringBuilder(word) to use the chunking logic for oversized
words.
- Around line 195-203: The page-break check in writeLine incorrectly only
compares currentY to BOTTOM_MARGIN, which can allow the next line (fontSize + 6
height) to overflow; update the check in writeLine(String text, boolean bold,
float fontSize) to reserve space for the upcoming line by calling addNewPage()
when currentY - (fontSize + 6) <= BOTTOM_MARGIN (instead of currentY <=
BOTTOM_MARGIN), keeping the rest of the method (setting font, showText,
newLineAtOffset, and decrementing currentY) unchanged and ensuring addNewPage
positions content/cursor as the fresh top of a page.
In `@src/test/java/backendlab/team4you/meeting/MeetingServiceTest.java`:
- Around line 376-391: The test
updateMeeting_shouldThrowMeetingHasProtocolException_whenMeetingHasProtocol uses
the wrong expected exception and lacks stubbing; change the assertion to expect
MeetingHasProtocolException (isInstanceOf(MeetingHasProtocolException.class))
and stub the locked lookup to return a meeting that has a protocol (mock meeting
with protocol present) via meetingRepository.findByIdWithLock(...) returning
Optional.of(mockMeetingWithProtocol); keep verifying
meetingRepository.findByIdWithLock(10L) was called and that
meetingRepository.save(...) was never invoked to ensure the protocol guard
triggers.
---
Nitpick comments:
In `@src/main/java/backendlab/team4you/protocol/ProtocolPdfService.java`:
- Around line 43-51: Protocol is loaded in ProtocolPdfService via
protocolRepository.findById(...) but you still call
ProtocolViewService.getParagraphsForViewer(protocolId, viewer) which causes a
duplicate DB fetch; add an overload in ProtocolViewService (e.g.,
getParagraphsForViewer(Protocol protocol, Viewer viewer) or similar) that maps
paragraphs from an already-loaded Protocol and update ProtocolPdfService to call
that overload instead of passing protocolId; also implement the new overload in
ProtocolViewService to reuse the existing mapping logic and remove any
protocolRepository lookups inside that path.
In `@src/test/java/backendlab/team4you/meeting/MeetingServiceTest.java`:
- Around line 167-355: Introduce a small test helper to centralize the repeated
stubbing of meeting lock and protocol checks (e.g., a private method
stubUnlockedMeeting(Long meetingId) in MeetingServiceTest) that calls
meetingRepository.findByIdWithLock(meetingId) -> Optional.of(meeting) and
protocolRepository.existsByMeetingId(meetingId) -> false; replace all duplicated
when(...) lines in tests (references: meetingRepository.findByIdWithLock and
protocolRepository.existsByMeetingId) with a single call to that helper to
reduce duplication and improve readability.
🪄 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: 5d513162-bae8-408c-9593-d988f4a60285
⛔ Files ignored due to path filters (2)
src/main/resources/fonts/NotoSans-Bold.ttfis excluded by!**/*.ttfsrc/main/resources/fonts/NotoSans-Regular.ttfis excluded by!**/*.ttf
📒 Files selected for processing (5)
src/main/java/backendlab/team4you/Team4youApplication.javasrc/main/java/backendlab/team4you/meeting/MeetingRepository.javasrc/main/java/backendlab/team4you/meeting/MeetingService.javasrc/main/java/backendlab/team4you/protocol/ProtocolPdfService.javasrc/test/java/backendlab/team4you/meeting/MeetingServiceTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/backendlab/team4you/Team4youApplication.java
JohanHiths
approved these changes
Apr 28, 2026
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
2 participants
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
Security & Access Control
Bug Fixes