Fix: onsitedetails crash on a non-numeric section id (#6106)

[nikhilprasad-data]

Description

This fixes the crash in onsitedetails() in studentonsite.py when the section id in the URL isn't a number, for example /learn/<program>/onsitedetails/abc. Before this change, the id went straight into ClassSection.objects.filter(id = secid), and Django raised "ValueError: Field 'id' expected a number but got 'abc'".

Now the id is converted with int() first. This is the same conversion Django was already doing internally, so every id that worked before still works the same way. If the conversion fails, the view treats it like a section that doesn't exist, which already redirects back to the schedule page (the same as /onsitedetails/999999). I chose the redirect instead of an error message like the one onsitecatalog() shows, because onsitedetails() has always redirected for an unknown section. If you'd prefer the error message, it's a small change.

I added tests in a new file, test_studentonsite_details.py. They cover a student who is enrolled in the section (page shown), a student who isn't enrolled, no id, unknown numeric ids (999999, 0, -1), non-numeric ids ('abc', '12abc', '0x10', '1e5', tested with the student enrolled in the section). A form like 1_000 is read as 1000 by int(), the same as before this change, so I don't count it as non-numeric, and a very large number. The tests only turn on the three modules the onsite pages need (StudentOnsite, StudentClassRegModule and StudentRegCore). ProgramFrameworkTest turns on all modules, and with all of them on, a student gets sent to the lottery page before the onsite view runs. I used a new file because my other PR (#6100) adds test_studentonsite.py and isn't merged yet. I'm happy to merge the two files later if you prefer.

Screenshot 1 shows the error on main before the change: /onsitedetails/999999 redirects and /onsitedetails/abc raises the ValueError. Screenshot 2 shows the 6 new tests passing after the change.

This only changes onsitedetails(). The other views listed in #6106 aren't touched in this PR.

Related Issue

Closes #6106

Type of Change

• Bug fix
• New feature
• Breaking change
• Documentation update
• Refactor / cleanup
• CI/CD or build change

Testing

• Existing tests pass
• New tests added (if applicable)
• Manually verified

AI Disclosure

I used an AI assistant to help review the codebase and double-check the test coverage. I did all the code implementation, local testing, and debugging myself.

Checklist

• Self-reviewed the code
• Updated documentation (if needed)
• No new warnings or errors introduced

[Copilot]

🟡 Changes recommended

Validate canonical, in-range IDs before querying and strengthen the malformed-ID tests.

2 open findings

• Test malformed IDs against an enrolled existing section · New
• Reject non-canonical section IDs before querying · New

What changed in this PR

This bug fix prevents onsitedetails() from crashing on invalid section IDs by redirecting them as missing sections.

Changes:

• Converts section IDs before database lookup.
• Adds regression tests for valid, unknown, malformed, and oversized IDs.
• Review found unresolved validation and test-coverage issues.

File	Summary
esp/​esp/​program/​modules/​tests/​test_studentonsite_details.py	Adds section-ID regression tests, but does not verify malformed IDs against an enrolled student and existing section.
esp/​esp/​program/​modules/​handlers/​studentonsite.py	Converts IDs before lookup, but still permits non-canonical and out-of-range values.

🧠 Review effort: Lite

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[nikhilprasad-data]

Thanks, you're right that the test never enrolled the student, so it couldn't catch a bad id falling back to a real section. I enrolled the student in test_non_numeric_section_id_redirects in the latest commit.

I didn't add "+" to the list. The URL patterns only allow letters, digits, "-", "_" and spaces in that part of the URL, so I expect a "+" to get a 404 from the router before it reaches the view, like the "1.5" case I removed earlier.

I also removed "1_000". Python reads int("1_000") as 1000, so it isn't really malformed, and it only passed because no section with that id exists. The out-of-range test on line 56 doesn't need an enrolled student, because no section can have that id.

[nikhilprasad-data]

I'd like to leave this as it is in this PR. Forms like "1_000" or " 1" being accepted isn't new: Django's own id lookup already converted the value with int() before my change, so they were accepted the same way. This PR only stops the crash and keeps everything else as it was.
If you'd prefer stricter parsing (only plain digits), it's a small change and I'm happy to add it with a test.