Feature/meeting protocol generation - #62
Merged
Merged
Conversation
…phNotFoundException
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 (1)
📝 WalkthroughWalkthroughThis PR introduces protocol management functionality to the backend system. It adds entities for storing protocol records and their decision paragraphs, creates service and repository layers for protocol operations, adds a new controller endpoint for managing protocols, and updates the UI with templates and styling. Exception handling is expanded to support protocol-specific errors, and database migrations establish the schema. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ProtocolController
participant ProtocolService
participant MeetingRepository
participant SequenceRepository
participant ProtocolRepository
participant Database
rect rgba(100, 150, 255, 0.5)
Note over Client,Database: Protocol Creation Flow
Client->>ProtocolController: POST /admin/protocols/meetings/{meetingId}
ProtocolController->>ProtocolService: createProtocolForCompletedMeeting(meetingId)
ProtocolService->>MeetingRepository: findById(meetingId)
MeetingRepository->>Database: Query Meeting
Database-->>MeetingRepository: Meeting record
MeetingRepository-->>ProtocolService: Meeting entity
alt Meeting is COMPLETED
ProtocolService->>ProtocolRepository: existsByMeetingId(meetingId)
ProtocolRepository->>Database: Check existing protocol
Database-->>ProtocolRepository: false
ProtocolRepository-->>ProtocolService: false
loop For each MeetingAgendaItem
ProtocolService->>SequenceRepository: findByRegistryIdAndYear (with lock)
SequenceRepository->>Database: Query with PESSIMISTIC_WRITE lock
Database-->>SequenceRepository: ProtocolParagraphSequence
SequenceRepository-->>ProtocolService: Sequence (locked)
ProtocolService->>SequenceRepository: increment() & flush
SequenceRepository->>Database: UPDATE last_value
Database-->>SequenceRepository: Success
ProtocolService->>ProtocolService: Create ProtocolParagraph
end
ProtocolService->>ProtocolRepository: save(protocol)
ProtocolRepository->>Database: INSERT protocol + paragraphs
Database-->>ProtocolRepository: Protocol created
ProtocolRepository-->>ProtocolService: Protocol entity
ProtocolService-->>ProtocolController: Protocol
else Meeting not COMPLETED
ProtocolService-->>ProtocolController: throws InvalidMeetingStateException
end
ProtocolController->>Client: Return admin-protocols fragment
end
rect rgba(150, 200, 100, 0.5)
Note over Client,Database: Decision Update Flow
Client->>ProtocolController: POST /admin/protocols/paragraphs/{paragraphId}/decision
ProtocolController->>ProtocolService: updateParagraphDecision(paragraphId, decisionType, decisionText)
ProtocolService->>Database: Find ProtocolParagraph by id
Database-->>ProtocolService: Paragraph entity
ProtocolService->>ProtocolService: updateDecision(type, text)
ProtocolService->>ProtocolRepository: save(protocol)
ProtocolRepository->>Database: UPDATE paragraph decision fields
Database-->>ProtocolRepository: Success
ProtocolRepository-->>ProtocolService: Updated protocol
ProtocolService-->>ProtocolController: Protocol
ProtocolController->>Client: Return updated admin-protocols fragment
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 16
🧹 Nitpick comments (20)
docker-compose.yaml (1)
23-32: Ephemeral LocalStack storage may desync with persistent Postgres data.Removing
localstack_datamakes S3 objects ephemeral whilepostgres_dataremains persistent. After alocalstackrestart, any DB rows referencing uploaded case files (e.g., the protocol/case-file entities introduced in this PR) will point to objects that no longer exist in the bucket, which can produce confusing 404s/NoSuchKey errors during local development and broken assertions in manual testing flows. Theinit-localstack.shscript will recreate the bucket idempotently, but it does not repopulate objects.If this is intentional (e.g., to force a clean S3 state per session), consider also resetting
postgres_datatogether, or document the expectation in the README so contributors don't chase phantom bugs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker-compose.yaml` around lines 23 - 32, The docker-compose change removed persistent storage for LocalStack, which causes S3 objects to vanish while postgres_data remains persistent and leads to dangling DB references; restore a named volume for LocalStack (e.g., add back localstack_data and mount it at LocalStack's data directory such as /tmp/localstack or the container volume used by the service) in the docker-compose service definition and declare the localstack_data volume alongside postgres_data, or alternatively update README and/or init-localstack.sh to document or explicitly reset postgres_data on LocalStack resets; reference the docker-compose service that uses init-localstack.sh, the init-localstack.sh script, and the postgres_data/localstack_data volume names when making the change.src/main/resources/db/migration/V20__make_meeting_agenda_order_constraint_deferrable.sql (1)
1-7: Constraint is deferrable but never actually deferred, and swap logic relies on undocumented use of 0 as sentinel value.The migration changes the constraint to
DEFERRABLE INITIALLY IMMEDIATE, but the swap implementation inMeetingService.moveAgendaItemUp()andmoveAgendaItemDown()parksagendaOrder = 0as a transient placeholder between two items, and there are noSET CONSTRAINTS ... DEFERREDcalls anywhere in the codebase. So the deferrable property has no runtime effect.The current approach works because:
- The 0-sentinel avoids constraint violations at each
saveAndFlush()step- No
CHECKconstraint preventsagendaOrder = 0in the entity or migration- The schema has only
NOT NULLon the columnHowever, two risks remain:
- No prevention of 0 in production data: If any agenda item is created with
agendaOrder = 0(via direct insert, data seeding, or concurrent operation), the swap will fail at the first flush when the sentinel collides.- No actual use of deferrable property: If concurrent swaps target the same meeting, INITIALLY IMMEDIATE means each flush checks immediately. The 0-sentinel pattern only works because of strict serialization via
saveAndFlush(). A single deferred transaction would be cleaner.Consider adding a
CHECK (agenda_order > 0)constraint to formalize the sentinel assumption, or refactor swaps to useSET CONSTRAINTS uk_meeting_agenda_item_meeting_order DEFERREDwith a true transactional swap if concurrency becomes a concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/db/migration/V20__make_meeting_agenda_order_constraint_deferrable.sql` around lines 1 - 7, The migration makes uk_meeting_agenda_item_meeting_order deferrable but the code never defers it and relies on agendaOrder=0 sentinel; update the migration to also add a CHECK (agenda_order > 0) constraint to prevent real rows from using 0 OR change the unique constraint to DEFERRABLE INITIALLY DEFERRED and/or add SQL to defer the constraint at swap time; then adjust MeetingService.moveAgendaItemUp() and moveAgendaItemDown() to perform swaps inside a single transaction and issue SET CONSTRAINTS uk_meeting_agenda_item_meeting_order DEFERRED before any save/flush (or stop using the 0-sentinel and use a true transactional three-way swap), referencing the constraint name uk_meeting_agenda_item_meeting_order and the methods MeetingService.moveAgendaItemUp / moveAgendaItemDown and the agenda_order column.src/main/resources/templates/fragments/admin-sidenav.html (1)
39-47: Use a different icon for "Sammanträden".
fa-calendar-daysis already used by "Bokningar" on line 33, so the two sidebar entries are visually indistinguishable. Consider something likefa-users/fa-people-group/fa-gavelto differentiate meetings from bookings.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/fragments/admin-sidenav.html` around lines 39 - 47, The sidebar entry for the "Sammanträden" link (the <a> with th:href="@{/admin/meetings}" / hx-get="/admin/meetings" and the <span>Sammanträden</span>) uses the same icon class fa-calendar-days as the "Bokningar" entry; update the <i> element's class to a distinct icon such as fa-users, fa-people-group, or fa-gavel (e.g., replace fa-calendar-days with one of those) so the meetings item is visually distinguishable from bookings.src/main/resources/db/migration/V21__create_protocol_table.sql (1)
18-30: Add a uniqueness constraint on(protocol_id, paragraph_number).Paragraph numbers are intended to be unique within a protocol (managed via
protocol_paragraph_sequence), but the schema does not enforce this. Any bug in the sequence logic, race, or manual insert could silently produce duplicate paragraph numbers in the same protocol, which would be hard to recover from. Defense-in-depth at the DB level is cheap here.🛡️ Proposed addition
create table protocol_paragraph ( id bigint generated by default as identity primary key, protocol_id bigint not null, case_record_id bigint not null, paragraph_number bigint not null, heading varchar(255) not null, constraint fk_protocol_paragraph_protocol foreign key (protocol_id) references protocol(id), constraint fk_protocol_paragraph_case_record - foreign key (case_record_id) references case_record(id) + foreign key (case_record_id) references case_record(id), + + constraint uk_protocol_paragraph_protocol_number + unique (protocol_id, paragraph_number) );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/db/migration/V21__create_protocol_table.sql` around lines 18 - 30, Add a uniqueness constraint to the protocol_paragraph table to enforce that (protocol_id, paragraph_number) is unique; update the CREATE TABLE for protocol_paragraph (or issue an ALTER TABLE if you prefer migration style) to add a UNIQUE constraint named something like uq_protocol_paragraph_protocol_paragraph_number on the column pair (protocol_id, paragraph_number) so duplicates within the same protocol cannot be inserted; reference the table name protocol_paragraph and the existing columns protocol_id and paragraph_number when making the change.src/main/java/backendlab/team4you/exceptions/InvalidMeetingStateException.java (1)
3-7: Consider tightening the semantics ofInvalidMeetingStateException.The class itself is fine, but cross-file usage in
MeetingService(snippets at lines 49-65, 93-111, 340-347) throws this for plain input validation — blank title, missing registry id,endsAt.isBefore(startsAt), missing diarium, etc. Those are argument/validation errors, not meeting-lifecycle/state errors.Mixing both under one type:
- makes the name misleading at the throw sites,
- prevents future divergence in HTTP mapping (e.g. 422 for validation vs 400/409 for state transitions),
- and overlaps with
IllegalArgumentException, which is already mapped to 400 inGlobalRestExceptionHandler.Recommend reserving
InvalidMeetingStateExceptionfor genuine state-machine violations (e.g.ProtocolService's "Only completed meetings can have protocols") and usingIllegalArgumentException(or a dedicatedMeetingValidationException) for input validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/exceptions/InvalidMeetingStateException.java` around lines 3 - 7, Change the semantics so InvalidMeetingStateException is only used for true state-machine violations (e.g., ProtocolService checks like "Only completed meetings can have protocols"); in MeetingService replace throws of InvalidMeetingStateException that are plain input/argument validations (blank title, missing registry id, endsAt.isBefore(startsAt), missing diarium at the sites currently throwing in MeetingService) with IllegalArgumentException or create and throw a new MeetingValidationException for clearer intent; update import/usages accordingly and ensure GlobalRestExceptionHandler mapping continues to handle IllegalArgumentException (or add a mapping for MeetingValidationException) so validation errors map to the appropriate HTTP response.src/main/resources/templates/admin-layout.html (1)
26-26: Add Subresource Integrity (SRI) to the htmx CDN script.Loading htmx from an unpkg CDN without an
integrityhash andcrossoriginattribute exposes the page to potential code injection if the CDN is compromised. Version pinning helps but doesn't protect against tampering at the CDN level.Suggested change
-<script src="https://unpkg.com/htmx.org@1.9.12"></script> +<script src="https://unpkg.com/htmx.org@1.9.12" + integrity="sha384-ujb1lZYygJmzgSwoxRggbCHcjc0rB2XoQrxeTUQyRjrOnlCoYta87iKBWq3EsdM2" + crossorigin="anonymous"></script>Alternatively, vendor htmx under
/static/js/to eliminate the CDN dependency.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/admin-layout.html` at line 26, The htmx CDN script tag in admin-layout.html lacks Subresource Integrity and crossorigin attributes; update the <script> element that loads "https://unpkg.com/htmx.org@1.9.12" to include the correct SRI integrity hash for htmx v1.9.12 and add crossorigin="anonymous", or alternatively replace the CDN reference by serving a vendor copy under /static/js/ (e.g., save the htmx v1.9.12 file to static/js and update the script src to that local path).src/main/java/backendlab/team4you/meeting/MeetingRepository.java (1)
15-26: LGTM — JPQL is correct.Fully-qualified enum reference (
backendlab.team4you.meeting.MeetingStatus.COMPLETED) is valid in JPQL, and thenot existscorrelated subquery againstProtocol.meetingmatches the@OneToOnemapping inProtocol. As the dataset grows, consider an index onmeeting(status, starts_at)and onprotocol(meeting_id)(the latter likely already exists from the@JoinColumn/unique constraint) if this query becomes hot.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/meeting/MeetingRepository.java` around lines 15 - 26, Add database indexes to support the query in MeetingRepository.findCompletedMeetingsWithoutProtocol(): create a composite index on Meeting(status, startsAt) and ensure there is an index on Protocol.meeting (meeting_id) to speed the correlated subquery; update the Meeting and Protocol entity mappings or migration scripts to add these indexes (reference the Meeting.status and Meeting.startsAt fields and the Protocol.meeting join/foreign-key) so the DB can use them when the dataset grows.src/main/resources/templates/fragments/admin-meetings.html (2)
188-199: Consider a confirmation prompt for destructive actions."Ta bort sammanträde" cascades to all agenda items / documents on the meeting; an accidental click is unrecoverable from the UI. Adding
hx-confirmis a one-attribute change that avoids most accidental deletions.♻️ Proposed change
<form th:attr="hx-post=@{/admin/meetings/{meetingId}/delete(meetingId=${selectedMeeting.id})}" hx-target="#content-area" hx-swap="innerHTML" + hx-confirm="Är du säker på att du vill ta bort sammanträdet?" class="delete-meeting-form" >The same applies to the per-agenda-item "Ta bort ärende" form (line 271).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/fragments/admin-meetings.html` around lines 188 - 199, Add a client-side confirmation to the destructive delete forms: update the form with class "delete-meeting-form" (the form whose submit button text is "Ta bort sammanträde") to include an hx-confirm attribute with a clear confirmation message so htmx will prompt the user before submitting; do the same for the per-agenda-item delete form (the form whose button text is "Ta bort ärende") to prevent accidental irreversible deletions.
19-22: Extract the CSRF hidden input into a reusable Thymeleaf fragment.The same CSRF hidden-input block is duplicated 9 times in this template (and likely repeats across other admin fragments). Extracting it to a small fragment removes the duplication and makes future tweaks (attribute changes, conditional rendering) one-line edits.
♻️ Example refactor
In a shared fragments file (e.g.,
fragments/csrf.html):<input th:fragment="token" type="hidden" th:if="${_csrf != null}" th:name="${_csrf.parameterName}" th:value="${_csrf.token}">Then replace each block with:
<th:block th:replace="~{fragments/csrf :: token}"></th:block>Also applies to: 137-140, 194-197, 211-214, 252-255, 264-267, 276-279, 302-305, 328-331
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/fragments/admin-meetings.html` around lines 19 - 22, Duplicate CSRF hidden-input blocks should be extracted into a single Thymeleaf fragment to remove repetition: create a fragment named "token" (e.g., in a shared fragments template) containing the input element with type="hidden", th:if="${_csrf != null}", th:name="${_csrf.parameterName}" and th:value="${_csrf.token}", then replace each duplicated input block in the template with a th:replace that references that fragment (e.g., use th:block th:replace="~{csrf :: token}" or equivalent fragment reference) for every occurrence of the original input element.src/main/java/backendlab/team4you/meeting/MeetingAgendaItem.java (1)
43-53: Validate required fields in the constructor (consistent with other PR entities).
meeting,caseRecord, andagendaOrderare all required (DB/JPA-level), but the constructor accepts them silently. Other entities introduced in this PR (Protocol,ProtocolParagraph,ProtocolParagraphSequence) useObjects.requireNonNullto fail fast at construction.♻️ Proposed change
public MeetingAgendaItem( Meeting meeting, CaseRecord caseRecord, Integer agendaOrder, String agendaNote ) { - this.meeting = meeting; - this.caseRecord = caseRecord; - this.agendaOrder = agendaOrder; + this.meeting = Objects.requireNonNull(meeting, "meeting is required"); + this.caseRecord = Objects.requireNonNull(caseRecord, "caseRecord is required"); + this.agendaOrder = Objects.requireNonNull(agendaOrder, "agendaOrder is required"); this.agendaNote = agendaNote; }Add
import java.util.Objects;at the top.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/meeting/MeetingAgendaItem.java` around lines 43 - 53, The MeetingAgendaItem constructor accepts required DB/JPA fields without validation; update the constructor for MeetingAgendaItem to perform null checks using Objects.requireNonNull for meeting, caseRecord and agendaOrder to fail fast (add import java.util.Objects if missing) and assign the validated values to this.meeting, this.caseRecord and this.agendaOrder; leave agendaNote as-is (nullable) to match other PR entities (e.g., Protocol, ProtocolParagraph, ProtocolParagraphSequence) that use Objects.requireNonNull.src/main/java/backendlab/team4you/meeting/MeetingAgendaDocument.java (1)
30-33: Validate required fields in the constructor (consistent with other PR entities).
Protocol,ProtocolParagraph, andProtocolParagraphSequenceall useObjects.requireNonNullin their public constructors. Mirroring that here surfaces programming errors at construction time rather than at JPA flush.♻️ Proposed change
public MeetingAgendaDocument(MeetingAgendaItem agendaItem, CaseFile caseFile) { - this.agendaItem = agendaItem; - this.caseFile = caseFile; + this.agendaItem = Objects.requireNonNull(agendaItem, "agendaItem is required"); + this.caseFile = Objects.requireNonNull(caseFile, "caseFile is required"); }Add
import java.util.Objects;at the top.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/meeting/MeetingAgendaDocument.java` around lines 30 - 33, The MeetingAgendaDocument constructor does not validate required fields; update the public constructor MeetingAgendaDocument(MeetingAgendaItem agendaItem, CaseFile caseFile) to validate both parameters using Objects.requireNonNull (add import java.util.Objects) so agendaItem and caseFile are checked at construction time and nulls throw immediately; reference the MeetingAgendaDocument class and its constructor when making the change.src/main/java/backendlab/team4you/protocol/ProtocolParagraphSequence.java (1)
47-49: Optional: routeincrement()throughsetLastValuefor consistency.
increment()mutateslastValuedirectly while the constructor delegates tosetLastValue(...). Funneling all writes through the validating setter keeps the invariant in one place and makes future invariant changes (e.g., upper bound) automatic.♻️ Proposed change
public void increment() { - this.lastValue++; + setLastValue(this.lastValue + 1); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/protocol/ProtocolParagraphSequence.java` around lines 47 - 49, The increment() method currently mutates lastValue directly; change it to call the existing setter so all writes go through the validation path: replace the direct increment with this.setLastValue(this.lastValue + 1) (i.e., have increment() invoke setLastValue(...) rather than doing this.lastValue++), keeping the constructor behavior consistent with setLastValue(...) and preserving any validation/invariants enforced there.src/main/java/backendlab/team4you/protocol/ProtocolController.java (3)
8-8: UnusedRedirectAttributesparameter.
redirectAttributesis never referenced insidecreateProtocol, and the import on line 8 only exists to satisfy this parameter. Drop both.♻️ Suggested fix
-import org.springframework.web.servlet.mvc.support.RedirectAttributes;`@PostMapping`("/meetings/{meetingId}") public String createProtocol( `@PathVariable` Long meetingId, - RedirectAttributes redirectAttributes, Model model ) {Also applies to: 40-46
🤖 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` at line 8, The import org.springframework.web.servlet.mvc.support.RedirectAttributes and the unused RedirectAttributes parameters should be removed; update the ProtocolController by deleting the import and removing the RedirectAttributes parameter from the createProtocol method signature (and any other methods in ProtocolController that take RedirectAttributes around the 40-46 region), then adjust any callers if necessary to match the new signatures and remove any references to the now-removed parameter name.
40-98: No inline error handling — UX inconsistent withMeetingController.
MeetingController.createMeeting/updateMeeting/...wrap service calls in try/catch and render the fragment with anerrorMessagemodel attribute, so HTMX swaps preserve the page and surface the failure inline. Here, anyInvalidMeetingStateException,MeetingNotFoundException,ProtocolAlreadyExistsException,ProtocolNotFoundException, orProtocolParagraphNotFoundExceptionthrown byProtocolServicewill bubble up toGlobalViewExceptionHandler, which on an HTMX request typically yields a full error page swapped into#content-area(or, worse, a 4xx that htmx ignores).Recommend mirroring the meeting-controller pattern — catch the expected domain exceptions, set
errorMessage, and re-renderfragments/admin-protocols :: contentwith the existing meeting/protocol lists populated.🤖 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 40 - 98, The controller methods createProtocol, viewProtocol and updateParagraphDecision currently let ProtocolService exceptions bubble up; change each to catch the domain exceptions (InvalidMeetingStateException, MeetingNotFoundException, ProtocolAlreadyExistsException, ProtocolNotFoundException, ProtocolParagraphNotFoundException) around the service calls, set model.addAttribute("errorMessage", <friendly message or ex.getMessage()>), repopulate the lists using meetingRepository.findCompletedMeetingsWithoutProtocol() and protocolRepository.findAll(), and return "fragments/admin-protocols :: content" so HTMX swaps show the inline error instead of delegating to GlobalViewExceptionHandler.
29-98: Repeated model-population block — extract a helper.The
completedMeetingsWithoutProtocol+protocols(+ optionalselectedProtocol) wiring is duplicated acrosslistProtocols,createProtocol,viewProtocol, andupdateParagraphDecision.MeetingController.populateMeetingsPageis a good template — consider apopulateProtocolsPage(Model, Protocol selected)helper to keep these handlers DRY.🤖 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 29 - 98, The handlers in ProtocolController (listProtocols, createProtocol, viewProtocol, updateParagraphDecision) repeatedly populate the Model with "completedMeetingsWithoutProtocol" and "protocols" (and sometimes "selectedProtocol"); extract a private helper method (e.g., private void populateProtocolsPage(Model model, Protocol selectedProtocol)) in ProtocolController that sets model.addAttribute for completedMeetingsWithoutProtocol, protocols and conditionally selectedProtocol, then replace the duplicated blocks in listProtocols, createProtocol, viewProtocol and updateParagraphDecision with calls to populateProtocolsPage(model, selectedProtocol) (or null when not applicable) so the wiring is centralized.src/main/resources/templates/fragments/admin-protocols.html (1)
166-173: Default decision text duplicated between template and service.The hard-coded
" beslutar att bifalla ärendet."here mirrors the APPROVED branch ofProtocolService.buildDefaultDecisionText. If that copy ever drifts (wording change, new decision types, or i18n), the initial textarea content can become inconsistent with what thedecision-textHTMX endpoint returns. Consider rendering the initial value via the same service helper (e.g., expose it through a model attribute) so there's a single source of truth for the default text.🤖 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 166 - 173, The template currently duplicates the default suffix (" beslutar att bifalla ärendet.") instead of using the single source in ProtocolService.buildDefaultDecisionText; update the controller/HTMX handler that renders this fragment to call ProtocolService.buildDefaultDecisionText(...) and add the result to the model (e.g., as defaultDecisionText), then change the textarea's th:text to use that model attribute (th:text="${paragraph.decisionText != null ? paragraph.decisionText : defaultDecisionText}"); reference ProtocolService.buildDefaultDecisionText and the HTMX decision-text endpoint when wiring the model value so both initial render and the endpoint use the same helper.src/main/java/backendlab/team4you/meeting/MeetingService.java (2)
295-303:validateCaseFileBelongsToAgendaItemCaseRecordis redundant afterfindByIdAndCaseRecordId.Line 295–297 already filters by
caseRecordId(taken from the agenda item), so the returnedCaseFileis guaranteed to belong to the same case record. The subsequent call on line 299 cannot fail meaningfully and is just dead validation. Either drop it or remove the upstream filter and rely on the helper for ownership checking — but doing both is misleading.♻️ Suggested fix
CaseFile caseFile = caseFileRepository .findByIdAndCaseRecordId(caseFileId, caseRecordId) .orElseThrow(() -> new CaseFileNotFoundException(caseRecordId, caseFileId)); - validateCaseFileBelongsToAgendaItemCaseRecord(agendaItem, caseFile); - if (meetingAgendaDocumentRepository.existsByAgendaItemAndCaseFile(agendaItem, caseFile)) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/meeting/MeetingService.java` around lines 295 - 303, The call to validateCaseFileBelongsToAgendaItemCaseRecord(agendaItem, caseFile) is redundant because caseFileRepository.findByIdAndCaseRecordId(caseFileId, caseRecordId) already guarantees the CaseFile belongs to that CaseRecord; remove the validateCaseFileBelongsToAgendaItemCaseRecord(...) invocation and keep the existing repository lookup and duplicate-check (meetingAgendaDocumentRepository.existsByAgendaItemAndCaseFile(...)) so ownership is enforced by the findByIdAndCaseRecordId call and no dead validation remains.
195-251: Sentinel value0is a valid order and risks collisions — consider using a definitely out-of-range sentinel.The PR adds
V20__make_meeting_agenda_order_constraint_deferrable.sql, which sets the unique(meeting_id, agenda_order)constraint toDEFERRABLE INITIALLY IMMEDIATE. Because the constraint is checked immediately after each statement (not deferred to commit), the sentinel pattern with multiplesaveAndFlushcalls is necessary to avoid violations during the swap.However, the sentinel value
0is itself a validIntegerorder. If two concurrent move operations on the same meeting both park their items atagenda_order = 0, the secondsaveAndFlushwill violate the constraint. Use a definitely out-of-range sentinel instead:
- Negative value (e.g.,
-1or per-call offset like-System.nanoTime())- Or a value guaranteed beyond max order (e.g.,
Integer.MAX_VALUE)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/meeting/MeetingService.java` around lines 195 - 251, The swap logic in moveAgendaItemUp and moveAgendaItemDown uses the sentinel value 0 which is a valid agendaOrder and can collide across concurrent swaps; change the temporary sentinel to a definitely out-of-range value (e.g., Integer.MIN_VALUE or Integer.MAX_VALUE) or a per-call unique negative value (e.g., -System.nanoTime() cast to int) when you set currentItem.setAgendaOrder(...), then keep the same saveAndFlush swap sequence to avoid unique-constraint violations; update both moveAgendaItemUp and moveAgendaItemDown to use this out-of-range sentinel.src/main/java/backendlab/team4you/protocol/ProtocolService.java (1)
136-138: Unused parametermeeting.
buildProtocolTitledoesn't referencemeeting. Either use it (e.g., include the meeting title or date) or drop the parameter.♻️ Suggested fix
- private String buildProtocolTitle(Meeting meeting, Registry registry, Integer year) { - return "Protokoll - " + registry.getName() + " - " + year; - } + private String buildProtocolTitle(Registry registry, Integer year) { + return "Protokoll - " + registry.getName() + " - " + year; + }And the call site at line 58:
- buildProtocolTitle(meeting, registry, year), + buildProtocolTitle(registry, year),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/protocol/ProtocolService.java` around lines 136 - 138, The method buildProtocolTitle has an unused parameter meeting; either incorporate meeting into the title (for example use meeting.getTitle() or meeting.getDate() to produce "Protokoll - {registry.getName()} - {meetingTitleOrDate} - {year}") inside buildProtocolTitle, or remove the meeting parameter and update all callers that pass a Meeting to buildProtocolTitle to stop providing it; adjust the method signature and any call sites accordingly (search for buildProtocolTitle usages) so compilation remains correct.src/main/java/backendlab/team4you/meeting/MeetingController.java (1)
298-319: Inconsistent HTTP verb:removeAgendaDocumentshould be aDELETElikeremoveAgendaItem.
removeAgendaItem(Line 246) uses@DeleteMapping("/{meetingId}/agenda-items/{agendaItemId}"), but the analogous remove operation here is aPOSTwith a/removesuffix. Aligning on@DeleteMapping("/{meetingId}/agenda-items/{agendaItemId}/documents/{documentId}")is more REST‑idiomatic and consistent with the existing pattern; HTMX supportshx-delete, so the template can be updated similarly.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/meeting/MeetingController.java` around lines 298 - 319, The removeAgendaDocument method in MeetingController is using a POST with a "/remove" suffix which is inconsistent with removeAgendaItem; change the mapping on removeAgendaDocument to `@DeleteMapping`("/{meetingId}/agenda-items/{agendaItemId}/documents/{documentId}") (remove the "/remove" suffix) so it becomes a proper DELETE endpoint, keep the method signature and body as-is, and update any client/HTMX call (templates/snippets using hx-post) to use hx-delete pointing to the new URL; reference MeetingController.removeAgendaDocument and the existing removeAgendaItem mapping to mirror the pattern.
🤖 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/exceptions/GlobalRestExceptionHandler.java`:
- Around line 64-69: Remove IllegalStateException from the `@ExceptionHandler`
list in GlobalRestExceptionHandler so it is no longer mapped to 400; leave
InvalidMeetingStateException in the list and allow any other
IllegalStateException to fall through to the generic handleUnexpected (500)
handler so unexpected server-side IllegalStateExceptions are logged and returned
as 500 instead of being treated as client errors.
In
`@src/main/java/backendlab/team4you/exceptions/GlobalViewExceptionHandler.java`:
- Around line 73-78: Replace the English exception message being shown to users
in GlobalViewExceptionHandler.handleProtocolAlreadyExists: instead of
model.addAttribute("errorMessage", ex.getMessage()), set a Swedish user-facing
message (for example "Protokoll för sammanträdet finns redan.") so the error
view shows a localized message; if ProtocolAlreadyExistsException exposes
identifiers you want shown to the user, format them into the Swedish message
there (e.g., include meeting id) rather than forwarding ex.getMessage().
In `@src/main/java/backendlab/team4you/exceptions/MeetingNotFoundException.java`:
- Around line 5-7: The MeetingNotFoundException constructor currently discards
the meetingId parameter; adjust MeetingNotFoundException to store the provided
Long meetingId in a private final field and expose it via a public getter (e.g.,
getMeetingId()), and update the constructor that currently accepts Long
meetingId to set that field and call super(...) with a message that includes the
id (or append it via getMessage override) so callers
(MeetingService.getMeetingById, updateMeeting, deleteMeeting, ProtocolService)
receive the failing id; alternatively, if you prefer no id, remove the Long
meetingId parameter from the constructor and update all callers accordingly.
In
`@src/main/java/backendlab/team4you/exceptions/ProtocolParagraphNotFoundException.java`:
- Around line 5-7: The exception ProtocolParagraphNotFoundException uses an
English user message while peers (e.g. MeetingNotFoundException) are Swedish and
GlobalRestExceptionHandler echoes getMessage() to clients; change
ProtocolParagraphNotFoundException to provide a Swedish user-facing message and
stop embedding paragraphId in the message: add a private final Long paragraphId
field, store it in the constructor, call super(...) with a Swedish message like
"Protokollstycket hittades inte." and expose a getter getParagraphId() so
handlers/logging can include the id separately.
In `@src/main/java/backendlab/team4you/meeting/MeetingController.java`:
- Around line 149-152: Replace the catch-all "catch (Exception exception)" in
MeetingController (the block shown and similar blocks in moveAgendaItemUp,
moveAgendaItemDown, addAgendaDocument, removeAgendaDocument) with explicit
catches for the domain exceptions your services declare (e.g.
MeetingNotFoundException, MeetingAgendaItemNotFoundException,
MeetingAgendaDocumentNotFoundException, InvalidMeetingStateException,
FileInUseException); in each specific catch set the model attribute
"errorMessage" from the exception and call populateMeetingsPage(model, null,
null), and do not swallow other exceptions—either let them propagate (no catch)
or rethrow them so the GlobalViewExceptionHandler can handle them.
- Around line 347-365: populateMeetingsPage currently does 2·N service/DB calls
inside the for-loop by invoking meetingService.getAgendaDocuments(...) and
meetingService.getAvailableCaseFilesForAgendaItem(...) per MeetingAgendaItem;
introduce a new MeetingService method (e.g.
getAgendaDocumentsAndAvailableFilesForMeeting(Long meetingId) or
getAgendaDataForMeeting(Long meetingId)) that returns both maps keyed by
agendaItemId using a single/few batched queries (fetch-join documents by
meetingId and compute available files per agenda item in one query), then
replace the loop in populateMeetingsPage to call that method and populate
documentsByAgendaItemId and availableFilesByAgendaItemId from its result;
additionally, guard the meetingsPage GET path around the
meetingService.getMeetingById(selectedMeetingId) call (or reuse the existing
populateMeetingsPageAfterMeetingAction fallback) to catch
MeetingNotFoundException and render a graceful fallback instead of letting it
propagate to the global error page.
- Around line 36-44: The meetingsPage method currently always returns the HTMX
fragment "fragments/admin-meetings :: content", which breaks direct browser
navigation; update meetingsPage to detect the HX-Request header (e.g. via
request.getHeader("HX-Request") or using `@RequestHeader`) and branch: if
HX-Request present return the fragment as now, otherwise return the full page
view (same template used by showMeeting/createMeeting) so the full layout and
assets are rendered. Apply the same HX-Request branching pattern to the other
handlers that unconditionally return fragments: updateMeeting, deleteMeeting,
moveAgendaItemUp, moveAgendaItemDown, addAgendaDocument and removeAgendaDocument
to return the full view when not an HTMX request.
- Around line 124-130: The try/catch around registryId retrieval swallows
MeetingNotFoundException and has broken indentation; update the block around
registryId = meetingService.getMeetingById(meetingId).getRegistry().getId() so
braces are properly aligned, and replace the empty catch
(MeetingNotFoundException ignored) with a catch that logs the event (e.g., using
the controller's logger) and leaves registryId null, then call
populateMeetingsPage(model, registryId, registryId == null ? null : meetingId)
exactly as before; reference the meetingService.getMeetingById(...),
MeetingNotFoundException, registryId, populateMeetingsPage, model and meetingId
symbols when applying the change.
- Around line 57-89: The LocalDateTime.parse(...) calls in createMeeting
(LocalDateTime.parse(startsAt) and parse(endsAt)) — and the analogous calls in
updateMeeting — can throw DateTimeParseException which is not currently caught;
modify the try/catch to also catch DateTimeParseException (or add a separate
catch) and handle it the same way as InvalidMeetingStateException: add an
appropriate model error message (e.g., "Ogiltigt datum/tidformat"), compute
safeRegistryId using registryRepository.existsById(...) as done now, call
populateMeetingsPage(model, safeRegistryId, null) to repopulate the form, and
return the same fragment/view flow so the user sees an inline form error instead
of the global error page.
In `@src/main/java/backendlab/team4you/meeting/MeetingRepository.java`:
- Around line 11-13: Remove the unused repository method
findByRegistryOrderByStartsAtAsc from the MeetingRepository interface; locate
the method declaration in MeetingRepository (the one named
findByRegistryOrderByStartsAtAsc) and delete it so only the used methods
findByRegistryOrderByStartsAtDesc and findAllByOrderByStartsAtDesc remain; no
other changes to MeetingService are required since it already calls the
descending variants.
In `@src/main/java/backendlab/team4you/meeting/MeetingService.java`:
- Around line 329-332: The updateMeetingStatus method currently throws
IllegalArgumentException for a null status; change it to throw
InvalidMeetingStateException (the same exception used by createMeeting and
updateMeeting validation paths) with the existing message so the
GlobalRestExceptionHandler/GlobalViewExceptionHandler will handle it
consistently; locate the updateMeetingStatus method and replace the
IllegalArgumentException throw with new InvalidMeetingStateException("Status
måste anges.") to match the other validation flows.
In `@src/main/java/backendlab/team4you/protocol/ProtocolService.java`:
- Around line 125-134: allocateNextParagraphNumber has a race when the sequence
row doesn't exist: two transactions can both create a new
ProtocolParagraphSequence and collide on insert, causing
DataIntegrityViolationException; update the
allocateNextParagraphNumber(Registry, Integer) method to catch
DataIntegrityViolationException, retry the read-and-increment flow with
exponential backoff (e.g., small initial delay, doubling, max attempts) and on
retry use sequenceRepository.findByRegistryIdAndYear(...) again so the existing
row is locked and incremented; alternatively (or additionally) add a pre-seed
path on registry creation that upserts a ProtocolParagraphSequence (INSERT ...
ON CONFLICT DO NOTHING) to ensure a row exists for each (registry, year) before
first allocation; reference: allocateNextParagraphNumber, sequenceRepository,
ProtocolParagraphSequence.
In `@src/main/resources/db/migration/V21__create_protocol_table.sql`:
- Around line 25-29: The foreign key constraint fk_protocol_paragraph_protocol
currently lacks ON DELETE CASCADE which mismatches the Protocol entity's
`@OneToMany`(cascade = CascadeType.ALL, orphanRemoval = true) behavior; either
update the V21 migration to add ON DELETE CASCADE to the
fk_protocol_paragraph_protocol foreign key so DB-level deletes mirror JPA
cascades, or add a clear SQL comment in the migration documenting the
intentional difference and why DB-level cascade was omitted.
In `@src/main/resources/static/css/meetings.css`:
- Around line 71-86: Add keyboard focus styles for buttons to meet WCAG 2.4.7 by
defining :focus-visible rules that mirror the visible focus ring used for
inputs; update the selectors .meeting-form .submit:focus-visible,
.danger-button:focus-visible, and .secondary-button:focus-visible to apply a
clear focus outline (or box-shadow and outline-offset) and ensure
background/foreground contrast remains readable; keep existing :hover styles
unchanged and reuse the same color/offset values used for input focus (lines
near input focus ring) so keyboard users get an accessible visible focus
indicator.
In `@src/test/java/backendlab/team4you/meeting/MeetingControllerTest.java`:
- Line 124: The stub for meetingService.getMeetingsForRegistry(1L) is never used
because safeRegistryId is null when registryRepository.existsById(1L) is not
stubbed; either stub registryRepository.existsById(1L) to return true so
populateMeetingsPage will call meetingService.getMeetingsForRegistry, or remove
that unused stub and instead stub meetingService.getAllMeetings() to match the
current fallback path; adjust the test accordingly around
registryRepository.existsById, safeRegistryId, populateMeetingsPage,
meetingService.getMeetingsForRegistry and meetingService.getAllMeetings.
In `@src/test/resources/application-test.properties`:
- Around line 7-10: The test config currently hardcodes spring.datasource.url to
the production DB and risks clobbering developer data; update the test profile
so spring.datasource.url is environment-overridable (e.g., use a ${DB_URL:...}
default) and point the default to a dedicated test database (such as
team4you_test) instead of team4you, or replace the static URL entirely by wiring
Testcontainers Postgres for tests (configure your test support that creates a
container and sets spring.datasource properties for `@SpringBootTest`), ensuring
spring.datasource.username and spring.datasource.password remain overridable
too.
---
Nitpick comments:
In `@docker-compose.yaml`:
- Around line 23-32: The docker-compose change removed persistent storage for
LocalStack, which causes S3 objects to vanish while postgres_data remains
persistent and leads to dangling DB references; restore a named volume for
LocalStack (e.g., add back localstack_data and mount it at LocalStack's data
directory such as /tmp/localstack or the container volume used by the service)
in the docker-compose service definition and declare the localstack_data volume
alongside postgres_data, or alternatively update README and/or
init-localstack.sh to document or explicitly reset postgres_data on LocalStack
resets; reference the docker-compose service that uses init-localstack.sh, the
init-localstack.sh script, and the postgres_data/localstack_data volume names
when making the change.
In
`@src/main/java/backendlab/team4you/exceptions/InvalidMeetingStateException.java`:
- Around line 3-7: Change the semantics so InvalidMeetingStateException is only
used for true state-machine violations (e.g., ProtocolService checks like "Only
completed meetings can have protocols"); in MeetingService replace throws of
InvalidMeetingStateException that are plain input/argument validations (blank
title, missing registry id, endsAt.isBefore(startsAt), missing diarium at the
sites currently throwing in MeetingService) with IllegalArgumentException or
create and throw a new MeetingValidationException for clearer intent; update
import/usages accordingly and ensure GlobalRestExceptionHandler mapping
continues to handle IllegalArgumentException (or add a mapping for
MeetingValidationException) so validation errors map to the appropriate HTTP
response.
In `@src/main/java/backendlab/team4you/meeting/MeetingAgendaDocument.java`:
- Around line 30-33: The MeetingAgendaDocument constructor does not validate
required fields; update the public constructor
MeetingAgendaDocument(MeetingAgendaItem agendaItem, CaseFile caseFile) to
validate both parameters using Objects.requireNonNull (add import
java.util.Objects) so agendaItem and caseFile are checked at construction time
and nulls throw immediately; reference the MeetingAgendaDocument class and its
constructor when making the change.
In `@src/main/java/backendlab/team4you/meeting/MeetingAgendaItem.java`:
- Around line 43-53: The MeetingAgendaItem constructor accepts required DB/JPA
fields without validation; update the constructor for MeetingAgendaItem to
perform null checks using Objects.requireNonNull for meeting, caseRecord and
agendaOrder to fail fast (add import java.util.Objects if missing) and assign
the validated values to this.meeting, this.caseRecord and this.agendaOrder;
leave agendaNote as-is (nullable) to match other PR entities (e.g., Protocol,
ProtocolParagraph, ProtocolParagraphSequence) that use Objects.requireNonNull.
In `@src/main/java/backendlab/team4you/meeting/MeetingController.java`:
- Around line 298-319: The removeAgendaDocument method in MeetingController is
using a POST with a "/remove" suffix which is inconsistent with
removeAgendaItem; change the mapping on removeAgendaDocument to
`@DeleteMapping`("/{meetingId}/agenda-items/{agendaItemId}/documents/{documentId}")
(remove the "/remove" suffix) so it becomes a proper DELETE endpoint, keep the
method signature and body as-is, and update any client/HTMX call
(templates/snippets using hx-post) to use hx-delete pointing to the new URL;
reference MeetingController.removeAgendaDocument and the existing
removeAgendaItem mapping to mirror the pattern.
In `@src/main/java/backendlab/team4you/meeting/MeetingRepository.java`:
- Around line 15-26: Add database indexes to support the query in
MeetingRepository.findCompletedMeetingsWithoutProtocol(): create a composite
index on Meeting(status, startsAt) and ensure there is an index on
Protocol.meeting (meeting_id) to speed the correlated subquery; update the
Meeting and Protocol entity mappings or migration scripts to add these indexes
(reference the Meeting.status and Meeting.startsAt fields and the
Protocol.meeting join/foreign-key) so the DB can use them when the dataset
grows.
In `@src/main/java/backendlab/team4you/meeting/MeetingService.java`:
- Around line 295-303: The call to
validateCaseFileBelongsToAgendaItemCaseRecord(agendaItem, caseFile) is redundant
because caseFileRepository.findByIdAndCaseRecordId(caseFileId, caseRecordId)
already guarantees the CaseFile belongs to that CaseRecord; remove the
validateCaseFileBelongsToAgendaItemCaseRecord(...) invocation and keep the
existing repository lookup and duplicate-check
(meetingAgendaDocumentRepository.existsByAgendaItemAndCaseFile(...)) so
ownership is enforced by the findByIdAndCaseRecordId call and no dead validation
remains.
- Around line 195-251: The swap logic in moveAgendaItemUp and moveAgendaItemDown
uses the sentinel value 0 which is a valid agendaOrder and can collide across
concurrent swaps; change the temporary sentinel to a definitely out-of-range
value (e.g., Integer.MIN_VALUE or Integer.MAX_VALUE) or a per-call unique
negative value (e.g., -System.nanoTime() cast to int) when you set
currentItem.setAgendaOrder(...), then keep the same saveAndFlush swap sequence
to avoid unique-constraint violations; update both moveAgendaItemUp and
moveAgendaItemDown to use this out-of-range sentinel.
In `@src/main/java/backendlab/team4you/protocol/ProtocolController.java`:
- Line 8: The import
org.springframework.web.servlet.mvc.support.RedirectAttributes and the unused
RedirectAttributes parameters should be removed; update the ProtocolController
by deleting the import and removing the RedirectAttributes parameter from the
createProtocol method signature (and any other methods in ProtocolController
that take RedirectAttributes around the 40-46 region), then adjust any callers
if necessary to match the new signatures and remove any references to the
now-removed parameter name.
- Around line 40-98: The controller methods createProtocol, viewProtocol and
updateParagraphDecision currently let ProtocolService exceptions bubble up;
change each to catch the domain exceptions (InvalidMeetingStateException,
MeetingNotFoundException, ProtocolAlreadyExistsException,
ProtocolNotFoundException, ProtocolParagraphNotFoundException) around the
service calls, set model.addAttribute("errorMessage", <friendly message or
ex.getMessage()>), repopulate the lists using
meetingRepository.findCompletedMeetingsWithoutProtocol() and
protocolRepository.findAll(), and return "fragments/admin-protocols :: content"
so HTMX swaps show the inline error instead of delegating to
GlobalViewExceptionHandler.
- Around line 29-98: The handlers in ProtocolController (listProtocols,
createProtocol, viewProtocol, updateParagraphDecision) repeatedly populate the
Model with "completedMeetingsWithoutProtocol" and "protocols" (and sometimes
"selectedProtocol"); extract a private helper method (e.g., private void
populateProtocolsPage(Model model, Protocol selectedProtocol)) in
ProtocolController that sets model.addAttribute for
completedMeetingsWithoutProtocol, protocols and conditionally selectedProtocol,
then replace the duplicated blocks in listProtocols, createProtocol,
viewProtocol and updateParagraphDecision with calls to
populateProtocolsPage(model, selectedProtocol) (or null when not applicable) so
the wiring is centralized.
In `@src/main/java/backendlab/team4you/protocol/ProtocolParagraphSequence.java`:
- Around line 47-49: The increment() method currently mutates lastValue
directly; change it to call the existing setter so all writes go through the
validation path: replace the direct increment with
this.setLastValue(this.lastValue + 1) (i.e., have increment() invoke
setLastValue(...) rather than doing this.lastValue++), keeping the constructor
behavior consistent with setLastValue(...) and preserving any
validation/invariants enforced there.
In `@src/main/java/backendlab/team4you/protocol/ProtocolService.java`:
- Around line 136-138: The method buildProtocolTitle has an unused parameter
meeting; either incorporate meeting into the title (for example use
meeting.getTitle() or meeting.getDate() to produce "Protokoll -
{registry.getName()} - {meetingTitleOrDate} - {year}") inside
buildProtocolTitle, or remove the meeting parameter and update all callers that
pass a Meeting to buildProtocolTitle to stop providing it; adjust the method
signature and any call sites accordingly (search for buildProtocolTitle usages)
so compilation remains correct.
In
`@src/main/resources/db/migration/V20__make_meeting_agenda_order_constraint_deferrable.sql`:
- Around line 1-7: The migration makes uk_meeting_agenda_item_meeting_order
deferrable but the code never defers it and relies on agendaOrder=0 sentinel;
update the migration to also add a CHECK (agenda_order > 0) constraint to
prevent real rows from using 0 OR change the unique constraint to DEFERRABLE
INITIALLY DEFERRED and/or add SQL to defer the constraint at swap time; then
adjust MeetingService.moveAgendaItemUp() and moveAgendaItemDown() to perform
swaps inside a single transaction and issue SET CONSTRAINTS
uk_meeting_agenda_item_meeting_order DEFERRED before any save/flush (or stop
using the 0-sentinel and use a true transactional three-way swap), referencing
the constraint name uk_meeting_agenda_item_meeting_order and the methods
MeetingService.moveAgendaItemUp / moveAgendaItemDown and the agenda_order
column.
In `@src/main/resources/db/migration/V21__create_protocol_table.sql`:
- Around line 18-30: Add a uniqueness constraint to the protocol_paragraph table
to enforce that (protocol_id, paragraph_number) is unique; update the CREATE
TABLE for protocol_paragraph (or issue an ALTER TABLE if you prefer migration
style) to add a UNIQUE constraint named something like
uq_protocol_paragraph_protocol_paragraph_number on the column pair (protocol_id,
paragraph_number) so duplicates within the same protocol cannot be inserted;
reference the table name protocol_paragraph and the existing columns protocol_id
and paragraph_number when making the change.
In `@src/main/resources/templates/admin-layout.html`:
- Line 26: The htmx CDN script tag in admin-layout.html lacks Subresource
Integrity and crossorigin attributes; update the <script> element that loads
"https://unpkg.com/htmx.org@1.9.12" to include the correct SRI integrity hash
for htmx v1.9.12 and add crossorigin="anonymous", or alternatively replace the
CDN reference by serving a vendor copy under /static/js/ (e.g., save the htmx
v1.9.12 file to static/js and update the script src to that local path).
In `@src/main/resources/templates/fragments/admin-meetings.html`:
- Around line 188-199: Add a client-side confirmation to the destructive delete
forms: update the form with class "delete-meeting-form" (the form whose submit
button text is "Ta bort sammanträde") to include an hx-confirm attribute with a
clear confirmation message so htmx will prompt the user before submitting; do
the same for the per-agenda-item delete form (the form whose button text is "Ta
bort ärende") to prevent accidental irreversible deletions.
- Around line 19-22: Duplicate CSRF hidden-input blocks should be extracted into
a single Thymeleaf fragment to remove repetition: create a fragment named
"token" (e.g., in a shared fragments template) containing the input element with
type="hidden", th:if="${_csrf != null}", th:name="${_csrf.parameterName}" and
th:value="${_csrf.token}", then replace each duplicated input block in the
template with a th:replace that references that fragment (e.g., use th:block
th:replace="~{csrf :: token}" or equivalent fragment reference) for every
occurrence of the original input element.
In `@src/main/resources/templates/fragments/admin-protocols.html`:
- Around line 166-173: The template currently duplicates the default suffix ("
beslutar att bifalla ärendet.") instead of using the single source in
ProtocolService.buildDefaultDecisionText; update the controller/HTMX handler
that renders this fragment to call ProtocolService.buildDefaultDecisionText(...)
and add the result to the model (e.g., as defaultDecisionText), then change the
textarea's th:text to use that model attribute
(th:text="${paragraph.decisionText != null ? paragraph.decisionText :
defaultDecisionText}"); reference ProtocolService.buildDefaultDecisionText and
the HTMX decision-text endpoint when wiring the model value so both initial
render and the endpoint use the same helper.
In `@src/main/resources/templates/fragments/admin-sidenav.html`:
- Around line 39-47: The sidebar entry for the "Sammanträden" link (the <a> with
th:href="@{/admin/meetings}" / hx-get="/admin/meetings" and the
<span>Sammanträden</span>) uses the same icon class fa-calendar-days as the
"Bokningar" entry; update the <i> element's class to a distinct icon such as
fa-users, fa-people-group, or fa-gavel (e.g., replace fa-calendar-days with one
of those) so the meetings item is visually distinguishable from bookings.
🪄 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: eb4c7fa1-9663-43be-bb37-48cdf54d5c0e
📒 Files selected for processing (51)
docker-compose.yamlsrc/main/java/backendlab/team4you/casefile/CaseFileRepository.javasrc/main/java/backendlab/team4you/casefile/CaseFileService.javasrc/main/java/backendlab/team4you/casefile/ui/CaseFileViewController.javasrc/main/java/backendlab/team4you/exceptions/DuplicateMeetingAgendaDocumentException.javasrc/main/java/backendlab/team4you/exceptions/DuplicateMeetingAgendaItemException.javasrc/main/java/backendlab/team4you/exceptions/FileInUseException.javasrc/main/java/backendlab/team4you/exceptions/GlobalRestExceptionHandler.javasrc/main/java/backendlab/team4you/exceptions/GlobalViewExceptionHandler.javasrc/main/java/backendlab/team4you/exceptions/InvalidMeetingStateException.javasrc/main/java/backendlab/team4you/exceptions/MeetingAgendaDocumentNotFoundException.javasrc/main/java/backendlab/team4you/exceptions/MeetingAgendaItemNotFoundException.javasrc/main/java/backendlab/team4you/exceptions/MeetingNotFoundException.javasrc/main/java/backendlab/team4you/exceptions/ProtocolAlreadyExistsException.javasrc/main/java/backendlab/team4you/exceptions/ProtocolNotFoundException.javasrc/main/java/backendlab/team4you/exceptions/ProtocolParagraphNotFoundException.javasrc/main/java/backendlab/team4you/meeting/Meeting.javasrc/main/java/backendlab/team4you/meeting/MeetingAgendaDocument.javasrc/main/java/backendlab/team4you/meeting/MeetingAgendaDocumentRepository.javasrc/main/java/backendlab/team4you/meeting/MeetingAgendaItem.javasrc/main/java/backendlab/team4you/meeting/MeetingAgendaItemRepository.javasrc/main/java/backendlab/team4you/meeting/MeetingController.javasrc/main/java/backendlab/team4you/meeting/MeetingRepository.javasrc/main/java/backendlab/team4you/meeting/MeetingService.javasrc/main/java/backendlab/team4you/meeting/MeetingStatus.javasrc/main/java/backendlab/team4you/protocol/Protocol.javasrc/main/java/backendlab/team4you/protocol/ProtocolController.javasrc/main/java/backendlab/team4you/protocol/ProtocolDecisionType.javasrc/main/java/backendlab/team4you/protocol/ProtocolParagraph.javasrc/main/java/backendlab/team4you/protocol/ProtocolParagraphRepository.javasrc/main/java/backendlab/team4you/protocol/ProtocolParagraphSequence.javasrc/main/java/backendlab/team4you/protocol/ProtocolParagraphSequenceRepository.javasrc/main/java/backendlab/team4you/protocol/ProtocolRepository.javasrc/main/java/backendlab/team4you/protocol/ProtocolService.javasrc/main/resources/db/migration/V19__create_meeting_tables.sqlsrc/main/resources/db/migration/V20__make_meeting_agenda_order_constraint_deferrable.sqlsrc/main/resources/db/migration/V21__create_protocol_table.sqlsrc/main/resources/db/migration/V22__add_decision_fields_to_protocol_paragraph.sqlsrc/main/resources/static/css/admin.csssrc/main/resources/static/css/meetings.csssrc/main/resources/static/css/protocols.csssrc/main/resources/templates/admin-layout.htmlsrc/main/resources/templates/fragments/admin-meetings.htmlsrc/main/resources/templates/fragments/admin-protocols.htmlsrc/main/resources/templates/fragments/admin-sidenav.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/ProtocolControllerTest.javasrc/test/java/backendlab/team4you/protocol/ProtocolServiceTest.javasrc/test/resources/application-test.properties
Comment on lines
+329
to
+332
| public Meeting updateMeetingStatus(Long meetingId, MeetingStatus status) { | ||
| if (status == null) { | ||
| throw new IllegalArgumentException("Status måste anges."); | ||
| } |
Contributor
There was a problem hiding this comment.
Inconsistent exception type for "Status måste anges.".
createMeeting (line 60), updateMeeting (line 110), and other validation paths in this service all throw InvalidMeetingStateException for missing/invalid input. updateMeetingStatus instead throws IllegalArgumentException — which won't be picked up by GlobalRestExceptionHandler/GlobalViewExceptionHandler mappings for InvalidMeetingStateException and will surface as a generic 500 to the caller.
♻️ Suggested fix
- throw new IllegalArgumentException("Status måste anges.");
+ throw new InvalidMeetingStateException("Status måste anges.");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public Meeting updateMeetingStatus(Long meetingId, MeetingStatus status) { | |
| if (status == null) { | |
| throw new IllegalArgumentException("Status måste anges."); | |
| } | |
| public Meeting updateMeetingStatus(Long meetingId, MeetingStatus status) { | |
| if (status == null) { | |
| throw new InvalidMeetingStateException("Status måste anges."); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/java/backendlab/team4you/meeting/MeetingService.java` around lines
329 - 332, The updateMeetingStatus method currently throws
IllegalArgumentException for a null status; change it to throw
InvalidMeetingStateException (the same exception used by createMeeting and
updateMeeting validation paths) with the existing message so the
GlobalRestExceptionHandler/GlobalViewExceptionHandler will handle it
consistently; locate the updateMeetingStatus method and replace the
IllegalArgumentException throw with new InvalidMeetingStateException("Status
måste anges.") to match the other validation flows.
Comment on lines
+25
to
+29
| constraint fk_protocol_paragraph_protocol | ||
| foreign key (protocol_id) references protocol(id), | ||
|
|
||
| constraint fk_protocol_paragraph_case_record | ||
| foreign key (case_record_id) references case_record(id) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect the Protocol entity's paragraph collection mapping (cascade/orphanRemoval) to confirm JPA-level handling.
fd -t f 'Protocol.java' | xargs rg -nP -C3 '(`@OneToMany`|paragraphs|cascade|orphanRemoval)'Repository: ithsjava25/project-backend-team4you
Length of output: 878
Align foreign key constraint with JPA cascade configuration or document the intentional difference.
The fk_protocol_paragraph_protocol foreign key constraint does not declare ON DELETE CASCADE. However, the Protocol entity is configured with @OneToMany(cascade = CascadeType.ALL, orphanRemoval = true) on the paragraphs collection. This creates a mismatch: JPA-managed deletes will cascade and remove orphans, but direct SQL deletes will not propagate. Either add ON DELETE CASCADE to the foreign key constraint to align with the pattern used in V19__create_meeting_tables.sql, or add a comment explaining why the intentional difference exists.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/db/migration/V21__create_protocol_table.sql` around lines
25 - 29, The foreign key constraint fk_protocol_paragraph_protocol currently
lacks ON DELETE CASCADE which mismatches the Protocol entity's
`@OneToMany`(cascade = CascadeType.ALL, orphanRemoval = true) behavior; either
update the V21 migration to add ON DELETE CASCADE to the
fk_protocol_paragraph_protocol foreign key so DB-level deletes mirror JPA
cascades, or add a clear SQL comment in the migration documenting the
intentional difference and why DB-level cascade was omitted.
…o broad, Localize the user-facing error message, meetingId constructor argument is silently discarded and Inconsistent message locale with sibling exceptions.
…ent — direct browser navigation is broken.
… allocation per (registry, year).
…ade configuration or document the intentional difference.
… is never exercised—remove it or stub registryRepository.existsById.
…es and can clobber the developer's DB. creates a testdb automaticly when building the container
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/main/java/backendlab/team4you/meeting/MeetingController.java (2)
148-151:⚠️ Potential issue | 🟡 MinorCatch-all
catch (Exception ...)still hides unexpected failures across five handlers.
deleteMeeting(148),moveAgendaItemUp(212),moveAgendaItemDown(235),addAgendaDocument(284) andremoveAgendaDocument(308) still swallow everyExceptionand surfaceex.getMessage()to the UI.NullPointerException,DataAccessException,DateTimeParseException, etc. will be rendered as Swedish-looking error messages with bug-leaking content, and will not be mapped to proper HTTP statuses byGlobalViewExceptionHandler.Note that
addAgendaItemandremoveAgendaItemalready catch the concrete domain exceptions — the same pattern should be applied to the other five for consistency and observability.Also applies to: 212-215, 235-238, 284-287, 308-311
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/meeting/MeetingController.java` around lines 148 - 151, Replace the broad catch(Exception) blocks in deleteMeeting, moveAgendaItemUp, moveAgendaItemDown, addAgendaDocument and removeAgendaDocument with targeted handling: catch only the specific domain exceptions you expect (e.g., MeetingNotFoundException, AgendaItemNotFoundException, DocumentUploadException — use the actual exception types used elsewhere like in addAgendaItem/removeAgendaItem), set model attributes for those cases, and rethrow (or let) any other unexpected exceptions so GlobalViewExceptionHandler can map proper HTTP statuses; also add a log entry for unexpected errors before rethrowing to preserve observability.
59-62:⚠️ Potential issue | 🟡 Minor
DateTimeParseExceptionstill escapes the form-error catch block.
LocalDateTime.parse(startsAt)/parse(endsAt)here and inupdateMeeting(Lines 101-104) throwDateTimeParseException, but the catch at Lines 76 and 119 only handlesInvalidMeetingStateException | MeetingNotFoundException | RegistryNotFoundException. A direct/programmatic POST with malformedstartsAt(or a browser without HTML5 datetime support) bypasses inline error rendering and lands onGlobalViewExceptionHandler→ genericerrorpage.Either include
DateTimeParseExceptionin the multi-catch, or pre-parse and translate failures intoInvalidMeetingStateException("Ogiltigt datum-/tidsformat.")so the user sees an inline form error instead.Also applies to: 101-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/meeting/MeetingController.java` around lines 59 - 62, Parsing of startsAt/endsAt via LocalDateTime.parse in the controller's create and update flows can throw DateTimeParseException which bypasses the current multi-catch (InvalidMeetingStateException | MeetingNotFoundException | RegistryNotFoundException); fix by either adding DateTimeParseException to that multi-catch or (preferred) wrap the LocalDateTime.parse calls in a small try/catch and rethrow new InvalidMeetingStateException("Ogiltigt datum-/tidsformat.") so the error is rendered inline; update the parse sites used in createMeeting and updateMeeting accordingly and ensure the thrown InvalidMeetingStateException is handled by the existing form-error flow.
🧹 Nitpick comments (5)
src/test/java/backendlab/team4you/protocol/ProtocolServiceTest.java (2)
81-83: Confirm Mockito can mockCaseRecord/Meetinghere.
CaseRecordis mocked viamock(CaseRecord.class), andMeetingAgendaItem(meeting, firstCaseRecord, …)later uses that mock as a real argument to a JPA entity. This works only ifCaseRecordis non-final and the Mockito version in use can subclass entity classes. If you ever addfinalfor performance/immutability, these tests will silently break.A small alternative is constructing a real
CaseRecordvia its constructor andsetField(caseRecord, "id", ...), mirroring howRegistryandMeetingare constructed insetUp(). Not blocking — flagging only because the rest of the test uses real entities + reflection consistently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/backendlab/team4you/protocol/ProtocolServiceTest.java` around lines 81 - 83, The test currently uses mock(CaseRecord.class) in setUp() and then passes that mock into MeetingAgendaItem(meeting, firstCaseRecord, ...), which can break if CaseRecord is made final or Mockito cannot subclass it; replace the mock usage with a real CaseRecord instance constructed via its constructor and set its id (and any required fields) with the same reflection helper used elsewhere (setField(caseRecord, "id", ...)), mirroring how Registry and Meeting are created in setUp(), so MeetingAgendaItem receives a real JPA entity instead of a Mockito proxy.
85-122: Optional: cover the “sequence row does not yet exist” branch.All
createProtocolForCompletedMeetingtests stubsequenceRepository.findByRegistryIdAndYear(...)to return a pre-existingProtocolParagraphSequence. TheorElseGet(...)branch inProtocolService#allocateNextParagraphNumber(first protocol ever for a(registry, year)pair) is therefore uncovered — exactly the path that triggers the race condition / unique-constraint risk discussed in the service review.A test that returns
Optional.empty()and asserts paragraph numbering starts at1L(and thatsaveAndFlushis invoked) would lock down this important entry path. A second test stubbing the firstsaveAndFlushto throwDataIntegrityViolationExceptionand the second to succeed would document the intended retry semantics (and would surface the rollback-only issue flagged inProtocolService).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/backendlab/team4you/protocol/ProtocolServiceTest.java` around lines 85 - 122, Add tests in ProtocolServiceTest that cover the branch where sequenceRepository.findByRegistryIdAndYear(...) returns Optional.empty(): call protocolService.createProtocolForCompletedMeeting(...) with a meeting whose registry/year pair has no existing ProtocolParagraphSequence and assert paragraphs start at 1L and that sequenceRepository.saveAndFlush(...) is invoked; additionally add a test that stubs sequenceRepository.saveAndFlush(...) to first throw DataIntegrityViolationException and then succeed to verify the retry semantics in ProtocolService#allocateNextParagraphNumber (and that createProtocolForCompletedMeeting still produces correct numbering), referencing ProtocolParagraphSequence, sequenceRepository.saveAndFlush, and ProtocolService#createProtocolForCompletedMeeting to locate the behavior to exercise.src/main/java/backendlab/team4you/exceptions/GlobalRestExceptionHandler.java (1)
211-221: Optional: foldFileInUseExceptioninto the existinghandleConflict.
handleFileInUseis functionally identical tohandleConflict— same status, sameErrorResponseDtoshape, sameex.getMessage()body. AddingFileInUseException.classto the@ExceptionHandlerarray onhandleConflictremoves the duplication.♻️ Proposed consolidation
`@ExceptionHandler`({ DuplicateRegistryNameException.class, DuplicateRegistryCodeException.class, DuplicateEmailException.class, FileKeyConflictException.class, DuplicateMeetingAgendaItemException.class, DuplicateMeetingAgendaDocumentException.class, - ProtocolAlreadyExistsException.class + ProtocolAlreadyExistsException.class, + FileInUseException.class }) public ResponseEntity<ErrorResponseDto> handleConflict(RuntimeException ex) { @@ - `@ExceptionHandler`(FileInUseException.class) - public ResponseEntity<ErrorResponseDto> handleFileInUse(FileInUseException ex) { - return ResponseEntity.status(HttpStatus.CONFLICT) - .body(new ErrorResponseDto( - HttpStatus.CONFLICT.value(), - "conflict", - ex.getMessage(), - LocalDateTime.now() - )); - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/exceptions/GlobalRestExceptionHandler.java` around lines 211 - 221, The handleFileInUse method duplicates handleConflict; remove the handleFileInUse(FileInUseException) method and add FileInUseException.class to the `@ExceptionHandler` annotation on handleConflict so that handleConflict handles both exceptions (preserving the same ResponseEntity<ErrorResponseDto> behavior and message). Ensure the method signature for handleConflict remains unchanged and that FileInUseException is referenced in the annotation array.src/main/java/backendlab/team4you/meeting/MeetingController.java (1)
156-170:showMeetinghas a redundant HX-Request branch.The explicit
if (htmx != null) { return "fragments/admin-meetings :: content"; }is functionally identical to falling through tomeetingsView(htmx)(which performs the same check). You can drop the inline check for consistency with the other handlers and reduce duplication.♻️ Proposed simplification
`@GetMapping`("/{meetingId}") public String showMeeting( `@PathVariable` Long meetingId, `@RequestHeader`(value = "HX-Request", required = false) String htmx, Model model ) { Meeting meeting = meetingService.getMeetingById(meetingId); populateMeetingsPage(model, meeting.getRegistry().getId(), meetingId); - if (htmx != null) { - return "fragments/admin-meetings :: content"; - } - return meetingsView(htmx); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/meeting/MeetingController.java` around lines 156 - 170, The showMeeting method contains a redundant HTMX check: remove the explicit if (htmx != null) branch and let the method fall through to meetingsView(htmx) instead; in MeetingController.showMeeting (which currently calls meetingService.getMeetingById(...) and populateMeetingsPage(...)), delete the inline return "fragments/admin-meetings :: content" branch so the HTMX handling is centralized in meetingsView(htmx) like the other handlers.src/main/java/backendlab/team4you/protocol/ProtocolService.java (1)
150-152: Unusedmeetingparameter inbuildProtocolTitle.
meetingis never referenced — onlyregistry.getName()andyearare used.♻️ Proposed cleanup
- private String buildProtocolTitle(Meeting meeting, Registry registry, Integer year) { + private String buildProtocolTitle(Registry registry, Integer year) { return "Protokoll - " + registry.getName() + " - " + year; }And update the call site at line 61 accordingly.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/protocol/ProtocolService.java` around lines 150 - 152, The method buildProtocolTitle has an unused Meeting parameter; remove the unused parameter from the method signature (change buildProtocolTitle(Meeting meeting, Registry registry, Integer year) to buildProtocolTitle(Registry registry, Integer year)) and update all callers (notably the call at the reported call site) to stop passing a Meeting argument, adjusting their call to buildProtocolTitle(registry, year); ensure imports/signatures that reference buildProtocolTitle are updated and tests/compilation verified.
🤖 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/MeetingController.java`:
- Line 73: Update the inconsistent success-message capitalization in
MeetingController by changing the model.addAttribute("successMessage",
"sammanträdet skapades.") and the other two occurrences that set
"successMessage" for meeting CUD actions to start with a capital letter (e.g.,
"Sammanträdet skapades.", "Sammanträdet uppdaterades.", "Sammanträdet
raderades."). Locate the three model.addAttribute calls in MeetingController
(the ones that set "successMessage" for create/update/delete of meetings) and
fix the string literals so they match the controller's capitalized message
style.
In `@src/main/java/backendlab/team4you/protocol/ProtocolService.java`:
- Around line 128-148: The retry loop in allocateNextParagraphNumber inside
ProtocolService is ineffective because DataIntegrityViolationException poisons
the outer `@Transactional` in createProtocolForCompletedMeeting; change the design
by either (A) pre-seed a ProtocolParagraphSequence row when a Registry is
created or via an upsert before allocation so
sequenceRepository.findByRegistryIdAndYear(registry.getId(), year) always
returns an existing row and pessimistic locking works, or (B) move
allocateNextParagraphNumber into its own method annotated with
`@Transactional`(propagation = REQUIRES_NEW) so it runs in a separate transaction
that can commit/rollback independently (adjust signature and calls accordingly);
also remove the unused meeting parameter from buildProtocolTitle or use it if
intended.
---
Duplicate comments:
In `@src/main/java/backendlab/team4you/meeting/MeetingController.java`:
- Around line 148-151: Replace the broad catch(Exception) blocks in
deleteMeeting, moveAgendaItemUp, moveAgendaItemDown, addAgendaDocument and
removeAgendaDocument with targeted handling: catch only the specific domain
exceptions you expect (e.g., MeetingNotFoundException,
AgendaItemNotFoundException, DocumentUploadException — use the actual exception
types used elsewhere like in addAgendaItem/removeAgendaItem), set model
attributes for those cases, and rethrow (or let) any other unexpected exceptions
so GlobalViewExceptionHandler can map proper HTTP statuses; also add a log entry
for unexpected errors before rethrowing to preserve observability.
- Around line 59-62: Parsing of startsAt/endsAt via LocalDateTime.parse in the
controller's create and update flows can throw DateTimeParseException which
bypasses the current multi-catch (InvalidMeetingStateException |
MeetingNotFoundException | RegistryNotFoundException); fix by either adding
DateTimeParseException to that multi-catch or (preferred) wrap the
LocalDateTime.parse calls in a small try/catch and rethrow new
InvalidMeetingStateException("Ogiltigt datum-/tidsformat.") so the error is
rendered inline; update the parse sites used in createMeeting and updateMeeting
accordingly and ensure the thrown InvalidMeetingStateException is handled by the
existing form-error flow.
---
Nitpick comments:
In
`@src/main/java/backendlab/team4you/exceptions/GlobalRestExceptionHandler.java`:
- Around line 211-221: The handleFileInUse method duplicates handleConflict;
remove the handleFileInUse(FileInUseException) method and add
FileInUseException.class to the `@ExceptionHandler` annotation on handleConflict
so that handleConflict handles both exceptions (preserving the same
ResponseEntity<ErrorResponseDto> behavior and message). Ensure the method
signature for handleConflict remains unchanged and that FileInUseException is
referenced in the annotation array.
In `@src/main/java/backendlab/team4you/meeting/MeetingController.java`:
- Around line 156-170: The showMeeting method contains a redundant HTMX check:
remove the explicit if (htmx != null) branch and let the method fall through to
meetingsView(htmx) instead; in MeetingController.showMeeting (which currently
calls meetingService.getMeetingById(...) and populateMeetingsPage(...)), delete
the inline return "fragments/admin-meetings :: content" branch so the HTMX
handling is centralized in meetingsView(htmx) like the other handlers.
In `@src/main/java/backendlab/team4you/protocol/ProtocolService.java`:
- Around line 150-152: The method buildProtocolTitle has an unused Meeting
parameter; remove the unused parameter from the method signature (change
buildProtocolTitle(Meeting meeting, Registry registry, Integer year) to
buildProtocolTitle(Registry registry, Integer year)) and update all callers
(notably the call at the reported call site) to stop passing a Meeting argument,
adjusting their call to buildProtocolTitle(registry, year); ensure
imports/signatures that reference buildProtocolTitle are updated and
tests/compilation verified.
In `@src/test/java/backendlab/team4you/protocol/ProtocolServiceTest.java`:
- Around line 81-83: The test currently uses mock(CaseRecord.class) in setUp()
and then passes that mock into MeetingAgendaItem(meeting, firstCaseRecord, ...),
which can break if CaseRecord is made final or Mockito cannot subclass it;
replace the mock usage with a real CaseRecord instance constructed via its
constructor and set its id (and any required fields) with the same reflection
helper used elsewhere (setField(caseRecord, "id", ...)), mirroring how Registry
and Meeting are created in setUp(), so MeetingAgendaItem receives a real JPA
entity instead of a Mockito proxy.
- Around line 85-122: Add tests in ProtocolServiceTest that cover the branch
where sequenceRepository.findByRegistryIdAndYear(...) returns Optional.empty():
call protocolService.createProtocolForCompletedMeeting(...) with a meeting whose
registry/year pair has no existing ProtocolParagraphSequence and assert
paragraphs start at 1L and that sequenceRepository.saveAndFlush(...) is invoked;
additionally add a test that stubs sequenceRepository.saveAndFlush(...) to first
throw DataIntegrityViolationException and then succeed to verify the retry
semantics in ProtocolService#allocateNextParagraphNumber (and that
createProtocolForCompletedMeeting still produces correct numbering), referencing
ProtocolParagraphSequence, sequenceRepository.saveAndFlush, and
ProtocolService#createProtocolForCompletedMeeting to locate the behavior to
exercise.
🪄 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: c094824f-a12a-446f-bb1d-726e0f6f4d32
📒 Files selected for processing (13)
docker-compose.yamlinit-postgres.sqlsrc/main/java/backendlab/team4you/exceptions/GlobalRestExceptionHandler.javasrc/main/java/backendlab/team4you/exceptions/GlobalViewExceptionHandler.javasrc/main/java/backendlab/team4you/exceptions/MeetingNotFoundException.javasrc/main/java/backendlab/team4you/exceptions/ProtocolParagraphNotFoundException.javasrc/main/java/backendlab/team4you/meeting/MeetingController.javasrc/main/java/backendlab/team4you/protocol/ProtocolService.javasrc/main/resources/db/migration/V23__add_cascade_delete_to_protocol_paragraph_fk.sqlsrc/main/resources/static/css/meetings.csssrc/test/java/backendlab/team4you/meeting/MeetingControllerTest.javasrc/test/java/backendlab/team4you/protocol/ProtocolServiceTest.javasrc/test/resources/application-test.properties
✅ Files skipped from review due to trivial changes (1)
- init-postgres.sql
🚧 Files skipped from review as they are similar to previous changes (5)
- src/main/java/backendlab/team4you/exceptions/MeetingNotFoundException.java
- src/main/java/backendlab/team4you/exceptions/ProtocolParagraphNotFoundException.java
- docker-compose.yaml
- src/main/resources/static/css/meetings.css
- src/test/java/backendlab/team4you/meeting/MeetingControllerTest.java
…oned after DataIntegrityViolationException.
gvaguirres
previously approved these changes
Apr 27, 2026
MartinStenhagen
dismissed
gvaguirres’s stale review
April 27, 2026 15:46
The merge-base changed after approval.
gvaguirres
previously approved these changes
Apr 27, 2026
JohanHiths
previously approved these changes
Apr 27, 2026
MartinStenhagen
dismissed stale reviews from JohanHiths and gvaguirres
April 27, 2026 15:52
The merge-base changed after approval.
gvaguirres
previously approved these changes
Apr 27, 2026
JohanHiths
previously approved these changes
Apr 27, 2026
MartinStenhagen
dismissed stale reviews from JohanHiths and gvaguirres
April 27, 2026 16:00
The merge-base changed after approval.
MartinStenhagen
force-pushed
the
feature/meeting-protocol-generation
branch
from
April 27, 2026 18:10
e0f23e1 to
ca02933
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/java/backendlab/team4you/casefile/CaseFileService.java (1)
174-194:⚠️ Potential issue | 🟠 MajorDon't swallow S3 deletion failures here.
Returning success after the DB row is deleted but the S3 object could not be removed hides partial failures and can leave orphaned files behind. Please add a compensating retry/outbox path or propagate a domain-specific failure so callers can react appropriately.
🤖 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 174 - 194, The deleteFile method currently deletes the DB row (caseFileRepository.delete) and then swallows S3 failures in the catch block, which can leave orphaned S3 objects; modify deleteFile to not hide S3 deletion failures by implementing a compensating path: after deleting the DB row, attempt s3Service.deleteFile(s3Key) and on failure either (A) enqueue a retry/outbox task (e.g., call an existing OutboxService.enqueueFileDeletion(s3Key, fileId) or create one) so a background worker retries deletion, or (B) throw a domain-specific runtime exception (e.g., FileDeletionFailedException) carrying s3Key and fileId so callers can handle it; ensure you remove the empty catch and either rethrow or invoke the outbox API from deleteFile and log the outcome (reference methods: deleteFile, s3Service.deleteFile, caseFileRepository.delete, meetingAgendaDocumentRepository.existsByCaseFileId).
♻️ Duplicate comments (2)
src/main/java/backendlab/team4you/meeting/MeetingController.java (2)
58-62:⚠️ Potential issue | 🟠 MajorHandle malformed timestamps in both POST handlers.
LocalDateTime.parse(...)can still throwDateTimeParseException, so a bad direct POST will bypass the inline error flow and surface as the generic error page. Catch it here and inupdateMeeting, then repopulate the form with a user-facing message.🛠️ Suggested fix
try { LocalDateTime parsedStartsAt = LocalDateTime.parse(startsAt); LocalDateTime parsedEndsAt = (endsAt == null || endsAt.isBlank()) ? null : LocalDateTime.parse(endsAt); + } catch (DateTimeParseException exception) { + model.addAttribute("errorMessage", "Ogiltigt datum-/tidsformat."); + Long safeRegistryId = (registryId != null && registryRepository.existsById(registryId)) + ? registryId + : null; + populateMeetingsPage(model, safeRegistryId, null); } catch (InvalidMeetingStateException | MeetingNotFoundException | RegistryNotFoundException exception) {Also applies to: 100-104
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/meeting/MeetingController.java` around lines 58 - 62, The LocalDateTime.parse calls (producing parsedStartsAt and parsedEndsAt) can throw DateTimeParseException and must be handled in both POST handlers (createMeeting and updateMeeting); wrap the parsing logic in a try/catch that catches DateTimeParseException, add a user-facing error message to the model (e.g., "Invalid date/time format for startsAt/endsAt"), repopulate the form fields with the original input values so the user can correct them, and return the same view used for form rendering instead of letting the exception bubble to the generic error page.
148-151:⚠️ Potential issue | 🟠 MajorStop swallowing unexpected exceptions in action handlers.
catch (Exception ...)masks real failures and turns them into a generic UI error. Narrow these blocks to the concrete domain exceptionsMeetingServiceactually throws; let everything else bubble to the global handler.Also applies to: 212-215, 235-238, 284-287, 308-311
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/backendlab/team4you/meeting/MeetingController.java` around lines 148 - 151, The current handlers catch a broad Exception which hides real failures; replace each catch (Exception exception) in MeetingController (the blocks that call populateMeetingsPage(...)) with catches for the concrete domain exceptions the MeetingService can throw (for example MeetingNotFoundException, MeetingValidationException or whatever specific exceptions your MeetingService API declares), set model.addAttribute("errorMessage", ex.getMessage()) for those specific exceptions, and let any other unexpected exceptions propagate (remove the generic catch or rethrow them) so the global exception handler can handle them; apply this change to the similar catch sites that wrap calls to MeetingService and call populateMeetingsPage (the blocks at the other listed locations) so only domain errors are turned into UI error messages.
🤖 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/protocol/ProtocolService.java`:
- Around line 93-106: The method updateParagraphDecision currently only
validates paragraphId; add a null-check for decisionType at the service boundary
by calling Objects.requireNonNull(decisionType, "decisionType is required") at
the start of updateParagraphDecision so null DecisionTypes fail fast and
consistently before invoking paragraph.updateDecision(decisionType,
decisionText) (reference ProtocolDecisionType and the updateParagraphDecision
method).
- Around line 48-76: Wrap the call to protocolRepository.save(protocol) in a
try-catch that catches org.springframework.dao.DataIntegrityViolationException
and rethrows new ProtocolAlreadyExistsException(meetingId) to translate DB
unique-constraint races into the same HTTP 409 behavior as the existsByMeetingId
check; follow the same pattern used in RegistryService.createRegistry(), leaving
the rest of the method intact and only adding the try-catch around
protocolRepository.save.
---
Outside diff comments:
In `@src/main/java/backendlab/team4you/casefile/CaseFileService.java`:
- Around line 174-194: The deleteFile method currently deletes the DB row
(caseFileRepository.delete) and then swallows S3 failures in the catch block,
which can leave orphaned S3 objects; modify deleteFile to not hide S3 deletion
failures by implementing a compensating path: after deleting the DB row, attempt
s3Service.deleteFile(s3Key) and on failure either (A) enqueue a retry/outbox
task (e.g., call an existing OutboxService.enqueueFileDeletion(s3Key, fileId) or
create one) so a background worker retries deletion, or (B) throw a
domain-specific runtime exception (e.g., FileDeletionFailedException) carrying
s3Key and fileId so callers can handle it; ensure you remove the empty catch and
either rethrow or invoke the outbox API from deleteFile and log the outcome
(reference methods: deleteFile, s3Service.deleteFile, caseFileRepository.delete,
meetingAgendaDocumentRepository.existsByCaseFileId).
---
Duplicate comments:
In `@src/main/java/backendlab/team4you/meeting/MeetingController.java`:
- Around line 58-62: The LocalDateTime.parse calls (producing parsedStartsAt and
parsedEndsAt) can throw DateTimeParseException and must be handled in both POST
handlers (createMeeting and updateMeeting); wrap the parsing logic in a
try/catch that catches DateTimeParseException, add a user-facing error message
to the model (e.g., "Invalid date/time format for startsAt/endsAt"), repopulate
the form fields with the original input values so the user can correct them, and
return the same view used for form rendering instead of letting the exception
bubble to the generic error page.
- Around line 148-151: The current handlers catch a broad Exception which hides
real failures; replace each catch (Exception exception) in MeetingController
(the blocks that call populateMeetingsPage(...)) with catches for the concrete
domain exceptions the MeetingService can throw (for example
MeetingNotFoundException, MeetingValidationException or whatever specific
exceptions your MeetingService API declares), set
model.addAttribute("errorMessage", ex.getMessage()) for those specific
exceptions, and let any other unexpected exceptions propagate (remove the
generic catch or rethrow them) so the global exception handler can handle them;
apply this change to the similar catch sites that wrap calls to MeetingService
and call populateMeetingsPage (the blocks at the other listed locations) so only
domain errors are turned into UI error messages.
🪄 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: 36ec4e47-54e5-4cb1-8a95-2ec003a11978
📒 Files selected for processing (6)
src/main/java/backendlab/team4you/casefile/CaseFileService.javasrc/main/java/backendlab/team4you/meeting/MeetingController.javasrc/main/java/backendlab/team4you/protocol/ProtocolParagraphSequenceRepository.javasrc/main/java/backendlab/team4you/protocol/ProtocolService.javasrc/test/java/backendlab/team4you/casefile/CaseFileServiceTest.javasrc/test/java/backendlab/team4you/meeting/MeetingControllerTest.java
🚧 Files skipped from review as they are similar to previous changes (3)
- src/test/java/backendlab/team4you/casefile/CaseFileServiceTest.java
- src/main/java/backendlab/team4you/protocol/ProtocolParagraphSequenceRepository.java
- src/test/java/backendlab/team4you/meeting/MeetingControllerTest.java
gvaguirres
approved these changes
Apr 27, 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
3 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.
resolves #48
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Improvements