-
Notifications
You must be signed in to change notification settings - Fork 829
fix: refactor api_new_meeting_registration. Fixes #7608 #7724
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
Changes from all commits
a56439f
457270e
ad4d870
0da8d61
06619ec
4ae7306
55771d5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -142,6 +142,7 @@ def post(self, request): | |
| # else: | ||
| # return HttpResponse(status=405) | ||
|
|
||
|
|
||
| @require_api_key | ||
| @role_required('Robot') | ||
| @csrf_exempt | ||
|
|
@@ -233,6 +234,155 @@ def err(code, text): | |
| return HttpResponse(status=405) | ||
|
|
||
|
|
||
| _new_registration_json_validator = jsonschema.Draft202012Validator( | ||
| schema={ | ||
| "type": "array", | ||
| "items": { | ||
| "type": "object", | ||
| "properties": { | ||
| "meeting": {"type": "string"}, | ||
| "first_name": {"type": "string"}, | ||
| "last_name": {"type": "string"}, | ||
| "affiliation": {"type": "string"}, | ||
| "country_code": {"type": "string"}, | ||
| "email": {"type": "string"}, | ||
| "reg_type": {"type": "string"}, | ||
| "ticket_type": {"type": "string"}, | ||
| "checkedin": {"type": "boolean"}, | ||
| "is_nomcom_volunteer": {"type": "boolean"}, | ||
| "cancelled": {"type": "boolean"}, | ||
| }, | ||
| "required": ["meeting", "first_name", "last_name", "affiliation", "country_code", "email", "reg_type", "ticket_type", "checkedin", "is_nomcom_volunteer", "cancelled"], | ||
| "additionalProperties": "false" | ||
| } | ||
| } | ||
| ) | ||
|
|
||
|
|
||
| @requires_api_token | ||
| @csrf_exempt | ||
| def api_new_meeting_registration_v2(request): | ||
| '''REST API to notify the datatracker about new or updated meeting registrations''' | ||
|
|
||
| def _safe_pop(lst): | ||
| if lst: | ||
| return lst.pop() | ||
| else: | ||
| return None | ||
|
|
||
| def _http_err(code, text): | ||
| return HttpResponse(text, status=code, content_type="text/plain") | ||
|
|
||
| def _api_response(result): | ||
| return JsonResponse(data={"result": result}) | ||
|
|
||
| if request.method != "POST": | ||
| return _http_err(405, "Method not allowed") | ||
|
|
||
| if request.content_type != "application/json": | ||
| return _http_err(415, "Content-Type must be application/json") | ||
|
|
||
| # Validate | ||
| try: | ||
| payload = json.loads(request.body) | ||
| _new_registration_json_validator.validate(payload) | ||
| except json.decoder.JSONDecodeError as err: | ||
| return _http_err(400, f"JSON parse error at line {err.lineno} col {err.colno}: {err.msg}") | ||
| except jsonschema.exceptions.ValidationError as err: | ||
| return _http_err(400, f"JSON schema error at {err.json_path}: {err.message}") | ||
| except Exception: | ||
| return _http_err(400, "Invalid request format") | ||
|
|
||
| # Validate consistency | ||
| # - if receive multiple records they should be for same meeting, same person (email) | ||
| if len(payload) > 1: | ||
| if len(set([r['meeting'] for r in payload])) != 1: | ||
| return _http_err(400, "Different meeting values") | ||
| if len(set([r['email'] for r in payload])) != 1: | ||
| return _http_err(400, "Different email values") | ||
|
|
||
| # Validate meeting | ||
| number = payload[0]['meeting'] | ||
| try: | ||
| meeting = Meeting.objects.get(number=number) | ||
| except Meeting.DoesNotExist: | ||
| return _http_err(400, "Invalid meeting value: '%s'" % (number, )) | ||
|
|
||
| # Validate email | ||
| email = payload[0]['email'] | ||
| try: | ||
| validate_email(email) | ||
| except ValidationError: | ||
| return _http_err(400, "Invalid email value: '%s'" % (email, )) | ||
|
|
||
| # handle cancelled. there should be only one record | ||
| if payload[0]['cancelled']: | ||
| if len(payload) > 1: | ||
| return _http_err(400, "Error. Received cancelled registration notification with more than one record. ({})".format(email)) | ||
| reg = MeetingRegistration.objects.filter( | ||
| meeting__number=number, | ||
| email=email, | ||
| reg_type=payload[0]['reg_type'], | ||
| ticket_type=payload[0]['ticket_type']).first() | ||
| if reg: | ||
| reg.delete() | ||
| return HttpResponse('Success', status=202, content_type='text/plain') | ||
|
|
||
| # get person | ||
| person = Person.objects.filter(email__address=email).first() | ||
| if not person: | ||
| log.log(f"api_new_meeting_registration_v2 no Person found for {email}") | ||
|
|
||
| # get existing records if any | ||
| regs = MeetingRegistration.objects.filter(meeting__number=number, email=email) | ||
| pks = [r.pk for r in regs] | ||
|
|
||
| for registration in payload: | ||
| new_reg = MeetingRegistration( | ||
| meeting=meeting, | ||
| first_name=registration['first_name'], | ||
| last_name=registration['last_name'], | ||
| affiliation=registration['affiliation'], | ||
| country_code=registration['country_code'], | ||
| person=person, | ||
| email=email, | ||
| reg_type=registration['reg_type'], | ||
| ticket_type=registration['ticket_type'], | ||
| checkedin=registration['checkedin']) | ||
|
|
||
| # update any existing records if there are any | ||
| new_reg.pk = _safe_pop(pks) | ||
| if new_reg.pk: | ||
| log.log(f"Updating MeetingRegistration record for meeting:{meeting} email:{email}") | ||
| else: | ||
| log.log(f"New MeetingRegistration record for meeting:{meeting} email:{email}") | ||
| new_reg.save() | ||
|
Comment on lines
+336
to
+359
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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?
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there a reason to prefer the current model where a single person's registration is represented as several I think that'll greatly simplify the API implementation. It'll mean a migration, but I don't think that'll be terribly difficult.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, as far as I can tell there are currently no models with foreign keys into
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
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. person A <- meeting 121 onsite one day pass Or maybe breakout another table? person A <- meeting 121 <- onsite one-day |
||
|
|
||
| # removed account creation email. Registration requires Datatracker account | ||
|
|
||
| # handle nomcom volunteer | ||
| if registration['is_nomcom_volunteer'] and person: | ||
| try: | ||
| nomcom = NomCom.objects.get(is_accepting_volunteers=True) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does the registration system check to see if a nomcom is accepting volunteers before offering the choice to someone?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
| except (NomCom.DoesNotExist, NomCom.MultipleObjectsReturned): | ||
| nomcom = None | ||
| if nomcom: | ||
| Volunteer.objects.get_or_create( | ||
| nomcom=nomcom, | ||
| person=person, | ||
| defaults={ | ||
| "affiliation": registration["affiliation"], | ||
| "origin": "registration" | ||
| } | ||
| ) | ||
|
|
||
| # delete any remaining records | ||
| if pks: | ||
| MeetingRegistration.objects.filter(pk__in=pks).delete() | ||
|
Comment on lines
+379
to
+381
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If L333-L356 change, this will have to be adjusted to match |
||
|
|
||
| return HttpResponse('Success', status=202, content_type='text/plain') | ||
|
|
||
|
|
||
| def version(request): | ||
| dumpdate = None | ||
| dumpinfo = DumpInfo.objects.order_by('-date').first() | ||
|
|
||
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.
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.
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 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.