Skip to content

Fix House Meeting Attendance Status Display #151

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
Aug 28, 2017

Conversation

liam-middlebrook
Copy link
Member

This probably works. But it's currently untested

This probably works. But it's currently untested
@@ -137,4 +137,6 @@ def get_hm(member):
HouseMeeting.date).filter(
HouseMeeting.date > start_of_year(),
MemberHouseMeetingAttendance.uid == member.uid)
if only_absent:
h_meetings = [hm for hm in h_meetings if hm.attendance_status == "Absent"]
Copy link
Member

Choose a reason for hiding this comment

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

Are SQLAlchemy queries lazily loaded? If so h_meetings should produce a SQLAlchemy Query object but not actually hit the database until it is used. IF that is the case

if only_absent:
    h_meetings = h_meetings.filter(MemberHouseMeetingAttendance.attendance_status == "Absent")

will work nicely and not require evaluating the entire queryset and filtering it in memory

Copy link
Member

Choose a reason for hiding this comment

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

Yup it does. Good catch, thanks.

@mbillow
Copy link
Member

mbillow commented Aug 28, 2017

Once this passes CI, I am fine to 🚢 it.

@mbillow mbillow changed the title UNTESTED: Fix House Meeting Attendance Status Display Fix House Meeting Attendance Status Display Aug 28, 2017
@mbillow mbillow added the bug label Aug 28, 2017
@mbillow mbillow merged commit 9ff04ed into ComputerScienceHouse:develop Aug 28, 2017
@liam-middlebrook liam-middlebrook deleted the fix-hm-display branch August 29, 2017 19:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants