fix: refactor api_new_meeting_registration. Fixes #7608

[rpcross]

• change to use JSON payload
• handle multiple records in one request

[rpcross]

I am working on tests for this. Using a JSON payload means the existing require_api_key decorator won't work. Assuming JSON payload is the way to go, are there other precedents for checking API key? Function can do it's own validation, or can update decorator.

[jennifer-richards]

are there other precedents for checking API key?

datatracker/ietf/api/ietf_utils.py

Lines 27 to 46 in b5ab4b6

	def requires_api_token(func_or_endpoint: Optional[Union[Callable, str]] = None):
	"""Validate API token before executing the wrapped method
	Usage:
	* Basic: endpoint defaults to the qualified name of the wrapped method. E.g., in ietf.api.views,
	@requires_api_token
	def my_view(request):
	...
	will require a token for "ietf.api.views.my_view"
	* Custom endpoint: specify the endpoint explicitly
	@requires_api_token("ietf.api.views.some_other_thing")
	def my_view(request):
	...
	will require a token for "ietf.api.views.some_other_thing"
	"""

is what we've started using for datatracker. This uses the X-Api-Key header to store a shared secret and keeps the auth concerns out of the API itself.

[rjsparks]

@rpcross This may have fallen into the cracks because of 120?

[rjsparks]

@rpcross - lets discuss this soon?

[codecov]

Codecov Report

Attention: Patch coverage is 68.11594% with 22 lines in your changes missing coverage. Please review.

Project coverage is 88.78%. Comparing base (def346e) to head (55771d5).
Report is 17 commits behind head on main.

Files with missing lines	Patch %	Lines
ietf/api/views.py	68.11%	22 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #7724      +/-   ##
==========================================
+ Coverage   88.71%   88.78%   +0.06%     
==========================================
  Files         310      312       +2     
  Lines       40882    40945      +63     
==========================================
+ Hits        36270    36351      +81     
+ Misses       4612     4594      -18     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[jennifer-richards]

There are a few comments inline, but I have reservations about the strategy used for removing/updating/replacing MeetingRegistration records. As it's done, I'm pretty sure there are race conditions that can lead to corrupt registrations. E.g., if two identical requests to register a user are at nearly the same time, two identical registrations may be created. This will happen if the later-running request handler executes line 334 (regs = MeetingRegistration.objects.filter(meeting__number=number, email=email)) before the earlier-running handler executes line 357 (new_reg.save()).

At minimum, we ought to put a uniqueness constraint on MeetingRegistration to guard against this. Not sure, but I think it'd need to be over email, meeting, and reg_type.

I think that constraint should be enough to avoid data corruption, but it would result in some API errors that were unnecessary. I.e., the two simultaneous identical requests would both "succeed" in that their work would be done as requested, but the later one would get back an error. It'd be very cool to rework the API using get_or_create (plus probably some additional subtlety handling) to avoid this, but I'm not sure it's necessary.

[rjsparks]

Do you want the registration system to know that the registration it just sent doesn't match a person? Think about whether telling the person that just registered "Oh no - something went wrong with your registration" might move some of the person-fixup work we do at meetings to the period before the meeting.

[rpcross]

I left this in for completeness, but do we expect this to happen? Since Registration now requires the participant to be logged in via the Datatracker, the existence of Person should be largely assured. My feeling is that all auth issues at the last few meetings have been either 1) someone is trying to access meetecho without having registered, 2) duplicate person issues.

[rjsparks]

This confuses me, and looks error-prone - it could effectively swap pks on records depending on the order L334 returns objects. Why isn't this using get_or_create with defaults and using the created flag instead?

[jennifer-richards]

Is there a reason to prefer the current model where a single person's registration is represented as several MeetingRegistration instances? The gymnastics in checking that the payload pertains only to a single email seems to be begging for a model that represents that as a single registration per email with fields representing the different options.

I think that'll greatly simplify the API implementation. It'll mean a migration, but I don't think that'll be terribly difficult.

[jennifer-richards]

Also, as far as I can tell there are currently no models with foreign keys into MeetingRegistration so it's not an going to break things now if PKs get swapped, but it'll do something in between cramping our style and causing serious bugs later.

[rpcross]

This confuses me, and looks error-prone - it could effectively swap pks on records depending on the order L334 returns objects. Why isn't this using get_or_create with defaults and using the created flag instead?

It certainly could swap pks. It is a brute force approach I chose for it's definitiveness. We can’t use get_or_create because there may be more than one existing registration record even for the same reg_type (two onsite one-day regs for example) AND the reg_type can change, switch from onsite to remote, so we’d be updating a record with a different reg_type. The simplest approach is to delete and re-create, or fully overwrite as I have done. Other approaches require more complexity. The following might work. Assuming that any notification will include all existing registrations for a given participant, and that a single registration save triggers the notification than it should be true that only one registration is added or was changed. If payload == 1 we can use get_or_create. (Except if payload == 1 and 2 already exist it means we missed a cancel somewhere). If payload > 1 we know that only one is new or updated and the rest should match existing records. We can do the work to determine which record doesn't match existing. This is a more complex procedure but would result in preserving the record PKs.

[rpcross]

Is there a reason to prefer the current model where a single person's registration is represented as several MeetingRegistration instances? The gymnastics in checking that the payload pertains only to a single email seems to be begging for a model that represents that as a single registration per email with fields representing the different options.

I think that'll greatly simplify the API implementation. It'll mean a migration, but I don't think that'll be terribly difficult.

We actually started with one MeetingRegistration record per person and had a reg_type field that was text and contained comma separated types, "onsite, hackathon_onsite", which proved problematic to deal with so we change the model to match registration, which could be multiple registration records per person. The current model seems appropriate.

              <- meeting 120 onsite week pass

person A <- meeting 121 onsite one day pass
<- meeting 121 remote week pass

Or maybe breakout another table?

person A <- meeting 121 <- onsite one-day
<- remote week pass

[rjsparks]

If L333-L356 change, this will have to be adjusted to match

[rjsparks]

Does the registration system check to see if a nomcom is accepting volunteers before offering the choice to someone?

[rpcross]

It does not. It's another level of integration that could be added. My understanding was that AMS would coordinate with the nomcom chair. When prepping to open registration if nomcom is accepting volunteers I would ensure the volunteer question appears on the reg form.

[rpcross]

Update:
The new proposal is to return to having a single MeetingRegistration record per person per meeting. The various registrations a single person can have for one meeting will be moved to a related object like this:

class MeetingRegistrationDetail(models.Model): # name?
meeting_registration = ForeignKey(MeetingRegistration)
reg_type = ForeignKey(RegistrationTypeName)
ticket_type = ForeignKey(RegistrationTicketTypeName, blank=True, null=True) # there are older registrations where this is empty

And use a custom object manager like this:

class MeetingRegistrationManager(models.Manager):
def onsite(self):
return self.get_queryset().filter(meetingregistrationdetail__reg_type__slug='onsite')
def remote(self):
# don't count people that also have a onsite registration
return self.get_queryset().filter(meetingregistrationdetail__reg_type__slug='remote').exclude(meetingregistrationdetail__reg_type__slug='onsite')

And queries like this:
MeetingRegistration.objects.onsite().filter(meeting__number=121)

We could combine reg_type and ticket_type, but the most frequent queries are those for specific reg_type “onsite” or “remote” so it seems reasonable to keep them separate. Are there better names for new classes, fields above?

Considerations:
Should we keep “checkedin” and “affialiation” on the MeetingRegistration record? Checkedin would only be set from a reg_type=onsite record. Hackathon or remote registrations would not affect this field. In theory you could have a different affiliation value for each registration, maybe prefer value from onsite over remote, and regular onsite|remote over hackathon, and populated over empty?

For simplicity is it acceptable to clear and recreate the related objects on update?

The above implementation is most flexible. Allowing the capture of all registration types currently represented, and new ones with the addition of new name types. Would a simpler approach of using boolean fields has_onsite, has_remote (has_hackathon_onsite, has_hackathon_remote) be sufficient. Note, you couldn’t represent two onsite one-day passes in that implementation.

Thoughts?

[rjsparks]

Should we keep “checkedin” and “affialiation” on the MeetingRegistration record?

We need checkedin somewhere. I think it's fine to be on the MeetingRegistration record - we can make it clear in the model that it means "picked up a badge onsite at the meeting".

I think we should keep "affiliation", but need to read the code to see if the datatracker is using it. Keeping it on MeetingRegistration makes sense.

[jennifer-richards]

Update: The new proposal is to return to having a single MeetingRegistration record per person per meeting. The various registrations a single person can have for one meeting will be moved to a related object like this:

I like the concept. A couple key things to watch are, 1) that this is simplifying our lives, not just mooshing complexity/bugs around; and 2) that we're not replicating parts of the registration app that shouldn't leak into the datatracker.

class MeetingRegistrationDetail(models.Model): # name? meeting_registration = ForeignKey(MeetingRegistration) reg_type = ForeignKey(RegistrationTypeName) ticket_type = ForeignKey(RegistrationTicketTypeName, blank=True, null=True) # there are older registrations where this is empty

I guess RegistrationTicketTypeName includes things like "hackathon", "week", "day"?

Name-wise, I like the ticket metaphor and would be inclined to call it a MeetingRegistrationTicket and change RegistrationTypeName to AttendanceTypeName or something (plus a matching change to reg_type).

It might be too early for this level of minutiae, but I'd encourage avoiding blank=True, null=True on the ticket_type field. Instead, use an unknown or some other type name that's marked as used=False and is not an option for new work. This will be closer to a guarantee we don't start creating incomplete records in the future.

And use a custom object manager like this:

class MeetingRegistrationManager(models.Manager): def onsite(self): return self.get_queryset().filter(meetingregistrationdetail__reg_type__slug='onsite') def remote(self): # don't count people that also have a onsite registration return self.get_queryset().filter(meetingregistrationdetail__reg_type__slug='remote').exclude(meetingregistrationdetail__reg_type__slug='onsite')

And queries like this: MeetingRegistration.objects.onsite().filter(meeting__number=121)

Checking an assumption, do we definitely need to support both onsite and remote registrations at the same meeting? I'd say we do either if we know we need it and/or if the reg system supports it, just so the datatracker side is expressive enough to capture everything. If not, life would be easier if the reg_type could move onto the MeetingRegistration model.

We could combine reg_type and ticket_type, but the most frequent queries are those for specific reg_type “onsite” or “remote” so it seems reasonable to keep them separate. Are there better names for new classes, fields above?

As noted above, I think you're describing an attendance type and it should be labeled as such instead of the more vague "registration type". The business logic about what combinations make sense belongs on the reg side and datatracker should be able to represent whatever reg says and not second guess its records.

Considerations: Should we keep “checkedin” and “affialiation” on the MeetingRegistration record? Checkedin would only be set from a reg_type=onsite record. Hackathon or remote registrations would not affect this field. In theory you could have a different affiliation value for each registration, maybe prefer value from onsite over remote, and regular onsite|remote over hackathon, and populated over empty?

I don't know for sure about affiliation, but my instinct says it is a MeetingRegistration property that applies to the person / meeting combination. Our records afaik only show one affiliation per person at each meeting. I can imagine why we might want separate affiliations, but I wouldn't add it here unless it's something supported by the reg side that we need to allow for.

I think checkedin should be tied to the "ticket" that was checked in, not to the whole MeetingRegistration. For day passes, the datatracker needs to be able to tell that a day pass is active and to which day it's being applied.

For simplicity is it acceptable to clear and recreate the related objects on update?

This depends on what the datatracker is doing with the data. It'd be nice if they didn't move around, but might not be a hard requirement. I think we'll want to be able to refer at least to MeetingRegistrations so those should be stable. For things below that, we mostly need to be clear about which things we're allowed to point at with FKs and be careful about deletion policies. We don't want to get into a situation where the datatracker prevents reg from accurately recording the true registration data.

The above implementation is most flexible. Allowing the capture of all registration types currently represented, and new ones with the addition of new name types. Would a simpler approach of using boolean fields has_onsite, has_remote (has_hackathon_onsite, has_hackathon_remote) be sufficient. Note, you couldn’t represent two onsite one-day passes in that implementation.

I prefer the better-normalized approach with the additional model to a raft of boolean flags. Not being able to represent multiple one-day passes makes it a non-starter.

[jennifer-richards]

Robert posted his reply while I was writing my tome. His comment (and an out-of-band discussion) convinced me that the suggestion I have about putting "checkedin" on the "ticket" instances is really a different concept.

[rpcross]

This PR is replaced by #8408

[jennifer-richards]

Closing as it's been superseded