[Bugfix:System] Fix crash on numeric section names#12529
Conversation
JManion32
left a comment
JManion32
left a comment
There was a problem hiding this comment.
Hey @srikaaviya, thank you for your contribution!
Please update your PR title to use our required format
[<TYPE>:<MODULE>] <SUBJECT>, where <SUBJECT> is 40 chars max.
See also: https://submitty.org/developer/getting_started/make_a_pull_request
Please also use our PR description template:
https://raw.githubusercontent.com/Submitty/Submitty/refs/heads/main/.github/PULL_REQUEST_TEMPLATE.md
Since your changes affect the user interface, please include relevant screenshots before and after your change to demonstrate that you tested and debugged your PR. This will help reviewers check your work.
Be sure to link the issue that you are addressing in this PR. If the issue is completely closed with this PR say “Fixes #1234”, if it only partially addresses the issue, “work related to issue #1234”
Once this pull request adheres to all Submitty conventions, we will proceed with a full review.
Additionally, it seems there are linting changes unrelated to the content of your PR. Please revert these.
github-project-automation
Bot
moved this from Seeking Reviewer
to Work in Progress
in Submitty Development
srikaaviya
changed the title
When a section like '01' is added while section '1' already exists, the system crashed with an unhandled Twig error. This is caused by two related issues: 1. The section insertion did not check for numerically-equivalent existing sections. Since registration_section_id is VARCHAR, '01' and '1' are distinct strings, so ON CONFLICT DO NOTHING did not prevent the insert. However, the sorting query for sections uses COALESCE(SUBSTRING(...)::NUMERIC, -1), treating '01' and '1' as the same integer, causing crashes downstream. Fix: Add a pre-check in insertNewRegistrationSection() that uses a PostgreSQL regex to detect existing sections with the same numeric value (e.g. '^0*1$' matches '1', '01', '001'). Returns 0 to trigger the existing 'already present' error message in the UI. 2. The RotatingSectionsForm.twig template used 'in reg_sections_count|keys' to check section presence. Twig's 'in' operator uses PHP loose comparison, so '01' == 1 (integer) evaluates to true, but then accessing reg_sections_count['01'] fails because only integer key 1 exists in the mapping. Fix: Replace 'in reg_sections_count|keys' with Twig's 'is defined' test, which performs an exact key lookup without type coercion. Also adds test cases for '01' and '001' to User::validateUserData() to document that leading-zero section names are valid format. Fixes Submitty#12484
7fcd9fa to
7e05d9d
Compare
JManion32
left a comment
JManion32
left a comment
There was a problem hiding this comment.
Code looks reasonable and functionality is now as intended with no page errors.
A few small things:
- Just leave the top comment in
DatabaseQueries.php, I don't think you need to elaborate further - Change the title to
[Bugfix:System]. Bug is not a valid prefix.
| $semester = $this->core->getConfig()->getTerm(); | ||
| $course = $this->core->getConfig()->getCourse(); | ||
| // Prevent numerically-equivalent sections from both being inserted (e.g. '01' vs '1'). | ||
| // registration_section_id is VARCHAR so '01' != '1' at the DB level, but the sort query |
There was a problem hiding this comment.
Just leave the first comment, no need to fully elaborate here.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #12529 +/- ##
============================================
+ Coverage 21.64% 21.67% +0.02%
- Complexity 9654 9809 +155
============================================
Files 268 268
Lines 36245 36730 +485
Branches 487 490 +3
============================================
+ Hits 7846 7960 +114
- Misses 27916 28284 +368
- Partials 483 486 +3
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
srikaaviya
changed the title
srikaaviya
changed the title
|
@JManion32 Thanks for the review. I've removed the explanatory lines. |
github-actions
Bot
added
the
Abandoned PR - Needs New Owner
automateprojectmangement
Bot
moved this from Work in Progress
to In Review
in Submitty Development
aconfo
left a comment
aconfo
left a comment
There was a problem hiding this comment.
I performed a functionality review. During my testing I tried various different combinations with section numbers to see if there was an error, including many leading 0's, many trailing 0's, numbers between 0's, and couldn't find any combination that caused the page to crash. Overall, it looks pretty good.
github-actions
Bot
removed
the
Abandoned PR - Needs New Owner
Thanks for the review! |
williamjallen
left a comment
williamjallen
left a comment
There was a problem hiding this comment.
It would be better to add a database-level check constraint for this.
srikaaviya
changed the title
Sure. I'm planning to add a migration file in (migration/migrator/migrations/master/) to add the database level unique index. Should I keep the PHP pre-check alongside the DB constraint or replace the PHP pre-check entirely and rely only on the DB constraint? |
bmcutler
moved this from Ready to Merge
to Awaiting Maintainer Review
in Submitty Development
automateprojectmangement
Bot
moved this from Awaiting Maintainer Review
to In Review
in Submitty Development
Rkoester47
left a comment
Rkoester47
left a comment
There was a problem hiding this comment.
Approved, the feature works properly and all tests are passing.
6 participants
Why is this Change Important & Necessary?
Fixes #12484
When an instructor tries to add a registration section named 01 (or 02, etc.) and a section 1 (or 2) already exists, the application crashes with an unhandled 500 error instead of showing a friendly message.
What is the New Behavior?
Before: Adding section 01 when section 1 already exists crashes the page with an OutputException / Twig rendering error.
After: Adding section 01 when section 1 exists shows: "Registration Section 01 already present" (same friendly message as adding a normal exact duplicate).
What steps should a reviewer take to reproduce or test the bug or new feature?
[ Before fix: 500 crash with Twig OutputException
After fix: friendly error message "Registration Section 01 already present"]
Automated Testing & Documentation
Added test cases for '01' and '001' to User::validateUserData() in
UserTester.php
— confirms leading-zero section names pass format validation
— All 66 existing
UserTester
— tests continue to pass
— No documentation changes needed.
Other information
No breaking changes or migrations. The fix is backward-compatible — existing sections are unaffected.
Two changes are made:
DatabaseQueries.php
— Pre-check in
insertNewRegistrationSection()
uses a PostgreSQL regex to detect numerically-equivalent existing sections ('^0*1$' matches 1, 01, 001). Returns 0 to trigger the existing "already present" UI error.
RotatingSectionsForm.twig
— Replaced in reg_sections_count|keys with Twig's is defined (exact key lookup), which prevents a crash if numerically-equivalent sections somehow both exist in the database (PHP loose comparison caused '01' == 1 to evaluate as true, passing the check but failing on key access).