Always return a value from _update_or_create_domain_metrics

[nospame]

Otherwise it will TypeError on unpacking metrics, __ and fail the whole update_domain_metrics_for_domains task

Technical Summary

Just a bugfix for an in-development feature.

Safety Assurance

Safety story

We're not swallowing any additional errors here, just returning a tuple of something that can be unpacked. The caller, update_domain_metrics_for_domains already accounts for metrics potentially being None.

Automated test coverage

No tests for this particular return value.

QA Plan

No QA planned.

Rollback instructions

• This PR can be reverted after deploy with no further considerations

Labels & Review

• Risk label is set correctly
• The set of people pinged as reviewers is appropriate for the level of risk of the change

[orangejenny]

Nitpick: I'd move this out of the except block, it'd be a tiny bit more robust to future updates.

[nospame]

What updates are you thinking this would help with? I'm imagining maybe if the except block is changed in the future to catch a more specific exception?

[orangejenny]

I was imagining the return statement gets moved out of the try...although that would presumably mean that it gets moved below the whole try/except, so then you would still want the return None, False to be inside the except.

Really, it just visually catches my attention when a function doesn't end with a return statement on the left-most level of indentation as the function body, and it'll catch my attention again if I read/modify/review this code in the future. But then I read the function and see that it will always return something.

[nospame]

That makes sense. Would it be clearer/more conventional to always assign metrics, created (even if we're not using created in practice, that's still what the value is) and return those two values?

Rethought this comment immediately after writing it. Since we're never using created, this can just return the metrics, and update the the update_domain_metrics_for_domains to only expect a single value to be returned. Something like:

def update_domain_metrics_for_domains(domains):
    ...
    for domain in domains:
            metrics = _update_or_create_domain_metrics(domain, all_stats)
    ...


def _update_or_create_domain_metrics(domain, all_stats)
    try:
        ...
        metrics, __ = DomainMetrics.objects.update_or_create(
            defaults=metrics_dict,
            domain=domain_obj.name,
        )
    except Exception as e:
        notify_exception(
            None, message='Failed to create or update domain metrics for {domain}: {}'.format(e, domain=domain)
        )
        metrics = None

    return metrics

What do you think?