Changeset View
Standalone View
profiles/models.py
- This file was added.
| from requests_oauthlib import OAuth2Session | |||||
| from django import urls | |||||
| from urllib.parse import urljoin | |||||
| from django.contrib.auth.models import User | |||||
| from django.db import models | |||||
| import io | |||||
| import pathlib | |||||
| from blender_id_oauth_client.views import blender_id_oauth_settings | |||||
| from common import mixins | |||||
| from common.upload_paths import get_upload_to_hashed_path | |||||
| class Profile(mixins.CreatedUpdatedMixin, models.Model): | |||||
| user = models.OneToOneField(User, on_delete=models.CASCADE, related_name='profile') | |||||
| full_name = models.CharField(max_length=255, blank=True, default='') | |||||
| avatar = models.ImageField(upload_to=get_upload_to_hashed_path, blank=True) | |||||
| def get_absolute_url(self): | |||||
| return urls.reverse('profile_detail', kwargs={'pk': self.pk}) | |||||
| def __str__(self): | |||||
| return self.full_name or (self.user.username if self.user else self.id) | |||||
sybren: Missing return type declaration. This also should be added to other functions, but here it's… | |||||
| def set_avatar(self): | |||||
| oauth_info = self.user.oauth_info | |||||
Done Inline ActionsThis 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. sybren: This connects to a remote system and fetches the avatar. Such complex behaviour, with various… | |||||
| oauth_user_id = oauth_info.oauth_user_id | |||||
Done Inline ActionsNot 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? sybren: Not all users may have an `oauth_info.oauth_user_id`. What happens when this is the case? How… | |||||
Done Inline ActionsFrom what I understand, there'll be no other way to set an avatar other than doing that in Blender ID. railla: From what I understand, there'll be no other way to set an avatar other than doing that in… | |||||
| bid_settings = blender_id_oauth_settings() | |||||
| bid = OAuth2Session(bid_settings.client) | |||||
Done Inline ActionsThis should document what happens when it fails. Does it automatically retry after a while? What will be returned when it fails? sybren: This should document what happens when it fails. Does it automatically retry after a while? | |||||
| resp = bid.get(urljoin(bid_settings.url_base, f'api/user/{oauth_user_id}/avatar')) | |||||
Done Inline ActionsThis 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. sybren: This will create a new Requests session for each call to this function. Fetching avatars can be… | |||||
| name = pathlib.Path(resp.url).name | |||||
Done Inline ActionsThis function is doing too many things at once:
It's probably better to have a separate class for this That class can then also manage the Requests session. sybren: This function is doing too many things at once:
- Obtain a user's Blender ID account number. | |||||
Done Inline ActionsMissing f string prefix. sybren: Missing `f` string prefix. | |||||
| self.avatar.save(name, io.BytesIO(resp.content), save=True) | |||||
Done Inline Actionspathlib.Path shouldn't be used to parse URLs. Use urllib instead. sybren: `pathlib.Path` shouldn't be used to parse URLs. Use `urllib` instead. | |||||
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.