[UI/UX:Submission] Inline warnings for submission limits#12613
[UI/UX:Submission] Inline warnings for submission limits#12613not-so-shubh wants to merge 16 commits into
Conversation
automateprojectmangement
Bot
moved this from Seeking Reviewer
to In Review
in Submitty Development
not-so-shubh
changed the title
There was a problem hiding this comment.
Pull request overview
This PR updates the gradeable submission UI to reduce confusion around upload limits by removing the always-visible “maximum files” message and instead showing inline warnings only when the current upload state exceeds configured limits, while preventing submission when invalid.
Changes:
- Replaces alert/pop-up style validations with inline warning banners per upload part.
- Adds client-side validation to block submission when file-count or total-size limits are exceeded.
- Plumbs
max_submission_sizefrom backend view data into the submission template and JS initialization.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| site/public/js/drag-and-drop.js | Adds inline warning helpers and client-side validation gates prior to submission. |
| site/app/views/submission/HomeworkView.php | Exposes max_submission_size to the submission template for frontend validation. |
| site/app/templates/submission/homework/SubmitBox.twig | Replaces static max-files text with an inline warning container and initializes max size in JS. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| function getCurrentUploadSize(part) { | ||
| let total = 0; | ||
| for (const file of file_array[part - 1]) { | ||
| total += file.size; | ||
| } | ||
| for (const file of previous_files[part - 1]) { | ||
| if (file.size !== undefined) { | ||
| total += file.size; | ||
| } | ||
| } |
There was a problem hiding this comment.
getCurrentUploadSize attempts to include sizes from previous_files, but previous_files is populated with filename strings (see readPrevious), so file.size is always undefined and previous-file bytes are never counted. This makes the inline total-size validation diverge from the server-side check (which includes merged previous files). Consider storing previous files as {name, size} (or a parallel size map) and summing those sizes here.
| function validateCurrentUploadState(part) { | ||
| const currentCount = file_array[part - 1].length + previous_files[part - 1].length; | ||
|
|
||
| if (currentCount > MAX_NUM_OF_FILES) { | ||
| showUploadWarning( | ||
| part, | ||
| `Too many files selected. Maximum allowed number of files to be uploaded is ${MAX_NUM_OF_FILES}.` | ||
| ); | ||
| return false; | ||
| } | ||
|
|
||
| if (typeof MAX_TOTAL_UPLOAD_SIZE !== 'undefined' && getCurrentUploadSize(part) > MAX_TOTAL_UPLOAD_SIZE) { | ||
| showUploadWarning( | ||
| part, | ||
| 'Selected files are too large. Total upload size exceeds the allowed limit.' | ||
| ); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
The max submission size is enforced server-side as a single total across all parts (and includes any merged previous files), but validateCurrentUploadState(part) compares getCurrentUploadSize(part) against MAX_TOTAL_UPLOAD_SIZE per-part. This can miss cases where each part is under the limit but the combined submission exceeds it. Consider validating against the aggregate size across all parts (and then showing the warning in an appropriate location).
| function validateCurrentUploadState(part) { | ||
| const currentCount = file_array[part - 1].length + previous_files[part - 1].length; | ||
|
|
||
| if (currentCount > MAX_NUM_OF_FILES) { | ||
| showUploadWarning( | ||
| part, | ||
| `Too many files selected. Maximum allowed number of files to be uploaded is ${MAX_NUM_OF_FILES}.` | ||
| ); | ||
| return false; | ||
| } |
There was a problem hiding this comment.
validateCurrentUploadState includes previous_files[part - 1].length in the file-count check. However MAX_NUM_OF_FILES is initialized from PHP's max_file_uploads (a limit on newly uploaded files per request), while previous_files are already on the server and only copied/merged. Counting previous files here can incorrectly block valid “use most recent submission + add a file” workflows. Consider checking the upload-count limit using only file_array (or total new files across all parts) and treating previous-file selection separately.
| $(function() { | ||
| setButtonStatus({{ viewing_inactive_version }}); | ||
| initMaxNoFiles({{ max_file_uploads }}); | ||
| initMaxTotalUploadSize({{ max_submission_size|default(100000) }}); |
There was a problem hiding this comment.
initMaxTotalUploadSize({{ max_submission_size|default(100000) }}): since max_submission_size is always provided by the view and the config schema requires it to be >= 1, the default(100000) fallback is likely dead code and could mask configuration/view bugs. Consider removing the fallback (or explicitly defaulting to the same value the backend uses) so the client-side limit can’t silently diverge.
| initMaxTotalUploadSize({{ max_submission_size|default(100000) }}); | |
| initMaxTotalUploadSize({{ max_submission_size }}); |
JManion32
left a comment
JManion32
left a comment
There was a problem hiding this comment.
Please include screenshots of user interface changes.
github-project-automation
Bot
moved this from In Review
to Work in Progress
in Submitty Development
|
Added screenshots demonstrating the new inline warning behavior (also included in PR description):
The UI now shows contextual inline warnings instead of static messages or popups. |
automateprojectmangement
Bot
moved this from Work in Progress
to In Review
in Submitty Development
github-actions
Bot
added
the
Abandoned PR - Needs New Owner
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #12613 +/- ##
=========================================
Coverage 21.62% 21.62%
Complexity 9819 9819
=========================================
Files 268 268
Lines 36812 36812
Branches 490 490
=========================================
Hits 7960 7960
Misses 28366 28366
Partials 486 486
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
This comment was marked as outdated.
This comment was marked as outdated.
github-project-automation
Bot
moved this from In Review
to Done
in Submitty Development
|
@not-so-shubh I made a mistake when closing your PR because I thought you were inactive, and I now see that it was Justin that didn't respond to you! I reopened the PR. I will also post my review in a seperate comment. |
roye2
left a comment
roye2
left a comment
There was a problem hiding this comment.
This is from Issue #12413 :
-
Let's clean up the clutter in the submission box. For most courses, students do not get close to this drag & drop file limit. (Classes with larger projects typically use a .zip file or VCS/GIT version submission and don't have a limit on # of files.). So let's delete this message "Maximum allowed number of files to be uploaded is 20" if they student has not exceeded this limit.
-
If a student does drag more than 20 files into this box, then let's display a large red/orange warning message -- even before they hit the submit button -- that there is a problem with the number of files in the submission that is being prepared. Explain that "Maximum allowed number of files to be uploaded is 20" or slight revision of this text. NOTE: I believe we already have a popup/failure message if the user clicks submit when > 20 files are in the drag & drop box.
-
We also have a limit on the total upload size. There is a default and the instructor can modify this limit in the config.json file. This is not displayed on the page. I beleive we already have a popup/failure message if the user clicks submit when the drag & drop box contains files whose total size exceeds this limit. Similar to above, let's also add a large red/orange warning message -- even before they hit the submit button -- that there is a problem with the file size(s).
-
We currently do not display the maximum number of submission TIMES on the page. Unless the instructor prepares an assignment_message the students don't know what the limit is for this assigment. If the student has less than or equal to 5 penalty free submissions remaining, let's add a message on the page (styling to be discussed) notifying them of the limit relative to the number of submissions they have already made. NOTE: So if an instructor sets the limit to 3, this warning message will always display. if the instructor leaves the limit at 20, this warning message will appear after the student has made 15 or more submission.
I like your implementation for the first 3 of these steps. However, please add the functionality for the 4th step, that is: Display the maximum number of submission times on the page if the user has less than or equal to 5 penalty free submissions remaining. It should be easy to add a simple check to do this. Additionally, I am re-running the checks now, but if anything is broken in the linting or testing please fix it as well.
roye2
changed the title
roye2
changed the title
|
@not-so-shubh Are you still working on this? |
I am WIP on moving the logic to SubmitBox.twig so that the error messages can be displayed in banners. I am still trying to figure out if I can make the banners update when the user removes items that they previously added to the drag-and-drop box. |
|
I am closing this PR in favor of the new PR, #12887 , that aims to address the same issue with our favored UI. @not-so-shubh, I have included your commits in the new PR so you retain credit :). |
### Why is this Change Important & Necessary? Closes #12413 . This change aims to make submission limits (file count, file size, number of penalty-free submissions) and the ui on the submission page more user-friendly so that students can more easily understand how many submissions they have remaining. ### What is the New Behavior? <img width="1646" height="1115" alt="image" src="https://github.com/user-attachments/assets/f93a64a9-72e1-449d-9ea0-b1a44d7bebd0" /> New warning banners have been added to tell the user if they have too many files, files that are too large, or are running out of penalty-free submissions. The penalty free submissions warning appears when a student has 5 or fewer penalty free submissions remaining. Additionally, if a red banner is showing, the submit button is disabled. ### What steps should a reviewer take to reproduce or test the bug or new feature? 1. Create a new gradeable of the type: ' Students will submit one or more files by direct upload to the Submitty website ' 2. Test the penalty free submission limit warning banner (the default is 20 penalty free submissions). 3. Test the file count and size limit banners. If the red banner appears, the submit button should be disabled. ### Automated Testing & Documentation I will make an issue about testing these banners with Cypress. ### Other information This is not a breaking change. Some commits are linked from an old, abandoned PR, #12613 . --------- Co-authored-by: Shubh Jaiswal <[email protected]> Co-authored-by: Garvit Khandelwal <[email protected]>
Why is this Change Important & Necessary?
Fixes #12413.
The current UI always shows the maximum file count, which can mislead students about actual submission limits. This change removes the static message and replaces it with contextual inline warnings that appear only when limits are exceeded.
What is the New Behavior?
UI Changes
What steps should a reviewer take to reproduce or test the bug or new feature?
Automated Testing & Documentation
Other information