[STC-645] Properly handle unsupported upload file types in CKEditor#23734
Draft
thykel wants to merge 4 commits into
Draft
[STC-645] Properly handle unsupported upload file types in CKEditor#23734thykel wants to merge 4 commits into
thykel wants to merge 4 commits into
Conversation
… image in work package description (WP STC-645)
|
Warning Flaky specs
|
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
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.
Merge pre-requirement: opf/commonmark-ckeditor-build#120
Ticket
https://community.openproject.org/work_packages/STC-645
What are you trying to accomplish?
When a user drags or pastes an image of an unsupported type (e.g. WebP) into the CKEditor description field on an instance where WebP is not on the attachment allowlist, the backend already returns a clear 422 error:
However, that message was silently discarded by the frontend. The upload progress toast dismissed itself on failure without ever surfacing the error text, and a generic CKEditor indicator was shown instead — leaving the user with no actionable explanation.
This PR ensures the specific backend error message is shown to the user in an OpenProject error toast whenever an attachment upload is rejected (by file-type filter or any other server-side validation).
Screenshots
N/A
What approach did you choose and why?
Root cause:
toast.component.htmlbound the(uploadError)output ofUploadProgressComponenttoremove(). TheHttpErrorResponsecarrying the backend message was emitted but never passed toToastService.addError(), so the only thing that happened on failure was that the upload toast silently disappeared.Fix — two-part:
ToastComponent(toast.component.ts+toast.component.html): Added anonUploadError(error: HttpErrorResponse)method that callsthis.toastService.addError(error)— which already knows how to extract HAL error messages — and then removes the upload toast. The template binding was changed from(uploadError)="remove()"to(uploadError)="onUploadError($event)".AttachmentsResourceService(attachments.service.ts): Added acatchErrorpipe inuploadAttachments()that converts the opaqueHttpErrorResponseinto a plainErrorwith the human-readable message before re-throwing. This ensures that callers further up the chain (such as the CKEditor upload plugin) receive a meaningful string rather than an unreadable object when callingloader.reject().Why not only fix the service layer? The
ToastService.addUpload()and theUploadProgressComponentare the natural point where per-file upload errors are observed; fixing the component handler is the minimal, targeted change that guarantees the message is shown exactly once without interfering with other callers. The service-layercatchErroris a complementary improvement for CKEditor's own error path and does not duplicate the toast display.No backend changes are required — the error message (
not_allowlisted) already exists and is translated in all supported locales.Merge checklist