[ENG-10737] Registrations are failing to auto-approve [privacy]#11718
[ENG-10737] Registrations are failing to auto-approve [privacy]#11718antkryt wants to merge 2 commits into
Conversation
cslzchen
left a comment
There was a problem hiding this comment.
First pass done, left a few questions / suggestions 🔥
| @property | ||
| def archive_job(self): | ||
| return self.archive_jobs.first() if self.archive_jobs.count() else None | ||
| return self.archive_jobs.first() |
There was a problem hiding this comment.
Nice clean-up, no need to check since .first() returns None if query set is empty.
| initiated_by = models.ForeignKey(settings.AUTH_USER_MODEL, null=True, blank=True, on_delete=models.CASCADE) | ||
|
|
||
| @property | ||
| def auto_approve_at(self): |
There was a problem hiding this comment.
Nitpicking: let's use auto_approval_time
| auto_approve_at = approval.auto_approve_at | ||
| if timezone.now() < auto_approve_at: | ||
| remaining = auto_approve_at - timezone.now() |
There was a problem hiding this comment.
Remember to change the local var auto_approval_at to auto_approval_time too.
Note: CMIIW, we need to use this local var because @property is calculated every time.
| try: | ||
| registration = Registration.objects.get(_id=registration_id) | ||
| except Registration.DoesNotExist: | ||
| registration = Registration.load(registration_id) |
There was a problem hiding this comment.
Maybe a personal preference but I think
registration = Registration.objects.filter(_id=registration_id).first()is better than calling the load().
There was a problem hiding this comment.
Field _id doesn't exist on Registration, we cannot query it. So, we should either filter by registration.id or load(registration._id) [or do something really weird: guid = Guid.objects.filter(_id=registration_id).first(); registration = guid.referent 😁]
|
|
||
| approval = registration.registration_approval | ||
| if not approval: | ||
| logger.info(f"Registration {registration_id} has no registration approval object") |
There was a problem hiding this comment.
I love the logs. Please take a look at what log level is most appropriate for each.
e.g. we sometime don't want to flood the logs and use debug, sometimes we want to highlight possible concerns by using warn, and some may need to be elevated to sentry
| logger.error(f"Error processing manual restart approval for {registration_id}: {e}") | ||
| sentry.log_exception(e) |
There was a problem hiding this comment.
👍 for the sentry logs, it might be easier for us to find / relate to the error message than different exceptions in sentry. Let's add the following:
sentry.log_message(f"Error processing manual restart approval for {registration_id}: {str(e)}")`| call_command( | ||
| 'process_manual_restart_approvals', | ||
| registration_id=registration_id, | ||
| dry_run=False, | ||
| hours_back=24, | ||
| verbosity=1 | ||
| ) | ||
| logger.info(f"Processing manual restart approval for registration {registration_id}") | ||
| approve_past_pendings([approval], dry_run=False) |
There was a problem hiding this comment.
Curious on why using approve_past_pendings() instead of process_manual_restart_approvals fixes the issue?
I see other places calling the command process_manual_restart_approvals, do they need to be updated, or they are unrelated to this ticket?
There was a problem hiding this comment.
process_manual_restart_approvals is just a weird wrapper for the approve_past_pendings function. It collects list of registrations to approve and runs approve_past_pendings for this list. But check_manual_restart_approval works with a single registration, that's why I call approve_past_pendings directly here
| if was_manually_restarted(dst): | ||
| logger.info(f'Registration {dst._id} was manually restarted, scheduling approval check') | ||
| delayed_manual_restart_approval.delay(dst._id, delay_minutes=5) |
There was a problem hiding this comment.
I understand we need to add this to force_archive() but why do we have to remove it from archive_success().
There was a problem hiding this comment.
archive_success is never triggered after the manual restart. IMO, we don't need delayed_manual_restart_approval function that runs if registration was_manually_restarted(dst) in the arhive_success()
I think this place is the main reason why ENG-10737 ticket was created, we run approval check where it makes no sense, it just doesn't do anything here
There was a problem hiding this comment.
Oh, this also means that I don't need to check if was_manually_restarted(dst) in the force_archive function (manual restart = force archive). I'll remove it
Ticket
Purpose
fix check manual restart approval
Changes
check_manual_restart_approvalafter force archive; remove it fromarchive_successcheck_manual_restart_approvalapprove_past_pendingsinstead ofprocess_manual_restart_approvalsto approve a single registrationSide Effects
QE Notes
CE Notes
Documentation