Skip to content

[Bugfix:Submission] File conflict check ignores extensions#12607

Merged
bmcutler merged 16 commits into
Submitty:mainfrom
Khine12:fix/bulk-upload-dot-assumption
Jul 10, 2026
Merged

[Bugfix:Submission] File conflict check ignores extensions#12607
bmcutler merged 16 commits into
Submitty:mainfrom
Khine12:fix/bulk-upload-dot-assumption

Conversation

@Khine12

@Khine12 Khine12 commented Mar 19, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Why is this Change Important & Necessary?

Fixes #10047

When bulk uploading files for students who have a previous submission,
the file conflict check used exact filename matching. This meant a
previously submitted file named answers (no extension) was not
recognized as conflicting with a newly uploaded answers.pdf, so no
merge option was offered for that student.

What is the New Behavior?

Files from previous submissions are now matched against new files by
base name (without extension), so answers and answers.pdf are
correctly treated as the same file and the merge/rename/overwrite
options are offered as expected.

What steps should a reviewer take to reproduce or test the bug or new feature?

  1. Create a bulk upload gradeable
  2. Have a student submit a file without an extension (e.g. answers)
  3. As instructor, bulk upload a PDF for that student (e.g. answers.pdf)
  4. Verify the Previous Submission dialog now correctly identifies
    the conflict and offers merge options

Automated Testing & Documentation

Added unit test testSecondVersionPreviousOverlapNoExtension in SubmissionControllerTester.php to verify that previous files without extensions are correctly detected as conflicting with new files that have extensions.

Other information

Not a breaking change. No migrations needed. No security concerns.

@github-project-automation github-project-automation Bot moved this to Seeking Reviewer in Submitty Development Mar 19, 2026
@Khine12 Khine12 changed the title [Fix] Bulk upload file conflict check ignores file extensions [Bugfix:Submission] File conflict check ignores extensions Mar 19, 2026
JManion32
JManion32 previously requested changes Mar 21, 2026
View reviewed changes

@JManion32 JManion32 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @Khine12, thank you for your contribution! I see that you mentioned creating an issue to update the unit test for this feature. Why don't you just add it here?

@github-project-automation github-project-automation Bot moved this from Seeking Reviewer to Work in Progress in Submitty Development Mar 21, 2026
@Khine12

Khine12 commented Mar 21, 2026

Copy link
Copy Markdown
Contributor Author

Hi @JManion32, I've added the unit test directly in this PR (testSecondVersionPreviousOverlapNoExtension in SubmissionControllerTester.php) and updated the PR description. Please let me know if any further changes are needed. Thank you!

@Khine12
Khine12 requested a review from JManion32 March 21, 2026 19:08
@automateprojectmangement automateprojectmangement Bot moved this from Work in Progress to In Review in Submitty Development Mar 21, 2026
@hyeenaa
hyeenaa self-requested a review March 23, 2026 17:50
@Khine12

Khine12 commented Mar 27, 2026

Copy link
Copy Markdown
Contributor Author

Hi @JManion32, just checking in — I've added the unit test as you requested. Could you or a maintainer approve the workflow run so CI can verify the changes? Let me know if anything else is needed. Thanks!

hyeenaa
hyeenaa approved these changes Mar 27, 2026
View reviewed changes

@hyeenaa hyeenaa left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As the original issue outlines, it does not account for files without an extension, and has strange behavior as described in 10047, ie. completely removing the old version without an extension.

Tested with PDFs and was able to verify that it does NOT ignore the old file without an extension. The new files can be merged, replaced, or regraded as functionality in the PR describes.

@codecov

codecov Bot commented Mar 29, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 21.30%. Comparing base (6582e94) to head (e1a5a94).
⚠️ Report is 4 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@             Coverage Diff              @@
##               main   #12607      +/-   ##
============================================
+ Coverage     21.29%   21.30%   +0.01%     
- Complexity    10071    10094      +23     
============================================
  Files           278      278              
  Lines         37697    37780      +83     
  Branches        516      516              
============================================
+ Hits           8028     8050      +22     
- Misses        29166    29227      +61     
  Partials        503      503
Flag Coverage Δ
autograder 21.16% <ø> (ø)
js 1.98% <ø> (ø)
migrator 100.00% <ø> (ø)
php 20.22% <100.00%> (+0.01%) ⬆️
python_submitty_utils 79.83% <ø> (ø)
submitty_daemon_jobs 91.13% <ø> (ø)
vue 100.00% <ø> (ø)

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.

@Khine12 Khine12 mentioned this pull request Mar 31, 2026
Closed
@Khine12

Khine12 commented Apr 20, 2026

Copy link
Copy Markdown
Contributor Author

Hi, just checking in — I've addressed the review feedback, and this PR is still active on my end. Happy to make any additional changes if needed. Let me know!

@hyeenaa

hyeenaa commented Apr 21, 2026

Copy link
Copy Markdown
Contributor

Hi, just checking in — I've addressed the review feedback, and this PR is still active on my end. Happy to make any additional changes if needed. Let me know!

I will let my team know to get you a last reviewer.

mattLif
mattLif approved these changes Jun 12, 2026
View reviewed changes

@mattLif mattLif left a comment •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good. I tested with same-named files with different extensions (or lack thereof) and received the expected new behavior.

@mattLif
mattLif dismissed JManion32’s stale review June 12, 2026 13:55

unit test was added

@bmcutler

bmcutler commented Jul 8, 2026

Copy link
Copy Markdown
Member

@Khine12 @mattLif

This PR is failing CI tests (not just flakey tests).
Please address these errors.

@bmcutler bmcutler moved this from In Review to Work in Progress in Submitty Development Jul 8, 2026
@bmcutler
bmcutler merged commit c603aab into Submitty:main Jul 10, 2026
28 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mattLif mattLif mattLif approved these changes
@hyeenaa hyeenaa hyeenaa approved these changes
@JManion32 JManion32 Awaiting requested review from JManion32

Assignees

No one assigned

Labels

None yet

Projects

Submitty Development
Archived in project

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

bulk upload checking for files from previous submissions assumes student upload files contain a dot

5 participants

@Khine12 @hyeenaa @bmcutler @mattLif @JManion32