-
Notifications
You must be signed in to change notification settings - Fork 424
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
feat: new meeting registration implementation #8408
base: main
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #8408 +/- ##
==========================================
- Coverage 88.81% 88.79% -0.02%
==========================================
Files 312 312
Lines 40876 41081 +205
==========================================
+ Hits 36304 36478 +174
- Misses 4572 4603 +31 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still have a few things to think about, but wanted to get some feedback to you. I like the direction. We'll need to create a feat branch to receive the series of PRs before merging to main. Maybe feat/registration
would be a good name?
last_name = models.CharField(max_length=255) | ||
affiliation = models.CharField(blank=True, max_length=255) | ||
country_code = models.CharField(max_length=2) # ISO 3166 | ||
person = ForeignKey(Person, blank=True, null=True) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need to handle Person records merging / being deleted. Should have on_delete=models.PROTECT
here - our custom ForeignKey
defaults to CASCADE, which would be bad news.
class RegistrationTicket(models.Model): | ||
registration = ForeignKey(Registration, related_name='tickets') | ||
attendance_type = ForeignKey(AttendanceTypeName) | ||
ticket_type = ForeignKey(RegistrationTicketTypeName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, should probably be models.PROTECT
on the attendance_type
and ticket_type
foreign keys. (For the registration
key, CASCADE
is sensible.)
for meeting_reg in meeting_regs: | ||
reg.tickets.create( | ||
attendance_type_id=meeting_reg.reg_type, | ||
ticket_type_id=meeting_reg.ticket_type, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a number of values in meeting_reg.reg_type
and .ticket_type
that aren't replicated in the new AttendanceTypeName
and TicketTypeName
world. Do you have a plan to rationalize these?
From the staging database:
>>> pp(Counter(MeetingRegistration.objects.values_list("ticket_type", flat=True)))
Counter({'': 45288,
'week_pass': 13756,
'full_week_pass': 3204,
'one_day': 2523,
'hackathon_combo': 1653,
'student': 1359,
'hackathon_only': 680,
'anrw_combo': 494,
'anrw_only': 13,
'one_day week_pass': 2,
'one_day full_week_pass': 1,
'full_week_pass one_day': 1,
'student full_week_pass': 1})
>>> pp(Counter(MeetingRegistration.objects.values_list("reg_type", flat=True)))
Counter({'': 33592,
'onsite': 14908,
'remote': 14835,
'hackathon_onsite': 3068,
'hackathon': 1397,
'hackathon_remote': 668,
'anrw_onsite': 507})
New implementation of meeting registrations. Moved from stats.MeetingRegistration to meeting.Registration and meeting.RegistrationTickets. Also new API and management command, migrate_registrations, to copy MeetingRegistration records to Registration.
This is just the first step in a series. The management command can be called from cron regularly to populate the new table, Registration, from the old. Future commits will migrate all the existing code that uses MeetingRegistration to using Registration. Once that is complete the Registration system can be changed to use the new API, stop running the migration, then remove the old table and API.
This PR replaces #7724