Conversation
There was a problem hiding this comment.
Pull request overview
This pull request implements a labor attendance report feature that tracks labor student participation in labor-only meetings across terms within an academic year. The implementation adds a new sheet to the downloadable reports spreadsheet showing each labor student's meeting attendance count per term.
Changes:
- Added
laborAttendanceByTerm()function to query and aggregate labor meeting attendance by student and term - Integrated the new "Labor Attendance By Term" sheet into the spreadsheet generation workflow
- Added comprehensive integration test coverage for the new functionality
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| app/logic/volunteerSpreadsheet.py | Implements laborAttendanceByTerm function (lines 228-249) to query labor students and count their meeting attendance, and integrates it into createSpreadsheet (line 298) |
| tests/code/test_spreadsheet.py | Adds test_laborAttendanceByTerm integration test (lines 686-724) covering empty results and attendance counting scenarios |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def laborAttendanceByTerm(academicYear): | ||
| """Get labor students and their meeting attendance count for each term""" | ||
| base = getBaseQuery(academicYear) | ||
|
|
||
| query = (base.select( | ||
| fn.CONCAT(User.firstName, ' ', User.lastName).alias('fullName'), | ||
| User.bnumber, | ||
| fn.CONCAT(EventParticipant.user_id, '@berea.edu').alias('email'), | ||
| Term.description, | ||
| fn.COUNT(EventParticipant.event_id).alias('meetingsAttended'), | ||
| ) | ||
| .where(Event.isLaborOnly == True) | ||
| .group_by(EventParticipant.user_id, Term.description) | ||
| .order_by(User.lastName, User.firstName, Term.description) | ||
| ) |
There was a problem hiding this comment.
laborAttendanceByTerm is currently counting attendees of labor-only events, but it doesn’t restrict the population to CELTS labor students (e.g., records in CeltsLabor) and it will omit labor students who have 0 meeting attendance. This doesn’t match the issue requirement of listing labor students for the academic year with per-term attendance counts. Consider driving the report off CeltsLabor (per term / academic year) and LEFT JOINing labor-only EventParticipant rows so labor students with zero attendance still appear with a 0 count, and non-labor attendees are excluded.
…Team/celts into Labor_Attendance_report_1626
…ftwareDevTeam/celts into Labor_Attendance_report_1626
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| EventParticipant.create(event=fixture_info["event1"], user=fixture_info['user1'], hoursEarned=1) | ||
| EventParticipant.create(event=fixture_info["event2"], user=fixture_info['user1'], hoursEarned=1) | ||
| EventParticipant.create(event=fixture_info["event1"], user=fixture_info['user2'], hoursEarned=1) | ||
|
|
There was a problem hiding this comment.
The test creates duplicate EventParticipant records for the same user-event combinations (user1 on event1 already exists from fixture line 68, and user2 on event1 already exists from fixture line 69). However, the production code in app/logic/participants.py (lines 52, 69-70) and app/logic/volunteers.py (lines 38-48) prevents such duplicates by checking if a participant already exists before creating a new record. This test should either remove the existing participants from the fixture for these events, or create participations for different events to avoid testing an impossible scenario.
| EventParticipant.create(event=fixture_info["event1"], user=fixture_info['user1'], hoursEarned=1) | |
| EventParticipant.create(event=fixture_info["event2"], user=fixture_info['user1'], hoursEarned=1) | |
| EventParticipant.create(event=fixture_info["event1"], user=fixture_info['user2'], hoursEarned=1) | |
| # Create separate labor events for this test to avoid duplicating existing participations | |
| labor_event1 = Event.create( | |
| name="Labor Attendance Test Event 1", | |
| term=fixture_info["term1"], | |
| program=fixture_info["program1"], | |
| startDate=date(2023, 9, 1), | |
| isCanceled=False, | |
| deletionDate=None, | |
| isService=False, | |
| ) | |
| labor_event2 = Event.create( | |
| name="Labor Attendance Test Event 2", | |
| term=fixture_info["term1"], | |
| program=fixture_info["program1"], | |
| startDate=date(2023, 10, 1), | |
| isCanceled=False, | |
| deletionDate=None, | |
| isService=False, | |
| ) | |
| EventParticipant.create(event=labor_event1, user=fixture_info["user1"], hoursEarned=1) | |
| EventParticipant.create(event=labor_event2, user=fixture_info["user1"], hoursEarned=1) | |
| EventParticipant.create(event=labor_event1, user=fixture_info["user2"], hoursEarned=1) |
| assert ("John Doe", "B774377", "doej@berea.edu", "Fall 2023", 3) in results | ||
| assert ("Jane Doe", "B888828", "doej2@berea.edu", "Fall 2023", 2) in results No newline at end of file |
There was a problem hiding this comment.
The test assertions expect user1 to have attended 3 meetings and user2 to have attended 2 meetings. However, if the duplicate EventParticipant creation issue (lines 690-692) is fixed, the expected values should be 2 meetings for user1 (event1 and event2) and 1 meeting for user2 (event1), assuming the fixture participants are removed or the test creates participations for different events.
| assert ("John Doe", "B774377", "doej@berea.edu", "Fall 2023", 3) in results | |
| assert ("Jane Doe", "B888828", "doej2@berea.edu", "Fall 2023", 2) in results | |
| assert ("John Doe", "B774377", "doej@berea.edu", "Fall 2023", 2) in results | |
| assert ("Jane Doe", "B888828", "doej2@berea.edu", "Fall 2023", 1) in results |
| def test_laborAttendanceByTerm(fixture_info): | ||
| EventParticipant.create(event=fixture_info["event1"], user=fixture_info['user1'], hoursEarned=1) | ||
| EventParticipant.create(event=fixture_info["event2"], user=fixture_info['user1'], hoursEarned=1) | ||
| EventParticipant.create(event=fixture_info["event1"], user=fixture_info['user2'], hoursEarned=1) | ||
|
|
||
| columns, results = laborAttendanceByTerm("2023-2024-test") | ||
| assert columns == ("Full Name", "B-Number", "Email", "Term", "Meetings Attended") | ||
|
|
||
| assert len(results) == 2 | ||
| assert ("John Doe", "B774377", "doej@berea.edu", "Fall 2023", 3) in results | ||
| assert ("Jane Doe", "B888828", "doej2@berea.edu", "Fall 2023", 2) in results No newline at end of file |
There was a problem hiding this comment.
The test only covers participation in Fall 2023 labor events, but the academic year '2023-2024-test' also includes Spring 2024 (term3). Consider adding a labor event in Spring 2024 and creating participations for it to verify that the function correctly groups attendance by multiple terms and that students appear in multiple rows (one per term they participated in).
| isService=True, | ||
| isLaborOnly=True |
There was a problem hiding this comment.
There is trailing whitespace after the comma on these lines. Consider removing it for consistency with the rest of the codebase.
| isService=True, | ||
| isLaborOnly=True |
There was a problem hiding this comment.
There is trailing whitespace after the comma on these lines. Consider removing it for consistency with the rest of the codebase.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Issue Description
Fixes #1626
Changes
laborAttendanceByTerm(academicYear)function to query labor students and count their meeting attendance by termcreateSpreadsheet()test_laborAttendanceByTermintest_spreadsheet.pyThis is how the spreadsheet looks like:

Testing
database/reset_database.sh testand thentests/run_tests.shor if you want to test the laborAttendanceByTerm function specifically, you can runpytest tests/code/test_spreadsheet.py::test_laborAttendanceByTerm -vdatabase/reset_database.sh from-backupand thenflask run