[ENG-10737] Registrations are failing to auto-approve [privacy]#11718
[ENG-10737] Registrations are failing to auto-approve [privacy]#11718antkryt wants to merge 1 commit into
Conversation
cslzchen
left a comment
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().
|
|
||
| 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?
| 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().
3 participants
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