1660 CCE minor students not showing

[Kafui123]

Issue Description

Fixes #1660

• Students who were declared as CCE minors did not appear on the CCE Minor Progress table.

Changes

• Created a new query function called getDeclaredMinorStudentsWithProgress() that starts from the user table and uses left outer joins to include declared minors when they have no IndividualRequirement records yet.
• Computes engagementCount, hasSummer, and hasCCEMinorProposal directly from the database.
• Added email to the returned declared-minor progress data to support existing email string generation logic.
• Modified the tests to use the getDeclaredMinorStudentsWithProgress function.

Testing

• Click on Admin and then Minor Management
• Add students to the interested section of cce minor candidates
• Declare the interested student
• The interested student should appear under the cce minor progress table

The student has been declared

Student shows up on the CCE Minor Progress Table

[Copilot]

Pull request overview

This pull request fixes an issue where students who declared a CCE minor did not appear in the CCE Minor Progress table. The solution introduces a new query function that uses left outer joins to include declared minor students even when they have no engagement records yet.

Changes:

• Created getDeclaredMinorStudentsWithProgress() function that queries declared minor students with their progress data using left outer joins
• Updated the controller to merge declared minor students with sustained engagement students for the CCE Minor Progress table
• Modified tests to use the new function and updated test user data

Reviewed changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 8 comments.

Show a summary per file

File	Description
app/logic/minor.py	Added new getDeclaredMinorStudentsWithProgress() function to query declared minors with progress using left outer joins
app/controllers/admin/minor.py	Updated to use new function and merge declared students with sustained engagement students for display
app/templates/admin/cceMinor.html	Changed template variable from sustainedEngagement to cceMinorStudents
tests/code/test_minor.py	Updated test to use new function, changed test user from "glek" to "nimelyj", fixed email typo in one location
database/test_data.py	Added "glek" user back to test data

---

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

[Copilot]

Pull request overview

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

---

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

[Copilot]

The use of fn.SUM(fn.DISTINCT(summerCase)) is problematic. The DISTINCT function operates on the entire expression, not on individual CCEMinorProposal records. Since the Case expression evaluates to either 1 or 0 for each row, DISTINCT will only see unique values of 1 or 0, which means if a student has multiple summer experiences, the sum might only count 1 instead of the actual count. This could lead to incorrect summer experience counts. Consider using fn.COUNT(fn.DISTINCT(Case(None, [(CCEMinorProposal.proposalType == "Summer Experience", CCEMinorProposal.id)], None))) or a simpler approach with filtering.

Suggested change

	summerCase = Case(None, [(CCEMinorProposal.proposalType == "Summer Experience", 1)], 0)
	q = (
	User
	.select(
	User,
	fn.COUNT(fn.DISTINCT(IndividualRequirement.id)).alias("rawEngagementCount"),
	fn.COALESCE(fn.SUM(fn.DISTINCT(summerCase)), 0).alias("summerCount"),
	summerCase = Case(
	None,
	[(CCEMinorProposal.proposalType == "Summer Experience", CCEMinorProposal.id)],
	None,
	)
	q = (
	User
	.select(
	User,
	fn.COUNT(fn.DISTINCT(IndividualRequirement.id)).alias("rawEngagementCount"),
	fn.COALESCE(fn.COUNT(fn.DISTINCT(summerCase)), 0).alias("summerCount"),

[Copilot]

Pull request overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.

---

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

[Copilot]

Using SUM(DISTINCT(summerCase)) is problematic because DISTINCT is applied to the CASE expression result (0 or 1), not to the CCEMinorProposal records. This means if a student has multiple Summer Experience proposals, the DISTINCT will reduce all 1s to a single 1, but the SUM will still only count it once. This differs from the original getMinorProgress() which uses fn.SUM(summerCase) without DISTINCT. The DISTINCT here doesn't serve the intended purpose and could lead to incorrect counts if the logic is meant to count distinct summer experiences. Consider using fn.COUNT(fn.DISTINCT(CCEMinorProposal.id)).filter(CCEMinorProposal.proposalType == "Summer Experience") instead, or matching the original implementation's approach.

Suggested change

	fn.COALESCE(fn.SUM(fn.DISTINCT(summerCase)), 0).alias("summerCount"),
	fn.COALESCE(fn.SUM(summerCase), 0).alias("summerCount"),

[Copilot]

Pull request overview

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

---

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

[Copilot]

Pull request overview

Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.

---

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

[Copilot]

The hidden input field with ID based on tab.id (e.g., "interestedStudentEmails") has been removed from the macro. However, the JavaScript in minorAdminPage.js still references $("#interestedStudentEmails") and $("#declaredStudentEmails") on lines 39 and 43.

Since the PR has moved these hidden inputs outside the macro (lines 108-114 in cceMinor.html with fixed IDs), the functionality should still work. However, the previous implementation was more flexible as it generated the IDs dynamically based on tab data. The new approach hardcodes the IDs, which is less maintainable if additional tabs are added in the future.

This change appears to be intentional to support the removal of the "Declared" tab while keeping the email functionality working, but it reduces code reusability.

[Copilot]

The query logic with LEFT OUTER JOINs and the WHERE clause needs careful consideration. When a student is declared but has no IndividualRequirement records yet (the main use case), the JOIN to IndividualRequirement will produce rows with NULL values. The WHERE clause (CertificationRequirement.certification_id == Certification.CCE) | (CertificationRequirement.id.is_null(True)) correctly handles this by including rows where CertificationRequirement.id is NULL.

However, there's a subtle issue: when IndividualRequirement is NULL, the JOIN to CertificationRequirement (line 204) will also produce NULL for CertificationRequirement.id. But the WHERE clause checks if either the certification_id equals CCE OR if the CertificationRequirement.id is null. This means:

• Students with CCE requirements will match the first condition
• Students with no requirements will match the second condition (is_null)
• Students with non-CCE requirements will NOT match either condition and be excluded

This logic appears correct for the intended behavior. However, consider adding a comment to explain this complex filtering logic.

[Copilot]

Pull request overview

Copilot reviewed 8 out of 8 changed files in this pull request and generated 4 comments.

---

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

[Copilot]

The engagement count calculation subtracts all summer experiences from the total count. However, looking at the original getMinorProgress() function, it subtracts hasSummer (which is the SUM of a CASE statement for summer experiences). The new function uses summerCount which counts distinct summer proposal IDs. This creates an inconsistency: the original counts summer experiences via the CASE statement result, while the new one subtracts the count of distinct proposal records. If a student has multiple summer proposals, the calculations will differ between the two functions.

[JohnCox2211]

Comments from PR review have been resolved

[JohnCox2211]

My last minor thing would be the remove from minor button. I think it would be better if remove from minor was the header of that part of the table and the button said 'remove,' which would also add consistency with other buttons, but it works fine as is as well.

[github-actions]

View Code Coverage