fix: correct meeting attendance calculations#4536
Conversation
But the tests aren't actually looking to see what numbers get generated yet.
Codecov Report
@@ Coverage Diff @@
## main #4536 +/- ##
==========================================
- Coverage 88.44% 88.43% -0.01%
==========================================
Files 296 296
Lines 39701 39683 -18
==========================================
- Hits 35112 35094 -18
Misses 4589 4589
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
rpcross
left a comment
rpcross
left a comment
There was a problem hiding this comment.
Consider that some people register (and attend) both onsite and remote. Some 40+ examples in 114. Usually people who registered one-day onsite. In this case total unique attendees would be less than onsite attendees + remote attendees.
| if number is None or number < 110: | ||
| return None | ||
| Attendance = namedtuple('Attendance', 'onsite online') | ||
| Attendance = namedtuple('Attendance', 'onsite online') # TODO: these should probably be 'onsite remote' |
There was a problem hiding this comment.
Agree with TODO comment, 'remote' matches the identifier in the Registration system
| meetingregistration__attended=True, | ||
| meetingregistration__reg_type__contains='remote', | ||
| ).distinct().count(), | ||
| onsite=attended.filter(meetingregistration__reg_type='onsite').count(), |
There was a problem hiding this comment.
This is inaccurate as it will include Persons with 'onsite' registrations for previous meetings
What is the best way to report this? Is the complexity of adding a total unique attendees in addition to onsite/remote warranted or would it be adequate to, say, count someone as onsite if they were onsite at all? Edit: in the current implementation, someone who registered both for online and remote will be counted in both bins. |
|
Input from the IETF Chair and Secretariat: They would like anyone who attended in person to only be counted as an in-person attendee even if they also registered/attended remotely. |
jennifer-richards
left a comment
jennifer-richards
left a comment
There was a problem hiding this comment.
Approving - one possible change to consider and take or leave.
| ).distinct() | ||
|
|
||
| onsite=set(attended.filter(meetingregistration__meeting=self, meetingregistration__reg_type='onsite')) | ||
| remote=set(attended.filter(meetingregistration__meeting=self, meetingregistration__reg_type='remote')) |
There was a problem hiding this comment.
Would it be better to do the exclusion in the query and just pull the counts over to Python? I'm thinking something like:
onsite = attended.filter(...type='onsite').count()
remote = attended.filter(...type='remote').exclude(...type='onsite').count()
(I'm not sure if that's an improvement.)
There was a problem hiding this comment.
I started there and it produced surprisingly wrong results. i have a dim memory that there is a bug in the stack that causes the sql query to fail - don't remember if the issue was in Django2 or in mysql. So I just avoided it for now, and I think the result is actually a little more readable.
There was a problem hiding this comment.
It does trade off doing things in django's memory rather than the databases, but this isn't a performance crunchpoint.
3 participants
No description provided.