Missing return type declaration. This also should be added to other functions, but here it's especially important as sometimes self.id is returned. This is incompatible with the official docs for __str__, which say that a string should be returned.

This connects to a remote system and fetches the avatar. Such complex behaviour, with various different ways in which it can fail or cause slowdowns, shouldn't be hidden behind a "plain setter function" name.

Not all users may have an oauth_info.oauth_user_id. What happens when this is the case? How will the user be notified of this when they do try and set an avatar?

This function is doing too many things at once:

• Obtain a user's Blender ID account number.
• Construct an OAuth2 session object.
• Construct the URL that can be used to obtain the avatar.
• Figure out the avatar's filename.
• Download the avatar image.
• Store the avatar on the Profile.

It's probably better to have a separate class for this That class can then also manage the Requests session.

This will create a new Requests session for each call to this function. Fetching avatars can be quite a lot of work with thousands of users in the database. Since the request doesn't have to be authenticated with a user-specific token, it's better to have a dedicated requests.Session (or OAuth2Session) that manages the TCP/IP connection and TLS handshake, and allows re-use.

pathlib.Path shouldn't be used to parse URLs. Use urllib instead.

This module should have a docstring that explains what it's for. What kind of code is expected to call these functions?

This is an indication that this functionality should actually be in a custom model manager of OAuthUserInfo.

Logging 'User pk={instance.pk} ... will make it clear what the logged number is. Blender ID account numbers are also numerical.

Use oauth_info.get('full_name') or '' for safety. Also put the fact that 'full_name' is used in the docstring.

How are communication errors handled here? And disk I/O errors saving the avatar?

Add a docstring that explains what this function is for.

if not getattr(instance, 'profile', None) will cover both the case that it just isn't there, or the hypothetical case that the property is there but set to None.

Add type declarations.

Include non-ASCII characters in names.

Test the entire name. Now a bug that ignores the name and turns it into ".jpg" won't be detected.

Return type declaration & docstring are missing.

This function is doing too many things at once, better to split it up into a few smaller functions.

You're careful here with the length check, which is nice. An extra layer of carefulness could be added by using body = request.body[:content_length+1]. That way you read at most only a byte too many, avoiding DoS attacks.

If the secret is going to be encoded anyway, why not store it in the config file as bytes?

What if the response is something else?

What if there is already a user with that username?

What if there is already a user with that email address?

This is doing quite a few DB queries. It'll probably be simpler to get all the groups in the same query that fetches the user (with select_related()), then use sets to compute the differences.

This should document what happens when it fails. Does it automatically retry after a while? What will be returned when it fails?

Missing f string prefix.

Fair enough, but why then bother checking the avatar name at all?

A return type Dict[str, Any] would work here as well I think.

👍

This is of quadratic complexity (len(group_names) × len(current_groups)). How about something like this?

def set_groups(user: User, group_names: Set[str]) -> None:
    current_group_names = {group.name for group in current_groups}

    names_to_add_to = group_names - current_group_names
    groups_to_add_to = [Group.objects.get_or_create(name=group_name) for group_name in names_to_add_to]
    user.groups.add(*groups_to_add_to)

    names_to_remove_from = current_group_names - group_names
    groups_to_remove_from = [
        group := Group.objects.filter(name=group_name).first
        for group_name in names_to_remove_from
        if group is not None]
    user.groups.remove(*groups_to_remove_from)

Disclaimer: I haven't tested this code ;-)
I think that for computing which groups to add/remove, the set semantics makes things more readable. What do you think about such an approach?

There is a lot happening on this one line. It'll probably make debugging easier if it's split up into a few smaller lines.

Nicknames in Blender ID aren't limited to ASCII, so be sure to test with non-ASCII nicknames too.

Ok, fair enough.