[Feature:Forum] Allow PDF attachments by not-so-shubh · Pull Request #12636 · Submitty/Submitty

not-so-shubh · 2026-03-24T07:29:36Z

What is the current behavior?

Discussion forum attachments currently allow valid image uploads, but reject PDF attachments.

Closes #12202

What is the new behavior?

This PR allows valid PDF attachments in discussion forum posts in addition to valid images.

What changed?

allow forum attachments that are valid images or PDFs
validation is content-based and not extension-only
preserved existing image validation behavior
kept existing forum attachment limits unchanged
updated the forum upload UI to accept PDFs (accept="image/*,.pdf")
added a separate PDF rendering path for forum attachments
kept existing inline image rendering behavior unchanged
updated edit/history views to correctly display PDF attachments
aligned attachment opening behavior with current forum behavior by using new-tab opening without popup-style window features
added unit tests for:
- valid PDF accepted
- invalid file rejected
- existing image behavior still works
added focused e2e coverage for:
- valid PDF upload
- invalid file rejection
- attachment visible on created post
- PDF preview rendering

Notes

This implementation was informed by feedback from #12213:

attachment opening behavior was simplified to stay consistent with the current forum attachment UX
PDF handling was separated from image handling instead of forcing both through the same rendering path
existing attachment data flow was preserved to avoid breaking forum edit/history views
tests were strengthened to verify actual outcomes rather than timing-based assumptions

Manual Testing

created a thread with a PDF attachment and verified the thread was created successfully
verified the PDF appears in the created post attachment section
verified PDF preview renders correctly
verified PDF open behavior uses a new tab
verified PDF appears correctly in the edit attachment table
verified remove/delete still works for PDF attachments
verified existing image upload, preview, and open behavior still works

Screenshots

Upload UI (PDF selectable)

PDF Preview on created post

Edit view showing PDF tile

Copilot

Pull request overview

Enables discussion forum attachments to include PDFs (in addition to images) while preserving existing limits and adding UI + rendering support for PDF previews.

Changes:

Updated client/server validation to accept either images or PDFs for forum attachments.
Added PDF-specific rendering paths (thumbnail indicator in edit UI; iframe preview + “Open PDF” in thread/history views).
Added/updated unit + e2e tests for valid PDF acceptance and invalid file rejection.

Reviewed changes

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
tests/e2e/test_forum.py	Adds an e2e test covering PDF upload + invalid file rejection + PDF preview rendering.
site/tests/app/libraries/UtilsTester.php	Adds unit tests for new server-side “image-or-PDF” upload validation.
site/public/js/forum.js	Extends attachment validation to PDFs; adds PDF preview rendering and updates open behavior.
site/public/css/forum.css	Adds styles for PDF thumbnails and inline PDF preview layout.
site/app/templates/forum/ThreadPostForm.twig	Updates upload UI label and `accept` attribute to allow PDFs.
site/app/templates/forum/RenderPost.twig	Updates history rendering to show PDFs with an iframe preview + open link.
site/app/templates/forum/EditImgTable.twig	Updates edit/history attachment table to support PDF tiles and “Attached Files” wording.
site/app/templates/forum/CreatePost.twig	Escapes encoded attachment JSON for safe inclusion in an HTML attribute.
site/app/libraries/Utils.php	Adds content-based PDF validation helpers and a new combined image-or-PDF validator.
site/app/libraries/ForumUtils.php	Annotates attachment type (image vs pdf) into encoded attachment payload used by the frontend.
site/app/controllers/forum/ForumController.php	Updates controller-side validation/error messaging to allow PDFs as valid attachments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

codecov · 2026-03-24T17:26:56Z

Codecov Report

❌ Patch coverage is 62.96296% with 10 lines in your changes missing coverage. Please review.
✅ Project coverage is 21.66%. Comparing base (68c6187) to head (06ad641).
⚠️ Report is 8 commits behind head on main.

Additional details and impacted files

@@             Coverage Diff              @@
##               main   #12636      +/-   ##
============================================
+ Coverage     21.64%   21.66%   +0.02%     
- Complexity     9654     9668      +14     
============================================
  Files           268      268              
  Lines         36244    36258      +14     
  Branches        487      487              
============================================
+ Hits           7845     7857      +12     
- Misses        27916    27918       +2     
  Partials        483      483

Flag	Coverage Δ
autograder	`21.32% <ø> (ø)`
js	`2.04% <ø> (ø)`
migrator	`100.00% <ø> (ø)`
php	`20.69% <62.96%> (+0.02%)`	⬆️
python_submitty_utils	`80.08% <ø> (ø)`
submitty_daemon_jobs	`91.13% <ø> (ø)`

Flags with carried forward coverage won't be shown. Click here to find out more.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

dagemcn · 2026-03-24T17:33:59Z

Please address failing CIs.

Rkoester47

Thank you for your contribution. I noticed an issue when testing the changes on this PR. When I try to delete an attached image or PDF, it works correctly. However, if there are multiple images/PDFs attached to a comment, it is impossible to delete the attachments. An error displays when attempting to update a post with a delete of one or more attachments on a post with multiple attachments.

not-so-shubh · 2026-03-25T11:23:20Z

Thanks for testing this, @Rkoester47!

I tried to reproduce the issue locally but wasn’t able to trigger it. I tested:

multiple PDFs/images
mixed image + PDF
deleting a single or middle attachment
editing existing posts

All of these worked on my end.

Could you share the exact steps you followed and your browser/environment? Also, if possible, any console/network errors when the update fails would be really helpful since the UI only shows a generic “Post update failed” message.

I’ll keep looking into it in parallel 👍

Copilot

Pull request overview

Copilot reviewed 11 out of 11 changed files in this pull request and generated 3 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-25T12:25:05Z

+function isValidForumAttachment(file) {
+    const mime_type = file.type || '';
+    const lower_name = file.name.toLowerCase();
+    return mime_type.startsWith('image/') || mime_type === 'application/pdf' || lower_name.endsWith('.pdf');


Client-side validation currently treats any file with a .pdf filename suffix as valid, even when the browser reports a different MIME type (e.g., text/plain). Consider only falling back to the filename extension when file.type is empty/unknown, so the UI-side validation stays meaningful and aligns better with server-side content-based validation.

Suggested change

return mime_type.startsWith('image/') || mime_type === 'application/pdf' || lower_name.endsWith('.pdf');

if (!mime_type) {

// Fallback to filename extension only when MIME type is empty/unknown

return lower_name.endsWith('.pdf');

}

return mime_type.startsWith('image/') || mime_type === 'application/pdf';

Copilot · 2026-03-25T12:25:06Z

+                    // Forum attachments are validated before storage, so any non-image attachment here is treated as a PDF.
+                    $mime_type = mime_content_type($file['path']);
+                    $attachment_type = is_string($mime_type) && str_starts_with($mime_type, 'image/') && getimagesize($file['path']) !== false ? 'image' : 'pdf';


$attachment_type defaults to 'pdf' for anything that isn’t a valid image. For defense-in-depth, it would be safer to explicitly detect PDFs (e.g., via Utils::isValidUploadedPdf) and treat any unexpected/unknown MIME type as an unsupported attachment type (render as a download link, or skip preview) rather than embedding it in an <iframe>. Also consider reusing Utils::isValidUploadedImage() here to avoid duplicating the image-validation logic.

Suggested change

// Forum attachments are validated before storage, so any non-image attachment here is treated as a PDF.

$mime_type = mime_content_type($file['path']);

$attachment_type = is_string($mime_type) && str_starts_with($mime_type, 'image/') && getimagesize($file['path']) !== false ? 'image' : 'pdf';

// Forum attachments are validated before storage, but we still explicitly

// distinguish between images, PDFs, and unsupported types for defense-in-depth.

if (Utils::isValidUploadedImage($file['path'])) {

$attachment_type = 'image';

}

elseif (Utils::isValidUploadedPdf($file['path'])) {

$attachment_type = 'pdf';

}

else {

$attachment_type = 'unsupported';

}

Copilot · 2026-03-25T12:25:06Z

+        $attachment_check = Utils::checkUploadedImageOrPdfFile($file_post) ? 1 : 0;
+        if ($attachment_check === 0 && !empty($_FILES[$file_post]['tmp_name'])) {
+            return $this->returnUserContentToPage("Invalid file type. Please upload only image or PDF files.", $isThread, $thread_id);
        }
-        return [$imageCheck];
+        return [$attachment_check];


checkGoodAttachment() only special-cases UPLOAD_ERR_NO_FILE and otherwise relies on MIME checks. If PHP sets other upload errors (e.g., size limits), tmp_name can be empty/nonexistent and the current validation path may still mark attachments as “good”, leading to missing files later when move_uploaded_file runs. Consider rejecting when any $_FILES[$file_post]['error'][i] !== UPLOAD_ERR_OK (and/or when tmp_name doesn’t exist) before returning success.

not-so-shubh · 2026-03-26T18:48:43Z

I pushed a follow-up commit to fix the JS lint issues and the PHPStan/type-hint errors. The remaining Cypress failure still appears to be in late_submission_warning_messages.spec.js, which looks unrelated to the forum PDF attachment changes.

dagemcn · 2026-03-27T02:33:44Z

Thank you for your contribution. I noticed an issue when testing the changes on this PR. When I try to delete an attached image or PDF, it works correctly. However, if there are multiple images/PDFs attached to a comment, it is impossible to delete the attachments. An error displays when attempting to update a post with a delete of one or more attachments on a post with multiple attachments.

@Rkoester47 I also was unable to replicate this issue. What browser/OS are you using?

dagemcn

Great work here. Code looks good and functionality work great. If someone can replicate the issue that @Rkoester47 is experiencing then we should fix that but otherwise everything looks good! Side note for the PHP linting, it likely will not pass until you generate a new baseline with
php vendor/bin/phpstan analyze app public/index.php socket/index.php --generate-baseline --memory-limit 2G
from the site directory. Add the changed baseline.neon file to your commits and that should fix it.

not-so-shubh · 2026-03-27T09:00:17Z

Thanks! I regenerated the PHPStan baseline and pushed the updated file.

not-so-shubh · 2026-03-27T09:09:53Z

Great work here. Code looks good and functionality work great. If someone can replicate the issue that @Rkoester47 is experiencing then we should fix that but otherwise everything looks good! Side note for the PHP linting, it likely will not pass until you generate a new baseline with php vendor/bin/phpstan analyze app public/index.php socket/index.php --generate-baseline --memory-limit 2G from the site directory. Add the changed baseline.neon file to your commits and that should fix it.

I was able to investigate this further and reproduce the behavior.

It seems the “Post update failed” message occurs when the edited post is submitted with an empty body, rather than being related to multiple attachments.

Specifically:

Deleting attachments and submitting with no post text → update fails
Including any text in the post → update succeeds, even with multiple attachments

In the screenshot shared above by @Rkoester47, the post body also appears to be empty, which would explain the failure.

So this appears to be due to existing validation that prevents empty posts, rather than an issue with handling multiple attachments.

I’ve attached a short screen recording demonstrating this behavior.

Screen.Recording.2026-03-27.at.14.38.07.mov

Farhanxikram

functionality review:
The PDF/image button works, and it's relatively intuitive to use. The same goes for actually viewing the documents/images on a post.

Rkoester47

@not-so-shubh Thank you for investigating this further. As I see it is the original functionality of the discussion forum causing this error to display, I can approve of this PR. Please ensure all checks are passing so that we will be able to merge once this PR is fully approved.

not-so-shubh · 2026-03-31T05:29:39Z

@Rkoester47 @dagemcn @Farhanxikram I looked into the remaining CI failures.

The Cypress failure is in late_submission_warning_messages.spec.js, and the CSS lint failure is in public/css/calendar.css. Since neither of these files are modified in this PR, they appear to be unrelated to the forum PDF attachment changes.

## What is the current behavior? Discussion forum attachments currently allow valid image uploads, but reject PDF attachments. Closes Submitty#12202 ## What is the new behavior? This PR allows valid PDF attachments in discussion forum posts in addition to valid images. ## What changed? - allow forum attachments that are valid images or PDFs - validation is content-based and not extension-only - preserved existing image validation behavior - kept existing forum attachment limits unchanged - updated the forum upload UI to accept PDFs (`accept="image/*,.pdf"`) - added a separate PDF rendering path for forum attachments - kept existing inline image rendering behavior unchanged - updated edit/history views to correctly display PDF attachments - aligned attachment opening behavior with current forum behavior by using new-tab opening without popup-style window features - added unit tests for: - valid PDF accepted - invalid file rejected - existing image behavior still works - added focused e2e coverage for: - valid PDF upload - invalid file rejection - attachment visible on created post - PDF preview rendering ## Notes This implementation was informed by feedback from Submitty#12213: - attachment opening behavior was simplified to stay consistent with the current forum attachment UX - PDF handling was separated from image handling instead of forcing both through the same rendering path - existing attachment data flow was preserved to avoid breaking forum edit/history views - tests were strengthened to verify actual outcomes rather than timing-based assumptions ## Manual Testing - created a thread with a PDF attachment and verified the thread was created successfully - verified the PDF appears in the created post attachment section - verified PDF preview renders correctly - verified PDF open behavior uses a new tab - verified PDF appears correctly in the edit attachment table - verified remove/delete still works for PDF attachments - verified existing image upload, preview, and open behavior still works ## Screenshots ### Upload UI (PDF selectable) ![Upload UI](https://github.com/user-attachments/assets/998badcc-1771-4db0-8672-114ab63303dc) ### PDF Preview on created post ![PDF Preview](https://github.com/user-attachments/assets/865d24fd-2a13-48e0-8563-38beb2c0ea61) ### Edit view showing PDF tile ![Edit View](https://github.com/user-attachments/assets/0d7f0070-5379-429b-b252-0cf6fc1fe8c9) --------- Co-authored-by: dagemcn <[email protected]> Co-authored-by: Barb Cutler <[email protected]>

Copilot AI review requested due to automatic review settings March 24, 2026 07:29

github-project-automation Bot added this to Submitty Development Mar 24, 2026

github-project-automation Bot moved this to Seeking Reviewer in Submitty Development Mar 24, 2026

automateprojectmangement Bot moved this from Seeking Reviewer to In Review in Submitty Development Mar 24, 2026

Copilot started reviewing on behalf of not-so-shubh March 24, 2026 07:30 View session

Copilot AI reviewed Mar 24, 2026

View reviewed changes

Comment thread site/public/js/forum.js Outdated

Comment thread site/public/js/forum.js Outdated

Comment thread site/public/js/forum.js Outdated

not-so-shubh force-pushed the forum-pdf-attachments-12202 branch from 7ba192f to 1a14c3f Compare March 24, 2026 07:46

Rkoester47 requested changes Mar 24, 2026

View reviewed changes

github-project-automation Bot moved this from In Review to Work in Progress in Submitty Development Mar 24, 2026

not-so-shubh force-pushed the forum-pdf-attachments-12202 branch from 1a14c3f to 39177ca Compare March 25, 2026 11:00

not-so-shubh changed the title ~~[Bugfix:Forum] Allow PDF attachments in discussion forum posts~~ [Feature:Forum] Allow PDF attachments Mar 25, 2026

not-so-shubh requested a review from Copilot March 25, 2026 12:19

Copilot started reviewing on behalf of not-so-shubh March 25, 2026 12:19 View session

Copilot AI reviewed Mar 25, 2026

View reviewed changes

not-so-shubh requested a review from Rkoester47 March 25, 2026 12:28

automateprojectmangement Bot moved this from Work in Progress to In Review in Submitty Development Mar 26, 2026

dagemcn approved these changes Mar 27, 2026

View reviewed changes

Farhanxikram approved these changes Mar 27, 2026

View reviewed changes

Rkoester47 approved these changes Mar 31, 2026

View reviewed changes

github-project-automation Bot moved this from In Review to Awaiting Maintainer Review in Submitty Development Mar 31, 2026

not-so-shubh added 3 commits March 31, 2026 07:23

Allow PDF forum attachments and rendering in discussion posts

7af86d9

Add forum PDF attachment unit and e2e coverage

d235eaa

Address forum attachment review comments

1b28b0c

not-so-shubh added 2 commits March 31, 2026 07:23

Fix forum PDF attachment CI lint and phpstan issues

c410e93

Update PHPStan baseline

7cf5866

not-so-shubh force-pushed the forum-pdf-attachments-12202 branch from e0df932 to 7cf5866 Compare March 31, 2026 01:53

Merge branch 'main' into forum-pdf-attachments-12202

65237f0

dagemcn removed the status in Submitty Development Apr 3, 2026

dagemcn moved this to Awaiting Maintainer Review in Submitty Development Apr 3, 2026

Merge branch 'main' into forum-pdf-attachments-12202

06ad641

bmcutler merged commit 44ce5b7 into Submitty:main Apr 10, 2026
25 checks passed

-    return mime_type.startsWith('image/') || mime_type === 'application/pdf' || lower_name.endsWith('.pdf');
+    if (!mime_type) {
+        // Fallback to filename extension only when MIME type is empty/unknown
+        return lower_name.endsWith('.pdf');
+    }
+    return mime_type.startsWith('image/') || mime_type === 'application/pdf';

-                    // Forum attachments are validated before storage, so any non-image attachment here is treated as a PDF.
-                    $mime_type = mime_content_type($file['path']);
-                    $attachment_type = is_string($mime_type) && str_starts_with($mime_type, 'image/') && getimagesize($file['path']) !== false ? 'image' : 'pdf';
+                    // Forum attachments are validated before storage, but we still explicitly
+                    // distinguish between images, PDFs, and unsupported types for defense-in-depth.
+                    if (Utils::isValidUploadedImage($file['path'])) {
+                        $attachment_type = 'image';
+                    }
+                    elseif (Utils::isValidUploadedPdf($file['path'])) {
+                        $attachment_type = 'pdf';
+                    }
+                    else {
+                        $attachment_type = 'unsupported';
+                    }

Uh oh!

Conversation

not-so-shubh commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What is the current behavior?

What is the new behavior?

What changed?

Notes

Manual Testing

Screenshots

Upload UI (PDF selectable)

PDF Preview on created post

Edit view showing PDF tile

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

codecov Bot commented Mar 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

dagemcn commented Mar 24, 2026

Uh oh!

Rkoester47 left a comment

Choose a reason for hiding this comment

Uh oh!

not-so-shubh commented Mar 25, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 25, 2026

Choose a reason for hiding this comment

Uh oh!

not-so-shubh commented Mar 26, 2026

Uh oh!

dagemcn commented Mar 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dagemcn left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

not-so-shubh commented Mar 27, 2026

Uh oh!

not-so-shubh commented Mar 27, 2026

Uh oh!

Farhanxikram left a comment

Choose a reason for hiding this comment

Uh oh!

Rkoester47 left a comment

Choose a reason for hiding this comment

Uh oh!

not-so-shubh commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

not-so-shubh commented Mar 24, 2026 •

edited

Loading

codecov Bot commented Mar 24, 2026 •

edited

Loading

dagemcn commented Mar 27, 2026 •

edited

Loading

dagemcn left a comment •

edited

Loading

not-so-shubh commented Mar 31, 2026 •

edited

Loading