feature/adding-Thymeleaf-frontend#11
Merged
Merged
Conversation
📝 WalkthroughWalkthroughThe PR adds document management capabilities to the application by introducing MinIO S3 storage infrastructure, web UI for document listing and upload/deletion operations, frontend drag-and-drop file handling, and supporting service layer enhancements. Configuration is updated to point to a locally-running MinIO server. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User/Browser
participant TVC as TicketViewController
participant DS as DocumentService
participant S3 as MinIO S3
User->>User: Select/drag file in upload form
User->>TVC: POST /tickets/{id}/documents/upload<br/>(multipart file)
TVC->>TVC: Extract Principal (current user)
TVC->>DS: uploadDocument(file, principal)
DS->>S3: Upload file to S3 bucket
S3-->>DS: File stored
DS->>DS: Create Document entity<br/>with S3 reference
DS-->>TVC: Document saved / RuntimeException
TVC->>TVC: Set flash attribute<br/>(success or error)
TVC-->>User: Redirect to ticket detail
User->>User: Flash message displayed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Nitpick comments (12)
src/main/java/org/group1/projectbackend/service/DocumentService.java (1)
12-12: Consider pagination forgetAllDocuments()before the dataset grows.Returning every document in one response is fine for a demo, but once the system accumulates uploads this will load the whole table into memory and render one giant HTML list. Consider exposing a
Pageable/paged variant (e.g.Page<DocumentResponse> getAllDocuments(Pageable pageable)) before this ships to anything production-like.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/group1/projectbackend/service/DocumentService.java` at line 12, Replace the unpaged getAllDocuments() API with a paged variant to avoid loading the entire table into memory: change the DocumentService signature List<DocumentResponse> getAllDocuments() to a pageable form such as Page<DocumentResponse> getAllDocuments(Pageable pageable), update any implementing class (e.g., DocumentServiceImpl.getAllDocuments) to accept the Pageable and return a Page by delegating to the repository method that returns Page<Document> (e.g., documentRepository.findAll(pageable)) and map entities to DocumentResponse, and update callers/controllers to accept pageable parameters (or provide defaults) and propagate the Page<DocumentResponse> to the client.src/main/java/org/group1/projectbackend/service/impl/DocumentServiceImpl.java (1)
88-91: Annotate read-only service methods with@Transactional(readOnly = true).
getAllDocuments()accesses lazy-loaded associations (uploadedByandticket) viaDocumentMapper.toResponseList(). Without an active transaction,LazyInitializationExceptionwill occur if the Hibernate session closes before mapping completes—particularly whenspring.jpa.open-in-viewis disabled. Adding the read-only annotation ensures the session remains open during the mapping operation and enables driver-level optimizations.♻️ Proposed change
`@Override` + `@Transactional`(readOnly = true) public List<DocumentResponse> getAllDocuments() { return DocumentMapper.toResponseList(documentRepository.findAll()); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/group1/projectbackend/service/impl/DocumentServiceImpl.java` around lines 88 - 91, getAllDocuments in DocumentServiceImpl calls DocumentMapper.toResponseList which accesses lazy associations (uploadedBy, ticket) and can throw LazyInitializationException when there's no active session; annotate the method with `@Transactional`(readOnly = true) on the getAllDocuments method in DocumentServiceImpl so a read-only transactional boundary is opened during mapping, ensuring the Hibernate session stays open and enabling read optimizations.src/main/resources/templates/tickets/list.html (1)
42-42: Keep accessor syntax consistent with neighbouring fields.Lines 38–41 all use record-accessor syntax (
ticket.id(),ticket.title(),ticket.status(),ticket.priority()), but line 42 switches to property-styleticket.createdAt. Both syntaxes work with Thymeleaf's#temporals.format()utility, but the mixed style is inconsistent.🎨 Suggested change
- <td th:text="${`#temporals.format`(ticket.createdAt, 'yyyy-MM-dd HH:mm')}">2026-04-22T12:00</td> + <td th:text="${`#temporals.format`(ticket.createdAt(), 'yyyy-MM-dd HH:mm')}">2026-04-22T12:00</td>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/tickets/list.html` at line 42, The template mixes record-accessor and property syntax: change the createdAt usage to the same record-accessor style as the neighbouring fields by calling ticket.createdAt() inside the `#temporals.format` call (i.e., use `#temporals.format`(ticket.createdAt(), 'yyyy-MM-dd HH:mm')) so the template consistently uses ticket.id(), ticket.title(), ticket.status(), ticket.priority(), and ticket.createdAt().src/main/resources/templates/tickets/detail.html (2)
67-67: Move layout styling out of inlinestyleattribute.Both new sections use
class="section-wide" style="margin-top: 2rem". The.section-wideclass is already intended for this styling; add themargin-topto the CSS rule (see the related bug inapp.csswheresection-wideis missing its leading.) and drop the inlinestyleattributes.Also applies to: 93-93
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/tickets/detail.html` at line 67, Move the inline margin-top styling into the CSS: update the CSS rule for the class section-wide (fix the bug where the selector is missing the leading dot so it reads ".section-wide") and add margin-top: 2rem there; then remove the style="margin-top: 2rem" attributes from the <section class="section-wide"> occurrences in the tickets/detail.html template (both places currently using that inline style) so layout is controlled by the .section-wide class only.
35-38: Record accessor style is inconsistent.
ticket.createdAt/ticket.updatedAtare used here without parentheses, while every other access in this template (e.g.ticket.title(),ticket.id(), anddocument.createdAt()at line 143) uses record-accessor style. Prefer one style for consistency.✏️ Proposed fix
- <dd th:text="${`#temporals.format`(ticket.createdAt, 'yyyy-MM-dd HH:mm')}"></dd> + <dd th:text="${`#temporals.format`(ticket.createdAt(), 'yyyy-MM-dd HH:mm')}"></dd> ... - <dd th:text="${`#temporals.format`(ticket.updatedAt, 'yyyy-MM-dd HH:mm')}"></dd> + <dd th:text="${`#temporals.format`(ticket.updatedAt(), 'yyyy-MM-dd HH:mm')}"></dd>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/tickets/detail.html` around lines 35 - 38, The template mixes record field access and accessor style; change ticket.createdAt and ticket.updatedAt to use the record-accessor form ticket.createdAt() and ticket.updatedAt() so they match other usages like ticket.title() and ticket.id(); update the two th:text expressions (the `#temporals.format` calls that currently use ticket.createdAt and ticket.updatedAt) to call the accessor methods instead.src/main/java/org/group1/projectbackend/controller/web/DocumentViewController.java (1)
31-36: Log the exception instead of swallowing it.On the error path the exception is silently discarded, so operators and developers have no trace when deletes fail. Use an SLF4J logger and log
exat WARN/ERROR.♻️ Proposed change
+import org.slf4j.Logger; +import org.slf4j.LoggerFactory; ... public class DocumentViewController { + private static final Logger log = LoggerFactory.getLogger(DocumentViewController.class); private final DocumentService documentService; ... } catch (RuntimeException ex) { + log.warn("Failed to delete document {}", documentId, ex); redirectAttributes.addFlashAttribute("documentError", "Dokumentet kunde inte tas bort."); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/org/group1/projectbackend/controller/web/DocumentViewController.java` around lines 31 - 36, The catch block in DocumentViewController currently swallows exceptions from documentService.deleteDocument(documentId); add an SLF4J logger (e.g., private static final Logger log = LoggerFactory.getLogger(DocumentViewController.class)) and log the caught exception (ex) at WARN or ERROR inside the catch before setting redirectAttributes.addFlashAttribute("documentError", ...), so the user-facing flash remains but the exception is recorded for operators and developers.compose.yml (3)
19-31: Bucketproject-documentsis not auto-created.
application.propertiesconfiguresstorage.s3.bucket=project-documents, but nothing in this compose file creates it. Freshdocker compose upwill leave the app throwing on the first upload until a developer manually creates the bucket in the MinIO console. Consider a one-shotmcsidecar to bootstrap it, or document the manual step clearly in the README.♻️ Proposed sidecar
minio-init: image: minio/mc depends_on: - minio entrypoint: > /bin/sh -c " until /usr/bin/mc alias set local http://minio:9000 \"$${MINIO_ROOT_USER:-minioadmin}\" \"$${MINIO_ROOT_PASSWORD:-minioadmin}\"; do sleep 2; done; /usr/bin/mc mb --ignore-existing local/project-documents; "🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compose.yml` around lines 19 - 31, The compose file starts MinIO (service name minio) but never auto-creates the configured S3 bucket (storage.s3.bucket=project-documents), so add a one-shot sidecar service (e.g., minio-init) that depends_on the minio service, uses the minio/mc image, and runs a small shell entrypoint which sets the mc alias pointing at the minio service using the existing MINIO_ROOT_USER/MINIO_ROOT_PASSWORD env vars and then runs mc mb --ignore-existing local/project-documents to create the bucket; alternatively, if you prefer not to modify compose, add a README step documenting that developers must manually create the project-documents bucket in the MinIO console before first upload.
23-25: Indentation inconsistency underenvironment.The keys are indented with 8 spaces while the
postgresservice uses 6 spaces. YAML accepts either, but matching the surrounding style keeps the file tidy.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compose.yml` around lines 23 - 25, The environment mapping for the MinIO service uses 8-space indentation which differs from the 6-space indentation used elsewhere (e.g., the postgres service); align the indentation of the environment block and its keys (MINIO_ROOT_USER and MINIO_ROOT_PASSWORD) to match the surrounding style (6 spaces) so the YAML is consistent.
19-31: Add a healthcheck for MinIO.The
postgresservice has a healthcheck; MinIO should too so dependents (and futuredepends_on: condition: service_healthy) can rely on readiness.♻️ Proposed addition
command: server /data --console-address ":9001" + healthcheck: + test: ["CMD", "curl", "-f", "http://localhost:9000/minio/health/live"] + interval: 10s + timeout: 5s + retries: 5🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@compose.yml` around lines 19 - 31, Add a Docker healthcheck to the minio service so other services can wait for readiness: in the minio service block (service name "minio") add a healthcheck that queries MinIO's readiness endpoint (e.g., HTTP GET to /minio/health/ready on the API port) using a command like curl -f, with sensible interval/timeout/retries/start_period values so the container only becomes healthy when MinIO is ready; keep the existing image, ports, volumes and command intact and ensure the healthcheck uses the same API port (9000 / ${MINIO_API_PORT}) and returns non-zero on failure.src/main/resources/static/js/document-upload.js (2)
12-14: Silent early-return hides template misconfiguration.If
fileInputis missing (which is currently the case — see the template review), the script returns without any signal. Logging aconsole.warnwould make this class of regression immediately visible during development.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/document-upload.js` around lines 12 - 14, The current early-return in the initialization (checking dropzone, fileInput, selectionText) silently hides missing DOM elements; update the condition in the top of the init block that checks dropzone, fileInput, and selectionText to emit a clear console.warn (including which element is missing, e.g., "fileInput missing") before returning so template misconfiguration is visible during development—target the existing identifiers dropzone, fileInput, selectionText and log their presence/absence then return unchanged.
41-50: File extension heuristic treats all non-JPEG images as PNG.Pasted clipboard images can legitimately be
image/webp,image/gif, etc. Thejpg/pngbranch will mislabel those. Consider deriving the extension fromblob.type:♻️ Proposed refactor
function createScreenshotFile(blob) { - const extension = blob.type === "image/jpeg" ? "jpg" : "png"; + const mime = blob.type || "image/png"; + const extension = mime === "image/jpeg" ? "jpg" : mime.split("/")[1] || "png"; const timestamp = new Date() .toISOString() .replace(/[-:]/g, "") .replace("T", "-") .slice(0, 15); - return new File([blob], `screenshot-${timestamp}.${extension}`, { type: blob.type || "image/png" }); + return new File([blob], `screenshot-${timestamp}.${extension}`, { type: mime }); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/static/js/document-upload.js` around lines 41 - 50, In createScreenshotFile, don't assume non-JPEG is PNG; derive the file extension from blob.type (e.g., const mime = blob.type || "image/png"; const ext = mime.split("/")[1] with special-case "jpeg" -> "jpg") and use that ext in the filename while preserving the blob.type when creating the new File; also ensure a safe fallback to "png" if blob.type is empty or malformed.src/main/resources/templates/documents/list.html (1)
46-46: Consider human-readable file size.Rendering
fileSize()as raw bytes (e.g.1048576) is unfriendly. A Thymeleaf utility or formatted server-side field (KB/MB) would improve readability. Optional.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/resources/templates/documents/list.html` at line 46, The template currently renders raw bytes via document.fileSize(); replace that with a human-readable value by adding a formatting accessor on the model (e.g., Document.getHumanReadableFileSize() or getReadableSize()) that converts bytes into KB/MB/GB with appropriate units and use th:text="${document.humanReadableFileSize}" in the template; implement the formatting logic in the Document class (or a small util used by Document) and ensure the new accessor is populated/accessible to the Thymeleaf template instead of calling document.fileSize().
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@compose.yml`:
- Line 20: Replace the deprecated Docker image reference "minio/minio" in the
compose.yml service definition with an actively maintained MinIO image (for
example "quay.io/minio/aistor/minio") or a documented community/mirror
alternative, and update the compose service's image field accordingly; also add
a short comment near the image declaration explaining which image was chosen and
the maintenance plan (source, update cadence, and owner) so reviewers know this
is a supported, actively maintained choice.
In `@README.md`:
- Around line 87-92: Add a heading (e.g., "Starta MinIO med Docker") before the
docker command and wrap the multi-line docker run snippet in a fenced bash code
block so the backslash line continuations and $ prompt render correctly; replace
the raw snippet shown in the README.md with a proper ```bash fenced block
containing the docker run command and add a short sentence pointing users to the
existing compose.yml / minio service as the preferred "docker compose up -d"
option.
In
`@src/main/java/org/group1/projectbackend/controller/web/DocumentViewController.java`:
- Around line 26-39: Add a TODO and a short inline authorization check note to
prevent unrestricted deletes in deleteDocument: in
DocumentViewController.deleteDocument, before calling
documentService.deleteDocument(documentId) add a comment (similar to the one
used in TicketViewController.createComment) that this is a temporary open
endpoint and must be replaced with an ownership/role check when auth is wired
in; implement the real fix by verifying current user ownership/role via a
service method (e.g., documentService.isOwnerOrAdmin(documentId, currentUser) or
an equivalent security utility) and only call
documentService.deleteDocument(documentId) when that check passes, otherwise set
documentError and avoid deletion.
In
`@src/main/java/org/group1/projectbackend/controller/web/TicketViewController.java`:
- Around line 110-122: Add a null-check for the Principal at the start of the
uploadDocument handler (the method using Principal and RedirectAttributes) and
if principal is null set a flash error on RedirectAttributes (mirroring
TicketViewController.createComment's "auth temporarily missing" behavior) then
return a redirect response instead of proceeding; specifically guard before
calling principal.getName(), and only call userRepository.findByUsername(...)
when principal is non-null to avoid NPEs.
- Around line 134-148: The delete endpoint deleteDocument(Long ticketId, Long
documentId, RedirectAttributes) currently ignores ticketId and calls
documentService.deleteDocument(documentId); change it to first load the document
(e.g., documentService.findById or documentService.getDocument(documentId)) and
verify its parent ticket's id equals the provided ticketId before calling
documentService.deleteDocument; if the document is missing or the ticketIds
don't match, set the "documentError" flash attribute and do not delete,
otherwise proceed to delete and set "documentSuccess". Ensure you reference the
controller method deleteDocument and the service methods
documentService.findById/getDocument and documentService.deleteDocument when
making the change.
- Around line 113-119: Replace the System.out.println calls (and any
printStackTrace usage) in TicketViewController's uploadDocument method with an
SLF4J logger: add a private static final Logger (or use existing logger) in
TicketViewController and log the principal, id, file metadata
(file.getOriginalFilename(), file.isEmpty(), file.getSize()) at an appropriate
level (debug/info), and log exceptions with logger.error when catching
exceptions instead of printStackTrace; update any similar prints around the same
method (including the occurrence at the later line noted) to use the logger
consistently.
- Line 128: Replace the user-facing flash that currently appends ex.getMessage()
with a stable, non-sensitive message (e.g., "Dokumentet kunde inte laddas upp.
Försök igen senare.") in TicketViewController where
redirectAttributes.addFlashAttribute("documentError", ...) is used, and
separately log the full exception server-side (e.g., using the controller's
logger and logging ex) so internal details are preserved for debugging but not
shown to the user; ensure you remove direct exposure of ex.getMessage() and keep
the log call close to the catch block that references ex.
In `@src/main/resources/application.properties`:
- Around line 21-26: Replace the hardcoded MinIO properties
(storage.s3.endpoint, storage.s3.region, storage.s3.access-key,
storage.s3.secret-key, storage.s3.bucket, storage.s3.path-style-access-enabled)
with environment-variable-style placeholders that supply local defaults (e.g.
use ${MINIO_ENDPOINT:-http://localhost:9000}, ${MINIO_REGION:-us-east-1},
${MINIO_ROOT_USER:-minioadmin}, ${MINIO_ROOT_PASSWORD:-minioadmin},
${MINIO_BUCKET:-project-documents}, ${STORAGE_S3_PATH_STYLE_ENABLED:-true}) so
credentials and endpoint can be overridden per environment; also add
STORAGE_S3_ACCESS_KEY and STORAGE_S3_SECRET_KEY to .env.example (empty values)
to document the override path.
In `@src/main/resources/static/css/app.css`:
- Around line 410-412: Change the bogus type selector "section-wide" to a class
selector ".section-wide" so it targets elements with class="section-wide"
(update the rule named section-wide to .section-wide in the CSS), and move the
inline margin-top used in tickets/detail.html into this .section-wide rule so
the padding and top-margin are consolidated into the .section-wide class (ensure
you keep padding: 1.15rem and add the former margin-top value).
In `@src/main/resources/templates/documents/list.html`:
- Around line 17-19: Update the user-facing copy in the documents list template:
replace the incorrect product name "MiniIO" with the correct "MinIO" in the text
block inside the template (look for the string "MiniIO" in
src/main/resources/templates/documents/list.html and change it to "MinIO").
In `@src/main/resources/templates/index.html`:
- Line 37: Replace the incorrect separated Swedish noun "Dokument metadata"
inside the index.html template paragraph (the string currently reading "Dokument
metadata och S3-kompatibel lagring finns för vidare demo och utveckling.") with
the compound "Dokumentmetadata" so the sentence reads "Dokumentmetadata och
S3-kompatibel lagring finns för vidare demo och utveckling." to fix the
särskrivning.
In `@src/main/resources/templates/tickets/detail.html`:
- Around line 106-118: The dropzone element (data-document-dropzone) lists
aria-describedby="document-upload-selection document-upload-hint" but there is
no element with id "document-upload-hint"; either remove the stale reference
from the aria-describedby on the document-dropzone or add a new descriptive
element with id="document-upload-hint" (for example a <p class="muted
document-upload-hint"> describing accepted file types/size and paste behavior)
and ensure it is referenced by aria-describedby alongside the existing
`#document-upload-selection`; update whichever approach you choose to keep the IDs
and class names (document-dropzone, document-upload-selection,
document-upload-hint) consistent.
- Around line 99-122: The upload form is missing the actual file input so
document-upload.js (which queries "[data-document-file-input]") returns null and
the controller TicketViewController.uploadDocument's `@RequestParam`("file")
fails; add an <input> file element to the form with id="file", name="file",
type="file" and the attribute data-document-file-input so the label (for="file")
and aria-controls="file" target a real element and JS can wire click/drag/paste
behavior; ensure the input is inside the form with enctype="multipart/form-data"
and include any needed attributes (e.g., accept or hidden class) consistent with
the dropzone UX.
---
Nitpick comments:
In `@compose.yml`:
- Around line 19-31: The compose file starts MinIO (service name minio) but
never auto-creates the configured S3 bucket
(storage.s3.bucket=project-documents), so add a one-shot sidecar service (e.g.,
minio-init) that depends_on the minio service, uses the minio/mc image, and runs
a small shell entrypoint which sets the mc alias pointing at the minio service
using the existing MINIO_ROOT_USER/MINIO_ROOT_PASSWORD env vars and then runs mc
mb --ignore-existing local/project-documents to create the bucket;
alternatively, if you prefer not to modify compose, add a README step
documenting that developers must manually create the project-documents bucket in
the MinIO console before first upload.
- Around line 23-25: The environment mapping for the MinIO service uses 8-space
indentation which differs from the 6-space indentation used elsewhere (e.g., the
postgres service); align the indentation of the environment block and its keys
(MINIO_ROOT_USER and MINIO_ROOT_PASSWORD) to match the surrounding style (6
spaces) so the YAML is consistent.
- Around line 19-31: Add a Docker healthcheck to the minio service so other
services can wait for readiness: in the minio service block (service name
"minio") add a healthcheck that queries MinIO's readiness endpoint (e.g., HTTP
GET to /minio/health/ready on the API port) using a command like curl -f, with
sensible interval/timeout/retries/start_period values so the container only
becomes healthy when MinIO is ready; keep the existing image, ports, volumes and
command intact and ensure the healthcheck uses the same API port (9000 /
${MINIO_API_PORT}) and returns non-zero on failure.
In
`@src/main/java/org/group1/projectbackend/controller/web/DocumentViewController.java`:
- Around line 31-36: The catch block in DocumentViewController currently
swallows exceptions from documentService.deleteDocument(documentId); add an
SLF4J logger (e.g., private static final Logger log =
LoggerFactory.getLogger(DocumentViewController.class)) and log the caught
exception (ex) at WARN or ERROR inside the catch before setting
redirectAttributes.addFlashAttribute("documentError", ...), so the user-facing
flash remains but the exception is recorded for operators and developers.
In `@src/main/java/org/group1/projectbackend/service/DocumentService.java`:
- Line 12: Replace the unpaged getAllDocuments() API with a paged variant to
avoid loading the entire table into memory: change the DocumentService signature
List<DocumentResponse> getAllDocuments() to a pageable form such as
Page<DocumentResponse> getAllDocuments(Pageable pageable), update any
implementing class (e.g., DocumentServiceImpl.getAllDocuments) to accept the
Pageable and return a Page by delegating to the repository method that returns
Page<Document> (e.g., documentRepository.findAll(pageable)) and map entities to
DocumentResponse, and update callers/controllers to accept pageable parameters
(or provide defaults) and propagate the Page<DocumentResponse> to the client.
In
`@src/main/java/org/group1/projectbackend/service/impl/DocumentServiceImpl.java`:
- Around line 88-91: getAllDocuments in DocumentServiceImpl calls
DocumentMapper.toResponseList which accesses lazy associations (uploadedBy,
ticket) and can throw LazyInitializationException when there's no active
session; annotate the method with `@Transactional`(readOnly = true) on the
getAllDocuments method in DocumentServiceImpl so a read-only transactional
boundary is opened during mapping, ensuring the Hibernate session stays open and
enabling read optimizations.
In `@src/main/resources/static/js/document-upload.js`:
- Around line 12-14: The current early-return in the initialization (checking
dropzone, fileInput, selectionText) silently hides missing DOM elements; update
the condition in the top of the init block that checks dropzone, fileInput, and
selectionText to emit a clear console.warn (including which element is missing,
e.g., "fileInput missing") before returning so template misconfiguration is
visible during development—target the existing identifiers dropzone, fileInput,
selectionText and log their presence/absence then return unchanged.
- Around line 41-50: In createScreenshotFile, don't assume non-JPEG is PNG;
derive the file extension from blob.type (e.g., const mime = blob.type ||
"image/png"; const ext = mime.split("/")[1] with special-case "jpeg" -> "jpg")
and use that ext in the filename while preserving the blob.type when creating
the new File; also ensure a safe fallback to "png" if blob.type is empty or
malformed.
In `@src/main/resources/templates/documents/list.html`:
- Line 46: The template currently renders raw bytes via document.fileSize();
replace that with a human-readable value by adding a formatting accessor on the
model (e.g., Document.getHumanReadableFileSize() or getReadableSize()) that
converts bytes into KB/MB/GB with appropriate units and use
th:text="${document.humanReadableFileSize}" in the template; implement the
formatting logic in the Document class (or a small util used by Document) and
ensure the new accessor is populated/accessible to the Thymeleaf template
instead of calling document.fileSize().
In `@src/main/resources/templates/tickets/detail.html`:
- Line 67: Move the inline margin-top styling into the CSS: update the CSS rule
for the class section-wide (fix the bug where the selector is missing the
leading dot so it reads ".section-wide") and add margin-top: 2rem there; then
remove the style="margin-top: 2rem" attributes from the <section
class="section-wide"> occurrences in the tickets/detail.html template (both
places currently using that inline style) so layout is controlled by the
.section-wide class only.
- Around line 35-38: The template mixes record field access and accessor style;
change ticket.createdAt and ticket.updatedAt to use the record-accessor form
ticket.createdAt() and ticket.updatedAt() so they match other usages like
ticket.title() and ticket.id(); update the two th:text expressions (the
`#temporals.format` calls that currently use ticket.createdAt and
ticket.updatedAt) to call the accessor methods instead.
In `@src/main/resources/templates/tickets/list.html`:
- Line 42: The template mixes record-accessor and property syntax: change the
createdAt usage to the same record-accessor style as the neighbouring fields by
calling ticket.createdAt() inside the `#temporals.format` call (i.e., use
`#temporals.format`(ticket.createdAt(), 'yyyy-MM-dd HH:mm')) so the template
consistently uses ticket.id(), ticket.title(), ticket.status(),
ticket.priority(), and ticket.createdAt().
🪄 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: 79482728-692e-4cd5-8b7c-56a2bb29e601
📒 Files selected for processing (15)
README.mdcompose.ymlsrc/main/java/org/group1/projectbackend/controller/web/DocumentViewController.javasrc/main/java/org/group1/projectbackend/controller/web/TicketViewController.javasrc/main/java/org/group1/projectbackend/service/DocumentService.javasrc/main/java/org/group1/projectbackend/service/impl/DocumentServiceImpl.javasrc/main/resources/application.propertiessrc/main/resources/static/css/app.csssrc/main/resources/static/js/document-upload.jssrc/main/resources/templates/documents/list.htmlsrc/main/resources/templates/fragments/layout.htmlsrc/main/resources/templates/index.htmlsrc/main/resources/templates/tickets/detail.htmlsrc/main/resources/templates/tickets/list.htmlsrc/main/resources/templates/tickets/new.html
Comment on lines
+26
to
+39
| @PostMapping("/documents/{documentId}/delete") | ||
| public String deleteDocument( | ||
| @PathVariable Long documentId, | ||
| RedirectAttributes redirectAttributes | ||
| ) { | ||
| try { | ||
| documentService.deleteDocument(documentId); | ||
| redirectAttributes.addFlashAttribute("documentSuccess", "Dokumentet togs bort."); | ||
| } catch (RuntimeException ex) { | ||
| redirectAttributes.addFlashAttribute("documentError", "Dokumentet kunde inte tas bort."); | ||
| } | ||
|
|
||
| return "redirect:/documents"; | ||
| } |
There was a problem hiding this comment.
Authorization gap: any caller can delete any document.
deleteDocument accepts a raw document id with no ownership/role check. Consistent with the existing "auth not yet implemented" pattern in this repo, but worth a TODO so this doesn't go live as-is when Spring Security is wired in.
Based on learnings from TicketViewController.createComment (PR #10), document the temporary nature here too.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/group1/projectbackend/controller/web/DocumentViewController.java`
around lines 26 - 39, Add a TODO and a short inline authorization check note to
prevent unrestricted deletes in deleteDocument: in
DocumentViewController.deleteDocument, before calling
documentService.deleteDocument(documentId) add a comment (similar to the one
used in TicketViewController.createComment) that this is a temporary open
endpoint and must be replaced with an ownership/role check when auth is wired
in; implement the real fix by verifying current user ownership/role via a
service method (e.g., documentService.isOwnerOrAdmin(documentId, currentUser) or
an equivalent security utility) and only call
documentService.deleteDocument(documentId) when that check passes, otherwise set
documentError and avoid deletion.
Comment on lines
+110
to
+122
| Principal principal, | ||
| RedirectAttributes redirectAttributes | ||
| ) { | ||
| try { | ||
| System.out.println("uploadDocument endpoint reached"); | ||
| System.out.println("Principal: " + principal); | ||
| System.out.println("Ticket id: " + id); | ||
| System.out.println("File original name: " + file.getOriginalFilename()); | ||
| System.out.println("File empty: " + file.isEmpty()); | ||
| System.out.println("File size: " + file.getSize()); | ||
|
|
||
| User user = userRepository.findByUsername(principal.getName()) | ||
| .orElseThrow(() -> new ResourceNotFoundException("User not found with username: " + principal.getName())); |
There was a problem hiding this comment.
Null-safety on Principal.
Until Spring Security is in place, an unauthenticated request would make principal null, NPEing at principal.getName(). Consider guarding explicitly and redirecting with a flash error, mirroring the existing "auth temporarily missing" pattern called out in prior learnings.
Based on learnings from TicketViewController.createComment (PR #10), this is intentionally temporary — still worth a null check so stray requests don't produce 500s.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/group1/projectbackend/controller/web/TicketViewController.java`
around lines 110 - 122, Add a null-check for the Principal at the start of the
uploadDocument handler (the method using Principal and RedirectAttributes) and
if principal is null set a flash error on RedirectAttributes (mirroring
TicketViewController.createComment's "auth temporarily missing" behavior) then
return a redirect response instead of proceeding; specifically guard before
calling principal.getName(), and only call userRepository.findByUsername(...)
when principal is non-null to avoid NPEs.
Comment on lines
+113
to
+119
| try { | ||
| System.out.println("uploadDocument endpoint reached"); | ||
| System.out.println("Principal: " + principal); | ||
| System.out.println("Ticket id: " + id); | ||
| System.out.println("File original name: " + file.getOriginalFilename()); | ||
| System.out.println("File empty: " + file.isEmpty()); | ||
| System.out.println("File size: " + file.getSize()); |
There was a problem hiding this comment.
Remove debug System.out.println/printStackTrace — use a logger.
These are debug artifacts that will spam stdout in production and don't integrate with the application's logging pipeline. Replace with an SLF4J logger at appropriate levels.
🧹 Proposed cleanup
+ private static final org.slf4j.Logger log = org.slf4j.LoggerFactory.getLogger(TicketViewController.class);
...
try {
- System.out.println("uploadDocument endpoint reached");
- System.out.println("Principal: " + principal);
- System.out.println("Ticket id: " + id);
- System.out.println("File original name: " + file.getOriginalFilename());
- System.out.println("File empty: " + file.isEmpty());
- System.out.println("File size: " + file.getSize());
+ log.debug("uploadDocument ticketId={}, fileName={}, size={}",
+ id, file.getOriginalFilename(), file.getSize());
User user = userRepository.findByUsername(principal.getName())
.orElseThrow(() -> new ResourceNotFoundException("User not found with username: " + principal.getName()));
documentService.uploadDocument(id, user.getId(), file);
redirectAttributes.addFlashAttribute("documentSuccess", "Dokumentet laddades upp.");
} catch (RuntimeException ex) {
- ex.printStackTrace();
+ log.warn("Document upload failed for ticket {}", id, ex);
redirectAttributes.addFlashAttribute("documentError", "Dokumentet kunde inte laddas upp: " + ex.getMessage());
}Also applies to: 127-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@src/main/java/org/group1/projectbackend/controller/web/TicketViewController.java`
around lines 113 - 119, Replace the System.out.println calls (and any
printStackTrace usage) in TicketViewController's uploadDocument method with an
SLF4J logger: add a private static final Logger (or use existing logger) in
TicketViewController and log the principal, id, file metadata
(file.getOriginalFilename(), file.isEmpty(), file.getSize()) at an appropriate
level (debug/info), and log exceptions with logger.error when catching
exceptions instead of printStackTrace; update any similar prints around the same
method (including the occurrence at the later line noted) to use the logger
consistently.
Comment on lines
+410
to
412
| section-wide { | ||
| padding: 1.15rem; | ||
| } |
There was a problem hiding this comment.
Bug: missing leading dot on .section-wide selector.
section-wide { ... } is a type selector targeting a nonexistent <section-wide> element. To style class="section-wide" (used in tickets/detail.html lines 67, 93), this must be a class selector:
🐛 Proposed fix
-section-wide {
- padding: 1.15rem;
+.section-wide {
+ padding: 1.15rem;
+ margin-top: 2rem;
}Today the padding happens to be applied indirectly because both current usages are on <section> elements (which already receive padding: 1.15rem at line 406–408), masking the bug. Fold the inline margin-top into the class while you're here.
📝 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
| section-wide { | |
| padding: 1.15rem; | |
| } | |
| .section-wide { | |
| padding: 1.15rem; | |
| margin-top: 2rem; | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/static/css/app.css` around lines 410 - 412, Change the
bogus type selector "section-wide" to a class selector ".section-wide" so it
targets elements with class="section-wide" (update the rule named section-wide
to .section-wide in the CSS), and move the inline margin-top used in
tickets/detail.html into this .section-wide rule so the padding and top-margin
are consolidated into the .section-wide class (ensure you keep padding: 1.15rem
and add the former margin-top value).
Comment on lines
+106
to
+118
| <div class="document-dropzone" | ||
| tabindex="0" | ||
| role="button" | ||
| aria-controls="file" | ||
| aria-describedby="document-upload-selection document-upload-hint" | ||
| data-document-dropzone> | ||
| <strong>Släpp en fil här</strong> | ||
| <span>Eller klicka för att välja fil</span> | ||
| <span>Du kan även klistra in en bild med Ctrl+V</span> | ||
| </div> | ||
| <p class="muted document-selection" id="document-upload-selection" data-document-selection> | ||
| Ingen fil vald ännu. | ||
| </p> |
There was a problem hiding this comment.
aria-describedby references a non-existent document-upload-hint element.
The dropzone declares aria-describedby="document-upload-selection document-upload-hint", but only #document-upload-selection exists. Either add a hint element (e.g. a .document-upload-hint paragraph describing accepted file types/size) or drop the stale id to keep accessibility metadata accurate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/main/resources/templates/tickets/detail.html` around lines 106 - 118, The
dropzone element (data-document-dropzone) lists
aria-describedby="document-upload-selection document-upload-hint" but there is
no element with id "document-upload-hint"; either remove the stale reference
from the aria-describedby on the document-dropzone or add a new descriptive
element with id="document-upload-hint" (for example a <p class="muted
document-upload-hint"> describing accepted file types/size and paste behavior)
and ensure it is referenced by aria-describedby alongside the existing
`#document-upload-selection`; update whichever approach you choose to keep the IDs
and class names (document-dropzone, document-upload-selection,
document-upload-hint) consistent.
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
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Documentation
Style