fix: build proceedings attendee list from MeetingRegistration table. …#6567
fix: build proceedings attendee list from MeetingRegistration table. …#6567rjsparks merged 3 commits intoietf-tools:mainfrom
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6567 +/- ##
==========================================
+ Coverage 88.85% 88.88% +0.02%
==========================================
Files 284 285 +1
Lines 40253 40283 +30
==========================================
+ Hits 35767 35804 +37
+ Misses 4486 4479 -7
|
|
This leaves some abandoned dbtemplates - it's reasonable to leave them in place in the short term should we need to fall back, but we'll need to track an action to remove them. Leaving this comment as a note to create an issue to track that task when this PR is merged. |
jennifer-richards
left a comment
jennifer-richards
left a comment
There was a problem hiding this comment.
I have a couple comments inline, but I think this is ok, assuming the duplicate MeetingRegistration data get cleaned up.
If the older data likely have similar issues, de-duplicating the records before displaying them would be something to consider. That would probably need to be paired with another method for detecting the problem, though, so that duplicates are not simply swept under the rug.
| if mr.person.pk in attended and mr.person.pk not in checked_in: | ||
| regs.append(mr) | ||
|
|
||
| meeting_registrations = sorted(regs, key=lambda x: (x.last_name, x.first_name)) |
There was a problem hiding this comment.
Bummer that first/last name is baked in here (and presumably deeper in the registration). If sorting these comes up again, it'd probably be worth factoring out a method to sort by name.
| queryset = Person.objects.all() | ||
| return queryset.filter(meetingregistration__meeting__in=list(previous_five),meetingregistration__attended=True).annotate(mtg_count=Count('meetingregistration')).filter(mtg_count__gte=3) | ||
|
|
||
| def participants_for_meeting(meeting): |
There was a problem hiding this comment.
Seems like this helper belongs in ietf.meeting instead of ietf.nomcom
rjsparks
left a comment
rjsparks
left a comment
There was a problem hiding this comment.
Jennifer's comments cause me to think about method signatures as well, and I do want some changes before we merge this.
| checked_in = meeting.meetingregistration_set.filter(reg_type='onsite', checkedin=True).values_list('person', flat=True) | ||
| sessions = meeting.session_set.filter(Q(type='plenary') | Q(group__type__in=['wg', 'rg'])) | ||
| attended = Attended.objects.filter(session__in=sessions).values_list('person', flat=True) | ||
| return (set(checked_in), set(attended)) |
There was a problem hiding this comment.
evaluating these to sets here is not right - we should return querysets and let whatever called it chose when to evaluate. (I also agree this should move to meeting)
There was a problem hiding this comment.
Moved function and changed to return querysets.
rjsparks
left a comment
rjsparks
left a comment
There was a problem hiding this comment.
Sorry - one more housekeeping change please.
| def test_participants_for_meeting(self): | ||
| person_a = PersonFactory() | ||
| person_b = PersonFactory() | ||
| person_c = PersonFactory() | ||
| person_d = PersonFactory() | ||
| m = MeetingFactory.create(type_id='ietf') | ||
| MeetingRegistrationFactory(meeting=m, person=person_a, reg_type='onsite', checkedin=True) | ||
| MeetingRegistrationFactory(meeting=m, person=person_b, reg_type='onsite', checkedin=False) | ||
| MeetingRegistrationFactory(meeting=m, person=person_c, reg_type='remote') | ||
| MeetingRegistrationFactory(meeting=m, person=person_d, reg_type='remote') | ||
| AttendedFactory(session__meeting=m, session__type_id='plenary', person=person_c) | ||
| checked_in, attended = participants_for_meeting(m) | ||
| self.assertTrue(person_a.pk in checked_in) | ||
| self.assertTrue(person_b.pk not in checked_in) | ||
| self.assertTrue(person_c.pk in attended) | ||
| self.assertTrue(person_d.pk not in attended) | ||
|
|
||
|
|
There was a problem hiding this comment.
This should move to the meeting tests.
3 participants
…Fixes #6265